Merge lp:~ajkavanagh/charms/trusty/glance/use-assess-status-lp1531767 into lp:~openstack-charmers-archive/charms/trusty/glance/next
- Trusty Tahr (14.04)
- use-assess-status-lp1531767
- Merge into next
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
David Ames (community) | Approve | ||
Liam Young (community) | Needs Fixing | ||
Review via email: mp+282343@code.launchpad.net |
Commit message
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.
uosci-testing-bot (uosci-testing-bot) wrote : | # |
charm_lint_check #17246 glance-next for ajkavanagh mp282343
LINT OK: passed
Build: http://
uosci-testing-bot (uosci-testing-bot) wrote : | # |
charm_unit_test #16113 glance-next for ajkavanagh mp282343
UNIT OK: passed
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_
if __name__ == '__main__':
try:
except UnregisteredHoo
set_
uosci-testing-bot (uosci-testing-bot) wrote : | # |
charm_amulet_test #8752 glance-next for ajkavanagh mp282343
AMULET OK: passed
Build: http://
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_
> 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_
>
> if __name__ == '__main__':
> try:
> hooks.execute(
> except UnregisteredHoo
> juju_log('Unknown hook {} - skipping.
> set_os_
> charm_func=
Yep, ok. I'll do an search of all the set_os_
- 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.
uosci-testing-bot (uosci-testing-bot) wrote : | # |
charm_lint_check #17318 glance-next for ajkavanagh mp282343
LINT OK: passed
Build: http://
uosci-testing-bot (uosci-testing-bot) wrote : | # |
charm_unit_test #16179 glance-next for ajkavanagh mp282343
UNIT OK: passed
uosci-testing-bot (uosci-testing-bot) wrote : | # |
charm_amulet_test #8778 glance-next for ajkavanagh mp282343
AMULET OK: passed
Build: http://
David Ames (thedac) wrote : | # |
Looks good. Merging.
Preview Diff
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 | 4 | import os | 4 | import os |
6 | 5 | 5 | ||
7 | 6 | from charmhelpers.core.host import service_pause, service_resume | 6 | from charmhelpers.core.host import service_pause, service_resume |
9 | 7 | from charmhelpers.core.hookenv import action_fail, status_set | 7 | from charmhelpers.core.hookenv import action_fail |
10 | 8 | from charmhelpers.core.unitdata import HookData, kv | ||
11 | 8 | 9 | ||
13 | 9 | from hooks.glance_utils import services | 10 | from hooks.glance_utils import services, assess_status |
14 | 11 | from hooks.glance_relations import CONFIGS | ||
15 | 10 | 12 | ||
16 | 11 | 13 | ||
17 | 12 | def pause(args): | 14 | def pause(args): |
18 | @@ -18,8 +20,9 @@ | |||
19 | 18 | stopped = service_pause(service) | 20 | stopped = service_pause(service) |
20 | 19 | if not stopped: | 21 | if not stopped: |
21 | 20 | raise Exception("{} didn't stop cleanly.".format(service)) | 22 | raise Exception("{} didn't stop cleanly.".format(service)) |
24 | 21 | status_set( | 23 | with HookData()(): |
25 | 22 | "maintenance", "Paused. Use 'resume' action to resume normal service.") | 24 | kv().set('unit-paused', True) |
26 | 25 | assess_status(CONFIGS) | ||
27 | 23 | 26 | ||
28 | 24 | 27 | ||
29 | 25 | def resume(args): | 28 | def resume(args): |
30 | @@ -31,7 +34,9 @@ | |||
31 | 31 | started = service_resume(service) | 34 | started = service_resume(service) |
32 | 32 | if not started: | 35 | if not started: |
33 | 33 | raise Exception("{} didn't start cleanly.".format(service)) | 36 | raise Exception("{} didn't start cleanly.".format(service)) |
35 | 34 | status_set("active", "") | 37 | with HookData()(): |
36 | 38 | kv().set('unit-paused', False) | ||
37 | 39 | assess_status(CONFIGS) | ||
38 | 35 | 40 | ||
39 | 36 | 41 | ||
40 | 37 | # A dictionary of all the defined actions to callables (which take | 42 | # A dictionary of all the defined actions to callables (which take |
41 | 38 | 43 | ||
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 | 24 | HAPROXY_CONF, | 24 | HAPROXY_CONF, |
47 | 25 | ceph_config_file, | 25 | ceph_config_file, |
48 | 26 | setup_ipv6, | 26 | setup_ipv6, |
52 | 27 | REQUIRED_INTERFACES, | 27 | swift_temp_url_key, |
53 | 28 | check_optional_relations, | 28 | assess_status, |
51 | 29 | swift_temp_url_key | ||
54 | 30 | ) | 29 | ) |
55 | 31 | from charmhelpers.core.hookenv import ( | 30 | from charmhelpers.core.hookenv import ( |
56 | 32 | config, | 31 | config, |
57 | @@ -67,7 +66,6 @@ | |||
58 | 67 | openstack_upgrade_available, | 66 | openstack_upgrade_available, |
59 | 68 | os_release, | 67 | os_release, |
60 | 69 | sync_db_with_multi_ipv6_addresses, | 68 | sync_db_with_multi_ipv6_addresses, |
61 | 70 | set_os_workload_status, | ||
62 | 71 | ) | 69 | ) |
63 | 72 | from charmhelpers.contrib.storage.linux.ceph import ( | 70 | from charmhelpers.contrib.storage.linux.ceph import ( |
64 | 73 | send_request_if_needed, | 71 | send_request_if_needed, |
65 | @@ -536,5 +534,4 @@ | |||
66 | 536 | hooks.execute(sys.argv) | 534 | hooks.execute(sys.argv) |
67 | 537 | except UnregisteredHookError as e: | 535 | except UnregisteredHookError as e: |
68 | 538 | juju_log('Unknown hook {} - skipping.'.format(e)) | 536 | juju_log('Unknown hook {} - skipping.'.format(e)) |
71 | 539 | set_os_workload_status(CONFIGS, REQUIRED_INTERFACES, | 537 | assess_status(CONFIGS) |
70 | 540 | charm_func=check_optional_relations) | ||
72 | 541 | 538 | ||
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 | 26 | relation_ids, | 26 | relation_ids, |
78 | 27 | service_name, | 27 | service_name, |
79 | 28 | status_get, | 28 | status_get, |
80 | 29 | status_set, | ||
81 | 29 | ) | 30 | ) |
82 | 30 | 31 | ||
83 | 31 | 32 | ||
84 | @@ -70,6 +71,11 @@ | |||
85 | 70 | retry_on_exception, | 71 | retry_on_exception, |
86 | 71 | ) | 72 | ) |
87 | 72 | 73 | ||
88 | 74 | from charmhelpers.core.unitdata import ( | ||
89 | 75 | HookData, | ||
90 | 76 | kv, | ||
91 | 77 | ) | ||
92 | 78 | |||
93 | 73 | 79 | ||
94 | 74 | CLUSTER_RES = "grp_glance_vips" | 80 | CLUSTER_RES = "grp_glance_vips" |
95 | 75 | 81 | ||
96 | @@ -491,3 +497,31 @@ | |||
97 | 491 | swift_connection.post_account(headers={'x-account-meta-temp-url-key': | 497 | swift_connection.post_account(headers={'x-account-meta-temp-url-key': |
98 | 492 | temp_url_key}) | 498 | temp_url_key}) |
99 | 493 | return temp_url_key | 499 | return temp_url_key |
100 | 500 | |||
101 | 501 | |||
102 | 502 | def is_paused(status_get=status_get): | ||
103 | 503 | """Is the unit paused?""" | ||
104 | 504 | with HookData()(): | ||
105 | 505 | if kv().get('unit-paused'): | ||
106 | 506 | return True | ||
107 | 507 | else: | ||
108 | 508 | return False | ||
109 | 509 | |||
110 | 510 | |||
111 | 511 | def assess_status(configs): | ||
112 | 512 | """Assess status of current unit | ||
113 | 513 | |||
114 | 514 | Decides what the state of the unit should be based on the current | ||
115 | 515 | configuration. | ||
116 | 516 | |||
117 | 517 | @param configs: a templating.OSConfigRenderer() object | ||
118 | 518 | """ | ||
119 | 519 | |||
120 | 520 | if is_paused(): | ||
121 | 521 | status_set("maintenance", | ||
122 | 522 | "Paused. Use 'resume' action to resume normal service.") | ||
123 | 523 | return | ||
124 | 524 | |||
125 | 525 | # set the status according to the current state of the contexts | ||
126 | 526 | set_os_workload_status( | ||
127 | 527 | configs, REQUIRED_INTERFACES, charm_func=check_optional_relations) | ||
128 | 494 | 528 | ||
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 | 1 | import os | 1 | import os |
134 | 2 | import mock | 2 | import mock |
135 | 3 | 3 | ||
136 | 4 | from test_utils import CharmTestCase | ||
137 | 5 | |||
138 | 6 | from mock import patch | ||
139 | 7 | |||
140 | 4 | os.environ['JUJU_UNIT_NAME'] = 'glance' | 8 | os.environ['JUJU_UNIT_NAME'] = 'glance' |
141 | 5 | 9 | ||
145 | 6 | from test_utils import CharmTestCase | 10 | with patch('actions.hooks.glance_utils.is_paused') as is_paused: |
146 | 7 | 11 | with patch('actions.hooks.glance_utils.register_configs') as configs: | |
147 | 8 | import actions.actions | 12 | import actions.actions |
148 | 9 | 13 | ||
149 | 10 | 14 | ||
150 | 11 | class PauseTestCase(CharmTestCase): | 15 | class PauseTestCase(CharmTestCase): |
151 | 12 | 16 | ||
152 | 13 | def setUp(self): | 17 | def setUp(self): |
153 | 14 | super(PauseTestCase, self).setUp( | 18 | super(PauseTestCase, self).setUp( |
155 | 15 | actions.actions, ["service_pause", "status_set"]) | 19 | actions.actions, ["service_pause", "HookData", "kv", |
156 | 20 | "assess_status"]) | ||
157 | 16 | 21 | ||
158 | 17 | def test_pauses_services(self): | 22 | def test_pauses_services(self): |
159 | 18 | """Pause action pauses all Glance services.""" | 23 | """Pause action pauses all Glance services.""" |
160 | @@ -46,32 +51,20 @@ | |||
161 | 46 | actions.actions.pause, []) | 51 | actions.actions.pause, []) |
162 | 47 | self.assertEqual(pause_calls, ['haproxy', 'glance-api']) | 52 | self.assertEqual(pause_calls, ['haproxy', 'glance-api']) |
163 | 48 | 53 | ||
183 | 49 | def test_status_mode(self): | 54 | def test_pause_sets_value(self): |
184 | 50 | """Pause action sets the status to maintenance.""" | 55 | """Pause action sets the unit-paused value to True.""" |
185 | 51 | status_calls = [] | 56 | self.HookData()().return_value = True |
186 | 52 | self.status_set.side_effect = lambda state, msg: status_calls.append( | 57 | |
187 | 53 | state) | 58 | actions.actions.pause([]) |
188 | 54 | 59 | self.kv().set.assert_called_with('unit-paused', True) | |
170 | 55 | actions.actions.pause([]) | ||
171 | 56 | self.assertEqual(status_calls, ["maintenance"]) | ||
172 | 57 | |||
173 | 58 | def test_status_message(self): | ||
174 | 59 | """Pause action sets a status message reflecting that it's paused.""" | ||
175 | 60 | status_calls = [] | ||
176 | 61 | self.status_set.side_effect = lambda state, msg: status_calls.append( | ||
177 | 62 | msg) | ||
178 | 63 | |||
179 | 64 | actions.actions.pause([]) | ||
180 | 65 | self.assertEqual( | ||
181 | 66 | status_calls, ["Paused. " | ||
182 | 67 | "Use 'resume' action to resume normal service."]) | ||
189 | 68 | 60 | ||
190 | 69 | 61 | ||
191 | 70 | class ResumeTestCase(CharmTestCase): | 62 | class ResumeTestCase(CharmTestCase): |
192 | 71 | 63 | ||
193 | 72 | def setUp(self): | 64 | def setUp(self): |
194 | 73 | super(ResumeTestCase, self).setUp( | 65 | super(ResumeTestCase, self).setUp( |
196 | 74 | actions.actions, ["service_resume", "status_set"]) | 66 | actions.actions, ["service_resume", "HookData", "kv", |
197 | 67 | "assess_status"]) | ||
198 | 75 | 68 | ||
199 | 76 | def test_resumes_services(self): | 69 | def test_resumes_services(self): |
200 | 77 | """Resume action resumes all Glance services.""" | 70 | """Resume action resumes all Glance services.""" |
201 | @@ -104,23 +97,12 @@ | |||
202 | 104 | actions.actions.resume, []) | 97 | actions.actions.resume, []) |
203 | 105 | self.assertEqual(resume_calls, ['haproxy', 'glance-api']) | 98 | self.assertEqual(resume_calls, ['haproxy', 'glance-api']) |
204 | 106 | 99 | ||
222 | 107 | def test_status_mode(self): | 100 | def test_resume_sets_value(self): |
223 | 108 | """Resume action sets the status to active.""" | 101 | """Resume action sets the unit-paused value to False.""" |
224 | 109 | status_calls = [] | 102 | self.HookData()().return_value = True |
225 | 110 | self.status_set.side_effect = lambda state, msg: status_calls.append( | 103 | |
226 | 111 | state) | 104 | actions.actions.resume([]) |
227 | 112 | 105 | self.kv().set.assert_called_with('unit-paused', False) | |
211 | 113 | actions.actions.resume([]) | ||
212 | 114 | self.assertEqual(status_calls, ["active"]) | ||
213 | 115 | |||
214 | 116 | def test_status_message(self): | ||
215 | 117 | """Resume action sets an empty status message.""" | ||
216 | 118 | status_calls = [] | ||
217 | 119 | self.status_set.side_effect = lambda state, msg: status_calls.append( | ||
218 | 120 | msg) | ||
219 | 121 | |||
220 | 122 | actions.actions.resume([]) | ||
221 | 123 | self.assertEqual(status_calls, [""]) | ||
228 | 124 | 106 | ||
229 | 125 | 107 | ||
230 | 126 | class MainTestCase(CharmTestCase): | 108 | class MainTestCase(CharmTestCase): |
231 | 127 | 109 | ||
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 | 293 | call('glance-registry'), | 293 | call('glance-registry'), |
237 | 294 | ] | 294 | ] |
238 | 295 | self.assertEquals(service_restart.call_args_list, expected) | 295 | self.assertEquals(service_restart.call_args_list, expected) |
239 | 296 | |||
240 | 297 | @patch.object(utils, 'HookData') | ||
241 | 298 | @patch.object(utils, 'kv') | ||
242 | 299 | def test_is_paused(self, kv, HookData): | ||
243 | 300 | """test_is_paused: Test is_paused() returns value | ||
244 | 301 | from kv('unit-paused')""" | ||
245 | 302 | HookData()().return_value = True | ||
246 | 303 | kv().get.return_value = True | ||
247 | 304 | self.assertEqual(utils.is_paused(), True) | ||
248 | 305 | kv().get.assert_called_with('unit-paused') | ||
249 | 306 | kv().get.return_value = False | ||
250 | 307 | self.assertEqual(utils.is_paused(), False) | ||
251 | 308 | |||
252 | 309 | @patch.object(utils, 'is_paused') | ||
253 | 310 | @patch.object(utils, 'status_set') | ||
254 | 311 | def test_assess_status(self, status_set, is_paused): | ||
255 | 312 | """test_assess_status: verify that it does pick the right status""" | ||
256 | 313 | # check that paused status does the right thing | ||
257 | 314 | is_paused.return_value = True | ||
258 | 315 | utils.assess_status(None) | ||
259 | 316 | status_set.assert_called_with( | ||
260 | 317 | "maintenance", | ||
261 | 318 | "Paused. Use 'resume' action to resume normal service.") | ||
262 | 319 | |||
263 | 320 | # if it isn't paused, the assess_status() calls | ||
264 | 321 | # set_os_workload_status() | ||
265 | 322 | is_paused.return_value = False | ||
266 | 323 | with patch.object(utils, 'set_os_workload_status') \ | ||
267 | 324 | as set_os_workload_status: | ||
268 | 325 | utils.assess_status("TEST CONFIG") | ||
269 | 326 | set_os_workload_status.assert_called_with( | ||
270 | 327 | "TEST CONFIG", | ||
271 | 328 | utils.REQUIRED_INTERFACES, | ||
272 | 329 | charm_func=utils.check_optional_relations) |
This look good, just a couple of comments inline