launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/chain-booting into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1034198 in MAAS: "Local booting a node uses PXE's unreliable LOCALBOOT"
  https://bugs.launchpad.net/maas/+bug/1034198

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/chain-booting/+merge/119314

(Somewhat pre-imped with jtv.)

After previously adding the PXE template to do local booting, I found out that the LOCALBOOT directive is apparently unreliable on Intel hardware.

This branch uses the chain.32 loader to do local boot on Intel instead.  There's a few changes to support this:

 * maas-import-pxe-files uses the install_bootloader stuff to install the chain.32 file in the right place
 * chain.32 comes from the local file system and is installed by the syslinux package so this is now mentioned in the required packages.  [As a side note, we could also start getting pxelinux.0 from there instead of downloading it]
 * The install_bootloader stuff was a bit broken in that it always copied everything to a single file called pxelinux.0 - this is because compose_bootloader_path returns the file name not just its path!  However, since the way PXE works all links off the location of the pxelinux.0 file I chose not to amend that function and instead operate in the way that PXE does and hack the filename off the end and use its base directory as a destination to copy whatever file is given by args.loader.
 * There's a new template that uses chain.c32 and a fix to render_pxe_config to make use of it.
 * Finally, the existing localboot template was using an arg of -1 to LOCALBOOT.  This forces an error condition in the loader and makes the BIOS head to its next configured boot device - we want to *force* a local boot instead, so -1 is now 0 which does that.

-- 
https://code.launchpad.net/~julian-edwards/maas/chain-booting/+merge/119314
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/chain-booting into lp:maas.

=== modified file 'required-packages/base'
--- required-packages/base	2012-08-01 05:55:00 +0000
+++ required-packages/base	2012-08-13 08:56:21 +0000
@@ -30,4 +30,5 @@
 python-txamqp
 python-yaml
 rabbitmq-server
+syslinux
 tgt

=== modified file 'scripts/maas-import-pxe-files'
--- scripts/maas-import-pxe-files	2012-08-03 15:12:09 +0000
+++ scripts/maas-import-pxe-files	2012-08-13 08:56:21 +0000
@@ -75,6 +75,8 @@
         # TODO: Pass sub-architecture once we support those.
         maas-provision install-pxe-bootloader \
             --arch=$arch --loader='pxelinux.0'
+        maas-provision install-pxe-bootloader \
+            --arch=$arch --loader='/usr/lib/syslinux/chain.c32'
     fi
 }
 

=== added file 'src/provisioningserver/pxe/config.localboot-intel.template'
--- src/provisioningserver/pxe/config.localboot-intel.template	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/pxe/config.localboot-intel.template	2012-08-13 08:56:21 +0000
@@ -0,0 +1,9 @@
+DEFAULT local
+PROMPT 0
+TIMEOUT 0
+TOTALTIMEOUT 0
+ONTIMEOUT local
+
+LABEL local
+        KERNEL chain.c32
+

=== modified file 'src/provisioningserver/pxe/config.localboot.template'
--- src/provisioningserver/pxe/config.localboot.template	2012-08-07 06:39:44 +0000
+++ src/provisioningserver/pxe/config.localboot.template	2012-08-13 08:56:21 +0000
@@ -5,5 +5,4 @@
 
 LABEL execute
         SAY Booting local disk ...
-        LOCALBOOT -1
-
+        LOCALBOOT 0

=== modified file 'src/provisioningserver/pxe/config.py'
--- src/provisioningserver/pxe/config.py	2012-08-07 06:39:44 +0000
+++ src/provisioningserver/pxe/config.py	2012-08-13 08:56:21 +0000
@@ -32,6 +32,8 @@
 template_filename = path.join(template_dir, "config.template")
 template_localboot_filename = path.join(
     template_dir, "config.localboot.template")
+template_localboot_intel_filename = path.join(
+    template_dir, "config.localboot-intel.template")
 
 
 def render_pxe_config(
@@ -48,9 +50,15 @@
         parameters generated in another component (for example, see
         `TFTPBackend.get_config_reader`) won't cause this to break.
     """
+    # Templates are loaded each time here so that they can be changed on
+    # the fly without restarting the provisioning server.
     if purpose == "local":
-        template = tempita.Template.from_filename(
-            template_localboot_filename, encoding="UTF-8")
+        if arch in ("i386", "amd64"):
+            template = tempita.Template.from_filename(
+                template_localboot_intel_filename, encoding="UTF-8")
+        else:
+            template = tempita.Template.from_filename(
+                template_localboot_filename, encoding="UTF-8")
         # No params in local boot pxeconfig, but using a template anyway
         # in case we decide to do so in the future.
         return template.substitute()

=== modified file 'src/provisioningserver/pxe/install_bootloader.py'
--- src/provisioningserver/pxe/install_bootloader.py	2012-07-23 10:24:39 +0000
+++ src/provisioningserver/pxe/install_bootloader.py	2012-08-13 08:56:21 +0000
@@ -44,7 +44,7 @@
     directory = os.path.dirname(path)
     if not os.path.isdir(directory):
         os.makedirs(directory)
-    return path
+    return directory
 
 
 def are_identical_files(old, new):
@@ -103,11 +103,9 @@
     """Install a PXE pre-boot loader into the TFTP directory structure.
 
     This won't overwrite an existing loader if its contents are unchanged.
-    However the new loader you give it will be deleted regardless.
     """
     config = Config.load(args.config_file)
     tftproot = config["tftp"]["root"]
-    destination = make_destination(tftproot, args.arch, args.subarch)
+    destination_path = make_destination(tftproot, args.arch, args.subarch)
+    destination = os.path.join(destination_path, os.path.basename(args.loader))
     install_bootloader(args.loader, destination)
-    if os.path.exists(args.loader):
-        os.remove(args.loader)

=== modified file 'src/provisioningserver/pxe/tests/test_config.py'
--- src/provisioningserver/pxe/tests/test_config.py	2012-08-07 06:39:35 +0000
+++ src/provisioningserver/pxe/tests/test_config.py	2012-08-13 08:56:21 +0000
@@ -97,4 +97,34 @@
             "append": factory.make_name("append"),
             }
         output = render_pxe_config(**options)
-        self.assertIn("LOCALBOOT -1", output)
+        self.assertIn("LOCALBOOT 0", output)
+
+    def test_render_pxe_config_with_local_purpose_i386_arch(self):
+        # Intel i386 is a special case and needs to use the chain.c32
+        # loader as the LOCALBOOT PXE directive is unreliable.
+        options = {
+            "arch": "i386",
+            "subarch": factory.make_name("subarch"),
+            "release": factory.make_name("release"),
+            "purpose": "local",
+            "bootpath": factory.make_name("bootpath"),
+            "append": factory.make_name("append"),
+            }
+        output = render_pxe_config(**options)
+        self.assertIn("chain.c32", output)
+        self.assertNotIn("LOCALBOOT", output)
+
+    def test_render_pxe_config_with_local_purpose_amd64_arch(self):
+        # Intel amd64 is a special case and needs to use the chain.c32
+        # loader as the LOCALBOOT PXE directive is unreliable.
+        options = {
+            "arch": "amd64",
+            "subarch": factory.make_name("subarch"),
+            "release": factory.make_name("release"),
+            "purpose": "local",
+            "bootpath": factory.make_name("bootpath"),
+            "append": factory.make_name("append"),
+            }
+        output = render_pxe_config(**options)
+        self.assertIn("chain.c32", output)
+        self.assertNotIn("LOCALBOOT", output)

=== modified file 'src/provisioningserver/pxe/tests/test_install_bootloader.py'
--- src/provisioningserver/pxe/tests/test_install_bootloader.py	2012-07-23 13:25:15 +0000
+++ src/provisioningserver/pxe/tests/test_install_bootloader.py	2012-08-13 08:56:21 +0000
@@ -35,7 +35,6 @@
     DirExists,
     FileContains,
     FileExists,
-    Not,
     )
 
 
@@ -58,19 +57,20 @@
             ("--config-file", config_fixture.filename, action, "--arch", arch,
              "--subarch", subarch, "--loader", loader))
 
+        bootloader_filename = os.path.join(
+            os.path.dirname(compose_bootloader_path(arch, subarch)),
+            os.path.basename(loader))
         self.assertThat(
             locate_tftp_path(
-                compose_bootloader_path(arch, subarch),
-                tftproot=tftproot),
+                bootloader_filename, tftproot=tftproot),
             FileExists())
-        self.assertThat(loader, Not(FileExists()))
 
     def test_make_destination_creates_directory_if_not_present(self):
         tftproot = self.make_dir()
         arch = factory.make_name('arch')
         subarch = factory.make_name('subarch')
         dest = make_destination(tftproot, arch, subarch)
-        self.assertThat(os.path.dirname(dest), DirExists())
+        self.assertThat(dest, DirExists())
 
     def test_make_destination_returns_existing_directory(self):
         tftproot = self.make_dir()
@@ -78,7 +78,7 @@
         subarch = factory.make_name('subarch')
         make_destination(tftproot, arch, subarch)
         dest = make_destination(tftproot, arch, subarch)
-        self.assertThat(os.path.dirname(dest), DirExists())
+        self.assertThat(dest, DirExists())
 
     def test_install_bootloader_installs_new_bootloader(self):
         contents = factory.getRandomString()