diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py index 12cbc69..5bbe55d 100644 --- a/src/maasserver/api/machines.py +++ b/src/maasserver/api/machines.py @@ -107,6 +107,7 @@ from maasserver.utils.orm import ( get_first, reload_object, ) +from piston3.utils import rc import yaml # Machine's fields exposed on the API. @@ -320,6 +321,24 @@ class MachineHandler(NodeHandler, OwnerDataMixin, PowerMixin): model = Machine fields = DISPLAYED_MACHINE_FIELDS + def delete(self, request, system_id): + """Delete a specific machine. + + A machine cannot be deleted if it hosts pod virtual machines. + Use `force` to override this behavior. Forcing deletion will also + remove hosted pods. + + Returns 404 if the node is not found. + Returns 403 if the user does not have permission to delete the node. + Returns 400 if the machine cannot be deleted. + Returns 204 if the node is successfully deleted. + """ + node = self.model.objects.get_node_or_404( + system_id=system_id, user=request.user, perm=NODE_PERMISSION.ADMIN) + node.as_self().delete( + force=get_optional_param(request.GET, 'force', False, StringBool)) + return rc.DELETED + @classmethod def boot_disk(handler, machine): """Return the boot_disk for the machine.""" diff --git a/src/maasserver/api/pods.py b/src/maasserver/api/pods.py index 141c9d9..0c6b28c 100644 --- a/src/maasserver/api/pods.py +++ b/src/maasserver/api/pods.py @@ -16,14 +16,12 @@ from maasserver.api.support import ( OperationsHandler, ) from maasserver.api.utils import get_mandatory_param -from maasserver.enum import NODE_CREATION_TYPE from maasserver.exceptions import MAASAPIValidationError from maasserver.forms.pods import ( ComposeMachineForm, PodForm, ) from maasserver.models.bmc import Pod -from maasserver.models.node import Machine from maasserver.utils.django_urls import reverse from piston3.utils import rc from provisioningserver.drivers.pod import Capabilities @@ -178,13 +176,7 @@ class PodHandler(OperationsHandler): Returns 204 if the pod is successfully deleted. """ pod = get_object_or_404(Pod, id=id) - # Calculate the wait time based on the number of none pre-existing - # machines. We allow maximum of 60 seconds per machine plus 60 seconds - # for the pod. - num_machines = Machine.objects.filter(bmc=pod) - num_machines = num_machines.exclude( - creation_type=NODE_CREATION_TYPE.PRE_EXISTING) - pod.async_delete().wait((num_machines.count() * 60) + 60) + pod.delete_and_wait() return rc.DELETED @admin_method diff --git a/src/maasserver/api/rackcontrollers.py b/src/maasserver/api/rackcontrollers.py index d09e8ed..ba59020 100644 --- a/src/maasserver/api/rackcontrollers.py +++ b/src/maasserver/api/rackcontrollers.py @@ -6,7 +6,6 @@ __all__ = [ 'RackControllersHandler', ] - from django.conf import settings from django.http import HttpResponse from formencode.validators import StringBool @@ -93,6 +92,12 @@ class RackControllerHandler(NodeHandler, PowerMixin): a `VLAN` and another rack controller cannot be used to provide DHCP for said VLAN. Use `force` to override this behavior. + Using `force` will also allow deleting a rack controller that is + hosting pod virtual machines. (The pod will also be deleted.) + + Rack controllers that are also region controllers will be converted + to a region controller (and hosted pods will not be affected). + :param force: Always delete the rack controller even if its the `primary_rack` on a `VLAN` and another rack controller cannot provide DHCP on said VLAN. This will disable DHCP on those VLANs. @@ -100,11 +105,11 @@ class RackControllerHandler(NodeHandler, PowerMixin): Returns 404 if the node is not found. Returns 403 if the user does not have permission to delete the node. + Returns 400 if the node cannot be deleted. Returns 204 if the node is successfully deleted. """ node = self.model.objects.get_node_or_404( - system_id=system_id, user=request.user, - perm=NODE_PERMISSION.ADMIN) + system_id=system_id, user=request.user, perm=NODE_PERMISSION.ADMIN) node.as_self().delete( force=get_optional_param(request.GET, 'force', False, StringBool)) return rc.DELETED diff --git a/src/maasserver/api/regioncontrollers.py b/src/maasserver/api/regioncontrollers.py index d366437..7eb0099 100644 --- a/src/maasserver/api/regioncontrollers.py +++ b/src/maasserver/api/regioncontrollers.py @@ -6,16 +6,19 @@ __all__ = [ 'RegionControllersHandler', ] +from formencode.validators import StringBool from maasserver.api.interfaces import DISPLAYED_INTERFACE_FIELDS from maasserver.api.nodes import ( NodeHandler, NodesHandler, ) from maasserver.api.support import admin_method +from maasserver.api.utils import get_optional_param from maasserver.enum import NODE_PERMISSION from maasserver.exceptions import MAASAPIValidationError from maasserver.forms import ControllerForm from maasserver.models import RegionController +from piston3.utils import rc # Region controller's fields exposed on the API. DISPLAYED_REGION_CONTROLLER_FIELDS = ( @@ -68,6 +71,24 @@ class RegionControllerHandler(NodeHandler): model = RegionController fields = DISPLAYED_REGION_CONTROLLER_FIELDS + def delete(self, request, system_id): + """Delete a specific region controller. + + A region controller cannot be deleted if it hosts pod virtual machines. + Use `force` to override this behavior. Forcing deletion will also + remove hosted pods. + + Returns 404 if the node is not found. + Returns 403 if the user does not have permission to delete the node. + Returns 400 if the node cannot be deleted. + Returns 204 if the node is successfully deleted. + """ + node = self.model.objects.get_node_or_404( + system_id=system_id, user=request.user, perm=NODE_PERMISSION.ADMIN) + node.as_self().delete( + force=get_optional_param(request.GET, 'force', False, StringBool)) + return rc.DELETED + @admin_method def update(self, request, system_id): """Update a specific Region controller. diff --git a/src/maasserver/api/tests/test_machine.py b/src/maasserver/api/tests/test_machine.py index 40f2834..d1979d6 100644 --- a/src/maasserver/api/tests/test_machine.py +++ b/src/maasserver/api/tests/test_machine.py @@ -8,10 +8,14 @@ __all__ = [] from base64 import b64encode import http.client from random import choice -from unittest.mock import ANY +from unittest.mock import ( + ANY, + call, +) from django.conf import settings from django.db import transaction +from django.utils.http import urlencode from maasserver import forms from maasserver.api import machines as machines_module from maasserver.enum import ( @@ -35,6 +39,7 @@ from maasserver.models import ( node as node_module, StaticIPAddress, ) +from maasserver.models.bmc import Pod from maasserver.models.node import RELEASABLE_STATUSES from maasserver.models.signals.testing import SignalsDisabled from maasserver.storage_layouts import ( @@ -44,6 +49,7 @@ from maasserver.storage_layouts import ( from maasserver.testing.api import ( APITestCase, APITransactionTestCase, + explain_unexpected_response, ) from maasserver.testing.architecture import make_usable_architecture from maasserver.testing.factory import factory @@ -63,6 +69,7 @@ from maastesting.matchers import ( HasLength, MockCalledOnce, MockCalledOnceWith, + MockCallsMatch, MockNotCalled, ) from metadataserver.enum import SCRIPT_TYPE @@ -1641,6 +1648,39 @@ class TestMachineAPI(APITestCase.ForUser): self.assertEqual(http.client.NOT_FOUND, response.status_code) + def test_DELETE_delete_with_force(self): + self.become_admin() + vlan = factory.make_VLAN() + subnet = factory.make_Subnet(vlan=vlan) + machine = factory.make_Machine_with_Interface_on_Subnet( + vlan=vlan, subnet=subnet) + ip = factory.make_StaticIPAddress(interface=machine.boot_interface) + factory.make_Pod(ip_address=ip) + mock_async_delete = self.patch(Pod, "async_delete") + response = self.client.delete( + self.get_machine_uri(machine), QUERY_STRING=urlencode({ + 'force': 'true' + }, doseq=True)) + self.assertEqual( + http.client.NO_CONTENT, response.status_code, + explain_unexpected_response(http.client.NO_CONTENT, response)) + self.assertThat(mock_async_delete, MockCallsMatch(call())) + + def test_pod_DELETE_delete_without_force(self): + self.become_admin() + vlan = factory.make_VLAN() + subnet = factory.make_Subnet(vlan=vlan) + machine = factory.make_Machine_with_Interface_on_Subnet( + vlan=vlan, subnet=subnet) + ip = factory.make_StaticIPAddress(interface=machine.boot_interface) + factory.make_Pod(ip_address=ip) + mock_async_delete = self.patch(Pod, "async_delete") + response = self.client.delete(self.get_machine_uri(machine)) + self.assertEqual( + http.client.BAD_REQUEST, response.status_code, + explain_unexpected_response(http.client.BAD_REQUEST, response)) + self.assertThat(mock_async_delete, MockNotCalled()) + class TestMachineAPITransactional(APITransactionTestCase.ForUser): """The following TestMachineAPI tests require APITransactionTestCase.""" diff --git a/src/maasserver/api/tests/test_rackcontroller.py b/src/maasserver/api/tests/test_rackcontroller.py index 1d0a485..686fe82 100644 --- a/src/maasserver/api/tests/test_rackcontroller.py +++ b/src/maasserver/api/tests/test_rackcontroller.py @@ -4,11 +4,14 @@ """Tests for the Rack Controller API.""" import http.client +from unittest.mock import call from django.utils.http import urlencode from maasserver.api import rackcontrollers +from maasserver.models.bmc import Pod from maasserver.testing.api import ( APITestCase, + APITransactionTestCase, explain_unexpected_response, ) from maasserver.testing.factory import factory @@ -18,11 +21,12 @@ from maasserver.utils.orm import reload_object from maastesting.matchers import ( MockCalledOnce, MockCalledOnceWith, + MockCallsMatch, MockNotCalled, ) -class TestRackControllerAPI(APITestCase.ForUser): +class TestRackControllerAPI(APITransactionTestCase.ForUser): """Tests for /api/2.0/rackcontrollers//.""" def test_handler_path(self): @@ -107,10 +111,14 @@ class TestRackControllerAPI(APITestCase.ForUser): def test_DELETE_delete_with_force(self): self.become_admin() vlan = factory.make_VLAN() + factory.make_Subnet(vlan=vlan) rack = factory.make_RackController(vlan=vlan) + ip = factory.make_StaticIPAddress(interface=rack.interface_set.first()) + factory.make_Pod(ip_address=ip) vlan.dhcp_on = True vlan.primary_rack = rack vlan.save() + mock_async_delete = self.patch(Pod, "async_delete") response = self.client.delete( self.get_rack_uri(rack), QUERY_STRING=urlencode({ 'force': 'true' @@ -118,6 +126,42 @@ class TestRackControllerAPI(APITestCase.ForUser): self.assertEqual( http.client.NO_CONTENT, response.status_code, explain_unexpected_response(http.client.NO_CONTENT, response)) + self.assertThat(mock_async_delete, MockCallsMatch(call())) + + def test_pod_DELETE_delete_without_force(self): + self.become_admin() + vlan = factory.make_VLAN() + factory.make_Subnet(vlan=vlan) + rack = factory.make_RackController(vlan=vlan) + ip = factory.make_StaticIPAddress(interface=rack.interface_set.first()) + factory.make_Pod(ip_address=ip) + vlan.dhcp_on = True + vlan.primary_rack = rack + vlan.save() + mock_async_delete = self.patch(Pod, "async_delete") + response = self.client.delete(self.get_rack_uri(rack)) + self.assertEqual( + http.client.BAD_REQUEST, response.status_code, + explain_unexpected_response(http.client.BAD_REQUEST, response)) + self.assertThat(mock_async_delete, MockNotCalled()) + + def test_DELETE_force_not_required_for_pod_region_rack(self): + self.become_admin() + vlan = factory.make_VLAN() + factory.make_Subnet(vlan=vlan) + rack = factory.make_RegionRackController(vlan=vlan) + ip = factory.make_StaticIPAddress( + interface=rack.interface_set.first()) + factory.make_Pod(ip_address=ip) + mock_async_delete = self.patch(Pod, "async_delete") + response = self.client.delete( + self.get_rack_uri(rack), QUERY_STRING=urlencode({ + 'force': 'true' + }, doseq=True)) + self.assertEqual( + http.client.NO_CONTENT, response.status_code, + explain_unexpected_response(http.client.NO_CONTENT, response)) + self.assertThat(mock_async_delete, MockNotCalled()) class TestRackControllersAPI(APITestCase.ForUser): diff --git a/src/maasserver/api/tests/test_regioncontroller.py b/src/maasserver/api/tests/test_regioncontroller.py index f37d557..19c7993 100644 --- a/src/maasserver/api/tests/test_regioncontroller.py +++ b/src/maasserver/api/tests/test_regioncontroller.py @@ -4,12 +4,23 @@ """Tests for the Region Controller API.""" import http.client +from unittest.mock import call -from maasserver.testing.api import APITestCase +from django.utils.http import urlencode +from maasserver.enum import NODE_TYPE +from maasserver.models.bmc import Pod +from maasserver.testing.api import ( + APITestCase, + explain_unexpected_response, +) from maasserver.testing.factory import factory from maasserver.utils.converters import json_load_bytes from maasserver.utils.django_urls import reverse from maasserver.utils.orm import reload_object +from maastesting.matchers import ( + MockCallsMatch, + MockNotCalled, +) class TestRegionControllerAPI(APITestCase.ForUser): @@ -39,6 +50,59 @@ class TestRegionControllerAPI(APITestCase.ForUser): response = self.client.put(self.get_region_uri(region), {}) self.assertEqual(http.client.FORBIDDEN, response.status_code) + def test_DELETE_delete_with_force(self): + self.become_admin() + vlan = factory.make_VLAN() + subnet = factory.make_Subnet(vlan=vlan) + region = factory.make_Node_with_Interface_on_Subnet( + node_type=NODE_TYPE.REGION_CONTROLLER, subnet=subnet, vlan=vlan) + ip = factory.make_StaticIPAddress( + interface=region.interface_set.first()) + factory.make_Pod(ip_address=ip) + mock_async_delete = self.patch(Pod, "async_delete") + response = self.client.delete( + self.get_region_uri(region), QUERY_STRING=urlencode({ + 'force': 'true' + }, doseq=True)) + self.assertEqual( + http.client.NO_CONTENT, response.status_code, + explain_unexpected_response(http.client.NO_CONTENT, response)) + self.assertThat(mock_async_delete, MockCallsMatch(call())) + + def test_DELETE_force_not_required_for_pod_region_rack(self): + self.become_admin() + vlan = factory.make_VLAN() + factory.make_Subnet(vlan=vlan) + rack = factory.make_RegionRackController(vlan=vlan) + ip = factory.make_StaticIPAddress( + interface=rack.interface_set.first()) + factory.make_Pod(ip_address=ip) + mock_async_delete = self.patch(Pod, "async_delete") + response = self.client.delete( + self.get_region_uri(rack), QUERY_STRING=urlencode({ + 'force': 'true' + }, doseq=True)) + self.assertEqual( + http.client.NO_CONTENT, response.status_code, + explain_unexpected_response(http.client.NO_CONTENT, response)) + self.assertThat(mock_async_delete, MockNotCalled()) + + def test_pod_DELETE_delete_without_force(self): + self.become_admin() + vlan = factory.make_VLAN() + subnet = factory.make_Subnet(vlan=vlan) + region = factory.make_Node_with_Interface_on_Subnet( + node_type=NODE_TYPE.REGION_CONTROLLER, subnet=subnet, vlan=vlan) + ip = factory.make_StaticIPAddress( + interface=region.interface_set.first()) + factory.make_Pod(ip_address=ip) + mock_async_delete = self.patch(Pod, "async_delete") + response = self.client.delete(self.get_region_uri(region)) + self.assertEqual( + http.client.BAD_REQUEST, response.status_code, + explain_unexpected_response(http.client.BAD_REQUEST, response)) + self.assertThat(mock_async_delete, MockNotCalled()) + class TestRegionControllersAPI(APITestCase.ForUser): """Tests for /api/2.0/regioncontrollers/.""" diff --git a/src/maasserver/models/bmc.py b/src/maasserver/models/bmc.py index addc187..2b80690 100644 --- a/src/maasserver/models/bmc.py +++ b/src/maasserver/models/bmc.py @@ -1282,6 +1282,21 @@ class Pod(BMC): "Use `async_delete` instead. Deleting a Pod takes " "an asynchronous action.") + def delete_and_wait(self): + """Block the current thread while waiting for the pod to be deleted. + + This must not be called from a deferToDatabase thread; use the + async_delete() method instead. + """ + # Calculate the wait time based on the number of none pre-existing + # machines. We allow maximum of 60 seconds per machine plus 60 seconds + # for the pod. + pod = self.as_pod() + num_machines = Machine.objects.filter(bmc=pod) + num_machines = num_machines.exclude( + creation_type=NODE_CREATION_TYPE.PRE_EXISTING) + pod.async_delete().wait((num_machines.count() * 60) + 60) + @asynchronous def async_delete(self): """Delete a pod asynchronously. diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py index b431d10..9e0b087 100644 --- a/src/maasserver/models/node.py +++ b/src/maasserver/models/node.py @@ -235,12 +235,14 @@ from provisioningserver.utils.twisted import ( synchronous, undefined, ) +from twisted.internet import reactor from twisted.internet.defer import ( Deferred, inlineCallbacks, succeed, ) from twisted.internet.threads import deferToThread +from twisted.python.threadable import isInIOThread log = LegacyLogger() @@ -2362,7 +2364,7 @@ class Node(CleanSave, TimestampedModel): callOut, maaslog.warning, "%s: Could not stop node to abort " "deployment; it must be stopped manually", hostname) - def delete(self): + def delete(self, *args, **kwargs): """Delete this node.""" bmc = self.bmc if (self.node_type == NODE_TYPE.MACHINE and @@ -2414,7 +2416,7 @@ class Node(CleanSave, TimestampedModel): "%s: Deleting my BMC '%s'", self.hostname, self.bmc) self.bmc.delete() - super(Node, self).delete() + super(Node, self).delete(*args, **kwargs) def set_random_hostname(self): """Set a random `hostname`.""" @@ -2966,10 +2968,11 @@ class Node(CleanSave, TimestampedModel): OwnerData.objects.filter(node=self).delete() def release_or_erase( - self, user, comment=None, - erase=False, secure_erase=None, quick_erase=None): + self, user, comment=None, erase=False, secure_erase=None, + quick_erase=None, force=False): """Either release the node or erase the node then release it, depending on settings and parameters.""" + self.maybe_delete_pods(not force) erase_on_release = Config.objects.get_config( 'enable_disk_erasing_on_release') if erase or erase_on_release: @@ -2979,6 +2982,27 @@ class Node(CleanSave, TimestampedModel): else: self.release(user, comment) + def maybe_delete_pods(self, dry_run: bool): + """Check if any pods are associated with this Node. + + All pods will be deleted if dry_run=False is passed in. + + :param dry_run: If True, raises NodeActionError rather than deleting + pods. + """ + hosted_pods = list( + self.get_hosted_pods().values_list('name', flat=True)) + if len(hosted_pods) > 0: + if dry_run: + raise ValidationError( + "The following pods must be removed first: %s" % ( + ", ".join(hosted_pods))) + for pod in self.get_hosted_pods(): + if isInIOThread(): + pod.async_delete() + else: + reactor.callFromThread(pod.async_delete) + def set_netboot(self, on=True): """Set netboot on or off.""" log.debug( @@ -4397,6 +4421,13 @@ class Node(CleanSave, TimestampedModel): else: return script_result.stdout.decode('utf-8').splitlines() + def get_hosted_pods(self) -> QuerySet: + # Circular imports + from maasserver.models import Pod + our_static_ips = StaticIPAddress.objects.filter( + interface__node=self).values_list('ip') + return Pod.objects.filter(ip_address__ip__in=our_static_ips) + # Piston serializes objects based on the object class. # Here we define a proxy class so that we can specialize how devices are @@ -4413,6 +4444,17 @@ class Machine(Node): super(Machine, self).__init__( node_type=NODE_TYPE.MACHINE, *args, **kwargs) + def delete(self, force=False): + """Deletes this Machine. + + Before deletion, checks if any hosted pods exist. + + Raises ValidationError if the machine is a host for one or more pods, + and `force=True` was not specified. + """ + self.maybe_delete_pods(not force) + return super().delete() + class Controller(Node): """A node which is either a rack or region controller.""" @@ -5352,6 +5394,11 @@ class RackController(Controller): def delete(self, force=False): """Delete this rack controller.""" + # Don't bother with the pod check if this is a region+rack, because + # deleting a region+rack results in a region-only controller. + if self.node_type != NODE_TYPE.REGION_AND_RACK_CONTROLLER: + self.maybe_delete_pods(not force) + # Avoid circular imports from maasserver.models import RegionRackRPCConnection @@ -5518,8 +5565,9 @@ class RegionController(Controller): super(RegionController, self).__init__( node_type=NODE_TYPE.REGION_CONTROLLER, *args, **kwargs) - def delete(self): + def delete(self, force=False): """Delete this region controller.""" + self.maybe_delete_pods(not force) # Avoid circular dependency. from maasserver.models import RegionControllerProcess connections = RegionControllerProcess.objects.filter( diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py index 5b367ae..e596dd8 100644 --- a/src/maasserver/models/tests/test_node.py +++ b/src/maasserver/models/tests/test_node.py @@ -34,6 +34,7 @@ from django.core.exceptions import ( ) from django.db import transaction from django.db.models.deletion import Collector +from django.db.models.query import QuerySet from fixtures import LoggerFixture from maasserver import ( bootresources, @@ -10844,3 +10845,18 @@ class TestControllerGetDiscoveryState(MAASServerTestCase): self.assertThat(monitoring_state, Contains('eth2')) self.assertThat( monitoring_state['eth1'], Equals(eth1.get_discovery_state())) + + +class TestNodeGetHostedPods(MAASServerTestCase): + + def test__returns_queryset(self): + node = factory.make_Node() + pods = node.get_hosted_pods() + self.assertThat(pods, IsInstance(QuerySet)) + + def test__returns_related_pods(self): + node = factory.make_Node_with_Interface_on_Subnet() + ip = factory.make_StaticIPAddress(interface=node.boot_interface) + pod = factory.make_Pod(ip_address=ip) + pods = node.get_hosted_pods() + self.assertThat(pods, Contains(pod))