Merge lp:~frankban/juju-quickstart/handle-machine-errors into lp:juju-quickstart

[Francesco Banconi]

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://codereview.appspot.com/51350044/

[Francesco Banconi (frankban)]

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):
   A [revision details]
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_utils.py
   A quickstart/tests/test_watchers.py
   M quickstart/utils.py
   A quickstart/watchers.py

[Brad Crittenden (bac)]

Code LGTM. Have not yet done QA.

https://codereview.appspot.com/51350044/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/51350044/diff/1/quickstart/app.py#newcode467
quickstart/app.py:467: # specific unit. For this reason, we exit the for
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://codereview.appspot.com/51350044/diff/1/quickstart/app.py#newcode481
quickstart/app.py:481: # We assume the mega-watcher contains a single
change for each
As above.

https://codereview.appspot.com/51350044/

[lp:~frankban/juju-quickstart/handle-machine-errors]

54. By Francesco Banconi on 2014-01-17

Changes as per review.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/51350044/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/51350044/diff/1/quickstart/app.py#newcode467
quickstart/app.py:467: # specific unit. For this reason, we exit the for
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://codereview.appspot.com/51350044/diff/1/quickstart/app.py#newcode481
quickstart/app.py:481: # We assume the mega-watcher contains a single
change for each
On 2014/01/17 15:20:15, bac wrote:
> As above.

Done.

https://codereview.appspot.com/51350044/

[Richard Harding (rharding)]

LGTM thanks for the updates and a lot more tests.

https://codereview.appspot.com/51350044/

[Brad Crittenden (bac)]

qa-ok, modulo the issue we talked about on IRC.

https://codereview.appspot.com/51350044/

[lp:~frankban/juju-quickstart/handle-machine-errors]

55. By Francesco Banconi on 2014-01-17

Fix typo.

[Francesco Banconi (frankban)]

*** 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://codereview.appspot.com/51350044

https://codereview.appspot.com/51350044/

[Francesco Banconi (frankban)]

Thank you!

https://codereview.appspot.com/51350044/

=== modified file 'quickstart/app.py'
--- quickstart/app.py	2014-01-08 21:25:36 +0000
+++ quickstart/app.py	2014-01-17 18:31:35 +0000
@@ -21,7 +21,6 @@
     unicode_literals,
 )
 
-import functools
 import json
 import logging
 import os
@@ -34,6 +33,7 @@
     juju,
     settings,
     utils,
+    watchers,
 )
 
 
@@ -427,55 +427,60 @@
 
 
 def watch(env, unit_name):
-    """Start watching the given unit.
+    """Start watching the given unit and the machine the unit belongs to.
 
-    Output a human readable messages from each time a relevant change is found.
+    Output a human readable message each time a relevant change is found.
 
     The following changes are considered relevant for a healthy unit:
+        - the machine is pending;
         - the unit is pending;
+        - the machine is started;
         - the unit is reachable;
         - the unit is installed;
         - the unit is started.
 
     Stop watching and return the unit public address when the unit is started.
     Raise a ProgramExit if the API server returns an error response, or if
-    the service unit is removed or in error.
+    either the service unit or the machine is removed or in error.
     """
-    processor = functools.partial(utils.unit_changes, unit_name)
-    watcher = env.watch_changes(processor)
-    address = current_status = ''
+    address = unit_status = machine_id = machine_status = ''
+    watcher = env.watch_changes(watchers.unit_machine_changes)
     while True:
         try:
-            entity, action, data = watcher.next()
+            unit_changes, machine_changes = watcher.next()
         except jujuclient.EnvError as err:
             raise ProgramExit(
                 'bad API server response: {}'.format(err.message))
-        # Exit with an error if the unit is removed.
-        if action == 'remove':
-            raise ProgramExit('{} unexpectedly removed'.format(unit_name))
-        # Exit with an error if the unit is in an error state.
-        status = data['Status']
-        if 'error' in status:
-            msg = '{} is in an error state: {}: {}'.format(
-                unit_name, status, data['StatusInfo'])
-            raise ProgramExit(msg)
-        # Notify when the unit becomes reachable.
-        if not address:
-            address = data['PublicAddress']
-            if address:
-                print('setting up {} on {}'.format(unit_name, address))
-        # Notify status changes.
-        if status != current_status:
-            if status == 'pending':
-                print('{} deployment is pending'.format(unit_name))
-            elif status == 'installed':
-                print('{} is installed'.format(unit_name))
-            elif address and status == 'started':
-                # We can exit this loop.
-                print('{} is ready on machine {}'.format(
-                    unit_name, data['MachineId']))
-                return address
-            current_status = status
+        # Process unit changes:
+        for action, data in unit_changes:
+            if data['Name'] == unit_name:
+                try:
+                    data = watchers.parse_unit_change(
+                        action, data, unit_status, address)
+                except ValueError as err:
+                    raise ProgramExit(bytes(err))
+                unit_status, address, machine_id = data
+                if address and unit_status == 'started':
+                    # We can exit this loop.
+                    return address
+                # The mega-watcher contains a single change for each specific
+                # unit. For this reason, we can exit the for loop here.
+                break
+        if not machine_id:
+            # No need to process machine changes: we still don't know what
+            # machine the unit belongs to.
+            continue
+        # Process machine changes.
+        for action, data in machine_changes:
+            if data['Id'] == machine_id:
+                try:
+                    machine_status = watchers.parse_machine_change(
+                        action, data, machine_status)
+                except ValueError as err:
+                    raise ProgramExit(bytes(err))
+                # The mega-watcher contains a single change for each specific
+                # machine. For this reason, we can exit the for loop here.
+                break
 
 
 def deploy_bundle(env, bundle_yaml, bundle_name, bundle_id):
 
=== modified file 'quickstart/manage.py'
--- quickstart/manage.py	2014-01-16 12:41:47 +0000
+++ quickstart/manage.py	2014-01-17 18:31:35 +0000
@@ -413,14 +413,16 @@
 
     # Handle bundle deployment.
     if options.bundle is not None:
-        print('deploying the bundle {} with the following services: {}'.format(
-            options.bundle_name, ', '.join(options.bundle_services)))
+        services = ', '.join(options.bundle_services)
+        print('requesting a deployment of the {} bundle with the following '
+              'services:\n  {}'.format(options.bundle_name, services))
         # We need to connect to an API WebSocket server supporting bundle
         # deployments. The GUI builtin server, listening on the Juju GUI
         # address, exposes an API suitable for deploying bundles.
         app.deploy_bundle(
             gui_env, options.bundle_yaml, options.bundle_name,
             options.bundle_id)
+        print('bundle deployment request accepted')
 
     if options.open_browser:
         token = app.create_auth_token(gui_env)
@@ -435,7 +437,7 @@
         'Run "juju quickstart -i" if you want to manage\n'
         'or bootstrap your Juju environments using the\n'
         'interactive session.\n'
-        'Run "juju destroy-environment -e {env_name} [-y]"\n'
+        'Run "{sudo}juju destroy-environment -e {env_name} [-y]"\n'
         'to destroy the environment you just bootstrapped.'.format(
-            env_name=options.env_name)
+            env_name=options.env_name, sudo='sudo ' if is_local else '')
     )
 
=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2014-01-08 21:25:36 +0000
+++ quickstart/tests/test_app.py	2014-01-17 18:31:35 +0000
@@ -1009,34 +1009,50 @@
 
 
 @helpers.mock_print
-class TestWatch(ProgramExitTestsMixin, unittest.TestCase):
+class TestWatch(
+        ProgramExitTestsMixin, helpers.ValueErrorTestsMixin,
+        unittest.TestCase):
 
     address = 'unit.example.com'
-    change_pending = ('unit', 'change', {
+    change_machine_pending = ('change', {
+        'Id': '0',
+        'Status': 'pending',
+    })
+    change_machine_started = ('change', {
+        'Id': '0',
+        'Status': 'started',
+    })
+    change_unit_pending = ('change', {
+        'MachineId': '0',
         'Name': 'django/42',
         'PublicAddress': '',
         'Status': 'pending',
     })
-    change_reachable = ('unit', 'change', {
+    change_unit_reachable = ('change', {
+        'MachineId': '0',
         'Name': 'django/42',
         'PublicAddress': address,
         'Status': 'pending',
     })
-    change_installed = ('unit', 'change', {
+    change_unit_installed = ('change', {
+        'MachineId': '0',
         'Name': 'django/42',
         'PublicAddress': address,
         'Status': 'installed',
     })
-    change_started = ('unit', 'change', {
+    change_unit_started = ('change', {
         'MachineId': '0',
         'Name': 'django/42',
         'PublicAddress': address,
         'Status': 'started',
     })
-    pending_call = mock.call('django/42 deployment is pending')
-    reachable_call = mock.call('setting up django/42 on {}'.format(address))
-    installed_call = mock.call('django/42 is installed')
-    started_call = mock.call('django/42 is ready on machine 0')
+    machine_pending_call = mock.call('machine 0 provisioning is pending')
+    machine_started_call = mock.call('machine 0 is started')
+    unit_pending_call = mock.call('django/42 deployment is pending')
+    unit_reachable_call = mock.call(
+        'setting up django/42 on {}'.format(address))
+    unit_installed_call = mock.call('django/42 is installed')
+    unit_started_call = mock.call('django/42 is ready on machine 0')
 
     def make_env(self, changes):
         """Create and return a patched Environment instance.
@@ -1048,131 +1064,220 @@
         env.watch_changes().next.side_effect = changes
         return env
 
+    # The following group of tests exercises both the function return value and
+    # the function output, even if the output is handled by sub-functions.
+    # This is done to simulate the different user experiences of observing the
+    # environment evolution while the unit is deploying.
+
     def test_unit_life(self, mock_print):
         # The glorious moments in the unit's life are properly highlighted.
+        # The machine achievements are also celebrated.
         env = self.make_env([
-            self.change_pending,
-            self.change_reachable,
-            self.change_installed,
-            self.change_started,
+            ([self.change_unit_pending], [self.change_machine_pending]),
+            ([], [self.change_machine_started]),
+            ([self.change_unit_reachable], []),
+            ([self.change_unit_installed], []),
+            ([self.change_unit_started], []),
         ])
         address = app.watch(env, 'django/42')
         self.assertEqual(self.address, address)
+        self.assertEqual(6, mock_print.call_count)
         mock_print.assert_has_calls([
-            self.pending_call,
-            self.reachable_call,
-            self.installed_call,
-            self.started_call,
+            self.unit_pending_call,
+            self.machine_pending_call,
+            self.machine_started_call,
+            self.unit_reachable_call,
+            self.unit_installed_call,
+            self.unit_started_call,
         ])
 
     def test_weird_order(self, mock_print):
         # Strange unit evolutions are handled.
         env = self.make_env([
-            self.change_reachable,
-            self.change_pending,
-            self.change_installed,
-            self.change_started,
+            # The unit is first reachable and then pending. The machine starts
+            # when the unit is already installed. All of this makes no sense
+            # and should never happen, but if it does, we deal with it.
+            ([self.change_unit_reachable], []),
+            ([self.change_unit_pending], [self.change_machine_pending]),
+            ([self.change_unit_installed], []),
+            ([], [self.change_machine_started]),
+            ([self.change_unit_started], []),
         ])
         address = app.watch(env, 'django/42')
         self.assertEqual(self.address, address)
+        self.assertEqual(6, mock_print.call_count)
         mock_print.assert_has_calls([
-            self.reachable_call,
-            self.pending_call,
-            self.installed_call,
-            self.started_call,
+            self.unit_reachable_call,
+            self.unit_pending_call,
+            self.machine_pending_call,
+            self.unit_installed_call,
+            self.machine_started_call,
+            self.unit_started_call,
         ])
 
     def test_missing_changes(self, mock_print):
-        # Only the started change is strictly required.
-        env = self.make_env([self.change_started])
-        address = app.watch(env, 'django/42')
-        self.assertEqual(self.address, address)
-        mock_print.assert_has_calls([
-            self.reachable_call,
-            self.started_call,
+        # Only the unit started change is strictly required.
+        env = self.make_env([([self.change_unit_started], [])])
+        address = app.watch(env, 'django/42')
+        self.assertEqual(self.address, address)
+        self.assertEqual(2, mock_print.call_count)
+        mock_print.assert_has_calls([
+            self.unit_reachable_call,
+            self.unit_started_call,
+        ])
+
+    def test_ignored_machine_changes(self, mock_print):
+        # All machine changes are ignored until the application knows what
+        # machine the unit belongs to.
+        env = self.make_env([
+            ([], [self.change_machine_pending]),
+            ([], [self.change_machine_started]),
+            ([self.change_unit_started], []),
+        ])
+        address = app.watch(env, 'django/42')
+        self.assertEqual(self.address, address)
+        # No machine related messages have been printed.
+        self.assertEqual(2, mock_print.call_count)
+        mock_print.assert_has_calls([
+            self.unit_reachable_call,
+            self.unit_started_call,
+        ])
+
+    def test_unit_already_deployed(self, mock_print):
+        # Simulate the unit we are observing has been already deployed.
+        # This happens, e.g., when executing Quickstart a second time, and both
+        # the unit and the machine are already started.
+        env = self.make_env([
+            ([self.change_unit_started], [self.change_machine_started]),
+        ])
+        address = app.watch(env, 'django/42')
+        self.assertEqual(self.address, address)
+        self.assertEqual(2, mock_print.call_count)
+
+    def test_machine_already_started(self, mock_print):
+        # Simulate the unit is being deployed on an already started machine.
+        # This happens, e.g., when running Quickstart on a non-local
+        # environment type: the unit is deployed on the bootstrap node, which
+        # is assumed to be started.
+        env = self.make_env([
+            ([self.change_unit_pending], [self.change_machine_started]),
+            ([self.change_unit_reachable], []),
+            ([self.change_unit_installed], []),
+            ([self.change_unit_started], []),
+        ])
+        address = app.watch(env, 'django/42')
+        self.assertEqual(self.address, address)
+        self.assertEqual(5, mock_print.call_count)
+        mock_print.assert_has_calls([
+            self.unit_pending_call,
+            self.machine_started_call,
+            self.unit_reachable_call,
+            self.unit_installed_call,
+            self.unit_started_call,
         ])
 
     def test_extraneous_changes(self, mock_print):
-        # Extraneous changes are properly handled.
+        # Changes to units or machines we are not observing are ignored. Also
+        # ensure that repeated changes to a single entity are ignored, even if
+        # they are unlikely to happen.
+        change_another_machine_pending = ('change', {
+            'Id': '42',
+            'Status': 'pending',
+        })
+        change_another_machine_started = ('change', {
+            'Id': '1',
+            'Status': 'started',
+        })
+        change_another_unit_pending = ('change', {
+            'MachineId': '0',
+            'Name': 'haproxy/0',
+            'Status': 'pending',
+        })
+        change_another_unit_started = ('change', {
+            'MachineId': '0',
+            'Name': 'haproxy/0',
+            'Status': 'started',
+        })
         env = self.make_env([
-            self.change_pending,
-            ('unit', 'change', {
-                'Name': 'django/42',
-                'Status': 'pending',
-                'PublicAddress': '',
-                'Series': 'precise',
-            }),
-            self.change_reachable,
-            self.change_installed,
-            ('unit', 'change', {
-                'Name': 'django/42',
-                'Status': 'installed',
-                'PublicAddress': self.address,
-                'Series': 'precise',
-                'MachineId': '3',
-            }),
-            self.change_started,
+            # Add a repeated change.
+            ([self.change_unit_pending, self.change_unit_pending],
+             [self.change_machine_pending]),
+            # Add extraneous unit and machine changes.
+            ([change_another_unit_pending], [change_another_machine_pending]),
+            # Add a change to an extraneous machine.
+            ([], [change_another_machine_started,
+                  self.change_machine_started]),
+            # Add a change to an extraneous unit.
+            ([change_another_unit_started, self.change_unit_reachable], []),
+            ([self.change_unit_installed], []),
+            # Add another repeated change.
+            ([self.change_unit_started, self.change_unit_started], []),
         ])
         address = app.watch(env, 'django/42')
         self.assertEqual(self.address, address)
+        self.assertEqual(6, mock_print.call_count)
         mock_print.assert_has_calls([
-            self.pending_call,
-            self.reachable_call,
-            self.installed_call,
-            self.started_call,
+            self.unit_pending_call,
+            self.machine_pending_call,
+            self.machine_started_call,
+            self.unit_reachable_call,
+            self.unit_installed_call,
+            self.unit_started_call,
         ])
 
     def test_api_error(self, mock_print):
         # A ProgramExit is raised if an error occurs in one of the API calls.
         env = self.make_env([
-            self.change_pending,
+            ([self.change_unit_pending], []),
             self.make_env_error('next returned an error'),
         ])
         expected = 'bad API server response: next returned an error'
         with self.assert_program_exit(expected):
             app.watch(env, 'django/42')
-        mock_print.assert_has_calls([self.pending_call])
+        self.assertEqual(1, mock_print.call_count)
+        mock_print.assert_has_calls([self.unit_pending_call])
 
     def test_other_errors(self, mock_print):
         # Any other errors occurred during the process are not trapped.
         error = ValueError('explode!')
-        env = self.make_env([self.change_installed, error])
-        with self.assertRaises(ValueError) as context_manager:
+        env = self.make_env([([self.change_unit_installed], []), error])
+        with self.assert_value_error('explode!'):
             app.watch(env, 'django/42')
-        mock_print.assert_has_calls([self.reachable_call, self.installed_call])
-        self.assertIs(error, context_manager.exception)
+        self.assertEqual(2, mock_print.call_count)
+        mock_print.assert_has_calls([
+            self.unit_reachable_call, self.unit_installed_call])
 
-    def test_unit_removed(self, mock_print):
-        # A ProgramExit is raised if Juju removed the unit.
-        env = self.make_env([
-            self.change_pending,
-            ('unit', 'remove', {
-                'Name': 'django/42',
-                'Status': 'pending',
-                'PublicAddress': '',
-            }),
-            self.change_reachable,
+    def test_machine_status_error(self, mock_print):
+        # A ProgramExit is raised if an the machine is found in an error state.
+        change_machine_error = ('change', {
+            'Id': '0',
+            'Status': 'error',
+            'StatusInfo': 'oddities',
+        })
+        # The unit pending change is required to make the function know which
+        # machine to observe.
+        env = self.make_env([(
+            [self.change_unit_pending], [change_machine_error]),
         ])
-        with self.assert_program_exit('django/42 unexpectedly removed'):
+        expected = 'machine 0 is in an error state: error: oddities'
+        with self.assert_program_exit(expected):
             app.watch(env, 'django/42')
-        mock_print.assert_has_calls([self.pending_call])
+        self.assertEqual(1, mock_print.call_count)
+        mock_print.assert_has_calls([self.unit_pending_call])
 
-    def test_status_error(self, mock_print):
+    def test_unit_status_error(self, mock_print):
         # A ProgramExit is raised if an the unit is found in an error state.
-        env = self.make_env([
-            self.change_pending,
-            ('unit', 'change', {
-                'Name': 'django/42',
-                'Status': 'error',
-                'StatusInfo': 'install failure',
-                'PublicAddress': '',
-            }),
-            self.change_reachable,
-        ])
+        change_unit_error = ('change', {
+            'MachineId': '0',
+            'Name': 'django/42',
+            'Status': 'error',
+            'StatusInfo': 'install failure',
+        })
+        env = self.make_env([([change_unit_error], [])])
         expected = 'django/42 is in an error state: error: install failure'
         with self.assert_program_exit(expected):
             app.watch(env, 'django/42')
-        mock_print.assert_has_calls([self.pending_call])
+        self.assertFalse(mock_print.called)
 
 
 class TestDeployBundle(ProgramExitTestsMixin, unittest.TestCase):
 
=== modified file 'quickstart/tests/test_utils.py'
--- quickstart/tests/test_utils.py	2014-01-13 16:51:35 +0000
+++ quickstart/tests/test_utils.py	2014-01-17 18:31:35 +0000
@@ -620,54 +620,6 @@
         self.assertEqual(list.append.__doc__, self.func.__doc__)
 
 
-class TestUnitChanges(unittest.TestCase):
-
-    unit = 'django/42'
-
-    def test_change_found(self):
-        # A relevant change is correctly found and returned.
-        change = ('unit', 'change', {'Name': self.unit, 'Status': 'pending'})
-        self.assertEqual(change, utils.unit_changes(self.unit, [change]))
-
-    def test_change_found_removed(self):
-        # A unit removal is considered a relevant change.
-        change = ('unit', 'remove', {'Name': self.unit, 'Status': 'started'})
-        self.assertEqual(change, utils.unit_changes(self.unit, [change]))
-
-    def test_not_a_unit(self):
-        # Changes to other entities are ignored.
-        change = ('service', 'change', {'Name': 'django', 'Exposed': True})
-        self.assertIsNone(utils.unit_changes(self.unit, [change]))
-
-    def test_not_a_specific_unit(self):
-        # Changes to other units are ignored.
-        change = ('unit', 'change', {'Name': 'django/0', 'Status': 'pending'})
-        self.assertIsNone(utils.unit_changes(self.unit, [change]))
-
-    def test_multiple_changes(self):
-        # A relevant change is found between multiple ones.
-        change = ('unit', 'change', {'Name': self.unit, 'Status': 'pending'})
-        changeset = [
-            ('machine', 'change', {'Id': '0', 'Status': 'started'}),
-            change,
-            ('service', 'change', {'Name': 'django', 'Exposed': True}),
-        ]
-        self.assertEqual(change, utils.unit_changes(self.unit, changeset))
-
-    def test_multiple_unit_changes(self):
-        # A changeset is not supposed to include multiple change for a single
-        # unit. The function just picks the first change.
-        data = {'Name': self.unit, 'Status': 'pending'}
-        changeset = [
-            ('unit', 'change', data),
-            ('unit', 'remove', data),
-            ('unit', 'exterminated', data),
-        ]
-        self.assertEqual(
-            ('unit', 'change', data),
-            utils.unit_changes(self.unit, changeset))
-
-
 class TestUrlread(unittest.TestCase):
 
     def patch_urlopen(self, contents=None, error=None, content_type=None):
 
=== added file 'quickstart/tests/test_watchers.py'
--- quickstart/tests/test_watchers.py	1970-01-01 00:00:00 +0000
+++ quickstart/tests/test_watchers.py	2014-01-17 18:31:35 +0000
@@ -0,0 +1,266 @@
+# This file is part of the Juju Quickstart Plugin, which lets users set up a
+# Juju environment in very few steps (https://launchpad.net/juju-quickstart).
+# Copyright (C) 2014 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License version 3, as published by
+# the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+# SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Tests for the Juju Quickstart environments watching utilities."""
+
+from __future__ import unicode_literals
+
+import unittest
+
+import mock
+
+from quickstart import watchers
+from quickstart.tests import helpers
+
+
+class TestParseMachineChange(helpers.ValueErrorTestsMixin, unittest.TestCase):
+
+    def test_machine_removed(self):
+        # A ValueError is raised if the change represents a machine removal.
+        data = {'Id': '1', 'Status': 'started'}
+        with self.assert_value_error('machine 1 unexpectedly removed'):
+            watchers.parse_machine_change('remove', data, '')
+
+    def test_machine_error(self):
+        # A ValueError is raised if the machine is in an error state.
+        data = {'Id': '1', 'Status': 'error', 'StatusInfo': 'bad wolf'}
+        expected_error = 'machine 1 is in an error state: error: bad wolf'
+        with self.assert_value_error(expected_error):
+            watchers.parse_machine_change('change', data, '')
+
+    @helpers.mock_print
+    def test_pending_status_notified(self, mock_print):
+        # A message is printed to stdout when the machine changes its status
+        # to "pending". The new status is also returned by the function.
+        data = {'Id': '1', 'Status': 'pending'}
+        status = watchers.parse_machine_change('change', data, '')
+        self.assertEqual('pending', status)
+        mock_print.assert_called_once_with('machine 1 provisioning is pending')
+
+    @helpers.mock_print
+    def test_started_status_notified(self, mock_print):
+        # A message is printed to stdout when the machine changes its status
+        # to "started". The new status is also returned by the function.
+        data = {'Id': '42', 'Status': 'started'}
+        status = watchers.parse_machine_change('change', data, 'pending')
+        self.assertEqual('started', status)
+        mock_print.assert_called_once_with('machine 42 is started')
+
+    @helpers.mock_print
+    def test_status_not_changed(self, mock_print):
+        # If the status in the machine change and the given current status are
+        # the same value, nothing is printed and the status is returned.
+        data = {'Id': '47', 'Status': 'pending'}
+        status = watchers.parse_machine_change('change', data, 'pending')
+        self.assertEqual('pending', status)
+        self.assertFalse(mock_print.called)
+
+
+class TestParseUnitChange(helpers.ValueErrorTestsMixin, unittest.TestCase):
+
+    def test_unit_removed(self):
+        # A ValueError is raised if the change represents a unit removal.
+        data = {'Name': 'django/42', 'Status': 'started'}
+        with self.assert_value_error('django/42 unexpectedly removed'):
+            # The last two arguments are the current status and address.
+            watchers.parse_unit_change('remove', data, '', '')
+
+    def test_unit_error(self):
+        # A ValueError is raised if the unit is in an error state.
+        data = {
+            'Name': 'django/0',
+            'Status': 'start error',
+            'StatusInfo': 'bad wolf',
+        }
+        expected_error = 'django/0 is in an error state: start error: bad wolf'
+        with self.assert_value_error(expected_error):
+            # The last two arguments are the current status and address.
+            watchers.parse_unit_change('change', data, '', '')
+
+    @helpers.mock_print
+    def test_address_notified(self, mock_print):
+        # A message is printed to stdout when the unit obtains a public
+        # address. The function returns the status, the new address and the
+        # machine identifier.
+        data = {
+            'Name': 'haproxy/2',
+            'Status': 'pending',
+            'PublicAddress': 'haproxy2.example.com',
+            'MachineId': '42',
+        }
+        status, address, machine_id = watchers.parse_unit_change(
+            'change', data, 'pending', '')
+        self.assertEqual('pending', status)
+        self.assertEqual('haproxy2.example.com', address)
+        self.assertEqual('42', machine_id)
+        mock_print.assert_called_once_with(
+            'setting up haproxy/2 on haproxy2.example.com')
+
+    @helpers.mock_print
+    def test_pending_status_notified(self, mock_print):
+        # A message is printed to stdout when the unit changes its status to
+        # "pending". The function returns the new status, the address and the
+        # machine identifier. The last two values are empty strings if the unit
+        # has not yet been assigned to a machine.
+        data = {'Name': 'django/1', 'Status': 'pending', 'PublicAddress': ''}
+        # The last two arguments are the current status and address.
+        status, address, machine_id = watchers.parse_unit_change(
+            'change', data, '', '')
+        self.assertEqual('pending', status)
+        self.assertEqual('', address)
+        self.assertEqual('', machine_id)
+        mock_print.assert_called_once_with('django/1 deployment is pending')
+
+    @helpers.mock_print
+    def test_installed_status_notified(self, mock_print):
+        # A message is printed to stdout when the unit changes its status to
+        # "installed". The function returns the new status, the address and the
+        # machine identifier.
+        data = {
+            'Name': 'django/42',
+            'Status': 'installed',
+            'PublicAddress': 'django42.example.com',
+            'MachineId': '1',
+        }
+        status, address, machine_id = watchers.parse_unit_change(
+            'change', data, 'pending', 'django42.example.com')
+        self.assertEqual('installed', status)
+        self.assertEqual('django42.example.com', address)
+        self.assertEqual('1', machine_id)
+        mock_print.assert_called_once_with('django/42 is installed')
+
+    @helpers.mock_print
+    def test_started_status_notified(self, mock_print):
+        # A message is printed to stdout when the unit changes its status to
+        # "started". The function returns the new status, the address and the
+        # machine identifier.
+        data = {
+            'Name': 'wordpress/0',
+            'Status': 'started',
+            'PublicAddress': 'wordpress0.example.com',
+            'MachineId': '0',
+        }
+        status, address, machine_id = watchers.parse_unit_change(
+            'change', data, '', 'wordpress0.example.com')
+        self.assertEqual('started', status)
+        self.assertEqual('wordpress0.example.com', address)
+        self.assertEqual('0', machine_id)
+        mock_print.assert_called_once_with('wordpress/0 is ready on machine 0')
+
+    @helpers.mock_print
+    def test_both_status_and_address_notified(self, mock_print):
+        # Both status and public address changes are notified if required.
+        data = {
+            'Name': 'django/0',
+            'Status': 'started',
+            'PublicAddress': 'django42.example.com',
+            'MachineId': '0',
+        }
+        # The last two arguments are the current status and address.
+        watchers.parse_unit_change('change', data, '', '')
+        self.assertEqual(2, mock_print.call_count)
+        mock_print.assert_has_calls([
+            mock.call('setting up django/0 on django42.example.com'),
+            mock.call('django/0 is ready on machine 0'),
+        ])
+
+    @helpers.mock_print
+    def test_status_not_changed(self, mock_print):
+        # If the status in the unit change and the given current status are the
+        # same value, nothing is printed and the current values are returned.
+        data = {'Name': 'django/1', 'Status': 'pending', 'PublicAddress': ''}
+        status, address, machine_id = watchers.parse_unit_change(
+            'change', data, 'pending', '')
+        self.assertEqual('pending', status)
+        self.assertEqual('', address)
+        self.assertEqual('', machine_id)
+        self.assertFalse(mock_print.called)
+
+
+class TestUnitMachineChanges(unittest.TestCase):
+
+    def test_unit_changes_found(self):
+        # Unit changes are correctly found and returned.
+        data1 = {'Name': 'django/42', 'Status': 'started'}
+        data2 = {'Name': 'django/47', 'Status': 'pending'}
+        changeset = [('unit', 'change', data1), ('unit', 'remove', data2)]
+        expected_unit_changes = [('change', data1), ('remove', data2)]
+        self.assertEqual(
+            (expected_unit_changes, []),
+            watchers.unit_machine_changes(changeset))
+
+    def test_machine_changes_found(self):
+        # Machine changes are correctly found and returned.
+        data1 = {'Id': '0', 'Status': 'started'}
+        data2 = {'Id': '1', 'Status': 'error'}
+        changeset = [
+            ('machine', 'change', data1),
+            ('machine', 'remove', data2),
+        ]
+        expected_machine_changes = [('change', data1), ('remove', data2)]
+        self.assertEqual(
+            ([], expected_machine_changes),
+            watchers.unit_machine_changes(changeset))
+
+    def test_unit_and_machine_changes_found(self):
+        # Changes to unit and machines are reordered, grouped and returned.
+        machine_data1 = {'Id': '0', 'Status': 'started'}
+        machine_data2 = {'Id': '42', 'Status': 'started'}
+        unit_data1 = {'Name': 'django/42', 'Status': 'error'}
+        unit_data2 = {'Name': 'haproxy/47', 'Status': 'pending'}
+        unit_data3 = {'Name': 'wordpress/0', 'Status': 'installed'}
+        changeset = [
+            ('machine', 'change', machine_data1),
+            ('unit', 'change', unit_data1),
+            ('machine', 'remove', machine_data2),
+            ('unit', 'change', unit_data2),
+            ('unit', 'remove', unit_data3),
+        ]
+        expected_unit_changes = [
+            ('change', unit_data1),
+            ('change', unit_data2),
+            ('remove', unit_data3),
+        ]
+        expected_machine_changes = [
+            ('change', machine_data1),
+            ('remove', machine_data2),
+        ]
+        self.assertEqual(
+            (expected_unit_changes, expected_machine_changes),
+            watchers.unit_machine_changes(changeset))
+
+    def test_other_entities(self):
+        # Changes to other entities (like services) are ignored.
+        machine_data = {'Id': '0', 'Status': 'started'}
+        unit_data = {'Name': 'django/42', 'Status': 'error'}
+        changeset = [
+            ('machine', 'change', machine_data),
+            ('service', 'change', {'Name': 'django', 'Status': 'pending'}),
+            ('unit', 'change', unit_data),
+            ('service', 'remove', {'Name': 'haproxy', 'Status': 'started'}),
+        ]
+        expected_changes = (
+            [('change', unit_data)],
+            [('change', machine_data)],
+        )
+        self.assertEqual(
+            expected_changes, watchers.unit_machine_changes(changeset))
+
+    def test_empty_changeset(self):
+        # Two empty lists are returned if the changeset is empty.
+        # This should never occur in the real world, but it's tested here to
+        # demonstrate this function behavior.
+        self.assertEqual(([], []), watchers.unit_machine_changes([]))
 
=== modified file 'quickstart/utils.py'
--- quickstart/utils.py	2014-01-10 15:30:49 +0000
+++ quickstart/utils.py	2014-01-17 18:31:35 +0000
@@ -341,18 +341,6 @@
     return decorated
 
 
-def unit_changes(unit_name, changeset):
-    """Parse the changeset and return the change related to the given unit.
-
-    This function is intended to be used as a processor callable for the
-    watch_changes method of quickstart.juju.Environment.
-    """
-    for change in changeset:
-        entity, _, data = change
-        if entity == 'unit' and data['Name'] == unit_name:
-            return change
-
-
 def urlread(url):
     """Open the given URL and return the page contents.
 
 
=== added file 'quickstart/watchers.py'
--- quickstart/watchers.py	1970-01-01 00:00:00 +0000
+++ quickstart/watchers.py	2014-01-17 18:31:35 +0000
@@ -0,0 +1,118 @@
+# This file is part of the Juju Quickstart Plugin, which lets users set up a
+# Juju environment in very few steps (https://launchpad.net/juju-quickstart).
+# Copyright (C) 2014 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License version 3, as published by
+# the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+# SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Juju Quickstart utilities for watching Juju environments."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+)
+
+
+def parse_machine_change(action, data, current_status):
+    """Parse the given machine change.
+
+    The change is represented by the given action/data pair.
+    Also receive the last known machine status, which can be an empty string
+    if the information is unknown.
+
+    Output a human readable message each time a relevant change is found.
+
+    Return the machine status.
+    Raise a ValueError if the machine is removed or in an error state.
+    """
+    machine_id = data['Id']
+    status = data['Status']
+    # Exit with an error if the machine is removed.
+    if action == 'remove':
+        msg = 'machine {} unexpectedly removed'.format(machine_id)
+        raise ValueError(msg.encode('utf-8'))
+    if 'error' in status:
+        msg = 'machine {} is in an error state: {}: {}'.format(
+            machine_id, status, data['StatusInfo'])
+        raise ValueError(msg.encode('utf-8'))
+    # Notify status changes.
+    if status != current_status:
+        if status == 'pending':
+            print('machine {} provisioning is pending'.format(
+                machine_id))
+        elif status == 'started':
+            print('machine {} is started'.format(machine_id))
+    return status
+
+
+def parse_unit_change(action, data, current_status, address):
+    """Parse the given unit change.
+
+    The change is represented by the given action/data pair.
+    Also receive the last known unit status and address, which can be empty
+    strings if those pieces of information are unknown.
+
+    Output a human readable message each time a relevant change is found.
+
+    Return the unit status, address and machine identifier.
+    Raise a ValueError if the service unit is removed or in an error state.
+    """
+    unit_name = data['Name']
+    # Exit with an error if the unit is removed.
+    if action == 'remove':
+        msg = '{} unexpectedly removed'.format(unit_name)
+        raise ValueError(msg.encode('utf-8'))
+    # Exit with an error if the unit is in an error state.
+    status = data['Status']
+    if 'error' in status:
+        msg = '{} is in an error state: {}: {}'.format(
+            unit_name, status, data['StatusInfo'])
+        raise ValueError(msg.encode('utf-8'))
+    # Notify when the unit becomes reachable.
+    if not address:
+        address = data['PublicAddress']
+        if address:
+            print('setting up {} on {}'.format(unit_name, address))
+    # Notify status changes.
+    if status != current_status:
+        if status == 'pending':
+            print('{} deployment is pending'.format(unit_name))
+        elif status == 'installed':
+            print('{} is installed'.format(unit_name))
+        elif address and status == 'started':
+            print('{} is ready on machine {}'.format(
+                unit_name, data['MachineId']))
+    return status, address, data.get('MachineId', '')
+
+
+def unit_machine_changes(changeset):
+    """Parse the changeset and return the units and machines related changes.
+
+    Changes to units and machines are grouped into two lists, e.g.:
+
+        unit_changes, machine_changes = unit_machine_changes(changeset)
+
+    Each list includes (action, data) tuples, in which:
+        - action is he change type (e.g. "change", "remove");
+        - data is the actual information about the changed entity (as a dict).
+
+    This function is intended to be used as a processor callable for the
+    watch_changes method of quickstart.juju.Environment.
+    """
+    unit_changes = []
+    machine_changes = []
+    for entity, action, data in changeset:
+        if entity == 'unit':
+            unit_changes.append((action, data))
+        elif entity == 'machine':
+            machine_changes.append((action, data))
+    return unit_changes, machine_changes