Merge lp:~tycho-s/maas/fix-1181334 into lp:~maas-committers/maas/trunk

[Tycho Andersen]

Use existing image instead of defaulting to i386

Fixes bug #1181334

Use existing image instead of defaulting to i386

Fixes bug #1181334

[Julian Edwards (julian-edwards)]

Thanks for the fix Tycho. It's got a bug though!

26 + images = BootImage.objects.filter(release=series)

This will not restrict images by nodegroup. Remember that each nodegroup (cluster) has its own set of images.

It would be better if you added a new, tested, BootImagesManager method so it's widely available. Something like:

    get_default_arch_image_in_nodegroup(self, nodegroup, series, purpose):
        """Return the first image available for any architecture in the nodegroup/series supplied."""
        images = BootImage.objects.filter(release=series, nodegroup=nodegroup, purpose=purpose)
        return get_one(images)

You need to test that:
 * it returns None if no images are available
 * it returns the only image if only one image is available
 * it returns the first image if multiple images are available

As an aside, pxeconfig is far too big already and needs better unit tests, but we'll leave that for another day...

Cheers.

review: Needs Fixing

[Tycho Andersen (tycho-s)]

On Thu, Nov 14, 2013 at 12:30:20AM -0000, Julian Edwards wrote:
> Review: Needs Fixing
>
> Thanks for the fix Tycho. It's got a bug though!
>
> 26 + images = BootImage.objects.filter(release=series)
>
> This will not restrict images by nodegroup. Remember that each nodegroup (cluster) has its own set of images.
>
> It would be better if you added a new, tested, BootImagesManager method so it's widely available. Something like:
>
> get_default_arch_image_in_nodegroup(self, nodegroup, series, purpose):
> """Return the first image available for any architecture in the nodegroup/series supplied."""
> images = BootImage.objects.filter(release=series, nodegroup=nodegroup, purpose=purpose)
> return get_one(images)
>
> You need to test that:
> * it returns None if no images are available
> * it returns the only image if only one image is available
> * it returns the first image if multiple images are available
>
> As an aside, pxeconfig is far too big already and needs better unit tests, but we'll leave that for another day...

Tried to make those changes, except now I can't get the test framework
to behave. As near as I can tell, I'm doing the same thing as I was
before. Thoughts?

\t

[Gavin Panella (allenap)]

I've tried getting the tests passing, and came up with: http://paste.ubuntu.com/6418301/

There needs to be some ordering in get_default_arch_image_in_nodegroup() to give a stable result.

I had to specify purpose. You were passing None. If that was meant to mean "no preference" then you'll need some more logic in get_default_arch_image_in_nodegroup() too.

[Tycho Andersen (tycho-s)]

On Thu, Nov 14, 2013 at 10:36:30PM -0000, Gavin Panella wrote:
> I've tried getting the tests passing, and came up with: http://paste.ubuntu.com/6418301/
>
> There needs to be some ordering in get_default_arch_image_in_nodegroup() to give a stable result.
>
> I had to specify purpose. You were passing None. If that was meant to mean "no preference" then you'll need some more logic in get_default_arch_image_in_nodegroup() too.

Ah ha, I felt like ignoring purpose was probably a bad idea. Thanks!

\t

[Julian Edwards (julian-edwards)]

Looks ok to me now, unless Gavin has any more to add?

review: Approve

[Gavin Panella (allenap)]

Nope, looks good.

review: Approve

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2013-11-15 13:24:55 +0000
+++ src/maasserver/api.py	2013-11-15 15:29:36 +0000
@@ -2229,6 +2229,13 @@
     """
     node = get_node_from_mac_string(request.GET.get('mac', None))
 
+    if node is None or node.status == NODE_STATUS.COMMISSIONING:
+        series = Config.objects.get_config('commissioning_distro_series')
+    else:
+        series = node.get_distro_series()
+
+    purpose = get_boot_purpose(node)
+
     if node:
         arch, subarch = node.architecture.split('/')
         preseed_url = compose_preseed_url(node)
@@ -2238,6 +2245,11 @@
         nodegroup = node.nodegroup
         domain = nodegroup.name
     else:
+        nodegroup = find_nodegroup_for_pxeconfig_request(request)
+        preseed_url = compose_enlistment_preseed_url(nodegroup=nodegroup)
+        hostname = 'maas-enlist'
+        domain = Config.objects.get_config('enlistment_domain')
+
         try:
             pxelinux_arch = request.GET['arch']
         except KeyError:
@@ -2246,9 +2258,15 @@
                 # to pxelinux.cfg/default-<arch>-<subarch> for arch detection.
                 return HttpResponse(status=httplib.NO_CONTENT)
             else:
-                # Request has already fallen back, so if arch is still not
-                # provided then use i386.
-                arch = ARCHITECTURE.i386.split('/')[0]
+                # Look in BootImage for an image that actually exists for the
+                # current series. If nothing is found, fall back to i386 like
+                # we used to. LP #1181334
+                image = BootImage.objects.get_default_arch_image_in_nodegroup(
+                    nodegroup, series, purpose=purpose)
+                if image is None:
+                    arch = ARCHITECTURE.i386.split('/')[0]
+                else:
+                    arch = image.architecture
         else:
             # Map from pxelinux namespace architecture names to MAAS namespace
             # architecture names. If this gets bigger, an external lookup table
@@ -2270,16 +2288,6 @@
             # 1-1 mapping.
             subarch = pxelinux_subarch
 
-        nodegroup = find_nodegroup_for_pxeconfig_request(request)
-        preseed_url = compose_enlistment_preseed_url(nodegroup=nodegroup)
-        hostname = 'maas-enlist'
-        domain = Config.objects.get_config('enlistment_domain')
-
-    if node is None or node.status == NODE_STATUS.COMMISSIONING:
-        series = Config.objects.get_config('commissioning_distro_series')
-    else:
-        series = node.get_distro_series()
-
     if node is not None:
         # We don't care if the kernel opts is from the global setting or a tag,
         # just get the options
@@ -2289,7 +2297,6 @@
         # we still need to return the global kernel options.
         extra_kernel_opts = Config.objects.get_config("kernel_opts")
 
-    purpose = get_boot_purpose(node)
     server_address = get_maas_facing_server_address(nodegroup=nodegroup)
     cluster_address = get_mandatory_param(request.GET, "local")
 
 
=== modified file 'src/maasserver/models/bootimage.py'
--- src/maasserver/models/bootimage.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/models/bootimage.py	2013-11-15 15:29:36 +0000
@@ -25,6 +25,7 @@
     )
 from maasserver import DefaultMeta
 from maasserver.models.nodegroup import NodeGroup
+from maasserver.utils.orm import get_first
 
 
 class BootImageManager(Manager):
@@ -61,6 +62,15 @@
         except BootImage.DoesNotExist:
             return False
 
+    def get_default_arch_image_in_nodegroup(self, nodegroup, series, purpose):
+        """Return the first image available for any architecture in the
+        nodegroup/series supplied.
+        """
+        images = BootImage.objects.filter(
+            release=series, nodegroup=nodegroup, purpose=purpose)
+        images = images.order_by('architecture')
+        return get_first(images)
+
 
 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	2013-10-07 09:12:40 +0000
+++ src/maasserver/models/tests/test_bootimage.py	2013-11-15 15:29:36 +0000
@@ -17,6 +17,7 @@
 from maasserver.models import (
     BootImage,
     NodeGroup,
+    Config,
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -52,3 +53,32 @@
         BootImage.objects.register_image(self.nodegroup, **params)
         self.assertTrue(
             BootImage.objects.have_image(self.nodegroup, **params))
+
+    def test_default_arch_image_none(self):
+        series = Config.objects.get_config('commissioning_distro_series')
+        result = BootImage.objects.get_default_arch_image_in_nodegroup(
+            self.nodegroup, series, None)
+        self.assertIsNone(result)
+
+    def test_default_arch_image_one(self):
+        series = Config.objects.get_config('commissioning_distro_series')
+        purpose = factory.make_name("purpose")
+        factory.make_boot_image(
+            architecture="amd64", release=series, nodegroup=self.nodegroup,
+            purpose=purpose)
+        result = BootImage.objects.get_default_arch_image_in_nodegroup(
+            self.nodegroup, series, purpose=purpose)
+        self.assertEqual(result.architecture, "amd64")
+
+    def test_default_arch_image_many(self):
+        series = Config.objects.get_config('commissioning_distro_series')
+        purpose = factory.make_name("purpose")
+        factory.make_boot_image(
+            architecture="amd64", release=series, nodegroup=self.nodegroup,
+            purpose=purpose)
+        factory.make_boot_image(
+            architecture="other", release=series, nodegroup=self.nodegroup,
+            purpose=purpose)
+        result = BootImage.objects.get_default_arch_image_in_nodegroup(
+            self.nodegroup, series, purpose=purpose)
+        self.assertEqual(result.architecture, "amd64")
 
=== modified file 'src/maasserver/tests/test_api_pxeconfig.py'
--- src/maasserver/tests/test_api_pxeconfig.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/tests/test_api_pxeconfig.py	2013-11-15 15:29:36 +0000
@@ -134,6 +134,17 @@
         response_dict = json.loads(response.content)
         self.assertEqual(value, response_dict['extra_opts'])
 
+    def test_pxeconfig_uses_present_boot_image(self):
+        release = Config.objects.get_config('commissioning_distro_series')
+        nodegroup = factory.make_node_group()
+        factory.make_boot_image(
+            architecture="amd64", release=release, nodegroup=nodegroup,
+            purpose="commissioning")
+        params = self.get_default_params()
+        params['cluster_uuid'] = nodegroup.uuid
+        params_out = self.get_pxeconfig(params)
+        self.assertEqual("amd64", params_out["arch"])
+
     def test_pxeconfig_defaults_to_i386_for_default(self):
         # As a lowest-common-denominator, i386 is chosen when the node is not
         # yet known to MAAS.