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

Proposed by John A Meinel
Status: Merged
Approved by: John A Meinel
Approved revision: 1291
Merged at revision: 1289
Proposed branch: lp:~jameinel/maas/1.2-kernel-option-tags
Merge into: lp:maas/1.2
Diff against target: 109 lines (+80/-1)
2 files modified
src/maasserver/models/node.py (+24/-1)
src/maasserver/tests/test_node.py (+56/-0)
To merge this branch: bzr merge lp:~jameinel/maas/1.2-kernel-option-tags
Reviewer Review Type Date Requested Status
Martin Packman (community) Approve
Review via email: mp+133228@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

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.

1291. By John A Meinel

Clarify in the docstring that when kernel_options might be None.

Revision history for this message
Martin Packman (gz) wrote :

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
Revision history for this message
John A Meinel (jameinel) wrote :

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.

Revision history for this message
John A Meinel (jameinel) 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.

Revision history for this message
Julian Edwards (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.

Revision history for this message
John A Meinel (jameinel) 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
=:->

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

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

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/models/node.py'
2--- src/maasserver/models/node.py 2012-10-30 15:11:10 +0000
3+++ src/maasserver/models/node.py 2012-11-07 12:51:22 +0000
4@@ -67,7 +67,10 @@
5 get_db_state,
6 strip_domain,
7 )
8-from maasserver.utils.orm import get_first
9+from maasserver.utils.orm import (
10+ get_first,
11+ get_one,
12+ )
13 from piston.models import Token
14 from provisioningserver.enum import (
15 POWER_TYPE,
16@@ -694,6 +697,26 @@
17 else:
18 return None
19
20+ def get_effective_kernel_options(self):
21+ """Determine any special kernel parameters for this node.
22+
23+ :return: (tag, kernel_options)
24+ tag is a Tag object or None. If None, the kernel_options came from
25+ the global setting.
26+ kernel_options, a string indicating extra kernel_options that
27+ should be used when booting this node. May be None if no tags match
28+ and no global setting has been configured.
29+ """
30+ # First, see if there are any tags associated with this node that has a
31+ # custom kernel parameter
32+ tags = self.tags.filter(kernel_opts__isnull=False)
33+ tags = tags.order_by('name')[:1]
34+ tag = get_one(tags)
35+ if tag is not None:
36+ return tag, tag.kernel_opts
37+ global_value = Config.objects.get_config('kernel_opts')
38+ return None, global_value
39+
40 @property
41 def work_queue(self):
42 """The name of the queue for tasks specific to this node."""
43
44=== modified file 'src/maasserver/tests/test_node.py'
45--- src/maasserver/tests/test_node.py 2012-10-30 15:11:10 +0000
46+++ src/maasserver/tests/test_node.py 2012-11-07 12:51:22 +0000
47@@ -303,6 +303,62 @@
48 successful_types = [node_power_types[node] for node in started_nodes]
49 self.assertItemsEqual(configless_power_types, successful_types)
50
51+ def test_get_effective_kernel_options_with_nothing_set(self):
52+ node = factory.make_node()
53+ self.assertEqual((None, None), node.get_effective_kernel_options())
54+
55+ def test_get_effective_kernel_options_sees_global_config(self):
56+ node = factory.make_node()
57+ kernel_opts = factory.getRandomString()
58+ Config.objects.set_config('kernel_opts', kernel_opts)
59+ self.assertEqual(
60+ (None, kernel_opts), node.get_effective_kernel_options())
61+
62+ def test_get_effective_kernel_options_not_confused_by_empty_tag(self):
63+ node = factory.make_node()
64+ tag = factory.make_tag()
65+ node.tags.add(tag)
66+ kernel_opts = factory.getRandomString()
67+ Config.objects.set_config('kernel_opts', kernel_opts)
68+ self.assertEqual(
69+ (None, kernel_opts), node.get_effective_kernel_options())
70+
71+ def test_get_effective_kernel_options_ignores_unassociated_tag_value(self):
72+ node = factory.make_node()
73+ tag = factory.make_tag(kernel_opts=factory.getRandomString())
74+ self.assertEqual((None, None), node.get_effective_kernel_options())
75+
76+ def test_get_effective_kernel_options_uses_tag_value(self):
77+ node = factory.make_node()
78+ tag = factory.make_tag(kernel_opts=factory.getRandomString())
79+ node.tags.add(tag)
80+ self.assertEqual(
81+ (tag, tag.kernel_opts), node.get_effective_kernel_options())
82+
83+ def test_get_effective_kernel_options_tag_overrides_global(self):
84+ node = factory.make_node()
85+ global_opts = factory.getRandomString()
86+ Config.objects.set_config('kernel_opts', global_opts)
87+ tag = factory.make_tag(kernel_opts=factory.getRandomString())
88+ node.tags.add(tag)
89+ self.assertEqual(
90+ (tag, tag.kernel_opts), node.get_effective_kernel_options())
91+
92+ def test_get_effective_kernel_options_uses_first_real_tag_value(self):
93+ node = factory.make_node()
94+ # Intentionally create them in reverse order, so the default 'db' order
95+ # doesn't work, and we have asserted that we sort them.
96+ tag3 = factory.make_tag(factory.make_name('tag-03-'),
97+ kernel_opts=factory.getRandomString())
98+ tag2 = factory.make_tag(factory.make_name('tag-02-'),
99+ kernel_opts=factory.getRandomString())
100+ tag1 = factory.make_tag(factory.make_name('tag-01-'), kernel_opts=None)
101+ self.assertTrue(tag1.name < tag2.name)
102+ self.assertTrue(tag2.name < tag3.name)
103+ node.tags.add(tag1, tag2, tag3)
104+ self.assertEqual(
105+ (tag2, tag2.kernel_opts), node.get_effective_kernel_options())
106+
107 def test_acquire(self):
108 node = factory.make_node(status=NODE_STATUS.READY)
109 user = factory.make_user()

Subscribers

People subscribed via source and target branches

to status/vote changes: