From 0ae59a82a621f7de15da3c8f49cf0060322ba8a8 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Mon, 25 Jul 2022 09:58:49 -0400 Subject: [PATCH] fix: avoid double-rendering mounts to docker-compose.tmp.yml (#669) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In certain code paths, such as in `tutor local quickstart`, `process_mount_points` is called more than once in the same process, causing mounts to be added to `COMPOSE_LOCAL[_JOBS]_TMP` redundantly. As a result, docker-compose[.jobs].tmp.yml was occasionally being rendered with duplicate volume specifiers. Some versions of Docker Compose ignored this; other versions warned or threw an error. In order to make `process_mount_points` tolerant to being called multiple times, we wrap its volume-adding callbacks within a new hooks context. This allows us to clear said hooks context every time `process_mount_points` is called, essentially making the function idempotent. Co-authored-by: Régis Behmo --- CHANGELOG.md | 1 + tests/commands/test_compose.py | 51 ++++++++++++++++++++++++++++++++++ tutor/commands/compose.py | 37 ++++++++++++++++-------- tutor/hooks/consts.py | 8 ++++-- 4 files changed, 83 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05eda87..7b16ced 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ Every user-facing change should have an entry in this changelog. Please respect ## Unreleased +- [Fix] `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) ## v14.0.3 (2022-07-09) diff --git a/tests/commands/test_compose.py b/tests/commands/test_compose.py index 67862a6..fc4dcc2 100644 --- a/tests/commands/test_compose.py +++ b/tests/commands/test_compose.py @@ -1,11 +1,16 @@ +import typing as t import unittest from click.exceptions import ClickException +from tutor import hooks from tutor.commands import compose class ComposeTests(unittest.TestCase): + + maxDiff = None # Ensure we can see long diffs of YAML files. + def test_mount_option_parsing(self) -> None: param = compose.MountParam() @@ -36,3 +41,49 @@ class ComposeTests(unittest.TestCase): ) with self.assertRaises(ClickException): param("lms,:/path/to/edx-platform:/openedx/edx-platform") + + def test_compose_local_tmp_generation(self) -> None: + """ + Ensure that docker-compose.tmp.yml is correctly generated, even when + mounts are processed more than once. + """ + param = compose.MountParam() + mount_args: t.Tuple[t.List[compose.MountParam.MountType], ...] = ( + # Auto-mounting of edx-platform to lms* and cms* + param("/path/to/edx-platform"), + # Manual mounting of some other folder to mfe and lms + param("mfe,lms:/path/to/something-else:/openedx/something-else"), + ) + + # In some cases, process_mount_arguments ends up being called more + # than once. To ensure that we can handle that situation, we call it + # multiple times here. + compose.process_mount_arguments(mount_args) + compose.process_mount_arguments(mount_args) + compose.process_mount_arguments(mount_args) + + 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 fd5d9bc..361e546 100644 --- a/tutor/commands/compose.py +++ b/tutor/commands/compose.py @@ -465,7 +465,7 @@ 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: +def process_mount_arguments(mounts: t.Tuple[t.List[MountParam.MountType], ...]) -> None: """ Process --mount arguments. @@ -496,18 +496,31 @@ def process_mount_arguments(mounts: t.Tuple[t.List[MountParam.MountType]]) -> No 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) + # Clear filter callbacks already created within the COMPOSE_CLI_MOUNTS context. + # This prevents us from redundantly specifying these volume mounts in cases + # where process_mount_arguments is called more than once. + hooks.Filters.COMPOSE_LOCAL_TMP.clear( + context=hooks.Contexts.COMPOSE_CLI_MOUNTS.name + ) + hooks.Filters.COMPOSE_LOCAL_JOBS_TMP.clear( + context=hooks.Contexts.COMPOSE_CLI_MOUNTS.name + ) - @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) + # Now, within that COMPOSE_CLI_MOUNTS context, (re-)create filter callbacks to + # specify these volume mounts within the docker-compose[.jobs].tmp.yml files. + with hooks.Contexts.COMPOSE_CLI_MOUNTS.enter(): + + @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_jobs_tmp: t.Dict[str, t.Any] + ) -> t.Dict[str, t.Any]: + return _add_mounts(docker_compose_jobs_tmp, job_mounts) @hooks.Filters.COMPOSE_MOUNTS.add() diff --git a/tutor/hooks/consts.py b/tutor/hooks/consts.py index 92f5709..99d24f1 100644 --- a/tutor/hooks/consts.py +++ b/tutor/hooks/consts.py @@ -354,8 +354,12 @@ class Contexts: #: Plugins will be installed and enabled within this context. PLUGINS = contexts.Context("plugins") - #: YAML-formatted v0 plugins will be installed within that context. + #: YAML-formatted v0 plugins will be installed within this context. PLUGINS_V0_YAML = contexts.Context("plugins:v0:yaml") - #: Python entrypoint plugins will be installed within that context. + #: Python entrypoint plugins will be installed within this context. PLUGINS_V0_ENTRYPOINT = contexts.Context("plugins:v0:entrypoint") + + #: Docker Compose volumes added via the CLI's ``--mount`` option will + #: be installed within this context. + COMPOSE_CLI_MOUNTS = contexts.Context("compose:cli:mounts")