From 0a59a19ad20221b088843458e3b1cc8c3cdca6c3 Mon Sep 17 00:00:00 2001 From: David Ames Date: Thu, 7 Feb 2019 19:48:10 +0000 Subject: [PATCH] Use leader set for ceilometer-upgrade checks The previous attempt at addressing Bug#1811108 was a bit naive. It was not HA aware and it failed to handle upgrade-charm. This change moves from kv to leader-set as only the leader needs to run the action and then inform the other nodes. Note: on upgrade-charm we must assume the deployment has run ceilometer-upgrade as there is no mechanism to determine this independently. If we do not do this, the charm is in blocked state after charm-upgrade. A warning is logged that this assumption has occurred. Partial-Bug: #1811108 Change-Id: Idcc26df53542e78f0671942c99edfcbf61eccf6c --- hooks/ceilometer_hooks.py | 15 +++++++++++++++ hooks/leader-settings-changed | 1 + lib/ceilometer_utils.py | 23 +++++++++-------------- unit_tests/test_ceilometer_hooks.py | 14 ++++++++++++++ unit_tests/test_ceilometer_utils.py | 16 ++++++++-------- 5 files changed, 47 insertions(+), 22 deletions(-) create mode 120000 hooks/leader-settings-changed diff --git a/hooks/ceilometer_hooks.py b/hooks/ceilometer_hooks.py index 9cb6f0f..0ba8a13 100755 --- a/hooks/ceilometer_hooks.py +++ b/hooks/ceilometer_hooks.py @@ -36,6 +36,9 @@ from charmhelpers.core.hookenv import ( status_set, WARNING, DEBUG, + is_leader, + leader_get, + leader_set, ) from charmhelpers.core.host import ( service_restart, @@ -258,6 +261,18 @@ def upgrade_charm(): any_changed() for rid in relation_ids('cluster'): cluster_joined(relation_id=rid) + # NOTE: (thedac) Currently there is no method to independently check if + # ceilometer-upgrade has been run short of manual DB queries. + # On upgrade-charm the leader node must assume it has already been run + # and assert so with leader-set. If this is not done, then the upgrade from + # the previous version of the charm will leave ceilometer in a blocked + # state. + if is_leader() and relation_ids("metric-service"): + if not leader_get("ceilometer_upgrade_run"): + log("Assuming ceilometer-upgrade has been run. If this is not the " + "case, please run the ceilometer-upgrade action on the leader " + "node.", level=WARNING) + leader_set(ceilometer_upgrade_run=True) @hooks.hook('cluster-relation-joined') diff --git a/hooks/leader-settings-changed b/hooks/leader-settings-changed new file mode 120000 index 0000000..c948469 --- /dev/null +++ b/hooks/leader-settings-changed @@ -0,0 +1 @@ +ceilometer_hooks.py \ No newline at end of file diff --git a/lib/ceilometer_utils.py b/lib/ceilometer_utils.py index 940cad2..dedf01e 100644 --- a/lib/ceilometer_utils.py +++ b/lib/ceilometer_utils.py @@ -19,8 +19,6 @@ import traceback from collections import OrderedDict -from charmhelpers.core.unitdata import kv - from charmhelpers.contrib.openstack import ( templating, context, @@ -54,6 +52,8 @@ from charmhelpers.contrib.openstack.utils import ( from charmhelpers.core.hookenv import ( config, is_leader, + leader_get, + leader_set, log, DEBUG, relation_ids, @@ -77,7 +77,6 @@ POLLING_CONF = "%s/polling.yaml" % CEILOMETER_CONF_DIR CEILOMETER_API_SYSTEMD_CONF = ( '/etc/systemd/system/ceilometer-api.service.d/override.conf' ) -CEILOMETER_UPGRADED = "ceilometer-upgrade-run" HTTPS_APACHE_CONF = "/etc/apache2/sites-available/openstack_https_frontend" HTTPS_APACHE_24_CONF = "/etc/apache2/sites-available/" \ "openstack_https_frontend.conf" @@ -659,10 +658,6 @@ def ceilometer_upgrade_helper(CONFIGS): 'unexpected error: {}'.format(e.message), outcome='ceilometer-upgrade failed, see traceback.', trace=traceback.format_exc()) - kvstore = kv() - if not kvstore.get(CEILOMETER_UPGRADED, False): - kvstore.set(key=CEILOMETER_UPGRADED, value=True) - kvstore.flush() def ceilometer_upgrade(action=False): @@ -677,6 +672,7 @@ def ceilometer_upgrade(action=False): log("Running ceilomter-upgrade: {}".format(" ".join(cmd)), DEBUG) subprocess.check_call(cmd) log("ceilometer-upgrade succeeded", DEBUG) + leader_set(ceilometer_upgrade_run=True) def check_ceilometer_upgraded(configs): @@ -690,12 +686,11 @@ def check_ceilometer_upgraded(configs): :return: str, str tuple or None, None """ - if relation_ids("metric-service"): - kvstore = kv() - if not kvstore.get(CEILOMETER_UPGRADED, False): - log("Action ceilometer-upgrade not yet run, setting status " - "blocked") - return "blocked", ("Run the ceilometer-upgrade action to " - "initialize ceilometer and gnocchi") + if (relation_ids("metric-service") and not + leader_get("ceilometer_upgrade_run")): + log("Action ceilometer-upgrade not yet run, setting status " + "blocked") + return "blocked", ("Run the ceilometer-upgrade action on the " + "leader to initialize ceilometer and gnocchi") # Avoid changing status check return None, None diff --git a/unit_tests/test_ceilometer_hooks.py b/unit_tests/test_ceilometer_hooks.py index d6a7b99..b515e3b 100644 --- a/unit_tests/test_ceilometer_hooks.py +++ b/unit_tests/test_ceilometer_hooks.py @@ -73,6 +73,9 @@ TO_PATCH = [ 'get_os_codename_install_source', 'services', 'remove_old_packages', + 'is_leader', + 'leader_get', + 'leader_set', ] @@ -182,6 +185,17 @@ class CeilometerHooksTest(CharmTestCase): self.assertEqual(cluster_joined.call_count, 3) any_changed.assert_called_once() + @patch.object(hooks, 'any_changed') + @patch('charmhelpers.core.hookenv.config') + @patch.object(hooks, 'cluster_joined') + def test_upgrade_charm_set_ceilometer_upgraded( + self, cluster_joined, mock_config, any_changed): + self.is_leader.return_value = True + self.leader_get.return_value = False + self.relation_ids.return_value = ['metric-service:1'] + hooks.hooks.execute(['hooks/upgrade-charm']) + self.leader_set.assert_called_once_with(ceilometer_upgrade_run=True) + @patch('charmhelpers.core.hookenv.config') @patch.object(hooks, 'ceilometer_joined') def test_config_changed_no_upgrade(self, diff --git a/unit_tests/test_ceilometer_utils.py b/unit_tests/test_ceilometer_utils.py index 1c531a7..eef0b35 100644 --- a/unit_tests/test_ceilometer_utils.py +++ b/unit_tests/test_ceilometer_utils.py @@ -38,6 +38,8 @@ TO_PATCH = [ 'token_cache_pkgs', 'os_release', 'is_leader', + 'leader_set', + 'leader_get', 'reset_os_release', 'relation_ids', ] @@ -438,11 +440,9 @@ class CeilometerUtilsTest(CharmTestCase): utils.ceilometer_upgrade_helper(self.CONFIGS) mock_ceilometer_upgrade.assert_called_once_with(action=True) - @patch.object(utils, 'kv') - def test_check_ceilometer_upgraded(self, mock_kv): + def test_check_ceilometer_upgraded(self): self.CONFIGS = MagicMock() - _kv = MagicMock() - mock_kv.return_value = _kv + self.is_leader.return_value = True # Not related self.relation_ids.return_value = [] @@ -452,14 +452,14 @@ class CeilometerUtilsTest(CharmTestCase): # Related not ready self.relation_ids.return_value = ['metric:1'] - _kv.get.return_value = False + self.leader_get.return_value = False self.assertEqual( - ("blocked", "Run the ceilometer-upgrade action to initialize " - "ceilometer and gnocchi"), + ("blocked", "Run the ceilometer-upgrade action on the leader " + "to initialize ceilometer and gnocchi"), utils.check_ceilometer_upgraded(self.CONFIGS)) # Related ready - _kv.get.return_value = True + self.leader_get.return_value = True self.assertEqual( (None, None), utils.check_ceilometer_upgraded(self.CONFIGS))