Merge ~ltrager/maas:lp1927657 into maas:master

[Lee Trager]

LP: #1927657 - Always include the default kernel command line options

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1927657 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10026/console
COMMIT: fd34ce29a2f4bea9b7bb5d176c76df907428284f

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1927657 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 72cda3925c38e338072361613eb9242eb1d63c7d

review: Approve

[Alberto Donato (ack)]

The current behavior was a deliberate change (see 7278a2491efb980181bb6a48f44faa87054385db).

Before that change, if you had a machine tagged with multiple tags with kernel cmdline, the first one alphabetically was chosen.
This wasn't very usable, as it didn't allow to combine tags in a usable way.

With the current behavior, all tags are collected (so you can have for tags for different purposes, e.g. serial console, configure hugepages, ...).
Since we now combine them, the global default is much less useful (it's mainly useful if you want parameters to be applied during enlistment), as it's easy to add an extra tag for what you need and set it on the machines you need.

review: Needs Information

[Lee Trager (ltrager)]

It was an improvement to include kernel options for all tags but it still leaves out the global kernel command line option. This includes everything. The global kernel command line options are defined first. This allows tags to override the kernel command line option as the kernel uses the last definition.

I've verified this by setting my kernel command line to the following and still having the system boot properly
BOOT_IMAGE=/boot/vmlinuz-5.4.0-73-generic root=/dev/null root=/dev/mapper/vg0-lv0 ro

I ran into this bug while testing UEFI SecureBoot with an LXD VM. I needed to add "earlyprintk=efi" to the kernel command line to gather additional debug information. The UI has no way to add a tag with a kernel option, the only way to do a kernel option in the UI is to add a global kernel command line option. The UI states "Boot parameters to pass to the kernel by default" which led me to believe it would always be added. When I deployed Ubuntu on an LXD VM my kernel command line option wasn't added. This was because MAAS automatically tags LXD VMs with "pod-console-logging" which contains a kernel command line option and any tag with a kernel option overrides the global default.

I feel this is safe to land because users can always override the global kernel command line option with a tag. It also makes it much clearer to always add all options instead of sometimes adding the global option and sometimes not.

[Alberto Donato (ack)]

Sounds good, +1

I think we should change the term "default" in the UI/doc with "global", to make it clear they're always applied, and also perhaps clarify the logic in the doc about how is the command line composed.

A small nit inline

review: Approve

[Lee Trager (ltrager)]

Thanks for the review. I've filed LP:1928253 to fix the wording in the UI

[MAAS Lander (maas-lander)]

LANDING
-b lp1927657 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10058/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1927657 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10059/console
COMMIT: 804938e78180d8b2dfaff13bb123c6376dd4525c

review: Needs Fixing

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index a1feb04..0fda60b 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -3008,16 +3008,16 @@ class Node(CleanSave, TimestampedModel):
             raise UnknownPowerType("Node power type is unconfigured")
         return self.bmc.power_type
 
-    def get_effective_kernel_options(self, default_kernel_opts=""):
+    def get_effective_kernel_options(self, default_kernel_opts=None):
         """Return a string with kernel commandline."""
-        options = (
+        options = list(
             self.tags.exclude(kernel_opts="")
             .order_by("name")
             .values_list("kernel_opts", flat=True)
         )
-        if options:
-            return " ".join(options)
-        return default_kernel_opts or ""
+        if default_kernel_opts:
+            options.insert(0, default_kernel_opts)
+        return " ".join(options)
 
     def get_osystem(self, default=undefined):
         """Return the operating system to install that node."""
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 605b238..669eeb4 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -2040,7 +2040,7 @@ class TestNode(MAASServerTestCase):
         node.tags.add(tag)
         self.assertEqual(node.get_effective_kernel_options(), tag.kernel_opts)
 
-    def test_get_effective_kernel_options_tag_overrides_default(self):
+    def test_get_effective_kernel_options_tag_includes_default(self):
         node = factory.make_Node()
         default_opts = factory.make_string()
         tag = factory.make_Tag(kernel_opts=factory.make_string())
@@ -2049,7 +2049,7 @@ class TestNode(MAASServerTestCase):
             node.get_effective_kernel_options(
                 default_kernel_opts=default_opts
             ),
-            tag.kernel_opts,
+            f"{default_opts} {tag.kernel_opts}",
         )
 
     def test_acquire(self):