From c3afbd3c5efb84a1df272bd44a46d4db60c69f0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Piliszek?= Date: Fri, 31 Jul 2020 17:54:52 +0200 Subject: [PATCH] Check config when checking the containers The proposed approach allows for checking whether config files are current, e.g. cases when the deployment was aborted after config files were generated but before they were injected into the containers which lead to old config staying in containers. After this patch we can do: kolla-ansible genconfig kolla-ansible deploy-containers and it would do what we expected rather than being a noop in the second part. We also lose the need to have notifies and whens in config and handler sections respectively. This is optimised in a separate patch. Future work: - optimise for large files - could we get away with comparing timestamps and sizes? container's should have a newer timestamp due to copy, could also preserve it Change-Id: I1d26e48e1958f13b854d8afded4bfba5021a2dec Closes-Bug: #1848775 Depends-On: https://review.opendev.org/c/openstack/kolla/+/773257 Co-Authored-By: Mark Goddard --- ansible/library/kolla_docker.py | 44 ++++++++- .../notes/bug-1848775-b0625b7586adac96.yaml | 8 ++ tests/test_kolla_docker.py | 98 +++++++++++++++++++ 3 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 releasenotes/notes/bug-1848775-b0625b7586adac96.yaml diff --git a/ansible/library/kolla_docker.py b/ansible/library/kolla_docker.py index c76392eda6..57313007a2 100644 --- a/ansible/library/kolla_docker.py +++ b/ansible/library/kolla_docker.py @@ -252,6 +252,9 @@ EXAMPLES = ''' ''' +COMPARE_CONFIG_CMD = ['/usr/local/bin/kolla_set_configs', '--check'] + + def get_docker_client(): return docker.APIClient @@ -333,7 +336,9 @@ class DockerWorker(object): def compare_container(self): container = self.check_container() - if not container or self.check_container_differs(): + if (not container or + self.check_container_differs() or + self.compare_config()): self.changed = True return self.changed @@ -583,6 +588,43 @@ class DockerWorker(object): if current_healthcheck: return True + def compare_config(self): + try: + job = self.dc.exec_create( + self.params['name'], + COMPARE_CONFIG_CMD, + user='root', + ) + output = self.dc.exec_start(job) + exec_inspect = self.dc.exec_inspect(job) + except docker.errors.APIError as e: + # NOTE(yoctozepto): If we have a client error, then the container + # cannot be used for config check (e.g., is restarting, or stopped + # in the mean time) - assume config is stale = return True. + # Else, propagate the server error back. + if e.is_client_error(): + return True + else: + raise + # Exit codes: + # 0: not changed + # 1: changed + # 137: abrupt exit -> changed + # else: error + if exec_inspect['ExitCode'] == 0: + return False + elif exec_inspect['ExitCode'] == 1: + return True + elif exec_inspect['ExitCode'] == 137: + # NOTE(yoctozepto): This is Docker's command exit due to container + # exit. It means the container is unstable so we are better off + # marking it as requiring a restart due to config update. + return True + else: + raise Exception('Failed to compare container configuration: ' + 'ExitCode: %s Message: %s' % + (exec_inspect['ExitCode'], output)) + def parse_image(self): full_image = self.params.get('image') diff --git a/releasenotes/notes/bug-1848775-b0625b7586adac96.yaml b/releasenotes/notes/bug-1848775-b0625b7586adac96.yaml new file mode 100644 index 0000000000..16c9c41d7b --- /dev/null +++ b/releasenotes/notes/bug-1848775-b0625b7586adac96.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixes an issue where configuration in containers could become stale. + This prevented containers with updated configuration from being + restarted, e.g., if the ``kolla-ansible genconfig`` and + ``kolla-ansible deploy-containers`` commands were used together. + `LP#1848775 `__ diff --git a/tests/test_kolla_docker.py b/tests/test_kolla_docker.py index 29db3111f4..aab0f21533 100644 --- a/tests/test_kolla_docker.py +++ b/tests/test_kolla_docker.py @@ -709,6 +709,104 @@ class TestImage(base.BaseTestCase): self.dw.dc.images.assert_called_once_with() self.assertTrue(return_data) + def test_compare_config_unchanged(self): + self.dw = get_DockerWorker(FAKE_DATA['params']) + job = mock.MagicMock() + self.dw.dc.exec_create.return_value = job + self.dw.dc.exec_start.return_value = 'fake output' + self.dw.dc.exec_inspect.return_value = {'ExitCode': 0} + return_data = self.dw.compare_config() + self.dw.dc.exec_create.assert_called_once_with( + FAKE_DATA['params']['name'], + kd.COMPARE_CONFIG_CMD, + user='root') + self.dw.dc.exec_start.assert_called_once_with(job) + self.dw.dc.exec_inspect.assert_called_once_with(job) + self.assertFalse(return_data) + + def test_compare_config_changed(self): + self.dw = get_DockerWorker(FAKE_DATA['params']) + job = mock.MagicMock() + self.dw.dc.exec_create.return_value = job + self.dw.dc.exec_start.return_value = 'fake output' + self.dw.dc.exec_inspect.return_value = {'ExitCode': 1} + return_data = self.dw.compare_config() + self.dw.dc.exec_create.assert_called_once_with( + FAKE_DATA['params']['name'], + kd.COMPARE_CONFIG_CMD, + user='root') + self.dw.dc.exec_start.assert_called_once_with(job) + self.dw.dc.exec_inspect.assert_called_once_with(job) + self.assertTrue(return_data) + + def test_compare_config_changed_container_exited(self): + self.dw = get_DockerWorker(FAKE_DATA['params']) + job = mock.MagicMock() + self.dw.dc.exec_create.return_value = job + self.dw.dc.exec_start.return_value = 'fake output' + self.dw.dc.exec_inspect.return_value = {'ExitCode': 137} + return_data = self.dw.compare_config() + self.dw.dc.exec_create.assert_called_once_with( + FAKE_DATA['params']['name'], + kd.COMPARE_CONFIG_CMD, + user='root') + self.dw.dc.exec_start.assert_called_once_with(job) + self.dw.dc.exec_inspect.assert_called_once_with(job) + self.assertTrue(return_data) + + def test_compare_config_changed_client_failure(self): + self.dw = get_DockerWorker(FAKE_DATA['params']) + job = mock.MagicMock() + self.dw.dc.exec_create.return_value = job + self.dw.dc.exec_start.return_value = 'fake output' + failure_response = mock.MagicMock() + failure_response.status_code = 409 # any client error should do here + self.dw.dc.exec_inspect.side_effect = docker_error.APIError( + message="foo", + response=failure_response, + ) + return_data = self.dw.compare_config() + self.dw.dc.exec_create.assert_called_once_with( + FAKE_DATA['params']['name'], + kd.COMPARE_CONFIG_CMD, + user='root') + self.dw.dc.exec_start.assert_called_once_with(job) + self.dw.dc.exec_inspect.assert_called_once_with(job) + self.assertTrue(return_data) + + def test_compare_config_error(self): + self.dw = get_DockerWorker(FAKE_DATA['params']) + job = mock.MagicMock() + self.dw.dc.exec_create.return_value = job + self.dw.dc.exec_start.return_value = 'fake output' + self.dw.dc.exec_inspect.return_value = {'ExitCode': -1} + self.assertRaises(Exception, self.dw.compare_config) # noqa: H202 + self.dw.dc.exec_create.assert_called_once_with( + FAKE_DATA['params']['name'], + kd.COMPARE_CONFIG_CMD, + user='root') + self.dw.dc.exec_start.assert_called_once_with(job) + self.dw.dc.exec_inspect.assert_called_once_with(job) + + def test_compare_config_error_server_failure(self): + self.dw = get_DockerWorker(FAKE_DATA['params']) + job = mock.MagicMock() + self.dw.dc.exec_create.return_value = job + self.dw.dc.exec_start.return_value = 'fake output' + failure_response = mock.MagicMock() + failure_response.status_code = 500 # any server error should do here + self.dw.dc.exec_inspect.side_effect = docker_error.APIError( + message="foo", + response=failure_response, + ) + self.assertRaises(docker_error.APIError, self.dw.compare_config) + self.dw.dc.exec_create.assert_called_once_with( + FAKE_DATA['params']['name'], + kd.COMPARE_CONFIG_CMD, + user='root') + self.dw.dc.exec_start.assert_called_once_with(job) + self.dw.dc.exec_inspect.assert_called_once_with(job) + def test_get_image_id_not_exists(self): self.dw = get_DockerWorker( {'image': 'myregistrydomain.com:5000/ubuntu:16.04'})