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

[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.

[Gavin Panella (allenap)]

Thanks for doing this. I have several suggestions. Please consider those before landing.

review: Approve

[Blake Rouse (blake-rouse)]

Thanks for the quick review. Fixed all your points! Will backport this all the way to 1.7 so the previous backport wont affect others.

[MAAS Lander (maas-lander)]

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

Ign http://security.ubuntu.com trusty-security InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty 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://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/universe 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 [598 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 (411 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 phantomjs postgresql pyflakes python-apt python-bson python-bzrlib python-convoy python-coverage 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-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip pyt...

=== modified file 'src/maasserver/bootresources.py'
--- src/maasserver/bootresources.py	2015-07-08 20:47:12 +0000
+++ src/maasserver/bootresources.py	2015-08-03 16:36:18 +0000
@@ -86,7 +86,10 @@
 from provisioningserver.import_images.keyrings import write_all_keyrings
 from provisioningserver.import_images.product_mapping import map_products
 from provisioningserver.logger import get_maas_logger
-from provisioningserver.rpc.cluster import ListBootImages
+from provisioningserver.rpc.cluster import (
+    ListBootImages,
+    ListBootImagesV2,
+)
 from provisioningserver.utils.fs import tempdir
 from provisioningserver.utils.shell import ExternalProcessError
 from provisioningserver.utils.twisted import (
@@ -103,6 +106,7 @@
 from twisted.internet import reactor
 from twisted.internet.defer import DeferredList
 from twisted.internet.threads import deferToThread
+from twisted.protocols.amp import UnhandledCommand
 from twisted.python import log
 
 
@@ -1122,7 +1126,14 @@
         clients = getAllClients()
 
         def get_images(client):
-            return client(ListBootImages).addCallback(itemgetter("images"))
+            def fallback_v1(failure):
+                failure.trap(UnhandledCommand)
+                return client(ListBootImages)
+
+            d = client(ListBootImagesV2)
+            d.addErrback(fallback_v1)
+            d.addCallback(itemgetter("images"))
+            return d
 
         d = DeferredList(imap(get_images, clients), consumeErrors=True)
 
 
=== modified file 'src/maasserver/clusterrpc/boot_images.py'
--- src/maasserver/clusterrpc/boot_images.py	2015-07-09 09:10:51 +0000
+++ src/maasserver/clusterrpc/boot_images.py	2015-08-03 16:36:18 +0000
@@ -32,8 +32,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
 
 
@@ -91,16 +93,34 @@
         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 = lambda client: partial(client, ListBootImages)
-    responses = async.gather(imap(listimages, getAllClients()))
-    for response in suppress_failures(responses):
+    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())
 
=== modified file 'src/maasserver/clusterrpc/tests/test_boot_images.py'
--- src/maasserver/clusterrpc/tests/test_boot_images.py	2015-07-09 09:16:20 +0000
+++ src/maasserver/clusterrpc/tests/test_boot_images.py	2015-08-03 16:36:18 +0000
@@ -16,6 +16,7 @@
 
 import os
 
+from maasserver.clusterrpc import boot_images as boot_images_module
 from maasserver.clusterrpc.boot_images import (
     get_all_available_boot_images,
     get_boot_images,
@@ -30,9 +31,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,
@@ -42,12 +58,17 @@
     boot_images,
     clusterservice,
 )
+from provisioningserver.rpc.cluster import (
+    ListBootImages,
+    ListBootImagesV2,
+)
 from provisioningserver.testing.boot_images import (
     make_boot_image_storage_params,
     make_image,
 )
 from provisioningserver.testing.config import ClusterConfigurationFixture
 from twisted.internet.defer import succeed
+from twisted.protocols.amp import UnhandledCommand
 
 
 def make_image_dir(image_params, tftp_root):
@@ -171,6 +192,28 @@
             ],
             get_boot_images(nodegroup))
 
+    def test_calls_ListBootImagesV2_before_ListBootImages(self):
+        nodegroup = factory.make_NodeGroup(status=NODEGROUP_STATUS.ENABLED)
+        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.ENABLED)
+        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_common_available_boot_images` and
@@ -251,6 +294,37 @@
 
         self.assertItemsEqual(images, self.get())
 
+    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, self.get())
+
     def test_returns_empty_list_when_all_clusters_fail(self):
         factory.make_NodeGroup().accept()
         factory.make_NodeGroup().accept()
 
=== modified file 'src/maasserver/tests/test_bootresources.py'
--- src/maasserver/tests/test_bootresources.py	2015-07-10 10:55:53 +0000
+++ src/maasserver/tests/test_bootresources.py	2015-08-03 16:36:18 +0000
@@ -85,7 +85,10 @@
 )
 from provisioningserver.auth import get_maas_user_gpghome
 from provisioningserver.import_images.product_mapping import ProductMapping
-from provisioningserver.rpc.cluster import ListBootImages
+from provisioningserver.rpc.cluster import (
+    ListBootImages,
+    ListBootImagesV2,
+)
 from provisioningserver.utils.text import normalise_whitespace
 from provisioningserver.utils.twisted import asynchronous
 from testtools.deferredruntest import extract_result
@@ -100,6 +103,7 @@
     fail,
     succeed,
 )
+from twisted.protocols.amp import UnhandledCommand
 
 
 def make_boot_resource_file_with_stream():
@@ -1460,28 +1464,66 @@
         factory.make_BootResource()
         self.assertTrue(service.are_boot_images_available_in_the_region())
 
-    def test__are_boot_images_available_in_any_cluster(self):
-        # Import the websocket handlers now: merely defining DeviceHandler,
-        # e.g., causes a database access, which will crash if it happens
-        # inside the reactor thread where database access is forbidden and
-        # prevented. My own opinion is that a class definition should not
-        # cause a database access and we ought to fix that.
-        import maasserver.websockets.handlers  # noqa
-
-        cluster = factory.make_NodeGroup()
-        service = bootresources.ImportResourcesProgressService()
-
-        self.useFixture(RegionEventLoopFixture("rpc"))
-        self.useFixture(RunningEventLoopFixture())
-        region_rpc = MockLiveRegionToClusterRPCFixture()
-        self.useFixture(region_rpc)
-
-        # are_boot_images_available_in_the_region() returns False when there
-        # are no clusters connected.
-        self.assertFalse(service.are_boot_images_available_in_any_cluster())
-
-        # Connect a cluster to the region via RPC.
-        cluster_rpc = region_rpc.makeCluster(cluster, ListBootImages)
+    def test__are_boot_images_available_in_any_cluster_v2(self):
+        # Import the websocket handlers now: merely defining DeviceHandler,
+        # e.g., causes a database access, which will crash if it happens
+        # inside the reactor thread where database access is forbidden and
+        # prevented. My own opinion is that a class definition should not
+        # cause a database access and we ought to fix that.
+        import maasserver.websockets.handlers  # noqa
+
+        cluster = factory.make_NodeGroup()
+        service = bootresources.ImportResourcesProgressService()
+
+        self.useFixture(RegionEventLoopFixture("rpc"))
+        self.useFixture(RunningEventLoopFixture())
+        region_rpc = MockLiveRegionToClusterRPCFixture()
+        self.useFixture(region_rpc)
+
+        # are_boot_images_available_in_the_region() returns False when there
+        # are no clusters connected.
+        self.assertFalse(service.are_boot_images_available_in_any_cluster())
+
+        # Connect a cluster to the region via RPC.
+        cluster_rpc = region_rpc.makeCluster(cluster, ListBootImagesV2)
+
+        # are_boot_images_available_in_the_region() returns False when none of
+        # the clusters have any images.
+        cluster_rpc.ListBootImagesV2.return_value = succeed({"images": []})
+        self.assertFalse(service.are_boot_images_available_in_any_cluster())
+
+        # are_boot_images_available_in_the_region() returns True when a
+        # cluster has an imported boot image.
+        response = {"images": [make_rpc_boot_image()]}
+        cluster_rpc.ListBootImagesV2.return_value = succeed(response)
+        self.assertTrue(service.are_boot_images_available_in_any_cluster())
+
+    def test__are_boot_images_available_in_any_cluster_v1(self):
+        # Import the websocket handlers now: merely defining DeviceHandler,
+        # e.g., causes a database access, which will crash if it happens
+        # inside the reactor thread where database access is forbidden and
+        # prevented. My own opinion is that a class definition should not
+        # cause a database access and we ought to fix that.
+        import maasserver.websockets.handlers  # noqa
+
+        cluster = factory.make_NodeGroup()
+        service = bootresources.ImportResourcesProgressService()
+
+        self.useFixture(RegionEventLoopFixture("rpc"))
+        self.useFixture(RunningEventLoopFixture())
+        region_rpc = MockLiveRegionToClusterRPCFixture()
+        self.useFixture(region_rpc)
+
+        # are_boot_images_available_in_the_region() returns False when there
+        # are no clusters connected.
+        self.assertFalse(service.are_boot_images_available_in_any_cluster())
+
+        # Connect a cluster to the region via RPC.
+        cluster_rpc = region_rpc.makeCluster(
+            cluster, ListBootImagesV2, ListBootImages)
+
+        # All calls to ListBootImagesV2 raises a UnhandledCommand.
+        cluster_rpc.ListBootImagesV2.side_effect = UnhandledCommand
 
         # are_boot_images_available_in_the_region() returns False when none of
         # the clusters have any images.
 
=== modified file 'src/provisioningserver/rpc/cluster.py'
--- src/provisioningserver/rpc/cluster.py	2015-07-31 20:07:44 +0000
+++ src/provisioningserver/rpc/cluster.py	2015-08-03 16:36:18 +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	2015-06-25 22:33:37 +0000
+++ src/provisioningserver/rpc/clusterservice.py	2015-08-03 16:36:18 +0000
@@ -151,6 +151,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-06-26 15:36:26 +0000
+++ src/provisioningserver/rpc/tests/test_clusterservice.py	2015-08-03 16:36:18 +0000
@@ -215,14 +215,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
@@ -232,7 +237,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)
 
@@ -282,7 +287,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	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/rpc/tests/test_docs.py	2015-08-03 16:36:18 +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))