Merge lp:~blake-rouse/maas/ppc64el-grub into lp:~maas-committers/maas/trunk

[Blake Rouse]

Hardware enablement support for PowerKVM

Hardware enablement for PowerKVM. Installs the correct grub boot loader files and modifies the dhcpd config to allow booting of PPC64EL. The UEFIBootMethod provides the rest of the support for grub booting.

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

[Andres Rodriguez (andreserl)]

Oh and please backport this to 1.5

[Gavin Panella (allenap)]

Looks good. The first 6 comments are all fairly trivial but [7] is a
blocker.

[1]

+    echo   '{{kernel_params | kernel_command}} BOOTIF=01-'${net_default_mac}

If the kernel command has any single quotes in it this might do funny
things. I see this kind of thing done in many places. I'm provisionally
calling it hope-quoting as of now. Omit this line if there's no
satisfactory way of quoting/escaping the kernel command that'll work
with the boot config's echo command.

[2]

+menuentry 'Local' {
+    echo 'Booting local disk ...'

Minor, trivial, point, but the ellipsis is separated by one space from
the sentence. Compare with:

 menuentry 'Commission' {
     echo   'Booting under MAAS direction...'

Can you make it consistent?

[3]

+GRUB_CONFIG = dedent("""
+    configfile (pxe)/grub/grub.cfg-${net_default_mac}
+    configfile (pxe)/grub/grub.cfg-default-ppc64el
+    """)

Remove the leading blank line with a back-slash:

GRUB_CONFIG = dedent("""\
    configfile (pxe)/grub/grub.cfg-${net_default_mac}
    configfile (pxe)/grub/grub.cfg-default-ppc64el
    """)

[4]

+    def match_config_path(self, path):
+        """Doesn't need to do anything, as the UEFIBootMethod provides
+        the grub implementation needed.
+        """
+        return None
+
+    def render_config(self, kernel_params, **extra):
+        """Doesn't need to do anything, as the UEFIBootMethod provides
+        the grub implementation needed.
+        """
+        return ""

I don't understand the docstrings here. Can you elaborate? If I'm simply
being stupid, just reply with the answer, otherwise please update the
docstring.

[5]

+            for module in glob.glob(os.path.join(module_dir, '*.mod')):
+                modules.append(os.path.splitext(os.path.basename(module))[0])

For clarity, can you do this in a few more lines, e.g.:

            for module_path in glob.glob(os.path.join(module_dir, '*.mod')):
                module_filename = os.path.basename(module_path)
                module_name, _ = os.path.splitext(module_filename)
                modules.append(module_name)

[6]

         options = {
-            "kernel_params": make_kernel_parameters(purpose="local"),
+            "kernel_params": make_kernel_parameters(
+                purpose="local", arch="amd64"),
             }

Why this change?

[7]

There are no tests.

review: Needs Fixing

[Blake Rouse (blake-rouse)]

[1,2,3] Fixed

[4] The methods actually do nothing as the UEFIBootMethod class provides the implementation.

[5] Fixed

[6] Changed as the wrong config would be used for the test.

[Blake Rouse (blake-rouse)]

[7] Tests added.

[Gavin Panella (allenap)]

Looks good! I think [8] needs fixing, but I'll mark this approved
because I don't want to delay you once you've fixed it.

[8]

+        ppc64el_module.tempdir = tempdir

Use self.patch() here too; I think this will leak otherwise.

[9]

+        tmp = tempfile.mkdtemp('', 'maas-test-')
+        dest = tempfile.mkdtemp('', 'maas-test-')
...
+        rmtree(tmp)
+        rmtree(dest)

Fwiw, the call to rmtree should be put into a clean-up as soon as the
directory is created, so that a test failure doesn't leave detritus
around. For example:

        tmp = tempfile.mkdtemp('', 'maas-test-')
        self.addCleanup(rmtree, tmp)

However, look at MAASTestCase.make_dir(). This does what you need.

review: Approve

=== modified file 'etc/maas/templates/commissioning-user-data/snippets/maas_enlist.sh'
--- etc/maas/templates/commissioning-user-data/snippets/maas_enlist.sh	2014-04-25 18:03:38 +0000
+++ etc/maas/templates/commissioning-user-data/snippets/maas_enlist.sh	2014-05-15 17:23:24 +0000
@@ -61,7 +61,7 @@
 get_host_subarchitecture() {
 	local arch=$1
 	case $arch in
-	    i386|amd64)
+	    i386|amd64|ppc64el)
 		# Skip the call to archdetect as that's what
 		# get_host_architecture does
 		echo generic

=== modified file 'etc/maas/templates/uefi/config.commissioning.template'
--- etc/maas/templates/uefi/config.commissioning.template	2014-03-18 17:41:14 +0000
+++ etc/maas/templates/uefi/config.commissioning.template	2014-05-15 17:23:24 +0000
@@ -1,10 +1,8 @@
 set default="0"
 set timeout=0
 
-# Force AMD64 for commissioning as UEFI only supports AMD64 currently.
 menuentry 'Commission' {
     echo   'Booting under MAAS direction...'
-    echo   '{{kernel_params() | kernel_command}} BOOTIF=01-'${net_default_mac}
-    linux  {{kernel_params(arch="amd64") | kernel_path }} {{kernel_params(arch="amd64") | kernel_command}} BOOTIF=01-${net_default_mac}
-    initrd {{kernel_params(arch="amd64") | initrd_path }}
+    linux  {{kernel_params | kernel_path }} {{kernel_params | kernel_command}} BOOTIF=01-${net_default_mac}
+    initrd {{kernel_params | initrd_path }}
 }

=== modified file 'etc/maas/templates/uefi/config.install.template'
--- etc/maas/templates/uefi/config.install.template	2014-03-04 16:59:37 +0000
+++ etc/maas/templates/uefi/config.install.template	2014-05-15 17:23:24 +0000
@@ -3,7 +3,6 @@
 
 menuentry 'Install' {
     echo   'Booting under MAAS direction...'
-    echo   '{{kernel_params | kernel_command}} BOOTIF=01-'${net_default_mac}
     linux  {{kernel_params | kernel_path }} {{kernel_params | kernel_command}} BOOTIF=01-${net_default_mac}
     initrd {{kernel_params | initrd_path }}
 }

=== renamed file 'etc/maas/templates/uefi/config.local.template' => 'etc/maas/templates/uefi/config.local.amd64.template'
--- etc/maas/templates/uefi/config.local.template	2014-03-28 19:03:46 +0000
+++ etc/maas/templates/uefi/config.local.amd64.template	2014-05-15 17:23:24 +0000
@@ -2,7 +2,7 @@
 set timeout=0
 
 menuentry 'Local' {
-    echo 'Booting local disk ...'
+    echo 'Booting local disk...'
     search --set=root --file /efi/ubuntu/grub.cfg
     configfile /efi/ubuntu/grub.cfg
 }

=== added file 'etc/maas/templates/uefi/config.local.ppc64el.template'
--- etc/maas/templates/uefi/config.local.ppc64el.template	1970-01-01 00:00:00 +0000
+++ etc/maas/templates/uefi/config.local.ppc64el.template	2014-05-15 17:23:24 +0000
@@ -0,0 +1,8 @@
+set default="0"
+set timeout=0
+
+menuentry 'Local' {
+    echo 'Booting local disk...'
+    search --set=root --file /boot/grub/grub.cfg
+    configfile /boot/grub/grub.cfg
+}

=== modified file 'src/provisioningserver/boot/__init__.py'
--- src/provisioningserver/boot/__init__.py	2014-04-24 14:09:58 +0000
+++ src/provisioningserver/boot/__init__.py	2014-05-15 17:23:24 +0000
@@ -202,11 +202,13 @@
 # Import the supported boot methods after defining BootMethod.
 from provisioningserver.boot.pxe import PXEBootMethod
 from provisioningserver.boot.uefi import UEFIBootMethod
+from provisioningserver.boot.ppc64el import PPC64ELBootMethod
 
 
 builtin_boot_methods = [
     PXEBootMethod(),
     UEFIBootMethod(),
+    PPC64ELBootMethod(),
 ]
 for method in builtin_boot_methods:
     BootMethodRegistry.register_item(method.name, method)

=== added file 'src/provisioningserver/boot/ppc64el.py'
--- src/provisioningserver/boot/ppc64el.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/boot/ppc64el.py	2014-05-15 17:23:24 +0000
@@ -0,0 +1,107 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""PPC64EL Boot Method"""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    'PPC64ELBootMethod',
+    ]
+
+import glob
+import os.path
+from textwrap import dedent
+
+from provisioningserver.boot import (
+    BootMethod,
+    BootMethodInstallError,
+    utils,
+    )
+from provisioningserver.boot.install_bootloader import install_bootloader
+from provisioningserver.utils import (
+    call_and_check,
+    tempdir,
+    )
+
+
+GRUB_CONFIG = dedent("""\
+    configfile (pxe)/grub/grub.cfg-${net_default_mac}
+    configfile (pxe)/grub/grub.cfg-default-ppc64el
+    """)
+
+
+class PPC64ELBootMethod(BootMethod):
+
+    name = "ppc64el"
+    template_subdir = "ppc64el"
+    bootloader_path = "bootppc64.bin"
+    arch_octet = "00:0C"
+
+    def match_config_path(self, path):
+        """Doesn't need to do anything, as the UEFIBootMethod provides
+        the grub implementation needed.
+        """
+        return None
+
+    def render_config(self, kernel_params, **extra):
+        """Doesn't need to do anything, as the UEFIBootMethod provides
+        the grub implementation needed.
+        """
+        return ""
+
+    def install_bootloader(self, destination):
+        """Installs the required files for PPC64EL booting into the
+        tftproot.
+        """
+        with tempdir() as tmp:
+            # Download the grub-ieee1275-bin package
+            data, filename = utils.get_updates_package(
+                'grub-ieee1275-bin', 'http://ports.ubuntu.com',
+                'main', 'ppc64el')
+            if data is None:
+                raise BootMethodInstallError(
+                    'Failed to download grub-ieee1275-bin package from '
+                    'the archive.')
+            grub_output = os.path.join(tmp, filename)
+            with open(grub_output, 'wb') as stream:
+                stream.write(data)
+
+            # Extract the package with dpkg, and install the shim
+            call_and_check(["dpkg", "-x", grub_output, tmp])
+
+            # Output the embedded config, so grub-mkimage can use it
+            config_output = os.path.join(tmp, 'grub.cfg')
+            with open(config_output, 'wb') as stream:
+                stream.write(GRUB_CONFIG.encode('utf-8'))
+
+            # Get list of grub modules
+            module_dir = os.path.join(
+                tmp, 'usr', 'lib', 'grub', 'powerpc-ieee1275')
+            modules = []
+            for module_path in glob.glob(os.path.join(module_dir, '*.mod')):
+                module_filename = os.path.basename(module_path)
+                module_name, _ = os.path.splitext(module_filename)
+                modules.append(module_name)
+
+            # Generate the grub bootloader
+            mkimage_output = os.path.join(tmp, self.bootloader_path)
+            args = [
+                'grub-mkimage',
+                '-o', mkimage_output,
+                '-O', 'powerpc-ieee1275',
+                '-d', module_dir,
+                '-c', config_output,
+                ]
+            call_and_check(args + modules)
+
+            install_bootloader(
+                mkimage_output,
+                os.path.join(destination, self.bootloader_path))

=== added file 'src/provisioningserver/boot/tests/test_ppc64el.py'
--- src/provisioningserver/boot/tests/test_ppc64el.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/boot/tests/test_ppc64el.py	2014-05-15 17:23:24 +0000
@@ -0,0 +1,92 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `provisioningserver.boot.ppc64el`."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+from contextlib import contextmanager
+import os
+
+from maastesting.factory import factory
+from maastesting.matchers import MockCalledOnceWith
+from maastesting.testcase import MAASTestCase
+from provisioningserver.boot import (
+    BootMethodInstallError,
+    ppc64el as ppc64el_module,
+    utils,
+    )
+from provisioningserver.boot.ppc64el import (
+    GRUB_CONFIG,
+    PPC64ELBootMethod,
+    )
+from provisioningserver.tests.test_kernel_opts import make_kernel_parameters
+
+
+class TestPPC64ELBootMethod(MAASTestCase):
+    """Tests `provisioningserver.boot.ppc64el.PPC64ELBootMethod`."""
+
+    def test_match_config_path_returns_none(self):
+        method = PPC64ELBootMethod()
+        paths = [factory.getRandomString() for _ in range(3)]
+        for path in paths:
+            self.assertEqual(None, method.match_config_path(path))
+
+    def test_render_config_returns_empty_string(self):
+        method = PPC64ELBootMethod()
+        params = [make_kernel_parameters() for _ in range(3)]
+        for param in params:
+            self.assertEqual("", method.render_config(params))
+
+    def test_install_bootloader_get_package_raises_error(self):
+        method = PPC64ELBootMethod()
+        self.patch(utils, 'get_updates_package').return_value = (None, None)
+        self.assertRaises(
+            BootMethodInstallError, method.install_bootloader, None)
+
+    def test_install_bootloader(self):
+        method = PPC64ELBootMethod()
+        filename = factory.make_name('dpkg')
+        data = factory.getRandomString()
+        tmp = self.make_dir()
+        dest = self.make_dir()
+
+        @contextmanager
+        def tempdir():
+            try:
+                yield tmp
+            finally:
+                pass
+
+        mock_get_updates_package = self.patch(utils, 'get_updates_package')
+        mock_get_updates_package.return_value = (data, filename)
+        self.patch(ppc64el_module, 'call_and_check')
+        self.patch(ppc64el_module, 'tempdir').side_effect = tempdir
+
+        mock_install_bootloader = self.patch(
+            ppc64el_module, 'install_bootloader')
+
+        method.install_bootloader(dest)
+
+        with open(os.path.join(tmp, filename), 'rb') as stream:
+            saved_data = stream.read()
+        self.assertEqual(data, saved_data)
+
+        with open(os.path.join(tmp, 'grub.cfg'), 'rb') as stream:
+            saved_config = stream.read().decode('utf-8')
+        self.assertEqual(GRUB_CONFIG, saved_config)
+
+        mkimage_expected = os.path.join(tmp, method.bootloader_path)
+        dest_expected = os.path.join(dest, method.bootloader_path)
+        self.assertThat(
+            mock_install_bootloader,
+            MockCalledOnceWith(mkimage_expected, dest_expected))

=== modified file 'src/provisioningserver/boot/tests/test_uefi.py'
--- src/provisioningserver/boot/tests/test_uefi.py	2014-04-24 14:09:58 +0000
+++ src/provisioningserver/boot/tests/test_uefi.py	2014-05-15 17:23:24 +0000
@@ -107,7 +107,8 @@
         # used.
         method = UEFIBootMethod()
         options = {
-            "kernel_params": make_kernel_parameters(purpose="local"),
+            "kernel_params": make_kernel_parameters(
+                purpose="local", arch="amd64"),
             }
         output = method.render_config(**options)
         self.assertIn("configfile /efi/ubuntu/grub.cfg", output)

=== modified file 'src/provisioningserver/driver/__init__.py'
--- src/provisioningserver/driver/__init__.py	2014-05-01 19:25:38 +0000
+++ src/provisioningserver/driver/__init__.py	2014-05-15 17:23:24 +0000
@@ -204,6 +204,7 @@
     Architecture(
         name="armhf/generic", description="armhf/generic",
         pxealiases=["arm"], kernel_options=["console=ttyAMA0"]),
+    Architecture(name="ppc64el/generic", description="ppc64el"),
 ]
 for arch in builtin_architectures:
     ArchitectureRegistry.register_item(arch.name, arch)