From 63b9fa56390796c93c76f3bb121015973663d9e7 Mon Sep 17 00:00:00 2001 From: Michal Arbet Date: Tue, 17 Jan 2023 13:26:24 +0100 Subject: [PATCH] Fix kolla_docker module This patch fixes kolla_docker module as it did not take into account common_options parameter. From patchset it's visible that module's default values are used always - even if user overrided some param in common_options dict. Closes-Bug: #2003079 Change-Id: I677fde708dd004decaff4bd39f2173d8d81052fb --- ansible/library/kolla_docker.py | 51 +++++-- .../notes/bug-2003079-911114b36ae745be.yaml | 7 + tests/link-module-utils.sh | 2 +- tests/test_kolla_docker.py | 140 +++++++++++++++++- 4 files changed, 179 insertions(+), 21 deletions(-) create mode 100644 releasenotes/notes/bug-2003079-911114b36ae745be.yaml diff --git a/ansible/library/kolla_docker.py b/ansible/library/kolla_docker.py index 839cc914ff..4b420bd986 100644 --- a/ansible/library/kolla_docker.py +++ b/ansible/library/kolla_docker.py @@ -275,7 +275,7 @@ def generate_module(): 'start_container', 'stop_container', 'stop_and_remove_container']), - api_version=dict(required=False, type='str', default='auto'), + api_version=dict(required=False, type='str'), auth_email=dict(required=False, type='str'), auth_password=dict(required=False, type='str', no_log=True), auth_registry=dict(required=False, type='str'), @@ -297,14 +297,14 @@ def generate_module(): cgroupns_mode=dict(required=False, type='str', choices=['private', 'host']), privileged=dict(required=False, type='bool', default=False), - graceful_timeout=dict(required=False, type='int', default=10), + graceful_timeout=dict(required=False, type='int'), remove_on_exit=dict(required=False, type='bool', default=True), restart_policy=dict(required=False, type='str', choices=[ 'no', 'on-failure', 'always', 'unless-stopped']), - restart_retries=dict(required=False, type='int', default=10), + restart_retries=dict(required=False, type='int'), state=dict(required=False, type='str', default='running', choices=['running', 'exited', @@ -318,7 +318,7 @@ def generate_module(): volumes_from=dict(required=False, type='list'), dimensions=dict(required=False, type='dict', default=dict()), tty=dict(required=False, type='bool', default=False), - client_timeout=dict(required=False, type='int', default=120), + client_timeout=dict(required=False, type='int'), ignore_missing=dict(required=False, type='bool', default=False), ) required_if = [ @@ -344,17 +344,42 @@ def generate_module(): bypass_checks=False ) - new_args = module.params.pop('common_options', dict()) + common_options_defaults = { + 'auth_email': None, + 'auth_password': None, + 'auth_registry': None, + 'auth_username': None, + 'environment': None, + 'restart_policy': None, + 'restart_retries': 10, + 'api_version': 'auto', + 'graceful_timeout': 10, + 'client_timeout': 120, + } - # NOTE(jeffrey4l): merge the environment - env = module.params.pop('environment', dict()) - if env: - new_args['environment'].update(env) + new_args = module.params.pop('common_options', dict()) or dict() + env_module_environment = module.params.pop('environment', dict()) or dict() - for key, value in module.params.items(): - if key in new_args and value is None: - continue - new_args[key] = value + for k, v in module.params.items(): + if v is None: + if k in common_options_defaults: + if k in new_args: + # From ansible groups vars the common options + # can be string or int + if isinstance(new_args[k], str) and new_args[k].isdigit(): + new_args[k] = int(new_args[k]) + continue + else: + if common_options_defaults[k] is not None: + new_args[k] = common_options_defaults[k] + else: + continue + if v is not None: + new_args[k] = v + + env_module_common_options = new_args.pop('environment', dict()) or dict() + new_args['environment'] = env_module_common_options + new_args['environment'].update(env_module_environment) # if pid_mode = ""/None/False, remove it if not new_args.get('pid_mode', False): diff --git a/releasenotes/notes/bug-2003079-911114b36ae745be.yaml b/releasenotes/notes/bug-2003079-911114b36ae745be.yaml new file mode 100644 index 0000000000..326e4de310 --- /dev/null +++ b/releasenotes/notes/bug-2003079-911114b36ae745be.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes ``kolla_docker`` module which did not take into account + the common_options parameter, so there were always module's + default values. + `LP#2003079 `__ diff --git a/tests/link-module-utils.sh b/tests/link-module-utils.sh index 70e264d42e..08819a8281 100755 --- a/tests/link-module-utils.sh +++ b/tests/link-module-utils.sh @@ -7,7 +7,7 @@ local_module_utils=${1}/ansible/module_utils -env_module_utils=${2}/ansible/module_utils +env_module_utils=$(/usr/bin/env python -c "import ansible; print(ansible.__path__[0] + '/module_utils')") for file_path in ${local_module_utils}/*.py; do file_name=$(basename ${file_path}) diff --git a/tests/test_kolla_docker.py b/tests/test_kolla_docker.py index 646f809c52..84baac5397 100644 --- a/tests/test_kolla_docker.py +++ b/tests/test_kolla_docker.py @@ -62,7 +62,7 @@ class ModuleArgsTest(base.BaseTestCase): 'start_container', 'stop_container', 'stop_and_remove_container']), - api_version=dict(required=False, type='str', default='auto'), + api_version=dict(required=False, type='str'), auth_email=dict(required=False, type='str'), auth_password=dict(required=False, type='str', no_log=True), auth_registry=dict(required=False, type='str'), @@ -83,14 +83,14 @@ class ModuleArgsTest(base.BaseTestCase): cgroupns_mode=dict(required=False, type='str', choices=['private', 'host']), privileged=dict(required=False, type='bool', default=False), - graceful_timeout=dict(required=False, type='int', default=10), + graceful_timeout=dict(required=False, type='int'), remove_on_exit=dict(required=False, type='bool', default=True), restart_policy=dict( required=False, type='str', choices=['no', 'on-failure', 'always', 'unless-stopped']), - restart_retries=dict(required=False, type='int', default=10), + restart_retries=dict(required=False, type='int'), state=dict(required=False, type='str', default='running', choices=['running', 'exited', @@ -104,7 +104,7 @@ class ModuleArgsTest(base.BaseTestCase): volumes_from=dict(required=False, type='list'), dimensions=dict(required=False, type='dict', default=dict()), tty=dict(required=False, type='bool', default=False), - client_timeout=dict(required=False, type='int', default=120), + client_timeout=dict(required=False, type='int'), healthcheck=dict(required=False, type='dict'), ignore_missing=dict(required=False, type='bool', default=False), ) @@ -138,9 +138,19 @@ class ModuleArgsTest(base.BaseTestCase): FAKE_DATA = { 'params': { + 'common_options': None, + 'api_version': None, + 'auth_username': None, + 'auth_password': None, + 'auth_registry': None, + 'restart_policy': None, + 'auth_email': None, + 'restart_retries': None, + 'graceful_timeout': None, + 'client_timeout': None, 'command': None, 'detach': True, - 'environment': {}, + 'environment': None, 'host_config': { 'network_mode': 'host', 'ipc_mode': '', @@ -162,7 +172,6 @@ FAKE_DATA = { 'remove_on_exit': True, 'volumes': None, 'tty': False, - 'client_timeout': 120, }, 'images': [ @@ -213,10 +222,71 @@ FAKE_DATA = { } +FAKE_DATA_COMMON_OPTS = { + 'auth_username': 'kevko', + 'auth_password': 'SECRET', + 'auth_registry': 'Quay.io/kolla', + 'restart_policy': 'unless-stopped', + 'auth_email': 'kevko@kevko.org', + 'restart_retries': 20, + 'environment': { + 'KOLLA_CONFIG_STRATEGY': 'COPY_ALWAYS' + }, + 'graceful_timeout': 60, + 'client_timeout': 150 +} + def get_DockerWorker(mod_param, docker_api_version='1.40'): module = mock.MagicMock() - module.params = mod_param + module.params = copy.deepcopy(mod_param) + + common_options_defaults = { + 'auth_email': None, + 'auth_password': None, + 'auth_registry': None, + 'auth_username': None, + 'environment': None, + 'restart_policy': None, + 'restart_retries': 10, + 'api_version': 'auto', + 'graceful_timeout': 10, + 'client_timeout': 120, + } + + new_args = module.params.pop('common_options', dict()) or dict() + env_module_environment = module.params.pop('environment', dict()) or dict() + + for k, v in module.params.items(): + if v is None: + if k in common_options_defaults: + if k in new_args: + # From ansible groups vars the common options + # can be string or int + if isinstance(new_args[k], str) and new_args[k].isdigit(): + new_args[k] = int(new_args[k]) + continue + else: + if common_options_defaults[k] is not None: + new_args[k] = common_options_defaults[k] + else: + continue + if v is not None: + new_args[k] = v + + env_module_common_options = new_args.pop('environment', dict()) + new_args['environment'] = env_module_common_options + new_args['environment'].update(env_module_environment) + + # if pid_mode = ""/None/False, remove it + if not new_args.get('pid_mode', False): + new_args.pop('pid_mode', None) + # if ipc_mode = ""/None/False, remove it + if not new_args.get('ipc_mode', False): + new_args.pop('ipc_mode', None) + + module.params = new_args + with mock.patch("docker.APIClient") as MockedDockerClientClass: MockedDockerClientClass.return_value._version = docker_api_version dw = dwm.DockerWorker(module) @@ -224,11 +294,21 @@ def get_DockerWorker(mod_param, docker_api_version='1.40'): return dw +def inject_env_when_create_container(container_data): + container_env = container_data.get('environment', dict()) or dict() + container_svc_name = container_data.get('name').replace('_', '-') + container_env.update({'KOLLA_SERVICE_NAME': container_svc_name}) + container_data['environment'] = container_env + + class TestMainModule(base.BaseTestCase): def setUp(self): super(TestMainModule, self).setUp() self.fake_data = copy.deepcopy(FAKE_DATA) + self.fake_data_common_opts = copy.deepcopy(FAKE_DATA) + self.fake_data_common_opts['params']['common_options'] = \ + FAKE_DATA_COMMON_OPTS @mock.patch("kolla_docker.traceback.format_exc") @mock.patch("kolla_docker_worker.get_docker_client") @@ -278,6 +358,50 @@ class TestMainModule(base.BaseTestCase): docker_api_version='1.42') self.assertTrue(self.dw._dimensions_kernel_memory_removed) + def test_common_options_defaults(self): + self.dw = get_DockerWorker(self.fake_data['params']) + self.assertEqual(self.dw.params['api_version'], 'auto') + self.assertEqual(self.dw.params['restart_retries'], 10) + self.assertEqual(self.dw.params['graceful_timeout'], 10) + self.assertEqual(self.dw.params['client_timeout'], 120) + self.assertEqual(self.dw.params['environment'], {}) + self.assertNotIn('auth_email', self.dw.params) + self.assertNotIn('auth_password', self.dw.params) + self.assertNotIn('auth_registry', self.dw.params) + self.assertNotIn('auth_username', self.dw.params) + self.assertNotIn('restart_policy', self.dw.params) + + def test_common_options(self): + self.dw = get_DockerWorker(self.fake_data_common_opts['params']) + self.assertEqual(self.dw.params['api_version'], 'auto') + self.assertEqual(self.dw.params['restart_retries'], 20) + self.assertEqual(self.dw.params['graceful_timeout'], 60) + self.assertEqual(self.dw.params['client_timeout'], 150) + self.assertEqual(self.dw.params['environment'], + {'KOLLA_CONFIG_STRATEGY': 'COPY_ALWAYS'}) + self.assertEqual(self.dw.params['auth_email'], 'kevko@kevko.org') + self.assertEqual(self.dw.params['auth_password'], 'SECRET') + self.assertEqual(self.dw.params['auth_registry'], 'Quay.io/kolla') + self.assertEqual(self.dw.params['auth_username'], 'kevko') + self.assertEqual(self.dw.params['restart_policy'], 'unless-stopped') + + def test_common_options_overriden(self): + self.fake_data_common_opts['params']['restart_retries'] = 50 + self.fake_data_common_opts['params']['graceful_timeout'] = 100 + self.fake_data_common_opts['params']['auth_email'] = 'kevko@kevko.sk' + self.dw = get_DockerWorker(self.fake_data_common_opts['params']) + self.assertEqual(self.dw.params['api_version'], 'auto') + self.assertEqual(self.dw.params['restart_retries'], 50) + self.assertEqual(self.dw.params['graceful_timeout'], 100) + self.assertEqual(self.dw.params['client_timeout'], 150) + self.assertEqual(self.dw.params['environment'], + {'KOLLA_CONFIG_STRATEGY': 'COPY_ALWAYS'}) + self.assertEqual(self.dw.params['auth_email'], 'kevko@kevko.sk') + self.assertEqual(self.dw.params['auth_password'], 'SECRET') + self.assertEqual(self.dw.params['auth_registry'], 'Quay.io/kolla') + self.assertEqual(self.dw.params['auth_username'], 'kevko') + self.assertEqual(self.dw.params['restart_policy'], 'unless-stopped') + class TestContainer(base.BaseTestCase): @@ -298,6 +422,7 @@ class TestContainer(base.BaseTestCase): self.dw.dc.create_host_config = mock.MagicMock( return_value=self.fake_data['params']['host_config']) self.dw.create_container() + inject_env_when_create_container(self.fake_data['params']) self.assertTrue(self.dw.changed) self.fake_data['params'].pop('dimensions') self.fake_data['params']['host_config']['blkio_weight'] = '10' @@ -328,6 +453,7 @@ class TestContainer(base.BaseTestCase): self.dw.dc.create_host_config = mock.MagicMock( return_value=self.fake_data['params']['host_config']) self.dw.create_container() + inject_env_when_create_container(self.fake_data['params']) self.assertTrue(self.dw.changed) expected_args = {'command', 'detach', 'environment', 'host_config', 'healthcheck', 'image', 'labels', 'name', 'tty',