diff --git a/CHANGES.md b/CHANGES.md index edea44773..3fb8dd08a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -33,6 +33,28 @@ If you were previously disabling the image cleaner replace: - `imageCleaner.host.enabled: false` ➡️ `imageCleaner.enabled: false` +### `binderhub.build.Build` class replaced by `binderhub.build.KubernetesBuildExecutor` + +The `binderhub.build.Build` class is replaced by the Traitlets based `binderhub.build.KubernetesBuildExecutor` class +[#1518](https://github.com/jupyterhub/binderhub/pull/1518), +[#1521](https://github.com/jupyterhub/binderhub/pull/1521). + +The following build configuration properties should be set using Traitlets in the BinderHub configuration: + +- `c.BinderHub.appendix` ➡️ `c.BuildExecutor.appendix` +- `c.BinderHub.sticky_builds` ➡️ `c.KubernetesBuildExecutor.sticky_builds` +- `c.BinderHub.log_tail_lines` ➡️ `c.KubernetesBuildExecutor.log_tail_lines` +- `c.BinderHub.push_secret` ➡️ `c.BuildExecutor.push_secret` +- `c.BinderHub.build_memory_request` ➡️ `c.KubernetesBuildExecutor.memory_request` +- `c.BinderHub.build_memory_limit` ➡️ `c.BuildExecutor.memory_limit` +- `c.BinderHub.build_docker_host` ➡️ `c.KubernetesBuildExecutor.docker_host` +- `c.BinderHub.build_namespace` ➡️ `c.KubernetesBuildExecutor.namespace` +- `c.BinderHub.build_image` ➡️ `c.KubernetesBuildExecutor.build_image` +- `c.BinderHub.build_node_selector` ➡️ `c.KubernetesBuildExecutor.node_selector` + +If you have subclassed `binderhub.build.Build` you must update your subclass (including `__init__()` if defined) to inherit from `binderhub.build.KubernetesBuildExecutor`. +The behaviour of the class is otherwise unchanged. + # 0.2.0 # master@{2019-07-01}...master@{2019-10-01} diff --git a/binderhub/app.py b/binderhub/app.py index 263687571..c4e10aeb2 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -8,6 +8,7 @@ import os import re import secrets +import warnings from binascii import a2b_hex from concurrent.futures import ThreadPoolExecutor from glob import glob @@ -41,7 +42,7 @@ from traitlets.config import Application from .base import AboutHandler, Custom404, VersionHandler -from .build import Build, BuildExecutor, KubernetesBuildExecutor +from .build import BuildExecutor, KubernetesBuildExecutor, KubernetesCleaner from .builder import BuildHandler from .config import ConfigHandler from .events import EventLog @@ -229,6 +230,8 @@ def _valid_badge_base_url(self, proposal): appendix = Unicode( help=""" + DEPRECATED: Use c.BuildExecutor.appendix + Appendix to pass to repo2docker A multi-line string of Docker directives to run. @@ -248,6 +251,8 @@ def _valid_badge_base_url(self, proposal): sticky_builds = Bool( False, help=""" + DEPRECATED: Use c.KubernetesBuildExecutor.sticky_builds + Attempt to assign builds for the same repository to the same node. In order to speed up re-builds of a repository all its builds will @@ -270,7 +275,7 @@ def _valid_badge_base_url(self, proposal): ) build_class = Type( - Build, + KubernetesBuildExecutor, klass=BuildExecutor, help=""" The class used to build repo2docker images. @@ -280,6 +285,15 @@ def _valid_badge_base_url(self, proposal): config=True, ) + build_cleaner_class = Type( + KubernetesCleaner, + allow_none=True, + help=""" + The class used to cleanup builders. + """, + config=True, + ) + registry_class = Type( DockerRegistry, help=""" @@ -369,6 +383,8 @@ def _pod_quota_deprecated(self, change): log_tail_lines = Integer( 100, help=""" + DEPRECATED: Use c.KubernetesBuildExecutor.log_tail_lines + Limit number of log lines to show when connecting to an already running build. """, config=True, @@ -378,6 +394,8 @@ def _pod_quota_deprecated(self, change): "binder-build-docker-config", allow_none=True, help=""" + DEPRECATED: Use c.BuildExecutor.push_secret + A kubernetes secret object that provides credentials for pushing built images. """, config=True, @@ -401,6 +419,8 @@ def _pod_quota_deprecated(self, change): build_memory_request = ByteSpecification( 0, help=""" + DEPRECATED: Use c.KubernetesBuildExecutor.memory_request + Amount of memory to request when scheduling a build 0 reserves no memory. @@ -416,6 +436,8 @@ def _pod_quota_deprecated(self, change): build_memory_limit = ByteSpecification( 0, help=""" + DEPRECATED: Use c.BuildExecutor.memory_limit + Max amount of memory allocated for each image build process. 0 sets no limit. @@ -440,6 +462,8 @@ def _pod_quota_deprecated(self, change): "/var/run/docker.sock", config=True, help=""" + DEPRECATED: Use c.KubernetesBuildExecutor.docker_host + The docker URL repo2docker should use to build the images. Currently, only paths are supported, and they are expected to be available on @@ -518,6 +542,8 @@ def _add_slash(self, proposal): build_namespace = Unicode( help=""" + DEPRECATED: Use c.KubernetesBuildExecutor.namespace + Kubernetes namespace to spawn build pods in. Note that the push_secret must refer to a secret in this namespace. @@ -532,6 +558,8 @@ def _default_build_namespace(self): build_image = Unicode( "quay.io/jupyterhub/repo2docker:2022.10.0", help=""" + DEPRECATED: Use c.KubernetesBuildExecutor.build_image + The repo2docker image to be used for doing builds """, config=True, @@ -541,6 +569,8 @@ def _default_build_namespace(self): {}, config=True, help=""" + DEPRECATED: Use c.KubernetesBuildExecutor.node_selector + Select the node where build pod runs on. """, ) @@ -737,6 +767,27 @@ def _template_path_default(self): help="Origin to use when emitting events. Defaults to hostname of request when empty", ) + _build_config_deprecated_map = { + "appendix": ("BuildExecutor", "appendix"), + "push_secret": ("BuildExecutor", "push_secret"), + "build_memory_limit": ("BuildExecutor", "memory_limit"), + "sticky_builds": ("KubernetesBuildExecutor", "sticky_builds"), + "log_tail_lines": ("KubernetesBuildExecutor", "log_tail_lines"), + "build_memory_request": ("KubernetesBuildExecutor", "memory_request"), + "build_docker_host": ("KubernetesBuildExecutor", "docker_host"), + "build_namespace": ("KubernetesBuildExecutor", "namespace"), + "build_image": ("KubernetesBuildExecutor", "build_image"), + "build_node_selector": ("KubernetesBuildExecutor", "node_selector"), + } + + @observe(*_build_config_deprecated_map) + def _build_config_deprecated(self, change): + dest_cls, dest_name = self._build_config_deprecated_map[change.name] + self.log.warning( + "BinderHub.%s is deprecated, use %s.%s", change.name, dest_cls, dest_name + ) + self.config[dest_cls][dest_name] = change.new + @staticmethod def add_url_prefix(prefix, handlers): """add a url prefix to handlers""" @@ -830,25 +881,22 @@ def initialize(self, *args, **kwargs): launch_quota = self.launch_quota_class(parent=self, executor=self.executor) + # Construct a Builder so that we can extract parameters such as the + # configuration or the version string to pass to /version and /health handlers + example_builder = self.build_class(parent=self) self.tornado_settings.update( { "log_function": log_request, - "push_secret": self.push_secret, "image_prefix": self.image_prefix, "debug": self.debug, "launcher": self.launcher, - "appendix": self.appendix, "ban_networks": self.ban_networks, "ban_networks_min_prefix_len": self.ban_networks_min_prefix_len, - "build_namespace": self.build_namespace, - "build_image": self.build_image, - "build_node_selector": self.build_node_selector, "build_pool": self.build_pool, "build_token_check_origin": self.build_token_check_origin, "build_token_secret": self.build_token_secret, "build_token_expires_seconds": self.build_token_expires_seconds, - "sticky_builds": self.sticky_builds, - "log_tail_lines": self.log_tail_lines, + "example_builder": example_builder, "pod_quota": self.pod_quota, "per_repo_quota": self.per_repo_quota, "per_repo_quota_higher": self.per_repo_quota_higher, @@ -866,9 +914,6 @@ def initialize(self, *args, **kwargs): "banner_message": self.banner_message, "extra_footer_scripts": self.extra_footer_scripts, "jinja2_env": jinja_env, - "build_memory_limit": self.build_memory_limit, - "build_memory_request": self.build_memory_request, - "build_docker_host": self.build_docker_host, "build_docker_config": self.build_docker_config, "base_url": self.base_url, "badge_base_url": self.badge_base_url, @@ -969,25 +1014,21 @@ def stop(self): self.build_pool.shutdown() async def watch_build_pods(self): - """Watch build pods + warnings.warn( + "watch_build_pods() is deprecated, use watch_builders()", DeprecationWarning + ) + await self.watch_builders() - Every build_cleanup_interval: - - delete stopped build pods - - delete running build pods older than build_max_age + async def watch_builders(self): + """ + Watch builders, run a cleanup function every build_cleanup_interval """ - while True: + while self.build_cleaner_class: + cleaner = self.build_cleaner_class() try: - await asyncio.wrap_future( - self.executor.submit( - lambda: Build.cleanup_builds( - self.kube_client, - self.build_namespace, - self.build_max_age, - ) - ) - ) + await asyncio.wrap_future(self.executor.submit(cleaner.cleanup)) except Exception: - app_log.exception("Failed to cleanup build pods") + app_log.exception("Failed to cleanup builders") await asyncio.sleep(self.build_cleanup_interval) def start(self, run_loop=True): @@ -998,7 +1039,7 @@ def start(self, run_loop=True): ) self.http_server.listen(self.port) if self.builder_required: - asyncio.ensure_future(self.watch_build_pods()) + asyncio.ensure_future(self.watch_builders()) if run_loop: tornado.ioloop.IOLoop.current().start() diff --git a/binderhub/base.py b/binderhub/base.py index bff76b55e..33474fcce 100644 --- a/binderhub/base.py +++ b/binderhub/base.py @@ -244,11 +244,11 @@ class VersionHandler(BaseHandler): async def get(self): self.set_header("Content-type", "application/json") - self.write( - json.dumps( - { - "builder": self.settings["build_image"], - "binderhub": binder_version, - } - ) - ) + r = { + "builder_info": self.settings["example_builder"].builder_info, + "binderhub": binder_version, + } + # Backwards compatibility + if "build_image" in r["builder_info"]: + r["builder"] = r["builder_info"]["build_image"] + self.write(json.dumps(r)) diff --git a/binderhub/build.py b/binderhub/build.py index 6a4e851e6..5e0234f0d 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -77,7 +77,6 @@ class BuildExecutor(LoggingConfigurable): git_credentials = Unicode( "", - allow_none=True, help=( "Git credentials to use when cloning the repository, passed via the GIT_CREDENTIAL_ENV environment variable." "Can be anything that will be accepted by git as a valid output from a git-credential helper. " @@ -88,7 +87,6 @@ class BuildExecutor(LoggingConfigurable): push_secret = Unicode( "", - allow_none=True, help="Implementation dependent secret for pushing image to a registry.", config=True, ) @@ -103,6 +101,14 @@ class BuildExecutor(LoggingConfigurable): config=True, ) + builder_info = Dict( + help=( + "Metadata about the builder e.g. repo2docker version. " + "This is included in the BinderHub version endpoint" + ), + config=True, + ) + def __init__(self, **kwargs): super().__init__(**kwargs) self.main_loop = IOLoop.current() @@ -220,6 +226,13 @@ def _default_api(self): kubernetes.config.load_kube_config() return client.CoreV1Api() + # Overrides the default for BuildExecutor + push_secret = Unicode( + "binder-build-docker-config", + help="Implementation dependent secret for pushing image to a registry.", + config=True, + ) + namespace = Unicode( help="Kubernetes namespace to spawn build pods into", config=True ) @@ -234,6 +247,10 @@ def _default_namespace(self): config=True, ) + @default("builder_info") + def _default_builder_info(self): + return {"build_image": self.build_image} + docker_host = Unicode( "/var/run/docker.sock", help=( @@ -732,89 +749,3 @@ def stream_logs(self): } ), ) - - -class Build(KubernetesBuildExecutor): - """DEPRECATED: Use KubernetesBuildExecutor and configure with Traitlets - - Represents a build of a git repository into a docker image. - - This ultimately maps to a single pod on a kubernetes cluster. Many - different build objects can point to this single pod and perform - operations on the pod. The code in this class needs to be careful and take - this into account. - - For example, operations a Build object tries might not succeed because - another Build object pointing to the same pod might have done something - else. This should be handled gracefully, and the build object should - reflect the state of the pod as quickly as possible. - - ``name`` - The ``name`` should be unique and immutable since it is used to - sync to the pod. The ``name`` should be unique for a - ``(repo_url, ref)`` tuple, and the same tuple should correspond - to the same ``name``. This allows use of the locking provided by k8s - API instead of having to invent our own locking code. - - """ - - """ - For backwards compatibility: BinderHub previously assumed a single cleaner for all builds - """ - _cleaners = {} - - def __init__( - self, - q, - api, - name, - *, - namespace, - repo_url, - ref, - build_image, - docker_host, - image_name, - git_credentials=None, - push_secret=None, - memory_limit=0, - memory_request=0, - node_selector=None, - appendix="", - log_tail_lines=100, - sticky_builds=False, - ): - warnings.warn( - "Class Build is deprecated, use KubernetesBuildExecutor and configure with Traitlets", - DeprecationWarning, - ) - - super().__init__() - - self.q = q - self.api = api - self.repo_url = repo_url - self.ref = ref - self.name = name - self.namespace = namespace - self.image_name = image_name - self.push_secret = push_secret - self.build_image = build_image - self.memory_limit = memory_limit - self.memory_request = memory_request - self.docker_host = docker_host - self.node_selector = node_selector - self.appendix = appendix - self.log_tail_lines = log_tail_lines - self.git_credentials = git_credentials - self.sticky_builds = sticky_builds - - @classmethod - def cleanup_builds(cls, kube, namespace, max_age): - """Delete stopped build pods and build pods that have aged out""" - key = (kube, namespace, max_age) - if key not in Build._cleaners: - Build._cleaners[key] = KubernetesCleaner( - kube=kube, namespace=namespace, max_age=max_age - ) - Build._cleaners[key].cleanup() diff --git a/binderhub/build_local.py b/binderhub/build_local.py index eb958367e..ddb8fca67 100644 --- a/binderhub/build_local.py +++ b/binderhub/build_local.py @@ -11,6 +11,7 @@ from threading import Thread from tornado.log import app_log +from traitlets import default from .build import BuildExecutor, ProgressEvent @@ -111,6 +112,16 @@ class LocalRepo2dockerBuild(BuildExecutor): WARNING: This is still under development. Breaking changes may be made at any time. """ + @default("builder_info") + def _default_builder_info(self): + try: + import repo2docker + + return {"repo2docker-version": repo2docker.__version__} + except ImportError: + self.log.error("repo2docker not installed") + return {} + def submit(self): """ Run a build to create the image for the repository. diff --git a/binderhub/builder.py b/binderhub/builder.py index de17837be..874f71616 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -22,7 +22,7 @@ from tornado.web import Finish, authenticated from .base import BaseHandler -from .build import Build, ProgressEvent +from .build import ProgressEvent from .quota import LaunchQuotaExceeded # Separate buckets for builds and launches. @@ -437,64 +437,21 @@ async def get(self, provider_prefix, _unescaped_spec): # Prepare to build q = Queue() - if self.settings["use_registry"] or self.settings["build_docker_config"]: - push_secret = self.settings["push_secret"] - else: - push_secret = None - BuildClass = self.settings.get("build_class") - appendix = self.settings["appendix"].format( - binder_url=self.binder_launch_host + self.binder_request, - persistent_binder_url=self.binder_launch_host - + self.binder_persistent_request, + build = BuildClass( + # All other properties should be set in traitlets config + parent=self.settings["traitlets_parent"], + q=q, + name=build_name, repo_url=repo_url, - ref_url=self.ref_url, + ref=ref, + image_name=image_name, + git_credentials=provider.git_credentials, ) + if not self.settings["use_registry"]: + build.push_secret = "" - if issubclass(BuildClass, Build): - # Deprecated, see docstring of the Build class for more details - build = BuildClass( - q=q, - # api object can be None if we are using FakeBuild - api=self.settings.get("kubernetes_client"), - name=build_name, - namespace=self.settings["build_namespace"], - repo_url=repo_url, - ref=ref, - image_name=image_name, - push_secret=push_secret, - build_image=self.settings["build_image"], - memory_limit=self.settings["build_memory_limit"], - memory_request=self.settings["build_memory_request"], - docker_host=self.settings["build_docker_host"], - node_selector=self.settings["build_node_selector"], - appendix=appendix, - log_tail_lines=self.settings["log_tail_lines"], - git_credentials=provider.git_credentials, - sticky_builds=self.settings["sticky_builds"], - ) - else: - build = BuildClass( - # Commented properties should be set in traitlets config - parent=self.settings["traitlets_parent"], - q=q, - name=build_name, - # namespace=self.settings["build_namespace"], - repo_url=repo_url, - ref=ref, - image_name=image_name, - # push_secret=push_secret, - # build_image=self.settings["build_image"], - # memory_limit=self.settings["build_memory_limit"], - # memory_request=self.settings["build_memory_request"], - # docker_host=self.settings["build_docker_host"], - # node_selector=self.settings["build_node_selector"], - # appendix=appendix, - # log_tail_lines=self.settings["log_tail_lines"], - git_credentials=provider.git_credentials, - # sticky_builds=self.settings["sticky_builds"], - ) self.build = build with BUILDS_INPROGRESS.track_inprogress(): diff --git a/binderhub/health.py b/binderhub/health.py index f862b6bcb..8b738028a 100644 --- a/binderhub/health.py +++ b/binderhub/health.py @@ -183,8 +183,8 @@ class KubernetesHealthHandler(HealthHandler): @at_most_every async def _get_pods(self): """Get information about build and user pods""" - namespace = self.settings["build_namespace"] - k8s = self.settings["kubernetes_client"] + namespace = self.settings["example_builder"].namespace + k8s = self.settings["example_builder"].api pool = self.settings["executor"] app_log.info(f"Getting pod statistics for {namespace}") diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index ee3a832c2..e2ef2fba3 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -13,7 +13,7 @@ from tornado.httputil import url_concat from tornado.queues import Queue -from binderhub.build import Build, KubernetesBuildExecutor, ProgressEvent +from binderhub.build import KubernetesBuildExecutor, ProgressEvent from binderhub.build_local import LocalRepo2dockerBuild, ProcessTerminated, _execute_cmd from .utils import async_requests @@ -143,8 +143,8 @@ def test_default_affinity(): mock_k8s_api = _list_image_builder_pods_mock() - build = Build( - mock.MagicMock(), + build = KubernetesBuildExecutor( + q=mock.MagicMock(), api=mock_k8s_api, name="test_build", namespace="build_namespace", @@ -154,7 +154,7 @@ def test_default_affinity(): image_name="name", push_secret="", memory_limit=0, - git_credentials=None, + git_credentials="", docker_host="http://mydockerregistry.local", node_selector={}, ) @@ -171,8 +171,8 @@ def test_sticky_builds_affinity(): # Setup some mock objects for the response from the k8s API mock_k8s_api = _list_image_builder_pods_mock() - build = Build( - mock.MagicMock(), + build = KubernetesBuildExecutor( + q=mock.MagicMock(), api=mock_k8s_api, name="test_build", namespace="build_namespace", @@ -182,7 +182,7 @@ def test_sticky_builds_affinity(): image_name="name", push_secret="", memory_limit=0, - git_credentials=None, + git_credentials="", docker_host="http://mydockerregistry.local", node_selector={}, sticky_builds=True, @@ -209,8 +209,8 @@ def test_git_credentials_passed_to_podspec_upon_submit(): mock_k8s_api = _list_image_builder_pods_mock() - build = Build( - mock.MagicMock(), + build = KubernetesBuildExecutor( + q=mock.MagicMock(), api=mock_k8s_api, name="test_build", namespace="build_namespace", diff --git a/binderhub/tests/test_main.py b/binderhub/tests/test_main.py index 0a9865dfe..b19761d08 100644 --- a/binderhub/tests/test_main.py +++ b/binderhub/tests/test_main.py @@ -108,7 +108,16 @@ async def test_versions_handler(app): assert r.status_code == 200 data = r.json() - assert data["builder"] == app.build_image + # builder_info is different for KubernetesExecutor and LocalRepo2dockerBuild + try: + import repo2docker + + allowed_builder_info = [{"repo2docker-version": repo2docker.__version__}] + except ImportError: + allowed_builder_info = [] + allowed_builder_info.append({"build_image": app.build_image}) + + assert data["builder_info"] in allowed_builder_info assert data["binderhub"].split("+")[0] == binder_version.split("+")[0] diff --git a/helm-chart/binderhub/values.yaml b/helm-chart/binderhub/values.yaml index 6c6495f13..7e26ae05e 100644 --- a/helm-chart/binderhub/values.yaml +++ b/helm-chart/binderhub/values.yaml @@ -48,6 +48,7 @@ config: # hub_url: # hub_url_local: use_registry: true + KubernetesBuildExecutor: {} extraConfig: {} diff --git a/testing/local-binder-local-hub/binderhub_config.py b/testing/local-binder-local-hub/binderhub_config.py index da6bd2770..2eae2c466 100644 --- a/testing/local-binder-local-hub/binderhub_config.py +++ b/testing/local-binder-local-hub/binderhub_config.py @@ -24,7 +24,7 @@ c.BinderHub.builder_required = False c.BinderHub.build_class = LocalRepo2dockerBuild -c.BinderHub.push_secret = None +c.BinderHub.push_secret = "" c.BinderHub.launch_quota_class = LaunchQuota c.BinderHub.about_message = "This is a local dev deployment without Kubernetes"