From e61a9a8d13d4bdf55f6af71c8c0b78724f5e71b6 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Wed, 13 Jan 2016 15:13:10 +0000 Subject: [PATCH 1/2] Change pause/resume actions use (new) assess_status() Implemented new is_paused() and assess_status() functions, and changed the pause and resume actions to use them. Changed existing and added new tests to verify functionality. --- actions/actions.py | 15 ++++++--- hooks/keystone_utils.py | 35 ++++++++++++++++++++ unit_tests/test_actions.py | 54 +++++++++++-------------------- unit_tests/test_keystone_utils.py | 34 +++++++++++++++++++ 4 files changed, 97 insertions(+), 41 deletions(-) diff --git a/actions/actions.py b/actions/actions.py index d14a6211..5e27acdc 100755 --- a/actions/actions.py +++ b/actions/actions.py @@ -4,9 +4,11 @@ import sys import os from charmhelpers.core.host import service_pause, service_resume -from charmhelpers.core.hookenv import action_fail, status_set +from charmhelpers.core.hookenv import action_fail +from charmhelpers.core.unitdata import HookData, kv -from hooks.keystone_utils import services +from hooks.keystone_utils import services, assess_status +from hooks.keystone_hooks import CONFIGS def pause(args): @@ -18,8 +20,9 @@ def pause(args): stopped = service_pause(service) if not stopped: raise Exception("{} didn't stop cleanly.".format(service)) - status_set( - "maintenance", "Paused. Use 'resume' action to resume normal service.") + with HookData()(): + kv().set('unit-paused', True) + assess_status(CONFIGS) def resume(args): @@ -31,7 +34,9 @@ def resume(args): started = service_resume(service) if not started: raise Exception("{} didn't start cleanly.".format(service)) - status_set("active", "") + with HookData()(): + kv().set('unit-paused', False) + assess_status(CONFIGS) # A dictionary of all the defined actions to callables (which take diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 87e71b76..62cfdb2a 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -83,6 +83,7 @@ from charmhelpers.core.hookenv import ( INFO, WARNING, status_get, + status_set, ) from charmhelpers.fetch import ( @@ -116,6 +117,12 @@ from charmhelpers.core.templating import render import keystone_context import keystone_ssl as ssl +from charmhelpers.core.unitdata import ( + HookData, + kv, +) + + TEMPLATES = 'templates/' # removed from original: charm-helper-sh @@ -1807,3 +1814,31 @@ def check_optional_relations(configs): return status_get() else: return 'unknown', 'No optional relations' + + +def is_paused(status_get=status_get): + """Is the unit paused?""" + with HookData()(): + if kv().get('unit-paused'): + return True + else: + return False + + +def assess_status(configs): + """Assess status of current unit + + Decides what the state of the unit should be based on the current + configuration. + + @param configs: a templating.OSConfigRenderer() object + """ + + if is_paused(): + status_set("maintenance", + "Paused. Use 'resume' action to resume normal service.") + return + + # set the status according to the current state of the contexts + set_os_workload_status( + configs, REQUIRED_INTERFACES, charm_func=check_optional_relations) diff --git a/unit_tests/test_actions.py b/unit_tests/test_actions.py index a9ce3ec2..c1e908a5 100644 --- a/unit_tests/test_actions.py +++ b/unit_tests/test_actions.py @@ -1,15 +1,19 @@ import mock +from mock import patch from test_utils import CharmTestCase -import actions.actions +with patch('actions.hooks.keystone_utils.is_paused') as is_paused: + with patch('actions.hooks.keystone_utils.register_configs') as configs: + import actions.actions class PauseTestCase(CharmTestCase): def setUp(self): super(PauseTestCase, self).setUp( - actions.actions, ["service_pause", "status_set"]) + actions.actions, ["service_pause", "HookData", "kv", + "assess_status"]) def test_pauses_services(self): """Pause action pauses all Keystone services.""" @@ -22,7 +26,8 @@ class PauseTestCase(CharmTestCase): self.service_pause.side_effect = fake_service_pause actions.actions.pause([]) - self.assertEqual(pause_calls, ['haproxy', 'keystone', 'apache2']) + self.assertItemsEqual( + pause_calls, ['haproxy', 'keystone', 'apache2']) def test_bails_out_early_on_error(self): """Pause action fails early if there are errors stopping a service.""" @@ -41,32 +46,20 @@ class PauseTestCase(CharmTestCase): actions.actions.pause, []) self.assertEqual(pause_calls, ['haproxy']) - def test_status_mode(self): - """Pause action sets the status to maintenance.""" - status_calls = [] - self.status_set.side_effect = lambda state, msg: status_calls.append( - state) + def test_pause_sets_value(self): + """Pause action sets the unit-paused value to True.""" + self.HookData()().return_value = True actions.actions.pause([]) - self.assertEqual(status_calls, ["maintenance"]) - - def test_status_message(self): - """Pause action sets a status message reflecting that it's paused.""" - status_calls = [] - self.status_set.side_effect = lambda state, msg: status_calls.append( - msg) - - actions.actions.pause([]) - self.assertEqual( - status_calls, ["Paused. " - "Use 'resume' action to resume normal service."]) + self.kv().set.assert_called_with('unit-paused', True) class ResumeTestCase(CharmTestCase): def setUp(self): super(ResumeTestCase, self).setUp( - actions.actions, ["service_resume", "status_set"]) + actions.actions, ["service_resume", "HookData", "kv", + "assess_status"]) def test_resumes_services(self): """Resume action resumes all Keystone services.""" @@ -97,23 +90,12 @@ class ResumeTestCase(CharmTestCase): actions.actions.resume, []) self.assertEqual(resume_calls, ['haproxy']) - def test_status_mode(self): - """Resume action sets the status to maintenance.""" - status_calls = [] - self.status_set.side_effect = lambda state, msg: status_calls.append( - state) + def test_resume_sets_value(self): + """Resume action sets the unit-paused value to False.""" + self.HookData()().return_value = True actions.actions.resume([]) - self.assertEqual(status_calls, ["active"]) - - def test_status_message(self): - """Resume action sets an empty status message.""" - status_calls = [] - self.status_set.side_effect = lambda state, msg: status_calls.append( - msg) - - actions.actions.resume([]) - self.assertEqual(status_calls, [""]) + self.kv().set.assert_called_with('unit-paused', False) class MainTestCase(CharmTestCase): diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index 03626237..56096ac5 100644 --- a/unit_tests/test_keystone_utils.py +++ b/unit_tests/test_keystone_utils.py @@ -729,3 +729,37 @@ class TestKeystoneUtils(CharmTestCase): ] self.assertEquals(render.call_args_list, expected) service_restart.assert_called_with('keystone') + + @patch.object(utils, 'HookData') + @patch.object(utils, 'kv') + def test_is_paused(self, kv, HookData): + """test_is_paused: Test is_paused() returns value + from kv('unit-paused')""" + HookData()().return_value = True + kv().get.return_value = True + self.assertEqual(utils.is_paused(), True) + kv().get.assert_called_with('unit-paused') + kv().get.return_value = False + self.assertEqual(utils.is_paused(), False) + + @patch.object(utils, 'is_paused') + @patch.object(utils, 'status_set') + def test_assess_status(self, status_set, is_paused): + """test_assess_status: verify that it does pick the right status""" + # check that paused status does the right thing + is_paused.return_value = True + utils.assess_status(None) + status_set.assert_called_with( + "maintenance", + "Paused. Use 'resume' action to resume normal service.") + + # if it isn't paused, the assess_status() calls + # set_os_workload_status() + is_paused.return_value = False + with patch.object(utils, 'set_os_workload_status') \ + as set_os_workload_status: + utils.assess_status("TEST CONFIG") + set_os_workload_status.assert_called_with( + "TEST CONFIG", + utils.REQUIRED_INTERFACES, + charm_func=utils.check_optional_relations) From e5c16e5fb6c547d7e89201027b79801ca7459992 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Thu, 14 Jan 2016 11:31:23 +0000 Subject: [PATCH 2/2] Change set_os_workload_status(...) call to assess_status(...) call in hooks/keystone_hooks.py:main This is so that invoking keystone_hooks.py directly doesn't clobber a user set paused state. --- hooks/keystone_hooks.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index c6b85a79..b827653e 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -47,7 +47,6 @@ from charmhelpers.contrib.openstack.utils import ( git_install_requested, openstack_upgrade_available, sync_db_with_multi_ipv6_addresses, - set_os_workload_status, ) from keystone_utils import ( @@ -82,8 +81,7 @@ from keystone_utils import ( force_ssl_sync, filter_null, ensure_ssl_dirs, - REQUIRED_INTERFACES, - check_optional_relations, + assess_status, ) from charmhelpers.contrib.hahelpers.cluster import ( @@ -649,8 +647,7 @@ def main(): hooks.execute(sys.argv) except UnregisteredHookError as e: log('Unknown hook {} - skipping.'.format(e)) - set_os_workload_status(CONFIGS, REQUIRED_INTERFACES, - charm_func=check_optional_relations) + assess_status(CONFIGS) if __name__ == '__main__':