Merge lp:~gmb/maas/label-in-pxe-config into lp:~maas-committers/maas/trunk

[Graham Binns]

Use the label of the latest available image for (nodegroup, arch, subarch, release, purpose) as the default label in pxeconfig(). Previously we always fell back to 'release' as the default label if one wasn't specified.

This was a fairly simple change but I'm not entirely happy with the if...else at line 15-18 of the diff. I put this in to avoid several pserv tests blowing up, but it now feels slightly lazy... Please tell me if I'm right about that and I'll go and fix up the tests and take that block out.

[Jeroen T. Vermeulen (jtv)]

Thanks for filing the bug. In these hectic days it's good to know we're keeping track.

The division of work here strikes me as very sensible.

I think the indentation in the test is missing something: the list comprehension for actual_images does not indent the arguments to get_latest_image.

No test for the case where you return no-such-image?

The structure of the test there is seems like it tries to do too many things, and the result is a bit muddled. I had to read it a few times to figure out your intention. Why not do a bit more to pry apart the behaviours you're trying to establish? “Returns the most recent matching image” is an easier test than what you have here — one purpose, one result. “Does not return image with different purpose” can also be simpler and clearer: create matching image, create newer image that matches except it's for a different purpose, show that the older (but matching) image is returned. And of course it'd be easy to throw in other newer images that differ from the matching one only in arch, subarch, or release — so that the test actually covers selection by the criteria other than purpose.

review: Approve

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2014-03-18 03:23:55 +0000
+++ src/maasserver/api.py	2014-03-18 14:42:32 +0000
@@ -2377,7 +2377,22 @@
 
         subarch = get_optional_param(request.GET, 'subarch', 'generic')
 
-    label = get_optional_param(request.GET, 'label', 'release')
+    # We use as our default label the label of the most recent image for
+    # the criteria we've assembled above. If there is no latest image
+    # (which should never happen in reality but may happen in tests), we
+    # fall back to using 'no-such-image' as our default.
+    latest_image = BootImage.objects.get_latest_image(
+        nodegroup, arch, subarch, series, purpose)
+    if latest_image is None:
+        # XXX 2014-03-18 gmb bug=1294131:
+        #     We really ought to raise an exception here so that client
+        #     and server can handle it according to their needs. At the
+        #     moment, though, that breaks too many tests in awkward
+        #     ways.
+        latest_label = 'no-such-image'
+    else:
+        latest_label = latest_image.label
+    label = get_optional_param(request.GET, 'label', latest_label)
 
     if node is not None:
         # We don't care if the kernel opts is from the global setting or a tag,
 
=== modified file 'src/maasserver/models/bootimage.py'
--- src/maasserver/models/bootimage.py	2014-03-11 23:08:35 +0000
+++ src/maasserver/models/bootimage.py	2014-03-18 14:42:32 +0000
@@ -106,6 +106,14 @@
             nodegroup, 'install')
         return arches_commissioning & arches_install
 
+    def get_latest_image(self, nodegroup, architecture, subarchitecture,
+                         release, purpose):
+        """Return the latest image for a set of criteria."""
+        return BootImage.objects.filter(
+            nodegroup=nodegroup, architecture=architecture,
+            subarchitecture=subarchitecture, release=release,
+            purpose=purpose).order_by('id').last()
+
 
 class BootImage(Model):
     """Available boot image (i.e. kernel and initrd).
 
=== modified file 'src/maasserver/models/tests/test_bootimage.py'
--- src/maasserver/models/tests/test_bootimage.py	2014-03-11 12:19:40 +0000
+++ src/maasserver/models/tests/test_bootimage.py	2014-03-18 14:42:32 +0000
@@ -162,3 +162,62 @@
         self.assertItemsEqual(
             [],
             BootImage.objects.get_usable_architectures(nodegroup))
+
+    def test_get_latest_image_returns_latest_image_for_criteria(self):
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('sub')
+        release = factory.make_name('release')
+        nodegroup = factory.make_node_group()
+        purpose = factory.make_name("purpose")
+        boot_image = factory.make_boot_image(
+            nodegroup=nodegroup, architecture=arch,
+            subarchitecture=subarch, release=release, purpose=purpose,
+            label=factory.make_name('label'))
+        self.assertEqual(
+            boot_image,
+            BootImage.objects.get_latest_image(
+                nodegroup, arch, subarch, release, purpose))
+
+    def test_get_latest_image_doesnt_return_images_for_other_purposes(self):
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('sub')
+        release = factory.make_name('release')
+        nodegroup = factory.make_node_group()
+        purpose = factory.make_name("purpose")
+        relevant_image = factory.make_boot_image(
+            nodegroup=nodegroup, architecture=arch,
+            subarchitecture=subarch, release=release, purpose=purpose,
+            label=factory.make_name('label'))
+
+        # Create a bunch of more recent but irrelevant BootImages..
+        factory.make_boot_image(
+            nodegroup=factory.make_node_group(), architecture=arch,
+            subarchitecture=subarch, release=release,
+            purpose=purpose, label=factory.make_name('label'))
+        factory.make_boot_image(
+            nodegroup=nodegroup,
+            architecture=factory.make_name('arch'),
+            subarchitecture=subarch, release=release, purpose=purpose,
+            label=factory.make_name('label'))
+        factory.make_boot_image(
+            nodegroup=nodegroup, architecture=arch,
+            subarchitecture=factory.make_name('subarch'),
+            release=release, purpose=purpose,
+            label=factory.make_name('label'))
+        factory.make_boot_image(
+            nodegroup=nodegroup,
+            architecture=factory.make_name('arch'),
+            subarchitecture=subarch,
+            release=factory.make_name('release'), purpose=purpose,
+            label=factory.make_name('label'))
+        factory.make_boot_image(
+            nodegroup=nodegroup,
+            architecture=factory.make_name('arch'),
+            subarchitecture=subarch, release=release,
+            purpose=factory.make_name('purpose'),
+            label=factory.make_name('label'))
+
+        self.assertEqual(
+            relevant_image,
+            BootImage.objects.get_latest_image(
+                nodegroup, arch, subarch, release, purpose))
 
=== modified file 'src/maasserver/tests/test_api_pxeconfig.py'
--- src/maasserver/tests/test_api_pxeconfig.py	2014-03-06 05:46:50 +0000
+++ src/maasserver/tests/test_api_pxeconfig.py	2014-03-18 14:42:32 +0000
@@ -341,3 +341,16 @@
         params['mac'] = mac.mac_address
         pxe_config = self.get_pxeconfig(params)
         self.assertEqual(None, pxe_config['extra_opts'])
+
+    def test_pxeconfig_sets_nonsense_label_for_insane_state(self):
+        # If pxeconfig() encounters a state where there is no relevant
+        # BootImage for a given set of (nodegroup, arch, subarch,
+        # release, purpose) it sets the label to no-such-image. This is
+        # clearly nonsensical, but this state only arises during tests
+        # or an insane environment.
+        mac = factory.make_mac_address()
+        params = self.get_default_params()
+        params['mac'] = mac.mac_address
+        params['arch'] = 'iHaveNoIdea'
+        pxe_config = self.get_pxeconfig(params)
+        self.assertEqual('no-such-image', pxe_config['label'])