launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/maas/1.2-pxeconfig-includes-kernel-opts into lp:maas/1.2.

Commit message:
Expose the new kernel options to the pxeconfig api.

Add the extra options to KernelParameters, and return them when the tftp server places its request.
Tweak the tftp server so that it adds the new parameters to the commandline.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jameinel/maas/1.2-pxeconfig-includes-kernel-opts/+merge/133255

This closes the loop, so that nodes should boot with kernel_opts that are set (either in the global config, or once my other patches land in a tag.)

The main downside is that this is technically an API break as 'pxe_config' is now returning extra data in the JSON response, but old clients are strict about what data they accept. (They take the exact JSON back into a dict and pass it as **kwargs to the namedtuple constructor, which requires an exact match.)

If we really care about that, then we can bump the api version or something like that. It doesn't really make sense to add another API call just to get a little bit more data. We already have to configure the one URL in pserv.yaml, it would be silly to add a lot of other config to just make one more request.

-- 
https://code.launchpad.net/~jameinel/maas/1.2-pxeconfig-includes-kernel-opts/+merge/133255
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jameinel/maas/1.2-pxeconfig-includes-kernel-opts into lp:maas/1.2.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-02 09:48:02 +0000
+++ src/maasserver/api.py	2012-11-07 14:11:21 +0000
@@ -1774,6 +1774,13 @@
     else:
         series = node.get_distro_series()
 
+    if node is not None:
+        # We don't care if the kernel opts is from the global setting or a tag,
+        # just get the options
+        _, extra_kernel_opts = node.get_effective_kernel_options()
+    else:
+        extra_kernel_opts = None
+
     purpose = get_boot_purpose(node)
     server_address = get_maas_facing_server_address()
     cluster_address = get_mandatory_param(request.GET, "local")
@@ -1781,7 +1788,8 @@
     params = KernelParameters(
         arch=arch, subarch=subarch, release=series, purpose=purpose,
         hostname=hostname, domain=domain, preseed_url=preseed_url,
-        log_host=server_address, fs_host=cluster_address)
+        log_host=server_address, fs_host=cluster_address,
+        extra_opts=extra_kernel_opts)
 
     return HttpResponse(
         json.dumps(params._asdict()),

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-06 18:19:54 +0000
+++ src/maasserver/tests/test_api.py	2012-11-07 14:11:21 +0000
@@ -3362,6 +3362,16 @@
         kernel_params = KernelParameters(**self.get_pxeconfig(params))
         self.assertEqual(params["local"], kernel_params.fs_host)
 
+    def test_pxeconfig_returns_extra_kernel_options(self):
+        node = factory.make_node()
+        kernel_opts = factory.getRandomString()
+        Config.objects.set_config('kernel_opts', kernel_opts)
+        mac = factory.make_mac_address(node=node)
+        params = self.get_default_params()
+        params['mac'] = mac.mac_address
+        pxe_config = self.get_pxeconfig(params)
+        self.assertEqual(kernel_opts, pxe_config['extra_opts'])
+
 
 class TestNodeGroupsAPI(APIv10TestMixin, MultipleUsersScenarios, TestCase):
     scenarios = [

=== modified file 'src/provisioningserver/kernel_opts.py'
--- src/provisioningserver/kernel_opts.py	2012-10-09 15:43:33 +0000
+++ src/provisioningserver/kernel_opts.py	2012-11-07 14:11:21 +0000
@@ -37,6 +37,8 @@
         "preseed_url",  # URL from which a preseed can be obtained.
         "log_host",  # Host/IP to which syslog can be streamed.
         "fs_host",  # Host/IP on which ephemeral filesystems are hosted.
+        "extra_opts", # String of extra options to supply, will be appended
+                      # verbatim to the kernel command line
         ))
 
 
@@ -176,4 +178,5 @@
     #       as it would be nice to have.
     options += compose_logging_opts(params.log_host)
     options += compose_arch_opts(params)
+    options.append(params.extra_opts)
     return ' '.join(options)

=== modified file 'src/provisioningserver/tests/test_kernel_opts.py'
--- src/provisioningserver/tests/test_kernel_opts.py	2012-10-09 15:39:54 +0000
+++ src/provisioningserver/tests/test_kernel_opts.py	2012-11-07 14:11:21 +0000
@@ -133,6 +133,14 @@
                 "overlayroot=tmpfs",
                 "ip=::::%s:BOOTIF" % params.hostname]))
 
+    def test_commissioning_compose_kernel_command_line_inc_extra_opts(self):
+        extra_opts = "special console=ABCD -- options to pass"
+        params = make_kernel_parameters(extra_opts=extra_opts)
+        cmdline = compose_kernel_command_line(params)
+        # There should be a blank space before the options, but otherwise added
+        # verbatim.
+        self.assertThat(cmdline, Contains(' ' + extra_opts))
+
     def test_compose_kernel_command_line_inc_common_opts(self):
         # Test that some kernel arguments appear on both commissioning
         # and install command lines.