Merge lp:~ajkavanagh/charms/trusty/glance/use-assess-status-lp1531767 into lp:~openstack-charmers-archive/charms/trusty/glance/next

[Alex Kavanagh]

Change to the implementation of pause and resume actions such that they use a new function called assess_status() which (currently) only checks if the unit is_paused() -- which is also new.

Note that it doesn't (as yet?) change any of the other status_set() functions which means this is only a partial solution.

[Liam Young (gnuoy)]

This look good, just a couple of comments inline

review: Needs Fixing

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #17246 glance-next for ajkavanagh mp282343
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/17246/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #16113 glance-next for ajkavanagh mp282343
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/16113/

[David Ames (thedac)]

As the purpose of the MP is to implement assess_status() we need to make sure it runs after every hook.

Currently in glance_relations.py set_os_workload_status is called which will clobber any paused status. This should be replaced with a call to asssess_status() and as Liam points out assess_status should ultimately call set_os_workload_status.

if __name__ == '__main__':
    try:
        hooks.execute(sys.argv)
    except UnregisteredHookError as e:
        juju_log('Unknown hook {} - skipping.'.format(e))
    set_os_workload_status(CONFIGS, REQUIRED_INTERFACES,
                           charm_func=check_optional_relations)

review: Needs Fixing

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8752 glance-next for ajkavanagh mp282343
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/8752/

[Alex Kavanagh (ajkavanagh)]

> As the purpose of the MP is to implement assess_status() we need to make sure
> it runs after every hook.
>
> Currently in glance_relations.py set_os_workload_status is called which will
> clobber any paused status. This should be replaced with a call to
> asssess_status() and as Liam points out assess_status should ultimately call
> set_os_workload_status.
>
> if __name__ == '__main__':
> try:
> hooks.execute(sys.argv)
> except UnregisteredHookError as e:
> juju_log('Unknown hook {} - skipping.'.format(e))
> set_os_workload_status(CONFIGS, REQUIRED_INTERFACES,
> charm_func=check_optional_relations)

Yep, ok. I'll do an search of all the set_os_workload_status() calls and see if they would be better as assess_status() -- i.e. just to check that it won't clobber a pause status as you say.

[lp:~ajkavanagh/charms/trusty/glance/use-assess-status-lp1531767]

161. By Alex Kavanagh on 2016-01-14

Change set_os_workload_status() call in hooks/glance_relations.py to assess_status(...) call

This is so that invoking glance_relations.py doesn't clobber a user set 'paused' state.

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #17318 glance-next for ajkavanagh mp282343
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/17318/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #16179 glance-next for ajkavanagh mp282343
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/16179/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8778 glance-next for ajkavanagh mp282343
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/8778/

[David Ames (thedac)]

Looks good. Merging.

review: Approve

=== modified file 'actions/actions.py'
--- actions/actions.py	2015-08-26 13:16:31 +0000
+++ actions/actions.py	2016-01-14 11:24:53 +0000
@@ -4,9 +4,11 @@
 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.glance_utils import services
+from hooks.glance_utils import services, assess_status
+from hooks.glance_relations import CONFIGS
 
 
 def pause(args):
@@ -18,8 +20,9 @@
         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 @@
         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

=== modified file 'hooks/glance_relations.py'
--- hooks/glance_relations.py	2015-10-30 22:11:38 +0000
+++ hooks/glance_relations.py	2016-01-14 11:24:53 +0000
@@ -24,9 +24,8 @@
     HAPROXY_CONF,
     ceph_config_file,
     setup_ipv6,
-    REQUIRED_INTERFACES,
-    check_optional_relations,
-    swift_temp_url_key
+    swift_temp_url_key,
+    assess_status,
 )
 from charmhelpers.core.hookenv import (
     config,
@@ -67,7 +66,6 @@
     openstack_upgrade_available,
     os_release,
     sync_db_with_multi_ipv6_addresses,
-    set_os_workload_status,
 )
 from charmhelpers.contrib.storage.linux.ceph import (
     send_request_if_needed,
@@ -536,5 +534,4 @@
         hooks.execute(sys.argv)
     except UnregisteredHookError as e:
         juju_log('Unknown hook {} - skipping.'.format(e))
-    set_os_workload_status(CONFIGS, REQUIRED_INTERFACES,
-                           charm_func=check_optional_relations)
+    assess_status(CONFIGS)

=== modified file 'hooks/glance_utils.py'
--- hooks/glance_utils.py	2015-10-30 22:11:38 +0000
+++ hooks/glance_utils.py	2016-01-14 11:24:53 +0000
@@ -26,6 +26,7 @@
     relation_ids,
     service_name,
     status_get,
+    status_set,
 )
 
 
@@ -70,6 +71,11 @@
     retry_on_exception,
 )
 
+from charmhelpers.core.unitdata import (
+    HookData,
+    kv,
+)
+
 
 CLUSTER_RES = "grp_glance_vips"
 
@@ -491,3 +497,31 @@
     swift_connection.post_account(headers={'x-account-meta-temp-url-key':
                                            temp_url_key})
     return temp_url_key
+
+
+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)

=== modified file 'unit_tests/test_actions.py'
--- unit_tests/test_actions.py	2015-09-02 11:40:06 +0000
+++ unit_tests/test_actions.py	2016-01-14 11:24:53 +0000
@@ -1,18 +1,23 @@
 import os
 import mock
 
+from test_utils import CharmTestCase
+
+from mock import patch
+
 os.environ['JUJU_UNIT_NAME'] = 'glance'
 
-from test_utils import CharmTestCase
-
-import actions.actions
+with patch('actions.hooks.glance_utils.is_paused') as is_paused:
+    with patch('actions.hooks.glance_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 Glance services."""
@@ -46,32 +51,20 @@
             actions.actions.pause, [])
         self.assertEqual(pause_calls, ['haproxy', 'glance-api'])
 
-    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)
-
-        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."])
+    def test_pause_sets_value(self):
+        """Pause action sets the unit-paused value to True."""
+        self.HookData()().return_value = True
+
+        actions.actions.pause([])
+        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 Glance services."""
@@ -104,23 +97,12 @@
             actions.actions.resume, [])
         self.assertEqual(resume_calls, ['haproxy', 'glance-api'])
 
-    def test_status_mode(self):
-        """Resume action sets the status to active."""
-        status_calls = []
-        self.status_set.side_effect = lambda state, msg: status_calls.append(
-            state)
-
-        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, [""])
+    def test_resume_sets_value(self):
+        """Resume action sets the unit-paused value to False."""
+        self.HookData()().return_value = True
+
+        actions.actions.resume([])
+        self.kv().set.assert_called_with('unit-paused', False)
 
 
 class MainTestCase(CharmTestCase):

=== modified file 'unit_tests/test_glance_utils.py'
--- unit_tests/test_glance_utils.py	2015-09-28 11:42:38 +0000
+++ unit_tests/test_glance_utils.py	2016-01-14 11:24:53 +0000
@@ -293,3 +293,37 @@
             call('glance-registry'),
         ]
         self.assertEquals(service_restart.call_args_list, expected)
+
+    @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)