Merge ~ack/maas:kernel-options-all-tags into maas:master

[Alberto Donato]

combine kernel commandline options node from all associated tags

[MAAS Lander (maas-lander)]

UNIT TESTS
-b kernel-options-all-tags lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

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

review: Needs Fixing

[Adam Collard (adam-collard)]

Make sure we document the order in which we combine kernel tags and nod towards that being important if the options aren't complete.

review: Approve

diff --git a/src/maasserver/api/examples/tags.json b/src/maasserver/api/examples/tags.json
index 98804ed..504001d 100644
--- a/src/maasserver/api/examples/tags.json
+++ b/src/maasserver/api/examples/tags.json
@@ -4,7 +4,7 @@
       "name": "virtual",
       "definition": "",
       "comment": "",
-      "kernel_opts": null,
+      "kernel_opts": "",
       "resource_uri": "/MAAS/api/2.0/tags/virtual/"
     }
   ],
@@ -12,7 +12,7 @@
     "name": "virtual",
     "definition": "",
     "comment": "",
-    "kernel_opts": null,
+    "kernel_opts": "",
     "resource_uri": "/MAAS/api/2.0/tags/virtual/"
   },
   "get-devices-by-tag": [
diff --git a/src/maasserver/api/tags.py b/src/maasserver/api/tags.py
index 42c8466..b4ce8eb 100644
--- a/src/maasserver/api/tags.py
+++ b/src/maasserver/api/tags.py
@@ -420,9 +420,9 @@ class TagsHandler(OperationsHandler):
         @param (string) "kernel_opts" [required=false] Nodes associated
         with this tag will add this string to their kernel options
         when booting. The value overrides the global ``kernel_opts``
-        setting. If more than one tag is associated with a node, the
-        one with the lower alphabetical name will be picked. For example,
-        ``01-my-tag`` will be chosen instead of ``99-tag-name``.
+        setting. If more than one tag is associated with a node, command
+        line will be concatenated from all associated tags, in alphabetic
+        tag name order.
         @param-example "kernel_opts"
             nouveau.noaccel=1
 
diff --git a/src/maasserver/migrations/maasserver/0230_tag_kernel_opts_blank_instead_of_null.py b/src/maasserver/migrations/maasserver/0230_tag_kernel_opts_blank_instead_of_null.py
new file mode 100644
index 0000000..397b945
--- /dev/null
+++ b/src/maasserver/migrations/maasserver/0230_tag_kernel_opts_blank_instead_of_null.py
@@ -0,0 +1,19 @@
+# Generated by Django 2.2.12 on 2021-03-05 10:56
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("maasserver", "0229_drop_physicalblockdevice_storage_pool"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="tag",
+            name="kernel_opts",
+            field=models.TextField(blank=True, default=""),
+            preserve_default=False,
+        ),
+    ]
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index a7c41d5..641e2d8 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -3024,26 +3024,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=undefined):
-        """Determine any special kernel parameters for this node.
-
-        :return: (tag, kernel_options)
-            tag is a Tag object or None. If None, the kernel_options came from
-            the global setting.
-            kernel_options, a string indicating extra kernel_options that
-            should be used when booting this node. May be None if no tags match
-            and no global setting has been configured.
-        """
-        # First, see if there are any tags associated with this node that has a
-        # custom kernel parameter
-        tags = self.tags.filter(kernel_opts__isnull=False)
-        tags = tags.order_by("name")
-        for tag in tags:
-            if tag.kernel_opts != "":
-                return tag, tag.kernel_opts
-        if default_kernel_opts is undefined:
-            default_kernel_opts = Config.objects.get_config("kernel_opts")
-        return None, default_kernel_opts
+    def get_effective_kernel_options(self, default_kernel_opts=""):
+        """Return a string with kernel commandline."""
+        options = (
+            self.tags.exclude(kernel_opts="")
+            .order_by("name")
+            .values_list("kernel_opts", flat=True)
+        )
+        if options:
+            return " ".join(options)
+        return default_kernel_opts or ""
 
     def get_osystem(self, default=undefined):
         """Return the operating system to install that node."""
diff --git a/src/maasserver/models/tag.py b/src/maasserver/models/tag.py
index 0a49b99..5806b3d 100644
--- a/src/maasserver/models/tag.py
+++ b/src/maasserver/models/tag.py
@@ -71,7 +71,7 @@ class Tag(CleanSave, TimestampedModel):
     )
     definition = TextField(blank=True)
     comment = TextField(blank=True)
-    kernel_opts = TextField(blank=True, null=True)
+    kernel_opts = TextField(blank=True)
 
     objects = TagManager()
 
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index d77a37d..954740f 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -1818,24 +1818,16 @@ class TestNode(MAASServerTestCase):
 
     def test_get_effective_kernel_options_with_nothing_set(self):
         node = factory.make_Node()
-        self.assertEqual((None, None), node.get_effective_kernel_options())
-
-    def test_get_effective_kernel_options_sees_global_config(self):
-        node = factory.make_Node()
-        kernel_opts = factory.make_string()
-        Config.objects.set_config("kernel_opts", kernel_opts)
-        self.assertEqual(
-            (None, kernel_opts), node.get_effective_kernel_options()
-        )
+        self.assertEqual(node.get_effective_kernel_options(), "")
 
     def test_get_effective_kernel_options_not_confused_by_None_opts(self):
         node = factory.make_Node()
         tag = factory.make_Tag()
         node.tags.add(tag)
         kernel_opts = factory.make_string()
-        Config.objects.set_config("kernel_opts", kernel_opts)
         self.assertEqual(
-            (None, kernel_opts), node.get_effective_kernel_options()
+            node.get_effective_kernel_options(default_kernel_opts=kernel_opts),
+            kernel_opts,
         )
 
     def test_get_effective_kernel_options_not_confused_by_empty_str_opts(self):
@@ -1843,69 +1835,44 @@ class TestNode(MAASServerTestCase):
         tag = factory.make_Tag(kernel_opts="")
         node.tags.add(tag)
         kernel_opts = factory.make_string()
-        Config.objects.set_config("kernel_opts", kernel_opts)
         self.assertEqual(
-            (None, kernel_opts), node.get_effective_kernel_options()
+            node.get_effective_kernel_options(default_kernel_opts=kernel_opts),
+            kernel_opts,
         )
 
     def test_get_effective_kernel_options_multiple_tags_with_opts(self):
-        # In this scenario:
-        #     global   kernel_opts='fish-n-chips'
-        #     tag_a    kernel_opts=null
-        #     tag_b    kernel_opts=''
-        #     tag_c    kernel_opts='bacon-n-eggs'
-        # we require that 'bacon-n-eggs' is chosen as it is the first
-        # tag with a valid kernel option.
-        Config.objects.set_config("kernel_opts", "fish-n-chips")
         node = factory.make_Node()
         node.tags.add(factory.make_Tag("tag_a"))
         node.tags.add(factory.make_Tag("tag_b", kernel_opts=""))
         tag_c = factory.make_Tag("tag_c", kernel_opts="bacon-n-eggs")
+        tag_d = factory.make_Tag("tag_d", kernel_opts="foo-bar")
         node.tags.add(tag_c)
-
+        node.tags.add(tag_d)
         self.assertEqual(
-            (tag_c, "bacon-n-eggs"), node.get_effective_kernel_options()
+            node.get_effective_kernel_options(), "bacon-n-eggs foo-bar"
         )
 
     def test_get_effective_kernel_options_ignores_unassociated_tag_value(self):
         node = factory.make_Node()
         factory.make_Tag(kernel_opts=factory.make_string())
-        self.assertEqual((None, None), node.get_effective_kernel_options())
+        self.assertEqual(node.get_effective_kernel_options(), "")
 
     def test_get_effective_kernel_options_uses_tag_value(self):
         node = factory.make_Node()
         tag = factory.make_Tag(kernel_opts=factory.make_string())
         node.tags.add(tag)
-        self.assertEqual(
-            (tag, tag.kernel_opts), node.get_effective_kernel_options()
-        )
+        self.assertEqual(node.get_effective_kernel_options(), tag.kernel_opts)
 
-    def test_get_effective_kernel_options_tag_overrides_global(self):
+    def test_get_effective_kernel_options_tag_overrides_default(self):
         node = factory.make_Node()
-        global_opts = factory.make_string()
-        Config.objects.set_config("kernel_opts", global_opts)
+        default_opts = factory.make_string()
         tag = factory.make_Tag(kernel_opts=factory.make_string())
         node.tags.add(tag)
         self.assertEqual(
-            (tag, tag.kernel_opts), node.get_effective_kernel_options()
-        )
-
-    def test_get_effective_kernel_options_uses_first_real_tag_value(self):
-        node = factory.make_Node()
-        # Intentionally create them in reverse order, so the default 'db' order
-        # doesn't work, and we have asserted that we sort them.
-        tag3 = factory.make_Tag(
-            factory.make_name("tag-03-"), kernel_opts=factory.make_string()
-        )
-        tag2 = factory.make_Tag(
-            factory.make_name("tag-02-"), kernel_opts=factory.make_string()
-        )
-        tag1 = factory.make_Tag(factory.make_name("tag-01-"), kernel_opts=None)
-        self.assertTrue(tag1.name < tag2.name)
-        self.assertTrue(tag2.name < tag3.name)
-        node.tags.add(tag1, tag2, tag3)
-        self.assertEqual(
-            (tag2, tag2.kernel_opts), node.get_effective_kernel_options()
+            node.get_effective_kernel_options(
+                default_kernel_opts=default_opts
+            ),
+            tag.kernel_opts,
         )
 
     def test_acquire(self):
diff --git a/src/maasserver/models/tests/test_tag.py b/src/maasserver/models/tests/test_tag.py
index 1a0666f..fdf7b58 100644
--- a/src/maasserver/models/tests/test_tag.py
+++ b/src/maasserver/models/tests/test_tag.py
@@ -28,7 +28,7 @@ class TagTest(MAASServerTestCase):
         self.assertEqual("tag-name", tag.name)
         self.assertEqual("//node[@id=display]", tag.definition)
         self.assertEqual("", tag.comment)
-        self.assertIs(None, tag.kernel_opts)
+        self.assertEqual(tag.kernel_opts, "")
         self.assertIsNot(None, tag.updated)
         self.assertIsNot(None, tag.created)
 
diff --git a/src/maasserver/rpc/boot.py b/src/maasserver/rpc/boot.py
index fdd9ef7..ce7af59 100644
--- a/src/maasserver/rpc/boot.py
+++ b/src/maasserver/rpc/boot.py
@@ -515,9 +515,7 @@ def get_config(
             machine, configs, purpose
         )
 
-        # We don't care if the kernel opts is from the global setting or a tag,
-        # just get the options
-        _, effective_kernel_opts = machine.get_effective_kernel_options(
+        extra_kernel_opts = machine.get_effective_kernel_options(
             default_kernel_opts=configs["kernel_opts"]
         )
 
@@ -527,21 +525,9 @@ def get_config(
             driver = get_third_party_driver(machine, series=series)
             driver_kernel_opts = driver.get("kernel_opts", "")
 
-            combined_opts = (
-                "%s %s"
-                % (
-                    ""
-                    if effective_kernel_opts is None
-                    else effective_kernel_opts,
-                    driver_kernel_opts,
-                )
-            ).strip()
-            if len(combined_opts):
-                extra_kernel_opts = combined_opts
-            else:
-                extra_kernel_opts = None
-        else:
-            extra_kernel_opts = effective_kernel_opts
+            extra_kernel_opts += f" {driver_kernel_opts}"
+
+        extra_kernel_opts.strip()
 
         kparams = BootResource.objects.get_kparams_for_node(
             machine,
diff --git a/src/maasserver/testing/factory.py b/src/maasserver/testing/factory.py
index 94a156d..d5bc9ea 100644
--- a/src/maasserver/testing/factory.py
+++ b/src/maasserver/testing/factory.py
@@ -1930,7 +1930,7 @@ class Factory(maastesting.factory.Factory):
         name=None,
         definition=None,
         comment="",
-        kernel_opts=None,
+        kernel_opts="",
         created=None,
         updated=None,
         populate=True,