Merge ~niedbalski/charm-prometheus-openstack-exporter/+git/charm-prometheus-openstack-exporter:reconfig-on-change into ~prometheus-charmers/charm-prometheus-openstack-exporter/+git/prometheus-openstack-exporter-charm:master

[Jorge Niedbalski]

This patch implements a reconfig_on_change method that validates that the event/relation data has changed prior to execute a reconfig and a subsequent service restart.

This fixes the behavior described on LP: #1835277

This patch implements a reconfig_on_change method that validates that the event/relation data has changed prior to execute a reconfig and a subsequent service restart.

This fixes the behavior described on LP: #1835277

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Felipe Reyes (freyes)]

On top of the comment I left inline, your patch was committed on top of a very old commit, please could you rebase your commit of the HEAD of the master repo?, I merged it locally and there are no conflicts, but still an extra testing would be appropriate.

review: Needs Fixing

[Jorge Niedbalski (niedbalski)]

@freyes,

Thanks for reviewing, I've rebased it against the proper branch and fixed the observations about logging INFO levels.

I'd appreciate your review.

Thanks.

[Felipe Reyes (freyes)]

LGTM, thanks for the patch, @niedbalski.

review: Approve

[Stuart Bishop (stub)]

Looks good. I think the first test could be improved per inline comment, which can be updated before landing if you agree.

review: Approve

[Jorge Niedbalski (niedbalski)]

@stub,

Yes, sure. Please add the inline comment before merging.

Thanks!

[Stuart Bishop (stub)]

Mergebot will land this branch when the merge proposal status is set to Approved. I can't make updates to the branch as I don't own it.

[Jorge Niedbalski (niedbalski)]

review: Approve

[Jorge Niedbalski (niedbalski)]

OK, reviewed @stub comments on the tests, should be ready for merge.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 6a6b506384c65f327c906078a21b3146e2183fb3

diff --git a/reactive/openstack_exporter.py b/reactive/openstack_exporter.py
index a654797..aa88150 100644
--- a/reactive/openstack_exporter.py
+++ b/reactive/openstack_exporter.py
@@ -63,6 +63,15 @@ def config_paths(key):
     return config_path
 
 
+def reconfig_on_change(key, data):
+    if not data_changed(key, data):
+        hookenv.log('{} data unchanged, skipping reconfig'.format(key), level=hookenv.DEBUG)
+        return
+    unitdata.kv().set(key, data)
+    hookenv.log('{} data changed, triggering reconfig'.format(key), level=hookenv.DEBUG)
+    set_state('exporter.do-reconfig')
+
+
 @when_not('exporter.installed')
 def install_packages():
     hookenv.status_set('maintenance', 'Installing software')
@@ -80,8 +89,7 @@ def save_swift_hosts(swift):
                    functools.reduce(lambda x, y: x + y,
                                     [s['hosts'] for s in services])]
     hookenv.log('services => {}, hosts => {}'.format(services, swift_hosts))
-    unitdata.kv().set('swift_hosts', swift_hosts)
-    set_state('exporter.do-reconfig')
+    reconfig_on_change('swift_hosts', swift_hosts)
 
 
 @when('identity-credentials.connected')
@@ -92,8 +100,7 @@ def configure_keystone_username(keystone):
 
 @when('identity-credentials.available')
 def save_creds(keystone):
-    unitdata.kv().set('keystone-relation-creds', keystone.get_creds())
-    set_state('exporter.do-reconfig')
+    reconfig_on_change('keystone-relation-creds', keystone.get_creds())
 
 
 @when_any('exporter.installed', 'exporter.do-check-reconfig')
diff --git a/unit_tests/test_reactive_openstack_exporter.py b/unit_tests/test_reactive_openstack_exporter.py
index 5691426..470d0b2 100644
--- a/unit_tests/test_reactive_openstack_exporter.py
+++ b/unit_tests/test_reactive_openstack_exporter.py
@@ -32,10 +32,7 @@ class SimpleConfigMock(dict):
 @mock.patch('os.chmod')
 @mock.patch('os.fchown')
 @mock.patch('reactive.openstack_exporter.hookenv.status_set')
-# @mock.patch('reactive.openstack_exporter.host.service_restart')
 @mock.patch('reactive.openstack_exporter.configure_keystone_username')
[e-mail]('reactive.openstack_exporter.set_state')
[e-mail]('reactive.openstack_exporter.data_changed')
 @mock.patch('reactive.openstack_exporter.hookenv.unit_get')
 @mock.patch('reactive.openstack_exporter.hookenv.config')
 class TestOpenstackExporterContext(unittest.TestCase):
@@ -118,3 +115,18 @@ class TestOpenstackExporterContext(unittest.TestCase):
     def test_restart(self, _mock_service_restart, *args):
         openstack_exporter.do_restart()
         _mock_service_restart.assert_called_once()
+
+    @mock.patch('reactive.openstack_exporter.set_state')
+    @mock.patch('reactive.openstack_exporter.data_changed')
+    def test_reconfig_on_change(self, data_changed, set_state, *args):
+        data_changed.return_value = True
+        openstack_exporter.reconfig_on_change('foo', "{'bar': 'foo'}")
+        data_changed.assert_called_once_with('foo', "{'bar': 'foo'}")
+        set_state.assert_called_once_with('exporter.do-reconfig')
+
+    @mock.patch('reactive.openstack_exporter.set_state')
+    @mock.patch('reactive.openstack_exporter.data_changed')
+    def test_reconfig_on_change_unchanged(self, data_changed, set_state, *args):
+        data_changed.return_value = False
+        openstack_exporter.reconfig_on_change('foo', "bar")
+        set_state.assert_not_called()