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

Proposed by Alex Kavanagh
Status: Merged
Merged at revision: 162
Proposed branch: lp:~ajkavanagh/charms/trusty/glance/use-assess-status-lp1531767
Merge into: lp:~openstack-charmers-archive/charms/trusty/glance/next
Diff against target: 272 lines (+104/-52)
5 files modified
actions/actions.py (+10/-5)
hooks/glance_relations.py (+3/-6)
hooks/glance_utils.py (+34/-0)
unit_tests/test_actions.py (+23/-41)
unit_tests/test_glance_utils.py (+34/-0)
To merge this branch: bzr merge lp:~ajkavanagh/charms/trusty/glance/use-assess-status-lp1531767
Reviewer Review Type Date Requested Status
David Ames (community) Approve
Liam Young Needs Fixing
Review via email: mp+282343@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Liam Young (gnuoy) wrote :

This look good, just a couple of comments inline

review: Needs Fixing
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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

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

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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

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

Revision history for this message
David Ames (thedac) wrote :

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
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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

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

Revision history for this message
Alex Kavanagh (ajkavanagh) wrote :

> 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.

161. By Alex Kavanagh

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.

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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

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

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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

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

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

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

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

Revision history for this message
David Ames (thedac) wrote :

Looks good. Merging.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'actions/actions.py'
2--- actions/actions.py 2015-08-26 13:16:31 +0000
3+++ actions/actions.py 2016-01-14 11:24:53 +0000
4@@ -4,9 +4,11 @@
5 import os
6
7 from charmhelpers.core.host import service_pause, service_resume
8-from charmhelpers.core.hookenv import action_fail, status_set
9+from charmhelpers.core.hookenv import action_fail
10+from charmhelpers.core.unitdata import HookData, kv
11
12-from hooks.glance_utils import services
13+from hooks.glance_utils import services, assess_status
14+from hooks.glance_relations import CONFIGS
15
16
17 def pause(args):
18@@ -18,8 +20,9 @@
19 stopped = service_pause(service)
20 if not stopped:
21 raise Exception("{} didn't stop cleanly.".format(service))
22- status_set(
23- "maintenance", "Paused. Use 'resume' action to resume normal service.")
24+ with HookData()():
25+ kv().set('unit-paused', True)
26+ assess_status(CONFIGS)
27
28
29 def resume(args):
30@@ -31,7 +34,9 @@
31 started = service_resume(service)
32 if not started:
33 raise Exception("{} didn't start cleanly.".format(service))
34- status_set("active", "")
35+ with HookData()():
36+ kv().set('unit-paused', False)
37+ assess_status(CONFIGS)
38
39
40 # A dictionary of all the defined actions to callables (which take
41
42=== modified file 'hooks/glance_relations.py'
43--- hooks/glance_relations.py 2015-10-30 22:11:38 +0000
44+++ hooks/glance_relations.py 2016-01-14 11:24:53 +0000
45@@ -24,9 +24,8 @@
46 HAPROXY_CONF,
47 ceph_config_file,
48 setup_ipv6,
49- REQUIRED_INTERFACES,
50- check_optional_relations,
51- swift_temp_url_key
52+ swift_temp_url_key,
53+ assess_status,
54 )
55 from charmhelpers.core.hookenv import (
56 config,
57@@ -67,7 +66,6 @@
58 openstack_upgrade_available,
59 os_release,
60 sync_db_with_multi_ipv6_addresses,
61- set_os_workload_status,
62 )
63 from charmhelpers.contrib.storage.linux.ceph import (
64 send_request_if_needed,
65@@ -536,5 +534,4 @@
66 hooks.execute(sys.argv)
67 except UnregisteredHookError as e:
68 juju_log('Unknown hook {} - skipping.'.format(e))
69- set_os_workload_status(CONFIGS, REQUIRED_INTERFACES,
70- charm_func=check_optional_relations)
71+ assess_status(CONFIGS)
72
73=== modified file 'hooks/glance_utils.py'
74--- hooks/glance_utils.py 2015-10-30 22:11:38 +0000
75+++ hooks/glance_utils.py 2016-01-14 11:24:53 +0000
76@@ -26,6 +26,7 @@
77 relation_ids,
78 service_name,
79 status_get,
80+ status_set,
81 )
82
83
84@@ -70,6 +71,11 @@
85 retry_on_exception,
86 )
87
88+from charmhelpers.core.unitdata import (
89+ HookData,
90+ kv,
91+)
92+
93
94 CLUSTER_RES = "grp_glance_vips"
95
96@@ -491,3 +497,31 @@
97 swift_connection.post_account(headers={'x-account-meta-temp-url-key':
98 temp_url_key})
99 return temp_url_key
100+
101+
102+def is_paused(status_get=status_get):
103+ """Is the unit paused?"""
104+ with HookData()():
105+ if kv().get('unit-paused'):
106+ return True
107+ else:
108+ return False
109+
110+
111+def assess_status(configs):
112+ """Assess status of current unit
113+
114+ Decides what the state of the unit should be based on the current
115+ configuration.
116+
117+ @param configs: a templating.OSConfigRenderer() object
118+ """
119+
120+ if is_paused():
121+ status_set("maintenance",
122+ "Paused. Use 'resume' action to resume normal service.")
123+ return
124+
125+ # set the status according to the current state of the contexts
126+ set_os_workload_status(
127+ configs, REQUIRED_INTERFACES, charm_func=check_optional_relations)
128
129=== modified file 'unit_tests/test_actions.py'
130--- unit_tests/test_actions.py 2015-09-02 11:40:06 +0000
131+++ unit_tests/test_actions.py 2016-01-14 11:24:53 +0000
132@@ -1,18 +1,23 @@
133 import os
134 import mock
135
136+from test_utils import CharmTestCase
137+
138+from mock import patch
139+
140 os.environ['JUJU_UNIT_NAME'] = 'glance'
141
142-from test_utils import CharmTestCase
143-
144-import actions.actions
145+with patch('actions.hooks.glance_utils.is_paused') as is_paused:
146+ with patch('actions.hooks.glance_utils.register_configs') as configs:
147+ import actions.actions
148
149
150 class PauseTestCase(CharmTestCase):
151
152 def setUp(self):
153 super(PauseTestCase, self).setUp(
154- actions.actions, ["service_pause", "status_set"])
155+ actions.actions, ["service_pause", "HookData", "kv",
156+ "assess_status"])
157
158 def test_pauses_services(self):
159 """Pause action pauses all Glance services."""
160@@ -46,32 +51,20 @@
161 actions.actions.pause, [])
162 self.assertEqual(pause_calls, ['haproxy', 'glance-api'])
163
164- def test_status_mode(self):
165- """Pause action sets the status to maintenance."""
166- status_calls = []
167- self.status_set.side_effect = lambda state, msg: status_calls.append(
168- state)
169-
170- actions.actions.pause([])
171- self.assertEqual(status_calls, ["maintenance"])
172-
173- def test_status_message(self):
174- """Pause action sets a status message reflecting that it's paused."""
175- status_calls = []
176- self.status_set.side_effect = lambda state, msg: status_calls.append(
177- msg)
178-
179- actions.actions.pause([])
180- self.assertEqual(
181- status_calls, ["Paused. "
182- "Use 'resume' action to resume normal service."])
183+ def test_pause_sets_value(self):
184+ """Pause action sets the unit-paused value to True."""
185+ self.HookData()().return_value = True
186+
187+ actions.actions.pause([])
188+ self.kv().set.assert_called_with('unit-paused', True)
189
190
191 class ResumeTestCase(CharmTestCase):
192
193 def setUp(self):
194 super(ResumeTestCase, self).setUp(
195- actions.actions, ["service_resume", "status_set"])
196+ actions.actions, ["service_resume", "HookData", "kv",
197+ "assess_status"])
198
199 def test_resumes_services(self):
200 """Resume action resumes all Glance services."""
201@@ -104,23 +97,12 @@
202 actions.actions.resume, [])
203 self.assertEqual(resume_calls, ['haproxy', 'glance-api'])
204
205- def test_status_mode(self):
206- """Resume action sets the status to active."""
207- status_calls = []
208- self.status_set.side_effect = lambda state, msg: status_calls.append(
209- state)
210-
211- actions.actions.resume([])
212- self.assertEqual(status_calls, ["active"])
213-
214- def test_status_message(self):
215- """Resume action sets an empty status message."""
216- status_calls = []
217- self.status_set.side_effect = lambda state, msg: status_calls.append(
218- msg)
219-
220- actions.actions.resume([])
221- self.assertEqual(status_calls, [""])
222+ def test_resume_sets_value(self):
223+ """Resume action sets the unit-paused value to False."""
224+ self.HookData()().return_value = True
225+
226+ actions.actions.resume([])
227+ self.kv().set.assert_called_with('unit-paused', False)
228
229
230 class MainTestCase(CharmTestCase):
231
232=== modified file 'unit_tests/test_glance_utils.py'
233--- unit_tests/test_glance_utils.py 2015-09-28 11:42:38 +0000
234+++ unit_tests/test_glance_utils.py 2016-01-14 11:24:53 +0000
235@@ -293,3 +293,37 @@
236 call('glance-registry'),
237 ]
238 self.assertEquals(service_restart.call_args_list, expected)
239+
240+ @patch.object(utils, 'HookData')
241+ @patch.object(utils, 'kv')
242+ def test_is_paused(self, kv, HookData):
243+ """test_is_paused: Test is_paused() returns value
244+ from kv('unit-paused')"""
245+ HookData()().return_value = True
246+ kv().get.return_value = True
247+ self.assertEqual(utils.is_paused(), True)
248+ kv().get.assert_called_with('unit-paused')
249+ kv().get.return_value = False
250+ self.assertEqual(utils.is_paused(), False)
251+
252+ @patch.object(utils, 'is_paused')
253+ @patch.object(utils, 'status_set')
254+ def test_assess_status(self, status_set, is_paused):
255+ """test_assess_status: verify that it does pick the right status"""
256+ # check that paused status does the right thing
257+ is_paused.return_value = True
258+ utils.assess_status(None)
259+ status_set.assert_called_with(
260+ "maintenance",
261+ "Paused. Use 'resume' action to resume normal service.")
262+
263+ # if it isn't paused, the assess_status() calls
264+ # set_os_workload_status()
265+ is_paused.return_value = False
266+ with patch.object(utils, 'set_os_workload_status') \
267+ as set_os_workload_status:
268+ utils.assess_status("TEST CONFIG")
269+ set_os_workload_status.assert_called_with(
270+ "TEST CONFIG",
271+ utils.REQUIRED_INTERFACES,
272+ charm_func=utils.check_optional_relations)

Subscribers

People subscribed via source and target branches