Merge lp:~mpontillo/maas/allow-commissioning-non-lts into lp:maas/trunk

Proposed by Mike Pontillo
Status: Rejected
Rejected by: MAAS Lander
Proposed branch: lp:~mpontillo/maas/allow-commissioning-non-lts
Merge into: lp:maas/trunk
Diff against target: 235 lines (+82/-25)
6 files modified
src/provisioningserver/config.py (+14/-6)
src/provisioningserver/drivers/osystem/tests/test_ubuntu.py (+30/-0)
src/provisioningserver/drivers/osystem/ubuntu.py (+10/-2)
src/provisioningserver/tests/test_config.py (+0/-7)
src/provisioningserver/utils/config.py (+5/-4)
src/provisioningserver/utils/tests/test_config.py (+23/-6)
To merge this branch: bzr merge lp:~mpontillo/maas/allow-commissioning-non-lts
Reviewer Review Type Date Requested Status
Andres Rodriguez (community) Needs Information
Gavin Panella (community) Approve
Review via email: mp+307904@code.launchpad.net

Commit message

Add option to rackd.conf that allows commissioning on non-LTS Ubuntu releases.

 * New option: allow_commissioning_non_lts: <bool>
 * Drive-by fix for a bug that prevented usages of some validators
   in the configuration subsystem, such as the StringBool.
   (values were being returned as strings rather than bool)

Description of the change

If someone wants to test with a development release, currently they need to hack the MAAS code. This change allows a simple rackd.conf change (followed by reloading the global settings page) to relax the LTS-only restriction and allow that testing.

To post a comment you must log in.
Gavin Panella (allenap) wrote :

Fixing the to_python/from_python mix-up is great, but it's had some fallout. Try running bin/test.rack in full and you'll see what I mean. I have a fix for you though: https://code.launchpad.net/~allenap/maas/mpontillo--allow-commissioning-non-lts/+merge/307918. I haven't run bin/test.region though; life's too short for that ;)

I think the code here is grand, but I wonder if it's a good idea to put this into the rack configuration file. I'm not against the principle but I wonder if it will get abused. In that regard it's good that it cannot be configured with `maas-rack config`, and I think it should stay that way. My concern is probably premature though: let's see if we get bugs arising from misuse of this option and then consider what to do.

One question: on the region, when compiling the list of commissioning releases, do we intersect the responses from all racks? This setting may then be confusing in environments with multiple rack controllers, because non-LTS releases won't appear until all racks have been configured to allow them.

review: Approve
Andres Rodriguez (andreserl) wrote :

What is the desire to have this configuration option ?

We made an agreement long ago to not allow commissioning with any other release than the LTS. This allows us to control the environment and prevents users from running into unsupported paths. We support hardware enablement kernels and as such, we should be allowing that instead of adding this option.

That said, it is not a good idea to put it in a configuration file, because, yes, it will get abused. We specifically did things the way we did for particular reasons. We should not be providing a path to anyone to do things different.

review: Needs Information
5461. By Mike Pontillo on 2016-10-07

Merge allenap's fixes from lp:~allenap/maas/mpontillo--allow-commissioning-non-lts.

Mike Pontillo (mpontillo) wrote :

@allenap, thanks for the fixes; I was going to ask you about that. I removed one of the test cases in that file because I wasn't sure that it was correct for the configuration store to allow an invalid directory to be returned. It seemed to me like we should catch that at the earliest possible time, so we don't propagate a directory that doesn't exist throughout the code and try to use it everywhere. But I can see the argument for both ways, and certainly there is less code change (and exception handling changes) required if we leave that scenario the way it is.

Also, the `maas-rack config` command has intentionally NOT been updated. Users will never see this option unless they look at the code.

I was curious about what we do in a multi-rack scenario, too. I didn't dwell on that too much, because this was strictly intended as an unsupported feature for QA/developers, and I figured if it misbehaved it's not the end of the world.

@Andres, I understand your concerns. But I think this feature is sufficiently hidden, and that the average MAAS user won't stumble upon it and try it out (at their peril). The only way they can enable this is to (1) know it exists, and (2) add it to their rackd.conf manually. If they found it by looking at the code, it should be clear from the surrounding text that it is NOT SUPPORTED and NOT RECOMMENDED. (I can call that out in clearer language, if you like.)

The reason you want to have this in a configuration file is because it makes it easier to do QA against MAAS; especially integration testing against the latest MAAS images. With this configuration option available, we can more easily test development releases in a CI environment, and thus catch problems that will occur with the *next* LTS sooner rather than later.

To be honest, I did this branch because I wanted to scratch an itch. I was trying to test out changes to the Yakkety images, and I was annoyed that I had to figure out where to hack the code each time I wanted to do that.

In conclusion, this should be something that can be easily enabled for QA or development purposes -- without hacking the Python files in the MAAS package, or using other workarounds.

Mike Pontillo (mpontillo) wrote :

I forgot to add: the other reason you want to have this is for supportability. If suddenly the LTS no longer commissions but a non-LTS image is fine (or already has the fix, whatever the case may be), you want to have a way to allow users to use a non-LTS image in an emergency situation.

The other idea I had was, we could rename the option to "allow_commissioning_unsupported_releases", just to make it crystal clear that users who do this will get no support.

MAAS Lander (maas-lander) wrote :

Transitioned to Git.

lp:maas has now moved from Bzr to Git.
Please propose your branches with Launchpad using Git.

git clone https://git.launchpad.net/maas

Unmerged revisions

5461. By Mike Pontillo on 2016-10-07

Merge allenap's fixes from lp:~allenap/maas/mpontillo--allow-commissioning-non-lts.

5460. By Mike Pontillo on 2016-10-07

Fix tests, address incorrect assumption.

5459. By Mike Pontillo on 2016-10-07

Add configuraiton option for commissioning on a non-LTS release. Drive-by fix for a problem that prevented correct usage of python conversions in rackd.conf.

5458. By Mike Pontillo on 2016-10-07

Add a constant to allow commissioning non-LTS releases.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/config.py'
2--- src/provisioningserver/config.py 2016-09-27 22:26:08 +0000
3+++ src/provisioningserver/config.py 2016-10-07 18:00:08 +0000
4@@ -140,6 +140,7 @@
5 from formencode.validators import (
6 Number,
7 Set,
8+ StringBool,
9 )
10 from provisioningserver.path import get_tentative_path
11 from provisioningserver.utils import typed
12@@ -735,10 +736,13 @@
13 except KeyError:
14 return self.validator.if_missing
15 else:
16- return self.validator.from_python(value)
17+ return self.validator.to_python(value)
18
19 def __set__(self, obj, value):
20- obj.store[self.name] = self.validator.to_python(value)
21+ entry = self.validator.from_python(value)
22+ # Make sure we'll be able to get the value back out later.
23+ self.validator.to_python(entry)
24+ obj.store[self.name] = entry
25
26 def __delete__(self, obj):
27 del obj.store[self.name]
28@@ -766,15 +770,19 @@
29 tftp_root = ConfigurationOption(
30 "tftp_root", "The root directory for TFTP resources.",
31 DirectoryString(
32- # Don't validate values that are already stored.
33- accept_python=True, if_missing=get_tentative_path(
34+ if_missing=get_tentative_path(
35 "/var/lib/maas/boot-resources/current")))
36
37+ allow_commissioning_non_lts = ConfigurationOption(
38+ "allow_commissioning_non_lts",
39+ "Allow commissioning using non-LTS Ubuntu releases. (Not generally "
40+ "supported; only recommended for developers.)",
41+ StringBool(if_missing=False))
42+
43 # GRUB options.
44-
45 @property
46 def grub_root(self):
47- "The root directory for GRUB resources."
48+ """The root directory for GRUB resources."""
49 return os.path.join(self.tftp_root, "grub")
50
51 # NodeGroup UUID Option, used for migrating to rack controller
52
53=== modified file 'src/provisioningserver/drivers/osystem/tests/test_ubuntu.py'
54--- src/provisioningserver/drivers/osystem/tests/test_ubuntu.py 2016-07-06 05:37:58 +0000
55+++ src/provisioningserver/drivers/osystem/tests/test_ubuntu.py 2016-10-07 18:00:08 +0000
56@@ -14,6 +14,8 @@
57 from maastesting.testcase import MAASTestCase
58 from provisioningserver.drivers.osystem import BOOT_IMAGE_PURPOSE
59 from provisioningserver.drivers.osystem.ubuntu import UbuntuOS
60+from provisioningserver.testing.config import ClusterConfigurationFixture
61+from testtools.matchers import Contains
62
63
64 class TestUbuntuOS(MAASTestCase):
65@@ -57,6 +59,8 @@
66 self.assertEqual(expected, self.get_lts_release())
67
68 def test_get_supported_commissioning_releases(self):
69+ self.useFixture(
70+ ClusterConfigurationFixture(allow_commissioning_non_lts=False))
71 self.patch_autospec(UbuntuDistroInfo, "is_lts").return_value = True
72 self.patch_autospec(UbuntuDistroInfo, "supported").return_value = [
73 'precise', 'trusty', 'vivid', 'wily', 'xenial'
74@@ -68,6 +72,8 @@
75 ['trusty', 'vivid', 'wily', 'xenial'], releases)
76
77 def test_get_supported_commissioning_releases_excludes_non_lts(self):
78+ self.useFixture(
79+ ClusterConfigurationFixture(allow_commissioning_non_lts=False))
80 supported = [
81 'precise', 'trusty', 'vivid', 'wily', 'xenial'
82 ]
83@@ -81,8 +87,30 @@
84 for release in non_lts_releases:
85 self.assertNotIn(release, releases)
86
87+ def test_get_supported_commissioning_releases_non_lts_option(self):
88+ self.useFixture(
89+ ClusterConfigurationFixture(allow_commissioning_non_lts=True))
90+ supported = [
91+ 'precise', 'trusty', 'vivid', 'wily', 'xenial'
92+ ]
93+ self.patch_autospec(
94+ UbuntuDistroInfo, "supported").return_value = supported
95+ osystem = UbuntuOS()
96+ releases = osystem.get_supported_commissioning_releases()
97+ self.assertIsInstance(releases, list)
98+ udi = UbuntuDistroInfo()
99+ non_lts_releases = [name for name in supported if not udi.is_lts(name)]
100+ # With the configuration option set, we should include all non-LTS
101+ # releases, plus LTS as well.
102+ for release in non_lts_releases:
103+ self.assertIn(release, releases)
104+ self.assertThat(releases, Contains("trusty"))
105+ self.assertThat(releases, Contains("xenial"))
106+
107 def test_get_supported_commissioning_releases_excludes_precise(self):
108 """Make sure we remove 'precise' from the list."""
109+ self.useFixture(
110+ ClusterConfigurationFixture(allow_commissioning_non_lts=False))
111 self.patch_autospec(UbuntuDistroInfo, "supported").return_value = [
112 'precise', 'trusty', 'vivid', 'wily', 'xenial'
113 ]
114@@ -93,6 +121,8 @@
115
116 def test_get_supported_commissioning_releases_excludes_unsupported_lts(
117 self):
118+ self.useFixture(
119+ ClusterConfigurationFixture(allow_commissioning_non_lts=False))
120 self.patch_autospec(UbuntuDistroInfo, "supported").return_value = [
121 'precise', 'trusty', 'vivid', 'wily', 'xenial'
122 ]
123
124=== modified file 'src/provisioningserver/drivers/osystem/ubuntu.py'
125--- src/provisioningserver/drivers/osystem/ubuntu.py 2016-07-06 05:37:58 +0000
126+++ src/provisioningserver/drivers/osystem/ubuntu.py 2016-10-07 18:00:08 +0000
127@@ -63,9 +63,17 @@
128 system that supports commissioning.
129 """
130 unsupported_releases = ['precise']
131- return [name for name in self.ubuntu_distro_info.supported()
132+ with ClusterConfiguration.open() as config:
133+ if config.allow_commissioning_non_lts is True:
134+ return [
135+ name for name in self.ubuntu_distro_info.supported()
136+ if name not in unsupported_releases
137+ ]
138+ return [
139+ name for name in self.ubuntu_distro_info.supported()
140 if name not in unsupported_releases
141- if self.ubuntu_distro_info.is_lts(name)]
142+ if self.ubuntu_distro_info.is_lts(name)
143+ ]
144
145 def get_default_commissioning_release(self):
146 """Gets the default commissioning release for Ubuntu. This only exists
147
148=== modified file 'src/provisioningserver/tests/test_config.py'
149--- src/provisioningserver/tests/test_config.py 2016-08-11 01:50:21 +0000
150+++ src/provisioningserver/tests/test_config.py 2016-10-07 18:00:08 +0000
151@@ -212,13 +212,6 @@
152 config = self.make_config()
153 self.assertIs(sentinel.missing, config.something)
154
155- def test_getting_something_is_not_validated(self):
156- # The value in the database is trusted.
157- config = self.make_config()
158- example_value = factory.make_name('not-an-ip-address')
159- config.store[config.__class__.something.name] = example_value
160- self.assertEqual(example_value, config.something)
161-
162 def test_setting_something(self):
163 config = self.make_config()
164 example_value = factory.make_ipv4_address()
165
166=== modified file 'src/provisioningserver/utils/config.py'
167--- src/provisioningserver/utils/config.py 2016-08-19 10:22:29 +0000
168+++ src/provisioningserver/utils/config.py 2016-10-07 18:00:08 +0000
169@@ -124,11 +124,12 @@
170 "notDir": "%(value)r does not exist or is not a directory",
171 }
172
173- def _validate_other(self, value, state=None):
174+ def from_python(self, value, state=None):
175 # Only validate on the way _in_; it's not the store's fault if it
176- # contains a directory which has since been removed.
177- if os.path.isdir(value):
178- return value
179+ # contains a directory which has since been removed. Check that the
180+ # original as-Python value represents a directory.
181+ if self.is_empty(value) or os.path.isdir(value):
182+ return super(DirectoryString, self).from_python(value, state)
183 else:
184 raise formencode.Invalid(
185 self.message("notDir", state, value=value),
186
187=== modified file 'src/provisioningserver/utils/tests/test_config.py'
188--- src/provisioningserver/utils/tests/test_config.py 2016-08-19 10:22:29 +0000
189+++ src/provisioningserver/utils/tests/test_config.py 2016-10-07 18:00:08 +0000
190@@ -146,22 +146,39 @@
191 class TestDirectory(MAASTestCase):
192 """Tests for `DirectoryString`."""
193
194- def test__validation_succeeds_when_directory_exists(self):
195+ def test__converting_succeeds_when_directory_exists(self):
196 directory = self.make_dir()
197- validator = config.DirectoryString(accept_python=False)
198+ validator = config.DirectoryString()
199 self.assertEqual(directory, validator.from_python(directory))
200 self.assertEqual(directory, validator.to_python(directory))
201
202- def test__validation_fails_when_directory_does_not_exist(self):
203+ def test__converting_from_python_fails_when_directory_not_exists(self):
204 directory = os.path.join(self.make_dir(), "not-here")
205- validator = config.DirectoryString(accept_python=False)
206+ validator = config.DirectoryString()
207 expected_exception = ExpectedException(
208 formencode.validators.Invalid, "^%s$" % re.escape(
209 "%r does not exist or is not a directory" % directory))
210 with expected_exception:
211 validator.from_python(directory)
212- with expected_exception:
213- validator.to_python(directory)
214+
215+ def test__converting_to_python_succeeds_when_directory_not_exists(self):
216+ directory = os.path.join(self.make_dir(), "not-here")
217+ validator = config.DirectoryString()
218+ self.assertEqual(directory, validator.to_python(directory))
219+
220+ def test__converting_succeeds_when_empty(self):
221+ validator = config.DirectoryString()
222+ self.assertIsNone(validator.from_python(None))
223+ self.assertIsNone(validator.to_python(None))
224+
225+ def test__converting_fails_when_empty_and_not_empty_set(self):
226+ validator = config.DirectoryString(not_empty=True)
227+ expected_exception = ExpectedException(
228+ formencode.validators.Invalid, "^Please enter a value$")
229+ with expected_exception:
230+ self.assertIsNone(validator.from_python(None))
231+ with expected_exception:
232+ self.assertIsNone(validator.to_python(None))
233
234
235 class TestExtendedURL(MAASTestCase):