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

Proposed by Blake Rouse
Status: Merged
Approved by: Blake Rouse
Approved revision: 2302
Merged at revision: 2385
Proposed branch: lp:~blake-rouse/maas/osystem-preseed-cleanup
Merge into: lp:maas
Diff against target: 295 lines (+60/-53)
8 files modified
src/maasserver/compose_preseed.py (+8/-0)
src/maasserver/enum.py (+0/-33)
src/maasserver/models/bootsourceselection.py (+10/-6)
src/maasserver/testing/factory.py (+3/-2)
src/maasserver/tests/test_api_boot_source_selections.py (+7/-7)
src/maasserver/tests/test_compose_preseed.py (+15/-0)
src/maasserver/tests/test_forms.py (+5/-5)
src/provisioningserver/driver/__init__.py (+12/-0)
To merge this branch: bzr merge lp:~blake-rouse/maas/osystem-preseed-cleanup
Reviewer Review Type Date Requested Status
Blake Rouse (community) Approve
Review via email: mp+221440@code.launchpad.net

This proposal supersedes a proposal from 2014-04-24.

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Blake Rouse (blake-rouse) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Blake Rouse (blake-rouse) wrote :

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

review: Approve
2302. By Blake Rouse

Remove code, from bad merge.

Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (80.7 KiB)

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...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/compose_preseed.py'
2--- src/maasserver/compose_preseed.py 2013-10-18 09:54:17 +0000
3+++ src/maasserver/compose_preseed.py 2014-05-29 18:37:02 +0000
4@@ -20,6 +20,7 @@
5
6 from maasserver.enum import NODE_STATUS
7 from maasserver.utils import absolute_reverse
8+from provisioningserver.driver import OperatingSystemRegistry
9 import yaml
10
11
12@@ -104,6 +105,13 @@
13 if node.status == NODE_STATUS.COMMISSIONING:
14 return compose_commissioning_preseed(token, base_url)
15 else:
16+ osystem = OperatingSystemRegistry[node.get_osystem()]
17+ metadata_url = absolute_reverse('metadata', base_url=base_url)
18+ try:
19+ return osystem.compose_preseed(node, token, metadata_url)
20+ except NotImplementedError:
21+ pass
22+
23 if node.should_use_traditional_installer():
24 return compose_cloud_init_preseed(token, base_url)
25 else:
26
27=== modified file 'src/maasserver/enum.py'
28--- src/maasserver/enum.py 2014-03-28 16:36:32 +0000
29+++ src/maasserver/enum.py 2014-05-29 18:37:02 +0000
30@@ -13,7 +13,6 @@
31
32 __metaclass__ = type
33 __all__ = [
34- 'COMMISSIONING_DISTRO_SERIES_CHOICES',
35 'COMPONENT',
36 'NODEGROUP_STATUS',
37 'NODEGROUP_STATUS_CHOICES',
38@@ -25,8 +24,6 @@
39 'NODE_STATUS_CHOICES',
40 'NODE_STATUS_CHOICES_DICT',
41 'PRESEED_TYPE',
42- 'DISTRO_SERIES',
43- 'DISTRO_SERIES_CHOICES',
44 'USERDATA_TYPE',
45 ]
46
47@@ -86,36 +83,6 @@
48 NODE_STATUS_CHOICES_DICT = OrderedDict(NODE_STATUS_CHOICES)
49
50
51-class DISTRO_SERIES:
52- """List of supported ubuntu releases."""
53- #:
54- default = ''
55- #:
56- precise = 'precise'
57- #:
58- quantal = 'quantal'
59- #:
60- raring = 'raring'
61- #:
62- saucy = 'saucy'
63- #:
64- trusty = 'trusty'
65-
66-DISTRO_SERIES_CHOICES = (
67- (DISTRO_SERIES.default, 'Default Ubuntu Release'),
68- (DISTRO_SERIES.precise, 'Ubuntu 12.04 LTS "Precise Pangolin"'),
69- (DISTRO_SERIES.quantal, 'Ubuntu 12.10 "Quantal Quetzal"'),
70- (DISTRO_SERIES.raring, 'Ubuntu 13.04 "Raring Ringtail"'),
71- (DISTRO_SERIES.saucy, 'Ubuntu 13.10 "Saucy Salamander"'),
72- (DISTRO_SERIES.trusty, 'Ubuntu 14.04 LTS "Trusty Tahr"'),
73-)
74-
75-
76-COMMISSIONING_DISTRO_SERIES_CHOICES = (
77- (DISTRO_SERIES.trusty, dict(DISTRO_SERIES_CHOICES)[DISTRO_SERIES.trusty]),
78-)
79-
80-
81 class NODE_PERMISSION:
82 """Permissions relating to nodes."""
83 VIEW = 'view_node'
84
85=== modified file 'src/maasserver/models/bootsourceselection.py'
86--- src/maasserver/models/bootsourceselection.py 2014-05-16 11:41:18 +0000
87+++ src/maasserver/models/bootsourceselection.py 2014-05-29 18:37:02 +0000
88@@ -24,12 +24,16 @@
89 )
90 import djorm_pgarray.fields
91 from maasserver import DefaultMeta
92-from maasserver.enum import (
93- DISTRO_SERIES,
94- DISTRO_SERIES_CHOICES,
95- )
96 from maasserver.models.cleansave import CleanSave
97 from maasserver.models.timestampedmodel import TimestampedModel
98+from provisioningserver.driver.os_ubuntu import UbuntuOS
99+
100+
101+def list_release_choices():
102+ """Return Django "choices" list for Ubuntu releases."""
103+ osystem = UbuntuOS()
104+ releases = osystem.get_supported_releases()
105+ return osystem.format_release_choices(releases)
106
107
108 class BootSourceSelectionManager(Manager):
109@@ -47,8 +51,8 @@
110 boot_source = ForeignKey('maasserver.BootSource', blank=False)
111
112 release = CharField(
113- max_length=20, choices=DISTRO_SERIES_CHOICES, blank=True,
114- default=DISTRO_SERIES.default,
115+ max_length=20, choices=list_release_choices(), blank=True,
116+ default='',
117 help_text="The Ubuntu release for which to import resources.")
118
119 arches = djorm_pgarray.fields.ArrayField(dbtype="text")
120
121=== modified file 'src/maasserver/testing/factory.py'
122--- src/maasserver/testing/factory.py 2014-05-29 17:03:20 +0000
123+++ src/maasserver/testing/factory.py 2014-05-29 18:37:02 +0000
124@@ -23,7 +23,6 @@
125 from django.contrib.auth.models import User
126 from maasserver.clusterrpc.power_parameters import get_power_types
127 from maasserver.enum import (
128- DISTRO_SERIES_CHOICES,
129 NODE_STATUS,
130 NODEGROUP_STATUS,
131 NODEGROUPINTERFACE_MANAGEMENT,
132@@ -62,6 +61,7 @@
133 # XXX 2014-05-13 blake-rouse bug=1319143
134 # Need to not import directly, use RPC to info from cluster.
135 from provisioningserver.driver import OperatingSystemRegistry
136+from provisioningserver.driver.os_ubuntu import UbuntuOS
137
138 # We have a limited number of public keys:
139 # src/maasserver/tests/data/test_rsa{0, 1, 2, 3, 4}.pub
140@@ -717,7 +717,8 @@
141 if boot_source is None:
142 boot_source = self.make_boot_source()
143 if release is None:
144- release = self.getRandomChoice(DISTRO_SERIES_CHOICES)
145+ ubuntu_os = UbuntuOS()
146+ release = self.getRandomRelease(ubuntu_os)
147 if arches is None:
148 arch_count = random.randint(1, 10)
149 arches = [self.make_name("arch") for i in range(arch_count)]
150
151=== modified file 'src/maasserver/tests/test_api_boot_source_selections.py'
152--- src/maasserver/tests/test_api_boot_source_selections.py 2014-05-27 13:53:21 +0000
153+++ src/maasserver/tests/test_api_boot_source_selections.py 2014-05-29 18:37:02 +0000
154@@ -19,11 +19,11 @@
155
156 from django.core.urlresolvers import reverse
157 from maasserver.api import DISPLAYED_BOOTSOURCESELECTION_FIELDS
158-from maasserver.enum import DISTRO_SERIES_CHOICES
159 from maasserver.models import BootSourceSelection
160 from maasserver.testing import reload_object
161 from maasserver.testing.api import APITestCase
162 from maasserver.testing.factory import factory
163+from provisioningserver.driver.os_ubuntu import UbuntuOS
164 from testtools.matchers import MatchesStructure
165
166
167@@ -98,8 +98,8 @@
168 def test_PUT_updates_boot_source_selection(self):
169 self.become_admin()
170 boot_source_selection = factory.make_boot_source_selection()
171- new_release = factory.getRandomChoice(
172- DISTRO_SERIES_CHOICES)
173+ ubuntu_os = UbuntuOS()
174+ new_release = factory.getRandomRelease(ubuntu_os)
175 new_values = {
176 'release': new_release,
177 'arches': [factory.make_name('arch'), factory.make_name('arch')],
178@@ -160,8 +160,8 @@
179 def test_POST_creates_boot_source_selection(self):
180 self.become_admin()
181 boot_source = factory.make_boot_source()
182- new_release = factory.getRandomChoice(
183- DISTRO_SERIES_CHOICES)
184+ ubuntu_os = UbuntuOS()
185+ new_release = factory.getRandomRelease(ubuntu_os)
186 params = {
187 'release': new_release,
188 'arches': [factory.make_name('arch'), factory.make_name('arch')],
189@@ -182,8 +182,8 @@
190
191 def test_POST_requires_admin(self):
192 boot_source = factory.make_boot_source()
193- new_release = factory.getRandomChoice(
194- DISTRO_SERIES_CHOICES)
195+ ubuntu_os = UbuntuOS()
196+ new_release = factory.getRandomRelease(ubuntu_os)
197 params = {
198 'release': new_release,
199 'arches': [factory.make_name('arch'), factory.make_name('arch')],
200
201=== modified file 'src/maasserver/tests/test_compose_preseed.py'
202--- src/maasserver/tests/test_compose_preseed.py 2013-10-07 09:12:40 +0000
203+++ src/maasserver/tests/test_compose_preseed.py 2014-05-29 18:37:02 +0000
204@@ -17,8 +17,10 @@
205 from maasserver.compose_preseed import compose_preseed
206 from maasserver.enum import NODE_STATUS
207 from maasserver.testing.factory import factory
208+from maasserver.testing.osystems import make_usable_osystem
209 from maasserver.testing.testcase import MAASServerTestCase
210 from maasserver.utils import absolute_reverse
211+from maastesting.matchers import MockCalledOnceWith
212 from metadataserver.models import NodeKey
213 from testtools.matchers import (
214 KeysEqual,
215@@ -110,3 +112,16 @@
216 self.assertEqual(
217 absolute_reverse('curtin-metadata'),
218 preseed['datasource']['MAAS']['metadata_url'])
219+
220+ def test_compose_preseed_with_osystem_compose_preseed(self):
221+ osystem = make_usable_osystem(self)
222+ mock_compose = self.patch(osystem, 'compose_preseed')
223+ node = factory.make_node(
224+ osystem=osystem.name, status=NODE_STATUS.READY)
225+
226+ token = NodeKey.objects.get_token_for_node(node)
227+ url = absolute_reverse('metadata')
228+ compose_preseed(node)
229+ self.assertThat(
230+ mock_compose,
231+ MockCalledOnceWith(node, token, url))
232
233=== modified file 'src/maasserver/tests/test_forms.py'
234--- src/maasserver/tests/test_forms.py 2014-05-29 17:03:20 +0000
235+++ src/maasserver/tests/test_forms.py 2014-05-29 18:37:02 +0000
236@@ -29,7 +29,6 @@
237 from django.http import QueryDict
238 from maasserver.clusterrpc.power_parameters import get_power_type_choices
239 from maasserver.enum import (
240- DISTRO_SERIES_CHOICES,
241 NODE_STATUS,
242 NODEGROUP_STATUS,
243 NODEGROUPINTERFACE_MANAGEMENT,
244@@ -112,6 +111,7 @@
245 from metadataserver.models import CommissioningScript
246 from netaddr import IPNetwork
247 from provisioningserver import tasks
248+from provisioningserver.driver.os_ubuntu import UbuntuOS
249 from testtools import TestCase
250 from testtools.matchers import (
251 AllMatch,
252@@ -2062,8 +2062,8 @@
253
254 def test_edits_boot_source_selection_object(self):
255 boot_source_selection = factory.make_boot_source_selection()
256- new_release = factory.getRandomChoice(
257- DISTRO_SERIES_CHOICES)
258+ ubuntu_os = UbuntuOS()
259+ new_release = factory.getRandomRelease(ubuntu_os)
260 params = {
261 'release': new_release,
262 'arches': [factory.make_name('arch'), factory.make_name('arch')],
263@@ -2080,8 +2080,8 @@
264
265 def test_creates_boot_source_selection_object(self):
266 boot_source = factory.make_boot_source()
267- new_release = factory.getRandomChoice(
268- DISTRO_SERIES_CHOICES)
269+ ubuntu_os = UbuntuOS()
270+ new_release = factory.getRandomRelease(ubuntu_os)
271 params = {
272 'release': new_release,
273 'arches': [factory.make_name('arch'), factory.make_name('arch')],
274
275=== modified file 'src/provisioningserver/driver/__init__.py'
276--- src/provisioningserver/driver/__init__.py 2014-05-15 17:27:53 +0000
277+++ src/provisioningserver/driver/__init__.py 2014-05-29 18:37:02 +0000
278@@ -158,6 +158,18 @@
279 :returns: list of supported purposes
280 """
281
282+ def compose_preseed(self, node, token, metadata_url):
283+ """Composes the preseed for the given node.
284+
285+ :param node: Node preseed needs generating.
286+ :param token: OAuth token for url.
287+ :param metadata_url: Metdata url for node.
288+ :returns: Preseed for node.
289+ :raise:
290+ NotImplementedError: doesn't implement a custom preseed
291+ """
292+ raise NotImplementedError()
293+
294
295 class HardwareDiscoverContext:
296