Merge lp:~ltrager/maas/lp1566419 into lp:~maas-committers/maas/trunk

[Lee Trager]

Output boot_resources_in_sync on the rack controller API

[Gavin Panella (allenap)]

This seems like an expensive operation to be doing every time we
serialise a rack model object. What is the use case? Perhaps this would
be better modelled as an operation instead of a field?

Then, it's doing non-database IO (RPC calls) within a transaction, which
is bad because it keeps the transaction open for an indefinite time.
Plus, if the transaction must be retried, the non-database IO must be
done again.

These latter points are really hard to fix from within a Piston/Django
handler. MAAS is littered with examples of them, and to fix them we need
a degree of change that's way way out of scope right now. So, you get a
pass on that.

However, the first point, and the points I make in the diff, do need
fixing.

review: Needs Fixing

[Lee Trager (ltrager)]

The purpose of this is to help users automate MAAS deployments. If you create a script which installs MAAS, sets up user config, downloads images, and then enlists and commissions them you end up with the machines you tried to commission stuck in the BIOS with a 'No such file or directory' message. This is because you can use the API to check if the boot images have been downloaded to the region, but there is no way to see if they've been downloaded to the rack.

I've taken your suggestion and created a separate operation, list-boot-images on the rack-controller API endpoint. This lists all the boot images available currently on the rack controller. The ListBootImagesV2 RPC call returns a list of boot images with one entry per osystem/release/architecture/subarchitecture. I compressed this list down to osystem/release/architecture with the sub-architectures being an array. This is similar to how maas <profile> boot-resources read presents its data.

When the rack controller is disconnected list-boot-images returns an empty list. I added a new element returned on a read operation, is_connected. This returns True/False depending on if the rack controller is currently connected.

[Gavin Panella (allenap)]

This is a good improvement. I have more questions though!

review: Needs Information

[Lee Trager (ltrager)]

I've removed is_connected and added a connected field to list_boot_images.

Gavin, to answer you question about why I combine the osystem and release fields into one name fields its to try and output similarly to how the boot-resources API outputs. I'm on the fence whether I combine the fields or output them individually so let me know what you think and I'll do that.

[Gavin Panella (allenap)]

Cool, thank you for making all those changes. Looks good!

review: Approve

[MAAS Lander (maas-lander)]

Attempt to merge into lp:maas failed due to conflicts:

text conflict in src/maasserver/models/tests/test_node.py

=== modified file 'src/maasserver/api/rackcontrollers.py'
--- src/maasserver/api/rackcontrollers.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/rackcontrollers.py	2016-04-11 07:25:45 +0000
@@ -6,6 +6,7 @@
     'RackControllersHandler',
     ]
 
+
 from django.conf import settings
 from django.http import HttpResponse
 from maasserver.api.nodes import (
@@ -104,6 +105,19 @@
             "Import of boot images started on %s" % rack.hostname,
             content_type=("text/plain; charset=%s" % settings.DEFAULT_CHARSET))
 
+    @operation(idempotent=True)
+    def list_boot_images(self, request, system_id):
+        """List all available boot images.
+
+        Shows all available boot images and lists whether they are in sync with
+        the region.
+
+        Returns 404 if the rack controller is not found.
+        """
+        rack = self.model.objects.get_node_or_404(
+            system_id=system_id, user=request.user, perm=NODE_PERMISSION.VIEW)
+        return rack.list_boot_images()
+
     @classmethod
     def resource_uri(cls, rackcontroller=None):
         rackcontroller_id = "system_id"

=== modified file 'src/maasserver/api/tests/test_rackcontroller.py'
--- src/maasserver/api/tests/test_rackcontroller.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/tests/test_rackcontroller.py	2016-04-11 07:25:45 +0000
@@ -70,6 +70,16 @@
             http.client.FORBIDDEN, response.status_code,
             explain_unexpected_response(http.client.FORBIDDEN, response))
 
+    def test_GET_list_boot_images(self):
+        rack = factory.make_RackController(owner=factory.make_User())
+        response = self.client.get(
+            self.get_rack_uri(rack), {'op': 'list_boot_images'})
+        self.assertEqual(
+            http.client.OK, response.status_code,
+            explain_unexpected_response(http.client.OK, response))
+        self.assertItemsEqual(
+            ['connected', 'images'], json_load_bytes(response.content))
+
 
 class TestRackControllersAPI(APITestCase):
     """Tests for /api/2.0/rackcontrollers/."""

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-04-10 16:17:48 +0000
+++ src/maasserver/models/node.py	2016-04-11 07:25:45 +0000
@@ -12,7 +12,10 @@
     ]
 
 
-from collections import namedtuple
+from collections import (
+    defaultdict,
+    namedtuple,
+)
 from datetime import timedelta
 from functools import partial
 from operator import attrgetter
@@ -3604,6 +3607,34 @@
                     "Missing connections to %d region controller(s)." % (
                         dead_regions + len(missing_regions)))
 
+    def list_boot_images(self):
+        """Return a list of boot images available on the rack controller."""
+        # Avoid circular imports
+        from maasserver.clusterrpc.boot_images import get_boot_images
+        try:
+            # Combine all boot images one per name and arch
+            downloaded_boot_images = defaultdict(set)
+            for image in get_boot_images(self):
+                if image['osystem'] == 'custom':
+                    name = image['release']
+                else:
+                    name = "%s/%s" % (image['osystem'], image['release'])
+                arch = image['architecture']
+                subarch = image['subarchitecture']
+                downloaded_boot_images[name, arch].add(subarch)
+            # Prevent lint error 'list comprehension redefines'
+            del name, arch
+            # Return a list of dictionaries each containing one entry per
+            # name, architecture like boot-resources does
+            images = [{
+                'name': name,
+                'architecture': arch,
+                'subarches': sorted(subarches),
+            } for (name, arch), subarches in downloaded_boot_images.items()]
+            return {'images': images, 'connected': True}
+        except NoConnectionsAvailable:
+            return {'images': [], 'connected': False}
+
 
 class RegionController(Node):
     """A node which is running multiple regiond's."""

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2016-04-08 20:50:21 +0000
+++ src/maasserver/models/tests/test_node.py	2016-04-11 07:25:45 +0000
@@ -16,6 +16,7 @@
 from django.db import transaction
 from fixtures import LoggerFixture
 from maasserver import preseed as preseed_module
+from maasserver.clusterrpc import boot_images
 from maasserver.clusterrpc.power import (
     power_off_node,
     power_query,
@@ -6847,6 +6848,57 @@
                         len(regions_with_processes) +
                         len(regions_without_processes)))))
 
+    def test_list_boot_images(self):
+        rack_controller = factory.make_RackController()
+        self.patch(boot_images, 'get_boot_images').return_value = [
+            {
+                'release': 'custom_os',
+                'osystem': 'custom',
+                'architecture': 'amd64',
+                'subarchitecture': 'generic',
+            },
+            {
+                'release': 'trusty',
+                'osystem': 'ubuntu',
+                'architecture': 'amd64',
+                'subarchitecture': 'generic',
+            },
+            {
+                'release': 'trusty',
+                'osystem': 'ubuntu',
+                'architecture': 'amd64',
+                'subarchitecture': 'hwe-t',
+            },
+            {
+                'release': 'trusty',
+                'osystem': 'ubuntu',
+                'architecture': 'amd64',
+                'subarchitecture': 'hwe-x',
+            },
+        ]
+        self.assertItemsEqual(
+            {
+                'connected': True,
+                'images': [
+                    {
+                        'name': 'ubuntu/trusty',
+                        'architecture': 'amd64',
+                        'subarches': ['generic', 'hwe-t', 'hwe-x'],
+                    },
+                    {
+                        'name': 'custom_os',
+                        'architecture': 'amd64',
+                        'subarches': ['generic'],
+                    }
+                ]
+            }, rack_controller.list_boot_images())
+
+    def test_list_boot_images_when_disconnected(self):
+        rack_controller = factory.make_RackController()
+        self.assertItemsEqual(
+            {'connected': False, 'images': []},
+            rack_controller.list_boot_images())
+
 
 class TestRegionController(MAASServerTestCase):
 

=== modified file 'src/provisioningserver/rpc/cluster.py'
--- src/provisioningserver/rpc/cluster.py	2016-04-01 21:14:24 +0000
+++ src/provisioningserver/rpc/cluster.py	2016-04-11 07:25:45 +0000
@@ -39,7 +39,7 @@
 
 
 class ListBootImages(amp.Command):
-    """List the boot images available on this cluster controller.
+    """List the boot images available on this rack controller.
 
     :since: 1.5
     """
@@ -60,7 +60,7 @@
 
 
 class ListBootImagesV2(amp.Command):
-    """List the boot images available on this cluster controller.
+    """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.