MAAS

Merge lp:~newell-jensen/maas/fix-1602487 into lp:~maas-committers/maas/trunk

  1. fix-1602487
  2. Merge into trunk
Proposed by Newell Jensen
Status: Merged
Approved by: Newell Jensen
Approved revision: no longer in the source branch.
Merged at revision: 5198
Proposed branch: lp:~newell-jensen/maas/fix-1602487
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 372 lines (+62/-67)
3 files modified
src/maasserver/models/node.py (+11/-8)
src/maasserver/models/tests/test_node.py (+45/-56)
src/maasserver/tests/test_node_action.py (+6/-3)
To merge this branch: bzr merge lp:~newell-jensen/maas/fix-1602487
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+300387@code.launchpad.net

Commit message

Factor out the addErrback for resetting the node's status into _start from start_commissioning. Makes it so if there is an error during deployment, the status will be reset.

To post a comment you must log in.
Revision history for this message
Newell Jensen (newell-jensen) wrote :

This was testing on OIL's integration server.

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/models/node.py'
2--- src/maasserver/models/node.py 2016-07-15 18:57:25 +0000
3+++ src/maasserver/models/node.py 2016-07-18 20:09:23 +0000
4@@ -1657,7 +1657,7 @@
5 # Node.start() has synchronous and asynchronous parts, so catch
6 # exceptions arising synchronously, and chain callbacks to the
7 # Deferred it returns for the asynchronous (post-commit) bits.
8- starting = self._start(user, commissioning_user_data)
9+ starting = self._start(user, commissioning_user_data, old_status)
10 except Exception as error:
11 self.status = old_status
12 self.save()
13@@ -1688,11 +1688,6 @@
14 callOut, self._start_commissioning_async, is_starting,
15 self.hostname)
16
17- # If there's an error, reset the node's status.
18- starting.addErrback(
19- callOutToDatabase, Node._set_status, self.system_id,
20- status=old_status)
21-
22 def eb_start(failure, hostname):
23 maaslog.error(
24 "%s: Could not start node for commissioning: %s",
25@@ -2135,6 +2130,7 @@
26
27 # Change the status of the node now to avoid races when starting
28 # nodes in bulk.
29+ old_status = self.status
30 self.status = NODE_STATUS.DISK_ERASING
31 self.save()
32
33@@ -2142,7 +2138,7 @@
34 # Node.start() has synchronous and asynchronous parts, so catch
35 # exceptions arising synchronously, and chain callbacks to the
36 # Deferred it returns for the asynchronous (post-commit) bits.
37- starting = self._start(user, disk_erase_user_data)
38+ starting = self._start(user, disk_erase_user_data, old_status)
39 except Exception as error:
40 # We always mark the node as failed here, although we could
41 # potentially move it back to the state it was in previously. For
42@@ -3019,7 +3015,7 @@
43 return client_idents, fallback_idents
44
45 @transactional
46- def _start(self, user, user_data=None):
47+ def _start(self, user, user_data=None, old_status=None):
48 """Request on given user's behalf that the node be started up.
49
50 :param user: Requesting user.
51@@ -3055,6 +3051,7 @@
52 NodeUserData.objects.set_user_data(self, user_data)
53
54 if self.status == NODE_STATUS.ALLOCATED:
55+ old_status = self.status
56 # Claim AUTO IP addresses for the node if it's ALLOCATED.
57 # The current state being ALLOCATED is our indication that the node
58 # is being deployed for the first time.
59@@ -3084,6 +3081,12 @@
60 callOutToDatabase, Node._set_status_expires,
61 self.system_id, deployment_timeout)
62
63+ if old_status is not None:
64+ # If there's an error, reset the node's status.
65+ d.addErrback(
66+ callOutToDatabase, Node._set_status, self.system_id,
67+ status=old_status)
68+
69 # If any part of this processes fails be sure to release the grabbed
70 # auto IP addresses.
71 if claimed_ips:
72
73=== modified file 'src/maasserver/models/tests/test_node.py'
74--- src/maasserver/models/tests/test_node.py 2016-07-15 18:57:25 +0000
75+++ src/maasserver/models/tests/test_node.py 2016-07-18 20:09:23 +0000
76@@ -1414,7 +1414,8 @@
77 status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
78 node_start = self.patch(node, '_start')
79 # Return a post-commit hook from Node.start().
80- node_start.side_effect = lambda user, user_data: post_commit()
81+ node_start.side_effect = (
82+ lambda user, user_data, old_status: post_commit())
83 Config.objects.set_config('disk_erase_with_secure_erase', True)
84 Config.objects.set_config('disk_erase_with_quick_erase', True)
85 with post_commit_hooks:
86@@ -1435,7 +1436,8 @@
87 status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
88 node_start = self.patch(node, '_start')
89 # Return a post-commit hook from Node.start().
90- node_start.side_effect = lambda user, user_data: post_commit()
91+ node_start.side_effect = (
92+ lambda user, user_data, old_status: post_commit())
93 Config.objects.set_config('disk_erase_with_secure_erase', False)
94 Config.objects.set_config('disk_erase_with_quick_erase', False)
95 with post_commit_hooks:
96@@ -1456,26 +1458,28 @@
97 status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
98 node_start = self.patch(node, '_start')
99 # Return a post-commit hook from Node.start().
100- node_start.side_effect = lambda user, user_data: post_commit()
101+ node_start.side_effect = (
102+ lambda user, user_data, old_status: post_commit())
103 with post_commit_hooks:
104 node.start_disk_erasing(owner)
105 self.expectThat(node.owner, Equals(owner))
106 self.expectThat(node.status, Equals(NODE_STATUS.DISK_ERASING))
107 self.expectThat(node.agent_name, Equals(agent_name))
108 self.assertThat(
109- node_start, MockCalledOnceWith(owner, ANY))
110+ node_start, MockCalledOnceWith(owner, ANY, NODE_STATUS.ALLOCATED))
111
112 def test_start_disk_erasing_logs_user_request(self):
113 owner = factory.make_User()
114 node = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=owner)
115 node_start = self.patch(node, '_start')
116 # Return a post-commit hook from Node.start().
117- node_start.side_effect = lambda user, user_data: post_commit()
118+ node_start.side_effect = (
119+ lambda user, user_data, old_status: post_commit())
120 register_event = self.patch(node, '_register_request_event')
121 with post_commit_hooks:
122 node.start_disk_erasing(owner)
123 self.assertThat(
124- node_start, MockCalledOnceWith(owner, ANY))
125+ node_start, MockCalledOnceWith(owner, ANY, NODE_STATUS.ALLOCATED))
126 self.assertThat(register_event, MockCalledOnceWith(
127 owner, EVENT_TYPES.REQUEST_NODE_ERASE_DISK,
128 action='start disk erasing', comment=None))
129@@ -1538,7 +1542,7 @@
130
131 self.assertThat(
132 node_start, MockCalledOnceWith(
133- admin, generate_user_data.return_value))
134+ admin, generate_user_data.return_value, NODE_STATUS.ALLOCATED))
135 self.assertEqual(NODE_STATUS.FAILED_DISK_ERASING, node.status)
136
137 def test_start_disk_erasing_sets_status_on_post_commit_error(self):
138@@ -2159,7 +2163,8 @@
139 interface=True, status=NODE_STATUS.NEW, power_type='manual')
140 node_start = self.patch(node, '_start')
141 # Return a post-commit hook from Node.start().
142- node_start.side_effect = lambda user, user_data: post_commit()
143+ node_start.side_effect = (
144+ lambda user, user_data, old_status: post_commit())
145 admin = factory.make_admin()
146 node.start_commissioning(admin)
147 post_commit_hooks.reset() # Ignore these for now.
148@@ -2168,7 +2173,8 @@
149 'status': NODE_STATUS.COMMISSIONING,
150 }
151 self.assertAttributes(node, expected_attrs)
152- self.assertThat(node_start, MockCalledOnceWith(admin, ANY))
153+ self.assertThat(node_start, MockCalledOnceWith(
154+ admin, ANY, NODE_STATUS.NEW))
155
156 def test_start_commissioning_sets_options(self):
157 rack = factory.make_RackController()
158@@ -2177,7 +2183,8 @@
159 bmc_connected_to=rack)
160 node_start = self.patch(node, '_start')
161 # Return a post-commit hook from Node.start().
162- node_start.side_effect = lambda user, user_data: post_commit()
163+ node_start.side_effect = (
164+ lambda user, user_data, old_status: post_commit())
165 admin = factory.make_admin()
166 enable_ssh = factory.pick_bool()
167 skip_networking = factory.pick_bool()
168@@ -2197,7 +2204,8 @@
169 def test_start_commissioning_sets_user_data(self):
170 node = factory.make_Node(status=NODE_STATUS.NEW)
171 node_start = self.patch(node, '_start')
172- node_start.side_effect = lambda user, user_data: post_commit()
173+ node_start.side_effect = (
174+ lambda user, user_data, old_status: post_commit())
175 user_data = factory.make_string().encode('ascii')
176 generate_user_data = self.patch(
177 commissioning, 'generate_user_data')
178@@ -2205,12 +2213,14 @@
179 admin = factory.make_admin()
180 node.start_commissioning(admin)
181 post_commit_hooks.reset() # Ignore these for now.
182- self.assertThat(node_start, MockCalledOnceWith(admin, user_data))
183+ self.assertThat(node_start, MockCalledOnceWith(
184+ admin, user_data, NODE_STATUS.NEW))
185
186 def test_start_commissioning_sets_min_hwe_kernel(self):
187 node = factory.make_Node(status=NODE_STATUS.NEW)
188 node_start = self.patch(node, '_start')
189- node_start.side_effect = lambda user, user_data: post_commit()
190+ node_start.side_effect = (
191+ lambda user, user_data, old_status: post_commit())
192 user_data = factory.make_string().encode('ascii')
193 generate_user_data = self.patch(
194 commissioning, 'generate_user_data')
195@@ -2224,7 +2234,8 @@
196 def test_start_commissioning_clears_node_commissioning_results(self):
197 node = factory.make_Node(status=NODE_STATUS.NEW)
198 node_start = self.patch(node, '_start')
199- node_start.side_effect = lambda user, user_data: post_commit()
200+ node_start.side_effect = (
201+ lambda user, user_data, old_status: post_commit())
202 NodeResult.objects.store_data(
203 node, factory.make_string(),
204 random.randint(0, 10),
205@@ -2237,7 +2248,8 @@
206 def test_start_commissioning_clears_storage_configuration(self):
207 node = factory.make_Node(status=NODE_STATUS.NEW)
208 node_start = self.patch(node, '_start')
209- node_start.side_effect = lambda user, user_data: post_commit()
210+ node_start.side_effect = (
211+ lambda user, user_data, old_status: post_commit())
212 clear_storage = self.patch_autospec(
213 node, '_clear_full_storage_configuration')
214 admin = factory.make_admin()
215@@ -2248,7 +2260,8 @@
216 def test_start_commissioning_doesnt_clear_storage_configuration(self):
217 node = factory.make_Node(status=NODE_STATUS.NEW)
218 node_start = self.patch(node, '_start')
219- node_start.side_effect = lambda user, user_data: post_commit()
220+ node_start.side_effect = (
221+ lambda user, user_data, old_status: post_commit())
222 clear_storage = self.patch_autospec(
223 node, '_clear_full_storage_configuration')
224 admin = factory.make_admin()
225@@ -2259,7 +2272,8 @@
226 def test_start_commissioning_calls__clear_networking_configuration(self):
227 node = factory.make_Node(status=NODE_STATUS.NEW)
228 node_start = self.patch(node, '_start')
229- node_start.side_effect = lambda user, user_data: post_commit()
230+ node_start.side_effect = (
231+ lambda user, user_data, old_status: post_commit())
232 clear_networking = self.patch_autospec(
233 node, '_clear_networking_configuration')
234 admin = factory.make_admin()
235@@ -2270,7 +2284,8 @@
236 def test_start_commissioning_doesnt_call__clear_networking(self):
237 node = factory.make_Node(status=NODE_STATUS.NEW)
238 node_start = self.patch(node, '_start')
239- node_start.side_effect = lambda user, user_data: post_commit()
240+ node_start.side_effect = (
241+ lambda user, user_data, old_status: post_commit())
242 clear_networking = self.patch_autospec(
243 node, '_clear_networking_configuration')
244 admin = factory.make_admin()
245@@ -2313,41 +2328,10 @@
246
247 self.assertThat(
248 node_start,
249- MockCalledOnceWith(admin, generate_user_data.return_value))
250+ MockCalledOnceWith(
251+ admin, generate_user_data.return_value, NODE_STATUS.NEW))
252 self.assertEqual(NODE_STATUS.NEW, node.status)
253
254- def test_start_commissioning_reverts_status_on_post_commit_error(self):
255- # When start_commissioning encounters an error in its post-commit
256- # hook, it will revert the node to its previous status.
257- admin = factory.make_admin()
258- status = random.choice(
259- (NODE_STATUS.NEW, NODE_STATUS.READY,
260- NODE_STATUS.FAILED_COMMISSIONING))
261- node = factory.make_Node(status=status)
262- # Patch out some things that we don't want to do right now.
263- self.patch(Node, '_set_status_expires')
264- self.patch(node, '_start').return_value = None
265- # Fake an error during the post-commit hook.
266- error_message = factory.make_name("error")
267- error_type = factory.make_exception_type()
268- _start_async = self.patch_autospec(node, "_start_commissioning_async")
269- _start_async.side_effect = error_type(error_message)
270- # Capture calls to _set_status.
271- self.patch_autospec(Node, "_set_status")
272-
273- with LoggerFixture("maas") as logger:
274- with ExpectedException(error_type):
275- with post_commit_hooks:
276- node.start_commissioning(admin)
277-
278- # The status is set to be reverted to its initial status.
279- self.assertThat(node._set_status, MockCalledOnceWith(
280- node.system_id, status=status))
281- # It's logged too.
282- self.assertThat(logger.output, Contains(
283- "%s: Could not start node for commissioning: %s\n"
284- % (node.hostname, error_message)))
285-
286 def test_start_commissioning_logs_and_raises_errors_in_starting(self):
287 admin = factory.make_admin()
288 node = factory.make_Node(status=NODE_STATUS.NEW)
289@@ -2368,7 +2352,8 @@
290 register_event = self.patch(node, '_register_request_event')
291 node_start = self.patch(node, '_start')
292 # Return a post-commit hook from Node.start().
293- node_start.side_effect = lambda user, user_data: post_commit()
294+ node_start.side_effect = (
295+ lambda user, user_data, old_status: post_commit())
296 admin = factory.make_admin()
297 node.start_commissioning(admin)
298 post_commit_hooks.reset() # Ignore these for now.
299@@ -2489,7 +2474,8 @@
300 enable_ssh=True)
301 node_start = self.patch(node, 'start')
302 # Return a post-commit hook from Node.start().
303- node_start.side_effect = lambda user, user_data: post_commit()
304+ node_start.side_effect = (
305+ lambda user, user_data, old_status: post_commit())
306 admin = factory.make_admin()
307 node.start_commissioning(admin)
308 post_commit_hooks.reset() # Ignore these for now.
309@@ -4632,6 +4618,7 @@
310 def test__adds_callbacks_and_errbacks_to_post_commit(self):
311 user = factory.make_User()
312 node = self.make_acquired_node_with_interface(user)
313+ old_status = node.status
314
315 post_commit_defer = self.patch(node_module, "post_commit")
316 mock_power_control = self.patch(Node, "_power_control_node")
317@@ -4645,10 +4632,12 @@
318 callOutToDatabase, Node._set_status_expires,
319 node.system_id, node.get_deployment_time()))
320
321- # Adds errback to release auto ips.
322+ # Adds errback to reset status and release auto ips.
323 self.assertThat(
324- post_commit_defer.addErrback, MockCalledOnceWith(
325- callOutToDatabase, node.release_auto_ips))
326+ post_commit_defer.addErrback, MockCallsMatch(
327+ call(callOutToDatabase, node._set_status,
328+ node.system_id, status=old_status),
329+ call(callOutToDatabase, node.release_auto_ips)))
330
331 def test_storage_layout_issues_returns_invalid_no_boot_arm64_non_efi(self):
332 node = factory.make_Node(
333
334=== modified file 'src/maasserver/tests/test_node_action.py'
335--- src/maasserver/tests/test_node_action.py 2016-07-15 00:42:25 +0000
336+++ src/maasserver/tests/test_node_action.py 2016-07-18 20:09:23 +0000
337@@ -292,14 +292,15 @@
338 interface=True, status=self.status,
339 power_type='manual', power_state=POWER_STATE.OFF)
340 node_start = self.patch(node, '_start')
341- node_start.side_effect = lambda user, user_data: post_commit()
342+ node_start.side_effect = (
343+ lambda user, user_data, old_status: post_commit())
344 admin = factory.make_admin()
345 action = Commission(node, admin)
346 with post_commit_hooks:
347 action.execute()
348 self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
349 self.assertThat(
350- node_start, MockCalledOnceWith(admin, ANY))
351+ node_start, MockCalledOnceWith(admin, ANY, ANY))
352
353
354 class TestAbortAction(MAASTransactionServerTestCase):
355@@ -734,6 +735,7 @@
356 interface=True, status=self.actionable_status,
357 power_type='ipmi', power_state=POWER_STATE.OFF,
358 owner=user, power_parameters=params)
359+ old_status = node.status
360 node_start = self.patch_autospec(node, '_start')
361 node_start.return_value = None
362
363@@ -742,7 +744,8 @@
364
365 self.expectThat(node.status, Equals(NODE_STATUS.DISK_ERASING))
366 self.assertThat(
367- node_start, MockCalledOnceWith(user, user_data=ANY))
368+ node_start, MockCalledOnceWith(
369+ user, user_data=ANY, old_status=old_status))
370
371 def test_Release_passes_secure_erase_and_quick_erase(self):
372 user = factory.make_User()