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

[Mike Pontillo]

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)

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.

[Gavin Panella (allenap)]

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)]

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

[lp:~mpontillo/maas/allow-commissioning-non-lts]

5461. By Mike Pontillo on 2016-10-07

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

[Mike Pontillo (mpontillo)]

@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)]

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)]

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.

=== modified file 'src/provisioningserver/config.py'
--- src/provisioningserver/config.py	2016-09-27 22:26:08 +0000
+++ src/provisioningserver/config.py	2016-10-07 18:00:08 +0000
@@ -140,6 +140,7 @@
 from formencode.validators import (
     Number,
     Set,
+    StringBool,
 )
 from provisioningserver.path import get_tentative_path
 from provisioningserver.utils import typed
@@ -735,10 +736,13 @@
             except KeyError:
                 return self.validator.if_missing
             else:
-                return self.validator.from_python(value)
+                return self.validator.to_python(value)
 
     def __set__(self, obj, value):
-        obj.store[self.name] = self.validator.to_python(value)
+        entry = self.validator.from_python(value)
+        # Make sure we'll be able to get the value back out later.
+        self.validator.to_python(entry)
+        obj.store[self.name] = entry
 
     def __delete__(self, obj):
         del obj.store[self.name]
@@ -766,15 +770,19 @@
     tftp_root = ConfigurationOption(
         "tftp_root", "The root directory for TFTP resources.",
         DirectoryString(
-            # Don't validate values that are already stored.
-            accept_python=True, if_missing=get_tentative_path(
+            if_missing=get_tentative_path(
                 "/var/lib/maas/boot-resources/current")))
 
+    allow_commissioning_non_lts = ConfigurationOption(
+        "allow_commissioning_non_lts",
+        "Allow commissioning using non-LTS Ubuntu releases. (Not generally "
+        "supported; only recommended for developers.)",
+        StringBool(if_missing=False))
+
     # GRUB options.
-
     @property
     def grub_root(self):
-        "The root directory for GRUB resources."
+        """The root directory for GRUB resources."""
         return os.path.join(self.tftp_root, "grub")
 
     # NodeGroup UUID Option, used for migrating to rack controller
 
=== modified file 'src/provisioningserver/drivers/osystem/tests/test_ubuntu.py'
--- src/provisioningserver/drivers/osystem/tests/test_ubuntu.py	2016-07-06 05:37:58 +0000
+++ src/provisioningserver/drivers/osystem/tests/test_ubuntu.py	2016-10-07 18:00:08 +0000
@@ -14,6 +14,8 @@
 from maastesting.testcase import MAASTestCase
 from provisioningserver.drivers.osystem import BOOT_IMAGE_PURPOSE
 from provisioningserver.drivers.osystem.ubuntu import UbuntuOS
+from provisioningserver.testing.config import ClusterConfigurationFixture
+from testtools.matchers import Contains
 
 
 class TestUbuntuOS(MAASTestCase):
@@ -57,6 +59,8 @@
         self.assertEqual(expected, self.get_lts_release())
 
     def test_get_supported_commissioning_releases(self):
+        self.useFixture(
+            ClusterConfigurationFixture(allow_commissioning_non_lts=False))
         self.patch_autospec(UbuntuDistroInfo, "is_lts").return_value = True
         self.patch_autospec(UbuntuDistroInfo, "supported").return_value = [
             'precise', 'trusty', 'vivid', 'wily', 'xenial'
@@ -68,6 +72,8 @@
             ['trusty', 'vivid', 'wily', 'xenial'], releases)
 
     def test_get_supported_commissioning_releases_excludes_non_lts(self):
+        self.useFixture(
+            ClusterConfigurationFixture(allow_commissioning_non_lts=False))
         supported = [
             'precise', 'trusty', 'vivid', 'wily', 'xenial'
         ]
@@ -81,8 +87,30 @@
         for release in non_lts_releases:
             self.assertNotIn(release, releases)
 
+    def test_get_supported_commissioning_releases_non_lts_option(self):
+        self.useFixture(
+            ClusterConfigurationFixture(allow_commissioning_non_lts=True))
+        supported = [
+            'precise', 'trusty', 'vivid', 'wily', 'xenial'
+        ]
+        self.patch_autospec(
+            UbuntuDistroInfo, "supported").return_value = supported
+        osystem = UbuntuOS()
+        releases = osystem.get_supported_commissioning_releases()
+        self.assertIsInstance(releases, list)
+        udi = UbuntuDistroInfo()
+        non_lts_releases = [name for name in supported if not udi.is_lts(name)]
+        # With the configuration option set, we should include all non-LTS
+        # releases, plus LTS as well.
+        for release in non_lts_releases:
+            self.assertIn(release, releases)
+        self.assertThat(releases, Contains("trusty"))
+        self.assertThat(releases, Contains("xenial"))
+
     def test_get_supported_commissioning_releases_excludes_precise(self):
         """Make sure we remove 'precise' from the list."""
+        self.useFixture(
+            ClusterConfigurationFixture(allow_commissioning_non_lts=False))
         self.patch_autospec(UbuntuDistroInfo, "supported").return_value = [
             'precise', 'trusty', 'vivid', 'wily', 'xenial'
         ]
@@ -93,6 +121,8 @@
 
     def test_get_supported_commissioning_releases_excludes_unsupported_lts(
             self):
+        self.useFixture(
+            ClusterConfigurationFixture(allow_commissioning_non_lts=False))
         self.patch_autospec(UbuntuDistroInfo, "supported").return_value = [
             'precise', 'trusty', 'vivid', 'wily', 'xenial'
         ]
 
=== modified file 'src/provisioningserver/drivers/osystem/ubuntu.py'
--- src/provisioningserver/drivers/osystem/ubuntu.py	2016-07-06 05:37:58 +0000
+++ src/provisioningserver/drivers/osystem/ubuntu.py	2016-10-07 18:00:08 +0000
@@ -63,9 +63,17 @@
         system that supports commissioning.
         """
         unsupported_releases = ['precise']
-        return [name for name in self.ubuntu_distro_info.supported()
+        with ClusterConfiguration.open() as config:
+            if config.allow_commissioning_non_lts is True:
+                return [
+                    name for name in self.ubuntu_distro_info.supported()
+                    if name not in unsupported_releases
+                ]
+            return [
+                name for name in self.ubuntu_distro_info.supported()
                 if name not in unsupported_releases
-                if self.ubuntu_distro_info.is_lts(name)]
+                if self.ubuntu_distro_info.is_lts(name)
+            ]
 
     def get_default_commissioning_release(self):
         """Gets the default commissioning release for Ubuntu. This only exists
 
=== modified file 'src/provisioningserver/tests/test_config.py'
--- src/provisioningserver/tests/test_config.py	2016-08-11 01:50:21 +0000
+++ src/provisioningserver/tests/test_config.py	2016-10-07 18:00:08 +0000
@@ -212,13 +212,6 @@
         config = self.make_config()
         self.assertIs(sentinel.missing, config.something)
 
-    def test_getting_something_is_not_validated(self):
-        # The value in the database is trusted.
-        config = self.make_config()
-        example_value = factory.make_name('not-an-ip-address')
-        config.store[config.__class__.something.name] = example_value
-        self.assertEqual(example_value, config.something)
-
     def test_setting_something(self):
         config = self.make_config()
         example_value = factory.make_ipv4_address()
 
=== modified file 'src/provisioningserver/utils/config.py'
--- src/provisioningserver/utils/config.py	2016-08-19 10:22:29 +0000
+++ src/provisioningserver/utils/config.py	2016-10-07 18:00:08 +0000
@@ -124,11 +124,12 @@
         "notDir": "%(value)r does not exist or is not a directory",
     }
 
-    def _validate_other(self, value, state=None):
+    def from_python(self, value, state=None):
         # Only validate on the way _in_; it's not the store's fault if it
-        # contains a directory which has since been removed.
-        if os.path.isdir(value):
-            return value
+        # contains a directory which has since been removed. Check that the
+        # original as-Python value represents a directory.
+        if self.is_empty(value) or os.path.isdir(value):
+            return super(DirectoryString, self).from_python(value, state)
         else:
             raise formencode.Invalid(
                 self.message("notDir", state, value=value),
 
=== modified file 'src/provisioningserver/utils/tests/test_config.py'
--- src/provisioningserver/utils/tests/test_config.py	2016-08-19 10:22:29 +0000
+++ src/provisioningserver/utils/tests/test_config.py	2016-10-07 18:00:08 +0000
@@ -146,22 +146,39 @@
 class TestDirectory(MAASTestCase):
     """Tests for `DirectoryString`."""
 
-    def test__validation_succeeds_when_directory_exists(self):
+    def test__converting_succeeds_when_directory_exists(self):
         directory = self.make_dir()
-        validator = config.DirectoryString(accept_python=False)
+        validator = config.DirectoryString()
         self.assertEqual(directory, validator.from_python(directory))
         self.assertEqual(directory, validator.to_python(directory))
 
-    def test__validation_fails_when_directory_does_not_exist(self):
+    def test__converting_from_python_fails_when_directory_not_exists(self):
         directory = os.path.join(self.make_dir(), "not-here")
-        validator = config.DirectoryString(accept_python=False)
+        validator = config.DirectoryString()
         expected_exception = ExpectedException(
             formencode.validators.Invalid, "^%s$" % re.escape(
                 "%r does not exist or is not a directory" % directory))
         with expected_exception:
             validator.from_python(directory)
-        with expected_exception:
-            validator.to_python(directory)
+
+    def test__converting_to_python_succeeds_when_directory_not_exists(self):
+        directory = os.path.join(self.make_dir(), "not-here")
+        validator = config.DirectoryString()
+        self.assertEqual(directory, validator.to_python(directory))
+
+    def test__converting_succeeds_when_empty(self):
+        validator = config.DirectoryString()
+        self.assertIsNone(validator.from_python(None))
+        self.assertIsNone(validator.to_python(None))
+
+    def test__converting_fails_when_empty_and_not_empty_set(self):
+        validator = config.DirectoryString(not_empty=True)
+        expected_exception = ExpectedException(
+            formencode.validators.Invalid, "^Please enter a value$")
+        with expected_exception:
+            self.assertIsNone(validator.from_python(None))
+        with expected_exception:
+            self.assertIsNone(validator.to_python(None))
 
 
 class TestExtendedURL(MAASTestCase):