From a2503f5191889d9f0b68396b16d26381687fe7ef Mon Sep 17 00:00:00 2001 From: Hao Liu Date: Mon, 6 Jun 2022 11:08:01 -0400 Subject: [PATCH] update how callback plugin gets copied and added to job container when in containerized environment - always copy callback plugin to private_data_dir/artifacts/{job_id}/callback - appending to `ANSIBLE_CALLBACK_PLUGINS` with the runner callback plugin location (from private_data/artifacts) - unused method `utils.callback_mount` - update unit tests Signed-off-by: Hao Liu Co-Authored-By: Alan Rominger Signed-off-by: Hao Liu Signed-off-by: Hao Liu --- ansible_runner/config/_base.py | 20 ++++++++++++++------ ansible_runner/utils/__init__.py | 21 --------------------- test/unit/config/test__base.py | 2 -- test/unit/config/test_ansible_cfg.py | 3 +-- test/unit/config/test_command.py | 2 -- test/unit/config/test_doc.py | 4 +--- test/unit/config/test_inventory.py | 3 +-- test/unit/config/test_runner.py | 15 +++++++++++++-- 8 files changed, 30 insertions(+), 40 deletions(-) diff --git a/ansible_runner/config/_base.py b/ansible_runner/config/_base.py index e3955a28f..52b90dac9 100644 --- a/ansible_runner/config/_base.py +++ b/ansible_runner/config/_base.py @@ -23,6 +23,7 @@ import re import stat import tempfile +import shutil from base64 import b64encode from uuid import uuid4 @@ -39,7 +40,6 @@ from ansible_runner.defaults import registry_auth_prefix from ansible_runner.loader import ArtifactLoader from ansible_runner.utils import ( - callback_mount, get_callback_dir, open_fifo_write, args2cmdline, @@ -224,6 +224,7 @@ def _prepare_env(self, runner_mode='pexpect'): self.env['AWX_ISOLATED_DATA_DIR'] = artifact_dir if self.fact_cache_type == 'jsonfile': self.env['ANSIBLE_CACHE_PLUGIN_CONNECTION'] = os.path.join(artifact_dir, 'fact_cache') + else: # seed env with existing shell env self.env = os.environ.copy() @@ -262,7 +263,18 @@ def _prepare_env(self, runner_mode='pexpect'): self.fact_cache = os.path.join(self.artifact_dir, self.settings['fact_cache']) # Use local callback directory - if not self.containerized: + if self.containerized: + # when containerized, copy the callback dir to $private_data_dir/artifacts//callback + # than append to env['ANSIBLE_CALLBACK_PLUGINS'] with the copyied location. + callback_dir = os.path.join(self.artifact_dir, 'callback') + # if callback dir already exist (on repeat execution with same ident), remove it first. + if os.path.exists(callback_dir): + shutil.rmtree(callback_dir) + shutil.copytree(get_callback_dir(), callback_dir) + + container_callback_dir = os.path.join("/runner/artifacts", "{}".format(self.ident), "callback") + self.env['ANSIBLE_CALLBACK_PLUGINS'] = ':'.join(filter(None, (self.env.get('ANSIBLE_CALLBACK_PLUGINS'), container_callback_dir))) + else: callback_dir = self.env.get('AWX_LIB_DIRECTORY', os.getenv('AWX_LIB_DIRECTORY')) if callback_dir is None: callback_dir = get_callback_dir() @@ -504,10 +516,6 @@ def wrap_args_for_containerization(self, args, execution_mode, cmdline_args): # custom show paths inside private_data_dir do not make sense self._update_volume_mount_paths(new_args, "{}".format(self.private_data_dir), dst_mount_path="/runner", labels=":Z") - # Mount the stdout callback plugin from the ansible-runner code base - mount_paths = callback_mount(copy_if_needed=True) - self._update_volume_mount_paths(new_args, mount_paths[0], dst_mount_path=mount_paths[1], labels=":Z") - if self.container_auth_data: # Pull in the necessary registry auth info, if there is a container cred self.registry_auth_path, registry_auth_conf_file = self._generate_container_auth_dir(self.container_auth_data) diff --git a/ansible_runner/utils/__init__.py b/ansible_runner/utils/__init__.py index 5474183f9..ba5d4e6a1 100644 --- a/ansible_runner/utils/__init__.py +++ b/ansible_runner/utils/__init__.py @@ -61,27 +61,6 @@ def is_dir_owner(directory): return bool(current_user == callback_owner) -def callback_mount(copy_if_needed=False): - ''' - Return a tuple that gives mount points for the standard out callback - in the form of (, ) - if copy_if_needed is set, and the install is owned by another user, - it will copy the plugin to a tmpdir for the mount in anticipation of SELinux problems - ''' - container_dot_ansible = '/home/runner/.ansible' - rel_path = ('callback', '',) - host_path = os.path.join(get_plugin_dir(), *rel_path) - if copy_if_needed: - callback_dir = get_callback_dir() - if not is_dir_owner(callback_dir): - tmp_path = tempfile.mkdtemp(prefix='ansible_runner_plugins_') - register_for_cleanup(tmp_path) - host_path = os.path.join(tmp_path, 'callback') - shutil.copytree(callback_dir, host_path) - container_path = os.path.join(container_dot_ansible, 'plugins', *rel_path) - return (host_path, container_path) - - class Bunch(object): ''' diff --git a/test/unit/config/test__base.py b/test/unit/config/test__base.py index 827b0e181..b66488d5c 100644 --- a/test/unit/config/test__base.py +++ b/test/unit/config/test__base.py @@ -13,7 +13,6 @@ from ansible_runner.config._base import BaseConfig, BaseExecutionMode from ansible_runner.loader import ArtifactLoader from ansible_runner.exceptions import ConfigurationError -from ansible_runner.utils import callback_mount try: Pattern = re._pattern_type @@ -331,7 +330,6 @@ def test_containerization_settings(tmp_path, runtime, mocker): expected_command_start.extend([ '-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir), '-v', '{}/:/runner/:Z'.format(rc.private_data_dir), - '-v', '{0}:{1}:Z'.format(*callback_mount()), '--env-file', '{}/env.list'.format(rc.artifact_dir), ]) diff --git a/test/unit/config/test_ansible_cfg.py b/test/unit/config/test_ansible_cfg.py index da7e7e0cb..ecc96bea7 100644 --- a/test/unit/config/test_ansible_cfg.py +++ b/test/unit/config/test_ansible_cfg.py @@ -6,7 +6,7 @@ from ansible_runner.config.ansible_cfg import AnsibleCfgConfig from ansible_runner.config._base import BaseExecutionMode from ansible_runner.exceptions import ConfigurationError -from ansible_runner.utils import get_executable_path, callback_mount +from ansible_runner.utils import get_executable_path def test_ansible_cfg_init_defaults(tmp_path, patch_private_data_dir): @@ -91,7 +91,6 @@ def test_prepare_config_command_with_containerization(tmp_path, runtime, mocker) expected_command_start.extend([ '-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir), '-v', '{}/:/runner/:Z'.format(rc.private_data_dir), - '-v', '{0}:{1}:Z'.format(*callback_mount()), '--env-file', '{}/env.list'.format(rc.artifact_dir), ]) diff --git a/test/unit/config/test_command.py b/test/unit/config/test_command.py index 5c2071ea6..75bc2db64 100644 --- a/test/unit/config/test_command.py +++ b/test/unit/config/test_command.py @@ -6,7 +6,6 @@ from ansible_runner.config.command import CommandConfig from ansible_runner.config._base import BaseExecutionMode from ansible_runner.exceptions import ConfigurationError -from ansible_runner.utils import callback_mount def test_ansible_config_defaults(tmp_path, patch_private_data_dir): @@ -106,7 +105,6 @@ def test_prepare_run_command_with_containerization(tmp_path, runtime, mocker): expected_command_start.extend([ '-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir), '-v', '{}/:/runner/:Z'.format(rc.private_data_dir), - '-v', '{0}:{1}:Z'.format(*callback_mount()), '--env-file', '{}/env.list'.format(rc.artifact_dir), ]) diff --git a/test/unit/config/test_doc.py b/test/unit/config/test_doc.py index a1dfd24f1..1058dac19 100755 --- a/test/unit/config/test_doc.py +++ b/test/unit/config/test_doc.py @@ -6,7 +6,7 @@ from ansible_runner.config.doc import DocConfig from ansible_runner.config._base import BaseExecutionMode from ansible_runner.exceptions import ConfigurationError -from ansible_runner.utils import get_executable_path, callback_mount +from ansible_runner.utils import get_executable_path def test_ansible_doc_defaults(tmp_path, patch_private_data_dir): @@ -101,7 +101,6 @@ def test_prepare_plugin_docs_command_with_containerization(tmp_path, runtime, mo expected_command_start.extend([ '-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir), '-v', '{}/:/runner/:Z'.format(rc.private_data_dir), - '-v', '{0}:{1}:Z'.format(*callback_mount()), '--env-file', '{}/env.list'.format(rc.artifact_dir), ]) @@ -170,7 +169,6 @@ def test_prepare_plugin_list_command_with_containerization(tmp_path, runtime, mo expected_command_start.extend([ '-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir), '-v', '{}/:/runner/:Z'.format(rc.private_data_dir), - '-v', '{0}:{1}:Z'.format(*callback_mount()), '--env-file', '{}/env.list'.format(rc.artifact_dir), ]) diff --git a/test/unit/config/test_inventory.py b/test/unit/config/test_inventory.py index dd81027f9..65fbe5d34 100644 --- a/test/unit/config/test_inventory.py +++ b/test/unit/config/test_inventory.py @@ -6,7 +6,7 @@ from ansible_runner.config.inventory import InventoryConfig from ansible_runner.config._base import BaseExecutionMode from ansible_runner.exceptions import ConfigurationError -from ansible_runner.utils import get_executable_path, callback_mount +from ansible_runner.utils import get_executable_path def test_ansible_inventory_init_defaults(tmp_path, patch_private_data_dir): @@ -126,7 +126,6 @@ def test_prepare_inventory_command_with_containerization(tmp_path, runtime, mock expected_command_start.extend([ '-v', '{}/artifacts/:/runner/artifacts/:Z'.format(rc.private_data_dir), '-v', '{}/:/runner/:Z'.format(rc.private_data_dir), - '-v', '{0}:{1}:Z'.format(*callback_mount()), '--env-file', '{}/env.list'.format(rc.artifact_dir), ]) diff --git a/test/unit/config/test_runner.py b/test/unit/config/test_runner.py index a05887898..91bc658c3 100644 --- a/test/unit/config/test_runner.py +++ b/test/unit/config/test_runner.py @@ -14,7 +14,6 @@ from ansible_runner.interface import init_runner from ansible_runner.loader import ArtifactLoader from ansible_runner.exceptions import ConfigurationError -from ansible_runner.utils import callback_mount try: Pattern = re._pattern_type @@ -715,6 +714,11 @@ def test_containerization_settings(tmp_path, runtime, mocker): mock_containerized = mocker.patch('ansible_runner.runner_config.RunnerConfig.containerized', new_callable=mocker.PropertyMock) mock_containerized.return_value = True + # In this test get_callback_dir() will not return a callback plugin dir that exist + # mock shutil.copytree and shutil.rmtree to just return True instead of trying to copy + mocker.patch('shutil.copytree', return_value=True) + mocker.patch('shutil.rmtree', return_value=True) + rc = RunnerConfig(tmp_path) rc.ident = 'foo' rc.playbook = 'main.yaml' @@ -725,6 +729,14 @@ def test_containerization_settings(tmp_path, runtime, mocker): rc.container_volume_mounts = ['/host1:/container1', '/host2:/container2'] rc.prepare() + # validate ANSIBLE_CALLBACK_PLUGINS env var is set + assert rc.env.get('ANSIBLE_CALLBACK_PLUGINS', None) is not None + + # validate ANSIBLE_CALLBACK_PLUGINS contains callback plugin dir + callback_plugins = rc.env['ANSIBLE_CALLBACK_PLUGINS'].split(':') + callback_dir = os.path.join("/runner/artifacts", "{}".format(rc.ident), "callback") + assert callback_dir in callback_plugins + extra_container_args = [] if runtime == 'podman': extra_container_args = ['--quiet'] @@ -733,7 +745,6 @@ def test_containerization_settings(tmp_path, runtime, mocker): expected_command_start = [runtime, 'run', '--rm', '--tty', '--interactive', '--workdir', '/runner/project'] + \ ['-v', '{}/:/runner/:Z'.format(rc.private_data_dir)] + \ - ['-v', '{0}:{1}:Z'.format(*callback_mount())] + \ ['-v', '/host1/:/container1/', '-v', '/host2/:/container2/'] + \ ['--env-file', '{}/env.list'.format(rc.artifact_dir)] + \ extra_container_args + \