From d04e88eb9e31ee01b710524259862daf0c438bbf Mon Sep 17 00:00:00 2001 From: Simon Li Date: Thu, 28 Jul 2022 23:33:02 +0100 Subject: [PATCH 01/18] Replace `Build` class with new traitlets classes `BuildExecutor` is the abstract class with common prperties and methods `KubernetesBuildExecutor` is the replacement for the current `Build` class `Build` is kept but deprecated. `LocalRepo2dockerBuild` has never been released and is marked as under development, so breaking changes are fine. --- binderhub/build.py | 386 ++++++++++++++++++++++------------ binderhub/build_local.py | 114 +--------- binderhub/tests/test_build.py | 16 +- 3 files changed, 260 insertions(+), 256 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index 6baa09d25..d4b011115 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -11,9 +11,12 @@ from typing import Union from urllib.parse import urlparse +import kubernetes.config from kubernetes import client, watch from tornado.ioloop import IOLoop from tornado.log import app_log +from traitlets import Any, Bool, Dict, Integer, Unicode, default +from traitlets.config import LoggingConfigurable from .utils import KUBE_REQUEST_TIMEOUT, rendezvous_rank @@ -49,133 +52,66 @@ def __init__(self, kind: Kind, payload: Union[str, BuildStatus]): self.payload = payload -class Build: - """Represents a build of a git repository into a docker image. +class BuildExecutor(LoggingConfigurable): + """Base class for a build of a version controlled repository to a self-contained + environment + """ - 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. + q = Any( + allow_none=True, + help="Queue that receives progress events after the build has been submitted", + ) - 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 = Unicode( + help=( + "A unique name for the thing (repo, ref) being built." + "Used to coalesce builds, make sure they are not being unnecessarily repeated." + ), + ) - ``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. + repo_url = Unicode(help="URL of repository to build.") - """ + ref = Unicode(help="Ref of repository to build.") - 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, - ): - """ - Parameters - ---------- - - q : tornado.queues.Queue - Queue that receives progress events after the build has been submitted - api : kubernetes.client.CoreV1Api() - Api object to make kubernetes requests via - name : str - A unique name for the thing (repo, ref) being built. Used to coalesce - builds, make sure they are not being unnecessarily repeated - namespace : str - Kubernetes namespace to spawn build pods into - repo_url : str - URL of repository to build. - Passed through to repo2docker. - ref : str - Ref of repository to build - Passed through to repo2docker. - build_image : str - Docker image containing repo2docker that is used to spawn the build - pods. - docker_host : str - The docker socket to use for building the image. - Must be a unix domain socket on a filesystem path accessible on the - node in which the build pod is running. - image_name : str - Full name of the image to build. Includes the tag. - Passed through to repo2docker. - git_credentials : str - Git credentials to use to clone private repositories. Passed - through to repo2docker 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. See - https://git-scm.com/docs/gitcredentials for more information. - push_secret : str - Kubernetes secret containing credentials to push docker image to registry. - memory_limit - Memory limit for the docker build process. Can be an integer in - bytes, or a byte specification (like 6M). - Passed through to repo2docker. - memory_request - Memory request of the build pod. The actual building happens in the - docker daemon, but setting request in the build pod makes sure that - memory is reserved for the docker build in the node by the kubernetes - scheduler. - node_selector : dict - Node selector for the kubernetes build pod. - appendix : str - Appendix to be added at the end of the Dockerfile used by repo2docker. - Passed through to repo2docker. - log_tail_lines : int - Number of log lines to fetch from a currently running build. - If a build with the same name is already running when submitted, - only the last `log_tail_lines` number of lines will be fetched and - displayed to the end user. If not, all log lines will be streamed. - sticky_builds : bool - If true, builds for the same repo (but different refs) will try to - schedule on the same node, to reuse cache layers in the docker daemon - being used. - """ - 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.main_loop = IOLoop.current() - 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 + image_name = Unicode(help="Full name of the image to build. Includes the tag.") - self.stop_event = threading.Event() - self.git_credentials = git_credentials + git_credentials = Any( + 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. " + "See https://git-scm.com/docs/gitcredentials for more information." + ), + config=True, + ) - self.sticky_builds = sticky_builds + push_secret = Unicode( + allow_none=True, + help="Implementation dependent name of a secret for pushing image to a registry.", + config=True, + ) + + memory_limit = Integer( + 0, help="Memory limit for the build process in bytes", config=True + ) + + appendix = Unicode( + allow_none=True, + help="Appendix to be added at the end of the Dockerfile used by repo2docker.", + config=True, + ) + + main_loop = Any() - self._component_label = "binderhub-build" + @default("main_loop") + def _default_main_loop(self): + return IOLoop.current() + + stop_event = Any() + + @default("stop_event") + def _default_stop_event(self): + return threading.Event() def get_r2d_cmd_options(self): """Get options/flags for repo2docker""" @@ -213,6 +149,131 @@ def get_cmd(self): return cmd + def progress(self, kind: ProgressEvent.Kind, payload: str): + """ + Put current progress info into the queue on the main thread + """ + self.main_loop.add_callback(self.q.put, ProgressEvent(kind, payload)) + + def submit(self): + """ + Run a build to create the image for the repository. + + Progress of the build can be monitored by listening for items in + the Queue passed to the constructor as `q`. + """ + raise NotImplementedError() + + def stream_logs(self): + """ + Stream build logs to the queue in self.q + """ + pass + + def cleanup(self): + """ + Stream build logs to the queue in self.q + """ + pass + + def stop(self): + """ + Stop watching progress of build + + TODO: What exactly is the purpose of this method? + """ + self.stop_event.set() + + +class KubernetesBuildExecutor(BuildExecutor): + """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. + + """ + + api = Any( + help="Kubernetes API object to make requests (kubernetes.client.CoreV1Api())", + ) + + @default("api") + def _default_api(self): + try: + kubernetes.config.load_incluster_config() + except kubernetes.config.ConfigException: + kubernetes.config.load_kube_config() + return client.CoreV1Api() + + namespace = Unicode( + help="Kubernetes namespace to spawn build pods into", config=True + ) + + build_image = Unicode( + help="Docker image containing repo2docker that is used to spawn the build pods.", + config=True, + ) + + docker_host = Unicode( + help=( + "The docker socket to use for building the image. " + "Must be a unix domain socket on a filesystem path accessible on the node " + "in which the build pod is running." + ), + config=True, + ) + + memory_request = Integer( + 0, + help=( + "Memory request of the build pod. " + "The actual building happens in the docker daemon, " + "but setting request in the build pod makes sure that memory is reserved for the docker build " + "in the node by the kubernetes scheduler." + ), + config=True, + ) + + node_selector = Dict( + allow_none=True, help="Node selector for the kubernetes build pod.", config=True + ) + + log_tail_lines = Integer( + 100, + help=( + "Number of log lines to fetch from a currently running build. " + "If a build with the same name is already running when submitted, " + "only the last `log_tail_lines` number of lines will be fetched and displayed to the end user. " + "If not, all log lines will be streamed." + ), + config=True, + ) + + sticky_builds = Bool( + False, + help=( + "If true, builds for the same repo (but different refs) will try to schedule on the same node, " + "to reuse cache layers in the docker daemon being used." + ), + config=True, + ) + + _component_label = Unicode("binderhub-build") + @classmethod def cleanup_builds(cls, kube, namespace, max_age): """Delete stopped build pods and build pods that have aged out""" @@ -273,12 +334,6 @@ def cleanup_builds(cls, kube, namespace, max_age): "Build phase summary: %s", json.dumps(phases, sort_keys=True, indent=1) ) - def progress(self, kind: ProgressEvent.Kind, payload: str): - """ - Put current progress info into the queue on the main thread - """ - self.main_loop.add_callback(self.q.put, ProgressEvent(kind, payload)) - def get_affinity(self): """Determine the affinity term for the build pod. @@ -569,16 +624,10 @@ def cleanup(self): else: raise - def stop(self): - """ - Stop wathcing for progress of build. - """ - self.stop_event.set() - -class FakeBuild(Build): +class FakeBuild(BuildExecutor): """ - Fake Building process to be able to work on the UI without a running Minikube. + Fake Building process to be able to work on the UI without a builder. """ def submit(self): @@ -630,3 +679,74 @@ 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. + + """ + + 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 diff --git a/binderhub/build_local.py b/binderhub/build_local.py index b356e4138..f840acb11 100644 --- a/binderhub/build_local.py +++ b/binderhub/build_local.py @@ -8,12 +8,11 @@ # These methods are synchronous so don't use tornado.queue import queue import subprocess -from threading import Event, Thread +from threading import Thread -from tornado.ioloop import IOLoop from tornado.log import app_log -from .build import Build, ProgressEvent +from .build import BuildExecutor, ProgressEvent DEFAULT_READ_TIMEOUT = 1 @@ -104,7 +103,7 @@ def read_to_queue(proc, capture, q): raise subprocess.CalledProcessError(proc.returncode, cmd) -class LocalRepo2dockerBuild(Build): +class LocalRepo2dockerBuild(BuildExecutor): """Represents a build of a git repository into a docker image. This runs a build using the repo2docker command line tool. @@ -112,104 +111,6 @@ class LocalRepo2dockerBuild(Build): WARNING: This is still under development. Breaking changes may be made at any time. """ - 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, - ): - """ - Parameters - ---------- - - q : tornado.queues.Queue - Queue that receives progress events after the build has been submitted - api : ignored - name : str - A unique name for the thing (repo, ref) being built. Used to coalesce - builds, make sure they are not being unnecessarily repeated - namespace : ignored - repo_url : str - URL of repository to build. - Passed through to repo2docker. - ref : str - Ref of repository to build - Passed through to repo2docker. - build_image : ignored - docker_host : ignored - image_name : str - Full name of the image to build. Includes the tag. - Passed through to repo2docker. - git_credentials : str - Git credentials to use to clone private repositories. Passed - through to repo2docker 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. See - https://git-scm.com/docs/gitcredentials for more information. - push_secret : ignored - memory_limit - Memory limit for the docker build process. Can be an integer in - bytes, or a byte specification (like 6M). - Passed through to repo2docker. - memory_request - Memory request of the build pod. The actual building happens in the - docker daemon, but setting request in the build pod makes sure that - memory is reserved for the docker build in the node by the kubernetes - scheduler. - node_selector : ignored - appendix : str - Appendix to be added at the end of the Dockerfile used by repo2docker. - Passed through to repo2docker. - log_tail_lines : int - Number of log lines to fetch from a currently running build. - If a build with the same name is already running when submitted, - only the last `log_tail_lines` number of lines will be fetched and - displayed to the end user. If not, all log lines will be streamed. - sticky_builds : ignored - """ - self.q = q - self.repo_url = repo_url - self.ref = ref - self.name = name - self.image_name = image_name - self.push_secret = push_secret - self.main_loop = IOLoop.current() - self.memory_limit = memory_limit - self.memory_request = memory_request - self.appendix = appendix - self.log_tail_lines = log_tail_lines - - self.stop_event = Event() - self.git_credentials = git_credentials - - @classmethod - def cleanup_builds(cls, kube, namespace, max_age): - app_log.debug("Not implemented") - - def progress(self, kind: ProgressEvent.Kind, payload: str): - """ - Put current progress info into the queue on the main thread - """ - self.main_loop.add_callback(self.q.put, ProgressEvent(kind, payload)) - - def get_affinity(self): - raise NotImplementedError() - def submit(self): """ Run a build to create the image for the repository. @@ -266,12 +167,3 @@ def _handle_log(self, line): } ) self.progress(ProgressEvent.Kind.LOG_MESSAGE, line) - - def stream_logs(self): - pass - - def cleanup(self): - pass - - def stop(self): - self.stop_event.set() diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 2f9477ef3..bb5b32272 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -222,14 +222,10 @@ async def test_local_repo2docker_build(): name = str(uuid4()) build = LocalRepo2dockerBuild( - q, - None, - name, - namespace=None, + q=q, + name=name, repo_url=repo_url, ref=ref, - build_image=None, - docker_host=None, image_name=name, ) build.submit() @@ -259,14 +255,10 @@ async def test_local_repo2docker_build_stop(event_loop): name = str(uuid4()) build = LocalRepo2dockerBuild( - q, - None, - name, - namespace=None, + q=q, + name=name, repo_url=repo_url, ref=ref, - build_image=None, - docker_host=None, image_name=name, ) event_loop.run_in_executor(None, build.submit) From 6e65ec4fad3ed61d8751a64012ba72514e3f9712 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Thu, 28 Jul 2022 23:44:19 +0100 Subject: [PATCH 02/18] `Build.cleanup_builds` is now a standalone traitlets-based class --- binderhub/build.py | 155 +++++++++++++++++++++++++++------------------ 1 file changed, 95 insertions(+), 60 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index d4b011115..649a4c950 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -274,66 +274,6 @@ def _default_api(self): _component_label = Unicode("binderhub-build") - @classmethod - def cleanup_builds(cls, kube, namespace, max_age): - """Delete stopped build pods and build pods that have aged out""" - builds = kube.list_namespaced_pod( - namespace=namespace, - label_selector="component=binderhub-build", - ).items - phases = defaultdict(int) - app_log.debug("%i build pods", len(builds)) - now = datetime.datetime.now(tz=datetime.timezone.utc) - start_cutoff = now - datetime.timedelta(seconds=max_age) - deleted = 0 - for build in builds: - phase = build.status.phase - phases[phase] += 1 - annotations = build.metadata.annotations or {} - repo = annotations.get("binder-repo", "unknown") - delete = False - if build.status.phase in {"Failed", "Succeeded", "Evicted"}: - # log Deleting Failed build build-image-... - # print(build.metadata) - app_log.info( - "Deleting %s build %s (repo=%s)", - build.status.phase, - build.metadata.name, - repo, - ) - delete = True - else: - # check age - started = build.status.start_time - if max_age and started and started < start_cutoff: - app_log.info( - "Deleting long-running build %s (repo=%s)", - build.metadata.name, - repo, - ) - delete = True - - if delete: - deleted += 1 - try: - kube.delete_namespaced_pod( - name=build.metadata.name, - namespace=namespace, - body=client.V1DeleteOptions(grace_period_seconds=0), - ) - except client.rest.ApiException as e: - if e.status == 404: - # Is ok, someone else has already deleted it - pass - else: - raise - - if deleted: - app_log.info("Deleted %i/%i build pods", deleted, len(builds)) - app_log.debug( - "Build phase summary: %s", json.dumps(phases, sort_keys=True, indent=1) - ) - def get_affinity(self): """Determine the affinity term for the build pod. @@ -625,6 +565,86 @@ def cleanup(self): raise +class KubernetesCleaner(LoggingConfigurable): + """Regular cleanup utility for kubernetes builds + + Instantiate this class, and call cleanup() periodically. + """ + + kube = Any(help="kubernetes API client") + + @default("kube") + def _default_kube(self): + try: + kubernetes.config.load_incluster_config() + except kubernetes.config.ConfigException: + kubernetes.config.load_kube_config() + return client.CoreV1Api() + + namespace = Unicode(help="Kubernetes namespace", config=True) + + max_age = Integer(help="Maximum age of build pods to keep", config=True) + + def cleanup(self): + """Delete stopped build pods and build pods that have aged out""" + builds = self.kube.list_namespaced_pod( + namespace=self.namespace, + label_selector="component=binderhub-build", + ).items + phases = defaultdict(int) + app_log.debug("%i build pods", len(builds)) + now = datetime.datetime.now(tz=datetime.timezone.utc) + start_cutoff = now - datetime.timedelta(seconds=self.max_age) + deleted = 0 + for build in builds: + phase = build.status.phase + phases[phase] += 1 + annotations = build.metadata.annotations or {} + repo = annotations.get("binder-repo", "unknown") + delete = False + if build.status.phase in {"Failed", "Succeeded", "Evicted"}: + # log Deleting Failed build build-image-... + # print(build.metadata) + app_log.info( + "Deleting %s build %s (repo=%s)", + build.status.phase, + build.metadata.name, + repo, + ) + delete = True + else: + # check age + started = build.status.start_time + if self.max_age and started and started < start_cutoff: + app_log.info( + "Deleting long-running build %s (repo=%s)", + build.metadata.name, + repo, + ) + delete = True + + if delete: + deleted += 1 + try: + self.kube.delete_namespaced_pod( + name=build.metadata.name, + namespace=self.namespace, + body=client.V1DeleteOptions(grace_period_seconds=0), + ) + except client.rest.ApiException as e: + if e.status == 404: + # Is ok, someone else has already deleted it + pass + else: + raise + + if deleted: + app_log.info("Deleted %i/%i build pods", deleted, len(builds)) + app_log.debug( + "Build phase summary: %s", json.dumps(phases, sort_keys=True, indent=1) + ) + + class FakeBuild(BuildExecutor): """ Fake Building process to be able to work on the UI without a builder. @@ -705,6 +725,11 @@ class Build(KubernetesBuildExecutor): """ + """ + For backwards compatibility: BinderHub previously assumed a single cleaner for all builds + """ + _cleaners = {} + def __init__( self, q, @@ -750,3 +775,13 @@ def __init__( 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() From 5c6874163d3f4aab5684e9bc2a2dbc916aba7a6c Mon Sep 17 00:00:00 2001 From: Simon Li Date: Sat, 30 Jul 2022 19:00:49 +0100 Subject: [PATCH 03/18] Use traitlets BuildExecutor in app --- binderhub/app.py | 6 ++-- binderhub/builder.py | 66 ++++++++++++++++++++++++++++++-------------- 2 files changed, 49 insertions(+), 23 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index 47198e60c..ae57543a2 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -41,7 +41,7 @@ from traitlets.config import Application from .base import AboutHandler, Custom404, VersionHandler -from .build import Build +from .build import Build, BuildExecutor from .builder import BuildHandler from .config import ConfigHandler from .events import EventLog @@ -270,10 +270,11 @@ def _valid_badge_base_url(self, proposal): build_class = Type( Build, + klass=BuildExecutor, help=""" The class used to build repo2docker images. - Must inherit from binderhub.build.Build + Must inherit from binderhub.build.BuildExecutor """, config=True, ) @@ -818,6 +819,7 @@ def initialize(self, *args, **kwargs): "build_class": self.build_class, "registry": registry, "traitlets_config": self.config, + "traitlets_parent": self, "google_analytics_code": self.google_analytics_code, "google_analytics_domain": self.google_analytics_domain, "about_message": self.about_message, diff --git a/binderhub/builder.py b/binderhub/builder.py index 09dc3cb61..b3a178a56 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 ProgressEvent +from .build import Build, ProgressEvent from .utils import KUBE_REQUEST_TIMEOUT # Separate buckets for builds and launches. @@ -425,26 +425,50 @@ async def get(self, provider_prefix, _unescaped_spec): ref_url=self.ref_url, ) - self.build = 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"], - ) + if issubclass(BuildClass, Build): + # Deprecated + 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(): done = False From 0726b2ec4f81838ae54c3b1ede3d5bedf4c079b0 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Sat, 30 Jul 2022 19:45:39 +0100 Subject: [PATCH 04/18] Pass event loop instead of using current() --- binderhub/app.py | 7 ++++++- binderhub/build.py | 7 +------ binderhub/builder.py | 7 ++++--- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index ae57543a2..cd97268cc 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -26,6 +26,7 @@ from tornado.httpserver import HTTPServer from tornado.log import app_log from traitlets import ( + Any, Bool, Bytes, Dict, @@ -700,6 +701,8 @@ def _template_path_default(self): help="Origin to use when emitting events. Defaults to hostname of request when empty", ) + ioloop = Any(help="Event loop to use") + @staticmethod def add_url_prefix(prefix, handlers): """add a url prefix to handlers""" @@ -727,6 +730,7 @@ def init_pycurl(self): def initialize(self, *args, **kwargs): """Load configuration settings.""" super().initialize(*args, **kwargs) + self.ioloop = tornado.ioloop.IOLoop.current() self.load_config_file(self.config_file) # hook up tornado logging if self.debug: @@ -839,6 +843,7 @@ def initialize(self, *args, **kwargs): "auth_enabled": self.auth_enabled, "event_log": self.event_log, "normalized_origin": self.normalized_origin, + "ioloop": self.ioloop, } ) if self.auth_enabled: @@ -960,7 +965,7 @@ def start(self, run_loop=True): if self.builder_required: asyncio.ensure_future(self.watch_build_pods()) if run_loop: - tornado.ioloop.IOLoop.current().start() + self.ioloop.start() main = BinderHub.launch_instance diff --git a/binderhub/build.py b/binderhub/build.py index 649a4c950..49d85e146 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -13,7 +13,6 @@ import kubernetes.config from kubernetes import client, watch -from tornado.ioloop import IOLoop from tornado.log import app_log from traitlets import Any, Bool, Dict, Integer, Unicode, default from traitlets.config import LoggingConfigurable @@ -101,11 +100,7 @@ class BuildExecutor(LoggingConfigurable): config=True, ) - main_loop = Any() - - @default("main_loop") - def _default_main_loop(self): - return IOLoop.current() + main_loop = Any(allow_none=False) stop_event = Any() diff --git a/binderhub/builder.py b/binderhub/builder.py index b3a178a56..6061871db 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -15,7 +15,6 @@ from prometheus_client import Counter, Gauge, Histogram from tornado import gen from tornado.httpclient import HTTPClientError -from tornado.ioloop import IOLoop from tornado.iostream import StreamClosedError from tornado.log import app_log from tornado.queues import Queue @@ -260,7 +259,7 @@ async def get(self, provider_prefix, _unescaped_spec): return # create a heartbeat - IOLoop.current().spawn_callback(self.keep_alive) + self.settings["ioloop"].spawn_callback(self.keep_alive) spec = spec.rstrip("/") key = f"{provider_prefix}:{spec}" @@ -447,6 +446,7 @@ async def get(self, provider_prefix, _unescaped_spec): git_credentials=provider.git_credentials, sticky_builds=self.settings["sticky_builds"], ) + build.main_loop = self.settings["ioloop"] else: build = BuildClass( # Commented properties should be set in traitlets config @@ -467,6 +467,7 @@ async def get(self, provider_prefix, _unescaped_spec): # log_tail_lines=self.settings["log_tail_lines"], git_credentials=provider.git_credentials, # sticky_builds=self.settings["sticky_builds"], + main_loop=self.settings["ioloop"], ) self.build = build @@ -491,7 +492,7 @@ def _check_result(future): # Start building submit_future = pool.submit(build.submit) submit_future.add_done_callback(_check_result) - IOLoop.current().add_callback(lambda: submit_future) + self.settings["ioloop"].add_callback(lambda: submit_future) log_future = None From 2a10ed1c2b4b1a10f79f60e9c5f0bb35feb49e18 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Mon, 1 Aug 2022 20:28:15 +0100 Subject: [PATCH 05/18] CI: don't rely on current ioloop --- binderhub/tests/conftest.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/binderhub/tests/conftest.py b/binderhub/tests/conftest.py index 6bdf13186..4cde689f1 100644 --- a/binderhub/tests/conftest.py +++ b/binderhub/tests/conftest.py @@ -128,14 +128,12 @@ def mock_asynchttpclient(request): @pytest.fixture -def io_loop(event_loop, request): +async def io_loop(event_loop, request): """Same as pytest-tornado.io_loop, but runs with pytest-asyncio""" io_loop = AsyncIOMainLoop() - io_loop.make_current() assert io_loop.asyncio_loop is event_loop def _close(): - io_loop.clear_current() io_loop.close(all_fds=True) request.addfinalizer(_close) From 57a1d8da6c109234f51b66708126905a03328ab9 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Mon, 1 Aug 2022 21:22:39 +0100 Subject: [PATCH 06/18] ci: test.yml add time limit for jobs --- .github/workflows/test.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index da406f61a..a3b830add 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -43,6 +43,7 @@ jobs: # jobs or have "template" jobs, so we use `if` conditions on steps tests: runs-on: ubuntu-20.04 + timeout-minutes: 10 strategy: # keep running so we can see if tests with other k3s/k8s/helm versions pass fail-fast: false @@ -278,6 +279,7 @@ jobs: test-local: runs-on: ubuntu-20.04 + timeout-minutes: 5 steps: - uses: actions/checkout@v3 From 9e204375973683ef295995d31ca8d067f5963556 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Mon, 1 Aug 2022 22:14:05 +0100 Subject: [PATCH 07/18] Mock Build tests: need to use correct types and set loop --- binderhub/tests/conftest.py | 2 +- binderhub/tests/test_build.py | 50 ++++++++++++++++++----------------- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/binderhub/tests/conftest.py b/binderhub/tests/conftest.py index 4cde689f1..1c05124f9 100644 --- a/binderhub/tests/conftest.py +++ b/binderhub/tests/conftest.py @@ -128,7 +128,7 @@ def mock_asynchttpclient(request): @pytest.fixture -async def io_loop(event_loop, request): +def io_loop(event_loop, request): """Same as pytest-tornado.io_loop, but runs with pytest-asyncio""" io_loop = AsyncIOMainLoop() assert io_loop.asyncio_loop is event_loop diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index bb5b32272..7cee08059 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -122,15 +122,15 @@ def test_default_affinity(): api=mock_k8s_api, name="test_build", namespace="build_namespace", - repo_url=mock.MagicMock(), - ref=mock.MagicMock(), - build_image=mock.MagicMock(), - image_name=mock.MagicMock(), - push_secret=mock.MagicMock(), - memory_limit=mock.MagicMock(), + repo_url=mock.MagicMock(spec=str), + ref=mock.MagicMock(spec=str), + build_image=mock.MagicMock(spec=str), + image_name=mock.MagicMock(spec=str), + push_secret=mock.MagicMock(spec=str), + memory_limit=mock.MagicMock(spec=int), git_credentials=None, docker_host="http://mydockerregistry.local", - node_selector=mock.MagicMock(), + node_selector=mock.MagicMock(spec=dict), ) affinity = build.get_affinity() @@ -150,15 +150,15 @@ def test_sticky_builds_affinity(): api=mock_k8s_api, name="test_build", namespace="build_namespace", - repo_url=mock.MagicMock(), - ref=mock.MagicMock(), - build_image=mock.MagicMock(), - image_name=mock.MagicMock(), - push_secret=mock.MagicMock(), - memory_limit=mock.MagicMock(), + repo_url=mock.MagicMock(spec=str), + ref=mock.MagicMock(spec=str), + build_image=mock.MagicMock(spec=str), + image_name=mock.MagicMock(spec=str), + push_secret=mock.MagicMock(spec=str), + memory_limit=mock.MagicMock(spec=int), git_credentials=None, docker_host="http://mydockerregistry.local", - node_selector=mock.MagicMock(), + node_selector=mock.MagicMock(spec=dict), sticky_builds=True, ) @@ -188,15 +188,15 @@ def test_git_credentials_passed_to_podspec_upon_submit(): api=mock_k8s_api, name="test_build", namespace="build_namespace", - repo_url=mock.MagicMock(), - ref=mock.MagicMock(), + repo_url=mock.MagicMock(spec=str), + ref=mock.MagicMock(spec=str), git_credentials=git_credentials, - build_image=mock.MagicMock(), - image_name=mock.MagicMock(), - push_secret=mock.MagicMock(), - memory_limit=mock.MagicMock(), + build_image=mock.MagicMock(spec=str), + image_name=mock.MagicMock(spec=str), + push_secret=mock.MagicMock(spec=str), + memory_limit=mock.MagicMock(spec=int), docker_host="http://mydockerregistry.local", - node_selector=mock.MagicMock(), + node_selector=mock.MagicMock(spec=dict), ) with mock.patch.object(build.stop_event, "is_set", return_value=True): @@ -215,7 +215,7 @@ def test_git_credentials_passed_to_podspec_upon_submit(): assert env["GIT_CREDENTIAL_ENV"] == git_credentials -async def test_local_repo2docker_build(): +async def test_local_repo2docker_build(io_loop): q = Queue() repo_url = "https://github.com/binderhub-ci-repos/cached-minimal-dockerfile" ref = "HEAD" @@ -227,6 +227,7 @@ async def test_local_repo2docker_build(): repo_url=repo_url, ref=ref, image_name=name, + main_loop=io_loop, ) build.submit() @@ -246,7 +247,7 @@ async def test_local_repo2docker_build(): @pytest.mark.asyncio(timeout=20) -async def test_local_repo2docker_build_stop(event_loop): +async def test_local_repo2docker_build_stop(io_loop): q = Queue() # We need a slow build here so that we can interrupt it, so pick a large repo that # will take several seconds to clone @@ -260,8 +261,9 @@ async def test_local_repo2docker_build_stop(event_loop): repo_url=repo_url, ref=ref, image_name=name, + main_loop=io_loop, ) - event_loop.run_in_executor(None, build.submit) + io_loop.run_in_executor(None, build.submit) # Get first few log messages to check it successfully stared event = await q.get() From 78fb0aa0b63ec73984f3097b09754c2552228b67 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Tue, 2 Aug 2022 22:48:42 +0100 Subject: [PATCH 08/18] KubernetesBuildExecutor: ensure traitlets have defaults --- binderhub/build.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/binderhub/build.py b/binderhub/build.py index 49d85e146..610e4c253 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -4,6 +4,7 @@ import datetime import json +import os import threading import warnings from collections import defaultdict @@ -218,12 +219,18 @@ def _default_api(self): help="Kubernetes namespace to spawn build pods into", config=True ) + @default("namespace") + def _default_namespace(self): + return os.getenv("BUILD_NAMESPACE", "default") + build_image = Unicode( + "quay.io/jupyterhub/repo2docker:2022.02.0", help="Docker image containing repo2docker that is used to spawn the build pods.", config=True, ) docker_host = Unicode( + "/var/run/docker.sock", help=( "The docker socket to use for building the image. " "Must be a unix domain socket on a filesystem path accessible on the node " @@ -244,7 +251,7 @@ def _default_api(self): ) node_selector = Dict( - allow_none=True, help="Node selector for the kubernetes build pod.", config=True + {}, help="Node selector for the kubernetes build pod.", config=True ) log_tail_lines = Integer( @@ -578,6 +585,10 @@ def _default_kube(self): namespace = Unicode(help="Kubernetes namespace", config=True) + @default("namespace") + def _default_namespace(self): + return os.getenv("BUILD_NAMESPACE", "default") + max_age = Integer(help="Maximum age of build pods to keep", config=True) def cleanup(self): From bbfb96baf1dc1cbe29ebde94b7f6b57ddcad6148 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Mon, 8 Aug 2022 19:31:46 +0100 Subject: [PATCH 09/18] BuildExecutor: appendix git_credentials push_secret q types --- binderhub/build.py | 11 ++++++----- binderhub/tests/test_build.py | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index 610e4c253..6b1355ef0 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -15,7 +15,7 @@ import kubernetes.config from kubernetes import client, watch from tornado.log import app_log -from traitlets import Any, Bool, Dict, Integer, Unicode, default +from traitlets import Any, Bool, CUnicode, Dict, Integer, Unicode, default from traitlets.config import LoggingConfigurable from .utils import KUBE_REQUEST_TIMEOUT, rendezvous_rank @@ -58,7 +58,6 @@ class BuildExecutor(LoggingConfigurable): """ q = Any( - allow_none=True, help="Queue that receives progress events after the build has been submitted", ) @@ -75,7 +74,8 @@ class BuildExecutor(LoggingConfigurable): image_name = Unicode(help="Full name of the image to build. Includes the tag.") - git_credentials = Any( + git_credentials = CUnicode( + "", allow_none=True, help=( "Git credentials to use when cloning the repository, passed via the GIT_CREDENTIAL_ENV environment variable." @@ -86,8 +86,9 @@ class BuildExecutor(LoggingConfigurable): ) push_secret = Unicode( + "", allow_none=True, - help="Implementation dependent name of a secret for pushing image to a registry.", + help="Implementation dependent secret for pushing image to a registry.", config=True, ) @@ -96,7 +97,7 @@ class BuildExecutor(LoggingConfigurable): ) appendix = Unicode( - allow_none=True, + "", help="Appendix to be added at the end of the Dockerfile used by repo2docker.", config=True, ) diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 7cee08059..2ddb8ac83 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -212,7 +212,7 @@ def test_git_credentials_passed_to_podspec_upon_submit(): env = {env_var.name: env_var.value for env_var in pod.spec.containers[0].env} - assert env["GIT_CREDENTIAL_ENV"] == git_credentials + assert env["GIT_CREDENTIAL_ENV"] == str(git_credentials) async def test_local_repo2docker_build(io_loop): From 99a8ac85dcd741f9ccd0d34c2d1aa88fbd421e76 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Mon, 8 Aug 2022 20:06:21 +0100 Subject: [PATCH 10/18] Remove MagicMock from test_build Build params --- binderhub/tests/test_build.py | 42 +++++++++++++++++------------------ 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 2ddb8ac83..86b9799ae 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -122,15 +122,15 @@ def test_default_affinity(): api=mock_k8s_api, name="test_build", namespace="build_namespace", - repo_url=mock.MagicMock(spec=str), - ref=mock.MagicMock(spec=str), - build_image=mock.MagicMock(spec=str), - image_name=mock.MagicMock(spec=str), - push_secret=mock.MagicMock(spec=str), - memory_limit=mock.MagicMock(spec=int), + repo_url="repo", + ref="ref", + build_image="image", + image_name="name", + push_secret="", + memory_limit=0, git_credentials=None, docker_host="http://mydockerregistry.local", - node_selector=mock.MagicMock(spec=dict), + node_selector={}, ) affinity = build.get_affinity() @@ -150,15 +150,15 @@ def test_sticky_builds_affinity(): api=mock_k8s_api, name="test_build", namespace="build_namespace", - repo_url=mock.MagicMock(spec=str), - ref=mock.MagicMock(spec=str), - build_image=mock.MagicMock(spec=str), - image_name=mock.MagicMock(spec=str), - push_secret=mock.MagicMock(spec=str), - memory_limit=mock.MagicMock(spec=int), + repo_url="repo", + ref="ref", + build_image="image", + image_name="name", + push_secret="", + memory_limit=0, git_credentials=None, docker_host="http://mydockerregistry.local", - node_selector=mock.MagicMock(spec=dict), + node_selector={}, sticky_builds=True, ) @@ -188,15 +188,15 @@ def test_git_credentials_passed_to_podspec_upon_submit(): api=mock_k8s_api, name="test_build", namespace="build_namespace", - repo_url=mock.MagicMock(spec=str), - ref=mock.MagicMock(spec=str), + repo_url="repo", + ref="ref", + build_image="image", + image_name="name", + push_secret="", + memory_limit=0, git_credentials=git_credentials, - build_image=mock.MagicMock(spec=str), - image_name=mock.MagicMock(spec=str), - push_secret=mock.MagicMock(spec=str), - memory_limit=mock.MagicMock(spec=int), docker_host="http://mydockerregistry.local", - node_selector=mock.MagicMock(spec=dict), + node_selector={}, ) with mock.patch.object(build.stop_event, "is_set", return_value=True): From 8b0dafa56f6e83989155ebfc693b58161e63985a Mon Sep 17 00:00:00 2001 From: Simon Li Date: Mon, 8 Aug 2022 20:08:59 +0100 Subject: [PATCH 11/18] Don't pass ioloop around app, only pass to BuildExecutor --- binderhub/app.py | 7 +------ binderhub/builder.py | 9 +++++---- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index cd97268cc..ae57543a2 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -26,7 +26,6 @@ from tornado.httpserver import HTTPServer from tornado.log import app_log from traitlets import ( - Any, Bool, Bytes, Dict, @@ -701,8 +700,6 @@ def _template_path_default(self): help="Origin to use when emitting events. Defaults to hostname of request when empty", ) - ioloop = Any(help="Event loop to use") - @staticmethod def add_url_prefix(prefix, handlers): """add a url prefix to handlers""" @@ -730,7 +727,6 @@ def init_pycurl(self): def initialize(self, *args, **kwargs): """Load configuration settings.""" super().initialize(*args, **kwargs) - self.ioloop = tornado.ioloop.IOLoop.current() self.load_config_file(self.config_file) # hook up tornado logging if self.debug: @@ -843,7 +839,6 @@ def initialize(self, *args, **kwargs): "auth_enabled": self.auth_enabled, "event_log": self.event_log, "normalized_origin": self.normalized_origin, - "ioloop": self.ioloop, } ) if self.auth_enabled: @@ -965,7 +960,7 @@ def start(self, run_loop=True): if self.builder_required: asyncio.ensure_future(self.watch_build_pods()) if run_loop: - self.ioloop.start() + tornado.ioloop.IOLoop.current().start() main = BinderHub.launch_instance diff --git a/binderhub/builder.py b/binderhub/builder.py index 6061871db..7ae9da061 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -15,6 +15,7 @@ from prometheus_client import Counter, Gauge, Histogram from tornado import gen from tornado.httpclient import HTTPClientError +from tornado.ioloop import IOLoop from tornado.iostream import StreamClosedError from tornado.log import app_log from tornado.queues import Queue @@ -259,7 +260,7 @@ async def get(self, provider_prefix, _unescaped_spec): return # create a heartbeat - self.settings["ioloop"].spawn_callback(self.keep_alive) + IOLoop.current().spawn_callback(self.keep_alive) spec = spec.rstrip("/") key = f"{provider_prefix}:{spec}" @@ -446,7 +447,7 @@ async def get(self, provider_prefix, _unescaped_spec): git_credentials=provider.git_credentials, sticky_builds=self.settings["sticky_builds"], ) - build.main_loop = self.settings["ioloop"] + build.main_loop = IOLoop.current() else: build = BuildClass( # Commented properties should be set in traitlets config @@ -467,7 +468,7 @@ async def get(self, provider_prefix, _unescaped_spec): # log_tail_lines=self.settings["log_tail_lines"], git_credentials=provider.git_credentials, # sticky_builds=self.settings["sticky_builds"], - main_loop=self.settings["ioloop"], + main_loop=IOLoop.current(), ) self.build = build @@ -492,7 +493,7 @@ def _check_result(future): # Start building submit_future = pool.submit(build.submit) submit_future.add_done_callback(_check_result) - self.settings["ioloop"].add_callback(lambda: submit_future) + IOLoop.current().add_callback(lambda: submit_future) log_future = None From eeee549c9308f17883034877eecd80796d8a018a Mon Sep 17 00:00:00 2001 From: Simon Li Date: Mon, 8 Aug 2022 20:29:13 +0100 Subject: [PATCH 12/18] conftest: io_loop fixture is async --- binderhub/tests/conftest.py | 2 +- binderhub/tests/test_build.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/binderhub/tests/conftest.py b/binderhub/tests/conftest.py index 1c05124f9..4cde689f1 100644 --- a/binderhub/tests/conftest.py +++ b/binderhub/tests/conftest.py @@ -128,7 +128,7 @@ def mock_asynchttpclient(request): @pytest.fixture -def io_loop(event_loop, request): +async def io_loop(event_loop, request): """Same as pytest-tornado.io_loop, but runs with pytest-asyncio""" io_loop = AsyncIOMainLoop() assert io_loop.asyncio_loop is event_loop diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 86b9799ae..fbf487918 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -216,6 +216,7 @@ def test_git_credentials_passed_to_podspec_upon_submit(): async def test_local_repo2docker_build(io_loop): + io_loop = await io_loop q = Queue() repo_url = "https://github.com/binderhub-ci-repos/cached-minimal-dockerfile" ref = "HEAD" @@ -248,6 +249,7 @@ async def test_local_repo2docker_build(io_loop): @pytest.mark.asyncio(timeout=20) async def test_local_repo2docker_build_stop(io_loop): + io_loop = await io_loop q = Queue() # We need a slow build here so that we can interrupt it, so pick a large repo that # will take several seconds to clone From aea5b3d4c0c32fa17baba957568f95e8519690e4 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Mon, 8 Aug 2022 20:39:50 +0100 Subject: [PATCH 13/18] Setup main_loop inside BuildExecutor --- binderhub/build.py | 5 +++++ binderhub/builder.py | 2 -- binderhub/tests/test_build.py | 5 +---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index 6b1355ef0..c82a65386 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -14,6 +14,7 @@ import kubernetes.config from kubernetes import client, watch +from tornado.ioloop import IOLoop from tornado.log import app_log from traitlets import Any, Bool, CUnicode, Dict, Integer, Unicode, default from traitlets.config import LoggingConfigurable @@ -104,6 +105,10 @@ class BuildExecutor(LoggingConfigurable): main_loop = Any(allow_none=False) + @default("main_loop") + def _default_main_loop(self): + return IOLoop.current() + stop_event = Any() @default("stop_event") diff --git a/binderhub/builder.py b/binderhub/builder.py index 7ae9da061..b3a178a56 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -447,7 +447,6 @@ async def get(self, provider_prefix, _unescaped_spec): git_credentials=provider.git_credentials, sticky_builds=self.settings["sticky_builds"], ) - build.main_loop = IOLoop.current() else: build = BuildClass( # Commented properties should be set in traitlets config @@ -468,7 +467,6 @@ async def get(self, provider_prefix, _unescaped_spec): # log_tail_lines=self.settings["log_tail_lines"], git_credentials=provider.git_credentials, # sticky_builds=self.settings["sticky_builds"], - main_loop=IOLoop.current(), ) self.build = build diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index fbf487918..eab0afb9a 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -215,8 +215,7 @@ def test_git_credentials_passed_to_podspec_upon_submit(): assert env["GIT_CREDENTIAL_ENV"] == str(git_credentials) -async def test_local_repo2docker_build(io_loop): - io_loop = await io_loop +async def test_local_repo2docker_build(): q = Queue() repo_url = "https://github.com/binderhub-ci-repos/cached-minimal-dockerfile" ref = "HEAD" @@ -228,7 +227,6 @@ async def test_local_repo2docker_build(io_loop): repo_url=repo_url, ref=ref, image_name=name, - main_loop=io_loop, ) build.submit() @@ -263,7 +261,6 @@ async def test_local_repo2docker_build_stop(io_loop): repo_url=repo_url, ref=ref, image_name=name, - main_loop=io_loop, ) io_loop.run_in_executor(None, build.submit) From 60429f75662a84b727e352c9a2fe1841ffad9d47 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Mon, 8 Aug 2022 21:02:23 +0100 Subject: [PATCH 14/18] Setup main_loop inside BuildExecutor.__init__ --- binderhub/build.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index c82a65386..fb8bb3eab 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -103,11 +103,9 @@ class BuildExecutor(LoggingConfigurable): config=True, ) - main_loop = Any(allow_none=False) - - @default("main_loop") - def _default_main_loop(self): - return IOLoop.current() + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.main_loop = IOLoop.current() stop_event = Any() From e666855aab7dc272fcaab8dce236438cac4c42f8 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Fri, 12 Aug 2022 20:37:55 +0100 Subject: [PATCH 15/18] git_credentials must be a string, don't cast --- binderhub/build.py | 4 ++-- binderhub/tests/test_build.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/binderhub/build.py b/binderhub/build.py index fb8bb3eab..5d429978a 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -16,7 +16,7 @@ from kubernetes import client, watch from tornado.ioloop import IOLoop from tornado.log import app_log -from traitlets import Any, Bool, CUnicode, Dict, Integer, Unicode, default +from traitlets import Any, Bool, Dict, Integer, Unicode, default from traitlets.config import LoggingConfigurable from .utils import KUBE_REQUEST_TIMEOUT, rendezvous_rank @@ -75,7 +75,7 @@ class BuildExecutor(LoggingConfigurable): image_name = Unicode(help="Full name of the image to build. Includes the tag.") - git_credentials = CUnicode( + git_credentials = Unicode( "", allow_none=True, help=( diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index eab0afb9a..83f9a0990 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -176,10 +176,10 @@ def test_sticky_builds_affinity(): def test_git_credentials_passed_to_podspec_upon_submit(): - git_credentials = { + git_credentials = """{ "client_id": "my_username", "access_token": "my_access_token", - } + }""" mock_k8s_api = _list_dind_pods_mock() @@ -212,7 +212,7 @@ def test_git_credentials_passed_to_podspec_upon_submit(): env = {env_var.name: env_var.value for env_var in pod.spec.containers[0].env} - assert env["GIT_CREDENTIAL_ENV"] == str(git_credentials) + assert env["GIT_CREDENTIAL_ENV"] == git_credentials async def test_local_repo2docker_build(): From eea8e6e12f7e28ee9ff3b27e19fc8756481d127a Mon Sep 17 00:00:00 2001 From: Simon Li Date: Fri, 12 Aug 2022 20:50:40 +0100 Subject: [PATCH 16/18] BuildExecutor.appendix set by traitlets, not passed from BinderHub.appendix --- binderhub/builder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index b3a178a56..fffafb15f 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -463,7 +463,7 @@ async def get(self, provider_prefix, _unescaped_spec): # memory_request=self.settings["build_memory_request"], # docker_host=self.settings["build_docker_host"], # node_selector=self.settings["build_node_selector"], - appendix=appendix, + # appendix=appendix, # log_tail_lines=self.settings["log_tail_lines"], git_credentials=provider.git_credentials, # sticky_builds=self.settings["sticky_builds"], From dd2d26c3616d70b606c880e617ca7cb30b7333f7 Mon Sep 17 00:00:00 2001 From: Simon Li Date: Sun, 14 Aug 2022 22:48:44 +0100 Subject: [PATCH 17/18] builder.py deprecation: xref Build class Co-authored-by: Erik Sundell --- binderhub/builder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index fffafb15f..6b49bd593 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -426,7 +426,7 @@ async def get(self, provider_prefix, _unescaped_spec): ) if issubclass(BuildClass, Build): - # Deprecated + # Deprecated, see docstring of the Build class for more details build = BuildClass( q=q, # api object can be None if we are using FakeBuild From b49edf613cd681b9d11edc6d13f26bd752e3f60f Mon Sep 17 00:00:00 2001 From: Simon Li Date: Sun, 14 Aug 2022 22:52:06 +0100 Subject: [PATCH 18/18] BuildExecutor.stop: docstring clasrification --- binderhub/build.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/binderhub/build.py b/binderhub/build.py index 5d429978a..c7dad35ca 100644 --- a/binderhub/build.py +++ b/binderhub/build.py @@ -180,7 +180,8 @@ def stop(self): """ Stop watching progress of build - TODO: What exactly is the purpose of this method? + Frees up build watchers that are no longer hooked up to any current requests. + This is not related to stopping the build. """ self.stop_event.set()