Merge lp:~frankban/juju-quickstart/handle-machine-errors into lp:juju-quickstart
- handle-machine-errors
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 47 |
Proposed branch: | lp:~frankban/juju-quickstart/handle-machine-errors |
Merge into: | lp:juju-quickstart |
Diff against target: |
982 lines (+618/-182) 7 files modified
quickstart/app.py (+39/-34) quickstart/manage.py (+6/-4) quickstart/tests/test_app.py (+189/-84) quickstart/tests/test_utils.py (+0/-48) quickstart/tests/test_watchers.py (+266/-0) quickstart/utils.py (+0/-12) quickstart/watchers.py (+118/-0) |
To merge this branch: | bzr merge lp:~frankban/juju-quickstart/handle-machine-errors |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju GUI Hackers | Pending | ||
Review via email: mp+202105@code.launchpad.net |
Commit message
Description of the change
Improve machine errors handling.
Quickstart no longer hangs when the GUI
machine goes in an "error" state and, as
a consequence, the unit is forever in
a "pending" state.
Also fixed the message printed at the
end of the process suggesting how to
destroy the environment
(sudo is used where required).
Tests: `make check`.
QA: I already asked Rick to reproduce the trusty/LXC
errors he encountered QAing my previous branch.
The GUI no longer hangs.
I'd also appreciate any other suggestion and additional
QA, given that this branch is a good candidate for 1.0.
Francesco Banconi (frankban) wrote : | # |
Brad Crittenden (bac) wrote : | # |
Code LGTM. Have not yet done QA.
https:/
File quickstart/app.py (right):
https:/
quickstart/
loop here.
You don't sound convinced. Any chance that assumption is wrong? The
penalty for looping over the rest non-matching items is trivial. But if
you're sure, maybe keep this logic but change the comment to be more
confident.
https:/
quickstart/
change for each
As above.
- 54. By Francesco Banconi
-
Changes as per review.
Francesco Banconi (frankban) wrote : | # |
Please take a look.
https:/
File quickstart/app.py (right):
https:/
quickstart/
loop here.
On 2014/01/17 15:20:15, bac wrote:
> You don't sound convinced. Any chance that assumption is wrong? The
penalty
> for looping over the rest non-matching items is trivial. But if
you're sure,
> maybe keep this logic but change the comment to be more confident.
Done.
https:/
quickstart/
change for each
On 2014/01/17 15:20:15, bac wrote:
> As above.
Done.
Richard Harding (rharding) wrote : | # |
LGTM thanks for the updates and a lot more tests.
Brad Crittenden (bac) wrote : | # |
qa-ok, modulo the issue we talked about on IRC.
- 55. By Francesco Banconi
-
Fix typo.
Francesco Banconi (frankban) wrote : | # |
*** Submitted:
Improve machine errors handling.
Quickstart no longer hangs when the GUI
machine goes in an "error" state and, as
a consequence, the unit is forever in
a "pending" state.
Also fixed the message printed at the
end of the process suggesting how to
destroy the environment
(sudo is used where required).
Tests: `make check`.
QA: I already asked Rick to reproduce the trusty/LXC
errors he encountered QAing my previous branch.
The GUI no longer hangs.
I'd also appreciate any other suggestion and additional
QA, given that this branch is a good candidate for 1.0.
R=bac, rharding
CC=
https:/
Francesco Banconi (frankban) wrote : | # |
Thank you!
Preview Diff
1 | === modified file 'quickstart/app.py' |
2 | --- quickstart/app.py 2014-01-08 21:25:36 +0000 |
3 | +++ quickstart/app.py 2014-01-17 18:31:35 +0000 |
4 | @@ -21,7 +21,6 @@ |
5 | unicode_literals, |
6 | ) |
7 | |
8 | -import functools |
9 | import json |
10 | import logging |
11 | import os |
12 | @@ -34,6 +33,7 @@ |
13 | juju, |
14 | settings, |
15 | utils, |
16 | + watchers, |
17 | ) |
18 | |
19 | |
20 | @@ -427,55 +427,60 @@ |
21 | |
22 | |
23 | def watch(env, unit_name): |
24 | - """Start watching the given unit. |
25 | + """Start watching the given unit and the machine the unit belongs to. |
26 | |
27 | - Output a human readable messages from each time a relevant change is found. |
28 | + Output a human readable message each time a relevant change is found. |
29 | |
30 | The following changes are considered relevant for a healthy unit: |
31 | + - the machine is pending; |
32 | - the unit is pending; |
33 | + - the machine is started; |
34 | - the unit is reachable; |
35 | - the unit is installed; |
36 | - the unit is started. |
37 | |
38 | Stop watching and return the unit public address when the unit is started. |
39 | Raise a ProgramExit if the API server returns an error response, or if |
40 | - the service unit is removed or in error. |
41 | + either the service unit or the machine is removed or in error. |
42 | """ |
43 | - processor = functools.partial(utils.unit_changes, unit_name) |
44 | - watcher = env.watch_changes(processor) |
45 | - address = current_status = '' |
46 | + address = unit_status = machine_id = machine_status = '' |
47 | + watcher = env.watch_changes(watchers.unit_machine_changes) |
48 | while True: |
49 | try: |
50 | - entity, action, data = watcher.next() |
51 | + unit_changes, machine_changes = watcher.next() |
52 | except jujuclient.EnvError as err: |
53 | raise ProgramExit( |
54 | 'bad API server response: {}'.format(err.message)) |
55 | - # Exit with an error if the unit is removed. |
56 | - if action == 'remove': |
57 | - raise ProgramExit('{} unexpectedly removed'.format(unit_name)) |
58 | - # Exit with an error if the unit is in an error state. |
59 | - status = data['Status'] |
60 | - if 'error' in status: |
61 | - msg = '{} is in an error state: {}: {}'.format( |
62 | - unit_name, status, data['StatusInfo']) |
63 | - raise ProgramExit(msg) |
64 | - # Notify when the unit becomes reachable. |
65 | - if not address: |
66 | - address = data['PublicAddress'] |
67 | - if address: |
68 | - print('setting up {} on {}'.format(unit_name, address)) |
69 | - # Notify status changes. |
70 | - if status != current_status: |
71 | - if status == 'pending': |
72 | - print('{} deployment is pending'.format(unit_name)) |
73 | - elif status == 'installed': |
74 | - print('{} is installed'.format(unit_name)) |
75 | - elif address and status == 'started': |
76 | - # We can exit this loop. |
77 | - print('{} is ready on machine {}'.format( |
78 | - unit_name, data['MachineId'])) |
79 | - return address |
80 | - current_status = status |
81 | + # Process unit changes: |
82 | + for action, data in unit_changes: |
83 | + if data['Name'] == unit_name: |
84 | + try: |
85 | + data = watchers.parse_unit_change( |
86 | + action, data, unit_status, address) |
87 | + except ValueError as err: |
88 | + raise ProgramExit(bytes(err)) |
89 | + unit_status, address, machine_id = data |
90 | + if address and unit_status == 'started': |
91 | + # We can exit this loop. |
92 | + return address |
93 | + # The mega-watcher contains a single change for each specific |
94 | + # unit. For this reason, we can exit the for loop here. |
95 | + break |
96 | + if not machine_id: |
97 | + # No need to process machine changes: we still don't know what |
98 | + # machine the unit belongs to. |
99 | + continue |
100 | + # Process machine changes. |
101 | + for action, data in machine_changes: |
102 | + if data['Id'] == machine_id: |
103 | + try: |
104 | + machine_status = watchers.parse_machine_change( |
105 | + action, data, machine_status) |
106 | + except ValueError as err: |
107 | + raise ProgramExit(bytes(err)) |
108 | + # The mega-watcher contains a single change for each specific |
109 | + # machine. For this reason, we can exit the for loop here. |
110 | + break |
111 | |
112 | |
113 | def deploy_bundle(env, bundle_yaml, bundle_name, bundle_id): |
114 | |
115 | === modified file 'quickstart/manage.py' |
116 | --- quickstart/manage.py 2014-01-16 12:41:47 +0000 |
117 | +++ quickstart/manage.py 2014-01-17 18:31:35 +0000 |
118 | @@ -413,14 +413,16 @@ |
119 | |
120 | # Handle bundle deployment. |
121 | if options.bundle is not None: |
122 | - print('deploying the bundle {} with the following services: {}'.format( |
123 | - options.bundle_name, ', '.join(options.bundle_services))) |
124 | + services = ', '.join(options.bundle_services) |
125 | + print('requesting a deployment of the {} bundle with the following ' |
126 | + 'services:\n {}'.format(options.bundle_name, services)) |
127 | # We need to connect to an API WebSocket server supporting bundle |
128 | # deployments. The GUI builtin server, listening on the Juju GUI |
129 | # address, exposes an API suitable for deploying bundles. |
130 | app.deploy_bundle( |
131 | gui_env, options.bundle_yaml, options.bundle_name, |
132 | options.bundle_id) |
133 | + print('bundle deployment request accepted') |
134 | |
135 | if options.open_browser: |
136 | token = app.create_auth_token(gui_env) |
137 | @@ -435,7 +437,7 @@ |
138 | 'Run "juju quickstart -i" if you want to manage\n' |
139 | 'or bootstrap your Juju environments using the\n' |
140 | 'interactive session.\n' |
141 | - 'Run "juju destroy-environment -e {env_name} [-y]"\n' |
142 | + 'Run "{sudo}juju destroy-environment -e {env_name} [-y]"\n' |
143 | 'to destroy the environment you just bootstrapped.'.format( |
144 | - env_name=options.env_name) |
145 | + env_name=options.env_name, sudo='sudo ' if is_local else '') |
146 | ) |
147 | |
148 | === modified file 'quickstart/tests/test_app.py' |
149 | --- quickstart/tests/test_app.py 2014-01-08 21:25:36 +0000 |
150 | +++ quickstart/tests/test_app.py 2014-01-17 18:31:35 +0000 |
151 | @@ -1009,34 +1009,50 @@ |
152 | |
153 | |
154 | @helpers.mock_print |
155 | -class TestWatch(ProgramExitTestsMixin, unittest.TestCase): |
156 | +class TestWatch( |
157 | + ProgramExitTestsMixin, helpers.ValueErrorTestsMixin, |
158 | + unittest.TestCase): |
159 | |
160 | address = 'unit.example.com' |
161 | - change_pending = ('unit', 'change', { |
162 | + change_machine_pending = ('change', { |
163 | + 'Id': '0', |
164 | + 'Status': 'pending', |
165 | + }) |
166 | + change_machine_started = ('change', { |
167 | + 'Id': '0', |
168 | + 'Status': 'started', |
169 | + }) |
170 | + change_unit_pending = ('change', { |
171 | + 'MachineId': '0', |
172 | 'Name': 'django/42', |
173 | 'PublicAddress': '', |
174 | 'Status': 'pending', |
175 | }) |
176 | - change_reachable = ('unit', 'change', { |
177 | + change_unit_reachable = ('change', { |
178 | + 'MachineId': '0', |
179 | 'Name': 'django/42', |
180 | 'PublicAddress': address, |
181 | 'Status': 'pending', |
182 | }) |
183 | - change_installed = ('unit', 'change', { |
184 | + change_unit_installed = ('change', { |
185 | + 'MachineId': '0', |
186 | 'Name': 'django/42', |
187 | 'PublicAddress': address, |
188 | 'Status': 'installed', |
189 | }) |
190 | - change_started = ('unit', 'change', { |
191 | + change_unit_started = ('change', { |
192 | 'MachineId': '0', |
193 | 'Name': 'django/42', |
194 | 'PublicAddress': address, |
195 | 'Status': 'started', |
196 | }) |
197 | - pending_call = mock.call('django/42 deployment is pending') |
198 | - reachable_call = mock.call('setting up django/42 on {}'.format(address)) |
199 | - installed_call = mock.call('django/42 is installed') |
200 | - started_call = mock.call('django/42 is ready on machine 0') |
201 | + machine_pending_call = mock.call('machine 0 provisioning is pending') |
202 | + machine_started_call = mock.call('machine 0 is started') |
203 | + unit_pending_call = mock.call('django/42 deployment is pending') |
204 | + unit_reachable_call = mock.call( |
205 | + 'setting up django/42 on {}'.format(address)) |
206 | + unit_installed_call = mock.call('django/42 is installed') |
207 | + unit_started_call = mock.call('django/42 is ready on machine 0') |
208 | |
209 | def make_env(self, changes): |
210 | """Create and return a patched Environment instance. |
211 | @@ -1048,131 +1064,220 @@ |
212 | env.watch_changes().next.side_effect = changes |
213 | return env |
214 | |
215 | + # The following group of tests exercises both the function return value and |
216 | + # the function output, even if the output is handled by sub-functions. |
217 | + # This is done to simulate the different user experiences of observing the |
218 | + # environment evolution while the unit is deploying. |
219 | + |
220 | def test_unit_life(self, mock_print): |
221 | # The glorious moments in the unit's life are properly highlighted. |
222 | + # The machine achievements are also celebrated. |
223 | env = self.make_env([ |
224 | - self.change_pending, |
225 | - self.change_reachable, |
226 | - self.change_installed, |
227 | - self.change_started, |
228 | + ([self.change_unit_pending], [self.change_machine_pending]), |
229 | + ([], [self.change_machine_started]), |
230 | + ([self.change_unit_reachable], []), |
231 | + ([self.change_unit_installed], []), |
232 | + ([self.change_unit_started], []), |
233 | ]) |
234 | address = app.watch(env, 'django/42') |
235 | self.assertEqual(self.address, address) |
236 | + self.assertEqual(6, mock_print.call_count) |
237 | mock_print.assert_has_calls([ |
238 | - self.pending_call, |
239 | - self.reachable_call, |
240 | - self.installed_call, |
241 | - self.started_call, |
242 | + self.unit_pending_call, |
243 | + self.machine_pending_call, |
244 | + self.machine_started_call, |
245 | + self.unit_reachable_call, |
246 | + self.unit_installed_call, |
247 | + self.unit_started_call, |
248 | ]) |
249 | |
250 | def test_weird_order(self, mock_print): |
251 | # Strange unit evolutions are handled. |
252 | env = self.make_env([ |
253 | - self.change_reachable, |
254 | - self.change_pending, |
255 | - self.change_installed, |
256 | - self.change_started, |
257 | + # The unit is first reachable and then pending. The machine starts |
258 | + # when the unit is already installed. All of this makes no sense |
259 | + # and should never happen, but if it does, we deal with it. |
260 | + ([self.change_unit_reachable], []), |
261 | + ([self.change_unit_pending], [self.change_machine_pending]), |
262 | + ([self.change_unit_installed], []), |
263 | + ([], [self.change_machine_started]), |
264 | + ([self.change_unit_started], []), |
265 | ]) |
266 | address = app.watch(env, 'django/42') |
267 | self.assertEqual(self.address, address) |
268 | + self.assertEqual(6, mock_print.call_count) |
269 | mock_print.assert_has_calls([ |
270 | - self.reachable_call, |
271 | - self.pending_call, |
272 | - self.installed_call, |
273 | - self.started_call, |
274 | + self.unit_reachable_call, |
275 | + self.unit_pending_call, |
276 | + self.machine_pending_call, |
277 | + self.unit_installed_call, |
278 | + self.machine_started_call, |
279 | + self.unit_started_call, |
280 | ]) |
281 | |
282 | def test_missing_changes(self, mock_print): |
283 | - # Only the started change is strictly required. |
284 | - env = self.make_env([self.change_started]) |
285 | - address = app.watch(env, 'django/42') |
286 | - self.assertEqual(self.address, address) |
287 | - mock_print.assert_has_calls([ |
288 | - self.reachable_call, |
289 | - self.started_call, |
290 | + # Only the unit started change is strictly required. |
291 | + env = self.make_env([([self.change_unit_started], [])]) |
292 | + address = app.watch(env, 'django/42') |
293 | + self.assertEqual(self.address, address) |
294 | + self.assertEqual(2, mock_print.call_count) |
295 | + mock_print.assert_has_calls([ |
296 | + self.unit_reachable_call, |
297 | + self.unit_started_call, |
298 | + ]) |
299 | + |
300 | + def test_ignored_machine_changes(self, mock_print): |
301 | + # All machine changes are ignored until the application knows what |
302 | + # machine the unit belongs to. |
303 | + env = self.make_env([ |
304 | + ([], [self.change_machine_pending]), |
305 | + ([], [self.change_machine_started]), |
306 | + ([self.change_unit_started], []), |
307 | + ]) |
308 | + address = app.watch(env, 'django/42') |
309 | + self.assertEqual(self.address, address) |
310 | + # No machine related messages have been printed. |
311 | + self.assertEqual(2, mock_print.call_count) |
312 | + mock_print.assert_has_calls([ |
313 | + self.unit_reachable_call, |
314 | + self.unit_started_call, |
315 | + ]) |
316 | + |
317 | + def test_unit_already_deployed(self, mock_print): |
318 | + # Simulate the unit we are observing has been already deployed. |
319 | + # This happens, e.g., when executing Quickstart a second time, and both |
320 | + # the unit and the machine are already started. |
321 | + env = self.make_env([ |
322 | + ([self.change_unit_started], [self.change_machine_started]), |
323 | + ]) |
324 | + address = app.watch(env, 'django/42') |
325 | + self.assertEqual(self.address, address) |
326 | + self.assertEqual(2, mock_print.call_count) |
327 | + |
328 | + def test_machine_already_started(self, mock_print): |
329 | + # Simulate the unit is being deployed on an already started machine. |
330 | + # This happens, e.g., when running Quickstart on a non-local |
331 | + # environment type: the unit is deployed on the bootstrap node, which |
332 | + # is assumed to be started. |
333 | + env = self.make_env([ |
334 | + ([self.change_unit_pending], [self.change_machine_started]), |
335 | + ([self.change_unit_reachable], []), |
336 | + ([self.change_unit_installed], []), |
337 | + ([self.change_unit_started], []), |
338 | + ]) |
339 | + address = app.watch(env, 'django/42') |
340 | + self.assertEqual(self.address, address) |
341 | + self.assertEqual(5, mock_print.call_count) |
342 | + mock_print.assert_has_calls([ |
343 | + self.unit_pending_call, |
344 | + self.machine_started_call, |
345 | + self.unit_reachable_call, |
346 | + self.unit_installed_call, |
347 | + self.unit_started_call, |
348 | ]) |
349 | |
350 | def test_extraneous_changes(self, mock_print): |
351 | - # Extraneous changes are properly handled. |
352 | + # Changes to units or machines we are not observing are ignored. Also |
353 | + # ensure that repeated changes to a single entity are ignored, even if |
354 | + # they are unlikely to happen. |
355 | + change_another_machine_pending = ('change', { |
356 | + 'Id': '42', |
357 | + 'Status': 'pending', |
358 | + }) |
359 | + change_another_machine_started = ('change', { |
360 | + 'Id': '1', |
361 | + 'Status': 'started', |
362 | + }) |
363 | + change_another_unit_pending = ('change', { |
364 | + 'MachineId': '0', |
365 | + 'Name': 'haproxy/0', |
366 | + 'Status': 'pending', |
367 | + }) |
368 | + change_another_unit_started = ('change', { |
369 | + 'MachineId': '0', |
370 | + 'Name': 'haproxy/0', |
371 | + 'Status': 'started', |
372 | + }) |
373 | env = self.make_env([ |
374 | - self.change_pending, |
375 | - ('unit', 'change', { |
376 | - 'Name': 'django/42', |
377 | - 'Status': 'pending', |
378 | - 'PublicAddress': '', |
379 | - 'Series': 'precise', |
380 | - }), |
381 | - self.change_reachable, |
382 | - self.change_installed, |
383 | - ('unit', 'change', { |
384 | - 'Name': 'django/42', |
385 | - 'Status': 'installed', |
386 | - 'PublicAddress': self.address, |
387 | - 'Series': 'precise', |
388 | - 'MachineId': '3', |
389 | - }), |
390 | - self.change_started, |
391 | + # Add a repeated change. |
392 | + ([self.change_unit_pending, self.change_unit_pending], |
393 | + [self.change_machine_pending]), |
394 | + # Add extraneous unit and machine changes. |
395 | + ([change_another_unit_pending], [change_another_machine_pending]), |
396 | + # Add a change to an extraneous machine. |
397 | + ([], [change_another_machine_started, |
398 | + self.change_machine_started]), |
399 | + # Add a change to an extraneous unit. |
400 | + ([change_another_unit_started, self.change_unit_reachable], []), |
401 | + ([self.change_unit_installed], []), |
402 | + # Add another repeated change. |
403 | + ([self.change_unit_started, self.change_unit_started], []), |
404 | ]) |
405 | address = app.watch(env, 'django/42') |
406 | self.assertEqual(self.address, address) |
407 | + self.assertEqual(6, mock_print.call_count) |
408 | mock_print.assert_has_calls([ |
409 | - self.pending_call, |
410 | - self.reachable_call, |
411 | - self.installed_call, |
412 | - self.started_call, |
413 | + self.unit_pending_call, |
414 | + self.machine_pending_call, |
415 | + self.machine_started_call, |
416 | + self.unit_reachable_call, |
417 | + self.unit_installed_call, |
418 | + self.unit_started_call, |
419 | ]) |
420 | |
421 | def test_api_error(self, mock_print): |
422 | # A ProgramExit is raised if an error occurs in one of the API calls. |
423 | env = self.make_env([ |
424 | - self.change_pending, |
425 | + ([self.change_unit_pending], []), |
426 | self.make_env_error('next returned an error'), |
427 | ]) |
428 | expected = 'bad API server response: next returned an error' |
429 | with self.assert_program_exit(expected): |
430 | app.watch(env, 'django/42') |
431 | - mock_print.assert_has_calls([self.pending_call]) |
432 | + self.assertEqual(1, mock_print.call_count) |
433 | + mock_print.assert_has_calls([self.unit_pending_call]) |
434 | |
435 | def test_other_errors(self, mock_print): |
436 | # Any other errors occurred during the process are not trapped. |
437 | error = ValueError('explode!') |
438 | - env = self.make_env([self.change_installed, error]) |
439 | - with self.assertRaises(ValueError) as context_manager: |
440 | + env = self.make_env([([self.change_unit_installed], []), error]) |
441 | + with self.assert_value_error('explode!'): |
442 | app.watch(env, 'django/42') |
443 | - mock_print.assert_has_calls([self.reachable_call, self.installed_call]) |
444 | - self.assertIs(error, context_manager.exception) |
445 | + self.assertEqual(2, mock_print.call_count) |
446 | + mock_print.assert_has_calls([ |
447 | + self.unit_reachable_call, self.unit_installed_call]) |
448 | |
449 | - def test_unit_removed(self, mock_print): |
450 | - # A ProgramExit is raised if Juju removed the unit. |
451 | - env = self.make_env([ |
452 | - self.change_pending, |
453 | - ('unit', 'remove', { |
454 | - 'Name': 'django/42', |
455 | - 'Status': 'pending', |
456 | - 'PublicAddress': '', |
457 | - }), |
458 | - self.change_reachable, |
459 | + def test_machine_status_error(self, mock_print): |
460 | + # A ProgramExit is raised if an the machine is found in an error state. |
461 | + change_machine_error = ('change', { |
462 | + 'Id': '0', |
463 | + 'Status': 'error', |
464 | + 'StatusInfo': 'oddities', |
465 | + }) |
466 | + # The unit pending change is required to make the function know which |
467 | + # machine to observe. |
468 | + env = self.make_env([( |
469 | + [self.change_unit_pending], [change_machine_error]), |
470 | ]) |
471 | - with self.assert_program_exit('django/42 unexpectedly removed'): |
472 | + expected = 'machine 0 is in an error state: error: oddities' |
473 | + with self.assert_program_exit(expected): |
474 | app.watch(env, 'django/42') |
475 | - mock_print.assert_has_calls([self.pending_call]) |
476 | + self.assertEqual(1, mock_print.call_count) |
477 | + mock_print.assert_has_calls([self.unit_pending_call]) |
478 | |
479 | - def test_status_error(self, mock_print): |
480 | + def test_unit_status_error(self, mock_print): |
481 | # A ProgramExit is raised if an the unit is found in an error state. |
482 | - env = self.make_env([ |
483 | - self.change_pending, |
484 | - ('unit', 'change', { |
485 | - 'Name': 'django/42', |
486 | - 'Status': 'error', |
487 | - 'StatusInfo': 'install failure', |
488 | - 'PublicAddress': '', |
489 | - }), |
490 | - self.change_reachable, |
491 | - ]) |
492 | + change_unit_error = ('change', { |
493 | + 'MachineId': '0', |
494 | + 'Name': 'django/42', |
495 | + 'Status': 'error', |
496 | + 'StatusInfo': 'install failure', |
497 | + }) |
498 | + env = self.make_env([([change_unit_error], [])]) |
499 | expected = 'django/42 is in an error state: error: install failure' |
500 | with self.assert_program_exit(expected): |
501 | app.watch(env, 'django/42') |
502 | - mock_print.assert_has_calls([self.pending_call]) |
503 | + self.assertFalse(mock_print.called) |
504 | |
505 | |
506 | class TestDeployBundle(ProgramExitTestsMixin, unittest.TestCase): |
507 | |
508 | === modified file 'quickstart/tests/test_utils.py' |
509 | --- quickstart/tests/test_utils.py 2014-01-13 16:51:35 +0000 |
510 | +++ quickstart/tests/test_utils.py 2014-01-17 18:31:35 +0000 |
511 | @@ -620,54 +620,6 @@ |
512 | self.assertEqual(list.append.__doc__, self.func.__doc__) |
513 | |
514 | |
515 | -class TestUnitChanges(unittest.TestCase): |
516 | - |
517 | - unit = 'django/42' |
518 | - |
519 | - def test_change_found(self): |
520 | - # A relevant change is correctly found and returned. |
521 | - change = ('unit', 'change', {'Name': self.unit, 'Status': 'pending'}) |
522 | - self.assertEqual(change, utils.unit_changes(self.unit, [change])) |
523 | - |
524 | - def test_change_found_removed(self): |
525 | - # A unit removal is considered a relevant change. |
526 | - change = ('unit', 'remove', {'Name': self.unit, 'Status': 'started'}) |
527 | - self.assertEqual(change, utils.unit_changes(self.unit, [change])) |
528 | - |
529 | - def test_not_a_unit(self): |
530 | - # Changes to other entities are ignored. |
531 | - change = ('service', 'change', {'Name': 'django', 'Exposed': True}) |
532 | - self.assertIsNone(utils.unit_changes(self.unit, [change])) |
533 | - |
534 | - def test_not_a_specific_unit(self): |
535 | - # Changes to other units are ignored. |
536 | - change = ('unit', 'change', {'Name': 'django/0', 'Status': 'pending'}) |
537 | - self.assertIsNone(utils.unit_changes(self.unit, [change])) |
538 | - |
539 | - def test_multiple_changes(self): |
540 | - # A relevant change is found between multiple ones. |
541 | - change = ('unit', 'change', {'Name': self.unit, 'Status': 'pending'}) |
542 | - changeset = [ |
543 | - ('machine', 'change', {'Id': '0', 'Status': 'started'}), |
544 | - change, |
545 | - ('service', 'change', {'Name': 'django', 'Exposed': True}), |
546 | - ] |
547 | - self.assertEqual(change, utils.unit_changes(self.unit, changeset)) |
548 | - |
549 | - def test_multiple_unit_changes(self): |
550 | - # A changeset is not supposed to include multiple change for a single |
551 | - # unit. The function just picks the first change. |
552 | - data = {'Name': self.unit, 'Status': 'pending'} |
553 | - changeset = [ |
554 | - ('unit', 'change', data), |
555 | - ('unit', 'remove', data), |
556 | - ('unit', 'exterminated', data), |
557 | - ] |
558 | - self.assertEqual( |
559 | - ('unit', 'change', data), |
560 | - utils.unit_changes(self.unit, changeset)) |
561 | - |
562 | - |
563 | class TestUrlread(unittest.TestCase): |
564 | |
565 | def patch_urlopen(self, contents=None, error=None, content_type=None): |
566 | |
567 | === added file 'quickstart/tests/test_watchers.py' |
568 | --- quickstart/tests/test_watchers.py 1970-01-01 00:00:00 +0000 |
569 | +++ quickstart/tests/test_watchers.py 2014-01-17 18:31:35 +0000 |
570 | @@ -0,0 +1,266 @@ |
571 | +# This file is part of the Juju Quickstart Plugin, which lets users set up a |
572 | +# Juju environment in very few steps (https://launchpad.net/juju-quickstart). |
573 | +# Copyright (C) 2014 Canonical Ltd. |
574 | +# |
575 | +# This program is free software: you can redistribute it and/or modify it under |
576 | +# the terms of the GNU Affero General Public License version 3, as published by |
577 | +# the Free Software Foundation. |
578 | +# |
579 | +# This program is distributed in the hope that it will be useful, but WITHOUT |
580 | +# ANY WARRANTY; without even the implied warranties of MERCHANTABILITY, |
581 | +# SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU |
582 | +# Affero General Public License for more details. |
583 | +# |
584 | +# You should have received a copy of the GNU Affero General Public License |
585 | +# along with this program. If not, see <http://www.gnu.org/licenses/>. |
586 | + |
587 | +"""Tests for the Juju Quickstart environments watching utilities.""" |
588 | + |
589 | +from __future__ import unicode_literals |
590 | + |
591 | +import unittest |
592 | + |
593 | +import mock |
594 | + |
595 | +from quickstart import watchers |
596 | +from quickstart.tests import helpers |
597 | + |
598 | + |
599 | +class TestParseMachineChange(helpers.ValueErrorTestsMixin, unittest.TestCase): |
600 | + |
601 | + def test_machine_removed(self): |
602 | + # A ValueError is raised if the change represents a machine removal. |
603 | + data = {'Id': '1', 'Status': 'started'} |
604 | + with self.assert_value_error('machine 1 unexpectedly removed'): |
605 | + watchers.parse_machine_change('remove', data, '') |
606 | + |
607 | + def test_machine_error(self): |
608 | + # A ValueError is raised if the machine is in an error state. |
609 | + data = {'Id': '1', 'Status': 'error', 'StatusInfo': 'bad wolf'} |
610 | + expected_error = 'machine 1 is in an error state: error: bad wolf' |
611 | + with self.assert_value_error(expected_error): |
612 | + watchers.parse_machine_change('change', data, '') |
613 | + |
614 | + @helpers.mock_print |
615 | + def test_pending_status_notified(self, mock_print): |
616 | + # A message is printed to stdout when the machine changes its status |
617 | + # to "pending". The new status is also returned by the function. |
618 | + data = {'Id': '1', 'Status': 'pending'} |
619 | + status = watchers.parse_machine_change('change', data, '') |
620 | + self.assertEqual('pending', status) |
621 | + mock_print.assert_called_once_with('machine 1 provisioning is pending') |
622 | + |
623 | + @helpers.mock_print |
624 | + def test_started_status_notified(self, mock_print): |
625 | + # A message is printed to stdout when the machine changes its status |
626 | + # to "started". The new status is also returned by the function. |
627 | + data = {'Id': '42', 'Status': 'started'} |
628 | + status = watchers.parse_machine_change('change', data, 'pending') |
629 | + self.assertEqual('started', status) |
630 | + mock_print.assert_called_once_with('machine 42 is started') |
631 | + |
632 | + @helpers.mock_print |
633 | + def test_status_not_changed(self, mock_print): |
634 | + # If the status in the machine change and the given current status are |
635 | + # the same value, nothing is printed and the status is returned. |
636 | + data = {'Id': '47', 'Status': 'pending'} |
637 | + status = watchers.parse_machine_change('change', data, 'pending') |
638 | + self.assertEqual('pending', status) |
639 | + self.assertFalse(mock_print.called) |
640 | + |
641 | + |
642 | +class TestParseUnitChange(helpers.ValueErrorTestsMixin, unittest.TestCase): |
643 | + |
644 | + def test_unit_removed(self): |
645 | + # A ValueError is raised if the change represents a unit removal. |
646 | + data = {'Name': 'django/42', 'Status': 'started'} |
647 | + with self.assert_value_error('django/42 unexpectedly removed'): |
648 | + # The last two arguments are the current status and address. |
649 | + watchers.parse_unit_change('remove', data, '', '') |
650 | + |
651 | + def test_unit_error(self): |
652 | + # A ValueError is raised if the unit is in an error state. |
653 | + data = { |
654 | + 'Name': 'django/0', |
655 | + 'Status': 'start error', |
656 | + 'StatusInfo': 'bad wolf', |
657 | + } |
658 | + expected_error = 'django/0 is in an error state: start error: bad wolf' |
659 | + with self.assert_value_error(expected_error): |
660 | + # The last two arguments are the current status and address. |
661 | + watchers.parse_unit_change('change', data, '', '') |
662 | + |
663 | + @helpers.mock_print |
664 | + def test_address_notified(self, mock_print): |
665 | + # A message is printed to stdout when the unit obtains a public |
666 | + # address. The function returns the status, the new address and the |
667 | + # machine identifier. |
668 | + data = { |
669 | + 'Name': 'haproxy/2', |
670 | + 'Status': 'pending', |
671 | + 'PublicAddress': 'haproxy2.example.com', |
672 | + 'MachineId': '42', |
673 | + } |
674 | + status, address, machine_id = watchers.parse_unit_change( |
675 | + 'change', data, 'pending', '') |
676 | + self.assertEqual('pending', status) |
677 | + self.assertEqual('haproxy2.example.com', address) |
678 | + self.assertEqual('42', machine_id) |
679 | + mock_print.assert_called_once_with( |
680 | + 'setting up haproxy/2 on haproxy2.example.com') |
681 | + |
682 | + @helpers.mock_print |
683 | + def test_pending_status_notified(self, mock_print): |
684 | + # A message is printed to stdout when the unit changes its status to |
685 | + # "pending". The function returns the new status, the address and the |
686 | + # machine identifier. The last two values are empty strings if the unit |
687 | + # has not yet been assigned to a machine. |
688 | + data = {'Name': 'django/1', 'Status': 'pending', 'PublicAddress': ''} |
689 | + # The last two arguments are the current status and address. |
690 | + status, address, machine_id = watchers.parse_unit_change( |
691 | + 'change', data, '', '') |
692 | + self.assertEqual('pending', status) |
693 | + self.assertEqual('', address) |
694 | + self.assertEqual('', machine_id) |
695 | + mock_print.assert_called_once_with('django/1 deployment is pending') |
696 | + |
697 | + @helpers.mock_print |
698 | + def test_installed_status_notified(self, mock_print): |
699 | + # A message is printed to stdout when the unit changes its status to |
700 | + # "installed". The function returns the new status, the address and the |
701 | + # machine identifier. |
702 | + data = { |
703 | + 'Name': 'django/42', |
704 | + 'Status': 'installed', |
705 | + 'PublicAddress': 'django42.example.com', |
706 | + 'MachineId': '1', |
707 | + } |
708 | + status, address, machine_id = watchers.parse_unit_change( |
709 | + 'change', data, 'pending', 'django42.example.com') |
710 | + self.assertEqual('installed', status) |
711 | + self.assertEqual('django42.example.com', address) |
712 | + self.assertEqual('1', machine_id) |
713 | + mock_print.assert_called_once_with('django/42 is installed') |
714 | + |
715 | + @helpers.mock_print |
716 | + def test_started_status_notified(self, mock_print): |
717 | + # A message is printed to stdout when the unit changes its status to |
718 | + # "started". The function returns the new status, the address and the |
719 | + # machine identifier. |
720 | + data = { |
721 | + 'Name': 'wordpress/0', |
722 | + 'Status': 'started', |
723 | + 'PublicAddress': 'wordpress0.example.com', |
724 | + 'MachineId': '0', |
725 | + } |
726 | + status, address, machine_id = watchers.parse_unit_change( |
727 | + 'change', data, '', 'wordpress0.example.com') |
728 | + self.assertEqual('started', status) |
729 | + self.assertEqual('wordpress0.example.com', address) |
730 | + self.assertEqual('0', machine_id) |
731 | + mock_print.assert_called_once_with('wordpress/0 is ready on machine 0') |
732 | + |
733 | + @helpers.mock_print |
734 | + def test_both_status_and_address_notified(self, mock_print): |
735 | + # Both status and public address changes are notified if required. |
736 | + data = { |
737 | + 'Name': 'django/0', |
738 | + 'Status': 'started', |
739 | + 'PublicAddress': 'django42.example.com', |
740 | + 'MachineId': '0', |
741 | + } |
742 | + # The last two arguments are the current status and address. |
743 | + watchers.parse_unit_change('change', data, '', '') |
744 | + self.assertEqual(2, mock_print.call_count) |
745 | + mock_print.assert_has_calls([ |
746 | + mock.call('setting up django/0 on django42.example.com'), |
747 | + mock.call('django/0 is ready on machine 0'), |
748 | + ]) |
749 | + |
750 | + @helpers.mock_print |
751 | + def test_status_not_changed(self, mock_print): |
752 | + # If the status in the unit change and the given current status are the |
753 | + # same value, nothing is printed and the current values are returned. |
754 | + data = {'Name': 'django/1', 'Status': 'pending', 'PublicAddress': ''} |
755 | + status, address, machine_id = watchers.parse_unit_change( |
756 | + 'change', data, 'pending', '') |
757 | + self.assertEqual('pending', status) |
758 | + self.assertEqual('', address) |
759 | + self.assertEqual('', machine_id) |
760 | + self.assertFalse(mock_print.called) |
761 | + |
762 | + |
763 | +class TestUnitMachineChanges(unittest.TestCase): |
764 | + |
765 | + def test_unit_changes_found(self): |
766 | + # Unit changes are correctly found and returned. |
767 | + data1 = {'Name': 'django/42', 'Status': 'started'} |
768 | + data2 = {'Name': 'django/47', 'Status': 'pending'} |
769 | + changeset = [('unit', 'change', data1), ('unit', 'remove', data2)] |
770 | + expected_unit_changes = [('change', data1), ('remove', data2)] |
771 | + self.assertEqual( |
772 | + (expected_unit_changes, []), |
773 | + watchers.unit_machine_changes(changeset)) |
774 | + |
775 | + def test_machine_changes_found(self): |
776 | + # Machine changes are correctly found and returned. |
777 | + data1 = {'Id': '0', 'Status': 'started'} |
778 | + data2 = {'Id': '1', 'Status': 'error'} |
779 | + changeset = [ |
780 | + ('machine', 'change', data1), |
781 | + ('machine', 'remove', data2), |
782 | + ] |
783 | + expected_machine_changes = [('change', data1), ('remove', data2)] |
784 | + self.assertEqual( |
785 | + ([], expected_machine_changes), |
786 | + watchers.unit_machine_changes(changeset)) |
787 | + |
788 | + def test_unit_and_machine_changes_found(self): |
789 | + # Changes to unit and machines are reordered, grouped and returned. |
790 | + machine_data1 = {'Id': '0', 'Status': 'started'} |
791 | + machine_data2 = {'Id': '42', 'Status': 'started'} |
792 | + unit_data1 = {'Name': 'django/42', 'Status': 'error'} |
793 | + unit_data2 = {'Name': 'haproxy/47', 'Status': 'pending'} |
794 | + unit_data3 = {'Name': 'wordpress/0', 'Status': 'installed'} |
795 | + changeset = [ |
796 | + ('machine', 'change', machine_data1), |
797 | + ('unit', 'change', unit_data1), |
798 | + ('machine', 'remove', machine_data2), |
799 | + ('unit', 'change', unit_data2), |
800 | + ('unit', 'remove', unit_data3), |
801 | + ] |
802 | + expected_unit_changes = [ |
803 | + ('change', unit_data1), |
804 | + ('change', unit_data2), |
805 | + ('remove', unit_data3), |
806 | + ] |
807 | + expected_machine_changes = [ |
808 | + ('change', machine_data1), |
809 | + ('remove', machine_data2), |
810 | + ] |
811 | + self.assertEqual( |
812 | + (expected_unit_changes, expected_machine_changes), |
813 | + watchers.unit_machine_changes(changeset)) |
814 | + |
815 | + def test_other_entities(self): |
816 | + # Changes to other entities (like services) are ignored. |
817 | + machine_data = {'Id': '0', 'Status': 'started'} |
818 | + unit_data = {'Name': 'django/42', 'Status': 'error'} |
819 | + changeset = [ |
820 | + ('machine', 'change', machine_data), |
821 | + ('service', 'change', {'Name': 'django', 'Status': 'pending'}), |
822 | + ('unit', 'change', unit_data), |
823 | + ('service', 'remove', {'Name': 'haproxy', 'Status': 'started'}), |
824 | + ] |
825 | + expected_changes = ( |
826 | + [('change', unit_data)], |
827 | + [('change', machine_data)], |
828 | + ) |
829 | + self.assertEqual( |
830 | + expected_changes, watchers.unit_machine_changes(changeset)) |
831 | + |
832 | + def test_empty_changeset(self): |
833 | + # Two empty lists are returned if the changeset is empty. |
834 | + # This should never occur in the real world, but it's tested here to |
835 | + # demonstrate this function behavior. |
836 | + self.assertEqual(([], []), watchers.unit_machine_changes([])) |
837 | |
838 | === modified file 'quickstart/utils.py' |
839 | --- quickstart/utils.py 2014-01-10 15:30:49 +0000 |
840 | +++ quickstart/utils.py 2014-01-17 18:31:35 +0000 |
841 | @@ -341,18 +341,6 @@ |
842 | return decorated |
843 | |
844 | |
845 | -def unit_changes(unit_name, changeset): |
846 | - """Parse the changeset and return the change related to the given unit. |
847 | - |
848 | - This function is intended to be used as a processor callable for the |
849 | - watch_changes method of quickstart.juju.Environment. |
850 | - """ |
851 | - for change in changeset: |
852 | - entity, _, data = change |
853 | - if entity == 'unit' and data['Name'] == unit_name: |
854 | - return change |
855 | - |
856 | - |
857 | def urlread(url): |
858 | """Open the given URL and return the page contents. |
859 | |
860 | |
861 | === added file 'quickstart/watchers.py' |
862 | --- quickstart/watchers.py 1970-01-01 00:00:00 +0000 |
863 | +++ quickstart/watchers.py 2014-01-17 18:31:35 +0000 |
864 | @@ -0,0 +1,118 @@ |
865 | +# This file is part of the Juju Quickstart Plugin, which lets users set up a |
866 | +# Juju environment in very few steps (https://launchpad.net/juju-quickstart). |
867 | +# Copyright (C) 2014 Canonical Ltd. |
868 | +# |
869 | +# This program is free software: you can redistribute it and/or modify it under |
870 | +# the terms of the GNU Affero General Public License version 3, as published by |
871 | +# the Free Software Foundation. |
872 | +# |
873 | +# This program is distributed in the hope that it will be useful, but WITHOUT |
874 | +# ANY WARRANTY; without even the implied warranties of MERCHANTABILITY, |
875 | +# SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. See the GNU |
876 | +# Affero General Public License for more details. |
877 | +# |
878 | +# You should have received a copy of the GNU Affero General Public License |
879 | +# along with this program. If not, see <http://www.gnu.org/licenses/>. |
880 | + |
881 | +"""Juju Quickstart utilities for watching Juju environments.""" |
882 | + |
883 | +from __future__ import ( |
884 | + print_function, |
885 | + unicode_literals, |
886 | +) |
887 | + |
888 | + |
889 | +def parse_machine_change(action, data, current_status): |
890 | + """Parse the given machine change. |
891 | + |
892 | + The change is represented by the given action/data pair. |
893 | + Also receive the last known machine status, which can be an empty string |
894 | + if the information is unknown. |
895 | + |
896 | + Output a human readable message each time a relevant change is found. |
897 | + |
898 | + Return the machine status. |
899 | + Raise a ValueError if the machine is removed or in an error state. |
900 | + """ |
901 | + machine_id = data['Id'] |
902 | + status = data['Status'] |
903 | + # Exit with an error if the machine is removed. |
904 | + if action == 'remove': |
905 | + msg = 'machine {} unexpectedly removed'.format(machine_id) |
906 | + raise ValueError(msg.encode('utf-8')) |
907 | + if 'error' in status: |
908 | + msg = 'machine {} is in an error state: {}: {}'.format( |
909 | + machine_id, status, data['StatusInfo']) |
910 | + raise ValueError(msg.encode('utf-8')) |
911 | + # Notify status changes. |
912 | + if status != current_status: |
913 | + if status == 'pending': |
914 | + print('machine {} provisioning is pending'.format( |
915 | + machine_id)) |
916 | + elif status == 'started': |
917 | + print('machine {} is started'.format(machine_id)) |
918 | + return status |
919 | + |
920 | + |
921 | +def parse_unit_change(action, data, current_status, address): |
922 | + """Parse the given unit change. |
923 | + |
924 | + The change is represented by the given action/data pair. |
925 | + Also receive the last known unit status and address, which can be empty |
926 | + strings if those pieces of information are unknown. |
927 | + |
928 | + Output a human readable message each time a relevant change is found. |
929 | + |
930 | + Return the unit status, address and machine identifier. |
931 | + Raise a ValueError if the service unit is removed or in an error state. |
932 | + """ |
933 | + unit_name = data['Name'] |
934 | + # Exit with an error if the unit is removed. |
935 | + if action == 'remove': |
936 | + msg = '{} unexpectedly removed'.format(unit_name) |
937 | + raise ValueError(msg.encode('utf-8')) |
938 | + # Exit with an error if the unit is in an error state. |
939 | + status = data['Status'] |
940 | + if 'error' in status: |
941 | + msg = '{} is in an error state: {}: {}'.format( |
942 | + unit_name, status, data['StatusInfo']) |
943 | + raise ValueError(msg.encode('utf-8')) |
944 | + # Notify when the unit becomes reachable. |
945 | + if not address: |
946 | + address = data['PublicAddress'] |
947 | + if address: |
948 | + print('setting up {} on {}'.format(unit_name, address)) |
949 | + # Notify status changes. |
950 | + if status != current_status: |
951 | + if status == 'pending': |
952 | + print('{} deployment is pending'.format(unit_name)) |
953 | + elif status == 'installed': |
954 | + print('{} is installed'.format(unit_name)) |
955 | + elif address and status == 'started': |
956 | + print('{} is ready on machine {}'.format( |
957 | + unit_name, data['MachineId'])) |
958 | + return status, address, data.get('MachineId', '') |
959 | + |
960 | + |
961 | +def unit_machine_changes(changeset): |
962 | + """Parse the changeset and return the units and machines related changes. |
963 | + |
964 | + Changes to units and machines are grouped into two lists, e.g.: |
965 | + |
966 | + unit_changes, machine_changes = unit_machine_changes(changeset) |
967 | + |
968 | + Each list includes (action, data) tuples, in which: |
969 | + - action is he change type (e.g. "change", "remove"); |
970 | + - data is the actual information about the changed entity (as a dict). |
971 | + |
972 | + This function is intended to be used as a processor callable for the |
973 | + watch_changes method of quickstart.juju.Environment. |
974 | + """ |
975 | + unit_changes = [] |
976 | + machine_changes = [] |
977 | + for entity, action, data in changeset: |
978 | + if entity == 'unit': |
979 | + unit_changes.append((action, data)) |
980 | + elif entity == 'machine': |
981 | + machine_changes.append((action, data)) |
982 | + return unit_changes, machine_changes |
Reviewers: mp+202105_ code.launchpad. net,
Message:
Please take a look.
Description:
Improve machine errors handling.
Quickstart no longer hangs when the GUI
machine goes in an "error" state and, as
a consequence, the unit is forever in
a "pending" state.
Also fixed the message printed at the
end of the process suggesting how to
destroy the environment
(sudo is used where required).
Tests: `make check`.
QA: I already asked Rick to reproduce the trusty/LXC
errors he encountered QAing my previous branch.
The GUI no longer hangs.
I'd also appreciate any other suggestion and additional
QA, given that this branch is a good candidate for 1.0.
https:/ /code.launchpad .net/~frankban/ juju-quickstart /handle- machine- errors/ +merge/ 202105
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/51350044/
Affected files (+616, -180 lines): manage. py tests/test_ app.py tests/test_ utils.py tests/test_ watchers. py watchers. py
A [revision details]
M quickstart/app.py
M quickstart/
M quickstart/
M quickstart/
A quickstart/
M quickstart/utils.py
A quickstart/