From f3148b9bd74bb08b840dcb45e5ce17736dcab981 Mon Sep 17 00:00:00 2001 From: David Ames Date: Wed, 7 Mar 2018 10:33:47 +0100 Subject: [PATCH] Run ceilometer-upgrade as an action The ceilometer-upgrade command needs to be run to update back end ceilometer data stores. When attempting to run this command during deploy time due to the number of required relations many inherent race conditions exist leading to Bug#1749280. This change allows the ceilometer-upgrade command to be run as an action post-deploy. Change-Id: I64a56d9a38532476b8a01df6227231a1276c708f Closes-Bug: #1749280 --- README.md | 5 ++ actions.yaml | 4 ++ actions/actions.py | 28 +++++++++-- actions/ceilometer-upgrade | 1 + hooks/ceilometer_hooks.py | 17 ------- lib/ceilometer_utils.py | 64 ++++++++++++++++++++++-- tests/basic_deployment.py | 76 +++++++++++++++++++---------- unit_tests/test_ceilometer_hooks.py | 42 +--------------- unit_tests/test_ceilometer_utils.py | 55 +++++++++++++++++++++ 9 files changed, 199 insertions(+), 93 deletions(-) create mode 120000 actions/ceilometer-upgrade diff --git a/README.md b/README.md index f510b3c..a0d5765 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,11 @@ for resource, metrics and measure storage: juju add-relation ceilometer gnocchi +Note: When ceilometer is related to gnocchi the ceilometer-upgrade action +must be run post deployment in order to update its data store in gnocchi. + + juju run-action ceilometer-upgrade + then Keystone and Rabbit relationships need to be established: juju add-relation ceilometer rabbitmq diff --git a/actions.yaml b/actions.yaml index dea9d08..0551b12 100644 --- a/actions.yaml +++ b/actions.yaml @@ -2,5 +2,9 @@ pause: description: Pause the Ceilometer unit. This action will stop Ceilometer services. resume: descrpition: Resume the Ceilometer unit. This action will start Ceilometer services. +ceilometer-upgrade: + description: | + Perform ceilometer-upgrade. This action will upgrade Ceilometer data stores. + *Note* This action must be run post deployment when ceilometer is related to gnocchi. openstack-upgrade: description: Perform openstack upgrades. Config option action-managed-upgrade must be set to True. diff --git a/actions/actions.py b/actions/actions.py index 82f32b9..88d5f52 100755 --- a/actions/actions.py +++ b/actions/actions.py @@ -17,11 +17,16 @@ import os import sys -from charmhelpers.core.hookenv import action_fail +from charmhelpers.core.hookenv import ( + action_fail, + action_set, +) from ceilometer_utils import ( + ceilometer_upgrade_helper, pause_unit_helper, - resume_unit_helper, register_configs, + resume_unit_helper, + FailedAction, ) @@ -40,9 +45,26 @@ def resume(args): resume_unit_helper(register_configs()) +def ceilometer_upgrade(args): + """Run ceilometer-upgrade + + @raises Exception if the ceilometer-upgrade fails. + """ + try: + ceilometer_upgrade_helper(register_configs()) + action_set({'outcome': 'success, ceilometer-upgrade completed.'}) + except FailedAction as e: + if e.outcome: + action_set({'outcome': e.outcome}) + if e.trace: + action_set({'traceback': e.trace}) + raise Exception(str(e.message)) + + # A dictionary of all the defined actions to callables (which take # parsed arguments). -ACTIONS = {"pause": pause, "resume": resume} +ACTIONS = {"pause": pause, "resume": resume, + "ceilometer-upgrade": ceilometer_upgrade} def main(args): diff --git a/actions/ceilometer-upgrade b/actions/ceilometer-upgrade new file mode 120000 index 0000000..405a394 --- /dev/null +++ b/actions/ceilometer-upgrade @@ -0,0 +1 @@ +actions.py \ No newline at end of file diff --git a/hooks/ceilometer_hooks.py b/hooks/ceilometer_hooks.py index 6e935f1..62a9654 100755 --- a/hooks/ceilometer_hooks.py +++ b/hooks/ceilometer_hooks.py @@ -72,7 +72,6 @@ from ceilometer_utils import ( set_shared_secret, assess_status, reload_systemd, - ceilometer_upgrade, ) from ceilometer_contexts import CEILOMETER_PORT from charmhelpers.contrib.openstack.ip import ( @@ -159,22 +158,6 @@ def any_changed(): for rid in relation_ids('identity-service'): keystone_joined(relid=rid) ceilometer_joined() - cmp_codename = CompareOpenStackReleases( - get_os_codename_install_source(config('openstack-origin'))) - if cmp_codename < 'queens': - identity_relation = 'identity-service' - else: - identity_relation = 'identity-credentials' - # NOTE(jamespage): ceilometer@ocata requires both gnocchi - # and mongodb to be configured to successfully - # upgrade the underlying data stores. - if ('metric-service' in CONFIGS.complete_contexts() and - identity_relation in CONFIGS.complete_contexts()): - # NOTE(jamespage): however at queens, this limitation has gone! - if (cmp_codename < 'queens' and - 'mongodb' not in CONFIGS.complete_contexts()): - return - ceilometer_upgrade() def configure_https(): diff --git a/lib/ceilometer_utils.py b/lib/ceilometer_utils.py index df968eb..8c659cd 100644 --- a/lib/ceilometer_utils.py +++ b/lib/ceilometer_utils.py @@ -15,6 +15,7 @@ import os import uuid import subprocess +import traceback from collections import OrderedDict @@ -45,10 +46,14 @@ from charmhelpers.contrib.openstack.utils import ( CompareOpenStackReleases, reset_os_release, ) -from charmhelpers.core.hookenv import config, log, is_leader +from charmhelpers.core.hookenv import ( + config, + is_leader, + log, + DEBUG, +) from charmhelpers.fetch import apt_update, apt_install, apt_upgrade from charmhelpers.core.host import init_is_systemd -from charmhelpers.core.decorators import retry_on_exception from copy import deepcopy HAPROXY_CONF = '/etc/haproxy/haproxy.cfg' @@ -524,14 +529,63 @@ def disable_package_apache_site(): subprocess.check_call(['a2dissite', 'ceilometer-api']) -@retry_on_exception(5, base_delay=60, exc_type=subprocess.CalledProcessError) -def ceilometer_upgrade(): +class FailedAction(Exception): + """ + A custom error to inform the caller that the action has failed. + Provides message, output and traceback. + """ + + def __init__(self, message, outcome=None, trace=None): + self.outcome = outcome + self.trace = trace + super(FailedAction, self).__init__(message) + + +def ceilometer_upgrade_helper(CONFIGS): + """Helper function to run ceilomter-upgrde, and then call assess_status(...) in + effect, so that the status is correctly updated. + Uses ceilomter_upgrde to do the work. + + @param configs: a templating.OSConfigRenderer() object + @returns None - this function is executed for its side-effect + """ + cmp_codename = CompareOpenStackReleases( + get_os_codename_install_source(config('openstack-origin'))) + if cmp_codename < 'queens': + identity_relation = 'identity-service' + else: + identity_relation = 'identity-credentials' + # NOTE(jamespage): ceilometer@ocata requires both gnocchi + # and mongodb to be configured to successfully + # upgrade the underlying data stores. + if ('metric-service' not in CONFIGS.complete_contexts() or + identity_relation not in CONFIGS.complete_contexts()): + raise FailedAction('The {} and or metric-service relations are not ' + 'complete. ceilometer-upgrade cannot be run until ' + 'they are ready.'.format(identity_relation)) + # NOTE(jamespage): however at queens, this limitation has gone! + if (cmp_codename < 'pike' and + 'mongodb' not in CONFIGS.complete_contexts()): + raise FailedAction('This version of ceilometer requires both gnocchi ' + 'and mongodb. Mongodb relation incomplete.') + try: + ceilometer_upgrade(action=True) + except subprocess.CalledProcessError as e: + raise FailedAction('ceilometer-upgrade resulted in an ' + 'unexpected error: {}'.format(e.message), + outcome='ceilometer-upgrade failed, see traceback.', + trace=traceback.format_exc()) + + +def ceilometer_upgrade(action=False): """Execute ceilometer-upgrade command, with retry on failure if gnocchi API is not ready for requests""" - if is_leader(): + if is_leader() or action: if (CompareOpenStackReleases(os_release('ceilometer-common')) >= 'newton'): cmd = ['ceilometer-upgrade'] else: cmd = ['ceilometer-dbsync'] + log("Running ceilomter-upgrade: {}".format(" ".join(cmd)), DEBUG) subprocess.check_call(cmd) + log("ceilometer-upgrade succeeded", DEBUG) diff --git a/tests/basic_deployment.py b/tests/basic_deployment.py index 28065d9..80ebc8b 100644 --- a/tests/basic_deployment.py +++ b/tests/basic_deployment.py @@ -69,7 +69,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): {'name': 'ceilometer-agent'}, {'name': 'nova-compute'} ] - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: other_services.extend([ {'name': 'gnocchi'}, {'name': 'memcached', 'location': 'cs:memcached'}, @@ -101,12 +101,8 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): 'glance:amqp': 'rabbitmq-server:amqp', 'nova-compute:image-service': 'glance:image-service' } - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: additional_relations = { - 'ceilometer:identity-credentials': 'keystone:' - 'identity-credentials', - 'ceilometer:identity-notifications': 'keystone:' - 'identity-notifications', 'ceilometer:metric-service': 'gnocchi:metric-service', 'ceph-mon:osd': 'ceph-osd:mon', 'gnocchi:identity-service': 'keystone:identity-service', @@ -114,12 +110,19 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): 'gnocchi:storage-ceph': 'ceph-mon:client', 'gnocchi:coordinator-memcached': 'memcached:cache', } + + if self._get_openstack_release() >= self.xenial_queens: + identity_relations = {'ceilometer:identity-credentials': + 'keystone:identity-credentials'} + else: + identity_relations = {'ceilometer:identity-service': + 'keystone:identity-service'} + additional_relations.update(identity_relations) else: additional_relations = { 'ceilometer:shared-db': 'mongodb:database', 'ceilometer:identity-service': 'keystone:identity-service'} relations.update(additional_relations) - print(relations) super(CeilometerBasicDeployment, self)._add_relations(relations) def _configure_services(self): @@ -136,7 +139,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): 'keystone': keystone_config, 'percona-cluster': pxc_config, } - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: configs['ceph-osd'] = {'osd-devices': '/dev/vdb', 'osd-reformat': 'yes', 'ephemeral-unmount': '/mnt'} @@ -154,7 +157,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): self.keystone_sentry = self.d.sentry['keystone'][0] self.rabbitmq_sentry = self.d.sentry['rabbitmq-server'][0] self.nova_sentry = self.d.sentry['nova-compute'][0] - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: self.gnocchi_sentry = self.d.sentry['gnocchi'][0] else: self.mongodb_sentry = self.d.sentry['mongodb'][0] @@ -169,7 +172,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): openstack_release=self._get_openstack_release()) self.log.debug('Instantiating ceilometer client...') - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: self.ceil = ceilo_client.Client(session=self.keystone_session,) else: # Authenticate admin with ceilometer endpoint @@ -187,10 +190,10 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): 'ceilometer-agent-central', 'ceilometer-agent-notification', ] - if release < self.xenial_queens: + if release < self.xenial_pike: ceilometer_svcs.append('ceilometer-collector') - if (release >= self.xenial_ocata and release < self.xenial_queens): + if (release >= self.xenial_ocata and release < self.xenial_pike): ceilometer_svcs.append('apache2') if release < self.xenial_ocata: @@ -211,7 +214,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): u.log.debug('OK') def test_105_memcache(self): - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: u.log.debug('Skipping memcache test as memcache server is external' ' to ceilometer') return @@ -222,7 +225,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): def test_110_service_catalog(self): """Verify that the service catalog endpoint data is valid.""" - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: u.log.debug('Skipping catalogue checks as ceilometer no longer ' 'registers endpoints') return @@ -251,7 +254,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): def test_112_keystone_api_endpoint(self): """Verify the ceilometer api endpoint data.""" - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: u.log.debug('Skipping catalogue checks as ceilometer no longer ' 'registers endpoints') return @@ -282,7 +285,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): def test_114_ceilometer_api_endpoint(self): """Verify the ceilometer api endpoint data.""" - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: u.log.debug('Skipping catalogue checks as ceilometer no longer ' 'registers endpoints') return @@ -307,7 +310,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): def test_200_ceilometer_identity_relation(self): """Verify the ceilometer to keystone identity-service relation data""" - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: u.log.debug('Skipping identity-service checks as ceilometer no ' 'longer has this rerlation') return @@ -338,7 +341,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): def test_201_keystone_ceilometer_identity_relation(self): """Verify the keystone to ceilometer identity-service relation data""" - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: u.log.debug('Skipping identity-service checks as ceilometer no ' 'longer has this rerlation') return @@ -450,9 +453,10 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): 'rabbitmq_password': u.not_null, 'port': '8767' } - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: expected['gnocchi_url'] = u.valid_url - expected['port'] = '8777' + if self._get_openstack_release() >= self.xenial_queens: + expected['port'] = '8777' else: expected['db_port'] = '27017' expected['db_name'] = 'ceilometer' @@ -495,15 +499,21 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): 'port': '8767', }, } - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: relation = self.gnocchi_sentry.relation( 'metric-service', 'ceilometer:metric-service') expected['dispatcher_gnocchi'] = {'url': relation['gnocchi_url']} - ks_rel = self.keystone_sentry.relation( - 'identity-credentials', - 'ceilometer:identity-credentials') - ks_key_prefix = 'credentials' + if self._get_openstack_release() >= self.xenial_queens: + ks_rel = self.keystone_sentry.relation( + 'identity-credentials', + 'ceilometer:identity-credentials') + ks_key_prefix = 'credentials' + else: + ks_rel = self.keystone_sentry.relation( + 'identity-service', + 'ceilometer:identity-service') + ks_key_prefix = 'service' else: db_relation = self.mongodb_sentry.relation('database', 'ceilometer:shared-db') @@ -565,7 +575,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): def test_400_api_connection(self): """Simple api calls to check service is up and responding""" - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: u.log.debug('Skipping API checks as ceilometer api has been ' 'removed') return @@ -590,7 +600,7 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): # Services which are expected to restart upon config change, # and corresponding config files affected by the change conf_file = '/etc/ceilometer/ceilometer.conf' - if self._get_openstack_release() >= self.xenial_queens: + if self._get_openstack_release() >= self.xenial_pike: services = { 'ceilometer-polling: AgentManager worker(0)': conf_file, 'ceilometer-agent-notification: NotificationService worker(0)': @@ -674,3 +684,15 @@ class CeilometerBasicDeployment(OpenStackAmuletDeployment): assert u.wait_on_action(action_id), "Resume action failed." assert u.status_get(unit)[0] == "active" u.log.debug('OK') + + def test_920_ceilometer_upgrade(self): + """Run ceilometer-upgrade""" + if self._get_openstack_release() < self.xenial_pike: + u.log.debug('Not checking ceilometer-upgrade') + return + u.log.debug('Checking ceilometer-upgrade') + unit = self.ceil_sentry + + action_id = unit.run_action("ceilometer-upgrade") + assert u.wait_on_action(action_id), "ceilometer-upgrade action failed" + u.log.debug('OK') diff --git a/unit_tests/test_ceilometer_hooks.py b/unit_tests/test_ceilometer_hooks.py index 15ac0a2..3f2a5a2 100644 --- a/unit_tests/test_ceilometer_hooks.py +++ b/unit_tests/test_ceilometer_hooks.py @@ -135,56 +135,16 @@ class CeilometerHooksTest(CharmTestCase): self.relation_set.assert_called_with( ceilometer_database='ceilometer') - @patch.object(hooks, 'ceilometer_upgrade') - @patch.object(hooks, 'keystone_joined') - @patch('charmhelpers.core.hookenv.config') - @patch.object(hooks, 'ceilometer_joined') - def test_any_changed_with_metrics(self, ceilometer_joined, mock_config, - keystone_joined, ceilometer_upgrade): - self.CONFIGS.complete_contexts.return_value = [ - 'metric-service', - 'identity-service', - 'mongodb' - ] - self.relation_ids.return_value = ['identity-service:1'] - hooks.hooks.execute(['hooks/shared-db-relation-changed']) - self.CONFIGS.write_all.assert_called_once() - ceilometer_joined.assert_called_once() - keystone_joined.assert_called_with(relid='identity-service:1') - ceilometer_upgrade.assert_called_once() - self.configure_https.assert_called_once() - - @patch.object(hooks, 'ceilometer_upgrade') - @patch.object(hooks, 'keystone_joined') - @patch('charmhelpers.core.hookenv.config') - @patch.object(hooks, 'ceilometer_joined') - def test_any_changed_queens(self, ceilometer_joined, mock_config, - keystone_joined, ceilometer_upgrade): - self.get_os_codename_install_source.return_value = 'queens' - self.CONFIGS.complete_contexts.return_value = [ - 'metric-service', - 'identity-credentials', - ] - self.relation_ids.return_value = [] - hooks.hooks.execute(['hooks/shared-db-relation-changed']) - self.CONFIGS.write_all.assert_called_once() - ceilometer_joined.assert_called_once() - keystone_joined.assert_not_called() - ceilometer_upgrade.assert_called_once() - self.configure_https.assert_called_once() - - @patch.object(hooks, 'ceilometer_upgrade') @patch.object(hooks, 'keystone_joined') @patch('charmhelpers.core.hookenv.config') @patch.object(hooks, 'ceilometer_joined') def test_any_changed(self, ceilometer_joined, mock_config, - keystone_joined, ceilometer_upgrade): + keystone_joined): self.relation_ids.return_value = ['identity-service:1'] hooks.hooks.execute(['hooks/shared-db-relation-changed']) self.assertTrue(self.CONFIGS.write_all.called) self.assertTrue(ceilometer_joined.called) keystone_joined.assert_called_with(relid='identity-service:1') - ceilometer_upgrade.assert_not_called() self.configure_https.assert_called_once() @patch('charmhelpers.core.hookenv.config') diff --git a/unit_tests/test_ceilometer_utils.py b/unit_tests/test_ceilometer_utils.py index e6367d0..b20abc9 100644 --- a/unit_tests/test_ceilometer_utils.py +++ b/unit_tests/test_ceilometer_utils.py @@ -327,3 +327,58 @@ class CeilometerUtilsTest(CharmTestCase): self.is_leader.return_value = False utils.ceilometer_upgrade() mock_subprocess.check_call.assert_not_called() + + @patch.object(utils, 'ceilometer_upgrade') + @patch('charmhelpers.core.hookenv.config') + def test_ceilometer_upgrade_helper_with_metrics(self, mock_config, + mock_ceilometer_upgrade): + self.get_os_codename_install_source.return_value = 'ocata' + self.CONFIGS = MagicMock() + self.CONFIGS.complete_contexts.return_value = [ + 'metric-service', + 'identity-service', + 'mongodb' + ] + utils.ceilometer_upgrade_helper(self.CONFIGS) + mock_ceilometer_upgrade.assert_called_once_with(action=True) + + @patch.object(utils, 'ceilometer_upgrade') + @patch('charmhelpers.core.hookenv.config') + def test_ceilometer_upgrade_helper_queens(self, mock_config, + mock_ceilometer_upgrade): + self.get_os_codename_install_source.return_value = 'queens' + self.CONFIGS = MagicMock() + self.CONFIGS.complete_contexts.return_value = [ + 'metric-service', + 'identity-credentials', + ] + utils.ceilometer_upgrade_helper(self.CONFIGS) + mock_ceilometer_upgrade.assert_called_once_with(action=True) + + @patch.object(utils, 'ceilometer_upgrade') + @patch('charmhelpers.core.hookenv.config') + def test_ceilometer_upgrade_helper_incomplete(self, mock_config, + mock_ceilometer_upgrade): + self.get_os_codename_install_source.return_value = 'ocata' + self.CONFIGS = MagicMock() + with self.assertRaises(utils.FailedAction): + utils.ceilometer_upgrade_helper(self.CONFIGS) + mock_ceilometer_upgrade.assert_not_called() + + @patch.object(utils, 'subprocess') + @patch.object(utils, 'ceilometer_upgrade') + @patch('charmhelpers.core.hookenv.config') + def test_ceilometer_upgrade_helper_raise(self, mock_config, + mock_ceilometer_upgrade, + mock_subprocess): + self.get_os_codename_install_source.return_value = 'ocata' + self.CONFIGS = MagicMock() + self.CONFIGS.complete_contexts.return_value = [ + 'metric-service', + 'identity-service', + 'mongodb' + ] + mock_ceilometer_upgrade.side_effect = utils.FailedAction("message") + with self.assertRaises(utils.FailedAction): + utils.ceilometer_upgrade_helper(self.CONFIGS) + mock_ceilometer_upgrade.assert_called_once_with(action=True)