Merge lp:~frankban/juju-quickstart/bundle-v4-api into lp:juju-quickstart

[Francesco Banconi]

Send the actual bundle version to GUI server.

When deploying bundles, if not taken from the
charm store (in which case they are always v4 bundles),
check the bundle syntax version and properly
inform the GUI server about whether a legacy
or new-style bundle is being deployed.
This is done if the GUI unit supports new bundle
v4, otherwise still try to proceed with the
deployment process, pretending a v3 bundle has been
provided.

Drop support for utopic and add support for wily.

Also update default Juju GUI revisions.

Tests: `make check`.

QA: deploy a bundle v4, preferably with unit placement,
e.g.: `devenv/bin/juju-quickstart -u http://dpaste.com/2E1JA1E.txt`.
The uncommitted state should co-locate units in the proper way,
thanks to the GUI fixes from Madison. Now remove the -u flag:
`devenv/bin/juju-quickstart http://dpaste.com/2E1JA1E.txt` and check
that also the GUI server is able to properly deploy v4 bundles.
But also try the usual mediawiki-single, etc...
Run quickstart again, run the interactive mode, ensure
quickstart works as usual, this branch will be the 2.2.1 release.

[j.c.sackett (jcsackett)]

Francesco--

Thanks, this looks good. I noted one typo, and I have one question about what things look like to the end user in the worst case; otherwise LGTM.

review: Approve

[j.c.sackett (jcsackett)]

QA notes:

When running with -u everything worked great.

When running without the -u I got the following error in the GUI.

"n error occurred while deploying the bundle: Invalid service placement nova-compute to ceph/0 Invalid service placement nova-compute to ceph/1 Invalid service placement nova-compute to ceph/2 Invalid service placement ceph-radosgw to lxc:ceph/1 Invalid service placement neutron-api to lxc:ceph/0 Invalid service placement nova-cloud-controller to lxc:ceph/2 Invalid service placement keystone to lxc:ceph/0 Invalid service placement cinder to lxc:ceph/2 Invalid service placement openstack-dashboard to lxc:ceph/1 Invalid service placement glance to lxc:ceph/1"

Not sure if this is a qa error or an indication that I've got the wrong thing running. I'm running trusty/juju-gui-38, which is the minimum listed revision for V4 bundles; is there a restriction on which providers this works on? I'm using a local env.

[Francesco Banconi (frankban)]

Thanks for the review JC, replied in line.

[lp:~frankban/juju-quickstart/bundle-v4-api]

146. By Francesco Banconi on 2015-08-12

Fix typo.

[Francesco Banconi (frankban)]

I cannot dupe this QA problem.
Could you please run "devenv/bin/juju-quickstart -e ec2 http://dpaste.com/2E1JA1E.txt --debug" again? Thanks!

[j.c.sackett (jcsackett)]

On EC2 I'm getting the same issue, log is https://pastebin.canonical.com/137407/

[Francesco Banconi (frankban)]

From the logs it seems that quickstart is not considering the bundle a v4 one ("Version": 4 is not included in the params of the Import WebSocket request).
This is not the case here locally. So could you please make another attempt, just to exclude local misconfigurations? "make clean", then "make" and then "devenv/bin/juju-quickstart -e ec2 http://dpaste.com/2E1JA1E.txt --debug" again? Thanks a lot.

[Madison Scott-Clary (makyo)]

LGTM, thanks for the update

review: Approve (code)

[j.c.sackett (jcsackett)]

After blowing away the branch and getting this again, it seems to work. I see Version: 4 in the bundle output and the bundle has deployed.

QA OK.

review: Approve

=== modified file 'README.rst'
--- README.rst	2015-08-07 10:24:11 +0000
+++ README.rst	2015-08-12 16:25:17 +0000
@@ -34,7 +34,7 @@
 ------------------
 
 Juju Quickstart is available on Ubuntu releases 12.04 LTS (precise), 14.04 LTS
-(trusty), 15.04 (vivid) and on OS X (10.7 and later).
+(trusty), 15.04 (vivid), 15.10 (wily) and on OS X (10.7 and later).
 
 Starting from version 1.5.0, Juju Quickstart only supports Juju >= 1.18.1.
 
 
=== modified file 'quickstart/app.py'
--- quickstart/app.py	2015-06-05 13:07:47 +0000
+++ quickstart/app.py	2015-08-12 16:25:17 +0000
@@ -683,10 +683,28 @@
 
     print('requesting a deployment of {} with the following services:\n'
           '  {}'.format(bundle, services))
-    # XXX frankban 2015-02-26: use new bundle format if the GUI server is
-    # capable of handling bundle deployments with the API version 4.
-    yaml = bundle.serialize_legacy()
-    version = 3
+    version = bundle.version
+
+    if version > 3:
+        # The bundle uses a new-style syntax. Check whether the currently used
+        # GUI server is capable of handling new-style bundles.
+        revision = settings.MINIMUM_REVISIONS_FOR_V4_BUNDLES[charm_ref.series]
+        if jujugui.is_promulgated(charm_ref) and charm_ref.revision < revision:
+            logging.warn(
+                'new style bundle syntax is not supported by currently '
+                'deployed Juju GUI charm ({}): trying to deploy the bundle '
+                'using old syntax'.format(charm_ref))
+            # Pretend the bundle is a legacy one. Note that this might not work
+            # depending on whether new style unit placement is used in the
+            # bundle contents.
+            yaml = bundle.serialize_legacy()
+            version = 3
+        else:
+            yaml = bundle.serialize()
+    else:
+        # The bundle uses the legacy syntax.
+        yaml = bundle.serialize_legacy()
+
     # XXX frankban 2015-02-26: find and implement a better way to increase the
     # bundle deployments count.
     bundle_id = None if ref is None else ref.charmworld_id
 
=== modified file 'quickstart/models/bundles.py'
--- quickstart/models/bundles.py	2015-05-13 08:46:33 +0000
+++ quickstart/models/bundles.py	2015-08-12 16:25:17 +0000
@@ -63,7 +63,7 @@
 class Bundle(object):
     """Store information about a charm store bundle entity"""
 
-    def __init__(self, data, reference=None):
+    def __init__(self, data, reference=None, version=4):
         """Initialize the bundle.
 
         The data argument is the bundle YAML decoded content.
@@ -72,6 +72,7 @@
         """
         self.data = data
         self.reference = reference
+        self.version = version
 
     def __str__(self):
         """Return the byte string representation of this bundle."""
@@ -118,7 +119,7 @@
 
 
 def from_source(source, name=None):
-    """Return a bundle YAML encoded string and id from the given source.
+    """Return a bundle instance from the given source.
 
     The source argument is a string, and can be provided as:
 
@@ -167,16 +168,18 @@
     is_remote = source.startswith('http://') or source.startswith('https://')
     if has_extension and not is_remote:
         # The source refers to a local file.
-        data = _parse_and_flatten_yaml(_retrieve_from_file(source), name)
-        return Bundle(data)
+        content = _retrieve_from_file(source)
+        data, version = _parse_and_flatten_yaml(content, name)
+        return Bundle(data, version=version)
 
     try:
         reference = references.Reference.from_jujucharms_url(source)
     except ValueError:
         if is_remote:
             # The source is an arbitrary URL to a YAML/JSON content.
-            data = _parse_and_flatten_yaml(_retrieve_from_url(source), name)
-            return Bundle(data)
+            content = _retrieve_from_url(source)
+            data, version = _parse_and_flatten_yaml(content, name)
+            return Bundle(data, version=version)
         # No other options are available.
         raise
 
@@ -214,7 +217,8 @@
     Note that charmworld URLs always represent a bundle.
 
     Return a Bundle instance which includes the retrieved data and the bundle
-    reference corresponding to the given charmworld URL.
+    reference corresponding to the given charmworld URL. The returned bundle
+    is retrieved from the charm store and always uses version 4 syntax.
     Raise a ValueError if the provided URL is not valid, or if the bundle
     content is not valid.
     Raise a IOError if a problem is encountered while fetching the YAML
@@ -325,8 +329,8 @@
 
     The content is provided as a YAML encoded string and can be either a new
     style flat bundle or a legacy bundle format.
-    In both cases, the returned YAML decoded data represents a new style
-    bundle (API version 4).
+    In both cases, the returned YAML decoded data represents a single flat
+    bundle. The version is returned as an integer number.
 
     Raise a ValueError if:
         - the bundle YAML contents are not parsable by YAML;
@@ -338,17 +342,18 @@
     """
     data = charmstore.load_bundle_yaml(content)
     _ensure_is_dict(data)
+    version = 4
     if is_legacy_bundle(data):
         data = _flatten_data(data, name)
+        version = 3
     validate(data)
-    return data
+    return data, version
 
 
 def _flatten_data(data, name):
     """Retrieve the bundle content from data for a specific bundle name.
 
-    The returned YAML decoded data represents a new style bundle
-    (API version 4).
+    The returned YAML decoded data represents a single flat bundle.
 
     Raise a ValueError if:
         - the YAML data is not properly structured;
 
=== modified file 'quickstart/settings.py'
--- quickstart/settings.py	2015-06-17 14:02:03 +0000
+++ quickstart/settings.py	2015-08-12 16:25:17 +0000
@@ -37,8 +37,8 @@
 # temporary connection/charm store errors.
 # Keep this list sorted by release date (older first).
 DEFAULT_CHARM_URLS = collections.OrderedDict((
-    ('precise', 'cs:precise/juju-gui-117'),
-    ('trusty', 'cs:trusty/juju-gui-32'),
+    ('precise', 'cs:precise/juju-gui-121'),
+    ('trusty', 'cs:trusty/juju-gui-38'),
 ))
 
 # The quickstart app short description.
@@ -61,7 +61,8 @@
 
 # The possible values for the environments.yaml default-series field.
 JUJU_DEFAULT_SERIES = (
-    'precise', 'quantal', 'raring', 'saucy', 'trusty', 'utopic', 'vivid')
+    'precise', 'quantal', 'raring', 'saucy', 'trusty', 'utopic', 'vivid',
+    'wily')
 
 # Retrieve the current juju-core home.
 JUJU_HOME = os.getenv('JUJU_HOME', '~/.juju')
@@ -96,3 +97,8 @@
 # deployments. Assume not listed series to always support uncommitted state.
 MINIMUM_REVISIONS_FOR_UNCOMMITTED_BUNDLES = collections.defaultdict(
     lambda: 0, {'precise': 115, 'trusty': 28})
+
+# The minimum Juju GUI charm revision supporting bundle syntax version 4.
+# Assume not listed series to always support v4 syntax.
+MINIMUM_REVISIONS_FOR_V4_BUNDLES = collections.defaultdict(
+    lambda: 0, {'precise': 121, 'trusty': 38})
 
=== modified file 'quickstart/tests/models/test_bundles.py'
--- quickstart/tests/models/test_bundles.py	2015-05-13 08:46:33 +0000
+++ quickstart/tests/models/test_bundles.py	2015-08-12 16:25:17 +0000
@@ -44,9 +44,19 @@
             self.bundle_data, reference=self.reference)
 
     def test_attributes(self):
-        # The bundle data and the optional reference are stored as attributes.
+        # The bundle data, the optional reference and the default version are
+        # stored as attributes.
         self.assertEqual(self.bundle_data, self.bundle.data)
         self.assertEqual(self.reference, self.bundle.reference)
+        self.assertEqual(4, self.bundle.version)
+
+    def test_version_and_no_reference(self):
+        # The bundle syntax version can be provided as an argument. The bundle
+        # reference is optional.
+        bundle = bundles.Bundle(self.bundle_data, version=3)
+        self.assertEqual(self.bundle_data, bundle.data)
+        self.assertIsNone(bundle.reference)
+        self.assertEqual(3, bundle.version)
 
     def test_string(self):
         # The bundle correctly represents itself as a string.
@@ -144,6 +154,7 @@
             bundle = bundles.from_source('bundle:mediawiki/single')
         self.assertEqual(self.bundle_data, bundle.data)
         self.assertEqual('cs:bundle/mediawiki-single', bundle.reference.id())
+        self.assertEqual(4, bundle.version)
         # The charmworld id is properly set when passing charmworld URLs.
         # XXX frankban 2015-03-09: remove this check once we get rid of the
         # charmworld id concept.
@@ -184,6 +195,7 @@
             bundle = bundles.from_source('bundle:mediawiki/bundle1')
         self.assertEqual(self.bundle_data, bundle.data)
         self.assertEqual('cs:bundle/mediawiki', bundle.reference.id())
+        self.assertEqual(4, bundle.version)
         # The charmworld id is properly set when passing charmworld URLs.
         # XXX frankban 2015-03-09: remove this check once we get rid of the
         # charmworld id concept.
@@ -334,6 +346,7 @@
             bundle = bundles.from_source('django')
         self.assertEqual(self.bundle_data, bundle.data)
         self.assertEqual('cs:bundle/django', bundle.reference.id())
+        self.assertEqual(4, bundle.version)
         mock_urlread.assert_called_once_with(
             settings.CHARMSTORE_API + 'bundle/django/archive/bundle.yaml')
 
@@ -402,6 +415,7 @@
         bundle = bundles.from_source(path)
         self.assertEqual(self.bundle_data, bundle.data)
         self.assertIsNone(bundle.reference)
+        self.assertEqual(4, bundle.version)
 
     def test_local_file_legacy_bundle(self):
         # A bundle instance can be created from a local file source including
@@ -419,6 +433,7 @@
         bundle = bundles.from_source(path)
         self.assertEqual(legacy_bundle_data['bundle'], bundle.data)
         self.assertIsNone(bundle.reference)
+        self.assertEqual(3, bundle.version)
 
     def test_local_file_not_found(self):
         # An IOError is raised if a local file source cannot be found.
@@ -485,6 +500,7 @@
             bundle = bundles.from_source('https://1.2.3.4')
         self.assertEqual(self.bundle_data, bundle.data)
         self.assertIsNone(bundle.reference)
+        self.assertEqual(4, bundle.version)
         mock_urlread.assert_called_once_with('https://1.2.3.4')
 
     def test_remote_url_legacy_bundle(self):
@@ -496,6 +512,7 @@
             bundle = bundles.from_source('https://1.2.3.4:8000', 'bundle2')
         self.assertEqual(self.legacy_bundle_data['bundle2'], bundle.data)
         self.assertIsNone(bundle.reference)
+        self.assertEqual(3, bundle.version)
         mock_urlread.assert_called_once_with('https://1.2.3.4:8000')
 
     def test_remote_url_not_reachable(self):
 
=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2015-06-05 13:07:47 +0000
+++ quickstart/tests/test_app.py	2015-08-12 16:25:17 +0000
@@ -1759,10 +1759,8 @@
         env, uncommitted = mock.Mock(), False
         token = app.deploy_bundle(env, self.bundle, uncommitted, self.charm)
         self.assertIsNone(token)
-        # For the time being, the bundle version 3 is deployed by default.
-        expected_yaml = yaml.safe_dump({'bundle': self.bundle_data})
         env.deploy_bundle.assert_called_once_with(
-            expected_yaml, 3, bundle_id=None)
+            yaml.safe_dump(self.bundle_data), 4, bundle_id=None)
         self.assertFalse(env.set_changes.called)
         self.assertFalse(env.close.called)
         self.assertEqual(2, mock_print.call_count)
@@ -1775,6 +1773,38 @@
                 'use the GUI to check the bundle deployment progress'),
         ])
 
+    def test_legacy_bundle_deployment(self, mock_print):
+        # A bundle with legacy v3 bundle syntax is successfully deployed.
+        env, uncommitted = mock.Mock(), False
+        bundle = bundles.Bundle(self.bundle_data, version=3)
+        token = app.deploy_bundle(env, bundle, uncommitted, self.charm)
+        self.assertIsNone(token)
+        expected_yaml = yaml.safe_dump({'bundle': self.bundle_data})
+        env.deploy_bundle.assert_called_once_with(
+            expected_yaml, 3, bundle_id=None)
+        self.assertFalse(env.set_changes.called)
+        self.assertFalse(env.close.called)
+
+    def test_bundle_deployment_v4_not_supported(self, mock_print):
+        # The bundle deployment falls back to version 3 syntax if the Juju GUI
+        # unit is not recent enough. In this case there is no guarantee that
+        # the deployment succeeds.
+        env, uncommitted = mock.Mock(), False
+        charm = references.Reference.from_string('cs:trusty/juju-gui-36')
+        expected_logs = [
+            'new style bundle syntax is not supported by currently deployed '
+            'Juju GUI charm (cs:trusty/juju-gui-36): trying to deploy the '
+            'bundle using old syntax'
+        ]
+        with helpers.assert_logs(expected_logs, level='warn'):
+            token = app.deploy_bundle(env, self.bundle, uncommitted, charm)
+        self.assertIsNone(token)
+        expected_yaml = yaml.safe_dump({'bundle': self.bundle_data})
+        env.deploy_bundle.assert_called_once_with(
+            expected_yaml, 3, bundle_id=None)
+        self.assertFalse(env.set_changes.called)
+        self.assertFalse(env.close.called)
+
     def test_bundle_deployment_with_id(self, mock_print):
         # If the bundle reference includes the charmworld id, it is passed when
         # calling the GUI server API.
@@ -1787,7 +1817,7 @@
         bundle = bundles.Bundle(self.bundle_data, reference=ref)
         app.deploy_bundle(env, bundle, uncommitted, self.charm)
         env.deploy_bundle.assert_called_once_with(
-            self.bundle.serialize_legacy(), 3, bundle_id='django/single')
+            self.bundle.serialize(), 4, bundle_id='django/single')
 
     def test_api_error(self, mock_print):
         # A ProgramExit is raised if an error occurs while deploying the
 
=== modified file 'tox.ini'
--- tox.ini	2015-08-07 10:24:11 +0000
+++ tox.ini	2015-08-12 16:25:17 +0000
@@ -18,7 +18,7 @@
 # The envlist option must only include platform scenarios. In essence, those
 # are used to test Juju Quickstart with the specific dependencies present in
 # each supported OS platform.
-envlist = pypi,ppa,trusty,utopic,vivid
+envlist = pypi,ppa,trusty,vivid,wily
 
 # Define the base requirements and commands to use when testing the program.
 
@@ -86,16 +86,6 @@
     PyYAML==3.10
     urwid==1.1.1
 
-[testenv:utopic]
-deps =
-    {[testenv]deps}
-    # Ubuntu 14.10 (utopic) distro dependencies.
-    websocket-client==0.12.0
-    jujuclient==0.17.5
-    jujubundlelib==0.1.9
-    PyYAML==3.11
-    urwid==1.2.1
-
 [testenv:vivid]
 deps =
     {[testenv]deps}
@@ -106,6 +96,15 @@
     PyYAML==3.11
     urwid==1.2.1
 
+[testenv:wily]
+deps =
+    {[testenv]deps}
+    # Ubuntu 15.10 (wily) distro dependencies.
+    websocket-client==0.18.0
+    jujuclient==0.50.1
+    jujubundlelib==0.1.9
+    PyYAML==3.11
+    urwid==1.2.1
 
 # Define additional tox targets to be used to validate and lint the code.