Merge lp:~blake-rouse/maas/osystem-preseed-cleanup into lp:~maas-committers/maas/trunk

[Blake Rouse]

Removed DISTRO_SERIES enums. Added the ability for an operating system to compose its own preseed.

This is the final change in the series of changes to allow MAAS to deploy other operating systems. As we have Windows and CentOS support coming soon this is needed to easily add new operating systems.

Removed the DISTRO_SERIES enums as they are no longer needed, all information comes from the OperatingSystemRegistry.

Added the ability for an operating system to compose its own preseed. This will be used for Windows, CentOS and other operating systems.

[Jeroen T. Vermeulen (jtv)]

Thanks for doing this work. It's good to see those old hard-coded enums go at last. Unfortunately there's that pervasive problem of the registries living in a different process, possibly on a different machine. We should probably have been a lot clearer about that.

This branch answers a question I asked on one of your other merge proposals: does OperatingSystem really need to be a base class? If it's going to have a method for generating preseed data, and if it really can't be addressed with simple text templating, then it probably does.

An uncomfortable number of dimensional axes are coming together in compose_preseed: commissioning vs. deployment, operating systems with and without preseed code, conventional installer or fastpath (or whatever install options some other OS might have in their place). Gotta nip that sort of burgeoning complexity in the bud, or it will eventually stifle further development. Fight it with fire because by nature it will just keep growing.

So, assuming that we really do need OS-specific preseed-generating code, I would look for a way to move the Ubuntu-specific parts into the Ubuntu OperatingSystem. Or if most-but-not-all operating systems can generate their preseeds using templates, delegate most operating systems' implementations to a single template-based helper. (If templates can reasonably do the job for all operating systems we have in mind, of course, just use templating and stick with generic code.)

The hasattr check is best avoided, I think. If you're going to rely on dynamic dispatch for this, go all the way. You can accommodate some kind of sensible default in the dynamic method's definition, but an unconditional code path is going to be more manageable than special-casing in the code. Conditionals are our profession's equivalent of moving parts: they introduce complexity and risk.

There also seems to be a tacit rule in there that the commissioning OS must be, or work like, Ubuntu. That is worth being very explicit about. This isn't just a series of "if" statements to make things work, it's defining how a node can or cannot be installed. The details of of how these conditions interact are crucial, and yet I don't see any tests covering things like “we generate the customary Ubuntu preseed for a commissioning node, even if it's to be deployed with a different OS.”

By the way, I do appreciate how this branch is small and focused and not thrown in with a large bag of other changes!

review: Needs Fixing

[Blake Rouse (blake-rouse)]

Removed the hasattr check to use a NotImplemented exception. Can you give this another review?

[Jeroen T. Vermeulen (jtv)]

Thanks, this branch looks a lot simpler than I remember it! Could you add a note in the docstring for compose_preseed about the NotImplementedError and what it means? Similar to “:param …:” and “:return:” the format also supports “:raise …:”.

review: Approve

[Blake Rouse (blake-rouse)]

Already approved by jtv. Had to re-submit to fix the invalid prequisite branch. Self approving.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~blake-rouse/maas/osystem-preseed-cleanup into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [58.5 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [45.7 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [28.2 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [109 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [74.9 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 318 kB in 0s (975 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-netif...

=== modified file 'src/maasserver/compose_preseed.py'
--- src/maasserver/compose_preseed.py	2013-10-18 09:54:17 +0000
+++ src/maasserver/compose_preseed.py	2014-05-29 18:37:02 +0000
@@ -20,6 +20,7 @@
 
 from maasserver.enum import NODE_STATUS
 from maasserver.utils import absolute_reverse
+from provisioningserver.driver import OperatingSystemRegistry
 import yaml
 
 
@@ -104,6 +105,13 @@
     if node.status == NODE_STATUS.COMMISSIONING:
         return compose_commissioning_preseed(token, base_url)
     else:
+        osystem = OperatingSystemRegistry[node.get_osystem()]
+        metadata_url = absolute_reverse('metadata', base_url=base_url)
+        try:
+            return osystem.compose_preseed(node, token, metadata_url)
+        except NotImplementedError:
+            pass
+
         if node.should_use_traditional_installer():
             return compose_cloud_init_preseed(token, base_url)
         else:
 
=== modified file 'src/maasserver/enum.py'
--- src/maasserver/enum.py	2014-03-28 16:36:32 +0000
+++ src/maasserver/enum.py	2014-05-29 18:37:02 +0000
@@ -13,7 +13,6 @@
 
 __metaclass__ = type
 __all__ = [
-    'COMMISSIONING_DISTRO_SERIES_CHOICES',
     'COMPONENT',
     'NODEGROUP_STATUS',
     'NODEGROUP_STATUS_CHOICES',
@@ -25,8 +24,6 @@
     'NODE_STATUS_CHOICES',
     'NODE_STATUS_CHOICES_DICT',
     'PRESEED_TYPE',
-    'DISTRO_SERIES',
-    'DISTRO_SERIES_CHOICES',
     'USERDATA_TYPE',
     ]
 
@@ -86,36 +83,6 @@
 NODE_STATUS_CHOICES_DICT = OrderedDict(NODE_STATUS_CHOICES)
 
 
-class DISTRO_SERIES:
-    """List of supported ubuntu releases."""
-    #:
-    default = ''
-    #:
-    precise = 'precise'
-    #:
-    quantal = 'quantal'
-    #:
-    raring = 'raring'
-    #:
-    saucy = 'saucy'
-    #:
-    trusty = 'trusty'
-
-DISTRO_SERIES_CHOICES = (
-    (DISTRO_SERIES.default, 'Default Ubuntu Release'),
-    (DISTRO_SERIES.precise, 'Ubuntu 12.04 LTS "Precise Pangolin"'),
-    (DISTRO_SERIES.quantal, 'Ubuntu 12.10 "Quantal Quetzal"'),
-    (DISTRO_SERIES.raring, 'Ubuntu 13.04 "Raring Ringtail"'),
-    (DISTRO_SERIES.saucy, 'Ubuntu 13.10 "Saucy Salamander"'),
-    (DISTRO_SERIES.trusty, 'Ubuntu 14.04 LTS "Trusty Tahr"'),
-)
-
-
-COMMISSIONING_DISTRO_SERIES_CHOICES = (
-    (DISTRO_SERIES.trusty, dict(DISTRO_SERIES_CHOICES)[DISTRO_SERIES.trusty]),
-)
-
-
 class NODE_PERMISSION:
     """Permissions relating to nodes."""
     VIEW = 'view_node'
 
=== modified file 'src/maasserver/models/bootsourceselection.py'
--- src/maasserver/models/bootsourceselection.py	2014-05-16 11:41:18 +0000
+++ src/maasserver/models/bootsourceselection.py	2014-05-29 18:37:02 +0000
@@ -24,12 +24,16 @@
     )
 import djorm_pgarray.fields
 from maasserver import DefaultMeta
-from maasserver.enum import (
-    DISTRO_SERIES,
-    DISTRO_SERIES_CHOICES,
-    )
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.timestampedmodel import TimestampedModel
+from provisioningserver.driver.os_ubuntu import UbuntuOS
+
+
+def list_release_choices():
+    """Return Django "choices" list for Ubuntu releases."""
+    osystem = UbuntuOS()
+    releases = osystem.get_supported_releases()
+    return osystem.format_release_choices(releases)
 
 
 class BootSourceSelectionManager(Manager):
@@ -47,8 +51,8 @@
     boot_source = ForeignKey('maasserver.BootSource', blank=False)
 
     release = CharField(
-        max_length=20, choices=DISTRO_SERIES_CHOICES, blank=True,
-        default=DISTRO_SERIES.default,
+        max_length=20, choices=list_release_choices(), blank=True,
+        default='',
         help_text="The Ubuntu release for which to import resources.")
 
     arches = djorm_pgarray.fields.ArrayField(dbtype="text")
 
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2014-05-29 17:03:20 +0000
+++ src/maasserver/testing/factory.py	2014-05-29 18:37:02 +0000
@@ -23,7 +23,6 @@
 from django.contrib.auth.models import User
 from maasserver.clusterrpc.power_parameters import get_power_types
 from maasserver.enum import (
-    DISTRO_SERIES_CHOICES,
     NODE_STATUS,
     NODEGROUP_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
@@ -62,6 +61,7 @@
 # XXX 2014-05-13 blake-rouse bug=1319143
 # Need to not import directly, use RPC to info from cluster.
 from provisioningserver.driver import OperatingSystemRegistry
+from provisioningserver.driver.os_ubuntu import UbuntuOS
 
 # We have a limited number of public keys:
 # src/maasserver/tests/data/test_rsa{0, 1, 2, 3, 4}.pub
@@ -717,7 +717,8 @@
         if boot_source is None:
             boot_source = self.make_boot_source()
         if release is None:
-            release = self.getRandomChoice(DISTRO_SERIES_CHOICES)
+            ubuntu_os = UbuntuOS()
+            release = self.getRandomRelease(ubuntu_os)
         if arches is None:
             arch_count = random.randint(1, 10)
             arches = [self.make_name("arch") for i in range(arch_count)]
 
=== modified file 'src/maasserver/tests/test_api_boot_source_selections.py'
--- src/maasserver/tests/test_api_boot_source_selections.py	2014-05-27 13:53:21 +0000
+++ src/maasserver/tests/test_api_boot_source_selections.py	2014-05-29 18:37:02 +0000
@@ -19,11 +19,11 @@
 
 from django.core.urlresolvers import reverse
 from maasserver.api import DISPLAYED_BOOTSOURCESELECTION_FIELDS
-from maasserver.enum import DISTRO_SERIES_CHOICES
 from maasserver.models import BootSourceSelection
 from maasserver.testing import reload_object
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
+from provisioningserver.driver.os_ubuntu import UbuntuOS
 from testtools.matchers import MatchesStructure
 
 
@@ -98,8 +98,8 @@
     def test_PUT_updates_boot_source_selection(self):
         self.become_admin()
         boot_source_selection = factory.make_boot_source_selection()
-        new_release = factory.getRandomChoice(
-            DISTRO_SERIES_CHOICES)
+        ubuntu_os = UbuntuOS()
+        new_release = factory.getRandomRelease(ubuntu_os)
         new_values = {
             'release': new_release,
             'arches': [factory.make_name('arch'), factory.make_name('arch')],
@@ -160,8 +160,8 @@
     def test_POST_creates_boot_source_selection(self):
         self.become_admin()
         boot_source = factory.make_boot_source()
-        new_release = factory.getRandomChoice(
-            DISTRO_SERIES_CHOICES)
+        ubuntu_os = UbuntuOS()
+        new_release = factory.getRandomRelease(ubuntu_os)
         params = {
             'release': new_release,
             'arches': [factory.make_name('arch'), factory.make_name('arch')],
@@ -182,8 +182,8 @@
 
     def test_POST_requires_admin(self):
         boot_source = factory.make_boot_source()
-        new_release = factory.getRandomChoice(
-            DISTRO_SERIES_CHOICES)
+        ubuntu_os = UbuntuOS()
+        new_release = factory.getRandomRelease(ubuntu_os)
         params = {
             'release': new_release,
             'arches': [factory.make_name('arch'), factory.make_name('arch')],
 
=== modified file 'src/maasserver/tests/test_compose_preseed.py'
--- src/maasserver/tests/test_compose_preseed.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/tests/test_compose_preseed.py	2014-05-29 18:37:02 +0000
@@ -17,8 +17,10 @@
 from maasserver.compose_preseed import compose_preseed
 from maasserver.enum import NODE_STATUS
 from maasserver.testing.factory import factory
+from maasserver.testing.osystems import make_usable_osystem
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils import absolute_reverse
+from maastesting.matchers import MockCalledOnceWith
 from metadataserver.models import NodeKey
 from testtools.matchers import (
     KeysEqual,
@@ -110,3 +112,16 @@
         self.assertEqual(
             absolute_reverse('curtin-metadata'),
             preseed['datasource']['MAAS']['metadata_url'])
+
+    def test_compose_preseed_with_osystem_compose_preseed(self):
+        osystem = make_usable_osystem(self)
+        mock_compose = self.patch(osystem, 'compose_preseed')
+        node = factory.make_node(
+            osystem=osystem.name, status=NODE_STATUS.READY)
+
+        token = NodeKey.objects.get_token_for_node(node)
+        url = absolute_reverse('metadata')
+        compose_preseed(node)
+        self.assertThat(
+            mock_compose,
+            MockCalledOnceWith(node, token, url))
 
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2014-05-29 17:03:20 +0000
+++ src/maasserver/tests/test_forms.py	2014-05-29 18:37:02 +0000
@@ -29,7 +29,6 @@
 from django.http import QueryDict
 from maasserver.clusterrpc.power_parameters import get_power_type_choices
 from maasserver.enum import (
-    DISTRO_SERIES_CHOICES,
     NODE_STATUS,
     NODEGROUP_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
@@ -112,6 +111,7 @@
 from metadataserver.models import CommissioningScript
 from netaddr import IPNetwork
 from provisioningserver import tasks
+from provisioningserver.driver.os_ubuntu import UbuntuOS
 from testtools import TestCase
 from testtools.matchers import (
     AllMatch,
@@ -2062,8 +2062,8 @@
 
     def test_edits_boot_source_selection_object(self):
         boot_source_selection = factory.make_boot_source_selection()
-        new_release = factory.getRandomChoice(
-            DISTRO_SERIES_CHOICES)
+        ubuntu_os = UbuntuOS()
+        new_release = factory.getRandomRelease(ubuntu_os)
         params = {
             'release': new_release,
             'arches': [factory.make_name('arch'), factory.make_name('arch')],
@@ -2080,8 +2080,8 @@
 
     def test_creates_boot_source_selection_object(self):
         boot_source = factory.make_boot_source()
-        new_release = factory.getRandomChoice(
-            DISTRO_SERIES_CHOICES)
+        ubuntu_os = UbuntuOS()
+        new_release = factory.getRandomRelease(ubuntu_os)
         params = {
             'release': new_release,
             'arches': [factory.make_name('arch'), factory.make_name('arch')],
 
=== modified file 'src/provisioningserver/driver/__init__.py'
--- src/provisioningserver/driver/__init__.py	2014-05-15 17:27:53 +0000
+++ src/provisioningserver/driver/__init__.py	2014-05-29 18:37:02 +0000
@@ -158,6 +158,18 @@
         :returns: list of supported purposes
         """
 
+    def compose_preseed(self, node, token, metadata_url):
+        """Composes the preseed for the given node.
+
+        :param node: Node preseed needs generating.
+        :param token: OAuth token for url.
+        :param metadata_url: Metdata url for node.
+        :returns: Preseed for node.
+        :raise:
+            NotImplementedError: doesn't implement a custom preseed
+        """
+        raise NotImplementedError()
+
 
 class HardwareDiscoverContext: