From 288a400dbf30797b70e64af64f6deadcca582851 Mon Sep 17 00:00:00 2001
From: Dawud M <dawud@stackhpc.com>
Date: Wed, 15 Feb 2023 18:02:05 +0000
Subject: [PATCH] Add comprehensive checks for container restarts

When adding a dashboard to grafana the containers aren't restarted when
they should be. This is due to a bug in Kolla where the logic to
determine whether or the container needs to be restarted fails in the
case where the file does not exist in the container. This patch adds
more comprehensive checks for container restarts in the set_configs.py
file. This patch also adds a test to ensure that the functions work as
expected.

Closes-Bug: #1997984
Co-Authored-By: Will Szumski <will@stackhpc.com>
Change-Id: I67f5f12700d7b55f26bff81e9b54559303da6d83
(cherry picked from commit d9a6c5f3901e078a30d341397ac57d867d8191cc)
---
 docker/base/set_configs.py                    |  4 +-
 ...r-restart-conditions-57d36b13b0aa964b.yaml |  6 ++
 tests/test_set_config.py                      | 71 +++++++++++++++++++
 3 files changed, 80 insertions(+), 1 deletion(-)
 create mode 100644 releasenotes/notes/fixes-container-restart-conditions-57d36b13b0aa964b.yaml

diff --git a/docker/base/set_configs.py b/docker/base/set_configs.py
index b03bea7c59..4c5078cc46 100644
--- a/docker/base/set_configs.py
+++ b/docker/base/set_configs.py
@@ -162,9 +162,11 @@ class ConfigFile(object):
     def _cmp_file(self, source, dest):
         # check exist
         if (os.path.exists(source) and
-                not self.optional and
                 not os.path.exists(dest)):
             return False
+        if (os.path.exists(dest) and
+                not os.path.exists(source)):
+            return False
         # check content
         with open(source, 'rb') as f1, open(dest, 'rb') as f2:
             if f1.read() != f2.read():
diff --git a/releasenotes/notes/fixes-container-restart-conditions-57d36b13b0aa964b.yaml b/releasenotes/notes/fixes-container-restart-conditions-57d36b13b0aa964b.yaml
new file mode 100644
index 0000000000..bf6ae34747
--- /dev/null
+++ b/releasenotes/notes/fixes-container-restart-conditions-57d36b13b0aa964b.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    Fix container restart conditions to be tolerant of missing optional source
+    and/or destination files. For more details, see the following `bug report
+    <https://bugs.launchpad.net/kolla/+bug/1997984>`_
diff --git a/tests/test_set_config.py b/tests/test_set_config.py
index e74376c605..a5e09084ea 100644
--- a/tests/test_set_config.py
+++ b/tests/test_set_config.py
@@ -344,3 +344,74 @@ class ConfigFileTest(base.BaseTestCase):
         self.assertEqual([mock.call('/fake/file1', 'rb'),
                           mock.call('/fake/file2', 'rb')],
                          mock_open.call_args_list)
+
+    @mock.patch('glob.glob')
+    def test_check_non_optional_src_file_not_exists(self,
+                                                    mock_glob):
+        config_file = set_configs.ConfigFile(
+            '/var/lib/kolla/config_files/bar', '/foo', 'user1', '0644')
+
+        mock_glob.return_value = []
+
+        self.assertRaises(set_configs.MissingRequiredSource,
+                          config_file.check)
+
+    @mock.patch('glob.glob')
+    def test_check_optional_src_file_not_exists(self,
+                                                mock_glob):
+        config_file = set_configs.ConfigFile(
+            '/var/lib/kolla/config_files/bar', '/foo', 'user1', '0644',
+            optional=True)
+        mock_glob.return_value = []
+
+        self.assertIsNone(config_file.check())
+
+    @mock.patch('glob.glob')
+    @mock.patch('os.path.isdir')
+    @mock.patch.object(set_configs.ConfigFile, '_cmp_file')
+    def test_check_raises_config_bad_state(self,
+                                           mock_cmp_file,
+                                           mock_isdir,
+                                           mock_glob):
+        config_file = set_configs.ConfigFile(
+            '/var/lib/kolla/config_files/bar', '/foo', 'user1', '0644',
+            optional=True)
+        mock_cmp_file.return_value = False
+        mock_isdir.return_value = False
+        mock_glob.return_value = ['/var/lib/kolla/config_files/bar']
+
+        self.assertRaises(set_configs.ConfigFileBadState, config_file.check)
+
+    @mock.patch('os.path.exists', autospec=True)
+    def test_cmp_file_optional_src_exists_dest_no_exists(self, mock_os_exists):
+        config_file = set_configs.ConfigFile(
+            '/var/lib/kolla/config_files/bar', '/foo', 'user1', '0644',
+            optional=True)
+
+        def fake_exists(path):
+            if path == '/var/lib/kolla/config_files/bar':
+                return True
+            return False
+
+        mock_os_exists.side_effect = fake_exists
+
+        self.assertIs(False,
+                      config_file._cmp_file('/var/lib/kolla/config_files/bar',
+                                            '/foo'))
+
+    @mock.patch('os.path.exists', autospec=True)
+    def test_cmp_file_optional_src_no_exists_dest_exists(self, mock_os_exists):
+        config_file = set_configs.ConfigFile(
+            '/var/lib/kolla/config_files/bar', '/foo', 'user1', '0644',
+            optional=True)
+
+        def fake_exists(path):
+            if path == '/var/lib/kolla/config_files/bar':
+                return False
+            return True
+
+        mock_os_exists.side_effect = fake_exists
+
+        self.assertIs(False,
+                      config_file._cmp_file('/var/lib/kolla/config_files/bar',
+                                            '/foo'))