From 43a23127949e035506b89afd6fb01b5cfec04b63 Mon Sep 17 00:00:00 2001 From: Aurelien Lourot Date: Mon, 29 Nov 2021 15:17:44 +0100 Subject: [PATCH] Remove pause/resume logic This is a subordinate charm and since a recent commit [1] it shares a list of its services with the principal charm nova-compute, which has now the responsibility to pause and resume services. [2] The ceilometer-agent-compute service has a dependency to the nova-compute service anyway, so it is impossible for this charm to resume its service if its principal charm nova-compute is paused. This is what also led to errors in ceilometer-agent's post-series-upgrade hook. This hook attempted to resume its service although the principal service was still paused. Removing this logic entirely solves this issue. Validated by running openstack-upgrade and series-upgrade tests. [3] [1]: https://opendev.org/openstack/charm-ceilometer-agent/commit/be45f779 [2]: https://opendev.org/openstack/charm-nova-compute/commit/8fb37dc0 [3]: https://github.com/openstack-charmers/charmed-openstack-tester Closes-Bug: #1952882 Change-Id: Ia22b53b52b541250f7f803c6708968d75e64475c --- actions.yaml | 4 -- actions/.gitkeep | 0 actions/actions.py | 75 --------------------------- actions/pause | 1 - actions/resume | 1 - hooks/ceilometer_hooks.py | 37 ++++---------- hooks/ceilometer_utils.py | 36 ------------- unit_tests/test_actions.py | 78 ----------------------------- unit_tests/test_ceilometer_hooks.py | 13 ----- unit_tests/test_ceilometer_utils.py | 19 ------- 10 files changed, 11 insertions(+), 253 deletions(-) delete mode 100644 actions.yaml create mode 100644 actions/.gitkeep delete mode 100755 actions/actions.py delete mode 120000 actions/pause delete mode 120000 actions/resume delete mode 100644 unit_tests/test_actions.py diff --git a/actions.yaml b/actions.yaml deleted file mode 100644 index 4caee58..0000000 --- a/actions.yaml +++ /dev/null @@ -1,4 +0,0 @@ -pause: - description: Pause the ceilometer-agent unit. This action will stop ceilometer-agent services. -resume: - descrpition: Resume the ceilometer-agent unit. This action will start ceilometer-agent services. diff --git a/actions/.gitkeep b/actions/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/actions/actions.py b/actions/actions.py deleted file mode 100755 index 1d423d5..0000000 --- a/actions/actions.py +++ /dev/null @@ -1,75 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright 2016 Canonical Ltd -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -import os -import sys - - -_path = os.path.dirname(os.path.realpath(__file__)) -_hooks = os.path.abspath(os.path.join(_path, '../hooks')) -_root = os.path.abspath(os.path.join(_path, '..')) - - -def _add_path(path): - if path not in sys.path: - sys.path.insert(1, path) - - -_add_path(_hooks) -_add_path(_root) - - -from charmhelpers.core.hookenv import action_fail -from ceilometer_utils import ( - pause_unit_helper, - resume_unit_helper, - register_configs, -) - - -def pause(args): - """Pause the Ceilometer services. - @raises Exception should the service fail to stop. - """ - pause_unit_helper(register_configs()) - - -def resume(args): - """Resume the Ceilometer services. - @raises Exception should the service fail to start.""" - resume_unit_helper(register_configs()) - - -# A dictionary of all the defined actions to callables (which take -# parsed arguments). -ACTIONS = {"pause": pause, "resume": resume} - - -def main(args): - action_name = os.path.basename(args[0]) - try: - action = ACTIONS[action_name] - except KeyError: - return "Action %s undefined" % action_name - else: - try: - action(args) - except Exception as e: - action_fail(str(e)) - - -if __name__ == "__main__": - sys.exit(main(sys.argv)) diff --git a/actions/pause b/actions/pause deleted file mode 120000 index 405a394..0000000 --- a/actions/pause +++ /dev/null @@ -1 +0,0 @@ -actions.py \ No newline at end of file diff --git a/actions/resume b/actions/resume deleted file mode 120000 index 405a394..0000000 --- a/actions/resume +++ /dev/null @@ -1 +0,0 @@ -actions.py \ No newline at end of file diff --git a/hooks/ceilometer_hooks.py b/hooks/ceilometer_hooks.py index b5c238d..81b11ec 100755 --- a/hooks/ceilometer_hooks.py +++ b/hooks/ceilometer_hooks.py @@ -48,9 +48,6 @@ from charmhelpers.core.host import ( ) from charmhelpers.contrib.openstack.utils import ( pausable_restart_on_change as restart_on_change, - is_unit_paused_set, - series_upgrade_prepare, - series_upgrade_complete, ) from ceilometer_utils import ( restart_map, @@ -60,8 +57,6 @@ from ceilometer_utils import ( assess_status, get_packages, releases_packages_map, - pause_unit_helper, - resume_unit_helper, remove_old_packages, ) from charmhelpers.contrib.charmsupport import nrpe @@ -107,11 +102,21 @@ def upgrade_charm(): apt_install( filter_installed_packages(get_packages()), fatal=True) + packages_removed = remove_old_packages() - if packages_removed and not is_unit_paused_set(): + if packages_removed: + # NOTE(lourot): this code path is run when python2 packages have been + # purged and python3 packages installed. In this case it is best to + # restart the ceilometer-agent-compute service. log("Package purge detected, restarting services", "INFO") for s in services(): + # NOTE(lourot): if the principal nova-compute unit is paused, the + # ceilometer-agent-compute service, depending on the nova-compute + # service, will refuse to start, but this call won't raise. It'll + # just return False. This can safely be ignored: all services will + # finally be started when unpausing nova-compute. service_restart(s) + # NOTE(jamespage): Ensure any changes to nova presented data are made # during charm upgrades. for rid in relation_ids('nova-ceilometer'): @@ -122,12 +127,6 @@ def upgrade_charm(): @hooks.hook('config-changed') @restart_on_change(restart_map(), stopstart=True) def config_changed(): - # if we are paused, delay doing any config changed hooks. - # It is forced on the resume. - if is_unit_paused_set(): - log("Unit is pause or upgrading. Skipping config_changed", "WARN") - return - apt_install(filter_installed_packages(get_packages()), fatal=True) if is_relation_made('nrpe-external-master'): update_nrpe_config() @@ -146,20 +145,6 @@ def update_nrpe_config(): nrpe_setup.write() -@hooks.hook('pre-series-upgrade') -def pre_series_upgrade(): - log("Running prepare series upgrade hook", "INFO") - series_upgrade_prepare( - pause_unit_helper, CONFIGS) - - -@hooks.hook('post-series-upgrade') -def post_series_upgrade(): - log("Running complete series upgrade hook", "INFO") - series_upgrade_complete( - resume_unit_helper, CONFIGS) - - @hooks.hook('amqp-relation-joined') def amqp_joined(relation_id=None): relation_set(relation_id=relation_id, diff --git a/hooks/ceilometer_utils.py b/hooks/ceilometer_utils.py index 53fbcbe..730e8c1 100644 --- a/hooks/ceilometer_utils.py +++ b/hooks/ceilometer_utils.py @@ -24,8 +24,6 @@ from ceilometer_contexts import ( from charmhelpers.contrib.openstack.utils import ( get_os_codename_package, make_assess_status_func, - pause_unit, - resume_unit, os_application_version_set, token_cache_pkgs, enable_memcache, @@ -288,40 +286,6 @@ def assess_status_func(configs): services=services(), ports=None) -def pause_unit_helper(configs): - """Helper function to pause a unit, and then call assess_status(...) in - effect, so that the status is correctly updated. - Uses charmhelpers.contrib.openstack.utils.pause_unit() to do the work. - @param configs: a templating.OSConfigRenderer() object - @returns None - this function is executed for its side-effect - """ - _pause_resume_helper(pause_unit, configs) - - -def resume_unit_helper(configs): - """Helper function to resume a unit, and then call assess_status(...) in - effect, so that the status is correctly updated. - Uses charmhelpers.contrib.openstack.utils.resume_unit() to do the work. - @param configs: a templating.OSConfigRenderer() object - @returns None - this function is executed for its side-effect - """ - _pause_resume_helper(resume_unit, configs) - - -def _pause_resume_helper(f, configs): - """Helper function that uses the make_assess_status_func(...) from - charmhelpers.contrib.openstack.utils to create an assess_status(...) - function that can be used with the pause/resume of the unit - @param f: the function to be used with the assess_status(...) function - @returns None - this function is executed for its side-effect - """ - # TODO(ajkavanagh) - ports= has been left off because of the race hazard - # that exists due to service_start() - f(assess_status_func(configs), - services=services(), - ports=None) - - def _get_current_release(): return (get_os_codename_package('ceilometer-common', fatal=False) or 'icehouse') diff --git a/unit_tests/test_actions.py b/unit_tests/test_actions.py deleted file mode 100644 index d921671..0000000 --- a/unit_tests/test_actions.py +++ /dev/null @@ -1,78 +0,0 @@ -# Copyright 2016 Canonical Ltd -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -from unittest import mock -from unittest.mock import patch - -from test_utils import CharmTestCase - -with patch('ceilometer_utils.register_configs') as configs: - configs.return_value = 'test-config' - import actions - - -class PauseTestCase(CharmTestCase): - - def setUp(self): - super(PauseTestCase, self).setUp( - actions, ["pause_unit_helper"]) - - def test_pauses_services(self): - actions.pause([]) - self.pause_unit_helper.assert_called_once_with('test-config') - - -class ResumeTestCase(CharmTestCase): - - def setUp(self): - super(ResumeTestCase, self).setUp( - actions, ["resume_unit_helper"]) - - def test_pauses_services(self): - actions.resume([]) - self.resume_unit_helper.assert_called_once_with('test-config') - - -class MainTestCase(CharmTestCase): - - def setUp(self): - super(MainTestCase, self).setUp(actions, ["action_fail"]) - - def test_invokes_action(self): - dummy_calls = [] - - def dummy_action(args): - dummy_calls.append(True) - - with mock.patch.dict(actions.ACTIONS, {"foo": dummy_action}): - actions.main(["foo"]) - self.assertEqual(dummy_calls, [True]) - - def test_unknown_action(self): - """Unknown actions aren't a traceback.""" - exit_string = actions.main(["foo"]) - self.assertEqual("Action foo undefined", exit_string) - - def test_failing_action(self): - """Actions which traceback trigger action_fail() calls.""" - dummy_calls = [] - - self.action_fail.side_effect = dummy_calls.append - - def dummy_action(args): - raise ValueError("uh oh") - - with mock.patch.dict(actions.ACTIONS, {"foo": dummy_action}): - actions.main(["foo"]) - self.assertEqual(dummy_calls, ["uh oh"]) diff --git a/unit_tests/test_ceilometer_hooks.py b/unit_tests/test_ceilometer_hooks.py index 03e3df4..b60a7ad 100644 --- a/unit_tests/test_ceilometer_hooks.py +++ b/unit_tests/test_ceilometer_hooks.py @@ -32,7 +32,6 @@ TO_PATCH = [ 'releases_packages_map', 'services', 'is_relation_made', - 'is_unit_paused_set', 'relation_set', 'update_nrpe_config', ] @@ -92,7 +91,6 @@ class CeilometerHooksTest(CharmTestCase): @patch('charmhelpers.core.hookenv.config') def test_config_changed(self, mock_config): self.is_relation_made.return_value = True - self.is_unit_paused_set.return_value = False self.filter_installed_packages.return_value = ['pkg1', 'pkg2'] hooks.hooks.execute(['hooks/config-changed']) self.update_nrpe_config.assert_called_once_with() @@ -103,20 +101,9 @@ class CeilometerHooksTest(CharmTestCase): @patch('charmhelpers.core.hookenv.config') def test_config_changed_no_nrpe(self, mock_config): self.is_relation_made.return_value = False - self.is_unit_paused_set.return_value = False self.filter_installed_packages.return_value = ['pkg1', 'pkg2'] hooks.hooks.execute(['hooks/config-changed']) self.assertFalse(self.update_nrpe_config.called) self.CONFIGS.write_all.assert_called_once_with() self.apt_install.assert_called_once_with(['pkg1', 'pkg2'], fatal=True) self.is_relation_made.assert_called_once_with('nrpe-external-master') - - @patch('charmhelpers.core.hookenv.config') - def test_config_changed_paused(self, mock_config): - self.is_relation_made.return_value = True - self.is_unit_paused_set.return_value = True - self.filter_installed_packages.return_value = ['pkg1', 'pkg2'] - hooks.hooks.execute(['hooks/config-changed']) - self.assertFalse(self.update_nrpe_config.called) - self.assertFalse(self.CONFIGS.write_all.called) - self.assertFalse(self.apt_install.called) diff --git a/unit_tests/test_ceilometer_utils.py b/unit_tests/test_ceilometer_utils.py index 467086e..4bd751f 100644 --- a/unit_tests/test_ceilometer_utils.py +++ b/unit_tests/test_ceilometer_utils.py @@ -113,25 +113,6 @@ class CeilometerUtilsTest(CharmTestCase): make_assess_status_func.assert_called_once_with( 'test-config', REQUIRED_INTERFACES, services='s1', ports=None) - def test_pause_unit_helper(self): - with patch.object(utils, '_pause_resume_helper') as prh: - utils.pause_unit_helper('random-config') - prh.assert_called_once_with(utils.pause_unit, 'random-config') - with patch.object(utils, '_pause_resume_helper') as prh: - utils.resume_unit_helper('random-config') - prh.assert_called_once_with(utils.resume_unit, 'random-config') - - @patch.object(utils, 'services') - def test_pause_resume_helper(self, services): - f = MagicMock() - services.return_value = 's1' - with patch.object(utils, 'assess_status_func') as asf: - asf.return_value = 'assessor' - utils._pause_resume_helper(f, 'some-config') - asf.assert_called_once_with('some-config') - # ports=None whilst port checks are disabled. - f.assert_called_once_with('assessor', services='s1', ports=None) - def test_determine_purge_packages(self): 'Ensure no packages are identified for purge prior to rocky' self.get_os_codename_package.return_value = 'queens'