From bba7cff9b8c95e84b8bfe3c43b49b2fa7f9f7267 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 15 Oct 2024 16:28:08 +0200 Subject: [PATCH 1/7] allow z and Z for docker volumes --- lib/galaxy/tool_util/deps/container_classes.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/deps/container_classes.py b/lib/galaxy/tool_util/deps/container_classes.py index 769729f6bf9e..0bcce1acf3af 100644 --- a/lib/galaxy/tool_util/deps/container_classes.py +++ b/lib/galaxy/tool_util/deps/container_classes.py @@ -185,6 +185,10 @@ def parse_volume_str(rawstr: str) -> Tuple[str, str, str]: ('A', 'B', 'rw') >>> Volume.parse_volume_str('A:ro') ('A', 'A', 'ro') + >>> Volume.parse_volume_str('A:z') + ('A', 'A', 'z') + >>> Volume.parse_volume_str('A:Z') + ('A', 'A', 'Z') >>> Volume.parse_volume_str('A') ('A', 'A', 'rw') >>> Volume.parse_volume_str(' ') @@ -207,7 +211,7 @@ def parse_volume_str(rawstr: str) -> Tuple[str, str, str]: target = volume_parts[1] mode = volume_parts[2] elif len(volume_parts) == 2: - if volume_parts[1] not in ("rw", "ro", "default_ro"): + if volume_parts[1] not in ("rw", "ro", "default_ro", "z", "Z"): source = volume_parts[0] target = volume_parts[1] mode = "rw" From 753898efb7d8476d231256c56b9c7284c0f74eba Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Mon, 21 Oct 2024 13:42:10 +0200 Subject: [PATCH 2/7] allow any ordering in the mode string --- lib/galaxy/tool_util/deps/container_classes.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/deps/container_classes.py b/lib/galaxy/tool_util/deps/container_classes.py index 0bcce1acf3af..8c242605baab 100644 --- a/lib/galaxy/tool_util/deps/container_classes.py +++ b/lib/galaxy/tool_util/deps/container_classes.py @@ -211,7 +211,9 @@ def parse_volume_str(rawstr: str) -> Tuple[str, str, str]: target = volume_parts[1] mode = volume_parts[2] elif len(volume_parts) == 2: - if volume_parts[1] not in ("rw", "ro", "default_ro", "z", "Z"): + # not really parsing/checking mode here, just figuring out if the 2nd component is target or mode + mode_parts = volume_parts[1].split(",") + if any([mode_part not in ("rw", "ro", "default_ro", "z", "Z") for mode_part in mode_parts]): source = volume_parts[0] target = volume_parts[1] mode = "rw" From 8fd56f3d3555c1377eb3327fdd04c95c25f1f4f1 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Mon, 21 Oct 2024 13:49:07 +0200 Subject: [PATCH 3/7] remove unnecessary comprehension --- lib/galaxy/tool_util/deps/container_classes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/deps/container_classes.py b/lib/galaxy/tool_util/deps/container_classes.py index 8c242605baab..5d597d1979ee 100644 --- a/lib/galaxy/tool_util/deps/container_classes.py +++ b/lib/galaxy/tool_util/deps/container_classes.py @@ -213,7 +213,7 @@ def parse_volume_str(rawstr: str) -> Tuple[str, str, str]: elif len(volume_parts) == 2: # not really parsing/checking mode here, just figuring out if the 2nd component is target or mode mode_parts = volume_parts[1].split(",") - if any([mode_part not in ("rw", "ro", "default_ro", "z", "Z") for mode_part in mode_parts]): + if any(mode_part not in ("rw", "ro", "default_ro", "z", "Z") for mode_part in mode_parts): source = volume_parts[0] target = volume_parts[1] mode = "rw" From 84cc2ccc5667f29dc56991f4d8f0b04b6e546bb6 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 29 Oct 2024 14:13:27 +0100 Subject: [PATCH 4/7] also check for z in the re-duplication code --- lib/galaxy/tool_util/deps/container_volumes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tool_util/deps/container_volumes.py b/lib/galaxy/tool_util/deps/container_volumes.py index 2e969a3b4b63..0c04395d51a5 100644 --- a/lib/galaxy/tool_util/deps/container_volumes.py +++ b/lib/galaxy/tool_util/deps/container_volumes.py @@ -7,7 +7,7 @@ class ContainerVolume(metaclass=ABCMeta): - valid_modes = frozenset({"ro", "rw"}) + valid_modes = frozenset({"ro", "rw", "z"}) def __init__(self, path: str, host_path: Optional[str] = None, mode: Optional[str] = None): self.path = path @@ -50,7 +50,7 @@ def from_str(cls, as_str: str) -> "DockerVolume": kwds["path"] = kwds["host_path"] elif len(parts) == 2: # /host_path:mode is not (or is no longer?) valid Docker volume syntax - if parts[1] in DockerVolume.valid_modes: + if any(mode_part not in DockerVolume.valid_modes for mode_part in parts[1].split(",")): kwds["mode"] = parts[1] kwds["path"] = kwds["host_path"] else: From bc161f5b744349e838f1ac400f3fb9164db0a12d Mon Sep 17 00:00:00 2001 From: M Bernt Date: Wed, 30 Oct 2024 09:42:41 +0100 Subject: [PATCH 5/7] Update lib/galaxy/tool_util/deps/container_volumes.py Co-authored-by: Helena --- lib/galaxy/tool_util/deps/container_volumes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/deps/container_volumes.py b/lib/galaxy/tool_util/deps/container_volumes.py index 0c04395d51a5..45a4484abaaf 100644 --- a/lib/galaxy/tool_util/deps/container_volumes.py +++ b/lib/galaxy/tool_util/deps/container_volumes.py @@ -7,7 +7,7 @@ class ContainerVolume(metaclass=ABCMeta): - valid_modes = frozenset({"ro", "rw", "z"}) + valid_modes = frozenset({"ro", "rw", "z", "Z"}) def __init__(self, path: str, host_path: Optional[str] = None, mode: Optional[str] = None): self.path = path From 2d7cd6fcb6ab63f1b6fde28f81e9a66bd6cbc455 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Wed, 30 Oct 2024 09:54:10 +0100 Subject: [PATCH 6/7] fix logic --- lib/galaxy/tool_util/deps/container_volumes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/tool_util/deps/container_volumes.py b/lib/galaxy/tool_util/deps/container_volumes.py index 45a4484abaaf..01747bba15b8 100644 --- a/lib/galaxy/tool_util/deps/container_volumes.py +++ b/lib/galaxy/tool_util/deps/container_volumes.py @@ -50,7 +50,7 @@ def from_str(cls, as_str: str) -> "DockerVolume": kwds["path"] = kwds["host_path"] elif len(parts) == 2: # /host_path:mode is not (or is no longer?) valid Docker volume syntax - if any(mode_part not in DockerVolume.valid_modes for mode_part in parts[1].split(",")): + if all(mode_part in DockerVolume.valid_modes for mode_part in parts[1].split(",")): kwds["mode"] = parts[1] kwds["path"] = kwds["host_path"] else: From e09cba3d477150c72e53c4d0aa08552ab0ccc37d Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Wed, 30 Oct 2024 14:02:42 +0100 Subject: [PATCH 7/7] restructure to make roundtrip testable The combination - preprocess_volumes which creates Volume objects and creates strings again in the end and - creation of DockerVolumes which are only temporary and are soon converted to str again confused me and needed testing IMO. Also code was redundant. --- .../tool_util/deps/container_classes.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/tool_util/deps/container_classes.py b/lib/galaxy/tool_util/deps/container_classes.py index 5d597d1979ee..eefc3f7bf288 100644 --- a/lib/galaxy/tool_util/deps/container_classes.py +++ b/lib/galaxy/tool_util/deps/container_classes.py @@ -406,6 +406,18 @@ def add_var(name, value): return volumes_str +def _parse_volumes(volumes_raw: str, container_type: str) -> List[DockerVolume]: + """ + >>> volumes_raw = "$galaxy_root:ro,$tool_directory:ro,$job_directory:ro,$working_directory:z,$default_file_path:z" + >>> volumes = _parse_volumes(volumes_raw, "docker") + >>> [str(v) for v in volumes] + ['"$galaxy_root:$galaxy_root:ro"', '"$tool_directory:$tool_directory:ro"', '"$job_directory:$job_directory:ro"', '"$working_directory:$working_directory:z"', '"$default_file_path:$default_file_path:z"'] + """ + preprocessed_volumes_list = preprocess_volumes(volumes_raw, container_type) + # TODO: Remove redundant volumes... + return [DockerVolume.from_str(v) for v in preprocessed_volumes_list] + + class DockerContainer(Container, HasDockerLikeVolumes): container_type = DOCKER_CONTAINER_TYPE @@ -445,9 +457,7 @@ def containerize_command(self, command: str) -> str: raise Exception(f"Cannot containerize command [{working_directory}] without defined working directory.") volumes_raw = self._expand_volume_str(self.destination_info.get("docker_volumes", "$defaults")) - preprocessed_volumes_list = preprocess_volumes(volumes_raw, self.container_type) - # TODO: Remove redundant volumes... - volumes = [DockerVolume.from_str(v) for v in preprocessed_volumes_list] + volumes = _parse_volumes(volumes_raw, self.container_type) volumes_from = self.destination_info.get("docker_volumes_from", docker_util.DEFAULT_VOLUMES_FROM) docker_host_props = self.docker_host_props @@ -566,8 +576,7 @@ def containerize_command(self, command: str) -> str: raise Exception(f"Cannot containerize command [{working_directory}] without defined working directory.") volumes_raw = self._expand_volume_str(self.destination_info.get("singularity_volumes", "$defaults")) - preprocessed_volumes_list = preprocess_volumes(volumes_raw, self.container_type) - volumes = [DockerVolume.from_str(v) for v in preprocessed_volumes_list] + volumes = _parse_volumes(volumes_raw, self.container_type) run_command = singularity_util.build_singularity_run_command( command,