Merge lp:~allenap/maas/kernel-initrd-parameters into lp:~maas-committers/maas/trunk

[Gavin Panella]

Calculate the PXE options KERNEL and INITRD in the pxeconfig view.

Calculate the kernel and initrd options in the pxeconfig view, instead of relying on them being passed from the dynamic TFTP server. The latter does not have all the necessary information at its disposal.

[Raphaël Badin (rvb)]

Looks good. Just a few remarks but nothing major.

[0]

221 + title=title, kernel="{}/kernel".format(image_dir),
222 + initrd="{}/initrd.gz".format(image_dir), append=append)

I also like .format() but I think using "%s/kernel" might be a little bit clearer in this case.

[1]

28 + if node is None:
29 + # This node is enlisting, for which we use a commissioning
30 + # image. TODO: Do we?

I /think/ smoser is working on that, maybe get in touch with to see where he is at with that.

[2]

93 + # TODO: don't hard-code release.

You can reference bug 1013146 here.

review: Approve

[Gavin Panella (allenap)]

Thanks for the review, all suggestions gladly accepted.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-07-31 09:10:44 +0000
+++ src/maasserver/api.py	2012-07-31 15:56:24 +0000
@@ -1061,52 +1061,79 @@
         query={'op': 'get_preseed'})
 
 
-def compose_preseed_kernel_opt(mac):
-    """Compose a kernel option for preseed URL for node that owns `mac`."""
-    macaddress_match = MACAddress.objects.filter(mac_address=mac)
-    if len(macaddress_match) == 0:
+def compose_preseed_kernel_opt(node):
+    """Compose a kernel option for preseed URL for given `node`.
+
+    :param mac_address: A `Node`, or `None`.
+    """
+    if node is None:
         preseed_url = compose_enlistment_preseed_url()
     else:
-        [macaddress] = macaddress_match
-        preseed_url = compose_preseed_url(macaddress.node)
+        preseed_url = compose_preseed_url(node)
     return "auto url=%s" % preseed_url
 
 
+def get_boot_purpose(node):
+    """Return a suitable "purpose" for this boot, e.g. "install"."""
+    # XXX: allenap bug=1031406 2012-07-31: The boot purpose is still in
+    # flux. It may be that there will just be an "ephemeral" environment and
+    # an "install" environment, and the differing behaviour between, say,
+    # enlistment and commissioning - both of which will use the "ephemeral"
+    # environment - will be governed by varying the preseed or PXE
+    # configuration.
+    if node is None:
+        # This node is enlisting, for which we use a commissioning image.
+        return "commissioning"
+    elif node.status == NODE_STATUS.COMMISSIONING:
+        # It is commissioning.
+        return "commissioning"
+    elif node.status == NODE_STATUS.ALLOCATED:
+        # Install the node if netboot is enabled, otherwise boot locally.
+        if node.netboot:
+            return "install"
+        else:
+            return "local"  # TODO: Investigate.
+    else:
+        # Just poweroff? TODO: Investigate. Perhaps even send an IPMI signal
+        # to turn off power.
+        return "poweroff"
+
+
 def pxeconfig(request):
     """Get the PXE configuration given a node's details.
 
     :param mac: MAC address to produce a boot configuration for.  This
         parameter is optional.  If it is not given, the configuration
         will be the "default" one which boots into an enlistment image.
+    :param arch: Main machine architecture.
+    :param subarch: Sub-architecture, or "generic" if there is none.
     :param title: Title that the node should show in its PXE menu.
-    :param kernel: TFTP path to the kernel image that is to be booted.
-    :param initrd: TFTP path to the initrd that is to be booted.
     :param append: Additional parameters to append to the kernel command
         line.
-
-    In addition, the following parameters are expected, but are not used:
-
-    :param arch: Main machine architecture.
-    :param subarch: Sub-architecture, or "generic" if there is none.
     """
-    title = get_mandatory_param(request.GET, 'title')
-    kernel = get_mandatory_param(request.GET, 'kernel')
-    initrd = get_mandatory_param(request.GET, 'initrd')
-    append = get_mandatory_param(request.GET, 'append')
     mac = request.GET.get('mac', None)
-
-    # The following two parameters - arch and subarch - typically must be
-    # supplied, but are unused right now.
     arch = get_mandatory_param(request.GET, 'arch')
     subarch = request.GET.get('subarch', 'generic')
-    arch, subarch  # Suppress lint warnings.
+    title = get_mandatory_param(request.GET, 'title')
+    append = get_mandatory_param(request.GET, 'append')
+
+    # See if we have a record of this MAC address, and thus node.
+    try:
+        macaddress = MACAddress.objects.get(mac_address=mac)
+    except MACAddress.DoesNotExist:
+        macaddress = node = None
+    else:
+        node = macaddress.node
 
     # In addition to the "append" parameter, also add a URL for the
     # node's preseed to the kernel command line.
-    append = "%s %s" % (append, compose_preseed_kernel_opt(mac))
+    append = "%s %s" % (append, compose_preseed_kernel_opt(node))
+
+    # XXX: allenap 2012-07-31 bug=1013146: 'precise' is hardcoded here.
+    release = "precise"
 
     return HttpResponse(
         render_pxe_config(
-            title=title, kernel=kernel,
-            initrd=initrd, append=append),
+            title=title, arch=arch, subarch=subarch, release=release,
+            purpose=get_boot_purpose(node), append=append),
         content_type="text/plain; charset=utf-8")

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-07-31 09:10:44 +0000
+++ src/maasserver/tests/test_api.py	2012-07-31 15:56:24 +0000
@@ -87,6 +87,7 @@
     POWER_TYPE_CHOICES,
     )
 from testtools.matchers import (
+    Contains,
     Equals,
     MatchesListwise,
     StartsWith,
@@ -2261,8 +2262,6 @@
                 'subarch': "armadaxp",
                 'mac': factory.make_mac_address().mac_address,
                 'title': factory.make_name("Menu"),
-                'kernel': factory.make_name("/my/kernel"),
-                'initrd': factory.make_name("/my/initrd"),
                 'append': factory.make_name("append"),
             }
 
@@ -2300,8 +2299,6 @@
             'subarch': httplib.OK,
             'mac': httplib.OK,
             'title': httplib.BAD_REQUEST,
-            'kernel': httplib.BAD_REQUEST,
-            'initrd': httplib.BAD_REQUEST,
             'append': httplib.BAD_REQUEST,
             }
         observed = {
@@ -2337,15 +2334,15 @@
                 api.compose_preseed_url(node), StartsWith(url))
 
     def test_compose_preseed_kernel_opt_returns_option_for_known_node(self):
-        mac = factory.make_mac_address()
+        node = factory.make_node()
         self.assertEqual(
-            "auto url=%s" % api.compose_preseed_url(mac.node),
-            api.compose_preseed_kernel_opt(mac.mac_address))
+            "auto url=%s" % api.compose_preseed_url(node),
+            api.compose_preseed_kernel_opt(node))
 
     def test_compose_preseed_kernel_opt_returns_option_for_unknown_node(self):
         self.assertEqual(
             "auto url=%s" % api.compose_enlistment_preseed_url(),
-            api.compose_preseed_kernel_opt(factory.getRandomMACAddress()))
+            api.compose_preseed_kernel_opt(None))
 
     def test_pxe_config_appends_enlistment_preseed_url_for_unknown_node(self):
         params = self.get_params()
@@ -2359,6 +2356,37 @@
         response = self.client.get(reverse('pxeconfig'), params)
         self.assertIn(api.compose_preseed_url(node), response.content)
 
+    def test_get_boot_purpose_unknown_node(self):
+        # A node that's not yet known to MAAS is assumed to be enlisting,
+        # which uses a "commissioning" image.
+        self.assertEqual("commissioning", api.get_boot_purpose(None))
+
+    def test_get_boot_purpose_known_node(self):
+        # The following table shows the expected boot "purpose" for each set
+        # of node parameters.
+        options = [
+            ("poweroff", {"status": NODE_STATUS.DECLARED}),
+            ("commissioning", {"status": NODE_STATUS.COMMISSIONING}),
+            ("poweroff", {"status": NODE_STATUS.FAILED_TESTS}),
+            ("poweroff", {"status": NODE_STATUS.MISSING}),
+            ("poweroff", {"status": NODE_STATUS.READY}),
+            ("poweroff", {"status": NODE_STATUS.RESERVED}),
+            ("install", {"status": NODE_STATUS.ALLOCATED, "netboot": True}),
+            ("local", {"status": NODE_STATUS.ALLOCATED, "netboot": False}),
+            ("poweroff", {"status": NODE_STATUS.RETIRED}),
+            ]
+        node = factory.make_node()
+        for purpose, parameters in options:
+            for name, value in parameters.items():
+                setattr(node, name, value)
+            self.assertEqual(purpose, api.get_boot_purpose(node))
+
+    def test_pxe_config_uses_boot_purpose(self):
+        fake_boot_purpose = factory.make_name("purpose")
+        self.patch(api, "get_boot_purpose", lambda node: fake_boot_purpose)
+        response = self.client.get(reverse('pxeconfig'), self.get_params())
+        self.assertThat(response.content, Contains(fake_boot_purpose))
+
 
 class TestNodeGroupsAPI(APITestCase):
 

=== modified file 'src/provisioningserver/pxe/config.py'
--- src/provisioningserver/pxe/config.py	2012-07-30 20:59:31 +0000
+++ src/provisioningserver/pxe/config.py	2012-07-31 15:56:24 +0000
@@ -22,6 +22,7 @@
 
 from os import path
 
+from provisioningserver.pxe.tftppath import compose_image_path
 import tempita
 
 
@@ -30,13 +31,17 @@
 template = tempita.Template.from_filename(template_filename, encoding="UTF-8")
 
 
-def render_pxe_config(title, kernel, initrd, append):
+def render_pxe_config(title, arch, subarch, release, purpose, append):
     """Render a PXE configuration file as a unicode string.
 
     :param title: Title that the node should show on its boot menu.
-    :param kernel: TFTP path to the kernel image to boot.
-    :param initrd: TFTP path to the initrd file to boot from.
+    :param arch: Main machine architecture.
+    :param subarch: Sub-architecture, or "generic" if there is none.
+    :param release: The OS release, e.g. "precise".
+    :param purpose: What's the purpose of this boot, e.g. "install".
     :param append: Additional kernel parameters.
     """
+    image_dir = compose_image_path(arch, subarch, release, purpose)
     return template.substitute(
-        title=title, kernel=kernel, initrd=initrd, append=append)
+        title=title, kernel="%s/kernel" % image_dir,
+        initrd="%s/initrd.gz" % image_dir, append=append)

=== modified file 'src/provisioningserver/pxe/tests/test_config.py'
--- src/provisioningserver/pxe/tests/test_config.py	2012-07-31 08:34:59 +0000
+++ src/provisioningserver/pxe/tests/test_config.py	2012-07-31 15:56:24 +0000
@@ -12,13 +12,16 @@
 __metaclass__ = type
 __all__ = []
 
+import re
+
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
 from provisioningserver.pxe.config import render_pxe_config
+from provisioningserver.pxe.tftppath import compose_image_path
 from testtools.matchers import (
-    Contains,
     IsInstance,
     MatchesAll,
+    MatchesRegex,
     StartsWith,
     )
 
@@ -31,8 +34,10 @@
         # correctly rendered.
         options = {
             "title": factory.make_name("title"),
-            "kernel": factory.make_name("kernel"),
-            "initrd": factory.make_name("initrd"),
+            "arch": factory.make_name("arch"),
+            "subarch": factory.make_name("subarch"),
+            "release": factory.make_name("release"),
+            "purpose": factory.make_name("purpose"),
             "append": factory.make_name("append"),
             }
         output = render_pxe_config(**options)
@@ -41,6 +46,21 @@
         # The template has rendered without error. PXELINUX configurations
         # typically start with a DEFAULT line.
         self.assertThat(output, StartsWith("DEFAULT "))
-        # All of the values put in are included somewhere in the output.
-        expected = (Contains(value) for value in options.values())
-        self.assertThat(output, MatchesAll(*expected))
+        # The PXE parameters are all set according to the options.
+        image_dir = compose_image_path(
+            arch=options["arch"], subarch=options["subarch"],
+            release=options["release"], purpose=options["purpose"])
+        self.assertThat(
+            output, MatchesAll(
+                MatchesRegex(
+                    r'.*^MENU TITLE %s$' % re.escape(options["title"]),
+                    re.MULTILINE | re.DOTALL),
+                MatchesRegex(
+                    r'.*^\s+KERNEL %s/kernel$' % re.escape(image_dir),
+                    re.MULTILINE | re.DOTALL),
+                MatchesRegex(
+                    r'.*^\s+INITRD %s/initrd[.]gz$' % re.escape(image_dir),
+                    re.MULTILINE | re.DOTALL),
+                MatchesRegex(
+                    r'.*^\s+APPEND %s$' % re.escape(options["append"]),
+                    re.MULTILINE | re.DOTALL)))

=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py	2012-07-30 21:28:47 +0000
+++ src/provisioningserver/tests/test_tftp.py	2012-07-31 15:56:24 +0000
@@ -131,16 +131,10 @@
         arch = factory.make_name("arch").encode("ascii")
         subarch = factory.make_name("subarch").encode("ascii")
         mac = factory.getRandomMACAddress(b"-")
-        kernel = factory.make_name("kernel").encode("ascii")
-        initrd = factory.make_name("initrd").encode("ascii")
         title = factory.make_name("menu-title").encode("ascii")
         append = factory.make_name("append").encode("ascii")
-        backend_url = b"http://example.com/?" + urlencode({
-            b"kernel": kernel,
-            b"initrd": initrd,
-            b"title": title,
-            b"append": append,
-            })
+        backend_url = b"http://example.com/?" + (
+            urlencode({b"title": title, b"append": append}))
         backend = TFTPBackend(self.make_dir(), backend_url)
         # params is an example of the parameters obtained from a request.
         params = {"arch": arch, "subarch": subarch, "mac": mac}
@@ -149,8 +143,6 @@
         query = parse_qsl(generator_url.query)
         query_expected = [
             ("append", append),
-            ("kernel", kernel),
-            ("initrd", initrd),
             ("arch", arch),
             ("subarch", subarch),
             ("title", title),

=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2012-07-30 21:28:47 +0000
+++ src/provisioningserver/tftp.py	2012-07-31 15:56:24 +0000
@@ -111,8 +111,6 @@
         # TODO: update query defaults.
         query = {
             b"title": b"",
-            b"kernel": b"",
-            b"initrd": b"",
             b"append": b"",
             }
         # Merge parameters from the generator URL.