Merge lp:~jameinel/maas/1.2-kernel-option-tags into lp:maas/1.2

[John A Meinel]

Teach Node.get_effective_kernel_options how to handle tags.

This adds the lookup into the tags table for tags applied to this node, which have kernel options set, and in the right sort order.

[John A Meinel (jameinel)]

There *should* be a prereq on the other 1.2 branch I wrote, but the actual patch is still small enough it may not be worth doing.

[Martin Packman (gz)]

Looks good.

+ should be used when booting this node. May be None if no tags match
+ and no global setting has been configured.

I think we should probably ensure the second part of the tuple here is never None. If the global config is not set, we still want some kernel params. Maybe they should live in the maasserver code, or just be the empty string?

+ tags = tags.order_by('name')[:1]
+ tag = get_one(tags)
+ if tag is not None:

I trust this is sensible ORM magic.

review: Approve

[John A Meinel (jameinel)]

I'm not sure what you're thinking on the 'second part'. We can make it the empty string if we would rather not have it be None.

As for the "we still want some kernel params", a lot of the commandline we pass to the booting node is generated from state held in the region controller. So we will always be passing something.

If there are a few particular static parameters that we are passing, we could probably move those into a default global setting (especially if we want to be able to clearly override them). If we do that, though, we'll want some sort of 'reset to defaults' setting, in case you royally hose your system.

[John A Meinel (jameinel)]

> + tags = tags.order_by('name')[:1]
> + tag = get_one(tags)
> + if tag is not None:
>
> I trust this is sensible ORM magic.

The order_by is obvious, the [:1] is how you do a LIMIT 1 request, get_one is a helper that checks that we get either None or exactly 1 object (there is also a get_first helper, though I'm not sure if that gets the LIMIT passed down into the DB. It uses islice instead of [:1], which makes me think it is iterating the cursor/queryset. Which means the DB might have to do all the work just to return 1 item.

[Julian Edwards (julian-edwards)]

On Thursday 08 November 2012 05:05:24 you wrote:
> > + tags = tags.order_by('name')[:1]
> > + tag = get_one(tags)
> > + if tag is not None:
> >
> > I trust this is sensible ORM magic.
>
> The order_by is obvious, the [:1] is how you do a LIMIT 1 request, get_one
> is a helper that checks that we get either None or exactly 1 object (there
> is also a get_first helper, though I'm not sure if that gets the LIMIT
> passed down into the DB. It uses islice instead of [:1], which makes me
> think it is iterating the cursor/queryset. Which means the DB might have to
> do all the work just to return 1 item.

You might want to fix get_first() if you think it's not DTRT, and then use it
(so we're consistent across the code base).

Cheers.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/8/2012 9:28 AM, Julian Edwards wrote:
> On Thursday 08 November 2012 05:05:24 you wrote:
>>> + tags = tags.order_by('name')[:1] + tag = get_one(tags) + if
>>> tag is not None:
>>>
>>> I trust this is sensible ORM magic.
>>
>> The order_by is obvious, the [:1] is how you do a LIMIT 1
>> request, get_one is a helper that checks that we get either None
>> or exactly 1 object (there is also a get_first helper, though I'm
>> not sure if that gets the LIMIT passed down into the DB. It uses
>> islice instead of [:1], which makes me think it is iterating the
>> cursor/queryset. Which means the DB might have to do all the work
>> just to return 1 item.
>
> You might want to fix get_first() if you think it's not DTRT, and
> then use it (so we're consistent across the code base).
>
> Cheers.
>

So I think get_first is intending to support more than one use case.
Namely it is trying to allow you to pass any iterable, not just
something that can be passed a slice argument.

Specifically, the test suite explicitly passes in generators, etc.

So while I'm pretty sure it isn't doing a LIMIT in SQL, the
infrastructure as defined is intentionally not doing so.

Though I will say the only place that uses it in 'maasserver' that
isn't a test would be happy to LIMIT.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlCbR1UACgkQJdeBCYSNAANVlACgge0uQRbpx6czMEqmxbRuya0F
gC4AoL6UcJLRD4UVEGyDreCEJ3E1ojax
=i4Ov
-----END PGP SIGNATURE-----

[Julian Edwards (julian-edwards)]

On Thursday 08 November 2012 05:49:18 you wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 11/8/2012 9:28 AM, Julian Edwards wrote:
> > On Thursday 08 November 2012 05:05:24 you wrote:
> >>> + tags = tags.order_by('name')[:1] + tag = get_one(tags) + if
> >>> tag is not None:
> >>>
> >>> I trust this is sensible ORM magic.
> >>
> >> The order_by is obvious, the [:1] is how you do a LIMIT 1
> >> request, get_one is a helper that checks that we get either None
> >> or exactly 1 object (there is also a get_first helper, though I'm
> >> not sure if that gets the LIMIT passed down into the DB. It uses
> >> islice instead of [:1], which makes me think it is iterating the
> >> cursor/queryset. Which means the DB might have to do all the work
> >> just to return 1 item.
> >
> > You might want to fix get_first() if you think it's not DTRT, and
> > then use it (so we're consistent across the code base).
> >
> > Cheers.
>
> So I think get_first is intending to support more than one use case.
> Namely it is trying to allow you to pass any iterable, not just
> something that can be passed a slice argument.
>
> Specifically, the test suite explicitly passes in generators, etc.
>
> So while I'm pretty sure it isn't doing a LIMIT in SQL, the
> infrastructure as defined is intentionally not doing so.
>
> Though I will say the only place that uses it in 'maasserver' that
> isn't a test would be happy to LIMIT.
>
> John
> =:->

I think we need to work out if it is causing a LIMIT when it generates the
SQL. If not, I think we should make it do what your code does and fix the
tests.

I'd add a comment about get_one near your code so it can be grepped and fixed
later if needs be, don't worry about it in this branch.

Cheers.

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-10-30 15:11:10 +0000
+++ src/maasserver/models/node.py	2012-11-07 12:51:22 +0000
@@ -67,7 +67,10 @@
     get_db_state,
     strip_domain,
     )
-from maasserver.utils.orm import get_first
+from maasserver.utils.orm import (
+    get_first,
+    get_one,
+    )
 from piston.models import Token
 from provisioningserver.enum import (
     POWER_TYPE,
@@ -694,6 +697,26 @@
         else:
             return None
 
+    def get_effective_kernel_options(self):
+        """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')[:1]
+        tag = get_one(tags)
+        if tag is not None:
+            return tag, tag.kernel_opts
+        global_value = Config.objects.get_config('kernel_opts')
+        return None, global_value
+
     @property
     def work_queue(self):
         """The name of the queue for tasks specific to this node."""

=== modified file 'src/maasserver/tests/test_node.py'
--- src/maasserver/tests/test_node.py	2012-10-30 15:11:10 +0000
+++ src/maasserver/tests/test_node.py	2012-11-07 12:51:22 +0000
@@ -303,6 +303,62 @@
         successful_types = [node_power_types[node] for node in started_nodes]
         self.assertItemsEqual(configless_power_types, successful_types)
 
+    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.getRandomString()
+        Config.objects.set_config('kernel_opts', kernel_opts)
+        self.assertEqual(
+            (None, kernel_opts), node.get_effective_kernel_options())
+
+    def test_get_effective_kernel_options_not_confused_by_empty_tag(self):
+        node = factory.make_node()
+        tag = factory.make_tag()
+        node.tags.add(tag)
+        kernel_opts = factory.getRandomString()
+        Config.objects.set_config('kernel_opts', kernel_opts)
+        self.assertEqual(
+            (None, kernel_opts), node.get_effective_kernel_options())
+
+    def test_get_effective_kernel_options_ignores_unassociated_tag_value(self):
+        node = factory.make_node()
+        tag = factory.make_tag(kernel_opts=factory.getRandomString())
+        self.assertEqual((None, None), 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.getRandomString())
+        node.tags.add(tag)
+        self.assertEqual(
+            (tag, tag.kernel_opts), node.get_effective_kernel_options())
+
+    def test_get_effective_kernel_options_tag_overrides_global(self):
+        node = factory.make_node()
+        global_opts = factory.getRandomString()
+        Config.objects.set_config('kernel_opts', global_opts)
+        tag = factory.make_tag(kernel_opts=factory.getRandomString())
+        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.getRandomString())
+        tag2 = factory.make_tag(factory.make_name('tag-02-'),
+                                kernel_opts=factory.getRandomString())
+        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())
+
     def test_acquire(self):
         node = factory.make_node(status=NODE_STATUS.READY)
         user = factory.make_user()