clean up _update_volume_mount_paths, _handle_automounts

src/ansible_runner/config/_base.py

@@ -92,6 +92,9 @@ def __init__(self,
                  ):
         # pylint: disable=W0613
 
+        # Currently mounted container items
+        self._mounted_volumes: list[str] = []
+
         # common params
         self.host_cwd = host_cwd
         self.envvars = envvars
@@ -168,6 +171,13 @@ def __init__(self,
     def containerized(self):
         return self.process_isolation and self.process_isolation_executable in self._CONTAINER_ENGINES
 
+    def _volume_is_mounted(self, volume: str):
+        return volume in self._mounted_volumes
+
+    def _record_volume_mount(self, volume: str):
+        if not self._volume_is_mounted(volume):
+            self._mounted_volumes.append(volume)
+
     def prepare_env(self, runner_mode: str = 'pexpect') -> None:
         """
         Manages reading environment metadata files under ``private_data_dir`` and merging/updating
@@ -392,11 +402,10 @@ def _get_playbook_path(self, cmdline_args: list[str]) -> str | None:
         return _playbook
 
     def _update_volume_mount_paths(self,
-                                   args_list: list[str],
                                    src_mount_path: str | None,
                                    dst_mount_path: str | None = None,
                                    labels: str | None = None
-                                   ) -> None:
+                                   ) -> list[str]:
 
         if src_mount_path is None or not os.path.exists(src_mount_path):
             logger.debug("Source volume mount path does not exist: %s", src_mount_path)
@@ -441,8 +450,11 @@ def _update_volume_mount_paths(self,
             volume_mount_path += labels
 
         # check if mount path already added in args list
-        if volume_mount_path not in args_list:
-            args_list.extend(["-v", volume_mount_path])
+        if not self._volume_is_mounted(volume_mount_path):
+            self._record_volume_mount(volume_mount_path)
+            return ["-v", volume_mount_path]
+
+        return []
 
     def _handle_ansible_cmd_options_bind_mounts(self, args_list: list[str], cmdline_args: list[str]) -> None:
         inventory_file_options = ['-i', '--inventory', '--inventory-file']
@@ -461,7 +473,7 @@ def _handle_ansible_cmd_options_bind_mounts(self, args_list: list[str], cmdline_
             if 'ansible-playbook' in value:
                 playbook_file_path = self._get_playbook_path(cmdline_args)
                 if playbook_file_path:
-                    self._update_volume_mount_paths(args_list, playbook_file_path)
+                    args_list.extend(self._update_volume_mount_paths(playbook_file_path))
                     break
 
         cmdline_args_copy = cmdline_args.copy()
@@ -485,7 +497,7 @@ def _handle_ansible_cmd_options_bind_mounts(self, args_list: list[str], cmdline_
                 # comma separated host list provided as value
                 continue
 
-            self._update_volume_mount_paths(args_list, optional_arg_value)
+            args_list.extend(self._update_volume_mount_paths(optional_arg_value))
 
     def wrap_args_for_containerization(self,
                                        args: list[str],
@@ -505,7 +517,7 @@ def wrap_args_for_containerization(self,
         elif self.host_cwd is not None and os.path.exists(self.host_cwd):
             # mount current host working diretory if passed and exist
             self._ensure_path_safe_to_mount(self.host_cwd)
-            self._update_volume_mount_paths(new_args, self.host_cwd)
+            new_args.extend(self._update_volume_mount_paths(self.host_cwd))
             workdir = self.host_cwd
         else:
             workdir = "/runner/project"
@@ -521,7 +533,7 @@ def wrap_args_for_containerization(self,
                 self._handle_ansible_cmd_options_bind_mounts(new_args, cmdline_args)
 
             # Handle automounts for .ssh config
-            self._handle_automounts(new_args)
+            new_args.extend(self._handle_automounts())
 
             if 'podman' in self.process_isolation_executable:
                 # container namespace stuff
@@ -536,10 +548,12 @@ def wrap_args_for_containerization(self,
                     os.mkdir(subdir_path, 0o700)
 
             # runtime commands need artifacts mounted to output data
-            self._update_volume_mount_paths(new_args,
-                                            f"{self.private_data_dir}/artifacts",
-                                            dst_mount_path="/runner/artifacts",
-                                            labels=":Z")
+            new_args.extend(
+                self._update_volume_mount_paths(
+                    f"{self.private_data_dir}/artifacts",
+                    dst_mount_path="/runner/artifacts",
+                    labels=":Z")
+            )
 
         else:
             subdir_path = os.path.join(self.private_data_dir, 'artifacts')
@@ -548,7 +562,9 @@ def wrap_args_for_containerization(self,
 
         # Mount the entire private_data_dir
         # custom show paths inside private_data_dir do not make sense
-        self._update_volume_mount_paths(new_args, self.private_data_dir, dst_mount_path="/runner", labels=":Z")
+        new_args.extend(
+            self._update_volume_mount_paths(self.private_data_dir, dst_mount_path="/runner", labels=":Z")
+        )
 
         if self.container_auth_data:
             # Pull in the necessary registry auth info, if there is a container cred
@@ -571,7 +587,9 @@ def wrap_args_for_containerization(self,
                 labels = None
                 if len(volume_mounts) == 3:
                     labels = f":{volume_mounts[2]}"
-                self._update_volume_mount_paths(new_args, volume_mounts[0], dst_mount_path=volume_mounts[1], labels=labels)
+                new_args.extend(
+                    self._update_volume_mount_paths(volume_mounts[0], dst_mount_path=volume_mounts[1], labels=labels)
+                )
 
         # Reference the file with list of keys to pass into container
         # this file will be written in ansible_runner.runner
@@ -664,7 +682,9 @@ def wrap_args_with_ssh_agent(self,
             args.extend(['sh', '-c', cmd])
         return args
 
-    def _handle_automounts(self, new_args: list[str]) -> None:
+    def _handle_automounts(self) -> list[str]:
+        new_args: list[str] = []
+
         for cli_automount in cli_mounts():
             for env in cli_automount['ENVS']:
                 if env in os.environ:
@@ -678,10 +698,16 @@ def _handle_automounts(self, new_args: list[str]) -> None:
                         else:
                             dest_path = os.environ[env]
 
-                        self._update_volume_mount_paths(new_args, os.environ[env], dst_mount_path=dest_path)
+                        new_args.extend(
+                            self._update_volume_mount_paths(os.environ[env], dst_mount_path=dest_path)
+                        )
 
                     new_args.extend(["-e", f"{env}={dest_path}"])
 
             for paths in cli_automount['PATHS']:
                 if os.path.exists(paths['src']):
-                    self._update_volume_mount_paths(new_args, paths['src'], dst_mount_path=paths['dest'])
+                    new_args.extend(
+                        self._update_volume_mount_paths(paths['src'], dst_mount_path=paths['dest'])
+                    )
+
+        return new_args

test/unit/config/test__base.py

@@ -22,6 +22,19 @@ def load_file_side_effect(path, value, *args, **kwargs):
     raise ConfigurationError
 
 
+def test_base_config_volume_mounts():
+    # pylint: disable=W0212
+    base = BaseConfig()
+    base._record_volume_mount("src1:dst1:Z")
+    base._record_volume_mount("src1:dst1:Z")
+    base._record_volume_mount("src2:dst2")
+
+    # Volumes should not be duplicated
+    assert base._mounted_volumes == ["src1:dst1:Z", "src2:dst2"]
+    assert base._volume_is_mounted("src1:dst1:Z")
+    assert base._volume_is_mounted("src2:dst2")
+
+
 def test_base_config_init_defaults(tmp_path):
     rc = BaseConfig(private_data_dir=tmp_path.as_posix())
     assert rc.private_data_dir == tmp_path.as_posix()

test/unit/config/test_container_volmount_generation.py

@@ -77,7 +77,7 @@ def test_check_not_safe_to_mount_dir(not_safe, mocker):
         bc = BaseConfig()
         mocker.patch("os.path.exists", return_value=True)
         bc._update_volume_mount_paths(
-            args_list=[], src_mount_path=not_safe, dst_mount_path=None
+            src_mount_path=not_safe, dst_mount_path=None
         )
 
 
@@ -89,7 +89,7 @@ def test_check_not_safe_to_mount_file(not_safe, mocker):
         bc = BaseConfig()
         mocker.patch("os.path.exists", return_value=True)
         bc._update_volume_mount_paths(
-            args_list=[], src_mount_path=file_path, dst_mount_path=None
+            src_mount_path=file_path, dst_mount_path=None
         )
 
 
@@ -101,20 +101,23 @@ def test_duplicate_detection_dst(path, mocker):
     base_config = BaseConfig()
 
     def generate():
+        vols = []
         for entry in dir_variations:
             for label in labels:
-                base_config._update_volume_mount_paths(
-                    args_list=first_pass,
-                    src_mount_path=path.path,
-                    dst_mount_path=entry.path,
-                    labels=label,
+                vols.extend(
+                    base_config._update_volume_mount_paths(
+                        src_mount_path=path.path,
+                        dst_mount_path=entry.path,
+                        labels=label,
+                    )
                 )
+        return vols
 
-    first_pass = []
-    generate()
-    second_pass = first_pass[:]
-    generate()
-    assert first_pass == second_pass
+    first_pass = generate()
+    second_pass = generate()
+
+    assert not second_pass
+    assert first_pass != second_pass
 
 
 @pytest.mark.parametrize("labels", labels, ids=id_for_label)
@@ -127,9 +130,8 @@ def test_no_dst_all_dirs(path, labels, mocker):
     dst_str = src_str
     expected = generate_volmount_args(src_str=src_str, dst_str=dst_str, vol_labels=labels)
 
-    result = []
-    BaseConfig()._update_volume_mount_paths(
-        args_list=result, src_mount_path=path.path, dst_mount_path=None, labels=labels
+    result = BaseConfig()._update_volume_mount_paths(
+        src_mount_path=path.path, dst_mount_path=None, labels=labels
     )
 
     explanation = (
@@ -152,9 +154,8 @@ def test_src_dst_all_dirs(src, dst, labels, mocker):
     dst_str = os.path.join(resolve_path(dst.path), "")
     expected = generate_volmount_args(src_str=src_str, dst_str=dst_str, vol_labels=labels)
 
-    result = []
-    BaseConfig()._update_volume_mount_paths(
-        args_list=result, src_mount_path=src.path, dst_mount_path=dst.path, labels=labels
+    result = BaseConfig()._update_volume_mount_paths(
+        src_mount_path=src.path, dst_mount_path=dst.path, labels=labels
     )
 
     explanation = (
@@ -174,15 +175,14 @@ def test_src_dst_all_files(path, labels, mocker):
     dst_str = src_str
     expected = generate_volmount_args(src_str=src_str, dst_str=dst_str, vol_labels=labels)
 
-    result = []
     src_file = os.path.join(path.path, "", "file.txt")
     dest_file = src_file
 
     base_config = BaseConfig()
     mocker.patch("os.path.exists", return_value=True)
     mocker.patch("os.path.isdir", return_value=False)
-    base_config._update_volume_mount_paths(
-        args_list=result, src_mount_path=src_file, dst_mount_path=dest_file, labels=labels
+    result = base_config._update_volume_mount_paths(
+        src_mount_path=src_file, dst_mount_path=dest_file, labels=labels
     )
 
     explanation = (
@@ -209,9 +209,8 @@ def test_src_dst_all_relative_dirs(src, dst, labels, relative, mocker):
     dst_str = os.path.join(resolve_path(os.path.join(workdir, relative_dst)), "")
     expected = generate_volmount_args(src_str=src_str, dst_str=dst_str, vol_labels=labels)
 
-    result = []
-    BaseConfig(container_workdir=workdir)._update_volume_mount_paths(
-        args_list=result, src_mount_path=relative_src, dst_mount_path=relative_dst, labels=labels
+    result = BaseConfig(container_workdir=workdir)._update_volume_mount_paths(
+        src_mount_path=relative_src, dst_mount_path=relative_dst, labels=labels
     )
 
     explanation = (