Merge lp:~blake-rouse/maas/boot-images-rpc-v2-1.7 into lp:maas/1.7

[Blake Rouse]

Add ListBootImagesV2 RPC command. Fallback to using ListBootImages RPC when the ListBootImagesV2 is not handled on the cluster.

The signature of the ListBootImages RPC call was changed to support more data in the response. This change will cause connected clusters that are not updated the same time as the region to stop communicating and error. This allows both methods to exist on the cluster where V2 will be called first then fallback to the older call in error.

Backport: https://code.launchpad.net/~blake-rouse/maas/boot-images-rpc-v2/+merge/266728

[Blake Rouse (blake-rouse)]

Self-approving backport.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~blake-rouse/maas/boot-images-rpc-v2-1.7 into lp:maas/1.7 failed. Below is the output from the failed tests.

Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://security.ubuntu.com trusty-security Release
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [63.5 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [227 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [130 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [599 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [301 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,322 kB in 3s (413 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-amqplib python-bson python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mimeparse python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-seamicroclient python-simplejson py...

=== modified file 'src/maasserver/clusterrpc/boot_images.py'
--- src/maasserver/clusterrpc/boot_images.py	2014-09-30 18:13:21 +0000
+++ src/maasserver/clusterrpc/boot_images.py	2015-08-03 19:11:39 +0000
@@ -21,6 +21,7 @@
 ]
 
 from functools import partial
+from itertools import imap
 
 from maasserver.rpc import (
     getAllClients,
@@ -30,8 +31,10 @@
 from provisioningserver.rpc.cluster import (
     IsImportBootImagesRunning,
     ListBootImages,
+    ListBootImagesV2,
     )
 from provisioningserver.utils.twisted import synchronous
+from twisted.protocols.amp import UnhandledCommand
 from twisted.python.failure import Failure
 
 
@@ -89,46 +92,51 @@
         30 seconds.
     """
     client = getClientFor(nodegroup.uuid, timeout=1)
-    call = client(ListBootImages)
-    return call.wait(30).get("images")
+    try:
+        call = client(ListBootImagesV2)
+        return call.wait(30).get("images")
+    except UnhandledCommand:
+        call = client(ListBootImages)
+        return call.wait(30).get("images")
+
+
+@synchronous
+def _get_available_boot_images():
+    """Obtain boot images available on connected clusters."""
+    listimages_v1 = lambda client: partial(client, ListBootImages)
+    listimages_v2 = lambda client: partial(client, ListBootImagesV2)
+    clients_v2 = getAllClients()
+    responses_v2 = async.gather(imap(listimages_v2, clients_v2))
+    clients_v1 = []
+    for i, response in enumerate(responses_v2):
+        if (isinstance(response, Failure) and
+                response.check(UnhandledCommand) is not None):
+            clients_v1.append(clients_v2[i])
+        elif not isinstance(response, Failure):
+            # Convert each image to a frozenset of its items.
+            yield frozenset(
+                frozenset(image.viewitems())
+                for image in response["images"]
+            )
+    responses_v1 = async.gather(imap(listimages_v1, clients_v1))
+    for response in suppress_failures(responses_v1):
+        # Convert each image to a frozenset of its items.
+        yield frozenset(
+            frozenset(image.viewitems())
+            for image in response["images"]
+        )
 
 
 @synchronous
 def get_available_boot_images():
     """Obtain boot images that are available on all clusters."""
-    responses = async.gather(
-        partial(client, ListBootImages)
-        for client in getAllClients())
-    responses = [
-        response["images"]
-        for response in suppress_failures(responses)
-        ]
-    if len(responses) == 0:
-        return []
-
-    # Create the initial set of images from the first response. This will be
-    # used to perform the intersection of all the other responses.
-    images = responses.pop()
-    images = {
-        frozenset(image.items())
-        for image in images
-        }
-
-    # Intersect all of the remaining responses to get only the images that
-    # exist on all clusters.
-    for response in responses:
-        response_images = {
-            frozenset(image.items())
-            for image in response
-            }
-        images = images & response_images
-
-    # Return only boot images on all cluster, in the same format as
-    # get_boot_images.
-    return [
-        dict(image)
-        for image in list(images)
-        ]
+    image_sets = list(_get_available_boot_images())
+    if len(image_sets) > 0:
+        images = frozenset.intersection(*image_sets)
+    else:
+        images = frozenset()
+    # Return using the same format as get_boot_images.
+    return list(dict(image) for image in images)
 
 
 @synchronous

=== modified file 'src/maasserver/clusterrpc/tests/test_boot_images.py'
--- src/maasserver/clusterrpc/tests/test_boot_images.py	2014-12-16 19:29:45 +0000
+++ src/maasserver/clusterrpc/tests/test_boot_images.py	2015-08-03 19:11:39 +0000
@@ -16,6 +16,7 @@
 
 import os
 
+from maasserver.clusterrpc import boot_images as boot_images_module
 from maasserver.clusterrpc.boot_images import (
     get_available_boot_images,
     get_boot_images,
@@ -29,9 +30,24 @@
     NODEGROUP_STATUS,
     )
 from maasserver.rpc import getAllClients
-from maasserver.rpc.testing.fixtures import RunningClusterRPCFixture
+from maasserver.rpc.testing.fixtures import (
+    MockLiveRegionToClusterRPCFixture,
+    RunningClusterRPCFixture,
+    )
+from maasserver.testing.eventloop import (
+    RegionEventLoopFixture,
+    RunningEventLoopFixture,
+    )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockCallsMatch,
+    )
+from mock import (
+    call,
+    MagicMock,
+    )
 from provisioningserver.boot.tests import test_tftppath
 from provisioningserver.boot.tftppath import (
     compose_image_path,
@@ -41,11 +57,16 @@
     boot_images,
     clusterservice,
     )
+from provisioningserver.rpc.cluster import (
+    ListBootImages,
+    ListBootImagesV2,
+    )
 from provisioningserver.testing.boot_images import (
     make_boot_image_storage_params,
     make_image,
     )
 from twisted.internet.defer import succeed
+from twisted.protocols.amp import UnhandledCommand
 
 
 def make_image_dir(image_params, tftproot):
@@ -164,6 +185,28 @@
             ],
             get_boot_images(nodegroup))
 
+    def test_calls_ListBootImagesV2_before_ListBootImages(self):
+        nodegroup = factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
+        mock_client = MagicMock()
+        self.patch_autospec(
+            boot_images_module, "getClientFor").return_value = mock_client
+        get_boot_images(nodegroup)
+        self.assertThat(mock_client, MockCalledOnceWith(ListBootImagesV2))
+
+    def test_calls_ListBootImages_if_raised_UnhandledCommand(self):
+        nodegroup = factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
+        mock_client = MagicMock()
+        self.patch_autospec(
+            boot_images_module, "getClientFor").return_value = mock_client
+        mock_client.return_value.wait.side_effect = [
+            UnhandledCommand(),
+            {"images": []},
+            ]
+        get_boot_images(nodegroup)
+        self.assertThat(mock_client, MockCallsMatch(
+            call(ListBootImagesV2),
+            call(ListBootImages)))
+
 
 class TestGetAvailableBootImages(MAASServerTestCase):
     """Tests for `get_available_boot_images`."""
@@ -239,6 +282,37 @@
             images,
             get_available_boot_images())
 
+    def test_fallback_to_ListBootImages_on_old_clusters(self):
+        nodegroup_1 = factory.make_NodeGroup()
+        nodegroup_1.accept()
+        nodegroup_2 = factory.make_NodeGroup()
+        nodegroup_2.accept()
+        nodegroup_3 = factory.make_NodeGroup()
+        nodegroup_3.accept()
+
+        images = [make_rpc_boot_image() for _ in range(3)]
+
+        # Limit the region's event loop to only the "rpc" service.
+        self.useFixture(RegionEventLoopFixture("rpc"))
+        # Now start the region's event loop.
+        self.useFixture(RunningEventLoopFixture())
+        # This fixture allows us to simulate mock clusters.
+        rpc = self.useFixture(MockLiveRegionToClusterRPCFixture())
+
+        # This simulates an older cluster, one without ListBootImagesV2.
+        cluster_1 = rpc.makeCluster(nodegroup_1, ListBootImages)
+        cluster_1.ListBootImages.return_value = succeed({'images': images})
+
+        # This simulates a newer cluster, one with ListBootImagesV2.
+        cluster_2 = rpc.makeCluster(nodegroup_2, ListBootImagesV2)
+        cluster_2.ListBootImagesV2.return_value = succeed({'images': images})
+
+        # This simulates a broken cluster.
+        cluster_3 = rpc.makeCluster(nodegroup_3, ListBootImagesV2)
+        cluster_3.ListBootImagesV2.side_effect = ZeroDivisionError
+
+        self.assertItemsEqual(images, get_available_boot_images())
+
     def test_returns_empty_list_when_all_clusters_fail(self):
         factory.make_NodeGroup().accept()
         factory.make_NodeGroup().accept()

=== added file 'src/maasserver/utils/version.py'
--- src/maasserver/utils/version.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/utils/version.py	2015-08-03 19:11:39 +0000
@@ -0,0 +1,131 @@
+# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Version utilities."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    "get_maas_doc_version",
+    "get_maas_version_subversion",
+    "get_maas_version_ui",
+    ]
+
+import apt_pkg
+
+
+try:
+    from bzrlib.branch import Branch
+    from bzrlib.errors import NotBranchError
+except ImportError:
+    Branch = None
+
+# Initialize apt_pkg.
+apt_pkg.init()
+
+# Name of maas package to get version from.
+REGION_PACKAGE_NAME = "maas-region-controller-min"
+
+
+def get_version_from_apt(package):
+    """Return the version output from `apt_pkg.Cache` for the given package."""
+    cache = apt_pkg.Cache(None)
+    version = None
+    if package in cache:
+        apt_package = cache[package]
+        version = apt_package.current_ver
+
+    if version is not None:
+        return version.ver_str
+    else:
+        return ""
+
+
+def extract_version_subversion(version):
+    """Return a tuple (version, subversion) from the given apt version."""
+    if "~" in version:
+        main_version, extra = version.split("~", 1)
+        return main_version, extra.split("-", 1)[0]
+    elif "+" in version:
+        main_version, extra = version.split("+", 1)
+        return main_version, "+" + extra.split("-", 1)[0]
+    else:
+        return version.split("-", 1)[0], ''
+
+
+def get_maas_branch():
+    """Return the `bzrlib.branch.Branch` for this running MAAS."""
+    if Branch is None:
+        return None
+    try:
+        return Branch.open(".")
+    except NotBranchError:
+        return None
+
+
+_cache = {}
+
+
+# A very simply memoize function: when we switch to Django 1.7 we should use
+# Django's lru_cache method.
+def simple_cache(fun):
+    def wrapped(*args, **kwargs):
+        key = hash(repr(fun) + repr(args) + repr(kwargs))
+        if key not in _cache:
+            _cache[key] = fun(*args, **kwargs)
+        return _cache[key]
+
+    wrapped.__doc__ = "%s %s" % (fun.__doc__, "(cached)")
+    return wrapped
+
+
+@simple_cache
+def get_maas_package_version():
+    """Return the apt version for the main MAAS package."""
+    return get_version_from_apt(REGION_PACKAGE_NAME)
+
+
+@simple_cache
+def get_maas_version_subversion():
+    """Return a tuple with the MAAS version and the MAAS subversion."""
+    apt_version = get_maas_package_version()
+    if apt_version:
+        return extract_version_subversion(apt_version)
+    else:
+        # Get the branch information
+        branch = get_maas_branch()
+        if branch is None:
+            # Not installed not in branch, then no way to identify. This should
+            # not happen, but just in case.
+            return "unknown", ''
+        else:
+            return "from source (+bzr%s)" % branch.revno(), ''
+
+
+@simple_cache
+def get_maas_version_ui():
+    """Return the version string for the running MAAS region.
+
+    The returned string is suitable to display in the UI.
+    """
+    version, subversion = get_maas_version_subversion()
+    return "%s (%s)" % (version, subversion) if subversion else version
+
+
+@simple_cache
+def get_maas_doc_version():
+    """Return the doc version for the running MAAS region."""
+    doc_prefix = 'docs'
+    apt_version = get_maas_package_version()
+    if apt_version:
+        version, _ = extract_version_subversion(apt_version)
+        return doc_prefix + '.'.join(version.split('.')[:2])
+    else:
+        return doc_prefix

=== removed file 'src/maasserver/utils/version.py'
--- src/maasserver/utils/version.py	2015-04-08 15:32:33 +0000
+++ src/maasserver/utils/version.py	1970-01-01 00:00:00 +0000
@@ -1,131 +0,0 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Version utilities."""
-
-from __future__ import (
-    absolute_import,
-    print_function,
-    unicode_literals,
-    )
-
-str = None
-
-__metaclass__ = type
-__all__ = [
-    "get_maas_doc_version",
-    "get_maas_version_subversion",
-    "get_maas_version_ui",
-    ]
-
-import apt_pkg
-
-
-try:
-    from bzrlib.branch import Branch
-    from bzrlib.errors import NotBranchError
-except ImportError:
-    Branch = None
-
-# Initialize apt_pkg.
-apt_pkg.init()
-
-# Name of maas package to get version from.
-REGION_PACKAGE_NAME = "maas-region-controller-min"
-
-
-def get_version_from_apt(package):
-    """Return the version output from `apt_pkg.Cache` for the given package."""
-    cache = apt_pkg.Cache(None)
-    version = None
-    if package in cache:
-        apt_package = cache[package]
-        version = apt_package.current_ver
-
-    if version is not None:
-        return version.ver_str
-    else:
-        return ""
-
-
-def extract_version_subversion(version):
-    """Return a tuple (version, subversion) from the given apt version."""
-    if "~" in version:
-        main_version, extra = version.split("~", 1)
-        return main_version, extra.split("-", 1)[0]
-    elif "+" in version:
-        main_version, extra = version.split("+", 1)
-        return main_version, "+" + extra.split("-", 1)[0]
-    else:
-        return version.split("-", 1)[0], ''
-
-
-def get_maas_branch():
-    """Return the `bzrlib.branch.Branch` for this running MAAS."""
-    if Branch is None:
-        return None
-    try:
-        return Branch.open(".")
-    except NotBranchError:
-        return None
-
-
-_cache = {}
-
-
-# A very simply memoize function: when we switch to Django 1.7 we should use
-# Django's lru_cache method.
-def simple_cache(fun):
-    def wrapped(*args, **kwargs):
-        key = hash(repr(fun) + repr(args) + repr(kwargs))
-        if not key in _cache:
-            _cache[key] = fun(*args, **kwargs)
-        return _cache[key]
-
-    wrapped.__doc__ = "%s %s" % (fun.__doc__, "(cached)")
-    return wrapped
-
-
-@simple_cache
-def get_maas_package_version():
-    """Return the apt version for the main MAAS package."""
-    return get_version_from_apt(REGION_PACKAGE_NAME)
-
-
-@simple_cache
-def get_maas_version_subversion():
-    """Return a tuple with the MAAS version and the MAAS subversion."""
-    apt_version = get_maas_package_version()
-    if apt_version:
-        return extract_version_subversion(apt_version)
-    else:
-        # Get the branch information
-        branch = get_maas_branch()
-        if branch is None:
-            # Not installed not in branch, then no way to identify. This should
-            # not happen, but just in case.
-            return "unknown", ''
-        else:
-            return "from source (+bzr%s)" % branch.revno(), ''
-
-
-@simple_cache
-def get_maas_version_ui():
-    """Return the version string for the running MAAS region.
-
-    The returned string is suitable to display in the UI.
-    """
-    version, subversion = get_maas_version_subversion()
-    return "%s (%s)" % (version, subversion) if subversion else version
-
-
-@simple_cache
-def get_maas_doc_version():
-    """Return the doc version for the running MAAS region."""
-    doc_prefix = 'docs'
-    apt_version = get_maas_package_version()
-    if apt_version:
-        version, _ = extract_version_subversion(apt_version)
-        return doc_prefix + '.'.join(version.split('.')[:2])
-    else:
-        return doc_prefix

=== modified file 'src/provisioningserver/rpc/cluster.py'
--- src/provisioningserver/rpc/cluster.py	2015-07-31 20:46:29 +0000
+++ src/provisioningserver/rpc/cluster.py	2015-08-03 19:11:39 +0000
@@ -59,6 +59,30 @@
 
     arguments = []
     response = [
+        (b"images", amp.AmpList(
+            [(b"osystem", amp.Unicode()),
+             (b"architecture", amp.Unicode()),
+             (b"subarchitecture", amp.Unicode()),
+             (b"release", amp.Unicode()),
+             (b"label", amp.Unicode()),
+             (b"purpose", amp.Unicode()),
+             (b"xinstall_type", amp.Unicode()),
+             (b"xinstall_path", amp.Unicode())]))
+    ]
+    errors = []
+
+
+class ListBootImagesV2(amp.Command):
+    """List the boot images available on this cluster controller.
+
+    This command compresses the images list to allow more images in the
+    response and to remove the amp.TooLong error.
+
+    :since: 1.7.6
+    """
+
+    arguments = []
+    response = [
         (b"images", CompressedAmpList(
             [(b"osystem", amp.Unicode()),
              (b"architecture", amp.Unicode()),

=== modified file 'src/provisioningserver/rpc/clusterservice.py'
--- src/provisioningserver/rpc/clusterservice.py	2014-12-18 13:53:48 +0000
+++ src/provisioningserver/rpc/clusterservice.py	2015-08-03 19:11:39 +0000
@@ -149,6 +149,15 @@
         """
         return {"images": list_boot_images()}
 
+    @cluster.ListBootImagesV2.responder
+    def list_boot_images_v2(self):
+        """list_boot_images_v2()
+
+        Implementation of
+        :py:class:`~provisioningserver.rpc.cluster.ListBootImagesV2`.
+        """
+        return {"images": list_boot_images()}
+
     @cluster.ImportBootImages.responder
     def import_boot_images(self, sources, http_proxy=None, https_proxy=None):
         """import_boot_images()

=== modified file 'src/provisioningserver/rpc/tests/test_clusterservice.py'
--- src/provisioningserver/rpc/tests/test_clusterservice.py	2015-03-02 16:12:40 +0000
+++ src/provisioningserver/rpc/tests/test_clusterservice.py	2015-08-03 19:11:39 +0000
@@ -217,14 +217,19 @@
         return d.addCallback(check)
 
 
-class TestClusterProtocol_ListBootImages(MAASTestCase):
+class TestClusterProtocol_ListBootImages_and_ListBootImagesV2(MAASTestCase):
 
     run_tests_with = MAASTwistedRunTest.make_factory(timeout=5)
 
+    scenarios = (
+        ("ListBootImages", {"rpc_call": cluster.ListBootImages}),
+        ("ListBootImagesV2", {"rpc_call": cluster.ListBootImagesV2}),
+        )
+
     def test_list_boot_images_is_registered(self):
         protocol = Cluster()
         responder = protocol.locateResponder(
-            cluster.ListBootImages.commandName)
+            self.rpc_call.commandName)
         self.assertIsNotNone(responder)
 
     @inlineCallbacks
@@ -233,7 +238,7 @@
         list_boot_images = self.patch(tftppath, "list_boot_images")
         list_boot_images.return_value = []
 
-        response = yield call_responder(Cluster(), cluster.ListBootImages, {})
+        response = yield call_responder(Cluster(), self.rpc_call, {})
 
         self.assertEqual({"images": []}, response)
 
@@ -281,7 +286,7 @@
                 expected_image['xinstall_path'] = ''
                 expected_image['xinstall_type'] = ''
 
-        response = yield call_responder(Cluster(), cluster.ListBootImages, {})
+        response = yield call_responder(Cluster(), self.rpc_call, {})
 
         self.assertThat(response, KeysEqual("images"))
         self.assertItemsEqual(expected_images, response["images"])

=== modified file 'src/provisioningserver/rpc/tests/test_docs.py'
--- src/provisioningserver/rpc/tests/test_docs.py	2014-06-25 11:26:56 +0000
+++ src/provisioningserver/rpc/tests/test_docs.py	2015-08-03 19:11:39 +0000
@@ -70,6 +70,7 @@
             self.since_clause_missing_message, Contains(":since:"))
         since_clause_contains_version = Annotate(
             self.since_clause_version_not_recognised, MatchesRegex(
-                ".*^:since: *[1-9][.][0-9]+$", re.DOTALL | re.MULTILINE))
+                ".*^:since: *[1-9][.][0-9]+([.][0-9]+)?$",
+                re.DOTALL | re.MULTILINE))
         self.assertThat(getdoc(self.command), MatchesAll(
             contains_since_clause, since_clause_contains_version))