MAAS

Merge ~adam-collard/maas:clone-storage-errors into maas:master

  1. Git
  2. lp:~adam-collard/maas
  3. clone-storage-errors
  4. Merge into master
Proposed by Adam Collard
Status: Merged
Approved by: Adam Collard
Approved revision: 9d4e59fef8fb4abf969acb5b88bc8c7989643a37
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~adam-collard/maas:clone-storage-errors
Merge into: maas:master
Diff against target: 361 lines (+170/-38)
5 files modified
src/maasserver/forms/clone.py (+46/-17)
src/maasserver/forms/tests/test_clone.py (+8/-12)
src/maasserver/node_action.py (+31/-4)
src/maasserver/websockets/handlers/machine.py (+1/-1)
src/maasserver/websockets/handlers/tests/test_machine.py (+84/-4)
Reviewer Review Type Date Requested Status
MAAS Lander Approve
Alberto Donato (community) Approve
Review via email: mp+407628@code.launchpad.net

Commit message

Rework errors of CloneForm

Retry the form after removing the invalid destinations. Persist errors
from the first attempt though.

To post a comment you must log in.
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b clone-storage-errors lp:~adam-collard/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10757/console
COMMIT: 8626d9d51b15e10e3c60528292fdc470f63c0eef

review: Needs Fixing
Revision history for this message
Adam Collard (adam-collard) :
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b clone-storage-errors lp:~adam-collard/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10760/console
COMMIT: 046224d69584edd5f2eb16b592105b1fb35670ac

review: Needs Fixing
Revision history for this message
Adam Collard (adam-collard) wrote :

jenkins: !test

Revision history for this message
Alberto Donato (ack) wrote :

nice, +1!

one small nit inline

review: Approve
Revision history for this message
Adam Collard (adam-collard) :
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b clone-storage-errors lp:~adam-collard/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10762/console
COMMIT: 046224d69584edd5f2eb16b592105b1fb35670ac

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

LANDING
-b clone-storage-errors lp:~adam-collard/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/10766/consoleText

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

UNIT TESTS
-b clone-storage-errors lp:~adam-collard/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 9d4e59fef8fb4abf969acb5b88bc8c7989643a37

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/forms/clone.py b/src/maasserver/forms/clone.py
2index fd76ee1..85900c7 100644
3--- a/src/maasserver/forms/clone.py
4+++ b/src/maasserver/forms/clone.py
5@@ -31,7 +31,10 @@ class CloneForm(forms.Form):
6 label="Destinations",
7 min_length=1,
8 error_messages={
9- "item_invalid": "Machine %(nth)s in the array did not validate:"
10+ "item_invalid": "Machine %(nth)s is invalid:",
11+ "is-source": "Source machine %(machine)s cannot be a destination machine.",
12+ "storage": "%(machine)s is invalid:",
13+ "networking": "%(machine)s is invalid:",
14 },
15 help_text="The destinations to clone to.",
16 )
17@@ -75,19 +78,18 @@ class CloneForm(forms.Form):
18 self.add_error("source", "This field is required.")
19 destinations = self.cleaned_data.get("destinations")
20 destination_field = self.fields["destinations"]
21- item_invalid = destination_field.error_messages["item_invalid"]
22 storage = self.cleaned_data.get("storage", False)
23 interfaces = self.cleaned_data.get("interfaces", False)
24 if source and destinations:
25- for index, dest in enumerate(destinations, 1):
26+ for dest in destinations:
27 if source == dest:
28- error = prefix_validation_error(
29- ValidationError(
30- "Source machine cannot be a destination machine."
31- ),
32- prefix=item_invalid,
33- code="item_invalid",
34- params={"nth": index},
35+ error = ValidationError(
36+ destination_field.error_messages["is-source"],
37+ code="is-source",
38+ params={
39+ "machine": str(dest),
40+ "system_id": dest.system_id,
41+ },
42 )
43 self.add_error("destinations", error)
44 else:
45@@ -97,9 +99,14 @@ class CloneForm(forms.Form):
46 except ValidationError as exc:
47 error = prefix_validation_error(
48 exc,
49- prefix=item_invalid,
50- code="item_invalid",
51- params={"nth": index},
52+ prefix=destination_field.error_messages[
53+ "storage"
54+ ],
55+ code="storage",
56+ params={
57+ "machine": str(dest),
58+ "system_id": dest.system_id,
59+ },
60 )
61 self.add_error("destinations", error)
62 if interfaces:
63@@ -108,9 +115,14 @@ class CloneForm(forms.Form):
64 except ValidationError as exc:
65 error = prefix_validation_error(
66 exc,
67- prefix=item_invalid,
68- code="item_invalid",
69- params={"nth": index},
70+ prefix=destination_field.error_messages[
71+ "networking"
72+ ],
73+ code="networking",
74+ params={
75+ "machine": str(dest),
76+ "system_id": dest.system_id,
77+ },
78 )
79 self.add_error("destinations", error)
80 if not storage and not interfaces:
81@@ -123,11 +135,28 @@ class CloneForm(forms.Form):
82 )
83 return cleaned_data
84
85+ def strip_failed_destinations(self):
86+ """Remove destinations that have errors."""
87+ if "destinations" not in self.cleaned_data:
88+ submitted_destinations = self.data.get("destinations")
89+ # Don't try and manipulate empty submission
90+ if not submitted_destinations:
91+ return
92+ errors = self.errors.as_data()
93+ bogus_system_ids = {
94+ error.params["system_id"] for error in errors["destinations"]
95+ }
96+ return [
97+ dest
98+ for dest in submitted_destinations
99+ if dest not in bogus_system_ids
100+ ]
101+
102 def save(self):
103 """Clone the storage and/or interfaces configuration to the
104 destinations."""
105 source = self.cleaned_data.get("source")
106- destinations = self.cleaned_data.get("destinations")
107+ destinations = self.cleaned_data.get("destinations", [])
108 storage = self.cleaned_data.get("storage", False)
109 interfaces = self.cleaned_data.get("interfaces", False)
110 for dest in destinations:
111diff --git a/src/maasserver/forms/tests/test_clone.py b/src/maasserver/forms/tests/test_clone.py
112index d33e412..5275af8 100644
113--- a/src/maasserver/forms/tests/test_clone.py
114+++ b/src/maasserver/forms/tests/test_clone.py
115@@ -45,14 +45,13 @@ class TestCloneForm(MAASServerTestCase):
116 self.assertEqual(
117 {
118 "destinations": [
119- "Machine 1 in the array did not validate: "
120- "Source machine cannot be a destination machine."
121+ f"Source machine {source} cannot be a destination machine.",
122 ]
123 },
124 form.errors,
125 )
126
127- def test_source_destination_smaller_storage(self):
128+ def test_destination_smaller_storage(self):
129 user = factory.make_admin()
130 source = factory.make_Machine(with_boot_disk=False)
131 factory.make_PhysicalBlockDevice(
132@@ -79,7 +78,7 @@ class TestCloneForm(MAASServerTestCase):
133 self.assertEqual(
134 {
135 "destinations": [
136- "Machine 1 in the array did not validate: "
137+ f"{destination} is invalid: "
138 "destination boot disk(sda) is smaller than "
139 "source boot disk(sda)"
140 ]
141@@ -113,9 +112,8 @@ class TestCloneForm(MAASServerTestCase):
142 self.assertEqual(
143 {
144 "destinations": [
145- "Machine 1 in the array did not validate: "
146- "Source machine cannot be a destination machine.",
147- "Machine 2 in the array did not validate: "
148+ f"Source machine {source} cannot be a destination machine.",
149+ f"{destination} is invalid: "
150 "destination boot disk(sda) is smaller than "
151 "source boot disk(sda)",
152 ]
153@@ -123,7 +121,7 @@ class TestCloneForm(MAASServerTestCase):
154 form.errors,
155 )
156
157- def test_source_destination_missing_nic(self):
158+ def test_destination_missing_nic(self):
159 user = factory.make_admin()
160 source = factory.make_Machine(with_boot_disk=False)
161 factory.make_Interface(node=source, name="eth0")
162@@ -146,7 +144,7 @@ class TestCloneForm(MAASServerTestCase):
163 self.assertEqual(
164 {
165 "destinations": [
166- "Machine 1 in the array did not validate: "
167+ f"{destination} is invalid: "
168 "destination node physical interfaces do not match "
169 "the source nodes physical interfaces: eth0"
170 ]
171@@ -184,9 +182,7 @@ class TestCloneForm(MAASServerTestCase):
172 self.assertEqual(
173 {
174 "destinations": [
175- "Machine 1 in the array did not validate: "
176- "Select a valid choice. %s is not one of the available "
177- "choices." % destination.system_id
178+ f"Machine 1 is invalid: Select a valid choice. {destination.system_id} is not one of the available choices."
179 ]
180 },
181 form.errors,
182diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py
183index 1f810cb..2162065 100644
184--- a/src/maasserver/node_action.py
185+++ b/src/maasserver/node_action.py
186@@ -969,18 +969,45 @@ class Clone(NodeAction):
187 def get_node_action_audit_description(self, action):
188 return self.audit_description % action.node.hostname
189
190- def _execute(self, destinations=None, storage=False, interfaces=False):
191- source = self.node
192+ def _execute(
193+ self,
194+ destinations=None,
195+ storage=False,
196+ interfaces=False,
197+ _error_data=None,
198+ ):
199 data = {
200- "source": source,
201+ "source": self.node,
202 "destinations": destinations,
203 "storage": storage,
204 "interfaces": interfaces,
205 }
206 form = CloneForm(self.user, data=data)
207+ if form.has_error("destinations", "storage") or form.has_error(
208+ "destinations", "network"
209+ ):
210+ # Try again with all the bad destinations removed
211+ new_destinations = form.strip_failed_destinations()
212+ if new_destinations:
213+ return self._execute(
214+ destinations=new_destinations,
215+ storage=storage,
216+ interfaces=interfaces,
217+ _error_data=form.errors,
218+ )
219 if not form.is_valid():
220 raise NodeActionError(form.errors.as_json())
221- form.save()
222+ try:
223+ form.save()
224+ except ValidationError as exc:
225+ raise NodeActionError(exc.errors.as_json())
226+ if _error_data:
227+ for name, error_list in _error_data.items():
228+ if name in form.errors:
229+ form.errors[name].extend(error_list)
230+ else:
231+ form.errors[name] = error_list
232+ raise NodeActionError(form.errors.as_json())
233
234
235 ACTION_CLASSES = (
236diff --git a/src/maasserver/websockets/handlers/machine.py b/src/maasserver/websockets/handlers/machine.py
237index 4425c78..1016f2d 100644
238--- a/src/maasserver/websockets/handlers/machine.py
239+++ b/src/maasserver/websockets/handlers/machine.py
240@@ -949,7 +949,7 @@ class MachineHandler(NodeHandler):
241 action = actions.get(action_name)
242 if action is None:
243 raise NodeActionError(
244- "%s action is not available for this node." % action_name
245+ f"{action_name} action is not available for this node."
246 )
247 extra_params = params.get("extra", {})
248 return action.execute(**extra_params)
249diff --git a/src/maasserver/websockets/handlers/tests/test_machine.py b/src/maasserver/websockets/handlers/tests/test_machine.py
250index 7f34e00..cc00d6d 100644
251--- a/src/maasserver/websockets/handlers/tests/test_machine.py
252+++ b/src/maasserver/websockets/handlers/tests/test_machine.py
253@@ -46,6 +46,7 @@ from maasserver.enum import (
254 NODE_STATUS,
255 NODE_STATUS_CHOICES,
256 NODE_TYPE,
257+ PARTITION_TABLE_TYPE,
258 POWER_STATE,
259 )
260 from maasserver.exceptions import NodeActionError, NodeStateViolation
261@@ -3780,17 +3781,96 @@ class TestMachineHandler(MAASServerTestCase):
262 {
263 "destinations": [
264 {
265- "message": "Machine 1 in the array did not validate: destination boot disk(sda) is smaller than source boot disk(sda)",
266- "code": "item_invalid",
267+ "message": f"{destination1} is invalid: destination boot disk(sda) is smaller than source boot disk(sda)",
268+ "code": "storage",
269 },
270 {
271- "message": "Machine 2 in the array did not validate: destination boot disk(sda) is smaller than source boot disk(sda)",
272- "code": "item_invalid",
273+ "message": f"{destination2} is invalid: destination boot disk(sda) is smaller than source boot disk(sda)",
274+ "code": "storage",
275 },
276 ],
277 },
278 )
279
280+ def test_clone_where_possible(self):
281+ user = factory.make_admin()
282+ request = HttpRequest()
283+ request.user = user
284+ source = factory.make_Machine(with_boot_disk=False)
285+ boot_disk = factory.make_PhysicalBlockDevice(
286+ node=source, size=8 * 1024 ** 3, name="sda"
287+ )
288+ partition_table = factory.make_PartitionTable(
289+ table_type=PARTITION_TABLE_TYPE.MBR, block_device=boot_disk
290+ )
291+ efi_partition = factory.make_Partition(
292+ partition_table=partition_table,
293+ uuid="6efc2c3d-bc9d-4ee5-a7ed-c6e1574d5398",
294+ size=512 * 1024 ** 2,
295+ bootable=True,
296+ )
297+ root_partition = factory.make_Partition(
298+ partition_table=partition_table,
299+ uuid="f74ff260-2a5b-4a36-b1b8-37f746b946bf",
300+ size=2 * 1024 ** 3,
301+ bootable=False,
302+ )
303+ destination1 = factory.make_Machine(
304+ status=NODE_STATUS.READY, with_boot_disk=False
305+ )
306+ factory.make_PhysicalBlockDevice(
307+ node=destination1, size=1024 ** 3, name="sda"
308+ )
309+ destination2 = factory.make_Machine(
310+ status=NODE_STATUS.FAILED_TESTING,
311+ with_boot_disk=False,
312+ )
313+ factory.make_PhysicalBlockDevice(
314+ node=destination2, size=8 * 1024 ** 3, name="sda"
315+ )
316+
317+ handler = MachineHandler(user, {}, request)
318+ exc = self.assertRaises(
319+ NodeActionError,
320+ handler.action,
321+ {
322+ "system_id": source.system_id,
323+ "action": "clone",
324+ "extra": {
325+ "storage": True,
326+ "interfaces": False,
327+ "destinations": [
328+ destination1.system_id,
329+ destination2.system_id,
330+ ],
331+ },
332+ },
333+ )
334+ (errors,) = exc.args
335+ self.assertEqual(
336+ json.loads(errors),
337+ {
338+ "destinations": [
339+ {
340+ "message": f"{destination1} is invalid: destination boot disk(sda) is smaller than source boot disk(sda)",
341+ "code": "storage",
342+ },
343+ ],
344+ },
345+ )
346+ partition_table = (
347+ destination2.blockdevice_set.first().get_partitiontable()
348+ )
349+ self.assertIsNotNone(partition_table)
350+ partitions = partition_table.partitions
351+ maybe_efi, maybe_root, *rest = partitions.all()
352+ self.assertEqual(rest, [])
353+ self.assertEqual(maybe_efi.size, efi_partition.size)
354+ self.assertTrue(maybe_efi.bootable)
355+
356+ self.assertEqual(maybe_root.size, root_partition.size)
357+ self.assertFalse(maybe_root.bootable)
358+
359 def test_create_physical_creates_interface(self):
360 user = factory.make_admin()
361 node = factory.make_Node(interface=False)

Subscribers

People subscribed via source and target branches