MAAS

Merge lp:~julian-edwards/maas/commission-monitor-bug-1303925 into lp:~maas-committers/maas/trunk

  1. commission-monitor-bug-1303925
  2. Merge into trunk
Proposed by Julian Edwards
Status: Merged
Approved by: Julian Edwards
Approved revision: no longer in the source branch.
Merged at revision: 3332
Proposed branch: lp:~julian-edwards/maas/commission-monitor-bug-1303925
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 317 lines (+74/-3)
7 files modified
src/maasserver/api/tests/test_enlistment.py (+4/-1)
src/maasserver/api/tests/test_nodes.py (+3/-0)
src/maasserver/models/node.py (+14/-2)
src/maasserver/models/tests/test_node.py (+29/-0)
src/maasserver/tests/test_node_action.py (+4/-0)
src/metadataserver/api.py (+1/-0)
src/metadataserver/tests/test_api.py (+19/-0)
To merge this branch: bzr merge lp:~julian-edwards/maas/commission-monitor-bug-1303925
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+240541@code.launchpad.net

Commit message

Add a transition monitor for commissioning, so that the node is marked as FAILED_COMMISSIONING when it times out.

Description of the change

Note that the monitor stuff already coped with this failure type; the code to enforce it was just never written. This branch fixes that.

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

Two inline notes.

review: Approve
Revision history for this message
Julian Edwards (julian-edwards) wrote :

On Tuesday 04 November 2014 08:23:10 you wrote:
> Review: Approve

Thanks for reviewing.

> > + def get_commissioning_time(self):
> > + """Return the commissioning time of this node (in seconds)."""
>
> What is a node's "commissioning time"? Is it the time the node has been
> commissioning? Is it the time that commissioning is allowed to take? Is
> it the time the node started commissioning? A more specific name would
> help.

As I said yesterday I was being consistent with "get_deployment_time". I've
made it clear in both docstrings what the func is doing.

> > class TestCommissioningAPI(MAASServerTestCase):
> > + def setUp(self):
> > + super(TestCommissioningAPI, self).setUp()
> > + self.patch(Node, 'stop_transition_monitor')
>
> I'm not familiar with transition monitors, so this makes me wonder: does the
> entire test case need this patch? If it's only for those two tests that
> query the mock, an explicit patch in the tests would be better than an
> implicit one on every test.

It make an RPC to the cluster, so it needs patching out globally, and makes
sense to do it in one place rather than everywhere.

Cheers!

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

The attempt to merge lp:~julian-edwards/maas/commission-monitor-bug-1303925 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Get:2 http://security.ubuntu.com trusty-security Release [62.0 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [62.0 kB]
Get:5 http://security.ubuntu.com trusty-security/main Sources [49.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [11.2 kB]
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [153 kB]
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [51.2 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Get:9 http://security.ubuntu.com trusty-security/main Translation-en [75.0 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [136 kB]
Get:11 http://security.ubuntu.com trusty-security/universe Translation-en [30.5 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [89.3 kB]
Get:13 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [356 kB]
Get:14 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [217 kB]
Get:15 http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en [161 kB]
Get:16 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en [109 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,564 kB in 2s (522 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mimeparse python-mock python-netaddr python-netifaces python-nose python-oauth python-oops python-oops-amqp pyth...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api/tests/test_enlistment.py'
2--- src/maasserver/api/tests/test_enlistment.py 2014-10-30 13:07:43 +0000
3+++ src/maasserver/api/tests/test_enlistment.py 2014-11-05 04:40:50 +0000
4@@ -52,6 +52,7 @@
5 def setUp(self):
6 super(EnlistmentAPITest, self).setUp()
7 self.patch_autospec(node_module, 'wait_for_power_commands')
8+ self.patch_autospec(Node, 'start_transition_monitor')
9
10 def test_POST_new_creates_node(self):
11 architecture = make_usable_architecture(self)
12@@ -354,6 +355,7 @@
13 def setUp(self):
14 super(NodeHostnameEnlistmentTest, self).setUp()
15 self.patch_autospec(node_module, 'wait_for_power_commands')
16+ self.patch_autospec(Node, 'start_transition_monitor')
17
18 scenarios = [
19 ('anon', dict(userfactory=lambda: AnonymousUser())),
20@@ -639,6 +641,7 @@
21 def setUp(self):
22 super(AdminLoggedInEnlistmentAPITest, self).setUp()
23 self.patch_autospec(node_module, 'wait_for_power_commands')
24+ self.patch_autospec(Node, 'start_transition_monitor')
25
26 def test_POST_new_sets_power_type_if_admin(self):
27 self.client_log_in(as_admin=True)
28@@ -715,9 +718,9 @@
29 'mac_addresses': ['AA:BB:CC:DD:EE:FF'],
30 })
31
32+ self.assertEqual(httplib.OK, response.status_code, response.content)
33 node = Node.objects.get(
34 system_id=json.loads(response.content)['system_id'])
35- self.assertEqual(httplib.OK, response.status_code)
36 self.assertEqual(
37 {'param': param},
38 reload_object(node).power_parameters)
39
40=== modified file 'src/maasserver/api/tests/test_nodes.py'
41--- src/maasserver/api/tests/test_nodes.py 2014-10-28 15:49:21 +0000
42+++ src/maasserver/api/tests/test_nodes.py 2014-11-05 04:40:50 +0000
43@@ -997,6 +997,7 @@
44 def test_POST_accept_gets_node_out_of_declared_state(self):
45 # This will change when we add provisioning. Until then,
46 # acceptance gets a node straight to Ready state.
47+ self.patch_autospec(Node, 'start_transition_monitor')
48 self.become_admin()
49 target_state = NODE_STATUS.COMMISSIONING
50
51@@ -1066,6 +1067,7 @@
52 def test_POST_accept_accepts_multiple_nodes(self):
53 # This will change when we add provisioning. Until then,
54 # acceptance gets a node straight to Ready state.
55+ self.patch_autospec(Node, 'start_transition_monitor')
56 self.become_admin()
57 target_state = NODE_STATUS.COMMISSIONING
58
59@@ -1083,6 +1085,7 @@
60 [reload_object(node).status for node in nodes])
61
62 def test_POST_accept_returns_actually_accepted_nodes(self):
63+ self.patch_autospec(Node, 'start_transition_monitor')
64 self.become_admin()
65 acceptable_nodes = [
66 factory.make_Node(status=NODE_STATUS.NEW)
67
68=== modified file 'src/maasserver/models/node.py'
69--- src/maasserver/models/node.py 2014-10-30 00:39:21 +0000
70+++ src/maasserver/models/node.py 2014-11-05 04:40:50 +0000
71@@ -543,11 +543,22 @@
72 return self.hostname
73
74 def get_deployment_time(self):
75- """Return the deployment time of this node (in seconds)."""
76+ """Return the deployment time of this node (in seconds).
77+
78+ This is the maximum time the deployment is allowed to take.
79+ """
80 # Return a *very* conservative estimate for now.
81 # Something that shouldn't conflict with any deployment.
82 return timedelta(minutes=40).total_seconds()
83
84+ def get_commissioning_time(self):
85+ """Return the commissioning time of this node (in seconds).
86+
87+ This is the maximum time the commissioning is allowed to take.
88+ """
89+ # Return a *very* conservative estimate for now.
90+ return timedelta(minutes=20).total_seconds()
91+
92 def start_deployment(self):
93 """Mark a node as being deployed."""
94 self.status = NODE_STATUS.DEPLOYING
95@@ -824,7 +835,6 @@
96 """Install OS and self-test a new node."""
97 # Avoid circular imports.
98 from metadataserver.user_data.commissioning import generate_user_data
99- from metadataserver.models import NodeResult
100
101 commissioning_user_data = generate_user_data(node=self)
102 NodeResult.objects.clear_results(self)
103@@ -838,6 +848,7 @@
104 self.status = NODE_STATUS.COMMISSIONING
105 self.save()
106 transaction.commit()
107+ self.start_transition_monitor(self.get_commissioning_time())
108 try:
109 self.start(user, user_data=commissioning_user_data)
110 except Exception as ex:
111@@ -862,6 +873,7 @@
112 % (self.system_id, NODE_STATUS_CHOICES_DICT[self.status]))
113 maaslog.info(
114 "%s: Aborting commissioning", self.hostname)
115+ self.stop_transition_monitor()
116 try:
117 self.stop(user)
118 except Exception as ex:
119
120=== modified file 'src/maasserver/models/tests/test_node.py'
121--- src/maasserver/models/tests/test_node.py 2014-10-30 00:39:21 +0000
122+++ src/maasserver/models/tests/test_node.py 2014-11-05 04:40:50 +0000
123@@ -1233,6 +1233,7 @@
124 target_state = NODE_STATUS.COMMISSIONING
125
126 node = factory.make_Node(status=NODE_STATUS.NEW)
127+ self.patch(node, 'start_transition_monitor')
128 return_value = node.accept_enlistment(factory.make_User())
129 self.assertEqual((node, target_state), (return_value, node.status))
130
131@@ -1293,6 +1294,7 @@
132 node_start = self.patch(node, 'start')
133 factory.make_MACAddress(node=node)
134 admin = factory.make_admin()
135+ self.patch(node, 'start_transition_monitor')
136 node.start_commissioning(admin)
137
138 expected_attrs = {
139@@ -1305,6 +1307,7 @@
140 def test_start_commissioning_sets_user_data(self):
141 node = factory.make_Node(status=NODE_STATUS.NEW)
142 node_start = self.patch(node, 'start')
143+ self.patch(node, 'start_transition_monitor')
144 user_data = factory.make_string().encode('ascii')
145 generate_user_data = self.patch(
146 commissioning, 'generate_user_data')
147@@ -1316,6 +1319,7 @@
148
149 def test_start_commissioning_clears_node_commissioning_results(self):
150 node = factory.make_Node(status=NODE_STATUS.NEW)
151+ self.patch(node, 'start_transition_monitor')
152 NodeResult.objects.store_data(
153 node, factory.make_string(),
154 random.randint(0, 10),
155@@ -1333,6 +1337,7 @@
156 node, filename, script_result, RESULT_TYPE.COMMISSIONING,
157 Bin(data))
158 other_node = factory.make_Node(status=NODE_STATUS.NEW)
159+ self.patch(other_node, 'start_transition_monitor')
160 other_node.start_commissioning(factory.make_admin())
161 self.assertEqual(
162 data, NodeResult.objects.get_data(node, filename))
163@@ -1343,6 +1348,7 @@
164 # status.
165 admin = factory.make_admin()
166 node = factory.make_Node(status=NODE_STATUS.NEW)
167+ self.patch(node, 'start_transition_monitor')
168 generate_user_data = self.patch(commissioning, 'generate_user_data')
169 node_start = self.patch(node, 'start')
170 node_start.side_effect = MultipleFailures(
171@@ -1365,6 +1371,7 @@
172 def test_start_commissioning_logs_and_raises_errors_in_starting(self):
173 admin = factory.make_admin()
174 node = factory.make_Node(status=NODE_STATUS.NEW)
175+ self.patch(node, 'start_transition_monitor')
176 maaslog = self.patch(node_module, 'maaslog')
177 exception = NoConnectionsAvailable(factory.make_name())
178 self.patch(node, 'start').side_effect = exception
179@@ -1383,6 +1390,7 @@
180 admin = factory.make_admin()
181 node = factory.make_Node(
182 status=NODE_STATUS.COMMISSIONING, power_type="virsh")
183+ self.patch(node, 'stop_transition_monitor')
184 node_stop = self.patch(node, 'stop')
185 node_stop.side_effect = MultipleFailures(
186 Failure(NoConnectionsAvailable()))
187@@ -1398,10 +1406,30 @@
188 self.assertThat(node_stop, MockCalledOnceWith(admin))
189 self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
190
191+ def test_start_commissioning_starts_monitor(self):
192+ node = factory.make_Node(status=NODE_STATUS.NEW)
193+ monitor_timeout = random.randint(1, 100)
194+ self.patch(
195+ node, 'get_commissioning_time').return_value = monitor_timeout
196+ start_transition_monitor = self.patch(
197+ node, 'start_transition_monitor')
198+ admin = factory.make_admin()
199+ node.start_commissioning(admin)
200+ self.assertThat(
201+ start_transition_monitor, MockCalledOnceWith(monitor_timeout))
202+
203+ def test_abort_commissioning_stops_monitor(self):
204+ node = factory.make_Node(status=NODE_STATUS.COMMISSIONING)
205+ stop_transition_monitor = self.patch(node, 'stop_transition_monitor')
206+ admin = factory.make_admin()
207+ node.abort_commissioning(admin)
208+ self.assertThat(stop_transition_monitor, MockCalledOnceWith())
209+
210 def test_abort_commissioning_logs_and_raises_errors_in_stopping(self):
211 admin = factory.make_admin()
212 node = factory.make_Node(status=NODE_STATUS.COMMISSIONING)
213 maaslog = self.patch(node_module, 'maaslog')
214+ self.patch(node, 'stop_transition_monitor')
215 exception_class = factory.make_exception_type()
216 exception = exception_class(factory.make_name())
217 self.patch(node, 'stop').side_effect = exception
218@@ -1419,6 +1447,7 @@
219 admin = factory.make_admin()
220
221 node_stop = self.patch(node, 'stop')
222+ self.patch(node, 'stop_transition_monitor')
223
224 node.abort_commissioning(admin)
225 expected_attrs = {
226
227=== modified file 'src/maasserver/tests/test_node_action.py'
228--- src/maasserver/tests/test_node_action.py 2014-10-30 02:50:27 +0000
229+++ src/maasserver/tests/test_node_action.py 2014-11-05 04:40:50 +0000
230@@ -233,6 +233,7 @@
231 node = factory.make_Node(
232 mac=True, status=self.status,
233 power_type='ether_wake')
234+ self.patch_autospec(node, 'start_transition_monitor')
235 node_start = self.patch(node, 'start')
236 admin = factory.make_admin()
237 action = Commission(node, admin)
238@@ -248,6 +249,7 @@
239 node = factory.make_Node(
240 mac=True, status=NODE_STATUS.COMMISSIONING,
241 power_type='virsh')
242+ self.patch_autospec(node, 'stop_transition_monitor')
243 node_stop = self.patch_autospec(node, 'stop')
244 admin = factory.make_admin()
245
246@@ -588,6 +590,8 @@
247 exception = self.make_exception()
248 self.patch(node, 'start').side_effect = exception
249 self.patch(node, 'stop').side_effect = exception
250+ self.patch_autospec(node, 'start_transition_monitor')
251+ self.patch_autospec(node, 'stop_transition_monitor')
252
253 def make_action(self, action_class, node_status):
254 node = factory.make_Node(
255
256=== modified file 'src/metadataserver/api.py'
257--- src/metadataserver/api.py 2014-10-24 23:45:33 +0000
258+++ src/metadataserver/api.py 2014-11-05 04:40:50 +0000
259@@ -281,6 +281,7 @@
260 if node.status == NODE_STATUS.COMMISSIONING:
261 self._store_commissioning_results(node, request)
262 store_node_power_parameters(node, request)
263+ node.stop_transition_monitor()
264 target_status = self.signaling_statuses.get(status)
265 elif node.status == NODE_STATUS.DEPLOYING:
266 self._store_installation_results(node, request)
267
268=== modified file 'src/metadataserver/tests/test_api.py'
269--- src/metadataserver/tests/test_api.py 2014-10-24 23:45:33 +0000
270+++ src/metadataserver/tests/test_api.py 2014-11-05 04:40:50 +0000
271@@ -41,6 +41,7 @@
272 SSHKey,
273 Tag,
274 )
275+from maasserver.models.node import Node
276 from maasserver.rpc.testing.mixins import PreseedRPCMixin
277 from maasserver.testing.factory import factory
278 from maasserver.testing.oauthclient import OAuthAuthenticatedClient
279@@ -501,6 +502,10 @@
280
281 class TestCommissioningAPI(MAASServerTestCase):
282
283+ def setUp(self):
284+ super(TestCommissioningAPI, self).setUp()
285+ self.patch(Node, 'stop_transition_monitor')
286+
287 def test_commissioning_scripts(self):
288 script = factory.make_CommissioningScript()
289 response = make_node_client().get(
290@@ -620,6 +625,13 @@
291 self.assertEqual(httplib.OK, response.status_code)
292 self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
293
294+ def test_signalling_commissioning_success_cancels_monitor(self):
295+ node = factory.make_Node(status=NODE_STATUS.COMMISSIONING)
296+ client = make_node_client(node=node)
297+ response = call_signal(client, status='OK')
298+ self.assertEqual(httplib.OK, response.status_code, response.content)
299+ self.assertThat(node.stop_transition_monitor, MockCalledOnceWith())
300+
301 def test_signaling_commissioning_success_is_idempotent(self):
302 node = factory.make_Node(status=NODE_STATUS.COMMISSIONING)
303 client = make_node_client(node=node)
304@@ -645,6 +657,13 @@
305 self.assertEqual(
306 NODE_STATUS.FAILED_COMMISSIONING, reload_object(node).status)
307
308+ def test_signalling_commissioning_failure_cancels_monitor(self):
309+ node = factory.make_Node(status=NODE_STATUS.COMMISSIONING)
310+ client = make_node_client(node=node)
311+ response = call_signal(client, status='FAILED')
312+ self.assertEqual(httplib.OK, response.status_code, response.content)
313+ self.assertThat(node.stop_transition_monitor, MockCalledOnceWith())
314+
315 def test_signaling_commissioning_failure_is_idempotent(self):
316 node = factory.make_Node(status=NODE_STATUS.COMMISSIONING)
317 client = make_node_client(node=node)