Refactor parsing of extra mounts

aiidalab_launch/__main__.py

@@ -21,7 +21,7 @@
 from .application_state import ApplicationState
 from .core import LOGGER
 from .instance import AiidaLabInstance
-from .profile import DEFAULT_IMAGE, DEFAULT_PORT, Profile
+from .profile import DEFAULT_IMAGE, DEFAULT_PORT, ExtraMount, Profile
 from .util import confirm_with_value, get_latest_version, spinner, webbrowser_available
 from .version import __version__
 
@@ -55,7 +55,7 @@
 
 Home mounted: {home_mount} -> /home/{system_user}"""
 
-MSG_EXTRA_VOLUME = "Extra volume mounted: {source} -> {target} {mode}"
+MSG_EXTRA_MOUNT = "Extra {mount_type} mount: {source} -> {target} {mode}"
 
 
 LOGGING_LEVELS = {
@@ -474,12 +474,13 @@ async def _async_start(
             )
 
             for extra_mount in profile.extra_mounts:
-                source, target, mode = profile.parse_extra_mount(extra_mount)
+                mount = ExtraMount.parse_mount_string(extra_mount)
                 click.secho(
-                    MSG_EXTRA_VOLUME.format(
-                        source=source,
-                        target=target,
-                        mode=f"({mode})" if mode else "",
+                    MSG_EXTRA_MOUNT.format(
+                        source=mount["Source"],
+                        target=mount["Target"],
+                        mode="ro" if mount["ReadOnly"] else "rw",
+                        mount_type=mount["Type"],
                     ).lstrip(),
                     fg="green",
                 )

aiidalab_launch/instance.py

@@ -16,7 +16,7 @@
 from docker.models.containers import Container
 
 from .core import LOGGER
-from .profile import Profile
+from .profile import ExtraMount, Profile
 from .util import _async_wrap_iter, docker_mount_for, get_docker_env
 
 
@@ -103,6 +103,7 @@ def _conda_mount(self) -> docker.types.Mount:
         return docker.types.Mount(
             target=f"/home/{self.profile.system_user}/.conda",
             source=self.profile.conda_volume_name(),
+            type="volume",
         )
 
     def _home_mount(self) -> docker.types.Mount:
@@ -115,13 +116,7 @@ def _home_mount(self) -> docker.types.Mount:
         )
 
     def _extra_mount(self, extra_mount: str) -> docker.types.Mount:
-        source_path, target_path, mode = self.profile.parse_extra_mount(extra_mount)
-        return docker.types.Mount(
-            target=str(target_path),
-            source=str(source_path),
-            read_only=True if mode == "ro" else False,
-            type="bind" if source_path.is_absolute() else "volume",
-        )
+        return ExtraMount.parse_mount_string(extra_mount)
 
     def _mounts(self) -> Generator[docker.types.Mount, None, None]:
         yield self._conda_mount()
@@ -237,7 +232,7 @@ def remove(self, conda: bool = False, data: bool = False) -> None:
             try:
                 self.client.volumes.get(self.profile.conda_volume_name()).remove()
             except docker.errors.NotFound:  # already removed
-                LOGGER.debug(
+                LOGGER.warning(
                     f"Failed to remove conda volume '{self.profile.conda_volume_name()}', likely already removed."
                 )
             except Exception as error:  # unexpected error
@@ -248,9 +243,10 @@ def remove(self, conda: bool = False, data: bool = False) -> None:
             home_mount_path = PurePosixPath(self.profile.home_mount)
             try:
                 if home_mount_path.is_absolute():
+                    # TODO: Perhaps we should ask user for confirmation here?
                     rmtree(home_mount_path)
                 else:
-                    self.client.volumes.get(str(home_mount_path)).remove()
+                    self.client.volumes.get(self.profile.home_mount).remove()
             except docker.errors.NotFound:
                 pass  # already removed
             except Exception as error:  # unexpected error

aiidalab_launch/profile.py

@@ -7,6 +7,7 @@
 from pathlib import Path, PurePosixPath
 from urllib.parse import quote_plus
 
+import docker
 import toml
 from docker.models.containers import Container
 
@@ -42,7 +43,7 @@ def _valid_volume_name(source: str) -> None:
     if not Path(source).is_absolute() and not re.fullmatch(
         _REGEX_VALID_IMAGE_NAMES, source
     ):
-        raise ValueError(
+        raise docker.errors.InvalidArgument(
             f"Invalid extra mount volume name '{source}'. Use absolute path for bind mounts."
         )
 
@@ -63,6 +64,43 @@ def _get_aiidalab_default_apps(container: Container) -> list:
         return []
 
 
+# We extend the Mount type from Docker API
+# with some extra validation to fail early if user provides wrong argument.
+# https://github.com/docker/docker-py/blob/bd164f928ab82e798e30db455903578d06ba2070/docker/types/services.py#L305
+class ExtraMount(docker.types.Mount):
+    @classmethod
+    def parse_mount_string(cls, mount_str) -> docker.types.Mount:
+        mount = super().parse_mount_string(mount_str)
+        # For some reason, Docker API allows Source to be None??
+        # Not on our watch!
+        if mount["Source"] is None:
+            raise docker.errors.InvalidArgument(
+                f"Invalid extra mount specification '{mount}'"
+            )
+
+        # If the read-write mode is not "rw", docker assumes "ro"
+        # Let's be more strict here to avoid confusing errors later.
+        parts = mount_str.split(":")
+        if len(parts) == 3:
+            mode = parts[2]
+            if mode not in ("ro", "rw"):
+                raise docker.errors.InvalidArgument(
+                    f"Invalid read-write mode in '{mount}'"
+                )
+
+        # Unlike for home_mount, we will not auto-create missing
+        # directories for extra mounts.
+        if mount["Type"] == "bind":
+            source_path = Path(mount["Source"])
+            if not source_path.exists():
+                raise docker.errors.InvalidArgument(
+                    f"Directory '{source_path}' does not exist!"
+                )
+        else:
+            _valid_volume_name(mount["Source"])
+        return mount
+
+
 @dataclass
 class Profile:
     name: str = MAIN_PROFILE_NAME
@@ -91,45 +129,24 @@ def __post_init__(self):
         # Normalize extra mount mode to be "rw" by default
         # so that we match Docker default but are explicit.
         for extra_mount in self.extra_mounts.copy():
-            self.parse_extra_mount(extra_mount)
-            if len(extra_mount.split(":")) == 2:
+            mount = ExtraMount.parse_mount_string(extra_mount)
+            mode = "ro" if mount["ReadOnly"] else "rw"
+            if not extra_mount.endswith(mode):
                 self.extra_mounts.remove(extra_mount)
-                self.extra_mounts.add(f"{extra_mount}:rw")
+                self.extra_mounts.add(f"{extra_mount}:{mode}")
 
         if (
-            self.image.split(":")[0] == "aiidalab/full-stack"
+            self.image.split(":")[0].endswith("aiidalab/full-stack")
             and self.system_user != "jovyan"
         ):
+            # TODO: ERROR out in this case
             LOGGER.warning(
                 "Resetting the system user may create issues for this image!"
             )
 
     def container_name(self) -> str:
         return f"{CONTAINER_PREFIX}{self.name}"
 
-    def parse_extra_mount(
-        self, extra_mount: str
-    ) -> tuple[Path, PurePosixPath, str | None]:
-        fields = extra_mount.split(":")
-        if len(fields) < 2 or len(fields) > 3:
-            raise ValueError(f"Invalid extra mount option '{extra_mount}'")
-
-        source, target = fields[:2]
-        _valid_volume_name(source)
-
-        source_path, target_path = Path(source), PurePosixPath(target)
-        # Unlike for home_mount, we will not auto-create missing
-        # directories for extra mounts.
-        if source_path.is_absolute() and not source_path.exists():
-            raise ValueError(f"Directory '{source}' does not exist")
-
-        # By default, extra mounts are writeable
-        mode = fields[2] if len(fields) == 3 else "rw"
-        if mode not in ("ro", "rw"):
-            raise ValueError(f"Invalid extra mount mode '{mode}'")
-
-        return source_path, target_path, mode
-
     def conda_volume_name(self) -> str:
         return f"{self.container_name()}_conda"
 

tests/conftest.py

@@ -27,7 +27,7 @@
 from aiidalab_launch.application_state import ApplicationState
 from aiidalab_launch.config import Config
 from aiidalab_launch.instance import AiidaLabInstance, RequiresContainerInstance
-from aiidalab_launch.profile import Profile
+from aiidalab_launch.profile import ExtraMount, Profile
 
 
 # Redefine event_loop fixture to be session-scoped.
@@ -143,8 +143,11 @@ def instance(docker_client, profile):
 
     def remove_extra_mounts():
         for extra_mount in instance.profile.extra_mounts:
-            extra_volume, _, _ = instance.profile.parse_extra_mount(extra_mount)
-            docker_client.volumes.get(str(extra_volume)).remove()
+            mount = ExtraMount.parse_mount_string(extra_mount)
+            # NOTE: We should also remove test bind mounts, but it feels pretty dangerous,
+            # we do not want to accidentaly remove users' home (even though we monkeypatch it).
+            if mount["Type"] == "volume":
+                docker_client.volumes.get(mount["Source"]).remove()
 
     for op in (
         instance.stop,

tests/test_profile.py

@@ -1,6 +1,7 @@
 from dataclasses import replace
 from pathlib import Path
 
+import docker
 import pytest
 
 from aiidalab_launch.profile import Profile
@@ -57,7 +58,7 @@ def test_profile_init_valid_home_mounts(profile, extra_volume_name, home_mount):
 @pytest.mark.parametrize("home_mount", INVALID_HOME_MOUNTS)
 def test_profile_init_invalid_home_mounts(profile, extra_volume_name, home_mount):
     home_mount = home_mount.format(path=Path.home(), vol=extra_volume_name)
-    with pytest.raises(ValueError):
+    with pytest.raises(docker.errors.InvalidArgument):
         replace(profile, home_mount=home_mount)
 
 
@@ -70,7 +71,7 @@ def test_profile_init_valid_extra_mounts(profile, extra_volume_name, extra_mount
 @pytest.mark.parametrize("extra_mount", INVALID_EXTRA_MOUNTS)
 def test_profile_init_invalid_extra_mounts(profile, extra_volume_name, extra_mount):
     extra_mounts = {extra_mount.format(path=Path.home(), vol=extra_volume_name)}
-    with pytest.raises(ValueError):
+    with pytest.raises(docker.errors.InvalidArgument):
         replace(profile, extra_mounts=extra_mounts)