Merge lp:~blake-rouse/maas/tftp-backend-readers-1.5 into lp:maas/1.5

[Blake Rouse]

Change BootMethods to return their own IReader per-request, update method names to reflect new usage.

Backport: https://code.launchpad.net/~blake-rouse/maas/tftp-backend-readers/+merge/221138

[Blake Rouse (blake-rouse)]

review: Approve

=== modified file 'src/provisioningserver/boot/__init__.py'
--- src/provisioningserver/boot/__init__.py	2014-05-23 16:32:07 +0000
+++ src/provisioningserver/boot/__init__.py	2014-05-28 13:35:49 +0000
@@ -23,6 +23,7 @@
     abstractproperty,
     )
 from errno import ENOENT
+from io import BytesIO
 from os import path
 
 from provisioningserver.boot.tftppath import compose_image_path
@@ -30,6 +31,23 @@
 from provisioningserver.utils import locate_config
 from provisioningserver.utils.registry import Registry
 import tempita
+from tftp.backend import IReader
+from zope.interface import implementer
+
+
+@implementer(IReader)
+class BytesReader:
+
+    def __init__(self, data):
+        super(BytesReader, self).__init__()
+        self.buffer = BytesIO(data)
+        self.size = len(data)
+
+    def read(self, size):
+        return self.buffer.read(size)
+
+    def finish(self):
+        self.buffer.close()
 
 
 class BootMethodError(Exception):
@@ -100,22 +118,24 @@
         """
 
     @abstractmethod
-    def match_config_path(self, path):
-        """Checks path for the configuration file that needs to be
-        generated.
+    def match_path(self, backend, path):
+        """Checks path for a file the boot method needs to handle.
 
+        :param backend: requesting backend
         :param path: requested path
         :returns: dict of match params from path, None if no match
         """
 
     @abstractmethod
-    def render_config(self, kernel_params, **extra):
-        """Render a configuration file as a unicode string.
+    def get_reader(self, backend, kernel_params, **extra):
+        """Gets the reader the backend will use for this combination of
+        boot method, kernel parameters, and extra parameters.
 
+        :param backend: requesting backend
         :param kernel_params: An instance of `KernelParameters`.
         :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.
+            `TFTPBackend.get_boot_method_reader`) won't cause this to break.
         """
 
     @abstractmethod

=== modified file 'src/provisioningserver/boot/powerkvm.py'
--- src/provisioningserver/boot/powerkvm.py	2014-05-23 16:32:07 +0000
+++ src/provisioningserver/boot/powerkvm.py	2014-05-28 13:35:49 +0000
@@ -45,17 +45,17 @@
     bootloader_path = "bootppc64.bin"
     arch_octet = "00:0C"
 
-    def match_config_path(self, path):
+    def match_path(self, backend, path):
         """Doesn't need to do anything, as the UEFIBootMethod provides
         the grub implementation needed.
         """
         return None
 
-    def render_config(self, kernel_params, **extra):
+    def get_reader(self, backend, kernel_params, **extra):
         """Doesn't need to do anything, as the UEFIBootMethod provides
         the grub implementation needed.
         """
-        return ""
+        return None
 
     def install_bootloader(self, destination):
         """Installs the required files for PowerKVM booting into the

=== modified file 'src/provisioningserver/boot/pxe.py'
--- src/provisioningserver/boot/pxe.py	2014-03-28 04:06:27 +0000
+++ src/provisioningserver/boot/pxe.py	2014-05-28 13:35:49 +0000
@@ -22,6 +22,7 @@
 
 from provisioningserver.boot import (
     BootMethod,
+    BytesReader,
     get_parameters,
     )
 from provisioningserver.boot.install_bootloader import install_bootloader
@@ -78,10 +79,11 @@
     bootloader_path = "pxelinux.0"
     arch_octet = "00:00"
 
-    def match_config_path(self, path):
+    def match_path(self, backend, path):
         """Checks path for the configuration file that needs to be
         generated.
 
+        :param backend: requesting backend
         :param path: requested path
         :returns: dict of match params from path, None if no match
         """
@@ -90,19 +92,20 @@
             return None
         return get_parameters(match)
 
-    def render_config(self, kernel_params, **extra):
+    def get_reader(self, backend, kernel_params, **extra):
         """Render a configuration file as a unicode string.
 
+        :param backend: requesting backend
         :param kernel_params: An instance of `KernelParameters`.
         :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.
+            `TFTPBackend.get_boot_method_reader`) won't cause this to break.
         """
         template = self.get_template(
             kernel_params.purpose, kernel_params.arch,
             kernel_params.subarch)
         namespace = self.compose_template_namespace(kernel_params)
-        return template.substitute(namespace)
+        return BytesReader(template.substitute(namespace).encode("utf-8"))
 
     def install_bootloader(self, destination):
         """Installs the required files for PXE booting into the

=== modified file 'src/provisioningserver/boot/tests/test_boot.py'
--- src/provisioningserver/boot/tests/test_boot.py	2014-03-21 19:01:40 +0000
+++ src/provisioningserver/boot/tests/test_boot.py	2014-05-28 13:35:49 +0000
@@ -24,6 +24,7 @@
 from provisioningserver import boot
 from provisioningserver.boot import (
     BootMethod,
+    BytesReader,
     gen_template_filenames,
     )
 import tempita
@@ -36,11 +37,11 @@
     bootloader_path = "fake.efi"
     arch_octet = "00:00"
 
-    def match_config_path(self, path):
+    def match_path(self, backend, path):
         return {}
 
-    def render_config(kernel_params, **extra):
-        return ""
+    def get_reader(backend, kernel_params, **extra):
+        return BytesReader("")
 
     def install_bootloader():
         pass

=== modified file 'src/provisioningserver/boot/tests/test_powerkvm.py'
--- src/provisioningserver/boot/tests/test_powerkvm.py	2014-05-23 16:32:07 +0000
+++ src/provisioningserver/boot/tests/test_powerkvm.py	2014-05-28 13:35:49 +0000
@@ -35,17 +35,17 @@
 class TestPowerKVMBootMethod(MAASTestCase):
     """Tests `provisioningserver.boot.powerkvm.PowerKVMBootMethod`."""
 
-    def test_match_config_path_returns_none(self):
+    def test_match_path_returns_None(self):
         method = PowerKVMBootMethod()
         paths = [factory.getRandomString() for _ in range(3)]
         for path in paths:
-            self.assertEqual(None, method.match_config_path(path))
+            self.assertEqual(None, method.match_path(None, path))
 
-    def test_render_config_returns_empty_string(self):
+    def test_get_reader_returns_None(self):
         method = PowerKVMBootMethod()
         params = [make_kernel_parameters() for _ in range(3)]
         for param in params:
-            self.assertEqual("", method.render_config(params))
+            self.assertEqual(None, method.get_reader(None, params))
 
     def test_install_bootloader_get_package_raises_error(self):
         method = PowerKVMBootMethod()

=== modified file 'src/provisioningserver/boot/tests/test_pxe.py'
--- src/provisioningserver/boot/tests/test_pxe.py	2014-03-28 04:31:32 +0000
+++ src/provisioningserver/boot/tests/test_pxe.py	2014-05-28 13:35:49 +0000
@@ -20,6 +20,7 @@
 from maastesting.factory import factory
 from maastesting.testcase import MAASTestCase
 from provisioningserver import kernel_opts
+from provisioningserver.boot import BytesReader
 from provisioningserver.boot.pxe import (
     ARP_HTYPE,
     PXEBootMethod,
@@ -150,14 +151,15 @@
 class TestPXEBootMethodRenderConfig(MAASTestCase):
     """Tests for `provisioningserver.boot.pxe.PXEBootMethod.render_config`."""
 
-    def test_render_install(self):
+    def test_get_reader_install(self):
         # Given the right configuration options, the PXE configuration is
         # correctly rendered.
         method = PXEBootMethod()
         params = make_kernel_parameters(self, purpose="install")
-        output = method.render_config(kernel_params=params)
-        # The output is always a Unicode string.
-        self.assertThat(output, IsInstance(unicode))
+        output = method.get_reader(backend=None, kernel_params=params)
+        # The output is a BytesReader.
+        self.assertThat(output, IsInstance(BytesReader))
+        output = output.read(10000)
         # The template has rendered without error. PXELINUX configurations
         # typically start with a DEFAULT line.
         self.assertThat(output, StartsWith("DEFAULT "))
@@ -177,54 +179,58 @@
                     r'.*^\s+APPEND .+?$',
                     re.MULTILINE | re.DOTALL)))
 
-    def test_render_with_extra_arguments_does_not_affect_output(self):
-        # render_config() allows any keyword arguments as a safety valve.
+    def test_get_reader_with_extra_arguments_does_not_affect_output(self):
+        # get_reader() allows any keyword arguments as a safety valve.
         method = PXEBootMethod()
         options = {
+            "backend": None,
             "kernel_params": make_kernel_parameters(self, purpose="install"),
         }
         # Capture the output before sprinking in some random options.
-        output_before = method.render_config(**options)
+        output_before = method.get_reader(**options).read(10000)
         # Sprinkle some magic in.
         options.update(
             (factory.make_name("name"), factory.make_name("value"))
             for _ in range(10))
         # Capture the output after sprinking in some random options.
-        output_after = method.render_config(**options)
+        output_after = method.get_reader(**options).read(10000)
         # The generated template is the same.
         self.assertEqual(output_before, output_after)
 
-    def test_render_config_with_local_purpose(self):
+    def test_get_reader_with_local_purpose(self):
         # If purpose is "local", the config.localboot.template should be
         # used.
         method = PXEBootMethod()
         options = {
+            "backend": None,
             "kernel_params": make_kernel_parameters(purpose="local"),
             }
-        output = method.render_config(**options)
+        output = method.get_reader(**options).read(10000)
         self.assertIn("LOCALBOOT 0", output)
 
-    def test_render_config_with_local_purpose_i386_arch(self):
+    def test_get_reader_with_local_purpose_i386_arch(self):
         # Intel i386 is a special case and needs to use the chain.c32
         # loader as the LOCALBOOT PXE directive is unreliable.
         method = PXEBootMethod()
         options = {
+            "backend": None,
             "kernel_params": make_kernel_parameters(
                 arch="i386", purpose="local"),
         }
-        output = method.render_config(**options)
+        output = method.get_reader(**options).read(10000)
         self.assertIn("chain.c32", output)
         self.assertNotIn("LOCALBOOT", output)
 
-    def test_render_config_with_local_purpose_amd64_arch(self):
+    def test_get_reader_with_local_purpose_amd64_arch(self):
         # Intel amd64 is a special case and needs to use the chain.c32
         # loader as the LOCALBOOT PXE directive is unreliable.
         method = PXEBootMethod()
         options = {
+            "backend": None,
             "kernel_params": make_kernel_parameters(
                 arch="amd64", purpose="local"),
         }
-        output = method.render_config(**options)
+        output = method.get_reader(**options).read(10000)
         self.assertIn("chain.c32", output)
         self.assertNotIn("LOCALBOOT", output)
 
@@ -237,17 +243,18 @@
         ("xinstall", dict(purpose="xinstall")),
         ]
 
-    def test_render_config_scenarios(self):
+    def test_get_reader_scenarios(self):
         # The commissioning config uses an extra PXELINUX module to auto
         # select between i386 and amd64.
         method = PXEBootMethod()
         get_ephemeral_name = self.patch(kernel_opts, "get_ephemeral_name")
         get_ephemeral_name.return_value = factory.make_name("ephemeral")
         options = {
+            "backend": None,
             "kernel_params": make_kernel_parameters(
                 testcase=self, subarch="generic", purpose=self.purpose),
         }
-        output = method.render_config(**options)
+        output = method.get_reader(**options).read(10000)
         config = parse_pxe_config(output)
         # The default section is defined.
         default_section_label = config.header["DEFAULT"]

=== modified file 'src/provisioningserver/boot/tests/test_uefi.py'
--- src/provisioningserver/boot/tests/test_uefi.py	2014-05-15 17:39:27 +0000
+++ src/provisioningserver/boot/tests/test_uefi.py	2014-05-28 13:35:49 +0000
@@ -18,6 +18,7 @@
 
 from maastesting.factory import factory
 from maastesting.testcase import MAASTestCase
+from provisioningserver.boot import BytesReader
 from provisioningserver.boot.tftppath import compose_image_path
 from provisioningserver.boot.uefi import (
     re_config_file,
@@ -60,14 +61,15 @@
 class TestRenderUEFIConfig(MAASTestCase):
     """Tests for `provisioningserver.boot.uefi.UEFIBootMethod`."""
 
-    def test_render(self):
+    def test_get_reader(self):
         # Given the right configuration options, the UEFI configuration is
         # correctly rendered.
         method = UEFIBootMethod()
         params = make_kernel_parameters(purpose="install")
-        output = method.render_config(kernel_params=params)
-        # The output is always a Unicode string.
-        self.assertThat(output, IsInstance(unicode))
+        output = method.get_reader(backend=None, kernel_params=params)
+        # The output is a BytesReader.
+        self.assertThat(output, IsInstance(BytesReader))
+        output = output.read(10000)
         # The template has rendered without error. UEFI configurations
         # typically start with a DEFAULT line.
         self.assertThat(output, StartsWith("set default=\"0\""))
@@ -85,32 +87,34 @@
                     r'.*^\s+initrd %s/di-initrd$' % re.escape(image_dir),
                     re.MULTILINE | re.DOTALL)))
 
-    def test_render_with_extra_arguments_does_not_affect_output(self):
-        # render_config() allows any keyword arguments as a safety valve.
+    def test_get_reader_with_extra_arguments_does_not_affect_output(self):
+        # get_reader() allows any keyword arguments as a safety valve.
         method = UEFIBootMethod()
         options = {
+            "backend": None,
             "kernel_params": make_kernel_parameters(purpose="install"),
         }
         # Capture the output before sprinking in some random options.
-        output_before = method.render_config(**options)
+        output_before = method.get_reader(**options).read(10000)
         # Sprinkle some magic in.
         options.update(
             (factory.make_name("name"), factory.make_name("value"))
             for _ in range(10))
         # Capture the output after sprinking in some random options.
-        output_after = method.render_config(**options)
+        output_after = method.get_reader(**options).read(10000)
         # The generated template is the same.
         self.assertEqual(output_before, output_after)
 
-    def test_render_config_with_local_purpose(self):
+    def test_get_reader_with_local_purpose(self):
         # If purpose is "local", the config.localboot.template should be
         # used.
         method = UEFIBootMethod()
         options = {
+            "backend": None,
             "kernel_params": make_kernel_parameters(
                 purpose="local", arch="amd64"),
             }
-        output = method.render_config(**options)
+        output = method.get_reader(**options).read(10000)
         self.assertIn("configfile /efi/ubuntu/grub.cfg", output)
 
 

=== modified file 'src/provisioningserver/boot/uefi.py'
--- src/provisioningserver/boot/uefi.py	2014-03-28 04:06:27 +0000
+++ src/provisioningserver/boot/uefi.py	2014-05-28 13:35:49 +0000
@@ -25,6 +25,7 @@
 from provisioningserver.boot import (
     BootMethod,
     BootMethodInstallError,
+    BytesReader,
     get_parameters,
     utils,
     )
@@ -120,10 +121,11 @@
     bootloader_path = "bootx64.efi"
     arch_octet = "00:07"  # AMD64 EFI
 
-    def match_config_path(self, path):
+    def match_path(self, backend, path):
         """Checks path for the configuration file that needs to be
         generated.
 
+        :param backend: requesting backend
         :param path: requested path
         :returns: dict of match params from path, None if no match
         """
@@ -139,19 +141,20 @@
 
         return params
 
-    def render_config(self, kernel_params, **extra):
+    def get_reader(self, backend, kernel_params, **extra):
         """Render a configuration file as a unicode string.
 
+        :param backend: requesting backend
         :param kernel_params: An instance of `KernelParameters`.
         :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.
+            `TFTPBackend.get_boot_method_reader`) won't cause this to break.
         """
         template = self.get_template(
             kernel_params.purpose, kernel_params.arch,
             kernel_params.subarch)
         namespace = self.compose_template_namespace(kernel_params)
-        return template.substitute(namespace)
+        return BytesReader(template.substitute(namespace).encode("utf-8"))
 
     def install_bootloader(self, destination):
         """Installs the required files for UEFI booting into the

=== modified file 'src/provisioningserver/tests/test_tftp.py'
--- src/provisioningserver/tests/test_tftp.py	2014-03-28 06:51:59 +0000
+++ src/provisioningserver/tests/test_tftp.py	2014-05-28 13:35:49 +0000
@@ -28,11 +28,11 @@
 from maastesting.testcase import MAASTestCase
 import mock
 from provisioningserver import tftp as tftp_module
+from provisioningserver.boot import BytesReader
 from provisioningserver.boot.pxe import PXEBootMethod
 from provisioningserver.boot.tests.test_pxe import compose_config_path
 from provisioningserver.tests.test_kernel_opts import make_kernel_parameters
 from provisioningserver.tftp import (
-    BytesReader,
     TFTPBackend,
     TFTPService,
     )
@@ -127,9 +127,9 @@
         self.assertEqual(b"", reader.read(1))
 
     @inlineCallbacks
-    def test_get_reader_config_file(self):
-        # For paths matching re_config_file, TFTPBackend.get_reader() returns
-        # a Deferred that will yield a BytesReader.
+    def test_get_render_file(self):
+        # For paths matching PXEBootMethod.match_path, TFTPBackend.get_reader()
+        # returns a Deferred that will yield a BytesReader.
         cluster_uuid = factory.getRandomUUID()
         self.patch(tftp_module, 'get_cluster_uuid').return_value = (
             cluster_uuid)
@@ -147,8 +147,8 @@
                 factory.getRandomPort()),
             }
 
-        @partial(self.patch, backend, "get_config_reader")
-        def get_config_reader(boot_method, params):
+        @partial(self.patch, backend, "get_boot_method_reader")
+        def get_boot_method_reader(boot_method, params):
             params_json = json.dumps(params)
             params_json_reader = BytesReader(params_json)
             return succeed(params_json_reader)
@@ -168,9 +168,10 @@
         self.assertEqual(expected_params, observed_params)
 
     @inlineCallbacks
-    def test_get_config_reader_returns_rendered_params(self):
-        # get_config_reader() takes a dict() of parameters and returns an
-        # `IReader` of a PXE configuration, rendered by `render_pxe_config`.
+    def test_get_boot_method_reader_returns_rendered_params(self):
+        # get_boot_method_reader() takes a dict() of parameters and returns an
+        # `IReader` of a PXE configuration, rendered by
+        # `PXEBootMethod.get_reader`.
         backend = TFTPBackend(self.make_dir(), b"http://example.com/")
         # Fake configuration parameters, as discovered from the file path.
         fake_params = {"mac": factory.getRandomMACAddress("-")}
@@ -182,15 +183,15 @@
         get_page_patch = self.patch(backend, "get_page")
         get_page_patch.return_value = succeed(fake_get_page_result)
 
-        # Stub render_config to return the render parameters.
+        # Stub get_reader to return the render parameters.
         method = PXEBootMethod()
-        fake_render_result = factory.make_name("render")
-        render_patch = self.patch(method, "render_config")
-        render_patch.return_value = fake_render_result
+        fake_render_result = factory.make_name("render").encode("utf-8")
+        render_patch = self.patch(method, "get_reader")
+        render_patch.return_value = BytesReader(fake_render_result)
 
         # Get the rendered configuration, which will actually be a JSON dump
         # of the render-time parameters.
-        reader = yield backend.get_config_reader(method, fake_params)
+        reader = yield backend.get_boot_method_reader(method, fake_params)
         self.addCleanup(reader.finish)
         self.assertIsInstance(reader, BytesReader)
         output = reader.read(10000)
@@ -198,13 +199,13 @@
         # The kernel parameters were fetched using `backend.get_page`.
         self.assertThat(backend.get_page, MockCalledOnceWith(mock.ANY))
 
-        # The result has been rendered by `backend.render_config`.
+        # The result has been rendered by `method.get_reader`.
         self.assertEqual(fake_render_result.encode("utf-8"), output)
-        self.assertThat(method.render_config, MockCalledOnceWith(
-            kernel_params=fake_kernel_params, **fake_params))
+        self.assertThat(method.get_reader, MockCalledOnceWith(
+            backend, kernel_params=fake_kernel_params, **fake_params))
 
     @inlineCallbacks
-    def test_get_config_reader_substitutes_armhf_in_params(self):
+    def test_get_boot_method_render_substitutes_armhf_in_params(self):
         # get_config_reader() should substitute "arm" for "armhf" in the
         # arch field of the parameters (mapping from pxe to maas
         # namespace).
@@ -224,8 +225,8 @@
                 factory.getRandomPort()),
             }
 
-        @partial(self.patch, backend, "get_config_reader")
-        def get_config_reader(boot_method, params):
+        @partial(self.patch, backend, "get_boot_method_reader")
+        def get_boot_method_reader(boot_method, params):
             params_json = json.dumps(params)
             params_json_reader = BytesReader(params_json)
             return succeed(params_json_reader)

=== modified file 'src/provisioningserver/tftp.py'
--- src/provisioningserver/tftp.py	2014-03-28 15:17:34 +0000
+++ src/provisioningserver/tftp.py	2014-05-28 13:35:49 +0000
@@ -18,7 +18,6 @@
     ]
 
 import httplib
-from io import BytesIO
 import json
 from urllib import urlencode
 from urlparse import (
@@ -34,10 +33,7 @@
     deferred,
     get_all_interface_addresses,
     )
-from tftp.backend import (
-    FilesystemSynchronousBackend,
-    IReader,
-    )
+from tftp.backend import FilesystemSynchronousBackend
 from tftp.errors import FileNotFound
 from tftp.protocol import TFTP
 from twisted.application import internet
@@ -45,22 +41,6 @@
 from twisted.python.context import get
 from twisted.web.client import getPage
 import twisted.web.error
-from zope.interface import implementer
-
-
-@implementer(IReader)
-class BytesReader:
-
-    def __init__(self, data):
-        super(BytesReader, self).__init__()
-        self.buffer = BytesIO(data)
-        self.size = len(data)
-
-    def read(self, size):
-        return self.buffer.read(size)
-
-    def finish(self):
-        self.buffer.close()
 
 
 class TFTPBackend(FilesystemSynchronousBackend):
@@ -118,7 +98,7 @@
     def get_boot_method(self, file_name):
         """Finds the correct boot method."""
         for _, method in BootMethodRegistry:
-            params = method.match_config_path(file_name)
+            params = method.match_path(self, file_name)
             if params is not None:
                 return method, params
         return None, None
@@ -142,21 +122,19 @@
         return d
 
     @deferred
-    def get_config_reader(self, boot_method, params):
+    def get_boot_method_reader(self, boot_method, params):
         """Return an `IReader` for a boot method.
 
         :param boot_method: Boot method that is generating the config
         :param params: Parameters so far obtained, typically from the file
             path requested.
         """
-        def generate_config(kernel_params):
-            config = boot_method.render_config(
-                kernel_params=kernel_params, **params)
-            return config.encode("utf-8")
+        def generate(kernel_params):
+            return boot_method.get_reader(
+                self, kernel_params=kernel_params, **params)
 
         d = self.get_kernel_params(params)
-        d.addCallback(generate_config)
-        d.addCallback(BytesReader)
+        d.addCallback(generate)
         return d
 
     @staticmethod
@@ -203,7 +181,7 @@
         remote_host, remote_port = get("remote", (None, None))
         params["remote"] = remote_host
         params["cluster_uuid"] = get_cluster_uuid()
-        d = self.get_config_reader(boot_method, params)
+        d = self.get_boot_method_reader(boot_method, params)
         d.addErrback(self.get_page_errback, file_name)
         return d