Merge ~smoser/maas:cleanup/render-curtin-config-internally into maas:master

Proposed by Scott Moser
Status: Rejected
Rejected by: Blake Rouse
Proposed branch: ~smoser/maas:cleanup/render-curtin-config-internally
Merge into: maas:master
Diff against target: 640 lines (+224/-127)
7 files modified
contrib/preseeds_v2/curtin_userdata (+18/-42)
contrib/preseeds_v2/curtin_userdata_centos (+3/-9)
contrib/preseeds_v2/curtin_userdata_windows (+2/-10)
src/maasserver/node_action.py (+2/-2)
src/maasserver/preseed.py (+133/-6)
src/maasserver/tests/test_node_action.py (+21/-21)
src/maasserver/tests/test_preseed.py (+45/-37)
Reviewer Review Type Date Requested Status
MAAS Lander Needs Fixing
Mike Pontillo (community) Needs Information
Andres Rodriguez Pending
Review via email: mp+332573@code.launchpad.net

Commit message

Internally handle most of curtin_userdata leaving templates to user.

The 'curtin_userdata' file was scary to look at, and hard to work
with for a user. Adding early_commands or late_commands meant that they
would have to edit an existing entry, and failure to do so would
break the install.

This takes what was previously in that those files and handles them
internally to maas. Thus, the user now has a blank slate to work on,
and an example of what could go there.

One change along the way was to render the writing of the binary
tmp file with the apt key via base64 rather than hex encoding.

Description of the change

This is a work in progress, I'm looking to see if it the general concept
is acceptable.

It is untested at this point, but I believe should be (close to) functional but does break third party drivers currently.

To post a comment you must log in.
Revision history for this message
Blake Rouse (blake-rouse) wrote :

I think the general idea is great, but we need to think about the upgrade path.

What happens if they made changes to the sections that you have not made internal? Will there changes override the internal changes?

Also this is more of a future thing, but I would prefer to see the templates move into the database with an API, because at the moment with HA, these are a problem.

Revision history for this message
Andres Rodriguez (andreserl) wrote :

That's my only concern, we need to make sure the upgrade path doesn't break
and this continuous to work with those that modify their curtin scripts.
Which is a lot of people atm.

On Mon, Oct 23, 2017 at 9:16 AM, Blake Rouse <email address hidden>
wrote:

> I think the general idea is great, but we need to think about the upgrade
> path.
>
> What happens if they made changes to the sections that you have not made
> internal? Will there changes override the internal changes?
>
> Also this is more of a future thing, but I would prefer to see the
> templates move into the database with an API, because at the moment with
> HA, these are a problem.
> --
> https://code.launchpad.net/~smoser/maas/+git/maas/+merge/332573
> Your team MAAS Committers is subscribed to branch maas:master.
>

--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.

Revision history for this message
Scott Moser (smoser) wrote :

Summary: over all, it will do reasonably well on "upgrade".

What is the upgrade path anyway, for *any* change to that file?
Do you have something in place other than debian conf-file upgrades?

If all you have is debian conf-file, then the admin running 'apt-get
dist-upgrade' is going to get prompted and see that those files are being
replaced and have the ability to diff them.

I'd consider that there are 2 likely paths:
a.) user does nothing, leaving the whole file in place as it was before.
b.) user looks at it, sees their changes and generally correctly merges
them by creating the file with *just* their changes inside.

So 'b' is the easy path, that should work or is generally not fixable
other than by the user.

for 'a', all the "builtin" config values are dictionaries.
When curtin merges dictionaries, the later one wins.
So as long as we keep the same keys in all cases, then nothing
would get run more than once and all should be well.
By 'key', i mean:
  early_commands/driver_*
and
  debconf_selections/maas

If you really wanted to safe-guard you could check the loaded file
and explicitly ignore the previously named keys, but that too could
actually cause breakage, if the user had modified those keys to
their liking.

Revision history for this message
Scott Moser (smoser) wrote :

Actually, what I think i'd do and suggest is to
a.) rename the keys from the names that did exist in that file
  early_commands/driver_* to early_commands/30_driver_*
  debconf_selections/maas_* to debconf_selections/maas_internal

b.) "clean" out the old names if they were present in the loaded config

this would mean that user's modifications to that file that modified *those* things would actually fail to have affect. But generally speaking they should not have done that, or we'd like to have bugs to see what was wrong.

then just warn in log that it is ignoring those things.
that'd keep their file basically working as expected other than some things
in it not actually having any affect.

Revision history for this message
Andres Rodriguez (andreserl) wrote :

For 2.3, this is too late, so we will target this for 2.4 once it opens.

Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b cleanup/render-curtin-config-internally lp:~smoser/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/519/console
COMMIT: d313a298a0eeb39535e8017b777db8481010f5e1

review: Needs Fixing
Revision history for this message
Ante Karamatić (ivoks) :
Revision history for this message
Scott Moser (smoser) wrote :

I addressed Ante's comment and updated to merge without conflicts on current master.

Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b cleanup/render-curtin-config-internally lp:~smoser/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/812/console
COMMIT: addfc9a36144635a35e1465f10cb7de905ae387d

review: Needs Fixing
Revision history for this message
Scott Moser (smoser) wrote :

I think this is ready for review at this point.
I've fixed up all the unit tests.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

This branch is looking much better, but where are the tests that cover the new code?

review: Needs Information
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b cleanup/render-curtin-config-internally lp:~smoser/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/823/console
COMMIT: 296fab455858b1b3970137fb23f27351db5b9d8e

review: Needs Fixing
Revision history for this message
Scott Moser (smoser) wrote :

well, we can add tests for the new code, but this is really just code motion (replace of template rendering with internal).
Ie, the net change is the same behavior. Whatever tests were there before (that I adjusted) should give the same total code coverage.

What specific things are you wanting to be tested?

Revision history for this message
Scott Moser (smoser) wrote :

Also, I can't see the results of the unit tests above.
roaksoax gave me a paste of them, but I'm not sure what woudl be causing the 'url'' to not get set (some help?)

Revision history for this message
Andres Rodriguez (andreserl) wrote :

Hi Scott,

We'll take a look at it next week. Right now the team is running on reduced
capacity.

On Fri, Dec 1, 2017 at 10:53 AM, Scott Moser <email address hidden>
wrote:

> Also, I can't see the results of the unit tests above.
> roaksoax gave me a paste of them, but I'm not sure what woudl be causing
> the 'url'' to not get set (some help?)
>
> --
> https://code.launchpad.net/~smoser/maas/+git/maas/+merge/332573
> You are reviewing the proposed merge of ~smoser/maas:cleanup/render-curtin-config-internally
> into maas:master.
>

--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.

Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b cleanup/render-curtin-config-internally lp:~smoser/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/868/console
COMMIT: d33d9f6b6da6b2306e167357579fe6e4ae8764dd

review: Needs Fixing
Revision history for this message
Scott Moser (smoser) wrote :

Bump?

Revision history for this message
Mike Pontillo (mpontillo) wrote :

@smoser, I'll take a closer look. Thanks for getting this work going.

Revision history for this message
Blake Rouse (blake-rouse) wrote :

This has been sitting for a long time, closing up the old queue.

Unmerged commits

d33d9f6... by Scott Moser

fix per make format

1936d8c... by Scott Moser

fix flake8

bc88c60... by Scott Moser

drop key_b64 variable

ff2cc32... by Scott Moser

drop 'subst' helper, no longer used

cafa3eb... by Scott Moser

remove debug print

7d1215b... by Scott Moser

tests pass.

5930dfe... by Scott Moser

replace old templates with stock empty

51bd856... by Scott Moser

Internally handle most of curtin_userdata leaving templates to user.

The 'curtin_userdata' file was scary to look at, and hard to work
with for a user. Adding early_commands or late_commands meant that they
would have to edit an existing entry, and failure to do so would
break the install.

This takes what was previously in that those files and handles them
internally to maas. Thus, the user now has a blank slate to work on,
and an example of what could go there.

LP: #1734391

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/contrib/preseeds_v2/curtin_userdata b/contrib/preseeds_v2/curtin_userdata
2index 4caddc4..a22a775 100644
3--- a/contrib/preseeds_v2/curtin_userdata
4+++ b/contrib/preseeds_v2/curtin_userdata
5@@ -1,42 +1,18 @@
6-#cloud-config
7-debconf_selections:
8- maas: |
9- {{for line in str(curtin_preseed).splitlines()}}
10- {{line}}
11- {{endfor}}
12-early_commands:
13-{{if third_party_drivers and driver}}
14- {{py: key_string = ''.join(['\\x%x' % x for x in driver['key_binary']])}}
15- {{if driver['key_binary'] and driver['repository'] and driver['package']}}
16- driver_00_get_key: /bin/echo -en '{{key_string}}' > /tmp/maas-{{driver['package']}}.gpg
17- driver_01_add_key: ["apt-key", "add", "/tmp/maas-{{driver['package']}}.gpg"]
18- {{endif}}
19- {{if driver['repository']}}
20- driver_02_add: ["add-apt-repository", "-y", "deb {{driver['repository']}} {{node.get_distro_series()}} main"]
21- {{endif}}
22- {{if driver['package']}}
23- driver_03_update_install: ["sh", "-c", "apt-get update --quiet && apt-get --assume-yes install {{driver['package']}}"]
24- {{endif}}
25- {{if driver['module']}}
26- driver_04_load: ["sh", "-c", "depmod && modprobe {{driver['module']}} || echo 'Warning: Failed to load module: {{driver['module']}}'"]
27- {{endif}}
28-{{else}}
29- driver_00: ["sh", "-c", "echo third party drivers not installed or necessary."]
30-{{endif}}
31-late_commands:
32- maas: [wget, '--no-proxy', {{node_disable_pxe_url|escape.json}}, '--post-data', {{node_disable_pxe_data|escape.json}}, '-O', '/dev/null']
33-{{if third_party_drivers and driver}}
34- {{if driver['key_binary'] and driver['repository'] and driver['package']}}
35- driver_00_key_get: curtin in-target -- sh -c "/bin/echo -en '{{key_string}}' > /tmp/maas-{{driver['package']}}.gpg"
36- driver_02_key_add: ["curtin", "in-target", "--", "apt-key", "add", "/tmp/maas-{{driver['package']}}.gpg"]
37- {{endif}}
38- {{if driver['repository']}}
39- driver_03_add: ["curtin", "in-target", "--", "add-apt-repository", "-y", "deb {{driver['repository']}} {{node.get_distro_series()}} main"]
40- {{endif}}
41- driver_04_update_install: ["curtin", "in-target", "--", "apt-get", "update", "--quiet"]
42- {{if driver['package']}}
43- driver_05_install: ["curtin", "in-target", "--", "apt-get", "-y", "install", "{{driver['package']}}"]
44- {{endif}}
45- driver_06_depmod: ["curtin", "in-target", "--", "depmod"]
46- driver_07_update_initramfs: ["curtin", "in-target", "--", "update-initramfs", "-u"]
47-{{endif}}
48+#curtin-config
49+## Here you can put custom data that will go to each node.
50+## The rendered output must be valid YAML.
51+##
52+## This file will be rendered as a tempita template.
53+## variables available for the rendering are
54+## osystem: "ubuntu", "suse", "windows", ...
55+## release: "xenial", "trusty", ...
56+## server_host: the maas facing server host
57+## server_url: ...
58+## syslog_host_port: ...
59+## metadata_enlist_url: ...
60+## node: the 'node' object
61+## license_key
62+##
63+## As a simple example, consider:
64+#late_commands:
65+# - ["curtin", "in-target", "--", "apt-get", "dist-upgrade"]
66diff --git a/contrib/preseeds_v2/curtin_userdata_centos b/contrib/preseeds_v2/curtin_userdata_centos
67index 3de9c99..2dd2023 100644
68--- a/contrib/preseeds_v2/curtin_userdata_centos
69+++ b/contrib/preseeds_v2/curtin_userdata_centos
70@@ -1,9 +1,3 @@
71-#cloud-config
72-debconf_selections:
73- maas: |
74- {{for line in str(curtin_preseed).splitlines()}}
75- {{line}}
76- {{endfor}}
77-
78-late_commands:
79- maas: [wget, '--no-proxy', '{{node_disable_pxe_url}}', '--post-data', '{{node_disable_pxe_data}}', '-O', '/dev/null']
80+#curtin-config
81+## This file is used only for centos deployments.
82+## See curtin_userdata for more information.
83diff --git a/contrib/preseeds_v2/curtin_userdata_windows b/contrib/preseeds_v2/curtin_userdata_windows
84index 77e4a65..4b7d891 100644
85--- a/contrib/preseeds_v2/curtin_userdata_windows
86+++ b/contrib/preseeds_v2/curtin_userdata_windows
87@@ -1,11 +1,3 @@
88 #cloud-config
89-debconf_selections:
90- maas: |
91- {{for line in str(curtin_preseed).splitlines()}}
92- {{line}}
93- {{endfor}}
94-
95-late_commands:
96- maas: [wget, '--no-proxy', '{{node_disable_pxe_url}}', '--post-data', '{{node_disable_pxe_data}}', '-O', '/dev/null']
97-
98-license_key: {{node.get_effective_license_key()}}
99+## This file is used only for windows deployments.
100+## See curtin_userdata for more information.
101diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py
102index 74c8df7..1830e2f 100644
103--- a/src/maasserver/node_action.py
104+++ b/src/maasserver/node_action.py
105@@ -44,7 +44,7 @@ from maasserver.node_status import (
106 is_failed_status,
107 NON_MONITORED_STATUSES,
108 )
109-from maasserver.preseed import get_curtin_config
110+from maasserver.preseed import get_curtin_configs
111 from maasserver.utils.orm import post_commit_do
112 from maasserver.utils.osystems import (
113 validate_hwe_kernel,
114@@ -347,7 +347,7 @@ class Deploy(NodeAction):
115 raise NodeActionError(e)
116
117 try:
118- get_curtin_config(self.node)
119+ get_curtin_configs(self.node)
120 except Exception as e:
121 raise NodeActionError(
122 "Failed to retrieve curtin config: %s" % e)
123diff --git a/src/maasserver/preseed.py b/src/maasserver/preseed.py
124index f2ed29c..d983a80 100644
125--- a/src/maasserver/preseed.py
126+++ b/src/maasserver/preseed.py
127@@ -15,6 +15,7 @@ __all__ = [
128 'OS_WITH_IPv6_SUPPORT',
129 ]
130
131+import base64
132 from collections import namedtuple
133 import json
134 import os.path
135@@ -344,7 +345,8 @@ def compose_curtin_verbose_preseed():
136
137 def get_curtin_yaml_config(node, default_region_ip=None):
138 """Return the curtin configration for the node."""
139- main_config = get_curtin_config(node)
140+ main_config = get_curtin_configs(node)
141+ custom_config = get_curtin_custom_config(node)
142 cloud_config = compose_curtin_cloud_config(node)
143 archive_config = compose_curtin_archive_config(node)
144 reporter_config = compose_curtin_maas_reporter(node)
145@@ -389,9 +391,9 @@ def get_curtin_yaml_config(node, default_region_ip=None):
146 storage_config = []
147
148 return (
149- storage_config + [main_config] + archive_config + reporter_config +
150+ storage_config + main_config + archive_config + reporter_config +
151 network_config + swap_config + kernel_config + verbose_config +
152- cloud_config)
153+ cloud_config + [custom_config])
154
155
156 def get_curtin_merged_config(node):
157@@ -475,7 +477,7 @@ def get_curtin_installer_url(node):
158 return url_prepend + url
159
160
161-def get_curtin_config(node, default_region_ip=None):
162+def get_curtin_preseed_context(node, default_region_ip=None):
163 """Return the curtin configuration to be used by curtin.pack_install.
164
165 :param node: The node for which to generate the configuration.
166@@ -483,8 +485,6 @@ def get_curtin_config(node, default_region_ip=None):
167 """
168 osystem = node.get_osystem()
169 series = node.get_distro_series()
170- template = load_preseed_template(
171- node, USERDATA_TYPE.CURTIN, osystem, series)
172 rack_controller = node.get_boot_rack_controller()
173 context = get_preseed_context(
174 osystem, series, rack_controller=rack_controller,
175@@ -497,6 +497,131 @@ def get_curtin_config(node, default_region_ip=None):
176 get_curtin_context(
177 node, rack_controller=rack_controller,
178 default_region_ip=default_region_ip))
179+ return context
180+
181+
182+def get_third_party_drivers_config_data(enabled, key, repo, package, module,
183+ series):
184+ ret = {'early_commands': {}, 'late_commands': {}}
185+ pre = 'driver_0'
186+
187+ if not enabled:
188+ msg = "echo third party drivers not installed or necessary."
189+ ret['early_commands'][pre + '0'] = ["echo", msg]
190+ ret['late_commands'][pre + '0'] = ["echo", msg]
191+ return ret
192+
193+ early = ret['early_commands']
194+ late = ret['late_commands']
195+
196+ in_target = ["curtin", "in-target", "--"]
197+ if key and repo and package:
198+ tmpf = "/tmp/maas-" + package
199+ echo_key = [
200+ 'sh', '-c',
201+ 'echo %s | base64 --decode > %s' % (base64.b64encode(key).decode(),
202+ tmpf)]
203+ apt_add_key = ['apt-key', 'add', tmpf]
204+ early[pre + '0_get_key'] = echo_key
205+ early[pre + '1_add_key'] = apt_add_key
206+ late[pre + '0_key_get'] = in_target + echo_key
207+ late[pre + '2_key_add'] = in_target + apt_add_key
208+ if repo:
209+ add_apt_repo = ["add-apt-repository", "-y",
210+ "deb %s %s main" % (repo, series)]
211+ early[pre + '2_add'] = add_apt_repo
212+ late[pre + '3_add'] = in_target + add_apt_repo
213+ if package:
214+ early[pre + '3_update_install'] = [
215+ "sh", "-c", ("apt-get update --quiet && "
216+ "apt-get --assume-yes install " + package)]
217+ late[pre + '4_update_install'] = in_target + [
218+ "apt-get", "update", "--quite"]
219+ late[pre + '5_install'] = in_target + [
220+ "apt-get", "-y", "install", package]
221+ if module:
222+ early[pre + '4_load'] = [
223+ "sh", "-c",
224+ ("depmod && modprobe %s || " % module +
225+ "echo 'Warning: Failed to load module: %s" % module)]
226+
227+ late[pre + '6_depmod'] = in_target + ["depmod"]
228+ late[pre + '7_update_initramfs'] = in_target + ["update-initramfs", "-u"]
229+ return ret
230+
231+
232+def _get_curtin_base_config_data(context):
233+ config = {'debconf_selections': {'maas': context['curtin_preseed']}}
234+ config['late_commands'] = {
235+ 'maas': ["wget", "--no-proxy", context["node_disable_pxe_url"],
236+ "--post-data", context["node_disable_pxe_data"],
237+ "-O", "/dev/null"]}
238+
239+ # Ensure we always set debconf_selections for grub to ensure it doesn't
240+ # overwrite the config sent by MAAS. See LP: #1642298
241+ grub2_debconf = {'grub2': 'grub2 grub2/update_nvram boolean false'}
242+ if 'debconf_selections' in config:
243+ config['debconf_selections'].update(grub2_debconf)
244+ else:
245+ config['debconf_selections'] = grub2_debconf
246+
247+ # Precise does not support cloud-init performing the reboot, so curtin
248+ # must have this statement.
249+ if context['release'] == "precise":
250+ config['power_state'] = {'mode': 'reboot'}
251+
252+ return config
253+
254+
255+def get_curtin_configs_ubuntu(node):
256+ """Return primary curtin config for ubuntu."""
257+ context = get_curtin_preseed_context(node)
258+ driver = context.get('driver', {})
259+ series = node.get_distro_series()
260+ return [yaml.safe_dump(c) for c in [
261+ _get_curtin_base_config_data(context),
262+ get_third_party_drivers_config_data(
263+ enabled=context.get('third_party_drivers'),
264+ key=driver.get('key_binary'),
265+ repo=driver.get('repository'),
266+ package=driver.get('package'),
267+ module=driver.get('module'),
268+ series=series)]]
269+
270+
271+def get_curtin_configs_other(node):
272+ """Return primary curtin config for non-ubuntu."""
273+ context = get_curtin_preseed_context(node)
274+ config = _get_curtin_base_config_data(context)
275+ if context['osystem'] == "windows":
276+ config['license_key'] = node.get_effective_license_key()
277+
278+ return [yaml.safe_dump(config)]
279+
280+
281+def get_curtin_configs(node):
282+ """Return the primary curtin configuration.
283+
284+ :param node: The node for which to generate the configuration.
285+ :rtype: unicode."""
286+ if node.get_osystem() == "ubuntu":
287+ return get_curtin_configs_ubuntu(node)
288+ return get_curtin_configs_other(node)
289+
290+
291+def get_curtin_custom_config(node):
292+ """Return the curtin configuration to be used by curtin.pack_install.
293+
294+ This reads and renders the template files in /etc/maas/preseeds.
295+
296+ :param node: The node for which to generate the configuration.
297+ :rtype: unicode.
298+ """
299+ context = get_curtin_preseed_context(node)
300+ osystem = node.get_osystem()
301+ series = node.get_distro_series()
302+ template = load_preseed_template(
303+ node, USERDATA_TYPE.CURTIN, osystem, series)
304 deprecated_context_variables = [
305 'main_archive_hostname', 'main_archive_directory',
306 'ports_archive_hostname', 'ports_archive_directory',
307@@ -507,6 +632,8 @@ def get_curtin_config(node, default_region_ip=None):
308 deprecated_context_variables.remove(var)
309 context.update(get_node_deprecated_preseed_context())
310 config = yaml.safe_load(template.substitute(**context))
311+ if not config:
312+ config = {}
313 # Remove deprecated config from the curtin preseed.
314 if 'power_state' in config:
315 del config['power_state']
316diff --git a/src/maasserver/tests/test_node_action.py b/src/maasserver/tests/test_node_action.py
317index a9368e0..79c46ce 100644
318--- a/src/maasserver/tests/test_node_action.py
319+++ b/src/maasserver/tests/test_node_action.py
320@@ -514,12 +514,12 @@ class TestDeployAction(MAASServerTestCase):
321 node = factory.make_Node(
322 interface=True, status=NODE_STATUS.ALLOCATED,
323 power_type='manual', owner=user)
324- mock_get_curtin_config = self.patch(
325- node_action_module, 'get_curtin_config')
326+ mock_get_curtin_configs = self.patch(
327+ node_action_module, 'get_curtin_configs')
328 mock_node_start = self.patch(node, 'start')
329 Deploy(node, user).execute()
330 self.expectThat(
331- mock_get_curtin_config, MockCalledOnceWith(node))
332+ mock_get_curtin_configs, MockCalledOnceWith(node))
333 self.expectThat(
334 mock_node_start, MockCalledOnceWith(user))
335
336@@ -528,9 +528,9 @@ class TestDeployAction(MAASServerTestCase):
337 node = factory.make_Node(
338 interface=True, status=NODE_STATUS.ALLOCATED,
339 power_type='manual', owner=user)
340- mock_get_curtin_config = self.patch(
341- node_action_module, 'get_curtin_config')
342- mock_get_curtin_config.side_effect = NodeActionError('error')
343+ mock_get_curtin_configs = self.patch(
344+ node_action_module, 'get_curtin_configs')
345+ mock_get_curtin_configs.side_effect = NodeActionError('error')
346 error = self.assertRaises(
347 NodeActionError, Deploy(node, user).execute)
348 self.assertEqual(
349@@ -559,8 +559,8 @@ class TestDeployAction(MAASServerTestCase):
350 node = factory.make_Node(
351 interface=True, status=NODE_STATUS.ALLOCATED,
352 power_type='manual', owner=user)
353- mock_get_curtin_config = self.patch(
354- node_action_module, 'get_curtin_config')
355+ mock_get_curtin_configs = self.patch(
356+ node_action_module, 'get_curtin_configs')
357 mock_node_start = self.patch(node, 'start')
358 osystem = make_usable_osystem(self)
359 os_name = osystem["name"]
360@@ -571,7 +571,7 @@ class TestDeployAction(MAASServerTestCase):
361 }
362 Deploy(node, user).execute(**extra)
363 self.expectThat(
364- mock_get_curtin_config, MockCalledOnceWith(node))
365+ mock_get_curtin_configs, MockCalledOnceWith(node))
366 self.expectThat(
367 mock_node_start, MockCalledOnceWith(user))
368 self.expectThat(node.osystem, Equals(os_name))
369@@ -583,8 +583,8 @@ class TestDeployAction(MAASServerTestCase):
370 node = factory.make_Node(
371 interface=True, status=NODE_STATUS.ALLOCATED,
372 power_type='manual', owner=user)
373- mock_get_curtin_config = self.patch(
374- node_action_module, 'get_curtin_config')
375+ mock_get_curtin_configs = self.patch(
376+ node_action_module, 'get_curtin_configs')
377 mock_node_start = self.patch(node, 'start')
378 osystem = make_usable_osystem(self)
379 os_name = osystem["name"]
380@@ -595,7 +595,7 @@ class TestDeployAction(MAASServerTestCase):
381 }
382 Deploy(node, user).execute(**extra)
383 self.expectThat(
384- mock_get_curtin_config, MockCalledOnceWith(node))
385+ mock_get_curtin_configs, MockCalledOnceWith(node))
386 self.expectThat(
387 mock_node_start, MockCalledOnceWith(user))
388 self.expectThat(node.osystem, Equals(os_name))
389@@ -607,8 +607,8 @@ class TestDeployAction(MAASServerTestCase):
390 node = factory.make_Node(
391 interface=True, status=NODE_STATUS.ALLOCATED,
392 power_type='manual', owner=user)
393- mock_get_curtin_config = self.patch(
394- node_action_module, 'get_curtin_config')
395+ mock_get_curtin_configs = self.patch(
396+ node_action_module, 'get_curtin_configs')
397 mock_node_start = self.patch(node, 'start')
398 osystem = make_osystem_with_releases(self)
399 extra = {
400@@ -616,7 +616,7 @@ class TestDeployAction(MAASServerTestCase):
401 }
402 Deploy(node, user).execute(**extra)
403 self.expectThat(
404- mock_get_curtin_config, MockCalledOnceWith(node))
405+ mock_get_curtin_configs, MockCalledOnceWith(node))
406 self.expectThat(
407 mock_node_start, MockCalledOnceWith(user))
408 self.expectThat(node.osystem, Equals(""))
409@@ -627,8 +627,8 @@ class TestDeployAction(MAASServerTestCase):
410 node = factory.make_Node(
411 interface=True, status=NODE_STATUS.ALLOCATED,
412 power_type='manual', owner=user)
413- mock_get_curtin_config = self.patch(
414- node_action_module, 'get_curtin_config')
415+ mock_get_curtin_configs = self.patch(
416+ node_action_module, 'get_curtin_configs')
417 mock_node_start = self.patch(node, 'start')
418 osystem = make_osystem_with_releases(self)
419 extra = {
420@@ -636,7 +636,7 @@ class TestDeployAction(MAASServerTestCase):
421 }
422 Deploy(node, user).execute(**extra)
423 self.expectThat(
424- mock_get_curtin_config, MockCalledOnceWith(node))
425+ mock_get_curtin_configs, MockCalledOnceWith(node))
426 self.expectThat(
427 mock_node_start, MockCalledOnceWith(user))
428 self.expectThat(node.osystem, Equals(""))
429@@ -645,14 +645,14 @@ class TestDeployAction(MAASServerTestCase):
430 def test_Deploy_allocates_node_if_node_not_already_allocated(self):
431 user = factory.make_User()
432 node = factory.make_Node(status=NODE_STATUS.READY, with_boot_disk=True)
433- mock_get_curtin_config = self.patch(
434- node_action_module, 'get_curtin_config')
435+ mock_get_curtin_configs = self.patch(
436+ node_action_module, 'get_curtin_configs')
437 mock_node_start = self.patch(node, 'start')
438 action = Deploy(node, user)
439 action.execute()
440
441 self.expectThat(
442- mock_get_curtin_config, MockCalledOnceWith(node))
443+ mock_get_curtin_configs, MockCalledOnceWith(node))
444 self.expectThat(
445 mock_node_start, MockCalledOnceWith(user))
446 self.expectThat(user, Equals(node.owner))
447diff --git a/src/maasserver/tests/test_preseed.py b/src/maasserver/tests/test_preseed.py
448index 6f5e7d6..bde4bc9 100644
449--- a/src/maasserver/tests/test_preseed.py
450+++ b/src/maasserver/tests/test_preseed.py
451@@ -13,6 +13,7 @@ from textwrap import dedent
452 from unittest.mock import sentinel
453 from urllib.parse import urlparse
454
455+from curtin.config import merge_config
456 from django.conf import settings
457 from maasserver import preseed as preseed_module
458 from maasserver.clusterrpc.testing.boot_images import make_rpc_boot_image
459@@ -47,7 +48,7 @@ from maasserver.preseed import (
460 curtin_maas_reporter,
461 GENERIC_FILENAME,
462 get_curtin_cloud_config,
463- get_curtin_config,
464+ get_curtin_configs,
465 get_curtin_context,
466 get_curtin_image,
467 get_curtin_installer_url,
468@@ -126,6 +127,14 @@ class BootImageHelperMixin:
469 preseed_module,
470 'get_boot_images_for').return_value = [boot_image]
471
472+ def get_merged_config(self, result):
473+ """Return the fully merged config from the list of yaml_blobs
474+ that is provided in result (as returned by get_curtin_configs)."""
475+ ret = {}
476+ for yaml_blob in result:
477+ merge_config(ret, yaml.safe_load(yaml_blob))
478+ return ret
479+
480
481 class TestSplitSubArch(MAASServerTestCase):
482 """Tests for `split_subarch`."""
483@@ -977,8 +986,9 @@ class TestRenderCurtinUserdataWithThirdPartyDrivers(
484 get_third_party_driver = self.patch(
485 preseed_module, "get_third_party_driver")
486 get_third_party_driver.return_value = self.driver
487- curtin_config_text = get_curtin_config(node)
488- config = yaml.safe_load(curtin_config_text)
489+ config = self.get_merged_config(get_curtin_configs(node))
490+ self.assertThat(config, Contains('early_commands'))
491+
492 self.assertThat(
493 config['early_commands'], Contains('driver_00_get_key'))
494 self.assertThat(
495@@ -1035,11 +1045,11 @@ class TestCurtinUtilities(
496 node = factory.make_Node_with_Interface_on_Subnet(
497 primary_rack=self.rpc_rack_controller)
498 self.configure_get_boot_images_for_node(node, 'xinstall')
499- config = get_curtin_config(node)
500- self.assertThat(
501- config,
502- Contains("debconf_selections:"))
503- self.assertThat(config, Not(Contains('mode: reboot')))
504+ config = self.get_merged_config(get_curtin_configs(node))
505+ self.assertIsNotNone(config.get('early_commands'),
506+ "Did not find 'early_commands' entry")
507+ self.assertNotEqual(
508+ config.get('power_state', {}).get('mode', None), 'reboot')
509
510 def test_get_curtin_config_removes_power_state(self):
511 node = factory.make_Node_with_Interface_on_Subnet(
512@@ -1051,8 +1061,9 @@ class TestCurtinUtilities(
513 """)
514 self.patch(preseed_module, "get_preseed_template").return_value = (
515 factory.make_name("filename"), power_state_template)
516- config = get_curtin_config(node)
517- self.assertThat(config, Not(Contains('mode: reboot')))
518+ config = self.get_merged_config(get_curtin_configs(node))
519+ self.assertNotEqual(
520+ 'reboot', config.get('power_state', {}).get('mode'), 'reboot')
521
522 def test_get_curtin_config_removes_apt_mirrors(self):
523 node = factory.make_Node_with_Interface_on_Subnet(
524@@ -1065,9 +1076,9 @@ class TestCurtinUtilities(
525 """)
526 self.patch(preseed_module, "get_preseed_template").return_value = (
527 factory.make_name("filename"), apt_mirrors_template)
528- config = get_curtin_config(node)
529- self.assertThat(config, Not(Contains('ubuntu_archive')))
530- self.assertThat(config, Not(Contains('ubuntu_security')))
531+ config = self.get_merged_config(get_curtin_configs(node))
532+ self.assertNotIn('ubuntu_archive', config.get('apt_mirrors', {}))
533+ self.assertNotIn('ubuntu_security', config.get('apt_mirrors', {}))
534
535 def test_get_curtin_config_removes_apt_proxy(self):
536 node = factory.make_Node_with_Interface_on_Subnet(
537@@ -1078,8 +1089,9 @@ class TestCurtinUtilities(
538 """)
539 self.patch(preseed_module, "get_preseed_template").return_value = (
540 factory.make_name("filename"), apt_proxy_template)
541- config = get_curtin_config(node)
542- self.assertThat(config, Not(Contains('127.0.0.1')))
543+ config = self.get_merged_config(get_curtin_configs(node))
544+ self.assertThat(config.get('apt_proxy', ''),
545+ Not(Contains('127.0.0.1')))
546
547 def test_get_curtin_config_contains_reboot_for_precise(self):
548 node = factory.make_Node_with_Interface_on_Subnet(
549@@ -1087,8 +1099,8 @@ class TestCurtinUtilities(
550 node.distro_series = "precise"
551 node.save()
552 self.configure_get_boot_images_for_node(node, 'xinstall')
553- config = get_curtin_config(node)
554- self.assertThat(config, Contains('mode: reboot'))
555+ config = self.get_merged_config(get_curtin_configs(node))
556+ self.assertEqual(config['power_state']['mode'], "reboot")
557
558 def test_get_curtin_config_with_ipv4_rack_url(self):
559 primary_rack = self.rpc_rack_controller
560@@ -1097,13 +1109,12 @@ class TestCurtinUtilities(
561 node = factory.make_Node_with_Interface_on_Subnet(
562 primary_rack=primary_rack)
563 self.configure_get_boot_images_for_node(node, 'xinstall')
564- config = get_curtin_config(node)
565- yaml_conf = yaml.safe_load(config)
566+ config = self.get_merged_config(get_curtin_configs(node))
567 self.assertEqual(
568 "%smetadata/latest/by-id/%s/" % (
569 primary_rack.url, node.system_id),
570- yaml_conf['late_commands']['maas'][2])
571- self.assertTrue('debconf_selections' in yaml_conf)
572+ config['late_commands']['maas'][2])
573+ self.assertTrue('debconf_selections' in config)
574
575 def test_get_curtin_config_with_ipv6_rack_url(self):
576 primary_rack = self.rpc_rack_controller
577@@ -1112,13 +1123,12 @@ class TestCurtinUtilities(
578 node = factory.make_Node_with_Interface_on_Subnet(
579 primary_rack=primary_rack)
580 self.configure_get_boot_images_for_node(node, 'xinstall')
581- config = get_curtin_config(node)
582- yaml_conf = yaml.safe_load(config)
583+ config = self.get_merged_config(get_curtin_configs(node))
584 self.assertEqual(
585 "%smetadata/latest/by-id/%s/" % (
586 primary_rack.url, node.system_id),
587- yaml_conf['late_commands']['maas'][2])
588- self.assertTrue('debconf_selections' in yaml_conf)
589+ config['late_commands']['maas'][2])
590+ self.assertTrue('debconf_selections' in config)
591
592 def test_get_curtin_config_with_name_rack_url(self):
593 primary_rack = self.rpc_rack_controller
594@@ -1127,13 +1137,12 @@ class TestCurtinUtilities(
595 node = factory.make_Node_with_Interface_on_Subnet(
596 primary_rack=primary_rack)
597 self.configure_get_boot_images_for_node(node, 'xinstall')
598- config = get_curtin_config(node)
599- yaml_conf = yaml.safe_load(config)
600+ config = self.get_merged_config(get_curtin_configs(node))
601 self.assertEqual(
602 "%smetadata/latest/by-id/%s/" % (
603 primary_rack.url, node.system_id),
604- yaml_conf['late_commands']['maas'][2])
605- self.assertTrue('debconf_selections' in yaml_conf)
606+ config['late_commands']['maas'][2])
607+ self.assertTrue('debconf_selections' in config)
608
609 def test_get_curtin_config_with_quote_rack_url(self):
610 primary_rack = self.rpc_rack_controller
611@@ -1142,22 +1151,21 @@ class TestCurtinUtilities(
612 node = factory.make_Node_with_Interface_on_Subnet(
613 primary_rack=primary_rack)
614 self.configure_get_boot_images_for_node(node, 'xinstall')
615- config = get_curtin_config(node)
616- yaml_conf = yaml.safe_load(config)
617+ config = self.get_merged_config(get_curtin_configs(node))
618 self.assertEqual(
619 "%smetadata/latest/by-id/%s/" % (primary_rack.url, node.system_id),
620- yaml_conf['late_commands']['maas'][2])
621- self.assertTrue('debconf_selections' in yaml_conf)
622+ config['late_commands']['maas'][2])
623+ self.assertTrue('debconf_selections' in config)
624
625 def test_get_curtin_config_has_grub2_debconf_selections(self):
626 node = factory.make_Node_with_Interface_on_Subnet(
627 primary_rack=self.rpc_rack_controller)
628 node.save()
629 self.configure_get_boot_images_for_node(node, 'xinstall')
630- config = get_curtin_config(node)
631- self.assertThat(
632- config,
633- Contains('grub2: grub2 grub2/update_nvram boolean false'))
634+ config = self.get_merged_config(get_curtin_configs(node))
635+ self.assertEqual(
636+ config['debconf_selections']['grub2'].split(),
637+ ['grub2', 'grub2/update_nvram', 'boolean', 'false'])
638
639 def make_fastpath_node(self, main_arch=None):
640 """Return a `Node`, with FPI enabled, and the given main architecture.

Subscribers

People subscribed via source and target branches