launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12076
[Merge] lp:~allenap/maas/kernel-cmdline-cleanup-redux into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/kernel-cmdline-cleanup-redux into lp:maas.
Requested reviews:
MAAS Maintainers (maas-maintainers)
For more details, see:
https://code.launchpad.net/~allenap/maas/kernel-cmdline-cleanup-redux/+merge/124700
This follows on from Scott's proposal:
https://code.launchpad.net/~smoser/maas/kernel-cmdline-cleanup/+merge/124226
It tidies a few bits, adds some more comments for the things I had to
look up (like "text priority=critical"), but the main changes were:
* Use KernelParameters' built-in capacity for modification. It's a
namedtuple, and _replace is aliased to __call__, so I removed the
extra code in make_kernel_parameters and used __call__ instead.
* Rename compose_kernel_command_line_new to drop the _new suffix.
There is no longer an old variant.
--
https://code.launchpad.net/~allenap/maas/kernel-cmdline-cleanup-redux/+merge/124700
Your team MAAS Maintainers is requested to review the proposed merge of lp:~allenap/maas/kernel-cmdline-cleanup-redux into lp:maas.
=== modified file 'src/provisioningserver/kernel_opts.py'
--- src/provisioningserver/kernel_opts.py 2012-09-17 03:16:23 +0000
+++ src/provisioningserver/kernel_opts.py 2012-09-17 14:23:22 +0000
@@ -11,7 +11,7 @@
__metaclass__ = type
__all__ = [
- 'compose_kernel_command_line_new',
+ 'compose_kernel_command_line',
'KernelParameters',
]
@@ -51,6 +51,7 @@
:param preseed_url: The URL from which a preseed can be fetched.
"""
+ # Is the "auto" a cargo-cult from Cobbler?
return "auto url=%s" % preseed_url
@@ -108,37 +109,35 @@
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
+ # These are kernel parameters read by the ephemeral environment.
return [
- # read by open-iscsi initramfs code
+ # Read by Open-iSCSI initramfs code.
"iscsi_target_name=%s:%s" % (
ISCSI_TARGET_NAME_PREFIX,
get_ephemeral_name(params.release, params.arch)),
"iscsi_target_ip=%s" % params.fs_host,
"iscsi_target_port=3260",
"iscsi_initiator=%s" % params.hostname,
- # read by klibc 'ipconfig' in initramfs
+ # Read by klibc 'ipconfig' in initramfs.
"ip=::::%s" % params.hostname,
- # cloud-images have this filesystem label
+ # cloud-images have this filesystem label.
"ro root=LABEL=cloudimg-rootfs",
- # read by overlayroot package
+ # Read by overlayroot package.
"overlayroot=tmpfs",
- # read by cloud-init
+ # Read by cloud-init.
"cloud-config-url=%s" % params.preseed_url,
]
else:
- # these are options used by the debian installer
+ # These are options used by the Debian Installer.
return [
- # 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,
+ # Use the text installer, display only critical messages.
+ "text priority=critical",
+ compose_preseed_opt(params.preseed_url),
compose_locale_opt(),
- ]
+ ]
def compose_arch_opts(params):
@@ -146,16 +145,18 @@
if (params.arch, params.subarch) == ("armhf", "highbank"):
return ["console=ttyAMA0"]
else:
- # on intel, send kernel output to both console and ttyS0
- return ["console=tty1 console=ttyS0"]
-
-
-def compose_kernel_command_line_new(params):
+ # On Intel send kernel output to both console and ttyS0.
+ return ["console=tty1", "console=ttyS0"]
+
+
+def compose_kernel_command_line(params):
"""Generate a line of kernel options for booting `node`.
:type params: `KernelParameters`.
"""
- options = ["nomodeset"]
+ options = []
+ # nomodeset prevents video mode switching.
+ 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
=== modified file 'src/provisioningserver/pxe/config.py'
--- src/provisioningserver/pxe/config.py 2012-08-30 10:43:27 +0000
+++ src/provisioningserver/pxe/config.py 2012-09-17 14:23:22 +0000
@@ -23,7 +23,7 @@
from errno import ENOENT
from os import path
-from provisioningserver.kernel_opts import compose_kernel_command_line_new
+from provisioningserver.kernel_opts import compose_kernel_command_line
from provisioningserver.pxe.tftppath import compose_image_path
import tempita
@@ -97,7 +97,7 @@
return "%s/linux" % image_dir(params)
def kernel_command(params):
- return compose_kernel_command_line_new(params)
+ return compose_kernel_command_line(params)
namespace = {
"initrd_path": initrd_path,
=== modified file 'src/provisioningserver/tests/test_kernel_opts.py'
--- src/provisioningserver/tests/test_kernel_opts.py 2012-09-17 03:16:23 +0000
+++ src/provisioningserver/tests/test_kernel_opts.py 2012-09-17 14:23:22 +0000
@@ -21,7 +21,7 @@
from maastesting.testcase import TestCase
from provisioningserver import kernel_opts
from provisioningserver.kernel_opts import (
- compose_kernel_command_line_new,
+ compose_kernel_command_line,
compose_preseed_opt,
EphemeralImagesDirectoryNotFound,
get_last_directory,
@@ -35,14 +35,12 @@
)
-def make_kernel_parameters(extra_parameters=None):
+def make_kernel_parameters():
"""Make a randomly populated `KernelParameters` instance."""
parms = {
- field: factory.make_name(field)
- for field in KernelParameters._fields
- }
- if extra_parameters is not None:
- parms.update(extra_parameters)
+ field: factory.make_name(field)
+ for field in KernelParameters._fields
+ }
return KernelParameters(**parms)
@@ -71,52 +69,56 @@
params = make_kernel_parameters()
self.assertIn(
"auto url=%s" % params.preseed_url,
- compose_kernel_command_line_new(params))
+ compose_kernel_command_line(params))
def test_install_compose_kernel_command_line_includes_name_domain(self):
- params = make_kernel_parameters({"purpose": "install"})
+ params = make_kernel_parameters()(purpose="install")
self.assertThat(
- compose_kernel_command_line_new(params),
+ compose_kernel_command_line(params),
ContainsAll([
"hostname=%s" % params.hostname,
"domain=%s" % params.domain,
]))
def test_install_compose_kernel_command_line_includes_locale(self):
- params = make_kernel_parameters({"purpose": "install"})
+ params = make_kernel_parameters()(purpose="install")
locale = "en_US"
self.assertIn(
"locale=%s" % locale,
- compose_kernel_command_line_new(params))
+ compose_kernel_command_line(params))
def test_install_compose_kernel_command_line_includes_log_settings(self):
- params = make_kernel_parameters({"purpose": "install"})
+ params = make_kernel_parameters()(purpose="install")
# Port 514 (UDP) is syslog.
log_port = "514"
- text_priority = "critical"
self.assertThat(
- compose_kernel_command_line_new(params),
+ compose_kernel_command_line(params),
ContainsAll([
"log_host=%s" % params.log_host,
"log_port=%s" % log_port,
- "text priority=%s" % text_priority,
]))
+ def test_install_compose_kernel_command_line_includes_di_settings(self):
+ params = make_kernel_parameters()(purpose="install")
+ self.assertThat(
+ compose_kernel_command_line(params),
+ Contains("text priority=critical"))
+
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({"purpose": "install"})
+ params = make_kernel_parameters()(purpose="install")
self.assertIn(
"netcfg/choose_interface=auto",
- compose_kernel_command_line_new(params))
+ compose_kernel_command_line(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)
+ get_ephemeral_name = self.patch(kernel_opts, "get_ephemeral_name")
+ get_ephemeral_name.return_value = "RELEASE-ARCH"
+ params = make_kernel_parameters()(purpose="commissioning")
+ cmdline = compose_kernel_command_line(params)
self.assertThat(
cmdline,
ContainsAll([
@@ -128,22 +130,18 @@
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"
+ get_ephemeral_name = self.patch(kernel_opts, "get_ephemeral_name")
+ 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)
+ params = make_kernel_parameters()(
+ purpose="commissioning", arch="i386")
+ cmdline = compose_kernel_command_line(params)
self.assertThat(cmdline, ContainsAll(expected))
- params = make_kernel_parameters({
- "purpose": "install",
- "arch": "i386",
- })
- cmdline = compose_kernel_command_line_new(params)
+ params = make_kernel_parameters()(
+ purpose="install", arch="i386")
+ cmdline = compose_kernel_command_line(params)
self.assertThat(cmdline, ContainsAll(expected))
def create_ephemeral_info(self, name, arch, release):
@@ -169,12 +167,11 @@
# The result of compose_kernel_command_line includes the purpose
# options for a "commissioning" node.
ephemeral_name = factory.make_name("ephemeral")
- params = make_kernel_parameters()
- params = params._replace(purpose="commissioning")
+ params = make_kernel_parameters()(purpose="commissioning")
self.create_ephemeral_info(
ephemeral_name, params.arch, params.release)
self.assertThat(
- compose_kernel_command_line_new(params),
+ compose_kernel_command_line(params),
ContainsAll([
"iscsi_target_name=%s:%s" % (
ISCSI_TARGET_NAME_PREFIX, ephemeral_name),
@@ -183,14 +180,13 @@
]))
def test_compose_kernel_command_line_reports_error_about_missing_dir(self):
- params = make_kernel_parameters()
- params = params._replace(purpose="commissioning")
+ params = make_kernel_parameters()(purpose="commissioning")
missing_dir = factory.make_name('missing-dir')
config = {"boot": {"ephemeral": {"directory": missing_dir}}}
self.useFixture(ConfigFixture(config))
self.assertRaises(
EphemeralImagesDirectoryNotFound,
- compose_kernel_command_line_new, params)
+ compose_kernel_command_line, params)
def test_compose_preseed_kernel_opt_returns_kernel_option(self):
dummy_preseed_url = factory.make_name("url")
@@ -199,15 +195,13 @@
compose_preseed_opt(dummy_preseed_url))
def test_compose_kernel_command_line_inc_arm_specific_option(self):
- params = make_kernel_parameters()
- params = params._replace(arch="armhf", subarch="highbank")
+ params = make_kernel_parameters()(arch="armhf", subarch="highbank")
self.assertThat(
- compose_kernel_command_line_new(params),
+ compose_kernel_command_line(params),
Contains("console=ttyAMA0"))
def test_compose_kernel_command_line_not_inc_arm_specific_option(self):
- params = make_kernel_parameters()
- params = params._replace(arch="i386")
+ params = make_kernel_parameters()(arch="i386")
self.assertThat(
- compose_kernel_command_line_new(params),
+ compose_kernel_command_line(params),
Not(Contains("console=ttyAMA0")))
