Merge lp:~smoser/maas/kernel-cmdline-cleanup into lp:maas/trunk

[Scott Moser]

cleanup of kernel command line arguments, add iscsi_inititator param

Move some options to purpose specific, drop some, change some.

ephemeral/commissioning:
 * add iscsi_initiator: this was always supposed to be required, but 12.04
   initramfs seem to work without it, so we didn't have it. 12.10 fail
   to configure the iscsi target in the initramfs without it. (LP: #1050523)
 * change url= to cloud-config-url=<preseed_url>
   This was really just re-named to cloud-config-url. cloud-init supports
   both 'url=' and 'cloud-config-url', but cloud-config-url is preferable
   as it is obviously more explicit in purpose.
 * change ip=dhcp to ip=::::<hostname>
   LP: #1046405 has more information, but this is how you would specify
   that the initramfs's dhcp client should specify hostname
 * add overlayroot=tmpfs
   cloud images for quantal now use the overlayroot package to accomplish
   read-only root. 12.04 images used a un-packaged earlier version that
   was hard-coded in the images to on. 12.10 and later need a kernel param
   to enable it.

move to install specific:
 netcfg/choose_interface, hostname, domain, text, priority, auto, locale

common:
 * add to intel: 'console=tty1 console=ttyS0'.
   This should log kernel messages to both serial console and graphical
   console if it is present. Note, the kernel assigns /dev/console to
       the last valid argument.
 * add 'nomodeset' to not switch video mode
 * remove 'suite'. Nothing that I am aware of reads this.

Other:
 * add 'SAY' of the kernel cmdline. At least in testing with kvm
   and -curses, this is very helpful as it goes to wherever the bios
   console is, and you can see it even if 'console=ttyS0' is given
   and the kernel's log of that information goes elsewhere.

cleanup of kernel command line arguments, add iscsi_inititator param

This generally cleans up the parameters that we pass on the kernel command
line.

ephemeral/commissioning:
 * add iscsi_initiator: this was always supposed to be required, but 12.04
   initramfs seem to work without it, so we didn't have it. 12.10 fail
   to configure the iscsi target in the initramfs without it. (LP: #1050523)
 * change url= to cloud-config-url=<preseed_url>
   This was really just re-named to cloud-config-url. cloud-init supports
   both 'url=' and 'cloud-config-url', but cloud-config-url is preferable
   as it is obviously more explicit in purpose.
 * change ip=dhcp to ip=::::<hostname>
   LP: #1046405 has more information, but this is how you would specify
   that the initramfs's dhcp client should specify hostname
 * add overlayroot=tmpfs
   cloud images for quantal now use the overlayroot package to accomplish
   read-only root. 12.04 images used a un-packaged earlier version that
   was hard-coded in the images to on. 12.10 and later need a kernel param
   to enable it.

move to install specific:
 netcfg/choose_interface, hostname, domain, text, priority, auto, locale

common:
 * add to intel: 'console=tty1 console=ttyS0'.
   This should log kernel messages to both serial console and graphical
   console if it is present. Note, the kernel assigns /dev/console to
       the last valid argument.
 * add 'nomodeset' to not switch video mode
 * remove 'suite'. Nothing that I am aware of reads this.

Other:
 * add 'SAY' of the kernel cmdline. At least in testing with kvm
   and -curses, this is very helpful as it goes to wherever the bios
   console is, and you can see it even if 'console=ttyS0' is given
   and the kernel's log of that information goes elsewhere.

[Raphaël Badin (rvb)]

Looks good, thanks for the comments in the code. I suppose you've field-tested this. A few remarks:

[0]

162 +def make_kernel_parameters(content=None):

Looks like 'content' could be renamed 'extra_parameters' or something…?

[1]

Some things are untested, I think it's worth adding a few tests or changing existing tests:

48 + "ip=::::%s" % params.hostname,

65 + "text priority=%s" % "critical",

79 + return ["console=tty1 console=ttyS0"]

97 + options = ["nomodeset"]

[2]

244 + self.assertIn("root=LABEL=cloudimg-rootfs", cmdline)
245 + self.assertIn("iscsi_initiator=", cmdline)
246 + self.assertIn("overlayroot=", cmdline)

If something goes wrong, this would be more easy to fix it it was written as a one line check:

Something along the lines of
        self.assertThat(
            cmdline,
            ContainsAll([
                "root=LABEL=cloudimg-rootfs",
                "iscsi_initiator=",
                "overlayroot=tmpfs",
                ]))

Even if I admit this is slightly less readable, the reasoning is that, if none of the strings that should be present in the cmdline are present, you'll have that information in one go as opposed to learning that "root=LABEL=cloudimg-rootfs" is not there, fix the problem, run the tests again, see that "iscsi_initiator=" is not there, fix the problem, etc.

[3]

88 - compose_initrd_opt(
89 - params.arch, params.subarch,
90 - params.release, params.purpose),

The method 'compose_initrd_opt' isn't called from anywhere now but is still in the code…?

fwiw, I spotted that in the coverage information:
$ ./bin/maas test --with-coverage --cover-package=provisioningserver.kernel_opts src/provisioningserver/tests/test_
kernel_opts.py -s
[...]
Name Stmts Miss Cover Missing
--------------------------------------------------------------
provisioningserver.kernel_opts 51 2 96% 51-52
----------------------------------------------------------------------
Ran 13 tests in 0.183s

[4]

$ make lint
src/provisioningserver/tests/test_kernel_opts.py:32: 'compose_image_path' imported but unused

review: Approve

[lp:~smoser/maas/kernel-cmdline-cleanup]

980. By Scott Moser on 2012-09-14

remove unused compose_initrd_opt, improve test coverage

=== modified file 'src/provisioningserver/kernel_opts.py'
--- src/provisioningserver/kernel_opts.py	2012-09-03 11:31:04 +0000
+++ src/provisioningserver/kernel_opts.py	2012-09-14 18:55:21 +0000
@@ -47,11 +47,6 @@
     __call__ = KernelParametersBase._replace
 
 
-def compose_initrd_opt(arch, subarch, release, purpose):
-    path = "%s/initrd.gz" % compose_image_path(arch, subarch, release, purpose)
-    return "initrd=%s" % path
-
-
 def compose_preseed_opt(preseed_url):
     """Compose a kernel option for preseed URL.
 
@@ -60,18 +55,6 @@
     return "auto url=%s" % preseed_url
 
 
-def compose_suite_opt(release):
-    return "suite=%s" % release
-
-
-def compose_hostname_opt(hostname):
-    return "hostname=%s" % hostname
-
-
-def compose_domain_opt(domain):
-    return "domain=%s" % domain
-
-
 def compose_locale_opt():
     locale = 'en_US'
     return "locale=%s" % locale
@@ -81,7 +64,6 @@
     return [
         'log_host=%s' % log_host,
         'log_port=%d' % 514,
-        'text priority=%s' % 'critical',
         ]
 
 
@@ -127,19 +109,37 @@
 def compose_purpose_opts(params):
     """Return the list of the purpose-specific kernel options."""
     if params.purpose == "commissioning":
+        # these are kernel parameters read by ephemeral
+        # read by open-iscsi initramfs code
         return [
+            # read by open-iscsi initramfs code
             "iscsi_target_name=%s:%s" % (
                 ISCSI_TARGET_NAME_PREFIX,
                 get_ephemeral_name(params.release, params.arch)),
-            "ip=dhcp",
-            "ro root=LABEL=cloudimg-rootfs",
             "iscsi_target_ip=%s" % params.fs_host,
             "iscsi_target_port=3260",
+            "iscsi_initiator=%s" % params.hostname,
+            # read by klibc 'ipconfig' in initramfs
+            "ip=::::%s" % params.hostname,
+            # cloud-images have this filesystem label
+            "ro root=LABEL=cloudimg-rootfs",
+            # read by overlayroot package
+            "overlayroot=tmpfs",
+            # read by cloud-init
+            "cloud-config-url=%s" % params.preseed_url,
             ]
     else:
+        # these are options used by the debian installer
         return [
-            "netcfg/choose_interface=auto"
-            ]
+            # read by debian installer
+            "netcfg/choose_interface=auto",
+            "hostname=%s" % params.hostname,
+            "domain=%s" % params.domain,
+            "text priority=%s" % "critical",
+            "auto",
+            "url=%s" % params.preseed_url,
+            compose_locale_opt(),
+        ]
 
 
 def compose_arch_opts(params):
@@ -147,7 +147,8 @@
     if (params.arch, params.subarch) == ("armhf", "highbank"):
         return ["console=ttyAMA0"]
     else:
-        return []
+        # on intel, send kernel output to both console and ttyS0
+        return ["console=tty1 console=ttyS0"]
 
 
 def compose_kernel_command_line_new(params):
@@ -155,17 +156,11 @@
 
     :type params: `KernelParameters`.
     """
-    options = [
-        compose_initrd_opt(
-            params.arch, params.subarch,
-            params.release, params.purpose),
-        compose_preseed_opt(params.preseed_url),
-        compose_suite_opt(params.release),
-        compose_hostname_opt(params.hostname),
-        compose_domain_opt(params.domain),
-        compose_locale_opt(),
-        ]
+    options = ["nomodeset"]
     options += compose_purpose_opts(params)
+    # Note: logging opts are not respected by ephemeral images, so
+    #       these are actually "purpose_opts" but were left generic
+    #       as it would be nice to have.
     options += compose_logging_opts(params.log_host)
     options += compose_arch_opts(params)
     return ' '.join(options)

=== modified file 'src/provisioningserver/pxe/config.commissioning.template'
--- src/provisioningserver/pxe/config.commissioning.template	2012-08-30 10:42:56 +0000
+++ src/provisioningserver/pxe/config.commissioning.template	2012-09-14 18:55:21 +0000
@@ -6,6 +6,7 @@
 
 LABEL amd64
   SAY Booting (amd64) under MAAS direction...
+  SAY {{kernel_params(arch="amd64") | kernel_command}}
   KERNEL {{kernel_params(arch="amd64") | kernel_path }}
   INITRD {{kernel_params(arch="amd64") | initrd_path }}
   APPEND {{kernel_params(arch="amd64") | kernel_command}}
@@ -13,6 +14,7 @@
 
 LABEL i386
   SAY Booting (i386) under MAAS direction...
+  SAY {{kernel_params(arch="i386") | kernel_command}}
   KERNEL {{kernel_params(arch="i386") | kernel_path }}
   INITRD {{kernel_params(arch="i386") | initrd_path }}
   APPEND {{kernel_params(arch="i386") | kernel_command}}

=== modified file 'src/provisioningserver/pxe/config.template'
--- src/provisioningserver/pxe/config.template	2012-08-30 10:42:56 +0000
+++ src/provisioningserver/pxe/config.template	2012-09-14 18:55:21 +0000
@@ -2,6 +2,7 @@
 
 LABEL execute
   SAY Booting under MAAS direction...
+  SAY {{kernel_params | kernel_command}}
   KERNEL {{kernel_params | kernel_path }}
   INITRD {{kernel_params | initrd_path }}
   APPEND {{kernel_params | kernel_command}}

=== modified file 'src/provisioningserver/tests/test_kernel_opts.py'
--- src/provisioningserver/tests/test_kernel_opts.py	2012-08-31 11:04:29 +0000
+++ src/provisioningserver/tests/test_kernel_opts.py	2012-09-14 18:55:21 +0000
@@ -19,6 +19,7 @@
 from maastesting.factory import factory
 from maastesting.matchers import ContainsAll
 from maastesting.testcase import TestCase
+from provisioningserver import kernel_opts
 from provisioningserver.kernel_opts import (
     compose_kernel_command_line_new,
     compose_preseed_opt,
@@ -27,6 +28,7 @@
     ISCSI_TARGET_NAME_PREFIX,
     KernelParameters,
     )
+
 from provisioningserver.pxe.tftppath import compose_image_path
 from provisioningserver.testing.config import ConfigFixture
 from testtools.matchers import (
@@ -35,12 +37,15 @@
     )
 
 
-def make_kernel_parameters():
+def make_kernel_parameters(extra_parameters=None):
     """Make a randomly populated `KernelParameters` instance."""
-    return KernelParameters(**{
+    parms = {
             field: factory.make_name(field)
             for field in KernelParameters._fields
-            })
+            }
+    if extra_parameters is not None:
+        parms.update(extra_parameters)
+    return KernelParameters(**parms)
 
 
 class TestUtilitiesKernelOpts(TestCase):
@@ -70,24 +75,8 @@
             "auto url=%s" % params.preseed_url,
             compose_kernel_command_line_new(params))
 
-    def test_compose_kernel_command_line_includes_initrd(self):
-        params = make_kernel_parameters()
-        initrd_path = compose_image_path(
-            params.arch, params.subarch, params.release,
-            purpose=params.purpose)
-        self.assertIn(
-            "initrd=%s" % initrd_path,
-            compose_kernel_command_line_new(params))
-
-    def test_compose_kernel_command_line_includes_suite(self):
-        # At the moment, the OS release we use is hard-coded to "precise."
-        params = make_kernel_parameters()
-        self.assertIn(
-            "suite=%s" % params.release,
-            compose_kernel_command_line_new(params))
-
-    def test_compose_kernel_command_line_includes_hostname_and_domain(self):
-        params = make_kernel_parameters()
+    def test_install_compose_kernel_command_line_includes_name_domain(self):
+        params = make_kernel_parameters({"purpose": "install"})
         self.assertThat(
             compose_kernel_command_line_new(params),
             ContainsAll([
@@ -95,15 +84,15 @@
                 "domain=%s" % params.domain,
                 ]))
 
-    def test_compose_kernel_command_line_includes_locale(self):
-        params = make_kernel_parameters()
+    def test_install_compose_kernel_command_line_includes_locale(self):
+        params = make_kernel_parameters({"purpose": "install"})
         locale = "en_US"
         self.assertIn(
             "locale=%s" % locale,
             compose_kernel_command_line_new(params))
 
-    def test_compose_kernel_command_line_includes_log_settings(self):
-        params = make_kernel_parameters()
+    def test_install_compose_kernel_command_line_includes_log_settings(self):
+        params = make_kernel_parameters({"purpose": "install"})
         # Port 514 (UDP) is syslog.
         log_port = "514"
         text_priority = "critical"
@@ -115,14 +104,46 @@
                 "text priority=%s" % text_priority,
                 ]))
 
-    def test_compose_kernel_command_line_inc_purpose_opts(self):
+    def test_install_compose_kernel_command_line_inc_purpose_opts(self):
         # The result of compose_kernel_command_line includes the purpose
         # options for a non "commissioning" node.
-        params = make_kernel_parameters()
+        params = make_kernel_parameters({"purpose": "install"})
         self.assertIn(
             "netcfg/choose_interface=auto",
             compose_kernel_command_line_new(params))
 
+    def test_commissioning_compose_kernel_command_line_inc_purpose_opts(self):
+        # The result of compose_kernel_command_line includes the purpose
+        # options for a non "commissioning" node.
+        self.patch(kernel_opts,
+                   "get_ephemeral_name").return_value = "RELEASE-ARCH"
+        params = make_kernel_parameters({"purpose": "commissioning"})
+        cmdline = compose_kernel_command_line_new(params)
+        self.assertThat(
+            cmdline,
+            ContainsAll([
+                "root=LABEL=cloudimg-rootfs",
+                "iscsi_initiator=",
+                "overlayroot=tmpfs",
+                "ip=::::%s" % params.hostname]))
+
+    def test_compose_kernel_command_line_inc_common_opts(self):
+        # Test that some kernel arguments appear on both commissioning
+        # and install command lines.
+        self.patch(kernel_opts,
+                   "get_ephemeral_name").return_value = "RELEASE-ARCH"
+        expected = ["console=tty1", "console=ttyS0", "nomodeset"]
+
+        params = make_kernel_parameters({
+            "purpose": "commissioning", "arch": "i386" })
+        cmdline = compose_kernel_command_line_new(params)
+        self.assertThat(cmdline, ContainsAll(expected))
+
+        params = make_kernel_parameters({
+            "purpose": "install", "arch": "i386" })
+        cmdline = compose_kernel_command_line_new(params)
+        self.assertThat(cmdline, ContainsAll(expected))
+
     def create_ephemeral_info(self, name, arch, release):
         """Create a pseudo-real ephemeral info file."""
         ephemeral_info = """