Merge ~mpontillo/maas:disallow-pod-release--bug-1782060 into maas:master
- Git
- lp:~mpontillo/maas
- disallow-pod-release--bug-1782060
- Merge into master
Proposed by
Mike Pontillo
Status: | Merged |
---|---|
Approved by: | Mike Pontillo |
Approved revision: | ae91151f656d8b9804d2018e859f81b9ea2b00b0 |
Merge reported by: | MAAS Lander |
Merged at revision: | not available |
Proposed branch: | ~mpontillo/maas:disallow-pod-release--bug-1782060 |
Merge into: | maas:master |
Diff against target: |
614 lines (+284/-20) 10 files modified
src/maasserver/api/machines.py (+19/-0) src/maasserver/api/pods.py (+1/-9) src/maasserver/api/rackcontrollers.py (+8/-3) src/maasserver/api/regioncontrollers.py (+21/-0) src/maasserver/api/tests/test_machine.py (+41/-1) src/maasserver/api/tests/test_rackcontroller.py (+45/-1) src/maasserver/api/tests/test_regioncontroller.py (+65/-1) src/maasserver/models/bmc.py (+15/-0) src/maasserver/models/node.py (+53/-5) src/maasserver/models/tests/test_node.py (+16/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Newell Jensen (community) | Approve | ||
Review via email: mp+355397@code.launchpad.net |
Commit message
LP: #1782060 - Don't allow nodes that are hosting pods to be deleted.
Description of the change
To post a comment you must log in.
Revision history for this message
Blake Rouse (blake-rouse) : | # |
Revision history for this message
Mike Pontillo (mpontillo) wrote : | # |
Revision history for this message
Newell Jensen (newell-jensen) wrote : | # |
Looks good. Nicely done.
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py | |||
2 | index 12cbc69..5bbe55d 100644 | |||
3 | --- a/src/maasserver/api/machines.py | |||
4 | +++ b/src/maasserver/api/machines.py | |||
5 | @@ -107,6 +107,7 @@ from maasserver.utils.orm import ( | |||
6 | 107 | get_first, | 107 | get_first, |
7 | 108 | reload_object, | 108 | reload_object, |
8 | 109 | ) | 109 | ) |
9 | 110 | from piston3.utils import rc | ||
10 | 110 | import yaml | 111 | import yaml |
11 | 111 | 112 | ||
12 | 112 | # Machine's fields exposed on the API. | 113 | # Machine's fields exposed on the API. |
13 | @@ -320,6 +321,24 @@ class MachineHandler(NodeHandler, OwnerDataMixin, PowerMixin): | |||
14 | 320 | model = Machine | 321 | model = Machine |
15 | 321 | fields = DISPLAYED_MACHINE_FIELDS | 322 | fields = DISPLAYED_MACHINE_FIELDS |
16 | 322 | 323 | ||
17 | 324 | def delete(self, request, system_id): | ||
18 | 325 | """Delete a specific machine. | ||
19 | 326 | |||
20 | 327 | A machine cannot be deleted if it hosts pod virtual machines. | ||
21 | 328 | Use `force` to override this behavior. Forcing deletion will also | ||
22 | 329 | remove hosted pods. | ||
23 | 330 | |||
24 | 331 | Returns 404 if the node is not found. | ||
25 | 332 | Returns 403 if the user does not have permission to delete the node. | ||
26 | 333 | Returns 400 if the machine cannot be deleted. | ||
27 | 334 | Returns 204 if the node is successfully deleted. | ||
28 | 335 | """ | ||
29 | 336 | node = self.model.objects.get_node_or_404( | ||
30 | 337 | system_id=system_id, user=request.user, perm=NODE_PERMISSION.ADMIN) | ||
31 | 338 | node.as_self().delete( | ||
32 | 339 | force=get_optional_param(request.GET, 'force', False, StringBool)) | ||
33 | 340 | return rc.DELETED | ||
34 | 341 | |||
35 | 323 | @classmethod | 342 | @classmethod |
36 | 324 | def boot_disk(handler, machine): | 343 | def boot_disk(handler, machine): |
37 | 325 | """Return the boot_disk for the machine.""" | 344 | """Return the boot_disk for the machine.""" |
38 | diff --git a/src/maasserver/api/pods.py b/src/maasserver/api/pods.py | |||
39 | index 141c9d9..0c6b28c 100644 | |||
40 | --- a/src/maasserver/api/pods.py | |||
41 | +++ b/src/maasserver/api/pods.py | |||
42 | @@ -16,14 +16,12 @@ from maasserver.api.support import ( | |||
43 | 16 | OperationsHandler, | 16 | OperationsHandler, |
44 | 17 | ) | 17 | ) |
45 | 18 | from maasserver.api.utils import get_mandatory_param | 18 | from maasserver.api.utils import get_mandatory_param |
46 | 19 | from maasserver.enum import NODE_CREATION_TYPE | ||
47 | 20 | from maasserver.exceptions import MAASAPIValidationError | 19 | from maasserver.exceptions import MAASAPIValidationError |
48 | 21 | from maasserver.forms.pods import ( | 20 | from maasserver.forms.pods import ( |
49 | 22 | ComposeMachineForm, | 21 | ComposeMachineForm, |
50 | 23 | PodForm, | 22 | PodForm, |
51 | 24 | ) | 23 | ) |
52 | 25 | from maasserver.models.bmc import Pod | 24 | from maasserver.models.bmc import Pod |
53 | 26 | from maasserver.models.node import Machine | ||
54 | 27 | from maasserver.utils.django_urls import reverse | 25 | from maasserver.utils.django_urls import reverse |
55 | 28 | from piston3.utils import rc | 26 | from piston3.utils import rc |
56 | 29 | from provisioningserver.drivers.pod import Capabilities | 27 | from provisioningserver.drivers.pod import Capabilities |
57 | @@ -178,13 +176,7 @@ class PodHandler(OperationsHandler): | |||
58 | 178 | Returns 204 if the pod is successfully deleted. | 176 | Returns 204 if the pod is successfully deleted. |
59 | 179 | """ | 177 | """ |
60 | 180 | pod = get_object_or_404(Pod, id=id) | 178 | pod = get_object_or_404(Pod, id=id) |
68 | 181 | # Calculate the wait time based on the number of none pre-existing | 179 | pod.delete_and_wait() |
62 | 182 | # machines. We allow maximum of 60 seconds per machine plus 60 seconds | ||
63 | 183 | # for the pod. | ||
64 | 184 | num_machines = Machine.objects.filter(bmc=pod) | ||
65 | 185 | num_machines = num_machines.exclude( | ||
66 | 186 | creation_type=NODE_CREATION_TYPE.PRE_EXISTING) | ||
67 | 187 | pod.async_delete().wait((num_machines.count() * 60) + 60) | ||
69 | 188 | return rc.DELETED | 180 | return rc.DELETED |
70 | 189 | 181 | ||
71 | 190 | @admin_method | 182 | @admin_method |
72 | diff --git a/src/maasserver/api/rackcontrollers.py b/src/maasserver/api/rackcontrollers.py | |||
73 | index d09e8ed..ba59020 100644 | |||
74 | --- a/src/maasserver/api/rackcontrollers.py | |||
75 | +++ b/src/maasserver/api/rackcontrollers.py | |||
76 | @@ -6,7 +6,6 @@ __all__ = [ | |||
77 | 6 | 'RackControllersHandler', | 6 | 'RackControllersHandler', |
78 | 7 | ] | 7 | ] |
79 | 8 | 8 | ||
80 | 9 | |||
81 | 10 | from django.conf import settings | 9 | from django.conf import settings |
82 | 11 | from django.http import HttpResponse | 10 | from django.http import HttpResponse |
83 | 12 | from formencode.validators import StringBool | 11 | from formencode.validators import StringBool |
84 | @@ -93,6 +92,12 @@ class RackControllerHandler(NodeHandler, PowerMixin): | |||
85 | 93 | a `VLAN` and another rack controller cannot be used to provide DHCP for | 92 | a `VLAN` and another rack controller cannot be used to provide DHCP for |
86 | 94 | said VLAN. Use `force` to override this behavior. | 93 | said VLAN. Use `force` to override this behavior. |
87 | 95 | 94 | ||
88 | 95 | Using `force` will also allow deleting a rack controller that is | ||
89 | 96 | hosting pod virtual machines. (The pod will also be deleted.) | ||
90 | 97 | |||
91 | 98 | Rack controllers that are also region controllers will be converted | ||
92 | 99 | to a region controller (and hosted pods will not be affected). | ||
93 | 100 | |||
94 | 96 | :param force: Always delete the rack controller even if its the | 101 | :param force: Always delete the rack controller even if its the |
95 | 97 | `primary_rack` on a `VLAN` and another rack controller cannot | 102 | `primary_rack` on a `VLAN` and another rack controller cannot |
96 | 98 | provide DHCP on said VLAN. This will disable DHCP on those VLANs. | 103 | provide DHCP on said VLAN. This will disable DHCP on those VLANs. |
97 | @@ -100,11 +105,11 @@ class RackControllerHandler(NodeHandler, PowerMixin): | |||
98 | 100 | 105 | ||
99 | 101 | Returns 404 if the node is not found. | 106 | Returns 404 if the node is not found. |
100 | 102 | Returns 403 if the user does not have permission to delete the node. | 107 | Returns 403 if the user does not have permission to delete the node. |
101 | 108 | Returns 400 if the node cannot be deleted. | ||
102 | 103 | Returns 204 if the node is successfully deleted. | 109 | Returns 204 if the node is successfully deleted. |
103 | 104 | """ | 110 | """ |
104 | 105 | node = self.model.objects.get_node_or_404( | 111 | node = self.model.objects.get_node_or_404( |
107 | 106 | system_id=system_id, user=request.user, | 112 | system_id=system_id, user=request.user, perm=NODE_PERMISSION.ADMIN) |
106 | 107 | perm=NODE_PERMISSION.ADMIN) | ||
108 | 108 | node.as_self().delete( | 113 | node.as_self().delete( |
109 | 109 | force=get_optional_param(request.GET, 'force', False, StringBool)) | 114 | force=get_optional_param(request.GET, 'force', False, StringBool)) |
110 | 110 | return rc.DELETED | 115 | return rc.DELETED |
111 | diff --git a/src/maasserver/api/regioncontrollers.py b/src/maasserver/api/regioncontrollers.py | |||
112 | index d366437..7eb0099 100644 | |||
113 | --- a/src/maasserver/api/regioncontrollers.py | |||
114 | +++ b/src/maasserver/api/regioncontrollers.py | |||
115 | @@ -6,16 +6,19 @@ __all__ = [ | |||
116 | 6 | 'RegionControllersHandler', | 6 | 'RegionControllersHandler', |
117 | 7 | ] | 7 | ] |
118 | 8 | 8 | ||
119 | 9 | from formencode.validators import StringBool | ||
120 | 9 | from maasserver.api.interfaces import DISPLAYED_INTERFACE_FIELDS | 10 | from maasserver.api.interfaces import DISPLAYED_INTERFACE_FIELDS |
121 | 10 | from maasserver.api.nodes import ( | 11 | from maasserver.api.nodes import ( |
122 | 11 | NodeHandler, | 12 | NodeHandler, |
123 | 12 | NodesHandler, | 13 | NodesHandler, |
124 | 13 | ) | 14 | ) |
125 | 14 | from maasserver.api.support import admin_method | 15 | from maasserver.api.support import admin_method |
126 | 16 | from maasserver.api.utils import get_optional_param | ||
127 | 15 | from maasserver.enum import NODE_PERMISSION | 17 | from maasserver.enum import NODE_PERMISSION |
128 | 16 | from maasserver.exceptions import MAASAPIValidationError | 18 | from maasserver.exceptions import MAASAPIValidationError |
129 | 17 | from maasserver.forms import ControllerForm | 19 | from maasserver.forms import ControllerForm |
130 | 18 | from maasserver.models import RegionController | 20 | from maasserver.models import RegionController |
131 | 21 | from piston3.utils import rc | ||
132 | 19 | 22 | ||
133 | 20 | # Region controller's fields exposed on the API. | 23 | # Region controller's fields exposed on the API. |
134 | 21 | DISPLAYED_REGION_CONTROLLER_FIELDS = ( | 24 | DISPLAYED_REGION_CONTROLLER_FIELDS = ( |
135 | @@ -68,6 +71,24 @@ class RegionControllerHandler(NodeHandler): | |||
136 | 68 | model = RegionController | 71 | model = RegionController |
137 | 69 | fields = DISPLAYED_REGION_CONTROLLER_FIELDS | 72 | fields = DISPLAYED_REGION_CONTROLLER_FIELDS |
138 | 70 | 73 | ||
139 | 74 | def delete(self, request, system_id): | ||
140 | 75 | """Delete a specific region controller. | ||
141 | 76 | |||
142 | 77 | A region controller cannot be deleted if it hosts pod virtual machines. | ||
143 | 78 | Use `force` to override this behavior. Forcing deletion will also | ||
144 | 79 | remove hosted pods. | ||
145 | 80 | |||
146 | 81 | Returns 404 if the node is not found. | ||
147 | 82 | Returns 403 if the user does not have permission to delete the node. | ||
148 | 83 | Returns 400 if the node cannot be deleted. | ||
149 | 84 | Returns 204 if the node is successfully deleted. | ||
150 | 85 | """ | ||
151 | 86 | node = self.model.objects.get_node_or_404( | ||
152 | 87 | system_id=system_id, user=request.user, perm=NODE_PERMISSION.ADMIN) | ||
153 | 88 | node.as_self().delete( | ||
154 | 89 | force=get_optional_param(request.GET, 'force', False, StringBool)) | ||
155 | 90 | return rc.DELETED | ||
156 | 91 | |||
157 | 71 | @admin_method | 92 | @admin_method |
158 | 72 | def update(self, request, system_id): | 93 | def update(self, request, system_id): |
159 | 73 | """Update a specific Region controller. | 94 | """Update a specific Region controller. |
160 | diff --git a/src/maasserver/api/tests/test_machine.py b/src/maasserver/api/tests/test_machine.py | |||
161 | index 40f2834..d1979d6 100644 | |||
162 | --- a/src/maasserver/api/tests/test_machine.py | |||
163 | +++ b/src/maasserver/api/tests/test_machine.py | |||
164 | @@ -8,10 +8,14 @@ __all__ = [] | |||
165 | 8 | from base64 import b64encode | 8 | from base64 import b64encode |
166 | 9 | import http.client | 9 | import http.client |
167 | 10 | from random import choice | 10 | from random import choice |
169 | 11 | from unittest.mock import ANY | 11 | from unittest.mock import ( |
170 | 12 | ANY, | ||
171 | 13 | call, | ||
172 | 14 | ) | ||
173 | 12 | 15 | ||
174 | 13 | from django.conf import settings | 16 | from django.conf import settings |
175 | 14 | from django.db import transaction | 17 | from django.db import transaction |
176 | 18 | from django.utils.http import urlencode | ||
177 | 15 | from maasserver import forms | 19 | from maasserver import forms |
178 | 16 | from maasserver.api import machines as machines_module | 20 | from maasserver.api import machines as machines_module |
179 | 17 | from maasserver.enum import ( | 21 | from maasserver.enum import ( |
180 | @@ -35,6 +39,7 @@ from maasserver.models import ( | |||
181 | 35 | node as node_module, | 39 | node as node_module, |
182 | 36 | StaticIPAddress, | 40 | StaticIPAddress, |
183 | 37 | ) | 41 | ) |
184 | 42 | from maasserver.models.bmc import Pod | ||
185 | 38 | from maasserver.models.node import RELEASABLE_STATUSES | 43 | from maasserver.models.node import RELEASABLE_STATUSES |
186 | 39 | from maasserver.models.signals.testing import SignalsDisabled | 44 | from maasserver.models.signals.testing import SignalsDisabled |
187 | 40 | from maasserver.storage_layouts import ( | 45 | from maasserver.storage_layouts import ( |
188 | @@ -44,6 +49,7 @@ from maasserver.storage_layouts import ( | |||
189 | 44 | from maasserver.testing.api import ( | 49 | from maasserver.testing.api import ( |
190 | 45 | APITestCase, | 50 | APITestCase, |
191 | 46 | APITransactionTestCase, | 51 | APITransactionTestCase, |
192 | 52 | explain_unexpected_response, | ||
193 | 47 | ) | 53 | ) |
194 | 48 | from maasserver.testing.architecture import make_usable_architecture | 54 | from maasserver.testing.architecture import make_usable_architecture |
195 | 49 | from maasserver.testing.factory import factory | 55 | from maasserver.testing.factory import factory |
196 | @@ -63,6 +69,7 @@ from maastesting.matchers import ( | |||
197 | 63 | HasLength, | 69 | HasLength, |
198 | 64 | MockCalledOnce, | 70 | MockCalledOnce, |
199 | 65 | MockCalledOnceWith, | 71 | MockCalledOnceWith, |
200 | 72 | MockCallsMatch, | ||
201 | 66 | MockNotCalled, | 73 | MockNotCalled, |
202 | 67 | ) | 74 | ) |
203 | 68 | from metadataserver.enum import SCRIPT_TYPE | 75 | from metadataserver.enum import SCRIPT_TYPE |
204 | @@ -1641,6 +1648,39 @@ class TestMachineAPI(APITestCase.ForUser): | |||
205 | 1641 | 1648 | ||
206 | 1642 | self.assertEqual(http.client.NOT_FOUND, response.status_code) | 1649 | self.assertEqual(http.client.NOT_FOUND, response.status_code) |
207 | 1643 | 1650 | ||
208 | 1651 | def test_DELETE_delete_with_force(self): | ||
209 | 1652 | self.become_admin() | ||
210 | 1653 | vlan = factory.make_VLAN() | ||
211 | 1654 | subnet = factory.make_Subnet(vlan=vlan) | ||
212 | 1655 | machine = factory.make_Machine_with_Interface_on_Subnet( | ||
213 | 1656 | vlan=vlan, subnet=subnet) | ||
214 | 1657 | ip = factory.make_StaticIPAddress(interface=machine.boot_interface) | ||
215 | 1658 | factory.make_Pod(ip_address=ip) | ||
216 | 1659 | mock_async_delete = self.patch(Pod, "async_delete") | ||
217 | 1660 | response = self.client.delete( | ||
218 | 1661 | self.get_machine_uri(machine), QUERY_STRING=urlencode({ | ||
219 | 1662 | 'force': 'true' | ||
220 | 1663 | }, doseq=True)) | ||
221 | 1664 | self.assertEqual( | ||
222 | 1665 | http.client.NO_CONTENT, response.status_code, | ||
223 | 1666 | explain_unexpected_response(http.client.NO_CONTENT, response)) | ||
224 | 1667 | self.assertThat(mock_async_delete, MockCallsMatch(call())) | ||
225 | 1668 | |||
226 | 1669 | def test_pod_DELETE_delete_without_force(self): | ||
227 | 1670 | self.become_admin() | ||
228 | 1671 | vlan = factory.make_VLAN() | ||
229 | 1672 | subnet = factory.make_Subnet(vlan=vlan) | ||
230 | 1673 | machine = factory.make_Machine_with_Interface_on_Subnet( | ||
231 | 1674 | vlan=vlan, subnet=subnet) | ||
232 | 1675 | ip = factory.make_StaticIPAddress(interface=machine.boot_interface) | ||
233 | 1676 | factory.make_Pod(ip_address=ip) | ||
234 | 1677 | mock_async_delete = self.patch(Pod, "async_delete") | ||
235 | 1678 | response = self.client.delete(self.get_machine_uri(machine)) | ||
236 | 1679 | self.assertEqual( | ||
237 | 1680 | http.client.BAD_REQUEST, response.status_code, | ||
238 | 1681 | explain_unexpected_response(http.client.BAD_REQUEST, response)) | ||
239 | 1682 | self.assertThat(mock_async_delete, MockNotCalled()) | ||
240 | 1683 | |||
241 | 1644 | 1684 | ||
242 | 1645 | class TestMachineAPITransactional(APITransactionTestCase.ForUser): | 1685 | class TestMachineAPITransactional(APITransactionTestCase.ForUser): |
243 | 1646 | """The following TestMachineAPI tests require APITransactionTestCase.""" | 1686 | """The following TestMachineAPI tests require APITransactionTestCase.""" |
244 | diff --git a/src/maasserver/api/tests/test_rackcontroller.py b/src/maasserver/api/tests/test_rackcontroller.py | |||
245 | index 1d0a485..686fe82 100644 | |||
246 | --- a/src/maasserver/api/tests/test_rackcontroller.py | |||
247 | +++ b/src/maasserver/api/tests/test_rackcontroller.py | |||
248 | @@ -4,11 +4,14 @@ | |||
249 | 4 | """Tests for the Rack Controller API.""" | 4 | """Tests for the Rack Controller API.""" |
250 | 5 | 5 | ||
251 | 6 | import http.client | 6 | import http.client |
252 | 7 | from unittest.mock import call | ||
253 | 7 | 8 | ||
254 | 8 | from django.utils.http import urlencode | 9 | from django.utils.http import urlencode |
255 | 9 | from maasserver.api import rackcontrollers | 10 | from maasserver.api import rackcontrollers |
256 | 11 | from maasserver.models.bmc import Pod | ||
257 | 10 | from maasserver.testing.api import ( | 12 | from maasserver.testing.api import ( |
258 | 11 | APITestCase, | 13 | APITestCase, |
259 | 14 | APITransactionTestCase, | ||
260 | 12 | explain_unexpected_response, | 15 | explain_unexpected_response, |
261 | 13 | ) | 16 | ) |
262 | 14 | from maasserver.testing.factory import factory | 17 | from maasserver.testing.factory import factory |
263 | @@ -18,11 +21,12 @@ from maasserver.utils.orm import reload_object | |||
264 | 18 | from maastesting.matchers import ( | 21 | from maastesting.matchers import ( |
265 | 19 | MockCalledOnce, | 22 | MockCalledOnce, |
266 | 20 | MockCalledOnceWith, | 23 | MockCalledOnceWith, |
267 | 24 | MockCallsMatch, | ||
268 | 21 | MockNotCalled, | 25 | MockNotCalled, |
269 | 22 | ) | 26 | ) |
270 | 23 | 27 | ||
271 | 24 | 28 | ||
273 | 25 | class TestRackControllerAPI(APITestCase.ForUser): | 29 | class TestRackControllerAPI(APITransactionTestCase.ForUser): |
274 | 26 | """Tests for /api/2.0/rackcontrollers/<rack>/.""" | 30 | """Tests for /api/2.0/rackcontrollers/<rack>/.""" |
275 | 27 | 31 | ||
276 | 28 | def test_handler_path(self): | 32 | def test_handler_path(self): |
277 | @@ -107,10 +111,14 @@ class TestRackControllerAPI(APITestCase.ForUser): | |||
278 | 107 | def test_DELETE_delete_with_force(self): | 111 | def test_DELETE_delete_with_force(self): |
279 | 108 | self.become_admin() | 112 | self.become_admin() |
280 | 109 | vlan = factory.make_VLAN() | 113 | vlan = factory.make_VLAN() |
281 | 114 | factory.make_Subnet(vlan=vlan) | ||
282 | 110 | rack = factory.make_RackController(vlan=vlan) | 115 | rack = factory.make_RackController(vlan=vlan) |
283 | 116 | ip = factory.make_StaticIPAddress(interface=rack.interface_set.first()) | ||
284 | 117 | factory.make_Pod(ip_address=ip) | ||
285 | 111 | vlan.dhcp_on = True | 118 | vlan.dhcp_on = True |
286 | 112 | vlan.primary_rack = rack | 119 | vlan.primary_rack = rack |
287 | 113 | vlan.save() | 120 | vlan.save() |
288 | 121 | mock_async_delete = self.patch(Pod, "async_delete") | ||
289 | 114 | response = self.client.delete( | 122 | response = self.client.delete( |
290 | 115 | self.get_rack_uri(rack), QUERY_STRING=urlencode({ | 123 | self.get_rack_uri(rack), QUERY_STRING=urlencode({ |
291 | 116 | 'force': 'true' | 124 | 'force': 'true' |
292 | @@ -118,6 +126,42 @@ class TestRackControllerAPI(APITestCase.ForUser): | |||
293 | 118 | self.assertEqual( | 126 | self.assertEqual( |
294 | 119 | http.client.NO_CONTENT, response.status_code, | 127 | http.client.NO_CONTENT, response.status_code, |
295 | 120 | explain_unexpected_response(http.client.NO_CONTENT, response)) | 128 | explain_unexpected_response(http.client.NO_CONTENT, response)) |
296 | 129 | self.assertThat(mock_async_delete, MockCallsMatch(call())) | ||
297 | 130 | |||
298 | 131 | def test_pod_DELETE_delete_without_force(self): | ||
299 | 132 | self.become_admin() | ||
300 | 133 | vlan = factory.make_VLAN() | ||
301 | 134 | factory.make_Subnet(vlan=vlan) | ||
302 | 135 | rack = factory.make_RackController(vlan=vlan) | ||
303 | 136 | ip = factory.make_StaticIPAddress(interface=rack.interface_set.first()) | ||
304 | 137 | factory.make_Pod(ip_address=ip) | ||
305 | 138 | vlan.dhcp_on = True | ||
306 | 139 | vlan.primary_rack = rack | ||
307 | 140 | vlan.save() | ||
308 | 141 | mock_async_delete = self.patch(Pod, "async_delete") | ||
309 | 142 | response = self.client.delete(self.get_rack_uri(rack)) | ||
310 | 143 | self.assertEqual( | ||
311 | 144 | http.client.BAD_REQUEST, response.status_code, | ||
312 | 145 | explain_unexpected_response(http.client.BAD_REQUEST, response)) | ||
313 | 146 | self.assertThat(mock_async_delete, MockNotCalled()) | ||
314 | 147 | |||
315 | 148 | def test_DELETE_force_not_required_for_pod_region_rack(self): | ||
316 | 149 | self.become_admin() | ||
317 | 150 | vlan = factory.make_VLAN() | ||
318 | 151 | factory.make_Subnet(vlan=vlan) | ||
319 | 152 | rack = factory.make_RegionRackController(vlan=vlan) | ||
320 | 153 | ip = factory.make_StaticIPAddress( | ||
321 | 154 | interface=rack.interface_set.first()) | ||
322 | 155 | factory.make_Pod(ip_address=ip) | ||
323 | 156 | mock_async_delete = self.patch(Pod, "async_delete") | ||
324 | 157 | response = self.client.delete( | ||
325 | 158 | self.get_rack_uri(rack), QUERY_STRING=urlencode({ | ||
326 | 159 | 'force': 'true' | ||
327 | 160 | }, doseq=True)) | ||
328 | 161 | self.assertEqual( | ||
329 | 162 | http.client.NO_CONTENT, response.status_code, | ||
330 | 163 | explain_unexpected_response(http.client.NO_CONTENT, response)) | ||
331 | 164 | self.assertThat(mock_async_delete, MockNotCalled()) | ||
332 | 121 | 165 | ||
333 | 122 | 166 | ||
334 | 123 | class TestRackControllersAPI(APITestCase.ForUser): | 167 | class TestRackControllersAPI(APITestCase.ForUser): |
335 | diff --git a/src/maasserver/api/tests/test_regioncontroller.py b/src/maasserver/api/tests/test_regioncontroller.py | |||
336 | index f37d557..19c7993 100644 | |||
337 | --- a/src/maasserver/api/tests/test_regioncontroller.py | |||
338 | +++ b/src/maasserver/api/tests/test_regioncontroller.py | |||
339 | @@ -4,12 +4,23 @@ | |||
340 | 4 | """Tests for the Region Controller API.""" | 4 | """Tests for the Region Controller API.""" |
341 | 5 | 5 | ||
342 | 6 | import http.client | 6 | import http.client |
343 | 7 | from unittest.mock import call | ||
344 | 7 | 8 | ||
346 | 8 | from maasserver.testing.api import APITestCase | 9 | from django.utils.http import urlencode |
347 | 10 | from maasserver.enum import NODE_TYPE | ||
348 | 11 | from maasserver.models.bmc import Pod | ||
349 | 12 | from maasserver.testing.api import ( | ||
350 | 13 | APITestCase, | ||
351 | 14 | explain_unexpected_response, | ||
352 | 15 | ) | ||
353 | 9 | from maasserver.testing.factory import factory | 16 | from maasserver.testing.factory import factory |
354 | 10 | from maasserver.utils.converters import json_load_bytes | 17 | from maasserver.utils.converters import json_load_bytes |
355 | 11 | from maasserver.utils.django_urls import reverse | 18 | from maasserver.utils.django_urls import reverse |
356 | 12 | from maasserver.utils.orm import reload_object | 19 | from maasserver.utils.orm import reload_object |
357 | 20 | from maastesting.matchers import ( | ||
358 | 21 | MockCallsMatch, | ||
359 | 22 | MockNotCalled, | ||
360 | 23 | ) | ||
361 | 13 | 24 | ||
362 | 14 | 25 | ||
363 | 15 | class TestRegionControllerAPI(APITestCase.ForUser): | 26 | class TestRegionControllerAPI(APITestCase.ForUser): |
364 | @@ -39,6 +50,59 @@ class TestRegionControllerAPI(APITestCase.ForUser): | |||
365 | 39 | response = self.client.put(self.get_region_uri(region), {}) | 50 | response = self.client.put(self.get_region_uri(region), {}) |
366 | 40 | self.assertEqual(http.client.FORBIDDEN, response.status_code) | 51 | self.assertEqual(http.client.FORBIDDEN, response.status_code) |
367 | 41 | 52 | ||
368 | 53 | def test_DELETE_delete_with_force(self): | ||
369 | 54 | self.become_admin() | ||
370 | 55 | vlan = factory.make_VLAN() | ||
371 | 56 | subnet = factory.make_Subnet(vlan=vlan) | ||
372 | 57 | region = factory.make_Node_with_Interface_on_Subnet( | ||
373 | 58 | node_type=NODE_TYPE.REGION_CONTROLLER, subnet=subnet, vlan=vlan) | ||
374 | 59 | ip = factory.make_StaticIPAddress( | ||
375 | 60 | interface=region.interface_set.first()) | ||
376 | 61 | factory.make_Pod(ip_address=ip) | ||
377 | 62 | mock_async_delete = self.patch(Pod, "async_delete") | ||
378 | 63 | response = self.client.delete( | ||
379 | 64 | self.get_region_uri(region), QUERY_STRING=urlencode({ | ||
380 | 65 | 'force': 'true' | ||
381 | 66 | }, doseq=True)) | ||
382 | 67 | self.assertEqual( | ||
383 | 68 | http.client.NO_CONTENT, response.status_code, | ||
384 | 69 | explain_unexpected_response(http.client.NO_CONTENT, response)) | ||
385 | 70 | self.assertThat(mock_async_delete, MockCallsMatch(call())) | ||
386 | 71 | |||
387 | 72 | def test_DELETE_force_not_required_for_pod_region_rack(self): | ||
388 | 73 | self.become_admin() | ||
389 | 74 | vlan = factory.make_VLAN() | ||
390 | 75 | factory.make_Subnet(vlan=vlan) | ||
391 | 76 | rack = factory.make_RegionRackController(vlan=vlan) | ||
392 | 77 | ip = factory.make_StaticIPAddress( | ||
393 | 78 | interface=rack.interface_set.first()) | ||
394 | 79 | factory.make_Pod(ip_address=ip) | ||
395 | 80 | mock_async_delete = self.patch(Pod, "async_delete") | ||
396 | 81 | response = self.client.delete( | ||
397 | 82 | self.get_region_uri(rack), QUERY_STRING=urlencode({ | ||
398 | 83 | 'force': 'true' | ||
399 | 84 | }, doseq=True)) | ||
400 | 85 | self.assertEqual( | ||
401 | 86 | http.client.NO_CONTENT, response.status_code, | ||
402 | 87 | explain_unexpected_response(http.client.NO_CONTENT, response)) | ||
403 | 88 | self.assertThat(mock_async_delete, MockNotCalled()) | ||
404 | 89 | |||
405 | 90 | def test_pod_DELETE_delete_without_force(self): | ||
406 | 91 | self.become_admin() | ||
407 | 92 | vlan = factory.make_VLAN() | ||
408 | 93 | subnet = factory.make_Subnet(vlan=vlan) | ||
409 | 94 | region = factory.make_Node_with_Interface_on_Subnet( | ||
410 | 95 | node_type=NODE_TYPE.REGION_CONTROLLER, subnet=subnet, vlan=vlan) | ||
411 | 96 | ip = factory.make_StaticIPAddress( | ||
412 | 97 | interface=region.interface_set.first()) | ||
413 | 98 | factory.make_Pod(ip_address=ip) | ||
414 | 99 | mock_async_delete = self.patch(Pod, "async_delete") | ||
415 | 100 | response = self.client.delete(self.get_region_uri(region)) | ||
416 | 101 | self.assertEqual( | ||
417 | 102 | http.client.BAD_REQUEST, response.status_code, | ||
418 | 103 | explain_unexpected_response(http.client.BAD_REQUEST, response)) | ||
419 | 104 | self.assertThat(mock_async_delete, MockNotCalled()) | ||
420 | 105 | |||
421 | 42 | 106 | ||
422 | 43 | class TestRegionControllersAPI(APITestCase.ForUser): | 107 | class TestRegionControllersAPI(APITestCase.ForUser): |
423 | 44 | """Tests for /api/2.0/regioncontrollers/.""" | 108 | """Tests for /api/2.0/regioncontrollers/.""" |
424 | diff --git a/src/maasserver/models/bmc.py b/src/maasserver/models/bmc.py | |||
425 | index addc187..2b80690 100644 | |||
426 | --- a/src/maasserver/models/bmc.py | |||
427 | +++ b/src/maasserver/models/bmc.py | |||
428 | @@ -1282,6 +1282,21 @@ class Pod(BMC): | |||
429 | 1282 | "Use `async_delete` instead. Deleting a Pod takes " | 1282 | "Use `async_delete` instead. Deleting a Pod takes " |
430 | 1283 | "an asynchronous action.") | 1283 | "an asynchronous action.") |
431 | 1284 | 1284 | ||
432 | 1285 | def delete_and_wait(self): | ||
433 | 1286 | """Block the current thread while waiting for the pod to be deleted. | ||
434 | 1287 | |||
435 | 1288 | This must not be called from a deferToDatabase thread; use the | ||
436 | 1289 | async_delete() method instead. | ||
437 | 1290 | """ | ||
438 | 1291 | # Calculate the wait time based on the number of none pre-existing | ||
439 | 1292 | # machines. We allow maximum of 60 seconds per machine plus 60 seconds | ||
440 | 1293 | # for the pod. | ||
441 | 1294 | pod = self.as_pod() | ||
442 | 1295 | num_machines = Machine.objects.filter(bmc=pod) | ||
443 | 1296 | num_machines = num_machines.exclude( | ||
444 | 1297 | creation_type=NODE_CREATION_TYPE.PRE_EXISTING) | ||
445 | 1298 | pod.async_delete().wait((num_machines.count() * 60) + 60) | ||
446 | 1299 | |||
447 | 1285 | @asynchronous | 1300 | @asynchronous |
448 | 1286 | def async_delete(self): | 1301 | def async_delete(self): |
449 | 1287 | """Delete a pod asynchronously. | 1302 | """Delete a pod asynchronously. |
450 | diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py | |||
451 | index b431d10..9e0b087 100644 | |||
452 | --- a/src/maasserver/models/node.py | |||
453 | +++ b/src/maasserver/models/node.py | |||
454 | @@ -235,12 +235,14 @@ from provisioningserver.utils.twisted import ( | |||
455 | 235 | synchronous, | 235 | synchronous, |
456 | 236 | undefined, | 236 | undefined, |
457 | 237 | ) | 237 | ) |
458 | 238 | from twisted.internet import reactor | ||
459 | 238 | from twisted.internet.defer import ( | 239 | from twisted.internet.defer import ( |
460 | 239 | Deferred, | 240 | Deferred, |
461 | 240 | inlineCallbacks, | 241 | inlineCallbacks, |
462 | 241 | succeed, | 242 | succeed, |
463 | 242 | ) | 243 | ) |
464 | 243 | from twisted.internet.threads import deferToThread | 244 | from twisted.internet.threads import deferToThread |
465 | 245 | from twisted.python.threadable import isInIOThread | ||
466 | 244 | 246 | ||
467 | 245 | 247 | ||
468 | 246 | log = LegacyLogger() | 248 | log = LegacyLogger() |
469 | @@ -2362,7 +2364,7 @@ class Node(CleanSave, TimestampedModel): | |||
470 | 2362 | callOut, maaslog.warning, "%s: Could not stop node to abort " | 2364 | callOut, maaslog.warning, "%s: Could not stop node to abort " |
471 | 2363 | "deployment; it must be stopped manually", hostname) | 2365 | "deployment; it must be stopped manually", hostname) |
472 | 2364 | 2366 | ||
474 | 2365 | def delete(self): | 2367 | def delete(self, *args, **kwargs): |
475 | 2366 | """Delete this node.""" | 2368 | """Delete this node.""" |
476 | 2367 | bmc = self.bmc | 2369 | bmc = self.bmc |
477 | 2368 | if (self.node_type == NODE_TYPE.MACHINE and | 2370 | if (self.node_type == NODE_TYPE.MACHINE and |
478 | @@ -2414,7 +2416,7 @@ class Node(CleanSave, TimestampedModel): | |||
479 | 2414 | "%s: Deleting my BMC '%s'", self.hostname, self.bmc) | 2416 | "%s: Deleting my BMC '%s'", self.hostname, self.bmc) |
480 | 2415 | self.bmc.delete() | 2417 | self.bmc.delete() |
481 | 2416 | 2418 | ||
483 | 2417 | super(Node, self).delete() | 2419 | super(Node, self).delete(*args, **kwargs) |
484 | 2418 | 2420 | ||
485 | 2419 | def set_random_hostname(self): | 2421 | def set_random_hostname(self): |
486 | 2420 | """Set a random `hostname`.""" | 2422 | """Set a random `hostname`.""" |
487 | @@ -2966,10 +2968,11 @@ class Node(CleanSave, TimestampedModel): | |||
488 | 2966 | OwnerData.objects.filter(node=self).delete() | 2968 | OwnerData.objects.filter(node=self).delete() |
489 | 2967 | 2969 | ||
490 | 2968 | def release_or_erase( | 2970 | def release_or_erase( |
493 | 2969 | self, user, comment=None, | 2971 | self, user, comment=None, erase=False, secure_erase=None, |
494 | 2970 | erase=False, secure_erase=None, quick_erase=None): | 2972 | quick_erase=None, force=False): |
495 | 2971 | """Either release the node or erase the node then release it, depending | 2973 | """Either release the node or erase the node then release it, depending |
496 | 2972 | on settings and parameters.""" | 2974 | on settings and parameters.""" |
497 | 2975 | self.maybe_delete_pods(not force) | ||
498 | 2973 | erase_on_release = Config.objects.get_config( | 2976 | erase_on_release = Config.objects.get_config( |
499 | 2974 | 'enable_disk_erasing_on_release') | 2977 | 'enable_disk_erasing_on_release') |
500 | 2975 | if erase or erase_on_release: | 2978 | if erase or erase_on_release: |
501 | @@ -2979,6 +2982,27 @@ class Node(CleanSave, TimestampedModel): | |||
502 | 2979 | else: | 2982 | else: |
503 | 2980 | self.release(user, comment) | 2983 | self.release(user, comment) |
504 | 2981 | 2984 | ||
505 | 2985 | def maybe_delete_pods(self, dry_run: bool): | ||
506 | 2986 | """Check if any pods are associated with this Node. | ||
507 | 2987 | |||
508 | 2988 | All pods will be deleted if dry_run=False is passed in. | ||
509 | 2989 | |||
510 | 2990 | :param dry_run: If True, raises NodeActionError rather than deleting | ||
511 | 2991 | pods. | ||
512 | 2992 | """ | ||
513 | 2993 | hosted_pods = list( | ||
514 | 2994 | self.get_hosted_pods().values_list('name', flat=True)) | ||
515 | 2995 | if len(hosted_pods) > 0: | ||
516 | 2996 | if dry_run: | ||
517 | 2997 | raise ValidationError( | ||
518 | 2998 | "The following pods must be removed first: %s" % ( | ||
519 | 2999 | ", ".join(hosted_pods))) | ||
520 | 3000 | for pod in self.get_hosted_pods(): | ||
521 | 3001 | if isInIOThread(): | ||
522 | 3002 | pod.async_delete() | ||
523 | 3003 | else: | ||
524 | 3004 | reactor.callFromThread(pod.async_delete) | ||
525 | 3005 | |||
526 | 2982 | def set_netboot(self, on=True): | 3006 | def set_netboot(self, on=True): |
527 | 2983 | """Set netboot on or off.""" | 3007 | """Set netboot on or off.""" |
528 | 2984 | log.debug( | 3008 | log.debug( |
529 | @@ -4397,6 +4421,13 @@ class Node(CleanSave, TimestampedModel): | |||
530 | 4397 | else: | 4421 | else: |
531 | 4398 | return script_result.stdout.decode('utf-8').splitlines() | 4422 | return script_result.stdout.decode('utf-8').splitlines() |
532 | 4399 | 4423 | ||
533 | 4424 | def get_hosted_pods(self) -> QuerySet: | ||
534 | 4425 | # Circular imports | ||
535 | 4426 | from maasserver.models import Pod | ||
536 | 4427 | our_static_ips = StaticIPAddress.objects.filter( | ||
537 | 4428 | interface__node=self).values_list('ip') | ||
538 | 4429 | return Pod.objects.filter(ip_address__ip__in=our_static_ips) | ||
539 | 4430 | |||
540 | 4400 | 4431 | ||
541 | 4401 | # Piston serializes objects based on the object class. | 4432 | # Piston serializes objects based on the object class. |
542 | 4402 | # Here we define a proxy class so that we can specialize how devices are | 4433 | # Here we define a proxy class so that we can specialize how devices are |
543 | @@ -4413,6 +4444,17 @@ class Machine(Node): | |||
544 | 4413 | super(Machine, self).__init__( | 4444 | super(Machine, self).__init__( |
545 | 4414 | node_type=NODE_TYPE.MACHINE, *args, **kwargs) | 4445 | node_type=NODE_TYPE.MACHINE, *args, **kwargs) |
546 | 4415 | 4446 | ||
547 | 4447 | def delete(self, force=False): | ||
548 | 4448 | """Deletes this Machine. | ||
549 | 4449 | |||
550 | 4450 | Before deletion, checks if any hosted pods exist. | ||
551 | 4451 | |||
552 | 4452 | Raises ValidationError if the machine is a host for one or more pods, | ||
553 | 4453 | and `force=True` was not specified. | ||
554 | 4454 | """ | ||
555 | 4455 | self.maybe_delete_pods(not force) | ||
556 | 4456 | return super().delete() | ||
557 | 4457 | |||
558 | 4416 | 4458 | ||
559 | 4417 | class Controller(Node): | 4459 | class Controller(Node): |
560 | 4418 | """A node which is either a rack or region controller.""" | 4460 | """A node which is either a rack or region controller.""" |
561 | @@ -5352,6 +5394,11 @@ class RackController(Controller): | |||
562 | 5352 | 5394 | ||
563 | 5353 | def delete(self, force=False): | 5395 | def delete(self, force=False): |
564 | 5354 | """Delete this rack controller.""" | 5396 | """Delete this rack controller.""" |
565 | 5397 | # Don't bother with the pod check if this is a region+rack, because | ||
566 | 5398 | # deleting a region+rack results in a region-only controller. | ||
567 | 5399 | if self.node_type != NODE_TYPE.REGION_AND_RACK_CONTROLLER: | ||
568 | 5400 | self.maybe_delete_pods(not force) | ||
569 | 5401 | |||
570 | 5355 | # Avoid circular imports | 5402 | # Avoid circular imports |
571 | 5356 | from maasserver.models import RegionRackRPCConnection | 5403 | from maasserver.models import RegionRackRPCConnection |
572 | 5357 | 5404 | ||
573 | @@ -5518,8 +5565,9 @@ class RegionController(Controller): | |||
574 | 5518 | super(RegionController, self).__init__( | 5565 | super(RegionController, self).__init__( |
575 | 5519 | node_type=NODE_TYPE.REGION_CONTROLLER, *args, **kwargs) | 5566 | node_type=NODE_TYPE.REGION_CONTROLLER, *args, **kwargs) |
576 | 5520 | 5567 | ||
578 | 5521 | def delete(self): | 5568 | def delete(self, force=False): |
579 | 5522 | """Delete this region controller.""" | 5569 | """Delete this region controller.""" |
580 | 5570 | self.maybe_delete_pods(not force) | ||
581 | 5523 | # Avoid circular dependency. | 5571 | # Avoid circular dependency. |
582 | 5524 | from maasserver.models import RegionControllerProcess | 5572 | from maasserver.models import RegionControllerProcess |
583 | 5525 | connections = RegionControllerProcess.objects.filter( | 5573 | connections = RegionControllerProcess.objects.filter( |
584 | diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py | |||
585 | index 5b367ae..e596dd8 100644 | |||
586 | --- a/src/maasserver/models/tests/test_node.py | |||
587 | +++ b/src/maasserver/models/tests/test_node.py | |||
588 | @@ -34,6 +34,7 @@ from django.core.exceptions import ( | |||
589 | 34 | ) | 34 | ) |
590 | 35 | from django.db import transaction | 35 | from django.db import transaction |
591 | 36 | from django.db.models.deletion import Collector | 36 | from django.db.models.deletion import Collector |
592 | 37 | from django.db.models.query import QuerySet | ||
593 | 37 | from fixtures import LoggerFixture | 38 | from fixtures import LoggerFixture |
594 | 38 | from maasserver import ( | 39 | from maasserver import ( |
595 | 39 | bootresources, | 40 | bootresources, |
596 | @@ -10844,3 +10845,18 @@ class TestControllerGetDiscoveryState(MAASServerTestCase): | |||
597 | 10844 | self.assertThat(monitoring_state, Contains('eth2')) | 10845 | self.assertThat(monitoring_state, Contains('eth2')) |
598 | 10845 | self.assertThat( | 10846 | self.assertThat( |
599 | 10846 | monitoring_state['eth1'], Equals(eth1.get_discovery_state())) | 10847 | monitoring_state['eth1'], Equals(eth1.get_discovery_state())) |
600 | 10848 | |||
601 | 10849 | |||
602 | 10850 | class TestNodeGetHostedPods(MAASServerTestCase): | ||
603 | 10851 | |||
604 | 10852 | def test__returns_queryset(self): | ||
605 | 10853 | node = factory.make_Node() | ||
606 | 10854 | pods = node.get_hosted_pods() | ||
607 | 10855 | self.assertThat(pods, IsInstance(QuerySet)) | ||
608 | 10856 | |||
609 | 10857 | def test__returns_related_pods(self): | ||
610 | 10858 | node = factory.make_Node_with_Interface_on_Subnet() | ||
611 | 10859 | ip = factory.make_StaticIPAddress(interface=node.boot_interface) | ||
612 | 10860 | pod = factory.make_Pod(ip_address=ip) | ||
613 | 10861 | pods = node.get_hosted_pods() | ||
614 | 10862 | self.assertThat(pods, Contains(pod)) |
!test