Merge lp:~frankban/juju-quickstart/local-provider into lp:juju-quickstart

[Francesco Banconi]

Add support for local providers.

Bootstrap the environment with "sudo" if
the environment is configured to use the
local provider.

Also improved debugging documentation.

Tests: make check

To QA this, just run `.venv/bin/python juju-quickstart`
as you are used to, but with the local provider:
try to deploy a bundle, try to re-run quickstart again
with the environment already bootstrapped.
In general the application should ask for sudo password
and then proceed as usual.

https://codereview.appspot.com/29540044/

[Francesco Banconi (frankban)]

Reviewers: mp+195952_code.launchpad.net,

Message:
Please take a look.

Description:
Add support for local providers.

Bootstrap the environment with "sudo" if
the environment is configured to use the
local provider.

Also improved debugging documentation.

Tests: make check

To QA this, just run `.venv/bin/python juju-quickstart`
as you are used to, but with the local provider:
try to deploy a bundle, try to re-run quickstart again
with the environment already bootstrapped.
In general the application should ask for sudo password
and then proceed as usual.

https://code.launchpad.net/~frankban/juju-quickstart/local-provider/+merge/195952

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/29540044/

Affected files (+133, -39 lines):
   M HACKING.rst
   A [revision details]
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_manage.py
   M quickstart/utils.py

[Richard Harding (rharding)]

Code looks good, doing qa.

https://codereview.appspot.com/29540044/

[Richard Harding (rharding)]

LGTM + qa ok in local

https://codereview.appspot.com/29540044/

[Francesco Banconi (frankban)]

*** Submitted:

Add support for local providers.

Bootstrap the environment with "sudo" if
the environment is configured to use the
local provider.

Also improved debugging documentation.

Tests: make check

To QA this, just run `.venv/bin/python juju-quickstart`
as you are used to, but with the local provider:
try to deploy a bundle, try to re-run quickstart again
with the environment already bootstrapped.
In general the application should ask for sudo password
and then proceed as usual.

R=rharding
CC=
https://codereview.appspot.com/29540044

https://codereview.appspot.com/29540044/

[Francesco Banconi (frankban)]

Thank you!

https://codereview.appspot.com/29540044/

=== modified file 'HACKING.rst'
--- HACKING.rst	2013-11-18 18:01:22 +0000
+++ HACKING.rst	2013-11-20 12:26:05 +0000
@@ -134,11 +134,27 @@
 the deployment process using the GUI.
 
 Under the hood, a bundle deployment is executed by the GUI builtin server,
-which in turn leverages the juju-deployer library. Sometimes, when an error
-occurs, it is not obvious where to retrieve information about what is going on.
-The GUI builtin server exposes some bundle information in two places:
+which in turn leverages the juju-deployer library. Since juju-deployer is not
+asynchronous, the actual deployment is executed in a separate process.
+
+Sometimes, when an error occurs, it is not obvious where to retrieve
+information about what is going on. The GUI builtin server exposes some bundle
+information in two places:
 
 - https://<juju-gui-url>/gui-server-info displays in JSON format the current
   status of all scheduled/started/completed bundle deployments;
 - /var/log/upstart/guiserver.log is the builtin server log file, which includes
   logs output from the juju-deployer library.
+
+Moreover, setting `builtin-server-logging=debug` gives more debugging
+information, e.g. it prints to the log the contents of the WebSocket messages
+sent by the client (usually the Juju GUI) and by the Juju API server.
+As mentioned, juju-deployer works on its own sandbox and uses its own API
+connections, and for this reason the WebSocket traffic it generates is not
+logged.
+
+Sometimes, while debugging, it is convenient to restart the builtin server
+(which also empties the bundle deployments queue). To do that, run the
+following in the Juju GUI machine:
+
+    service guiserver restart

=== modified file 'quickstart/app.py'
--- quickstart/app.py	2013-11-18 20:34:09 +0000
+++ quickstart/app.py	2013-11-20 12:26:05 +0000
@@ -20,6 +20,7 @@
 import functools
 import json
 import logging
+import time
 
 import jujuclient
 
@@ -44,7 +45,7 @@
         return 'juju-quickstart: error: {}'.format(self.message)
 
 
-def bootstrap(env_name, debug=False):
+def bootstrap(env_name, is_local=False, debug=False):
     """Bootstrap the Juju environment with the given name.
 
     Do not bootstrap the environment if already bootstrapped.
@@ -52,6 +53,10 @@
     Return True without errors if the environment is already bootstrapped.
     Return False otherwise. Only return when the bootstrap node is ready.
 
+    The is_local argument indicates whether the environment is configured to
+    use the local provider. If so, sudo privileges are requested in order to
+    bootstrap the environment.
+
     If debug is True and the environment not bootstrapped, execute the
     bootstrap command passing the --debug flag.
 
@@ -59,6 +64,11 @@
     """
     already_bootstrapped = False
     cmd = ['juju', 'bootstrap', '-e', env_name]
+    # XXX frankban 2013-11-20: avoid using sudo once this is no longer required
+    # in juju-core.
+    if is_local:
+        print('sudo privileges required to bootstrap the environment')
+        cmd.insert(0, 'sudo')
     if debug:
         cmd.append('--debug')
     retcode, _, error = utils.call(*cmd)
@@ -82,8 +92,16 @@
         # Juju is bootstrapped, but we don't know if it is ready yet. Fall
         # through to the next block for that check.
         already_bootstrapped = True
+        print('reusing the already bootstrapped {} environment'.format(
+            env_name))
     # Call "juju status" multiple times until the bootstrap node is ready.
-    for _ in range(5):
+    # Exit with an error if the agent is not ready after ten minutes.
+    # Note: when using the local provider, calling "juju status" is very fast,
+    # but e.g. on ec2 the first call (right after "bootstrap") can take
+    # several minutes, and subsequent calls are relatively fast (seconds).
+    print('retrieving the environment status')
+    timeout = time.time() + (60*10)
+    while time.time() < timeout:
         retcode, output, error = utils.call(
             'juju', 'status', '-e', env_name, '--format', 'yaml')
         if retcode:
@@ -95,6 +113,10 @@
             continue
         if agent_state == 'started':
             return already_bootstrapped
+        # If the agent is in an error state, there is nothing we can do, and
+        # it's not useful to keep trying.
+        if agent_state == 'error':
+            raise ProgramExit('state server failure:\n{}'.format(output))
     details = ''.join(filter(None, [output, error])).strip()
     raise ProgramExit('the state server is not ready:\n{}'.format(details))
 

=== modified file 'quickstart/manage.py'
--- quickstart/manage.py	2013-11-19 12:22:32 +0000
+++ quickstart/manage.py	2013-11-20 12:26:05 +0000
@@ -110,9 +110,6 @@
         env_type, admin_secret = utils.parse_env_file(env_file, env_name)
     except ValueError as err:
         return parser.error(str(err))
-    # XXX 2013-10-15 frankban: add support for local providers.
-    if env_type == 'local':
-        return parser.error('the local provider is not currently supported')
     # Update the options namespace with the new values.
     options.admin_secret = admin_secret
     options.env_file = env_file
@@ -223,10 +220,9 @@
     print('juju quickstart v{}'.format(version))
     print('bootstrapping the {} environment (type: {})'.format(
         options.env_name, options.env_type))
-    already_bootstrapped = app.bootstrap(options.env_name, debug=options.debug)
-    if already_bootstrapped:
-        print('reusing the already bootstrapped {} environment'.format(
-            options.env_name))
+    is_local = options.env_type == 'local'
+    already_bootstrapped = app.bootstrap(
+        options.env_name, is_local=is_local, debug=options.debug)
 
     print('retrieving the Juju API address')
     api_url = app.get_api_url(options.env_name)

=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2013-11-18 20:34:09 +0000
+++ quickstart/tests/test_app.py	2013-11-20 12:26:05 +0000
@@ -61,10 +61,12 @@
         return jujuclient.EnvError({'Error': message})
 
 
+@mock_print
 class TestBootstrap(
         helpers.CallTestsMixin, ProgramExitTestsMixin, unittest.TestCase):
 
-    env_name = 'ec2'
+    env_name = 'my-juju-env'
+    status_message = 'retrieving the environment status'
 
     def make_status_output(self, agent_state):
         """Create and return a YAML status output."""
@@ -96,7 +98,7 @@
             mock.call('juju', 'bootstrap', '-e', self.env_name),
         ] + self.make_status_calls(5))
 
-    def test_success(self):
+    def test_success(self, mock_print):
         # The environment is successfully bootstrapped.
         with self.patch_multiple_calls(self.make_side_effects()) as mock_call:
             already_bootstrapped = app.bootstrap(self.env_name)
@@ -104,8 +106,22 @@
         mock_call.assert_has_calls([
             mock.call('juju', 'bootstrap', '-e', self.env_name),
         ] + self.make_status_calls(1))
-
-    def test_success_debug(self):
+        mock_print.assert_called_once_with(self.status_message)
+
+    def test_success_local_provider(self, mock_print):
+        # The environment is bootstrapped with sudo using the local provider.
+        with self.patch_multiple_calls(self.make_side_effects()) as mock_call:
+            already_bootstrapped = app.bootstrap(self.env_name, is_local=True)
+        self.assertFalse(already_bootstrapped)
+        mock_call.assert_has_calls([
+            mock.call('sudo', 'juju', 'bootstrap', '-e', self.env_name),
+        ] + self.make_status_calls(1))
+        mock_print.assert_has_calls([
+            mock.call('sudo privileges required to bootstrap the environment'),
+            mock.call(self.status_message),
+        ])
+
+    def test_success_debug(self, mock_print):
         # The environment is successfully bootstrapped in debug mode.
         with self.patch_multiple_calls(self.make_side_effects()) as mock_call:
             already_bootstrapped = app.bootstrap(self.env_name, debug=True)
@@ -114,7 +130,7 @@
             mock.call('juju', 'bootstrap', '-e', self.env_name, '--debug'),
         ] + self.make_status_calls(1))
 
-    def test_already_bootstrapped(self):
+    def test_already_bootstrapped(self, mock_print):
         # The function succeeds and returns True if the environment is already
         # bootstrapped.
         side_effects = [
@@ -127,8 +143,13 @@
         mock_call.assert_has_calls([
             mock.call('juju', 'bootstrap', '-e', self.env_name),
         ] + self.make_status_calls(1))
+        existing_message = 'reusing the already bootstrapped {} environment'
+        mock_print.assert_has_calls([
+            mock.call(existing_message.format(self.env_name)),
+            mock.call(self.status_message),
+        ])
 
-    def test_bootstrap_failure(self):
+    def test_bootstrap_failure(self, mock_print):
         # A ProgramExit is raised if an error occurs while bootstrapping.
         with self.patch_call(1, error='bad wolf') as mock_call:
             with self.assert_program_exit('bad wolf'):
@@ -136,7 +157,7 @@
         mock_call.assert_called_once_with(
             'juju', 'bootstrap', '-e', self.env_name)
 
-    def test_status_retry_error(self):
+    def test_status_retry_error(self, mock_print):
         # Before raising a ProgramExit, the functions tries to call
         # "juju status" multiple times if it exits with an error.
         side_effects = [
@@ -151,7 +172,7 @@
         ]
         self.assert_status_retried(side_effects)
 
-    def test_status_retry_invalid_output(self):
+    def test_status_retry_invalid_output(self, mock_print):
         # Before raising a ProgramExit, the functions tries to call
         # "juju status" multiple times if its output is not well formed or if
         # the agent is not started.
@@ -167,7 +188,7 @@
         ]
         self.assert_status_retried(side_effects)
 
-    def test_status_retry_both(self):
+    def test_status_retry_both(self, mock_print):
         # Before raising a ProgramExit, the functions tries to call
         # "juju status" multiple times in any case.
         side_effects = [
@@ -182,20 +203,47 @@
         ]
         self.assert_status_retried(side_effects)
 
-    def test_status_failure(self):
+    def test_agent_error(self, mock_print):
+        # A ProgramExit is raised immediately if the Juju agent in the
+        # bootstrap node is in an error state.
+        status_output = self.make_status_output('error')
+        side_effects = [
+            (0, '', ''),  # Add the bootstrap call.
+            (0, status_output, ''),  # Add the status call: agent error.
+        ]
+        expected = 'state server failure:\n{}'.format(status_output)
+        with self.patch_multiple_calls(side_effects) as mock_call:
+            with self.assert_program_exit(expected):
+                app.bootstrap(self.env_name)
+        mock_call.assert_has_calls([
+            mock.call('juju', 'bootstrap', '-e', self.env_name),
+        ] + self.make_status_calls(1))
+
+    def test_status_failure(self, mock_print):
         # A ProgramExit is raised if "juju status" keeps failing.
-        status_side_effects = [
-            (i, 'output #{}\n'.format(i), 'error #{}\n'.format(i))
-            for i in range(5)
-        ]
-        side_effects = [(0, '', '')] + status_side_effects
-        expected = 'the state server is not ready:\noutput #4\nerror #4'
-        with self.patch_multiple_calls(side_effects) as mock_call:
-            with self.assert_program_exit(expected):
-                app.bootstrap(self.env_name)
+        call_side_effects = [
+            (0, '', ''),  # Add the bootstrap call.
+            (1, 'output1', 'error1'),  # Add the first status call: retried.
+            (1, 'output2', 'error2'),  # Add the second status call: error.
+        ]
+        time_side_effects = [
+            0,  # Start at time zero (expiration at time 600).
+            10,  # First call before the timeout expiration.
+            100,  # Second call before the timeout expiration.
+            1000,  # Third call after the timeout expiration.
+        ]
+        mock_time = mock.Mock(side_effect=time_side_effects)
+        expected = 'the state server is not ready:\noutput2error2'
+        with self.patch_multiple_calls(call_side_effects) as mock_call:
+            # Simulate the timeout expired: the first time call is used to
+            # calculate the timeout, the second one for the first status check,
+            # the third for the second status check, the fourth should fail.
+            with mock.patch('time.time', mock_time):
+                with self.assert_program_exit(expected):
+                    app.bootstrap(self.env_name)
         mock_call.assert_has_calls([
             mock.call('juju', 'bootstrap', '-e', self.env_name),
-        ] + self.make_status_calls(5))
+        ] + self.make_status_calls(2))
 
 
 class TestGetApiUrl(

=== modified file 'quickstart/tests/test_manage.py'
--- quickstart/tests/test_manage.py	2013-11-19 12:22:32 +0000
+++ quickstart/tests/test_manage.py	2013-11-20 12:26:05 +0000
@@ -238,8 +238,10 @@
         env_file = self.make_env_file(contents)
         options = self.make_options(env_file, env_name='lxc')
         manage._validate_env(options, self.parser)
-        expected = 'the local provider is not currently supported'
-        self.parser.error.assert_called_once_with(expected)
+        self.assertEqual('Secret!', options.admin_secret)
+        self.assertEqual(env_file, options.env_file)
+        self.assertEqual('lxc', options.env_name)
+        self.assertEqual('local', options.env_type)
 
 
 @mock.patch('quickstart.manage._validate_env', mock.Mock())
@@ -352,7 +354,7 @@
         options = self.make_options()
         manage.run(options)
         mock_app.bootstrap.assert_called_once_with(
-            options.env_name, debug=options.debug)
+            options.env_name, is_local=False, debug=options.debug)
         mock_app.get_api_url.assert_called_once_with(options.env_name)
         mock_app.connect.assert_called_once_with(
             mock_app.get_api_url(), options.admin_secret)
@@ -377,6 +379,13 @@
             'wss://gui.example.com:443/ws', options.admin_secret,
             'mybundle: contents', 'mybundle', None)
 
+    def test_local_provider(self, mock_app, mock_open):
+        # The application correctly handles working with local providers.
+        options = self.make_options(env_type='local')
+        manage.run(options)
+        mock_app.bootstrap.assert_called_once_with(
+            options.env_name, is_local=True, debug=options.debug)
+
     def test_no_browser(self, mock_app, mock_open):
         # It is possible to avoid opening the GUI in the browser.
         options = self.make_options(open_browser=False)

=== modified file 'quickstart/utils.py'
--- quickstart/utils.py	2013-11-18 20:34:09 +0000
+++ quickstart/utils.py	2013-11-20 12:26:05 +0000
@@ -35,7 +35,7 @@
 _juju_switch_expression = re.compile(r'Current environment: "([\w-]+)"\n')
 
 
-def call(*args):
+def call(command, *args):
     """Call a subprocess passing the given arguments.
 
     Take the subcommand and its parameters as args.
@@ -43,14 +43,15 @@
     Return a tuple containing the subprocess return code, output and error.
     """
     pipe = subprocess.PIPE
-    cmdline = ' '.join(map(pipes.quote, args))
+    cmd = (command,) + args
+    cmdline = ' '.join(map(pipes.quote, cmd))
     logging.debug('running the following: {}'.format(cmdline))
     try:
-        process = subprocess.Popen(args, stdout=pipe, stderr=pipe)
+        process = subprocess.Popen(cmd, stdout=pipe, stderr=pipe)
     except OSError as err:
         # A return code 127 is returned by the shell when the command is not
         # found in the PATH.
-        return 127, '', '{}: {}'.format(args[0], err)
+        return 127, '', '{}: {}'.format(command, err)
     output, error = process.communicate()
     retcode = process.poll()
     logging.debug('retcode: {} | output: {!r} | error: {!r}'.format(