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 <mark@stackhpc.com>

3 files changed, 149 insertions, 1 deletions

diff --git a/ansible/library/kolla_docker.py b/ansible/library/kolla_docker.py
index c76392eda..57313007a 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 000000000..16c9c41d7
--- /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 <https://launchpad.net/bugs/1848775>`__
diff --git a/tests/test_kolla_docker.py b/tests/test_kolla_docker.py
index 29db3111f..aab0f2153 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'})