Merge lp:~smoser/maas/lp1402042 into lp:~maas-committers/maas/trunk

[Scott Moser]

support using '---' as separator for copy-over kernel params

See bug 1402042 for a full description of why this change is necessary.

Suggestions on the variable name are welcome. I'd like to keep it consistent between maas and curtin.

[Raphaël Badin (rvb)]

Looks okay but here are my suggestions for this code: http://paste.ubuntu.com/11841389/
What do you think?

[Scott Moser (smoser)]

the only comment i have is that CURTIN_KERNEL_CMDLINE_NAME seems not useful.
the name 'KERNEL_CMDLINE_COPY_TO_INSTALL_SEP' is obnoxiously large, but it better describes it.
CURTIN_KERNEL_CMDLINE_NAME doesn't really mean anything if you read that by itself.

i fully accept that this is a bikeshed, but as i said in the description i'd like to have the name stay the same between maas and curtin.

its up to you, i dont really have a strong feelings.

[Raphaël Badin (rvb)]

> the only comment i have is that CURTIN_KERNEL_CMDLINE_NAME seems not useful.
> the name 'KERNEL_CMDLINE_COPY_TO_INSTALL_SEP' is obnoxiously large, but it
> better describes it.
> CURTIN_KERNEL_CMDLINE_NAME doesn't really mean anything if you read that by
> itself.
>
> i fully accept that this is a bikeshed, but as i said in the description i'd
> like to have the name stay the same between maas and curtin.
>
> its up to you, i dont really have a strong feelings.

Okay, sure, let's rename CURTIN_KERNEL_CMDLINE_NAME.

I agree it's convenient if the names are the same but note that the MAAS instance doesn't need to be exported (aka put in __all__) because this is only used in this module (well, and the tests but that doesn't count).

[Raphaël Badin (rvb)]

Looks good to me :).

review: Approve

=== modified file 'src/provisioningserver/kernel_opts.py'
--- src/provisioningserver/kernel_opts.py	2015-06-10 11:35:04 +0000
+++ src/provisioningserver/kernel_opts.py	2015-07-09 13:39:17 +0000
@@ -21,6 +21,7 @@
 from collections import namedtuple
 import os
 
+import curtin
 from provisioningserver.drivers import ArchitectureRegistry
 from provisioningserver.logger import get_maas_logger
 
@@ -171,6 +172,15 @@
         return []
 
 
+CURTIN_KERNEL_CMDLINE_NAME = 'KERNEL_CMDLINE_COPY_TO_INSTALL_SEP'
+
+
+def get_curtin_kernel_cmdline_sep():
+    """Return the separator for passing extra parameters to the kernel."""
+    return getattr(
+        curtin, CURTIN_KERNEL_CMDLINE_NAME, '--')
+
+
 def compose_kernel_command_line(params):
     """Generate a line of kernel options for booting `node`.
 
@@ -185,13 +195,16 @@
     #       as it would be nice to have.
     options += compose_logging_opts(params.log_host)
     options += compose_arch_opts(params)
+    cmdline_sep = get_curtin_kernel_cmdline_sep()
     if params.extra_opts:
-        # Using -- before extra opts makes both d-i and Curtin install
+        # Using --- before extra opts makes both d-i and Curtin install
         # them into the grub config when installing an OS, thus causing
         # the options to "stick" when local booting later.
-        options.append('--')
+        # see LP: #1402042 for info on '---' versus '--'
+        options.append(cmdline_sep)
         options.append(params.extra_opts)
     kernel_opts = ' '.join(options)
     maaslog.debug(
-        '%s: kernel parameters -- "%s"' % (params.hostname, kernel_opts))
+        '%s: kernel parameters %s "%s"' %
+        (cmdline_sep, params.hostname, kernel_opts))
     return kernel_opts
 
=== modified file 'src/provisioningserver/tests/test_kernel_opts.py'
--- src/provisioningserver/tests/test_kernel_opts.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/tests/test_kernel_opts.py	2015-07-09 13:39:17 +0000
@@ -20,6 +20,7 @@
 
 from maastesting.factory import factory
 from maastesting.testcase import MAASTestCase
+from mock import sentinel
 from provisioningserver import kernel_opts
 from provisioningserver.drivers import (
     Architecture,
@@ -29,6 +30,8 @@
     compose_arch_opts,
     compose_kernel_command_line,
     compose_preseed_opt,
+    CURTIN_KERNEL_CMDLINE_NAME,
+    get_curtin_kernel_cmdline_sep,
     get_ephemeral_name,
     get_last_directory,
     ISCSI_TARGET_NAME_PREFIX,
@@ -106,6 +109,29 @@
         self.assertNotIn('::' + target, full_name)
 
 
+class TestGetCurtinKernelCmdlineSepTest(MAASTestCase):
+
+    def test_get_curtin_kernel_cmdline_sep_returns_curtin_value(self):
+        sep = factory.make_name('separator')
+        self.patch(
+            kernel_opts.curtin, CURTIN_KERNEL_CMDLINE_NAME, sep)
+        self.assertEqual(sep, get_curtin_kernel_cmdline_sep())
+
+    def test_get_curtin_kernel_cmdline_sep_returns_default(self):
+        original_sep = getattr(
+            kernel_opts.curtin, CURTIN_KERNEL_CMDLINE_NAME,
+            sentinel.missing)
+
+        if original_sep != sentinel.missing:
+            def restore_sep():
+                setattr(
+                    kernel_opts.curtin,
+                    CURTIN_KERNEL_CMDLINE_NAME, original_sep)
+            self.addCleanup(restore_sep)
+            delattr(kernel_opts.curtin, CURTIN_KERNEL_CMDLINE_NAME)
+        self.assertEqual('--', get_curtin_kernel_cmdline_sep())
+
+
 class TestKernelOpts(MAASTestCase):
 
     def make_kernel_parameters(self, *args, **kwargs):
@@ -191,12 +217,16 @@
                 "ip=::::%s:BOOTIF" % params.hostname]))
 
     def test_commissioning_compose_kernel_command_line_inc_extra_opts(self):
+        mock_get_curtin_sep = self.patch(
+            kernel_opts, 'get_curtin_kernel_cmdline_sep')
+        sep = factory.make_name('sep')
+        mock_get_curtin_sep.return_value = sep
         extra_opts = "special console=ABCD -- options to pass"
         params = self.make_kernel_parameters(extra_opts=extra_opts)
         cmdline = compose_kernel_command_line(params)
-        # There should be a double dash (--) surrounded by spaces before
-        # the options, but otherwise added verbatim.
-        self.assertThat(cmdline, Contains(' -- ' + extra_opts))
+        # There should be KERNEL_CMDLINE_COPY_TO_INSTALL_SEP surrounded by
+        # spaces before the options, but otherwise added verbatim.
+        self.assertThat(cmdline, Contains(' %s ' % sep + extra_opts))
 
     def test_commissioning_compose_kernel_handles_extra_opts_None(self):
         params = self.make_kernel_parameters(extra_opts=None)