Merge lp:~blake-rouse/maas/fix-bug-1300285 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Resolves issue where if a node does not provide an architecture type on dhcp request, or no other boot method is available for that architecture, the node still uses pxelinux.0 to boot.

[bug=1300285] Fixes generate-dhcp-config, making it output the PXEBootMethod bootloader in a else branch, instead of an if with an architecture type. This allows nodes that do not provide an architecture type on dhcp request, or emulate pxelinux.0 to boot correctly.

[Jeroen T. Vermeulen (jtv)]

I think I understand what this does now, but the commit message is a bit cryptic. It summarises the code changes, but it doesn't give me much of a high-level idea of what the change means. It's that intention that's essential for review and maintenance. Could you perhaps say something like "make the PXE bootloader the default bootloader in the DHCP config, for when the node does not specify its architecture"? The extra sentence you provide in the description but not in the commit message is actually much more suitable for the commit message.

Also, there is no need to duplicate anything between the commit message and the description. The description is for the merge proposal only, and should help facilitate review. The commit message goes into revision history, with automatically added tags for author, reviewer, and bug number. Which also means: no need to prefix the bug yourself!

Typos in config.py: "sepcial" and, in that same comment, missing punctuation. Could you be a bit more descriptive? Someone reading this code and encountering "the special case PXEBootLoader" may still have trouble finding what that refers to. Are there several PXEBootLoaders, and is one of them a special case? Or do you mean that the PXEBootLoader is a special case?

In compose_conditional_bootloader, the loop structure and the code after the loop are now connected through the pxe_method variable, and through the "continue" statement in the loop. Those are often things worth avoiding, because it will take future maintainers time to figure out and preserve the intended behaviour. Not much, but it adds up — and what's more, they may not be aware of the need.

So might I suggest removing the "continue" and moving the variable closer to its point of use:

    for name, method in BootMethodRegistry:
        # PXEBootMethod is special. No matter what the arch_octet is,
        # it's always the last in the conditional, with an else. So
        # no matter the architecture it will always try to use pxelinux
        # to boot that node.
        if name != "pxe":
            output += CONDITIONAL_BOOTLOADER.format(
                behaviour=next(behaviour), arch_octet=method.arch_octet,
                bootloader=method.bootloader_path).strip() + ' '

    # Now put the PXE bootloader at the end.
    pxe_method = BootMethodRegistry.get_item('pxe')
    if pxe_method is not None:
        output += PXE_BOOTLOADER.format(
            bootloader=pxe_method.bootloader_path).strip()

(Also note that the "is its" in the comment could do with a comma.)

Alternatively, is it actually a problem to mention the PXE bootloader twice? If not, maybe repetition isn't so bad. That would give us something like:

    for name, method in BootMethodRegistry:
        output += CONDITIONAL_BOOTLOADER.format(
            behaviour=next(behaviour), arch_octet=method.arch_octet,
            bootloader=method.bootloader_path).strip() + ' '

    # Fall back on the PXE bootloader if architecture is unknown.
    pxe_method = BootMethodRegistry.get_item('pxe')
    if pxe_method is not None:
        output += PXE_BOOTLOADER.format(
            bootloader=pxe_method.bootloader_path).strip()

I...

review: Approve

[Blake Rouse (blake-rouse)]

Fixed the commit message, removed the continue from the loop, cleaned up the code comment.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~blake-rouse/maas/fix-bug-1300285 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://security.ubuntu.com trusty-security Release
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-netifaces python-oauth python-oops python-oops-amqp python-oops-datedir-repo python-oops-twisted python-oops-wsgi ...

=== modified file 'src/provisioningserver/dhcp/config.py'
--- src/provisioningserver/dhcp/config.py	2014-03-28 15:17:34 +0000
+++ src/provisioningserver/dhcp/config.py	2014-04-01 13:29:08 +0000
@@ -38,6 +38,13 @@
        }}
 """
 
+# Used to generate the PXEBootLoader special case
+PXE_BOOTLOADER = """
+else {{
+          filename \"{bootloader}\";
+       }}
+"""
+
 
 class DHCPConfigError(Exception):
     """Exception raised for errors processing the DHCP config."""
@@ -46,10 +53,20 @@
 def compose_conditional_bootloader():
     output = ""
     behaviour = chain(["if"], repeat("elsif"))
-    for _, method in BootMethodRegistry:
-        output += CONDITIONAL_BOOTLOADER.format(
-            behaviour=next(behaviour), arch_octet=method.arch_octet,
-            bootloader=method.bootloader_path).strip() + ' '
+    for name, method in BootMethodRegistry:
+        if name != "pxe":
+            output += CONDITIONAL_BOOTLOADER.format(
+                behaviour=next(behaviour), arch_octet=method.arch_octet,
+                bootloader=method.bootloader_path).strip() + ' '
+
+    # The PXEBootMethod is used in an else statement for the generated
+    # dhcpd config. This ensures that a booting node that does not
+    # provide an architecture octet, or architectures that emulate
+    # pxelinux can still boot.
+    pxe_method = BootMethodRegistry.get_item('pxe')
+    if pxe_method is not None:
+        output += PXE_BOOTLOADER.format(
+            bootloader=pxe_method.bootloader_path).strip()
     return output.strip()
 
 

=== modified file 'src/provisioningserver/dhcp/tests/test_config.py'
--- src/provisioningserver/dhcp/tests/test_config.py	2014-03-28 15:17:34 +0000
+++ src/provisioningserver/dhcp/tests/test_config.py	2014-04-01 13:29:08 +0000
@@ -116,8 +116,11 @@
 
     def test_compose_conditional_bootloader(self):
         output = config.compose_conditional_bootloader()
-        for _, method in BootMethodRegistry:
-            self.assertThat(output, Contains(method.arch_octet))
+        for name, method in BootMethodRegistry:
+            if name == "pxe":
+                self.assertThat(output, Contains("else"))
+            else:
+                self.assertThat(output, Contains(method.arch_octet))
             self.assertThat(output, Contains(method.bootloader_path))
 
     def test_config_contains_compose_conditional_bootloader(self):