launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/enlistment-pxe-on-import into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/enlistment-pxe-on-import/+merge/111259

This is as discussed with Julian.  The new script that imports PXE images (i.e. kernels and initrds to make available to booting nodes over TFTP) should also generate the “default” PXE config for each architecture/subarchitecture.  This PXE config is used for auto-enlisting nodes.

There is no need to support multiple OS releases for enlistment images, so this always uses the current release.

One small complication is that the “maas” executable must be in the script's PATH.  Luckily for my tests, all invocations of the import script went through a single utility function that was easily patched.

As per pre-imp, the script extension (topmost in the diff) specifies the TFTP directory to write the new config file to.  This was needed because tests should be able to run the script without writing to the default /var/lib/tftpboot.  That path was in turn encapsulated in python that runs out-of-process, in the PXEConfig class, so that I couldn't circumvent it by patching PXEConfig inside the test process.  Instead I had to extend both PXEConfig and the generate-enlistment-pxe maas command to permit overriding of the path.

Testing revealed a subtle bug in PXEConfig: os.makedirs() fails if the directory it's supposed to create already exists.  It's not quite the equivalent of “mkdir -p” much as that would seem to make sense.  A separate new test covers this situation, and those changes in turn provoked the imports-formatting script into fixing some badly-formatted imports.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/enlistment-pxe-on-import/+merge/111259
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/enlistment-pxe-on-import into lp:maas.

=== modified file 'scripts/maas-import-pxe-files'
--- scripts/maas-import-pxe-files	2012-06-15 13:29:26 +0000
+++ scripts/maas-import-pxe-files	2012-06-20 17:09:31 +0000
@@ -182,6 +182,10 @@
             mkdir -p -- $rel_dir
             update_install_files $rel_dir $arch $release
         done
+
+        maas generate_enlistment_pxe \
+            --arch=$arch --release=$CURRENT_RELEASE \
+            --pxe-target-dir=$TFTPROOT/maas
     done
 
     rm -rf -- $DOWNLOAD_DIR

=== modified file 'src/maas/tests/test_maas_import_pxe_files.py'
--- src/maas/tests/test_maas_import_pxe_files.py	2012-06-15 12:49:02 +0000
+++ src/maas/tests/test_maas_import_pxe_files.py	2012-06-20 17:09:31 +0000
@@ -19,6 +19,7 @@
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
 from testtools.matchers import (
+    Contains,
     FileContains,
     FileExists,
     Not,
@@ -97,10 +98,17 @@
         TFTP root directory, respectively.  Both bad ideas.
         """
         script = './scripts/maas-import-pxe-files'
+        path = ':'.join([
+            os.path.join(
+                os.path.dirname(__file__), os.path.pardir, os.path.pardir,
+                os.path.pardir, 'bin'),
+            os.environ['PATH'],
+            ])
         env = {
             'ARCHIVE': 'file://%s' % archive_dir,
             # Substitute curl for wget; it accepts file:// URLs.
             'DOWNLOAD': 'curl -O --silent',
+            'PATH': path,
             'TFTPROOT': tftproot,
         }
         if arch is not None:
@@ -200,3 +208,14 @@
         original_timestamp = get_write_time(tftp_path)
         self.call_script(archive, tftproot, arch=arch, release=release)
         self.assertEqual(original_timestamp, get_write_time(tftp_path))
+
+    def test_generates_default_pxe_config(self):
+        arch = make_name('arch')
+        release = 'precise'
+        tftproot = self.make_dir()
+        archive = self.make_downloads(arch=arch, release=release)
+        self.call_script(archive, tftproot, arch=arch, release=release)
+        self.assertThat(
+            os.path.join(
+                tftproot, 'maas', arch, 'generic', 'pxelinux.cfg', 'default'),
+            FileContains(matcher=Contains("MENU TITLE")))

=== modified file 'src/maasserver/management/commands/generate_enlistment_pxe.py'
--- src/maasserver/management/commands/generate_enlistment_pxe.py	2012-06-19 16:05:44 +0000
+++ src/maasserver/management/commands/generate_enlistment_pxe.py	2012-06-20 17:09:31 +0000
@@ -39,9 +39,13 @@
         make_option(
             '--release', dest='release', default=None,
             help="Ubuntu release to run when enlisting nodes."),
+        make_option(
+            '--pxe-target-dir', dest='pxe_target_dir', default=None,
+            help="Write PXE config here instead of in its normal location."),
         )
 
-    def handle(self, arch=None, subarch='generic', release=None, **kwargs):
+    def handle(self, arch=None, subarch='generic', release=None,
+               pxe_target_dir=None, **kwargs):
         image_path = '/maas/%s/%s/%s/install' % (arch, subarch, release)
         # TODO: This needs to go somewhere more appropriate, and
         # probably contain more appropriate options.
@@ -65,4 +69,5 @@
             'kernelimage': '/'.join([image_path, 'linux']),
             'append': kernel_opts,
         }
-        PXEConfig(arch, subarch).write_config(**template_args)
+        writer = PXEConfig(arch, subarch, pxe_target_dir=pxe_target_dir)
+        writer.write_config(**template_args)

=== modified file 'src/maasserver/tests/test_commands_generate_enlistment_pxe.py'
--- src/maasserver/tests/test_commands_generate_enlistment_pxe.py	2012-06-19 16:05:44 +0000
+++ src/maasserver/tests/test_commands_generate_enlistment_pxe.py	2012-06-20 17:09:31 +0000
@@ -28,7 +28,9 @@
         release = 'precise'
         tftpdir = self.make_dir()
         self.patch(PXEConfig, 'target_basedir', tftpdir)
-        call_command('generate_enlistment_pxe', arch=arch, release=release)
+        call_command(
+            'generate_enlistment_pxe', arch=arch, release=release,
+            pxe_target_dir=tftpdir)
         # This produces a "default" PXE config file in the right place.
         # It refers to the kernel and initrd for the requested
         # architecture and release.

=== modified file 'src/provisioningserver/pxe/pxeconfig.py'
--- src/provisioningserver/pxe/pxeconfig.py	2012-06-19 09:08:34 +0000
+++ src/provisioningserver/pxe/pxeconfig.py	2012-06-20 17:09:31 +0000
@@ -17,12 +17,12 @@
 
 
 import os
-import tempita
 
 from celeryconfig import (
     PXE_TARGET_DIR,
     PXE_TEMPLATES_DIR,
     )
+import tempita
 
 
 class PXEConfigFail(Exception):
@@ -47,6 +47,11 @@
         aa:bb:cc:dd:ee:ff.  This is the default for MAC addresses coming
         from the database fields in MAAS, so it's not heavily checked here.
     :type mac: string
+    :param pxe_target_dir: Base directory to write PXE configurations to,
+        e.g.  /var/lib/tftpboot/maas (which is also the default).  The
+        config file will go into a directory determined by the architecture
+        that it's for: `<target_dir>/<arch>/<subarch>/pxelinux.cfg/`
+    :type pxe_target_dir: string
 
     :raises PXEConfigFail: if there's a problem with template parameters
         or the MAC address looks incorrectly formatted.
@@ -62,9 +67,12 @@
             append="initrd=blah url=blah")
     """
 
-    def __init__(self, arch, subarch=None, mac=None):
+    def __init__(self, arch, subarch=None, mac=None, pxe_target_dir=None):
         if subarch is None:
             subarch = "generic"
+        if pxe_target_dir is None:
+            pxe_target_dir = PXE_TARGET_DIR
+        self.target_basedir = pxe_target_dir
         self._validate_mac(mac)
         self.template = os.path.join(self.template_basedir, "maas.template")
         self.target_dir = os.path.join(
@@ -82,10 +90,6 @@
     def template_basedir(self):
         return PXE_TEMPLATES_DIR
 
-    @property
-    def target_basedir(self):
-        return PXE_TARGET_DIR
-
     def _validate_mac(self, mac):
         # A MAC address should be of the form aa:bb:cc:dd:ee:ff with
         # precisely five colons in it.  We do a cursory check since most
@@ -120,6 +124,7 @@
         """
         template = self.get_template()
         rendered = self.render_template(template, **kwargs)
-        os.makedirs(self.target_dir)
+        if not os.path.isdir(self.target_dir):
+            os.makedirs(self.target_dir)
         with open(self.target_file, "w") as f:
             f.write(rendered)

=== modified file 'src/provisioningserver/pxe/tests/test_pxeconfig.py'
--- src/provisioningserver/pxe/tests/test_pxeconfig.py	2012-06-19 09:08:34 +0000
+++ src/provisioningserver/pxe/tests/test_pxeconfig.py	2012-06-20 17:09:31 +0000
@@ -14,7 +14,6 @@
 
 import os
 import re
-import tempita
 
 from celeryconfig import (
     PXE_TARGET_DIR,
@@ -26,7 +25,9 @@
     PXEConfig,
     PXEConfigFail,
     )
+import tempita
 from testtools.matchers import (
+    Contains,
     FileContains,
     MatchesRegex,
     )
@@ -97,11 +98,10 @@
                 "name 'kernelimage' is not defined at line \d+ column \d+ "
                 "in file %s" % re.escape(template_name)))
 
-    def test_write_config(self):
+    def test_write_config_writes_config(self):
         # Ensure that a rendered template is written to the right place.
         out_dir = self.make_dir()
-        self.patch(PXEConfig, 'target_basedir', out_dir)
-        pxeconfig = PXEConfig("armhf", "armadaxp")
+        pxeconfig = PXEConfig("armhf", "armadaxp", pxe_target_dir=out_dir)
         pxeconfig.write_config(
             menutitle="menutitle", kernelimage="/my/kernel", append="append")
 
@@ -111,3 +111,16 @@
             append="append")
 
         self.assertThat(pxeconfig.target_file, FileContains(expected))
+
+    def test_write_config_overwrites_config(self):
+        out_dir = self.make_dir()
+        pxeconfig = PXEConfig("amd64", "generic", pxe_target_dir=out_dir)
+        pxeconfig.write_config(
+            menutitle="oldtitle", kernelimage="/old/kernel", append="append")
+        pxeconfig = PXEConfig("amd64", "generic", pxe_target_dir=out_dir)
+        pxeconfig.write_config(
+            menutitle="newtitle", kernelimage="/new/kernel", append="append")
+
+        self.assertThat(
+            pxeconfig.target_file,
+            FileContains(matcher=Contains('newtitle')))