Merge lp:~jtv/maas/bug-1041318 into lp:maas/trunk

[Jeroen T. Vermeulen]

Allow PXE configs to come from TFTP paths starting at “pxelinux.cfg”

This may help Andres with his upgrade Q/A. He is not running MAAS's DHCP server, and so PXE clients are directed to download “pxelinux.0” (at the TFTP root, not in the “maas” directory).

That works for pxelinux.0 itself (you get the version pxelinux-common installs), but not for the config files. But it looks like the “maas” directory has been more or less abstracted away anyawy, so in this branch I make it optional. It's mostly just a small change to the regex in the TFTPBackend. That will still leave multi-architecture problems, but at least it will support i386 smoothly as a default.

I also added some missing negative tests for the regex. There were no tests to establish that the config-path regex was not matching things it shouldn't.

Jeroen

[Gavin Panella (allenap)]

> This may help Andres with his upgrade Q/A. He is not running MAAS's
> DHCP server, and so PXE clients are directed to download
> “pxelinux.0” (at the TFTP root, not in the “maas” directory).

Do you have any insight into why this is happening? Is is not possible
to configure the boot filename on Andres' DHCP server? This has a
bearing on our not-managing-DHCP story.

Should we get rid of the bootpath altogether, instead of making it
optional, or does it still serve a purpose? I think its original
purpose was so that MAAS's boot files could coexist alongside other
resources served by the TFTP server. Now that MAAS has a dedicated
server this is largely irrelevant.

review: Approve

[Gavin Panella (allenap)]

Some context that I was previously unaware of: Andres has said that
this is a problem for upgrades only, where users are running their own
DHCP servers. Their settings for MAAS in 12.04 will not work with the
current versions. This branch smooths the upgrade.

The question still stands about removing the maas/ prefix entirely.

[Julian Edwards (julian-edwards)]

On Friday 24 August 2012 22:04:19 Gavin Panella wrote:
> Some context that I was previously unaware of: Andres has said that
> this is a problem for upgrades only, where users are running their own
> DHCP servers. Their settings for MAAS in 12.04 will not work with the
> current versions. This branch smooths the upgrade.
>
> The question still stands about removing the maas/ prefix entirely.

It doesn't make a lot of sense to keep it, given that it's now optional.

=== modified file 'src/provisioningserver/pxe/config.py'
--- src/provisioningserver/pxe/config.py	2012-08-23 22:45:54 +0000
+++ src/provisioningserver/pxe/config.py	2012-08-24 19:22:22 +0000
@@ -80,6 +80,9 @@
         parameters generated in another component (for example, see
         `TFTPBackend.get_config_reader`) won't cause this to break.
     """
+    if bootpath is None:
+        bootpath = ''
+
     template = get_pxe_template(
         kernel_params.purpose, kernel_params.arch,
         kernel_params.subarch)

=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py	2012-08-24 16:23:15 +0000
+++ src/provisioningserver/tests/test_tftp.py	2012-08-24 19:22:22 +0000
@@ -23,7 +23,6 @@
 
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
-import mock
 from provisioningserver.pxe.tftppath import compose_config_path
 from provisioningserver.tests.test_kernel_opts import make_kernel_parameters
 from provisioningserver.tftp import (
@@ -78,7 +77,7 @@
         config_path = compose_config_path(components["mac"])
         return config_path, components
 
-    def test_re_config_file(self):
+    def test_re_config_file_is_compatible_with_config_path_generator(self):
         # The regular expression for extracting components of the file path is
         # compatible with the PXE config path generator.
         regex = TFTPBackend.re_config_file
@@ -110,6 +109,32 @@
         self.assertIsNotNone(match, config_path)
         self.assertEqual(args, match.groupdict())
 
+    def test_re_config_file_matches_classic_pxelinux_cfg(self):
+        # The default config path is simply "pxelinux.cfg" (without
+        # leading slash).  The regex matches this.
+        mac = 'aa-bb-cc-dd-ee-ff'
+        match = TFTPBackend.re_config_file.match('pxelinux.cfg/01-%s' % mac)
+        self.assertIsNotNone(match)
+        self.assertEqual({'mac': mac, 'bootpath': None}, match.groupdict())
+
+    def test_re_config_file_matches_pxelinux_cfg_with_leading_slash(self):
+        mac = 'aa-bb-cc-dd-ee-ff'
+        match = TFTPBackend.re_config_file.match('/pxelinux.cfg/01-%s' % mac)
+        self.assertIsNotNone(match)
+        self.assertEqual({'mac': mac, 'bootpath': None}, match.groupdict())
+
+    def test_re_config_file_does_not_match_non_config_file(self):
+        self.assertIsNone(
+            TFTPBackend.re_config_file.match('maas/pxelinux.cfg/kernel'))
+
+    def test_re_config_file_does_not_match_file_in_root(self):
+        self.assertIsNone(
+            TFTPBackend.re_config_file.match('01-aa-bb-cc-dd-ee-ff'))
+
+    def test_re_config_file_does_not_match_file_not_in_pxelinux_cfg(self):
+        self.assertIsNone(
+            TFTPBackend.re_config_file.match('foo/01-aa-bb-cc-dd-ee-ff'))
+
 
 class TestTFTPBackend(TestCase):
     """Tests for `provisioningserver.tftp.TFTPBackend`."""

=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2012-08-23 21:31:46 +0000
+++ src/provisioningserver/tftp.py	2012-08-24 19:22:22 +0000
@@ -85,11 +85,15 @@
     # always Ethernet.
     re_config_file = re.compile(
         r'''
+        # Optional leading slash(es).
         ^/?
-        (?P<bootpath>
-          maas     # Static namespacing.
-        )    # Capture boot directory.
-        /
+        # Optional boot path prefix (separated from the rest by slash):
+        (?:
+            (?P<bootpath>
+                maas
+            )
+            /
+        )?
         pxelinux[.]cfg    # PXELINUX expects this.
         /
         {htype:02x}    # ARP HTYPE.
@@ -170,7 +174,7 @@
         """See `IBackend.get_reader()`.
 
         If `file_name` matches `re_config_file` then the response is obtained
-        from a remote server. Otherwise the filesystem is used to service the
+        from a server. Otherwise the filesystem is used to service the
         response.
         """
         config_file_match = self.re_config_file.match(file_name)