launchpad-reviewers team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

Scott Moser has proposed merging lp:~smoser/maas/kernel-cmdline-cleanup into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)
Related bugs:
  Bug #1050523 in MAAS: "maas kernel cmdline must include iscsi_initiator"
  https://bugs.launchpad.net/maas/+bug/1050523

For more details, see:
https://code.launchpad.net/~smoser/maas/kernel-cmdline-cleanup/+merge/124226

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.

-- 
https://code.launchpad.net/~smoser/maas/kernel-cmdline-cleanup/+merge/124226
Your team MAAS Maintainers is requested to review the proposed merge of lp:~smoser/maas/kernel-cmdline-cleanup into lp:maas.

=== 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 15:10:26 +0000
@@ -60,18 +60,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 +69,6 @@
     return [
         'log_host=%s' % log_host,
         'log_port=%d' % 514,
-        'text priority=%s' % 'critical',
         ]
 
 
@@ -127,19 +114,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 +152,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 +161,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 15:10:26 +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 15:10:26 +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 15:10:26 +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(content=None):
     """Make a randomly populated `KernelParameters` instance."""
-    return KernelParameters(**{
+    parms = {
             field: factory.make_name(field)
             for field in KernelParameters._fields
-            })
+            }
+    if content is not None:
+        parms.update(content)
+    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,25 @@
                 "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.assertIn("root=LABEL=cloudimg-rootfs", cmdline)
+        self.assertIn("iscsi_initiator=", cmdline)
+        self.assertIn("overlayroot=", cmdline)
+
     def create_ephemeral_info(self, name, arch, release):
         """Create a pseudo-real ephemeral info file."""
         ephemeral_info = """