fix: avoid double-rendering mounts to docker-compose.tmp.yml (#669)

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 <regis@behmo.com>

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)

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)

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()

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")