Merge ~smoser/maas:cleanup/render-curtin-config-internally into maas:master
- Git
- lp:~smoser/maas
- cleanup/render-curtin-config-internally
- Merge into master
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) |
Related bugs: |
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.
Blake Rouse (blake-rouse) wrote : | # |
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:/
> Your team MAAS Committers is subscribed to branch maas:master.
>
--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.
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_
and
debconf_
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.
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_
debconf_
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.
Andres Rodriguez (andreserl) wrote : | # |
For 2.3, this is too late, so we will target this for 2.4 once it opens.
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b cleanup/
STATUS: FAILED
LOG: http://
COMMIT: d313a298a0eeb39
Ante Karamatić (ivoks) : | # |
Scott Moser (smoser) wrote : | # |
I addressed Ante's comment and updated to merge without conflicts on current master.
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b cleanup/
STATUS: FAILED
LOG: http://
COMMIT: addfc9a36144635
Scott Moser (smoser) wrote : | # |
I think this is ready for review at this point.
I've fixed up all the unit tests.
Mike Pontillo (mpontillo) wrote : | # |
This branch is looking much better, but where are the tests that cover the new code?
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b cleanup/
STATUS: FAILED
LOG: http://
COMMIT: 296fab455858b1b
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?
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?)
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:/
> You are reviewing the proposed merge of ~smoser/
> into maas:master.
>
--
Andres Rodriguez
Engineering Manager, MAAS
Canonical USA, Inc.
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b cleanup/
STATUS: FAILED
LOG: http://
COMMIT: d33d9f6b6da6b23
Scott Moser (smoser) wrote : | # |
Bump?
Mike Pontillo (mpontillo) wrote : | # |
@smoser, I'll take a closer look. Thanks for getting this work going.
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
1 | diff --git a/contrib/preseeds_v2/curtin_userdata b/contrib/preseeds_v2/curtin_userdata |
2 | index 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"] |
66 | diff --git a/contrib/preseeds_v2/curtin_userdata_centos b/contrib/preseeds_v2/curtin_userdata_centos |
67 | index 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. |
83 | diff --git a/contrib/preseeds_v2/curtin_userdata_windows b/contrib/preseeds_v2/curtin_userdata_windows |
84 | index 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. |
101 | diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py |
102 | index 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) |
123 | diff --git a/src/maasserver/preseed.py b/src/maasserver/preseed.py |
124 | index 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'] |
316 | diff --git a/src/maasserver/tests/test_node_action.py b/src/maasserver/tests/test_node_action.py |
317 | index 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)) |
447 | diff --git a/src/maasserver/tests/test_preseed.py b/src/maasserver/tests/test_preseed.py |
448 | index 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. |
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.