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

Proposed by Alex Kavanagh
Status: Merged
Merged at revision: 202
Proposed branch: lp:~ajkavanagh/charms/trusty/keystone/use-assess-status-lp1531767
Merge into: lp:~openstack-charmers-archive/charms/trusty/keystone/next
Diff against target: 294 lines (+109/-48) (has conflicts)
5 files modified
actions/actions.py (+10/-5)
hooks/keystone_hooks.py (+5/-3)
hooks/keystone_utils.py (+35/-0)
unit_tests/test_actions.py (+22/-40)
unit_tests/test_keystone_utils.py (+37/-0)
Text conflict in hooks/keystone_hooks.py
Text conflict in unit_tests/test_keystone_utils.py
To merge this branch: bzr merge lp:~ajkavanagh/charms/trusty/keystone/use-assess-status-lp1531767
Reviewer Review Type Date Requested Status
David Ames (community) Approve
Review via email: mp+282453@code.launchpad.net

Description of the change

For the pause/resume actions implementation, uses a new is_paused() and assess_status(...) functions that have been added to hooks/keystone_utils().

This means that the pause/resume no longer just use status_set() but rather, when resuming, the unit checks to see whether it can become active via set_os_workload_status(...)

To post a comment you must log in.
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

charm_lint_check #17247 keystone-next for ajkavanagh mp282453
    LINT OK: passed

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

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

charm_unit_test #16112 keystone-next for ajkavanagh mp282453
    UNIT OK: passed

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

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 keystone_hooks.py set_os_workload_status is called which will clobber any paused status. This should be replaced with a call to assess_status() and you already call set_os_workload_status *in* assess_status correctly.

def main():
    try:
        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)

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

charm_amulet_test #8753 keystone-next for ajkavanagh mp282453
    AMULET OK: passed

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

198. By Alex Kavanagh

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.

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 keystone_hooks.py set_os_workload_status is called which will
> clobber any paused status. This should be replaced with a call to
> assess_status() and you already call set_os_workload_status *in* assess_status
> correctly.
>
> def main():
> try:
> 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)

Okay, done a change of set_os_workload_status(...) -> assess_status(...).

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

charm_lint_check #17319 keystone-next for ajkavanagh mp282453
    LINT OK: passed

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

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

charm_unit_test #16178 keystone-next for ajkavanagh mp282453
    UNIT OK: passed

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

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

charm_amulet_test #8779 keystone-next for ajkavanagh mp282453
    AMULET OK: passed

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

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

Looks good. I cleaned up a couple of small merge conflicts just to speed this up.

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-19 09:47:12 +0000
3+++ actions/actions.py 2016-01-14 11:33:13 +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.keystone_utils import services
13+from hooks.keystone_utils import services, assess_status
14+from hooks.keystone_hooks 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/keystone_hooks.py'
43--- hooks/keystone_hooks.py 2016-01-12 11:09:46 +0000
44+++ hooks/keystone_hooks.py 2016-01-14 11:33:13 +0000
45@@ -47,7 +47,6 @@
46 git_install_requested,
47 openstack_upgrade_available,
48 sync_db_with_multi_ipv6_addresses,
49- set_os_workload_status,
50 )
51
52 from keystone_utils import (
53@@ -81,11 +80,15 @@
54 force_ssl_sync,
55 filter_null,
56 ensure_ssl_dirs,
57+<<<<<<< TREE
58 REQUIRED_INTERFACES,
59 check_optional_relations,
60 ensure_pki_cert_paths,
61 is_service_present,
62 delete_service_entry,
63+=======
64+ assess_status,
65+>>>>>>> MERGE-SOURCE
66 )
67
68 from charmhelpers.contrib.hahelpers.cluster import (
69@@ -646,8 +649,7 @@
70 hooks.execute(sys.argv)
71 except UnregisteredHookError as e:
72 log('Unknown hook {} - skipping.'.format(e))
73- set_os_workload_status(CONFIGS, REQUIRED_INTERFACES,
74- charm_func=check_optional_relations)
75+ assess_status(CONFIGS)
76
77
78 if __name__ == '__main__':
79
80=== modified file 'hooks/keystone_utils.py'
81--- hooks/keystone_utils.py 2016-01-12 15:50:54 +0000
82+++ hooks/keystone_utils.py 2016-01-14 11:33:13 +0000
83@@ -83,6 +83,7 @@
84 INFO,
85 WARNING,
86 status_get,
87+ status_set,
88 )
89
90 from charmhelpers.fetch import (
91@@ -116,6 +117,12 @@
92 import keystone_context
93 import keystone_ssl as ssl
94
95+from charmhelpers.core.unitdata import (
96+ HookData,
97+ kv,
98+)
99+
100+
101 TEMPLATES = 'templates/'
102
103 # removed from original: charm-helper-sh
104@@ -1829,3 +1836,31 @@
105 return status_get()
106 else:
107 return 'unknown', 'No optional relations'
108+
109+
110+def is_paused(status_get=status_get):
111+ """Is the unit paused?"""
112+ with HookData()():
113+ if kv().get('unit-paused'):
114+ return True
115+ else:
116+ return False
117+
118+
119+def assess_status(configs):
120+ """Assess status of current unit
121+
122+ Decides what the state of the unit should be based on the current
123+ configuration.
124+
125+ @param configs: a templating.OSConfigRenderer() object
126+ """
127+
128+ if is_paused():
129+ status_set("maintenance",
130+ "Paused. Use 'resume' action to resume normal service.")
131+ return
132+
133+ # set the status according to the current state of the contexts
134+ set_os_workload_status(
135+ configs, REQUIRED_INTERFACES, charm_func=check_optional_relations)
136
137=== modified file 'unit_tests/test_actions.py'
138--- unit_tests/test_actions.py 2015-08-19 07:33:27 +0000
139+++ unit_tests/test_actions.py 2016-01-14 11:33:13 +0000
140@@ -1,15 +1,19 @@
141 import mock
142+from mock import patch
143
144 from test_utils import CharmTestCase
145
146-import actions.actions
147+with patch('actions.hooks.keystone_utils.is_paused') as is_paused:
148+ with patch('actions.hooks.keystone_utils.register_configs') as configs:
149+ import actions.actions
150
151
152 class PauseTestCase(CharmTestCase):
153
154 def setUp(self):
155 super(PauseTestCase, self).setUp(
156- actions.actions, ["service_pause", "status_set"])
157+ actions.actions, ["service_pause", "HookData", "kv",
158+ "assess_status"])
159
160 def test_pauses_services(self):
161 """Pause action pauses all Keystone services."""
162@@ -22,7 +26,8 @@
163 self.service_pause.side_effect = fake_service_pause
164
165 actions.actions.pause([])
166- self.assertEqual(pause_calls, ['haproxy', 'keystone', 'apache2'])
167+ self.assertItemsEqual(
168+ pause_calls, ['haproxy', 'keystone', 'apache2'])
169
170 def test_bails_out_early_on_error(self):
171 """Pause action fails early if there are errors stopping a service."""
172@@ -41,32 +46,20 @@
173 actions.actions.pause, [])
174 self.assertEqual(pause_calls, ['haproxy'])
175
176- def test_status_mode(self):
177- """Pause action sets the status to maintenance."""
178- status_calls = []
179- self.status_set.side_effect = lambda state, msg: status_calls.append(
180- state)
181-
182- actions.actions.pause([])
183- self.assertEqual(status_calls, ["maintenance"])
184-
185- def test_status_message(self):
186- """Pause action sets a status message reflecting that it's paused."""
187- status_calls = []
188- self.status_set.side_effect = lambda state, msg: status_calls.append(
189- msg)
190-
191- actions.actions.pause([])
192- self.assertEqual(
193- status_calls, ["Paused. "
194- "Use 'resume' action to resume normal service."])
195+ def test_pause_sets_value(self):
196+ """Pause action sets the unit-paused value to True."""
197+ self.HookData()().return_value = True
198+
199+ actions.actions.pause([])
200+ self.kv().set.assert_called_with('unit-paused', True)
201
202
203 class ResumeTestCase(CharmTestCase):
204
205 def setUp(self):
206 super(ResumeTestCase, self).setUp(
207- actions.actions, ["service_resume", "status_set"])
208+ actions.actions, ["service_resume", "HookData", "kv",
209+ "assess_status"])
210
211 def test_resumes_services(self):
212 """Resume action resumes all Keystone services."""
213@@ -97,23 +90,12 @@
214 actions.actions.resume, [])
215 self.assertEqual(resume_calls, ['haproxy'])
216
217- def test_status_mode(self):
218- """Resume action sets the status to maintenance."""
219- status_calls = []
220- self.status_set.side_effect = lambda state, msg: status_calls.append(
221- state)
222-
223- actions.actions.resume([])
224- self.assertEqual(status_calls, ["active"])
225-
226- def test_status_message(self):
227- """Resume action sets an empty status message."""
228- status_calls = []
229- self.status_set.side_effect = lambda state, msg: status_calls.append(
230- msg)
231-
232- actions.actions.resume([])
233- self.assertEqual(status_calls, [""])
234+ def test_resume_sets_value(self):
235+ """Resume action sets the unit-paused value to False."""
236+ self.HookData()().return_value = True
237+
238+ actions.actions.resume([])
239+ self.kv().set.assert_called_with('unit-paused', False)
240
241
242 class MainTestCase(CharmTestCase):
243
244=== modified file 'unit_tests/test_keystone_utils.py'
245--- unit_tests/test_keystone_utils.py 2016-01-12 11:09:46 +0000
246+++ unit_tests/test_keystone_utils.py 2016-01-14 11:33:13 +0000
247@@ -703,6 +703,7 @@
248 ]
249 self.assertEquals(render.call_args_list, expected)
250 service_restart.assert_called_with('keystone')
251+<<<<<<< TREE
252
253 @patch.object(manager, 'KeystoneManager')
254 def test_is_service_present(self, KeystoneManager):
255@@ -725,3 +726,39 @@
256 KeystoneManager.return_value = mock_keystone
257 utils.delete_service_entry('bob', 'bill')
258 mock_keystone.api.services.delete.assert_called_with('sid1')
259+=======
260+
261+ @patch.object(utils, 'HookData')
262+ @patch.object(utils, 'kv')
263+ def test_is_paused(self, kv, HookData):
264+ """test_is_paused: Test is_paused() returns value
265+ from kv('unit-paused')"""
266+ HookData()().return_value = True
267+ kv().get.return_value = True
268+ self.assertEqual(utils.is_paused(), True)
269+ kv().get.assert_called_with('unit-paused')
270+ kv().get.return_value = False
271+ self.assertEqual(utils.is_paused(), False)
272+
273+ @patch.object(utils, 'is_paused')
274+ @patch.object(utils, 'status_set')
275+ def test_assess_status(self, status_set, is_paused):
276+ """test_assess_status: verify that it does pick the right status"""
277+ # check that paused status does the right thing
278+ is_paused.return_value = True
279+ utils.assess_status(None)
280+ status_set.assert_called_with(
281+ "maintenance",
282+ "Paused. Use 'resume' action to resume normal service.")
283+
284+ # if it isn't paused, the assess_status() calls
285+ # set_os_workload_status()
286+ is_paused.return_value = False
287+ with patch.object(utils, 'set_os_workload_status') \
288+ as set_os_workload_status:
289+ utils.assess_status("TEST CONFIG")
290+ set_os_workload_status.assert_called_with(
291+ "TEST CONFIG",
292+ utils.REQUIRED_INTERFACES,
293+ charm_func=utils.check_optional_relations)
294+>>>>>>> MERGE-SOURCE

Subscribers

People subscribed via source and target branches