Merge lp:~rvb/maas/comm-templ-bug-1410367 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

In the commissioning and the xinstall templates, ditch the architecture auto-detection and use what's provided in the boot parameters.

It's not entirely clear to me why we were using ifcpu64.c32 in the first place (it's been like that since inception) so I'm putting this up for review to get feedback.
I've tested this on my NUCs but I'm planning to do some additional testing once the CI is green again.

[Blake Rouse (blake-rouse)]

Looks good.

The ifcpu64 should only exists in the enlist template so the correct architecture is loaded for the kernel and initramfs. I guess it just happened to get copied into the commissioning script, its amazing it has been like this for this long.

review: Approve

[Raphaël Badin (rvb)]

> The ifcpu64 should only exists in the enlist template so the correct architecture is loaded for
> the kernel and initramfs. I guess it just happened to get copied into the commissioning
> script, its amazing it has been like this for this long.

Turns out the commissioning and the enlistment templates were the same… hence the bug. I've added an enlistment template for the case where arch-detection needs to happen.

[Blake Rouse (blake-rouse)]

You need you to add a uefi/config.enlist.template as well. The UEFIBootMethod will expect having that template as well since the pxeconfig call was changed to return enlist.

review: Needs Fixing

[Raphaël Badin (rvb)]

> You need you to add a uefi/config.enlist.template as well. The UEFIBootMethod
> will expect having that template as well since the pxeconfig call was changed
> to return enlist.

Well spotted! I added the template along with a couple of missing tests.

[Blake Rouse (blake-rouse)]

Awesome! Thanks for fixing it up.

A CI run would be good to make sure this is correct, but I don't know if that is fully working yet with the transaction issues.

review: Approve

[Raphaël Badin (rvb)]

> Awesome! Thanks for fixing it up.
>
> A CI run would be good to make sure this is correct, but I don't know if that
> is fully working yet with the transaction issues.

Already done, and it's working fine (http://162.213.35.104:8080/job/maas-utopic-trunk-manual/25/). And I also tested on my NUCs.

Don't forget you're talking to the CI police here :).

=== modified file 'etc/maas/templates/pxe/config.commissioning.template'
--- etc/maas/templates/pxe/config.commissioning.template	2013-06-05 06:19:05 +0000
+++ etc/maas/templates/pxe/config.commissioning.template	2015-01-19 13:41:25 +0000
@@ -1,20 +1,9 @@
 DEFAULT execute
 
-SAY Booting under MAAS direction...
-SAY {{kernel_params() | kernel_command}}
-
 LABEL execute
-  KERNEL ifcpu64.c32
-  APPEND amd64 -- i386
-
-LABEL amd64
-  KERNEL {{kernel_params(arch="amd64") | kernel_path }}
-  INITRD {{kernel_params(arch="amd64") | initrd_path }}
-  APPEND {{kernel_params(arch="amd64") | kernel_command}}
-  IPAPPEND 2
-
-LABEL i386
-  KERNEL {{kernel_params(arch="i386") | kernel_path }}
-  INITRD {{kernel_params(arch="i386") | initrd_path }}
-  APPEND {{kernel_params(arch="i386") | kernel_command}}
+  SAY Booting under MAAS direction...
+  SAY {{kernel_params() | kernel_command}}
+  KERNEL {{kernel_params | kernel_path }}
+  INITRD {{kernel_params | initrd_path }}
+  APPEND {{kernel_params | kernel_command}}
   IPAPPEND 2
 
=== added file 'etc/maas/templates/pxe/config.enlist.template'
--- etc/maas/templates/pxe/config.enlist.template	1970-01-01 00:00:00 +0000
+++ etc/maas/templates/pxe/config.enlist.template	2015-01-19 13:41:25 +0000
@@ -0,0 +1,19 @@
+DEFAULT execute
+ 
+SAY Booting under MAAS direction...
+SAY {{kernel_params() | kernel_command}}
+
+LABEL execute
+  KERNEL ifcpu64.c32
+  APPEND amd64 -- i386
+
+LABEL amd64
+  KERNEL {{kernel_params(arch="amd64") | kernel_path }}
+  INITRD {{kernel_params(arch="amd64") | initrd_path }}
+  APPEND {{kernel_params(arch="amd64") | kernel_command}}
+  IPAPPEND 2
+
+LABEL i386
+  KERNEL {{kernel_params(arch="i386") | kernel_path }}
+  INITRD {{kernel_params(arch="i386") | initrd_path }}
+  APPEND {{kernel_params(arch="i386") | kernel_command}}
 
=== modified file 'etc/maas/templates/pxe/config.xinstall.template'
--- etc/maas/templates/pxe/config.xinstall.template	2013-06-03 11:32:28 +0000
+++ etc/maas/templates/pxe/config.xinstall.template	2015-01-19 13:41:25 +0000
@@ -1,20 +1,9 @@
 DEFAULT execute
 
-SAY Booting under MAAS direction...
-SAY {{kernel_params() | kernel_command}}
-
 LABEL execute
-  KERNEL ifcpu64.c32
-  APPEND amd64 -- i386
-
-LABEL amd64
-  KERNEL {{kernel_params(arch="amd64") | kernel_path }}
-  INITRD {{kernel_params(arch="amd64") | initrd_path }}
-  APPEND {{kernel_params(arch="amd64") | kernel_command}}
-  IPAPPEND 2
-
-LABEL i386
-  KERNEL {{kernel_params(arch="i386") | kernel_path }}
-  INITRD {{kernel_params(arch="i386") | initrd_path }}
-  APPEND {{kernel_params(arch="i386") | kernel_command}}
+  SAY Booting under MAAS direction...
+  SAY {{kernel_params() | kernel_command}}
+  KERNEL {{kernel_params | kernel_path }}
+  INITRD {{kernel_params | initrd_path }}
+  APPEND {{kernel_params | kernel_command}}
   IPAPPEND 2
 
=== added file 'etc/maas/templates/uefi/config.enlist.template'
--- etc/maas/templates/uefi/config.enlist.template	1970-01-01 00:00:00 +0000
+++ etc/maas/templates/uefi/config.enlist.template	2015-01-19 13:41:25 +0000
@@ -0,0 +1,8 @@
+set default="0"
+set timeout=0
+
+menuentry 'Enlist' {
+    echo   'Booting under MAAS direction...'
+    linux  {{kernel_params | kernel_path }} {{kernel_params | kernel_command}} BOOTIF=01-${net_default_mac}
+    initrd {{kernel_params | initrd_path }}
+}
 
=== modified file 'src/maasserver/api/pxeconfig.py'
--- src/maasserver/api/pxeconfig.py	2014-12-12 19:21:25 +0000
+++ src/maasserver/api/pxeconfig.py	2015-01-19 13:41:25 +0000
@@ -135,6 +135,9 @@
         event_description=options[purpose])
 
 
+DEFAULT_ARCH = 'i386'
+
+
 def pxeconfig(request):
     """Get the PXE configuration given a node's details.
 
@@ -214,7 +217,7 @@
                     BootResource.objects.get_default_commissioning_resource(
                         osystem, series))
                 if resource is None:
-                    arch = 'i386'
+                    arch = DEFAULT_ARCH
                 else:
                     arch, _ = resource.split_arch()
 
@@ -293,6 +296,12 @@
     server_address = get_maas_facing_server_address(nodegroup=nodegroup)
     cluster_address = get_mandatory_param(request.GET, "local")
 
+    # If the node is enlisting and the arch is the default arch (i386),
+    # use the dedicated enlistment template which performs architecture
+    # detection.
+    if node is None and arch == DEFAULT_ARCH:
+        boot_purpose = "enlist"
+
     params = KernelParameters(
         osystem=osystem, arch=arch, subarch=subarch, release=series,
         label=label, purpose=boot_purpose, hostname=hostname, domain=domain,
 
=== modified file 'src/maasserver/api/tests/test_pxeconfig.py'
--- src/maasserver/api/tests/test_pxeconfig.py	2014-12-12 19:21:25 +0000
+++ src/maasserver/api/tests/test_pxeconfig.py	2015-01-19 13:41:25 +0000
@@ -396,11 +396,17 @@
         # test that purpose is set to "commissioning" for
         # enlistment (when node is None).
         params = self.get_default_params()
+        params['arch'] = 'armhf'
         response = self.client.get(reverse('pxeconfig'), params)
         self.assertEqual(
             "commissioning",
             json.loads(response.content)["purpose"])
 
+    def test_pxeconfig_returns_enlist_config_if_no_architecture_provided(self):
+        params = self.get_default_params()
+        pxe_config = self.get_pxeconfig(params)
+        self.assertEqual('enlist', pxe_config['purpose'])
+
     def test_pxeconfig_returns_fs_host_as_cluster_controller(self):
         # The kernel parameter `fs_host` points to the cluster controller
         # address, which is passed over within the `local` parameter.
 
=== modified file 'src/provisioningserver/boot/tests/test_pxe.py'
--- src/provisioningserver/boot/tests/test_pxe.py	2014-09-18 12:44:38 +0000
+++ src/provisioningserver/boot/tests/test_pxe.py	2015-01-19 13:41:25 +0000
@@ -324,6 +324,34 @@
         ]
 
     def test_get_reader_scenarios(self):
+        method = PXEBootMethod()
+        get_ephemeral_name = self.patch(kernel_opts, "get_ephemeral_name")
+        get_ephemeral_name.return_value = factory.make_name("ephemeral")
+        osystem = factory.make_name('osystem')
+        arch = factory.make_name('arch')
+        subarch = factory.make_name('subarch')
+        options = {
+            "backend": None,
+            "kernel_params": make_kernel_parameters(
+                testcase=self, osystem=osystem, subarch=subarch,
+                arch=arch, purpose=self.purpose),
+        }
+        output = method.get_reader(**options).read(10000)
+        config = parse_pxe_config(output)
+        # The default section is defined.
+        default_section_label = config.header["DEFAULT"]
+        self.assertThat(config, Contains(default_section_label))
+        default_section = dict(config[default_section_label])
+
+        contains_arch_path = StartsWith("%s/%s/%s" % (osystem, arch, subarch))
+        self.assertThat(default_section["KERNEL"], contains_arch_path)
+        self.assertThat(default_section["INITRD"], contains_arch_path)
+        self.assertEquals("2", default_section["IPAPPEND"])
+
+
+class TestPXEBootMethodRenderConfigScenariosEnlist(MAASTestCase):
+
+    def test_get_reader_scenarios(self):
         # The commissioning config uses an extra PXELINUX module to auto
         # select between i386 and amd64.
         method = PXEBootMethod()
@@ -334,7 +362,7 @@
             "backend": None,
             "kernel_params": make_kernel_parameters(
                 testcase=self, osystem=osystem, subarch="generic",
-                purpose=self.purpose),
+                purpose='enlist'),
         }
         output = method.get_reader(**options).read(10000)
         config = parse_pxe_config(output)
 
=== modified file 'src/provisioningserver/boot/tests/test_uefi.py'
--- src/provisioningserver/boot/tests/test_uefi.py	2014-09-18 12:44:38 +0000
+++ src/provisioningserver/boot/tests/test_uefi.py	2015-01-19 13:41:25 +0000
@@ -33,6 +33,7 @@
 from provisioningserver.rpc.testing import MockLiveClusterToRegionRPCFixture
 from provisioningserver.tests.test_kernel_opts import make_kernel_parameters
 from testtools.matchers import (
+    ContainsAll,
     IsInstance,
     MatchesAll,
     MatchesRegex,
@@ -152,6 +153,42 @@
         output = method.get_reader(**options).read(10000)
         self.assertIn("configfile /efi/ubuntu/grub.cfg", output)
 
+    def test_get_reader_with_enlist_purpose(self):
+        # If purpose is "enlist", the config.enlist.template should be
+        # used.
+        method = UEFIBootMethod()
+        params = make_kernel_parameters(
+            purpose="enlist", arch="amd64")
+        options = {
+            "backend": None,
+            "kernel_params": params,
+            }
+        output = method.get_reader(**options).read(10000)
+        self.assertThat(output, ContainsAll(
+            [
+                "menuentry 'Enlist'",
+                "%s/%s/%s" % (params.osystem, params.arch, params.subarch),
+                "boot-kernel",
+            ]))
+
+    def test_get_reader_with_commissioning_purpose(self):
+        # If purpose is "commissioning", the config.commissioning.template
+        # should be used.
+        method = UEFIBootMethod()
+        params = make_kernel_parameters(
+            purpose="commissioning", arch="amd64")
+        options = {
+            "backend": None,
+            "kernel_params": params,
+            }
+        output = method.get_reader(**options).read(10000)
+        self.assertThat(output, ContainsAll(
+            [
+                "menuentry 'Commission'",
+                "%s/%s/%s" % (params.osystem, params.arch, params.subarch),
+                "boot-kernel",
+            ]))
+
 
 class TestUEFIBootMethodRegex(MAASTestCase):
     """Tests `provisioningserver.boot.uefi.UEFIBootMethod.re_config_file`."""