From ba2c241acf40a56322ab5ab0e44b524dd75d75c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Behmo?= Date: Mon, 25 Jul 2022 19:19:28 +0200 Subject: [PATCH] fix: bind-mount in dev-specific services The -m/--mount option makes it possible to bind-mount volumes at runtime. The volumes are declared in a local/docker-compose.tmp.yml file. The problem with this approach is when we want to bind-mount a volume to a service which is specific to the dev context. For instance: the "learning" service when the MFE plugin is enabled. In such a case, starting the service triggers a call to `docker-compose stop` in the local context. This call fails because the "learning" service does not exist in the local context. Note that this issue only seems to occur with docker-compose v1. To resolve this issue, we create two additional filters for the dev context, which emulate the behaviour of the local context. With this approach, we convert the -m/--mount arguments right after they are parsed. Because they are parsed just once, we can get rid of the de-duplication logic initially introduced with the COMPOSE_CLI_MOUNTS context. Close #711. Close also https://github.com/overhangio/tutor-mfe/issues/57. --- CHANGELOG.md | 18 +++ tests/commands/test_compose.py | 46 +++++++ tutor/commands/compose.py | 218 ++++++++++++++++++--------------- tutor/commands/dev.py | 18 ++- tutor/commands/local.py | 24 +++- tutor/hooks/consts.py | 6 + tutor/utils.py | 1 + 7 files changed, 225 insertions(+), 106 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ba685a74b..1d0892ad9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,24 @@ Every user-facing change should have an entry in this changelog. Please respect ## Unreleased +- [Bugfix] Fix `tutor dev start -m /path/to/frontend-app-learning` by introducing dev-specific `COMPOSE_DEV_TMP` and `COMPOSE_DEV_JOBS_TMP` filters (by @regisb). +- [Bugfix] Log the shell commands that Tutor executes more accurately. (by @kdmccormick) +- [Bugfix] `tutor dev quickstart` would fail under certain versions of docker-compose due to a bug in the logic that handled volume mounting. (by @kdmccormick) +- [Bugfix] The `tutor k8s start` command will succeed even when `k8s-override` and `kustomization-patches-strategic-merge` are not specified. (by @edazzocaisser) +- [BugFix] `kubectl wait` checks deployments instead of pods as it could hang indefinitely if there are extra pods in a broken state. (by @keithgg) + +## v14.0.3 (2022-07-09) + +- [Bugfix] Build openedx-dev Docker image even when the host user is root, for instance on Windows. (by @regisb) +- [Bugfix] Patch nutmeg.1 release with [LTI 1.3 fix](https://github.com/openedx/edx-platform/pull/30716). (by @ormsbee) +- [Improvement] Make it possible to override k8s resources in plugins using `k8s-override` patch. (by @foadlind) + +## v14.0.2 (2022-06-27) + +- [Bugfix] Update problem with hint template so it works with newer python versions. (by @mariajgrimaldi) +- [Feature] Add default PYTHONBREAKPOINT to openedx/Dockerfile (by @Carlos-Muniz) +- [Bugfix] Fix smtp server port in `cms.yml` which was causing email sending failures in the Studio. (by @regisb) +- [Bugfix] Skip waiting for MongoDB if it is served using SRV records. (by @gabor-boros) - [Improvement] Use `git am` instead of `cherry-pick` to simplify patching process. ## v14.0.1 (2022-06-13) diff --git a/tests/commands/test_compose.py b/tests/commands/test_compose.py index 67862a602a..6d3c59f1db 100644 --- a/tests/commands/test_compose.py +++ b/tests/commands/test_compose.py @@ -1,8 +1,11 @@ import unittest +from io import StringIO +from unittest.mock import patch from click.exceptions import ClickException from tutor.commands import compose +from tutor.commands.local import LocalContext class ComposeTests(unittest.TestCase): @@ -36,3 +39,46 @@ def test_mount_option_parsing(self) -> None: ) with self.assertRaises(ClickException): param("lms,:/path/to/edx-platform:/openedx/edx-platform") + + @patch("sys.stdout", new_callable=StringIO) + def test_compose_local_tmp_generation(self, _mock_stdout: StringIO) -> None: + """ + Ensure that docker-compose.tmp.yml is correctly generated. + """ + param = compose.MountParam() + mount_args = ( + # Auto-mounting of edx-platform to lms* and cms* + param.convert_implicit_form("/path/to/edx-platform"), + # Manual mounting of some other folder to mfe and lms + param.convert_explicit_form( + "mfe,lms:/path/to/something-else:/openedx/something-else" + ), + ) + # Mount volumes + compose.mount_tmp_volumes(mount_args, LocalContext("")) + + compose_file: t.Dict[str, t.Any] = hooks.Filters.COMPOSE_LOCAL_TMP.apply({}) + actual_services: t.Dict[str, t.Any] = compose_file["services"] + expected_services: t.Dict[str, t.Any] = { + "cms": {"volumes": ["/path/to/edx-platform:/openedx/edx-platform"]}, + "cms-worker": {"volumes": ["/path/to/edx-platform:/openedx/edx-platform"]}, + "lms": { + "volumes": [ + "/path/to/edx-platform:/openedx/edx-platform", + "/path/to/something-else:/openedx/something-else", + ] + }, + "lms-worker": {"volumes": ["/path/to/edx-platform:/openedx/edx-platform"]}, + "mfe": {"volumes": ["/path/to/something-else:/openedx/something-else"]}, + } + self.assertEqual(actual_services, expected_services) + + compose_jobs_file: t.Dict[ + str, t.Any + ] = hooks.Filters.COMPOSE_LOCAL_JOBS_TMP.apply({}) + actual_jobs_services: t.Dict[str, t.Any] = compose_jobs_file["services"] + expected_jobs_services: t.Dict[str, t.Any] = { + "cms-job": {"volumes": ["/path/to/edx-platform:/openedx/edx-platform"]}, + "lms-job": {"volumes": ["/path/to/edx-platform:/openedx/edx-platform"]}, + } + self.assertEqual(actual_jobs_services, expected_jobs_services) diff --git a/tutor/commands/compose.py b/tutor/commands/compose.py index fd5d9bc503..31d1910122 100644 --- a/tutor/commands/compose.py +++ b/tutor/commands/compose.py @@ -1,6 +1,7 @@ import os import re import typing as t +from copy import deepcopy import click @@ -19,8 +20,6 @@ def __init__(self, root: str, config: Config): self.project_name = "" self.docker_compose_files: t.List[str] = [] self.docker_compose_job_files: t.List[str] = [] - self.docker_compose_tmp_path = "" - self.docker_compose_jobs_tmp_path = "" def docker_compose(self, *command: str) -> int: """ @@ -32,7 +31,6 @@ def docker_compose(self, *command: str) -> int: hooks.Actions.COMPOSE_PROJECT_STARTED.do( self.root, self.config, self.project_name ) - self.__update_docker_compose_tmp() args = [] for docker_compose_path in self.docker_compose_files: if os.path.exists(docker_compose_path): @@ -41,33 +39,45 @@ def docker_compose(self, *command: str) -> int: *args, "--project-name", self.project_name, *command ) - def __update_docker_compose_tmp(self) -> None: + def update_docker_compose_tmp( + self, + compose_tmp_filter: hooks.filters.Filter, + compose_jobs_tmp_filter: hooks.filters.Filter, + docker_compose_tmp_path: str, + docker_compose_jobs_tmp_path: str, + ) -> None: """ - Update the contents of the docker-compose.tmp.yml file, which is generated at runtime. + Update the contents of the docker-compose.tmp.yml and + docker-compose.jobs.tmp.yml files, which are generated at runtime. """ - docker_compose_tmp = { + compose_base = { "version": "{{ DOCKER_COMPOSE_VERSION }}", "services": {}, } - docker_compose_jobs_tmp = { - "version": "{{ DOCKER_COMPOSE_VERSION }}", - "services": {}, - } - docker_compose_tmp = hooks.Filters.COMPOSE_LOCAL_TMP.apply(docker_compose_tmp) - docker_compose_jobs_tmp = hooks.Filters.COMPOSE_LOCAL_JOBS_TMP.apply( - docker_compose_jobs_tmp - ) - docker_compose_tmp = tutor_env.render_unknown(self.config, docker_compose_tmp) - docker_compose_jobs_tmp = tutor_env.render_unknown( - self.config, docker_compose_jobs_tmp + + # 1. Apply compose_tmp filter + # 2. Render the resulting dict + # 3. Serialize to yaml + # 4. Save to disk + docker_compose_tmp: str = serialize.dumps( + tutor_env.render_unknown( + self.config, compose_tmp_filter.apply(deepcopy(compose_base)) + ) ) tutor_env.write_to( - serialize.dumps(docker_compose_tmp), - self.docker_compose_tmp_path, + docker_compose_tmp, + docker_compose_tmp_path, + ) + + # Same thing but with tmp jobs + docker_compose_jobs_tmp: str = serialize.dumps( + tutor_env.render_unknown( + self.config, compose_jobs_tmp_filter.apply(deepcopy(compose_base)) + ) ) tutor_env.write_to( - serialize.dumps(docker_compose_jobs_tmp), - self.docker_compose_jobs_tmp_path, + docker_compose_jobs_tmp, + docker_compose_jobs_tmp_path, ) def run_job(self, service: str, command: str) -> int: @@ -95,6 +105,9 @@ def run_job(self, service: str, command: str) -> int: class BaseComposeContext(BaseJobContext): + COMPOSE_TMP_FILTER: hooks.filters.Filter = NotImplemented + COMPOSE_JOBS_TMP_FILTER: hooks.filters.Filter = NotImplemented + def job_runner(self, config: Config) -> ComposeJobRunner: raise NotImplementedError @@ -117,32 +130,43 @@ def convert( param: t.Optional["click.Parameter"], ctx: t.Optional[click.Context], ) -> t.List["MountType"]: - mounts: t.List["MountParam.MountType"] = [] + mounts = self.convert_explicit_form(value) or self.convert_implicit_form(value) + return mounts + + def convert_explicit_form(self, value: str) -> t.List["MountParam.MountType"]: + """ + Argument is of the form "containers:/host/path:/container/path". + """ match = re.match(self.PARAM_REGEXP, value) - if match: - # Argument is of the form "containers:/host/path:/container/path" - services: t.List[str] = [ - service.strip() for service in match["services"].split(",") - ] - host_path = os.path.abspath(os.path.expanduser(match["host_path"])) - host_path = host_path.replace(os.path.sep, "/") - container_path = match["container_path"] - for service in services: - if not service: - self.fail( - f"incorrect services syntax: '{match['services']}'", param, ctx - ) - mounts.append((service, host_path, container_path)) - else: - # Argument is of the form "/host/path" - host_path = os.path.abspath(os.path.expanduser(value)) - volumes: t.Iterator[ - t.Tuple[str, str] - ] = hooks.Filters.COMPOSE_MOUNTS.iterate(os.path.basename(host_path)) - for service, container_path in volumes: - mounts.append((service, host_path, container_path)) + if not match: + return [] + + mounts: t.List["MountParam.MountType"] = [] + services: t.List[str] = [ + service.strip() for service in match["services"].split(",") + ] + host_path = os.path.abspath(os.path.expanduser(match["host_path"])) + host_path = host_path.replace(os.path.sep, "/") + container_path = match["container_path"] + for service in services: + if not service: + self.fail(f"incorrect services syntax: '{match['services']}'") + mounts.append((service, host_path, container_path)) + return mounts + + def convert_implicit_form(self, value: str) -> t.List["MountParam.MountType"]: + """ + Argument is of the form "/host/path" + """ + mounts: t.List["MountParam.MountType"] = [] + host_path = os.path.abspath(os.path.expanduser(value)) + volumes: t.Iterator[t.Tuple[str, str]] = hooks.Filters.COMPOSE_MOUNTS.iterate( + os.path.basename(host_path) + ) + for service, container_path in volumes: + mounts.append((service, host_path, container_path)) if not mounts: - raise self.fail(f"no mount found for {value}", param, ctx) + raise self.fail(f"no mount found for {value}") return mounts @@ -156,6 +180,48 @@ def convert( ) +def mount_tmp_volumes( + all_mounts: t.Tuple[t.List[MountParam.MountType], ...], + context: BaseComposeContext, +) -> None: + for mounts in all_mounts: + for service, host_path, container_path in mounts: + mount_tmp_volume(service, host_path, container_path, context) + + +def mount_tmp_volume( + service: str, + host_path: str, + container_path: str, + context: BaseComposeContext, +) -> None: + """ + Append user-defined bind-mounted volumes to the docker-compose.tmp file(s). + + The service/host path/container path values are appended to the docker-compose + files by mean of two filters. Each dev/local environment is then responsible for + generating the files based on the output of these filters. + + Bind-mounts that are associated to "*-job" services will be added to the + docker-compose jobs file. + """ + fmt.echo_info(f"Bind-mount: {host_path} -> {container_path} in {service}") + compose_tmp_filter: hooks.filters.Filter = ( + context.COMPOSE_JOBS_TMP_FILTER + if service.endswith("-job") + else context.COMPOSE_TMP_FILTER + ) + + @compose_tmp_filter.add() + def _add_mounts_to_docker_compose_tmp( + docker_compose: t.Dict[str, t.Any], + ) -> t.Dict[str, t.Any]: + services = docker_compose.setdefault("services", {}) + services.setdefault(service, {"volumes": []}) + services[service]["volumes"].append(f"{host_path}:{container_path}") + return docker_compose + + @click.command( short_help="Run all or a selection of services.", help="Run all or a selection of services. Docker images will be rebuilt where necessary.", @@ -178,9 +244,8 @@ def start( if detach: command.append("-d") - process_mount_arguments(mounts) - # Start services + mount_tmp_volumes(mounts, context) config = tutor_config.load(context.root) context.job_runner(config).docker_compose(*command, *services) @@ -240,7 +305,7 @@ def init( limit: str, mounts: t.Tuple[t.List[MountParam.MountType]], ) -> None: - process_mount_arguments(mounts) + mount_tmp_volumes(mounts, context) config = tutor_config.load(context.root) runner = context.job_runner(config) jobs.initialise(runner, limit_to=limit) @@ -321,11 +386,10 @@ def run( mounts: t.Tuple[t.List[MountParam.MountType]], args: t.List[str], ) -> None: - process_mount_arguments(mounts) extra_args = ["--rm"] if not utils.is_a_tty(): extra_args.append("-T") - context.invoke(dc_command, command="run", args=[*extra_args, *args]) + context.invoke(dc_command, mounts=mounts, command="run", args=[*extra_args, *args]) @click.command( @@ -444,10 +508,17 @@ def status(context: click.Context) -> None: context_settings={"ignore_unknown_options": True}, name="dc", ) +@mount_option @click.argument("command") @click.argument("args", nargs=-1) @click.pass_obj -def dc_command(context: BaseComposeContext, command: str, args: t.List[str]) -> None: +def dc_command( + context: BaseComposeContext, + mounts: t.Tuple[t.List[MountParam.MountType]], + command: str, + args: t.List[str], +) -> None: + mount_tmp_volumes(mounts, context) config = tutor_config.load(context.root) volumes, non_volume_args = bindmounts.parse_volumes(args) volume_args = [] @@ -465,51 +536,6 @@ def dc_command(context: BaseComposeContext, command: str, args: t.List[str]) -> context.job_runner(config).docker_compose(command, *volume_args, *non_volume_args) -def process_mount_arguments(mounts: t.Tuple[t.List[MountParam.MountType]]) -> None: - """ - Process --mount arguments. - - Most docker-compose commands support --mount arguments. This option - is used to bind-mount folders from the host. A docker-compose.tmp.yml is - generated at runtime and includes the bind-mounted volumes that were passed as CLI - arguments. - - Bind-mounts that are associated to "*-job" services will be added to the - docker-compose jobs file. - """ - app_mounts: t.List[MountParam.MountType] = [] - job_mounts: t.List[MountParam.MountType] = [] - for mount in mounts: - for service, host_path, container_path in mount: - if service.endswith("-job"): - job_mounts.append((service, host_path, container_path)) - else: - app_mounts.append((service, host_path, container_path)) - - def _add_mounts( - docker_compose: t.Dict[str, t.Any], bind_mounts: t.List[MountParam.MountType] - ) -> t.Dict[str, t.Any]: - services = docker_compose.setdefault("services", {}) - for service, host_path, container_path in bind_mounts: - fmt.echo_info(f"Bind-mount: {host_path} -> {container_path} in {service}") - services.setdefault(service, {"volumes": []}) - services[service]["volumes"].append(f"{host_path}:{container_path}") - return docker_compose - - # Save bind-mounts - @hooks.Filters.COMPOSE_LOCAL_TMP.add() - def _add_mounts_to_docker_compose_tmp( - docker_compose_tmp: t.Dict[str, t.Any] - ) -> t.Dict[str, t.Any]: - return _add_mounts(docker_compose_tmp, app_mounts) - - @hooks.Filters.COMPOSE_LOCAL_JOBS_TMP.add() - def _add_mounts_to_docker_compose_jobs_tmp( - docker_compose_tmp: t.Dict[str, t.Any] - ) -> t.Dict[str, t.Any]: - return _add_mounts(docker_compose_tmp, job_mounts) - - @hooks.Filters.COMPOSE_MOUNTS.add() def _mount_edx_platform( volumes: t.List[t.Tuple[str, str]], name: str diff --git a/tutor/commands/dev.py b/tutor/commands/dev.py index 297671e3f6..56fe5472eb 100644 --- a/tutor/commands/dev.py +++ b/tutor/commands/dev.py @@ -18,29 +18,39 @@ def __init__(self, root: str, config: Config): """ super().__init__(root, config) self.project_name = get_typed(self.config, "DEV_PROJECT_NAME", str) - self.docker_compose_tmp_path = tutor_env.pathjoin( + docker_compose_tmp_path = tutor_env.pathjoin( self.root, "dev", "docker-compose.tmp.yml" ) - self.docker_compose_jobs_tmp_path = tutor_env.pathjoin( + docker_compose_jobs_tmp_path = tutor_env.pathjoin( self.root, "dev", "docker-compose.jobs.tmp.yml" ) self.docker_compose_files += [ tutor_env.pathjoin(self.root, "local", "docker-compose.yml"), tutor_env.pathjoin(self.root, "dev", "docker-compose.yml"), - self.docker_compose_tmp_path, + docker_compose_tmp_path, tutor_env.pathjoin(self.root, "local", "docker-compose.override.yml"), tutor_env.pathjoin(self.root, "dev", "docker-compose.override.yml"), ] self.docker_compose_job_files += [ tutor_env.pathjoin(self.root, "local", "docker-compose.jobs.yml"), tutor_env.pathjoin(self.root, "dev", "docker-compose.jobs.yml"), - self.docker_compose_jobs_tmp_path, + docker_compose_jobs_tmp_path, tutor_env.pathjoin(self.root, "local", "docker-compose.jobs.override.yml"), tutor_env.pathjoin(self.root, "dev", "docker-compose.jobs.override.yml"), ] + # Update docker-compose.tmp files + self.update_docker_compose_tmp( + hooks.Filters.COMPOSE_DEV_TMP, + hooks.Filters.COMPOSE_DEV_JOBS_TMP, + docker_compose_tmp_path, + docker_compose_jobs_tmp_path, + ) class DevContext(compose.BaseComposeContext): + COMPOSE_TMP_FILTER = hooks.Filters.COMPOSE_DEV_TMP + COMPOSE_JOBS_TMP_FILTER = hooks.Filters.COMPOSE_DEV_JOBS_TMP + def job_runner(self, config: Config) -> DevJobRunner: return DevJobRunner(self.root, config) diff --git a/tutor/commands/local.py b/tutor/commands/local.py index 57ec977e69..e6aae8b4de 100644 --- a/tutor/commands/local.py +++ b/tutor/commands/local.py @@ -20,27 +20,38 @@ def __init__(self, root: str, config: Config): """ super().__init__(root, config) self.project_name = get_typed(self.config, "LOCAL_PROJECT_NAME", str) - self.docker_compose_tmp_path = tutor_env.pathjoin( + docker_compose_tmp_path = tutor_env.pathjoin( self.root, "local", "docker-compose.tmp.yml" ) - self.docker_compose_jobs_tmp_path = tutor_env.pathjoin( + docker_compose_jobs_tmp_path = tutor_env.pathjoin( self.root, "local", "docker-compose.jobs.tmp.yml" ) self.docker_compose_files += [ tutor_env.pathjoin(self.root, "local", "docker-compose.yml"), tutor_env.pathjoin(self.root, "local", "docker-compose.prod.yml"), - self.docker_compose_tmp_path, + docker_compose_tmp_path, tutor_env.pathjoin(self.root, "local", "docker-compose.override.yml"), ] self.docker_compose_job_files += [ tutor_env.pathjoin(self.root, "local", "docker-compose.jobs.yml"), - self.docker_compose_jobs_tmp_path, + docker_compose_jobs_tmp_path, tutor_env.pathjoin(self.root, "local", "docker-compose.jobs.override.yml"), ] + # Update docker-compose.tmp files + self.update_docker_compose_tmp( + hooks.Filters.COMPOSE_LOCAL_TMP, + hooks.Filters.COMPOSE_LOCAL_JOBS_TMP, + docker_compose_tmp_path, + docker_compose_jobs_tmp_path, + ) + # pylint: disable=too-few-public-methods class LocalContext(compose.BaseComposeContext): + COMPOSE_TMP_FILTER = hooks.Filters.COMPOSE_LOCAL_TMP + COMPOSE_JOBS_TMP_FILTER = hooks.Filters.COMPOSE_LOCAL_JOBS_TMP + def job_runner(self, config: Config) -> LocalJobRunner: return LocalJobRunner(self.root, config) @@ -62,6 +73,7 @@ def quickstart( non_interactive: bool, pullimages: bool, ) -> None: + compose.mount_tmp_volumes(mounts, context.obj) try: utils.check_macos_docker_memory() except exceptions.TutorError as e: @@ -128,9 +140,9 @@ def quickstart( click.echo(fmt.title("Docker image updates")) context.invoke(compose.dc_command, command="pull") click.echo(fmt.title("Starting the platform in detached mode")) - context.invoke(compose.start, mounts=mounts, detach=True) + context.invoke(compose.start, detach=True) click.echo(fmt.title("Database creation and migrations")) - context.invoke(compose.init, mounts=mounts) + context.invoke(compose.init) config = tutor_config.load(context.obj.root) fmt.echo_info( diff --git a/tutor/hooks/consts.py b/tutor/hooks/consts.py index 92f5709fa2..982a197820 100644 --- a/tutor/hooks/consts.py +++ b/tutor/hooks/consts.py @@ -121,6 +121,12 @@ def your_filter(items): #: :parameter list[tuple[str, tuple[str, ...]]] tasks: list of ``(service, path)`` tasks. (see :py:data:`COMMANDS_INIT`). COMMANDS_PRE_INIT = filters.get("commands:pre-init") + #: Same as :py:data:`COMPOSE_LOCAL_TMP` but for the development environment. + COMPOSE_DEV_TMP = filters.get("compose:dev:tmp") + + #: Same as :py:data:`COMPOSE_LOCAL_JOBS_TMP` but for the development environment. + COMPOSE_DEV_JOBS_TMP = filters.get("compose:dev-jobs:tmp") + #: List of folders to bind-mount in docker-compose containers, either in ``tutor local`` or ``tutor dev``. #: #: Many ``tutor local`` and ``tutor dev`` commands support ``--mounts`` options diff --git a/tutor/utils.py b/tutor/utils.py index 6175e6ab7d..444441c6ba 100644 --- a/tutor/utils.py +++ b/tutor/utils.py @@ -7,6 +7,7 @@ import struct import subprocess import sys +from functools import lru_cache from typing import List, Tuple import click