launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/pxe-template-lookup into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/pxe-template-lookup/+merge/120155

This reorganises the PXE templates so that we can do a
most-specific-first lookup on them, based on boot purpose, arch and
subarch.

It also removes all the default values from the templates themselves,
and tidies up a little.

It introduces mock to help with testing. This is going to be shipped
as unittest.mock in Python 3.3 (it's already in the beta, so there's
no doubts there), so it seemed safe to add it in. Seems like a nice
tool.

The factory also grows a make_names() method. This is a lazy-hack so
that test setup can be marginally shorter and/or prettier.

This branch started out in order to reorganise the TFTP layout to boot
Intel machines correctly (w.r.t. 32/64 bit), but meandered a bit and
got big, hence why it's not the most focused change. It is safe to
land on its own though.

-- 
https://code.launchpad.net/~allenap/maas/pxe-template-lookup/+merge/120155
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/pxe-template-lookup into lp:maas.

=== modified file 'buildout.cfg'
--- buildout.cfg	2012-08-16 09:51:53 +0000
+++ buildout.cfg	2012-08-17 12:40:34 +0000
@@ -33,6 +33,7 @@
   fixtures
   lockfile
   lxml
+  mock
   netaddr
   nose
   nose-subunit

=== modified file 'setup.py'
--- setup.py	2012-08-15 12:54:49 +0000
+++ setup.py	2012-08-17 12:40:34 +0000
@@ -117,6 +117,7 @@
             'lxml',
             'sst',
             'fixtures',
+            'mock',
             'nose',
             'nose-subunit',
             'python-subunit',

=== modified file 'src/maastesting/factory.py'
--- src/maastesting/factory.py	2012-08-10 13:10:53 +0000
+++ src/maastesting/factory.py	2012-08-17 12:40:34 +0000
@@ -135,6 +135,16 @@
         return sep.join(
             filter(None, [prefix, self.getRandomString(size=size)]))
 
+    def make_names(self, *prefixes):
+        """Generate random names.
+
+        Yields a name for each prefix specified.
+
+        :param prefixes: Zero or more prefixes. See `make_name`.
+        """
+        for prefix in prefixes:
+            yield self.make_name(prefix)
+
 
 # Create factory singleton.
 factory = Factory()

=== modified file 'src/maastesting/tests/test_factory.py'
--- src/maastesting/tests/test_factory.py	2012-08-10 13:10:53 +0000
+++ src/maastesting/tests/test_factory.py	2012-08-17 12:40:34 +0000
@@ -139,3 +139,9 @@
         self.assertThat(
             factory.make_name(size=100),
             MatchesAll(*[Not(Contains(char)) for char in '/ \t\n\r\\']))
+
+    def test_make_names_calls_make_name_with_each_prefix(self):
+        self.patch(factory, "make_name", lambda prefix: prefix + "-xxx")
+        self.assertSequenceEqual(
+            ["abc-xxx", "def-xxx", "ghi-xxx"],
+            list(factory.make_names("abc", "def", "ghi")))

=== added symlink 'src/provisioningserver/pxe/config.local.amd64.template'
=== target is u'config.local.x86.template'
=== added symlink 'src/provisioningserver/pxe/config.local.i386.template'
=== target is u'config.local.x86.template'
=== renamed file 'src/provisioningserver/pxe/config.localboot.template' => 'src/provisioningserver/pxe/config.local.template'
--- src/provisioningserver/pxe/config.localboot.template	2012-08-13 08:54:29 +0000
+++ src/provisioningserver/pxe/config.local.template	2012-08-17 12:40:34 +0000
@@ -1,8 +1,5 @@
-DEFAULT execute
-TIMEOUT 0
-PROMPT 0
-
-
-LABEL execute
-        SAY Booting local disk ...
-        LOCALBOOT 0
+DEFAULT local
+
+LABEL local
+  SAY Booting local disk ...
+  LOCALBOOT 0

=== renamed file 'src/provisioningserver/pxe/config.localboot-intel.template' => 'src/provisioningserver/pxe/config.local.x86.template'
--- src/provisioningserver/pxe/config.localboot-intel.template	2012-08-13 08:11:11 +0000
+++ src/provisioningserver/pxe/config.local.x86.template	2012-08-17 12:40:34 +0000
@@ -1,9 +1,5 @@
 DEFAULT local
-PROMPT 0
-TIMEOUT 0
-TOTALTIMEOUT 0
-ONTIMEOUT local
 
 LABEL local
-        KERNEL chain.c32
-
+  SAY Booting local disk ...
+  KERNEL chain.c32

=== modified file 'src/provisioningserver/pxe/config.py'
--- src/provisioningserver/pxe/config.py	2012-08-13 08:21:52 +0000
+++ src/provisioningserver/pxe/config.py	2012-08-17 12:40:34 +0000
@@ -20,6 +20,7 @@
     ]
 
 
+from errno import ENOENT
 from functools import partial
 from os import path
 
@@ -27,13 +28,47 @@
 from provisioningserver.pxe.tftppath import compose_image_path
 import tempita
 
-
+# TODO: make this configurable.
 template_dir = path.dirname(__file__)
-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 gen_pxe_template_filenames(purpose, arch, subarch):
+    """List possible PXE template filenames.
+
+    :param purpose: The boot purpose, e.g. "local".
+    :param arch: Main machine architecture.
+    :param subarch: Sub-architecture, or "generic" if there is none.
+    :param release: The Ubuntu release to be used.
+
+    Returns a list of possible PXE template filenames using the following
+    lookup order:
+
+      config.{purpose}.{arch}.{subarch}.template
+      config.{purpose}.{arch}.template
+      config.{purpose}.template
+      config.template
+
+    """
+    elements = [purpose, arch, subarch]
+    while len(elements) >= 1:
+        yield "config.%s.template" % ".".join(elements)
+        elements.pop()
+    yield "config.template"
+
+
+def get_pxe_template(purpose, arch, subarch):
+    # Templates are loaded each time here so that they can be changed on
+    # the fly without restarting the provisioning server.
+    for filename in gen_pxe_template_filenames(purpose, arch, subarch):
+        try:
+            return tempita.Template.from_filename(
+                path.join(template_dir, filename), encoding="UTF-8")
+        except IOError as error:
+            if error.errno != ENOENT:
+                raise
+    else:
+        raise AssertionError(
+            "No PXE template found in %r!" % template_dir)
 
 
 def render_pxe_config(
@@ -50,21 +85,7 @@
         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":
-        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()
-
-    template = tempita.Template.from_filename(
-        template_filename, encoding="UTF-8")
+    template = get_pxe_template(purpose, arch, subarch)
     image_dir = compose_image_path(arch, subarch, release, purpose)
     # The locations of the kernel image and the initrd are defined by
     # update_install_files(), in scripts/maas-import-pxe-files.

=== modified file 'src/provisioningserver/pxe/config.template'
--- src/provisioningserver/pxe/config.template	2012-08-02 12:40:52 +0000
+++ src/provisioningserver/pxe/config.template	2012-08-17 12:40:34 +0000
@@ -3,8 +3,8 @@
 SAY Booting under MAAS direction in 20 seconds.
 
 LABEL execute
-        SAY Booting under MAAS direction...
-        KERNEL {{kernel|relpath}}
-        INITRD {{initrd|relpath}}
-        APPEND {{append}}
-        IPAPPEND 2
+  SAY Booting under MAAS direction...
+  KERNEL {{kernel|relpath}}
+  INITRD {{initrd|relpath}}
+  APPEND {{append}}
+  IPAPPEND 2

=== modified file 'src/provisioningserver/pxe/tests/test_config.py'
--- src/provisioningserver/pxe/tests/test_config.py	2012-08-13 08:54:29 +0000
+++ src/provisioningserver/pxe/tests/test_config.py	2012-08-17 12:40:34 +0000
@@ -12,11 +12,15 @@
 __metaclass__ = type
 __all__ = []
 
+import errno
+from os import path
 import re
 
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
+import mock
 import posixpath
+from provisioningserver.pxe import config
 from provisioningserver.pxe.config import render_pxe_config
 from provisioningserver.pxe.tftppath import compose_image_path
 from testtools.matchers import (
@@ -27,6 +31,70 @@
     )
 
 
+class TestFunctions(TestCase):
+    """Test for functions in `provisioningserver.pxe.config`."""
+
+    def test_gen_pxe_template_filenames(self):
+        purpose = factory.make_name("purpose")
+        arch, subarch = factory.make_names("arch", "subarch")
+        expected = [
+            "config.%s.%s.%s.template" % (purpose, arch, subarch),
+            "config.%s.%s.template" % (purpose, arch),
+            "config.%s.template" % (purpose, ),
+            "config.template",
+            ]
+        observed = config.gen_pxe_template_filenames(purpose, arch, subarch)
+        self.assertSequenceEqual(expected, list(observed))
+
+    @mock.patch("tempita.Template.from_filename")
+    @mock.patch.object(config, "gen_pxe_template_filenames")
+    def test_get_pxe_template(self, gen_filenames, from_filename):
+        purpose = factory.make_name("purpose")
+        arch, subarch = factory.make_names("arch", "subarch")
+        filename = factory.make_name("filename")
+        # Set up the mocks that we've patched in.
+        gen_filenames.return_value = [filename]
+        from_filename.return_value = mock.sentinel.template
+        # The template returned matches the return value above.
+        template = config.get_pxe_template(purpose, arch, subarch)
+        self.assertEqual(mock.sentinel.template, template)
+        # gen_pxe_template_filenames is called to obtain filenames.
+        gen_filenames.assert_called_once_with(purpose, arch, subarch)
+        # Tempita.from_filename is called with an absolute path derived from
+        # the filename returned from gen_pxe_template_filenames.
+        from_filename.assert_called_once_with(
+            path.join(config.template_dir, filename), encoding="UTF-8")
+
+    def test_get_pxe_template_gets_default(self):
+        # There will not be a template matching the following purpose, arch,
+        # and subarch, so get_pxe_template() returns the default template.
+        purpose = factory.make_name("purpose")
+        arch, subarch = factory.make_names("arch", "subarch")
+        template = config.get_pxe_template(purpose, arch, subarch)
+        self.assertEqual(
+            path.join(config.template_dir, "config.template"),
+            template.name)
+
+    @mock.patch.object(config, "gen_pxe_template_filenames")
+    def test_get_pxe_template_not_found(self, gen_filenames):
+        # It is a critical and unrecoverable error if the default PXE template
+        # is not found.
+        gen_filenames.return_value = []
+        self.assertRaises(
+            AssertionError, config.get_pxe_template,
+            *factory.make_names("purpose", "arch", "subarch"))
+
+    @mock.patch("tempita.Template.from_filename")
+    def test_get_pxe_templates_only_suppresses_ENOENT(self, from_filename):
+        # The IOError arising from trying to load a template that doesn't
+        # exist is suppressed, but other errors are not.
+        from_filename.side_effect = IOError()
+        from_filename.side_effect.errno = errno.EACCES
+        self.assertRaises(
+            IOError, config.get_pxe_template,
+            *factory.make_names("purpose", "arch", "subarch"))
+
+
 class TestRenderPXEConfig(TestCase):
     """Tests for `provisioningserver.pxe.config.render_pxe_config`."""
 

=== modified file 'versions.cfg'
--- versions.cfg	2012-08-16 07:24:47 +0000
+++ versions.cfg	2012-08-17 12:40:34 +0000
@@ -57,6 +57,7 @@
 ipython = 0.12
 # Bug 251 is problematic in 0.9.2.
 django-debug-toolbar = 0.9.1
+mock = 1.0b1
 
 [versions-auto]
 # Added by Buildout Versions at 2012-02-24 15:51:04.865203