Merge lp:~ltrager/maas/fix_exception into lp:maas/trunk

Proposed by Lee Trager on 2016-05-11
Status: Merged
Approved by: Lee Trager on 2016-05-11
Approved revision: 5014
Merged at revision: 5015
Proposed branch: lp:~ltrager/maas/fix_exception
Merge into: lp:maas/trunk
Diff against target: 288 lines (+71/-73)
6 files modified
src/maasserver/api/devices.py (+1/-1)
src/maasserver/api/machines.py (+14/-1)
src/maasserver/api/tests/test_devices.py (+1/-1)
src/maasserver/api/tests/test_machine.py (+50/-13)
src/maasserver/models/node.py (+3/-20)
src/maasserver/models/tests/test_node.py (+2/-37)
To merge this branch: bzr merge lp:~ltrager/maas/fix_exception
Reviewer Review Type Date Requested Status
Andres Rodriguez Approve on 2016-05-11
Blake Rouse 2016-05-11 Approve on 2016-05-11
Review via email: mp+294421@code.launchpad.net

Commit message

In restore methods use assert on the model and raise NodeStateViolation on the API

Description of the change

As per the discussion in the stand up this turns the validation in set_initial_networking to be an assert which will be logged no matter where it is called from. The API now checks the node state and raises a NodeStateViolation if the state is not READY.

To post a comment you must log in.
Blake Rouse (blake-rouse) wrote :

Looks good. Need to fix one of the API validation messages and add a message to the assert as well.

review: Approve
Andres Rodriguez (andreserl) wrote :

Lee, before you alnd this, pelase attach this barnch to https://bugs.launchpad.net/maas/+bug/1580405 and fix the bug title and description to reflect this bugfix.

review: Approve
lp:~ltrager/maas/fix_exception updated on 2016-05-11
5014. By Lee Trager on 2016-05-11

Make blake_r corrections

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api/devices.py'
2--- src/maasserver/api/devices.py 2016-05-04 21:14:04 +0000
3+++ src/maasserver/api/devices.py 2016-05-11 21:24:10 +0000
4@@ -141,7 +141,7 @@
5 device = self.model.objects.get_node_or_404(
6 system_id=system_id, user=request.user,
7 perm=NODE_PERMISSION.ADMIN)
8- device.restore_default_configuration()
9+ device.set_initial_networking_configuration()
10 return reload_object(device)
11
12 @classmethod
13
14=== modified file 'src/maasserver/api/machines.py'
15--- src/maasserver/api/machines.py 2016-05-09 20:13:38 +0000
16+++ src/maasserver/api/machines.py 2016-05-11 21:24:10 +0000
17@@ -595,6 +595,10 @@
18 machine = self.model.objects.get_node_or_404(
19 system_id=system_id, user=request.user,
20 perm=NODE_PERMISSION.ADMIN)
21+ if machine.status != NODE_STATUS.READY:
22+ raise NodeStateViolation(
23+ "Machine must be in a ready state to restore networking "
24+ "configuration")
25 machine.set_initial_networking_configuration()
26 return reload_object(machine)
27
28@@ -608,6 +612,10 @@
29 machine = self.model.objects.get_node_or_404(
30 system_id=system_id, user=request.user,
31 perm=NODE_PERMISSION.ADMIN)
32+ if machine.status != NODE_STATUS.READY:
33+ raise NodeStateViolation(
34+ "Machine must be in a ready state to restore storage "
35+ "configuration.")
36 machine.set_default_storage_layout()
37 return reload_object(machine)
38
39@@ -621,7 +629,12 @@
40 machine = self.model.objects.get_node_or_404(
41 system_id=system_id, user=request.user,
42 perm=NODE_PERMISSION.ADMIN)
43- machine.restore_default_configuration()
44+ if machine.status != NODE_STATUS.READY:
45+ raise NodeStateViolation(
46+ "Machine must be in a ready state to restore default "
47+ "networking and storage configuration.")
48+ machine.set_default_storage_layout()
49+ machine.set_initial_networking_configuration()
50 return reload_object(machine)
51
52 @operation(idempotent=False)
53
54=== modified file 'src/maasserver/api/tests/test_devices.py'
55--- src/maasserver/api/tests/test_devices.py 2016-05-04 21:14:04 +0000
56+++ src/maasserver/api/tests/test_devices.py 2016-05-11 21:24:10 +0000
57@@ -376,7 +376,7 @@
58 self.become_admin()
59 device = factory.make_Device()
60 mock_set_initial_networking_config = self.patch(
61- node_module.Device, 'restore_default_configuration')
62+ node_module.Device, 'set_initial_networking_configuration')
63 response = self.client.post(
64 get_device_uri(device),
65 {'op': 'restore_default_configuration'})
66
67=== modified file 'src/maasserver/api/tests/test_machine.py'
68--- src/maasserver/api/tests/test_machine.py 2016-05-09 23:54:19 +0000
69+++ src/maasserver/api/tests/test_machine.py 2016-05-11 21:24:10 +0000
70@@ -60,6 +60,7 @@
71 HasLength,
72 MockCalledOnce,
73 MockCalledOnceWith,
74+ MockNotCalled,
75 )
76 from metadataserver.models import (
77 NodeKey,
78@@ -1894,7 +1895,7 @@
79 """Get the API URI for `machine`."""
80 return reverse('machine_handler', args=[machine.system_id])
81
82- def test_(self):
83+ def test_restore_networking_configuration(self):
84 self.become_admin()
85 machine = factory.make_Machine(status=NODE_STATUS.READY)
86 mock_set_initial_networking_config = self.patch(
87@@ -1915,6 +1916,17 @@
88 self.assertEqual(
89 http.client.FORBIDDEN, response.status_code, response.content)
90
91+ def test_restore_networking_configuration_checks_machine_status(self):
92+ self.become_admin()
93+ machine = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
94+ mock_set_initial_networking_config = self.patch(
95+ node_module.Machine, 'set_initial_networking_configuration')
96+ response = self.client.post(
97+ self.get_machine_uri(machine),
98+ {'op': 'restore_networking_configuration'})
99+ self.assertEqual(http.client.CONFLICT, response.status_code)
100+ self.assertThat(mock_set_initial_networking_config, MockNotCalled())
101+
102
103 class TestRestoreStorageConfiguration(APITestCase):
104 """Tests for
105@@ -1945,6 +1957,17 @@
106 self.assertEqual(
107 http.client.FORBIDDEN, response.status_code, response.content)
108
109+ def test_restore_storage_configuration_checks_machine_status(self):
110+ self.become_admin()
111+ machine = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
112+ mock_set_default_storage_layout = self.patch(
113+ node_module.Machine, 'set_default_storage_layout')
114+ response = self.client.post(
115+ self.get_machine_uri(machine),
116+ {'op': 'restore_storage_configuration'})
117+ self.assertEqual(http.client.CONFLICT, response.status_code)
118+ self.assertThat(mock_set_default_storage_layout, MockNotCalled())
119+
120
121 class TestRestoreDefaultConfiguration(APITestCase):
122 """Tests for
123@@ -1957,23 +1980,37 @@
124 def test_restore_default_configuration(self):
125 self.become_admin()
126 machine = factory.make_Machine(status=NODE_STATUS.READY)
127+ mock_set_default_storage_layout = self.patch(
128+ node_module.Machine, 'set_default_storage_layout')
129+ mock_set_initial_networking_config = self.patch(
130+ node_module.Machine, 'set_initial_networking_configuration')
131+ response = self.client.post(
132+ self.get_machine_uri(machine),
133+ {'op': 'restore_default_configuration'})
134+ self.assertEqual(http.client.OK, response.status_code)
135+ self.assertEqual(
136+ machine.system_id, json_load_bytes(response.content)['system_id'])
137+ self.assertThat(mock_set_default_storage_layout, MockCalledOnce())
138+ self.assertThat(mock_set_initial_networking_config, MockCalledOnce())
139+
140+ def test_restore_default_configuration_requires_admin(self):
141+ machine = factory.make_Machine()
142+ response = self.client.post(
143+ self.get_machine_uri(machine),
144+ {'op': 'restore_default_configuration'})
145+ self.assertEqual(
146+ http.client.FORBIDDEN, response.status_code, response.content)
147+
148+ def test_restore_default_configuration_checks_machine_status(self):
149+ self.become_admin()
150+ machine = factory.make_Machine(status=NODE_STATUS.DEPLOYED)
151 mock_restore_default_configuration = self.patch(
152 node_module.Machine, 'restore_default_configuration')
153 response = self.client.post(
154 self.get_machine_uri(machine),
155 {'op': 'restore_default_configuration'})
156- self.assertEqual(http.client.OK, response.status_code)
157- self.assertEqual(
158- machine.system_id, json_load_bytes(response.content)['system_id'])
159- self.assertThat(mock_restore_default_configuration, MockCalledOnce())
160-
161- def test_restore_default_configuration_requires_admin(self):
162- machine = factory.make_Machine()
163- response = self.client.post(
164- self.get_machine_uri(machine),
165- {'op': 'restore_default_configuration'})
166- self.assertEqual(
167- http.client.FORBIDDEN, response.status_code, response.content)
168+ self.assertEqual(http.client.CONFLICT, response.status_code)
169+ self.assertThat(mock_restore_default_configuration, MockNotCalled())
170
171
172 class TestMarkBroken(APITestCase):
173
174=== modified file 'src/maasserver/models/node.py'
175--- src/maasserver/models/node.py 2016-05-11 02:26:59 +0000
176+++ src/maasserver/models/node.py 2016-05-11 21:24:10 +0000
177@@ -2424,11 +2424,9 @@
178 # No interfaces on the node. Nothing to do.
179 return
180
181- if self.node_type == NODE_TYPE.MACHINE and self.status in [
182- NODE_STATUS.DEPLOYING, NODE_STATUS.DEPLOYED]:
183- raise ValidationError(
184- "Machine must be in a new, ready, allocated, or failed "
185- "deployment state to be reset.")
186+ assert \
187+ self.status not in [NODE_STATUS.DEPLOYING, NODE_STATUS.DEPLOYED], \
188+ 'Node cannot be in a deploying state when configuring network'
189
190 # Set AUTO mode on the boot interface.
191 auto_set = False
192@@ -3073,17 +3071,6 @@
193 super(Machine, self).__init__(
194 node_type=NODE_TYPE.MACHINE, *args, **kwargs)
195
196- def restore_default_configuration(self):
197- """Restores a machine's configuration to default settings."""
198- if self.status not in [
199- NODE_STATUS.NEW, NODE_STATUS.READY, NODE_STATUS.ALLOCATED,
200- NODE_STATUS.FAILED_DEPLOYMENT]:
201- raise ValidationError(
202- "Machine must be in a new, ready, allocated, or failed "
203- "deployment state to restore default configuration.")
204- self.set_default_storage_layout()
205- self.set_initial_networking_configuration()
206-
207
208 class Controller(Node):
209 """A node which is either a rack or region controller."""
210@@ -3825,10 +3812,6 @@
211 super(Device, self).__init__(
212 node_type=NODE_TYPE.DEVICE, *args, **kwargs)
213
214- def restore_default_configuration(self):
215- """Restores a device's configuration to default settings."""
216- self.set_initial_networking_configuration()
217-
218 def clean_architecture(self, prev):
219 # Devices aren't required to have a defined architecture
220 pass
221
222=== modified file 'src/maasserver/models/tests/test_node.py'
223--- src/maasserver/models/tests/test_node.py 2016-05-11 02:26:59 +0000
224+++ src/maasserver/models/tests/test_node.py 2016-05-11 21:24:10 +0000
225@@ -296,30 +296,6 @@
226 list(Machine.objects.get_available_machines_for_acquisition(user)))
227
228
229-class TestMachine(MAASServerTestCase):
230- def test_restore_default_configuration(self):
231- machine = factory.make_Machine(status=NODE_STATUS.NEW)
232- mock_set_default_storage_layout = self.patch(
233- machine, 'set_default_storage_layout')
234- mock_set_initial_networking_configuration = self.patch(
235- machine, 'set_initial_networking_configuration')
236- machine.restore_default_configuration()
237- self.assertThat(mock_set_default_storage_layout, MockCalledOnce())
238- self.assertThat(
239- mock_set_initial_networking_configuration, MockCalledOnce())
240-
241- def test_restore_default_configuration_error_wrong_status(self):
242- machine = factory.make_Machine_with_Interface_on_Subnet(
243- status=factory.pick_choice(
244- NODE_STATUS_CHOICES,
245- but_not=[
246- NODE_STATUS.NEW, NODE_STATUS.READY, NODE_STATUS.ALLOCATED,
247- NODE_STATUS.FAILED_DEPLOYMENT])
248- )
249- self.assertRaises(
250- ValidationError, machine.restore_default_configuration)
251-
252-
253 class TestControllerManager(MAASServerTestCase):
254 def test_controller_lists_node_type_rack_and_region(self):
255 racks_and_regions = set()
256@@ -508,17 +484,6 @@
257 self.assertEqual('', device.architecture)
258
259
260-class TestDevice(MAASServerTestCase):
261-
262- def test_restore_default_configuration(self):
263- device = factory.make_Device()
264- mock_set_initial_networking_configuration = self.patch(
265- device, 'set_initial_networking_configuration')
266- device.restore_default_configuration()
267- self.assertThat(
268- mock_set_initial_networking_configuration, MockCalledOnce())
269-
270-
271 class TestNode(MAASServerTestCase):
272
273 def setUp(self):
274@@ -3648,12 +3613,12 @@
275 alloc_type=IPADDRESS_TYPE.AUTO).first()
276 self.assertIsNone(auto_ip)
277
278- def test_set_initial_net_config_rasies_validation_error_wrong_status(self):
279+ def test_set_initial_net_config_asserts_proper_status(self):
280 machine = factory.make_Machine_with_Interface_on_Subnet(
281 status=random.choice([NODE_STATUS.DEPLOYING, NODE_STATUS.DEPLOYED])
282 )
283 self.assertRaises(
284- ValidationError, machine.set_initial_networking_configuration)
285+ AssertionError, machine.set_initial_networking_configuration)
286
287 def test_set_initial_networking_configuration_auto_on_boot_nic(self):
288 node = factory.make_Node_with_Interface_on_Subnet()