Merge ~ltrager/maas:remove_list_boot_images_v1 into maas:master

[Lee Trager]

Remove ListBootImagesV1 RPC call.

ListBootImagesV2 was added in MAAS 1.7. Every supported version of MAAS
has the V2 call. No unsupported version of MAAS that old could connect
to a recent version of MAAS.

[Björn Tillenius (bjornt)]

+1

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b remove_list_boot_images_v1 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: b63b9541f12540a117b788748e20f00ca3e10c30

review: Approve

diff --git a/src/maasserver/bootresources.py b/src/maasserver/bootresources.py
index 2d6aded..382fa10 100644
--- a/src/maasserver/bootresources.py
+++ b/src/maasserver/bootresources.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Boot Resources."""
@@ -32,7 +32,6 @@ from simplestreams.objectstores import ObjectStore
 from twisted.application.internet import TimerService
 from twisted.internet import reactor
 from twisted.internet.defer import Deferred, DeferredList, inlineCallbacks
-from twisted.protocols.amp import UnhandledCommand
 from twisted.python.failure import Failure
 
 from maasserver import locks
@@ -94,7 +93,7 @@ from provisioningserver.import_images.helpers import (
 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, LegacyLogger
-from provisioningserver.rpc.cluster import ListBootImages, ListBootImagesV2
+from provisioningserver.rpc.cluster import ListBootImages
 from provisioningserver.upgrade_cluster import create_gnupg_home
 from provisioningserver.utils.fs import tempdir
 from provisioningserver.utils.shell import ExternalProcessError
@@ -1612,12 +1611,7 @@ class ImportResourcesProgressService(TimerService):
         clients = getAllClients()
 
         def get_images(client):
-            def fallback_v1(failure):
-                failure.trap(UnhandledCommand)
-                return client(ListBootImages)
-
-            d = client(ListBootImagesV2)
-            d.addErrback(fallback_v1)
+            d = client(ListBootImages)
             d.addCallback(itemgetter("images"))
             return d
 
diff --git a/src/maasserver/clusterrpc/boot_images.py b/src/maasserver/clusterrpc/boot_images.py
index 68fe1db..17573e9 100644
--- a/src/maasserver/clusterrpc/boot_images.py
+++ b/src/maasserver/clusterrpc/boot_images.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Obtain list of boot images from rack controllers."""
@@ -18,7 +18,6 @@ from urllib.parse import ParseResult, urlparse
 
 from twisted.internet import reactor
 from twisted.internet.defer import DeferredList, DeferredSemaphore
-from twisted.protocols.amp import UnhandledCommand
 from twisted.python.failure import Failure
 
 from maasserver.models import BootResource, RackController
@@ -31,7 +30,6 @@ from provisioningserver.rpc.cluster import (
     ImportBootImages,
     IsImportBootImagesRunning,
     ListBootImages,
-    ListBootImagesV2,
 )
 from provisioningserver.rpc.exceptions import NoConnectionsAvailable
 from provisioningserver.utils import flatten
@@ -79,44 +77,25 @@ def get_boot_images(rack_controller):
         30 seconds.
     """
     client = getClientFor(rack_controller.system_id, timeout=1)
-    try:
-        call = client(ListBootImagesV2)
-        return call.wait(30).get("images")
-    except UnhandledCommand:
-        call = client(ListBootImages)
-        return call.wait(30).get("images")
+    call = client(ListBootImages)
+    return call.wait(30).get("images")
 
 
 @synchronous
 def _get_available_boot_images():
     """Obtain boot images available on connected rack controllers."""
 
-    def listimages_v1(client):
+    def listimages(client):
         return partial(client, ListBootImages)
 
-    def listimages_v2(client):
-        return partial(client, ListBootImagesV2)
-
-    clients_v2 = getAllClients()
-    responses_v2 = gather(map(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):
+    clients = getAllClients()
+    responses = gather(map(listimages, clients))
+    for i, response in enumerate(responses):
+        if not isinstance(response, Failure):
             # Convert each image to a frozenset of its items.
             yield frozenset(
                 frozenset(image.items()) for image in response["images"]
             )
-    responses_v1 = gather(map(listimages_v1, clients_v1))
-    for response in suppress_failures(responses_v1):
-        # Convert each image to a frozenset of its items.
-        yield frozenset(
-            frozenset(image.items()) for image in response["images"]
-        )
 
 
 @synchronous
diff --git a/src/maasserver/clusterrpc/tests/test_boot_images.py b/src/maasserver/clusterrpc/tests/test_boot_images.py
index 440c42c..d511c1e 100644
--- a/src/maasserver/clusterrpc/tests/test_boot_images.py
+++ b/src/maasserver/clusterrpc/tests/test_boot_images.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the `boot_images` module."""
@@ -6,7 +6,7 @@
 
 import os
 import random
-from unittest.mock import ANY, call, MagicMock, sentinel
+from unittest.mock import ANY, MagicMock, sentinel
 from urllib.parse import urlparse
 
 from testtools.matchers import (
@@ -19,7 +19,6 @@ from testtools.matchers import (
 )
 from twisted.internet.defer import DeferredLock, fail, maybeDeferred, succeed
 from twisted.internet.task import Clock
-from twisted.protocols.amp import UnhandledCommand
 from twisted.python.failure import Failure
 
 from maasserver.bootresources import get_simplestream_endpoint
@@ -50,20 +49,12 @@ from maasserver.testing.testcase import (
     MAASServerTestCase,
     MAASTransactionServerTestCase,
 )
-from maastesting.matchers import (
-    MockCalledOnceWith,
-    MockCallsMatch,
-    MockNotCalled,
-)
+from maastesting.matchers import MockCalledOnceWith, MockNotCalled
 from maastesting.twisted import TwistedLoggerFixture
 from provisioningserver.boot.tests import test_tftppath
 from provisioningserver.boot.tftppath import compose_image_path
 from provisioningserver.rpc import boot_images
-from provisioningserver.rpc.cluster import (
-    ImportBootImages,
-    ListBootImages,
-    ListBootImagesV2,
-)
+from provisioningserver.rpc.cluster import ImportBootImages, ListBootImages
 from provisioningserver.rpc.exceptions import NoConnectionsAvailable
 from provisioningserver.testing.boot_images import (
     make_boot_image_storage_params,
@@ -158,30 +149,14 @@ class TestGetBootImages(MAASServerTestCase):
         super().setUp()
         prepare_tftp_root(self)  # Sets self.tftp_root.
 
-    def test_calls_ListBootImagesV2_before_ListBootImages(self):
-        rack_controller = factory.make_RackController()
-        mock_client = MagicMock()
-        self.patch_autospec(
-            boot_images_module, "getClientFor"
-        ).return_value = mock_client
-        get_boot_images(rack_controller)
-        self.assertThat(mock_client, MockCalledOnceWith(ListBootImagesV2))
-
-    def test_calls_ListBootImages_if_raised_UnhandledCommand(self):
+    def test_calls_ListBootImages(self):
         rack_controller = factory.make_RackController()
         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(rack_controller)
-        self.assertThat(
-            mock_client,
-            MockCallsMatch(call(ListBootImagesV2), call(ListBootImages)),
-        )
+        self.assertThat(mock_client, MockCalledOnceWith(ListBootImages))
 
 
 class TestGetBootImagesTxn(MAASTransactionServerTestCase):
@@ -290,34 +265,6 @@ class TestGetAvailableBootImages(MAASTransactionServerTestCase):
 
         self.assertItemsEqual(images, self.get())
 
-    def test_fallback_to_ListBootImages_on_old_clusters(self):
-        rack_1 = factory.make_RackController()
-        rack_2 = factory.make_RackController()
-        rack_3 = factory.make_RackController()
-
-        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.
-        rack_1 = rpc.makeCluster(rack_1, ListBootImages)
-        rack_1.ListBootImages.return_value = succeed({"images": images})
-
-        # This simulates a newer cluster, one with ListBootImagesV2.
-        rack_2 = rpc.makeCluster(rack_2, ListBootImagesV2)
-        rack_2.ListBootImagesV2.return_value = succeed({"images": images})
-
-        # This simulates a broken cluster.
-        rack_3 = rpc.makeCluster(rack_3, ListBootImagesV2)
-        rack_3.ListBootImagesV2.side_effect = ZeroDivisionError
-
-        self.assertItemsEqual(images, self.get())
-
     def test_returns_empty_list_when_all_clusters_fail(self):
         factory.make_RackController()
         factory.make_RackController()
diff --git a/src/maasserver/tests/test_bootresources.py b/src/maasserver/tests/test_bootresources.py
index 609d784..6aa9711 100644
--- a/src/maasserver/tests/test_bootresources.py
+++ b/src/maasserver/tests/test_bootresources.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test maasserver.bootresources."""
@@ -28,7 +28,6 @@ from fixtures import FakeLogger, Fixture
 from testtools.matchers import Contains, ContainsAll, Equals, HasLength, Not
 from twisted.application.internet import TimerService
 from twisted.internet.defer import Deferred, fail, inlineCallbacks, succeed
-from twisted.protocols.amp import UnhandledCommand
 
 from maasserver import __version__, bootresources
 from maasserver.bootresources import (
@@ -93,7 +92,7 @@ from maastesting.testcase import MAASTestCase
 from maastesting.twisted import extract_result, TwistedLoggerFixture
 from provisioningserver.auth import get_maas_user_gpghome
 from provisioningserver.import_images.product_mapping import ProductMapping
-from provisioningserver.rpc.cluster import ListBootImages, ListBootImagesV2
+from provisioningserver.rpc.cluster import ListBootImages
 from provisioningserver.utils.text import normalise_whitespace
 from provisioningserver.utils.twisted import asynchronous, DeferredValue
 
@@ -2255,7 +2254,7 @@ class TestImportResourcesProgressServiceAsync(MAASTransactionServerTestCase):
         factory.make_BootResource()
         self.assertTrue(service.are_boot_images_available_in_the_region())
 
-    def test_are_boot_images_available_in_any_rack_v2(self):
+    def test_are_boot_images_available_in_any_rack(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
@@ -2276,46 +2275,7 @@ class TestImportResourcesProgressServiceAsync(MAASTransactionServerTestCase):
         self.assertFalse(service.are_boot_images_available_in_any_rack())
 
         # Connect a rack controller to the region via RPC.
-        cluster_rpc = region_rpc.makeCluster(rack_controller, 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_rack())
-
-        # 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_rack())
-
-    def test_are_boot_images_available_in_any_rack_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
-
-        rack_controller = factory.make_RackController()
-        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_rack())
-
-        # Connect a rack controller to the region via RPC.
-        cluster_rpc = region_rpc.makeCluster(
-            rack_controller, ListBootImagesV2, ListBootImages
-        )
-
-        # All calls to ListBootImagesV2 raises a UnhandledCommand.
-        cluster_rpc.ListBootImagesV2.side_effect = UnhandledCommand
+        cluster_rpc = region_rpc.makeCluster(rack_controller, ListBootImages)
 
         # are_boot_images_available_in_the_region() returns False when none of
         # the clusters have any images.
diff --git a/src/provisioningserver/rpc/cluster.py b/src/provisioningserver/rpc/cluster.py
index 750ab1e..e6aa0eb 100644
--- a/src/provisioningserver/rpc/cluster.py
+++ b/src/provisioningserver/rpc/cluster.py
@@ -53,33 +53,6 @@ from provisioningserver.rpc.common import Authenticate, Identify
 class ListBootImages(amp.Command):
     """List the boot images available on this rack controller.
 
-    :since: 1.5
-    """
-
-    arguments = []
-    response = [
-        (
-            b"images",
-            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 rack controller.
-
     This command compresses the images list to allow more images in the
     response and to remove the amp.TooLong error.
 
diff --git a/src/provisioningserver/rpc/clusterservice.py b/src/provisioningserver/rpc/clusterservice.py
index 0d5448b..460330a 100644
--- a/src/provisioningserver/rpc/clusterservice.py
+++ b/src/provisioningserver/rpc/clusterservice.py
@@ -295,15 +295,6 @@ class Cluster(RPCProtocol):
         """
         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()
diff --git a/src/provisioningserver/rpc/tests/test_clusterservice.py b/src/provisioningserver/rpc/tests/test_clusterservice.py
index f71d02c..1c0e890 100644
--- a/src/provisioningserver/rpc/tests/test_clusterservice.py
+++ b/src/provisioningserver/rpc/tests/test_clusterservice.py
@@ -208,18 +208,15 @@ class TestClusterProtocol_StartTLS(MAASTestCase):
         return d.addCallback(check)
 
 
-class TestClusterProtocol_ListBootImages_and_ListBootImagesV2(MAASTestCase):
+class TestClusterProtocol_ListBootImages(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(self.rpc_call.commandName)
+        responder = protocol.locateResponder(
+            cluster.ListBootImages.commandName
+        )
         self.assertIsNotNone(responder)
 
     @inlineCallbacks
@@ -229,7 +226,7 @@ class TestClusterProtocol_ListBootImages_and_ListBootImagesV2(MAASTestCase):
         list_boot_images = self.patch(tftppath, "list_boot_images")
         list_boot_images.return_value = []
 
-        response = yield call_responder(Cluster(), self.rpc_call, {})
+        response = yield call_responder(Cluster(), cluster.ListBootImages, {})
 
         self.assertEqual({"images": []}, response)
 
@@ -287,7 +284,7 @@ class TestClusterProtocol_ListBootImages_and_ListBootImagesV2(MAASTestCase):
                 expected_image["xinstall_path"] = ""
                 expected_image["xinstall_type"] = ""
 
-        response = yield call_responder(Cluster(), self.rpc_call, {})
+        response = yield call_responder(Cluster(), cluster.ListBootImages, {})
 
         self.assertThat(response, KeysEqual("images"))
         self.assertItemsEqual(expected_images, response["images"])