Merge lp:~newell-jensen/maas/fix-1596046 into lp:~maas-committers/maas/trunk

[Newell Jensen]

This branch fixes a regression for powerNV where the "b'ppc64el/" was getting hardcoded into the dhcpd.conf. Additionally, it adds `path` to GetBootConfig (new in MAAS 2.0) on the rack, which was left out and needed.

[Newell Jensen (newell-jensen)]

This was tested on powerNV hardware in 1ss. The nodes are now able to enlist, commission, and deploy with this fix.

[Gavin Panella (allenap)]

m.rpc.boot.get_config, the eventual receiver of the added "path"
argument, doesn't actually use it.

I'm about 95% sure that it should not be passed. The arguments to
GetBootConfig are those _derived_ from the TFTP request; a raw path
should not be amongst them.

I think the bug is that PowerNVBootMethod.get_params() should not be
returning a dict with a "path" key. What do you think?

review: Needs Information

[Newell Jensen (newell-jensen)]

After looking at it, I agree. I didn't originally write powernv.py but
when I looke through the other boot methods, I don't see them passing
"path" to the paraemters. Will change this and test again.

On Wed, Jun 29, 2016 at 11:41 AM, Gavin Panella <<email address hidden>
> wrote:

> Review: Needs Information
>
> m.rpc.boot.get_config, the eventual receiver of the added "path"
> argument, doesn't actually use it.
>
> I'm about 95% sure that it should not be passed. The arguments to
> GetBootConfig are those _derived_ from the TFTP request; a raw path
> should not be amongst them.
>
> I think the bug is that PowerNVBootMethod.get_params() should not be
> returning a dict with a "path" key. What do you think?
>
> --
> https://code.launchpad.net/~newell-jensen/maas/fix-1596046/+merge/298690
> You are the owner of lp:~newell-jensen/maas/fix-1596046.
>

[Newell Jensen (newell-jensen)]

Gavin,

So powerNV needs 'path' for its get_reader method. However, I was able to strip the need of modifying the RPC calls. Have another look. Thanks.

[Gavin Panella (allenap)]

This looks good but I suggest solving it by selecting arguments to pass to GetBootConfig instead of killing off errant keys.

PowerNV also trims the prefix in two different places, which seems wasteful, and confusing when reading the code.

I've fixed both of these in lp:~allenap/maas/newell-jensen--fix-1596046. If you pull that into your branch you can see what I've fixed in the last couple of revisions.

[Newell Jensen (newell-jensen)]

Gavin,

Merged your branch, tested it on the hardware, fixed the issues, and pushed again. Have a look again when you can, thanks.

[Gavin Panella (allenap)]

Looks good. I'm glad you figured out why it wasn't working. One comment to check out; it won't hold you up long.

review: Approve

=== modified file 'src/provisioningserver/boot/powernv.py'
--- src/provisioningserver/boot/powernv.py	2016-03-28 13:54:47 +0000
+++ src/provisioningserver/boot/powernv.py	2016-06-30 19:11:09 +0000
@@ -49,6 +49,20 @@
 re_config_file = re_config_file.encode("ascii")
 re_config_file = re.compile(re_config_file, re.VERBOSE)
 
+# Due to the "ppc64el" prefix all files requested from the client using
+# relative paths will have that prefix. Capturing the path after that prefix
+# will give us the correct path in the local tftp root on disk.
+re_other_file = r'''
+    # Optional leading slash(es).
+    ^/*
+    ppc64el           # PowerNV PXE prefix, set by dhcpd.
+    /
+    (?P<path>.+)      # Capture path.
+    $
+'''
+re_other_file = re_other_file.encode("ascii")
+re_other_file = re.compile(re_other_file, re.VERBOSE)
+
 
 def format_bootif(mac):
     """Formats a mac address into the BOOTIF format, expected by
@@ -65,15 +79,16 @@
     template_subdir = "pxe"
     bootloader_path = "pxelinux.0"
     arch_octet = "00:0E"
-    path_prefix = b"ppc64el/"
+    path_prefix = "ppc64el/"
 
     def get_params(self, backend, path):
         """Gets the matching parameters from the requested path."""
         match = re_config_file.match(path)
         if match is not None:
             return get_parameters(match)
-        if path.lstrip(b'/').startswith(self.path_prefix):
-            return {'path': path}
+        match = re_other_file.match(path)
+        if match is not None:
+            return get_parameters(match)
         return None
 
     def match_path(self, backend, path):
@@ -94,23 +109,22 @@
                 params['mac'] = mac
         return params
 
-    def get_reader(self, backend, kernel_params, **extra):
+    def get_reader(self, backend, kernel_params, mac=None, path=None, **extra):
         """Render a configuration file as a unicode string.
 
         :param backend: requesting backend
         :param kernel_params: An instance of `KernelParameters`.
+        :param path: Optional MAC address discovered by `match_path`.
+        :param path: Optional path discovered by `match_path`.
         :param extra: Allow for other arguments. This is a safety valve;
             parameters generated in another component (for example, see
             `TFTPBackend.get_config_reader`) won't cause this to break.
         """
-        # Due to the path prefix, all requested files from the client will
-        # contain that prefix. Removing the prefix from the path will return
-        # the correct path in the tftp root.
-        if 'path' in extra:
-            path = extra['path']
-            if path.startswith(self.path_prefix):
-                path = path[len(self.path_prefix):]
-            target_path = backend.base.descendant(path.split(b'/'))
+        if path is not None:
+            # This is a request for a static file, not a configuration file.
+            # The prefix was already trimmed by `match_path` so we need only
+            # return a FilesystemReader for `path` beneath the backend's base.
+            target_path = backend.base.descendant(path.split('/'))
             return FilesystemReader(target_path)
 
         # Return empty config for PowerNV local. PowerNV fails to
@@ -128,10 +142,8 @@
         # support the IPAPPEND pxelinux flag.
         def kernel_command(params):
             cmd_line = compose_kernel_command_line(params)
-            if 'mac' in extra:
-                mac = extra['mac']
-                mac = format_bootif(mac)
-                return '%s BOOTIF=%s' % (cmd_line, mac)
+            if mac is not None:
+                return '%s BOOTIF=%s' % (cmd_line, format_bootif(mac))
             return cmd_line
 
         namespace['kernel_command'] = kernel_command
 
=== modified file 'src/provisioningserver/boot/tests/test_powernv.py'
--- src/provisioningserver/boot/tests/test_powernv.py	2016-06-22 17:03:02 +0000
+++ src/provisioningserver/boot/tests/test_powernv.py	2016-06-30 19:11:09 +0000
@@ -5,7 +5,6 @@
 
 __all__ = []
 
-import os
 import re
 from unittest.mock import Mock
 
@@ -114,7 +113,7 @@
 
     def test_path_prefix(self):
         method = PowerNVBootMethod()
-        self.assertEqual(b'ppc64el/', method.path_prefix)
+        self.assertEqual('ppc64el/', method.path_prefix)
 
 
 class TestPowerNVBootMethodMatchPath(MAASTestCase):
@@ -150,7 +149,8 @@
         expected = {
             'arch': 'ppc64el',
             'mac': fake_mac,
-            'path': file_path,
+            # The "ppc64el/" prefix has been removed from the path.
+            'path': file_path.decode("utf-8")[8:],
             }
         self.assertEqual(expected, params)
 
@@ -242,41 +242,41 @@
 
 
 class TestPowerNVBootMethodPathPrefix(MAASTestCase):
-    """Tests for
-    `provisioningserver.boot.powernv.PowerNVBootMethod.get_reader`.
-    """
-
-    def test_get_reader_path_prefix(self):
+    """Tests for `provisioningserver.boot.powernv.PowerNVBootMethod`."""
+
+    def test_path_prefix_removed(self):
+        temp_dir = FilePath(self.make_dir())
+        backend = Mock(base=temp_dir)  # A `TFTPBackend`.
+
+        # Create a file in the backend's base directory.
         data = factory.make_string().encode("ascii")
-        temp_file = self.make_file(name="example", contents=data)
-        temp_dir = os.path.dirname(temp_file)
-        backend = Mock(base=FilePath(temp_dir))  # A `TFTPBackend`.
+        temp_file = temp_dir.child("example")
+        temp_file.setContent(data)
+
         method = PowerNVBootMethod()
-        options = {
-            'backend': backend,
-            'kernel_params': make_kernel_parameters(),
-            'path': b'ppc64el/example',
-        }
-        reader = method.get_reader(**options)
+        params = method.get_params(backend, b"ppc64el/example")
+        self.assertEqual({"path": "example"}, params)
+        reader = method.get_reader(backend, make_kernel_parameters(), **params)
         self.addCleanup(reader.finish)
         self.assertEqual(len(data), reader.size)
         self.assertEqual(data, reader.read(len(data)))
         self.assertEqual(b"", reader.read(1))
 
-    def test_get_reader_path_prefix_only_removes_first_occurrence(self):
+    def test_path_prefix_only_first_occurrence_removed(self):
+        temp_dir = FilePath(self.make_dir())
+        backend = Mock(base=temp_dir)  # A `TFTPBackend`.
+
+        # Create a file nested within a "ppc64el" directory.
         data = factory.make_string().encode("ascii")
-        temp_dir = self.make_dir()
-        temp_subdir = os.path.join(temp_dir, 'ppc64el')
-        os.mkdir(temp_subdir)
-        factory.make_file(temp_subdir, "example", data)
-        backend = Mock(base=FilePath(temp_dir))  # A `TFTPBackend`.
+        temp_subdir = temp_dir.child("ppc64el")
+        temp_subdir.createDirectory()
+        temp_file = temp_subdir.child("example")
+        temp_file.setContent(data)
+
         method = PowerNVBootMethod()
-        options = {
-            'backend': backend,
-            'kernel_params': make_kernel_parameters(),
-            'path': b'ppc64el/ppc64el/example',
-        }
-        reader = method.get_reader(**options)
+        params = method.get_params(backend, b"ppc64el/ppc64el/example")
+        self.assertEqual({"path": "ppc64el/example"}, params)
+        reader = method.get_reader(backend, make_kernel_parameters(), **params)
         self.addCleanup(reader.finish)
         self.assertEqual(len(data), reader.size)
         self.assertEqual(data, reader.read(len(data)))
 
=== modified file 'src/provisioningserver/pserv_services/tests/test_tftp.py'
--- src/provisioningserver/pserv_services/tests/test_tftp.py	2016-05-12 19:07:37 +0000
+++ src/provisioningserver/pserv_services/tests/test_tftp.py	2016-06-30 19:11:09 +0000
@@ -49,6 +49,7 @@
     UDPServer,
 )
 from provisioningserver.rpc.exceptions import BootConfigNoResponse
+from provisioningserver.rpc.region import GetBootConfig
 from provisioningserver.testing.boot_images import (
     make_boot_image_params,
     make_image,
@@ -504,6 +505,30 @@
         # ArchitectureRegistry is not stable, so we permit either here.
         self.assertIn(observed_params["arch"], ["armhf", "arm64"])
 
+    def test_get_kernel_params_filters_out_unnecessary_arguments(self):
+        params_okay = {
+            name.decode("ascii"): factory.make_name("value")
+            for name, _ in GetBootConfig.arguments
+        }
+        params_other = {
+            factory.make_name("name"): factory.make_name("value")
+            for _ in range(3)
+        }
+        params_all = params_okay.copy()
+        params_all.update(params_other)
+
+        client_service = Mock()
+        client = client_service.getClient.return_value
+        client.localIdent = params_okay["system_id"]
+        backend = TFTPBackend(self.make_dir(), client_service)
+        backend.fetcher = Mock()
+
+        backend.get_kernel_params(params_all)
+
+        self.assertThat(
+            backend.fetcher, MockCalledOnceWith(
+                client, GetBootConfig, **params_okay))
+
 
 class TestTFTPService(MAASTestCase):
 
 
=== modified file 'src/provisioningserver/pserv_services/tftp.py'
--- src/provisioningserver/pserv_services/tftp.py	2016-03-28 13:54:47 +0000
+++ src/provisioningserver/pserv_services/tftp.py	2016-06-30 19:11:09 +0000
@@ -204,6 +204,17 @@
             path requested.
         :return: A `KernelParameters` instance.
         """
+        # Extract from params only those arguments that GetBootConfig cares
+        # about; params is a context-like object and other stuff (too much?)
+        # gets in there.
+        arguments = (
+            name.decode("ascii")
+            for name, _ in GetBootConfig.arguments
+        )
+        params = {
+            name: params[name] for name in arguments
+            if name in params
+        }
         client = self.client_service.getClient()
         params["system_id"] = client.localIdent
         d = self.fetcher(client, GetBootConfig, **params)
@@ -223,9 +234,7 @@
             return boot_method.get_reader(
                 self, kernel_params=kernel_params, **params)
 
-        d = self.get_kernel_params(params)
-        d.addCallback(generate)
-        return d
+        return self.get_kernel_params(params).addCallback(generate)
 
     @staticmethod
     def no_response_errback(failure, file_name):