launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/omshell-delete-bug-1064801 into lp:maas.

Commit message:
Ensure that relevant DHCP host maps are deleted when a node is deleted.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1064801 in MAAS: "remove_dhcp_host_map() not called when a node is deleted"
  https://bugs.launchpad.net/maas/+bug/1064801

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/omshell-delete-bug-1064801/+merge/129078
-- 
https://code.launchpad.net/~julian-edwards/maas/omshell-delete-bug-1064801/+merge/129078
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/omshell-delete-bug-1064801 into lp:maas.

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-10-10 09:04:11 +0000
+++ src/maasserver/models/node.py	2012-10-11 01:23:20 +0000
@@ -60,10 +60,14 @@
     )
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.config import Config
+from maasserver.models.dhcplease import DHCPLease
 from maasserver.models.tag import Tag
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.utils import get_db_state
-from maasserver.utils.orm import get_first
+from maasserver.utils.orm import (
+    get_first,
+    get_one,
+    )
 from piston.models import Token
 from provisioningserver.enum import (
     POWER_TYPE,
@@ -72,6 +76,7 @@
 from provisioningserver.tasks import (
     power_off,
     power_on,
+    remove_dhcp_host_map,
     )
 
 
@@ -596,13 +601,27 @@
             [self.system_id], user, user_data=commissioning_user_data)
 
     def delete(self):
-        # Delete the related mac addresses first.
-        self.macaddress_set.all().delete()
         # Allocated nodes can't be deleted.
         if self.status == NODE_STATUS.ALLOCATED:
             raise NodeStateViolation(
                 "Cannot delete node %s: node is in state %s."
                 % (self.system_id, NODE_STATUS_CHOICES_DICT[self.status]))
+        nodegroup = self.nodegroup
+        if nodegroup.get_managed_interface() is not None:
+            # Delete the host map(s) in the DHCP server.
+            macs = self.macaddress_set.values_list('mac_address', flat=True)
+            leases = DHCPLease.objects.filter(
+                mac__in=macs, nodegroup=nodegroup)
+            for lease in leases:
+                task_kwargs = dict(
+                    ip_address=lease.ip,
+                    server_address="127.0.0.1",
+                    omapi_key=nodegroup.dhcp_key)
+                remove_dhcp_host_map.apply_async(
+                    queue=nodegroup.uuid, kwargs=task_kwargs)
+        # Delete the related mac addresses.
+        self.macaddress_set.all().delete()
+
         super(Node, self).delete()
 
     def set_mac_based_hostname(self, mac_address):

=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py	2012-10-01 17:54:16 +0000
+++ src/maasserver/tests/test_dns.py	2012-10-11 01:23:20 +0000
@@ -28,6 +28,7 @@
     NODEGROUP_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
     )
+from maasserver.models import node as node_module
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maastesting.bindfixture import BINDServer
@@ -323,6 +324,8 @@
     def test_delete_node_updates_zone(self):
         self.patch(settings, "DNS_CONNECT", True)
         nodegroup, node, lease = self.create_nodegroup_with_lease()
+        # Prevent omshell task dispatch.
+        self.patch(node_module, "remove_dhcp_host_map")
         node.delete()
         fqdn = "%s.%s" % (node.hostname, nodegroup.name)
         self.assertEqual([''], self.dig_resolve(fqdn))

=== modified file 'src/maasserver/tests/test_node.py'
--- src/maasserver/tests/test_node.py	2012-10-10 09:41:48 +0000
+++ src/maasserver/tests/test_node.py	2012-10-11 01:23:20 +0000
@@ -49,7 +49,11 @@
     )
 from provisioningserver.enum import POWER_TYPE
 from provisioningserver.power.poweraction import PowerAction
-from testtools.matchers import FileContains
+from testtools.matchers import (
+    Equals,
+    FileContains,
+    MatchesListwise,
+    )
 
 
 class NodeTest(TestCase):
@@ -147,6 +151,35 @@
         node = factory.make_node(status=NODE_STATUS.ALLOCATED)
         self.assertRaises(NodeStateViolation, node.delete)
 
+    def test_delete_node_also_deletes_dhcp_host_map(self):
+        lease = factory.make_dhcp_lease()
+        node = factory.make_node(nodegroup=lease.nodegroup)
+        node.add_mac_address(lease.mac)
+        mocked_task = self.patch(node_module, "remove_dhcp_host_map")
+        mocked_apply_async = self.patch(mocked_task, "apply_async")
+        node.delete()
+        args, kwargs = mocked_apply_async.call_args
+        expected = (
+            Equals(kwargs['queue']),
+            Equals({
+                'ip_address': lease.ip,
+                'server_address': "127.0.0.1",
+                'omapi_key': lease.nodegroup.dhcp_key,
+                }))
+        observed = node.work_queue, kwargs['kwargs']
+        self.assertThat(observed, MatchesListwise(expected))
+
+    def test_delete_node_removes_multiple_host_maps(self):
+        lease1 = factory.make_dhcp_lease()
+        lease2 = factory.make_dhcp_lease(nodegroup=lease1.nodegroup)
+        node = factory.make_node(nodegroup=lease1.nodegroup)
+        node.add_mac_address(lease1.mac)
+        node.add_mac_address(lease2.mac)
+        mocked_task = self.patch(node_module, "remove_dhcp_host_map")
+        mocked_apply_async = self.patch(mocked_task, "apply_async")
+        node.delete()
+        self.assertEqual(2, mocked_apply_async.call_count)
+
     def test_set_mac_based_hostname_default_enlistment_domain(self):
         # The enlistment domain defaults to `local`.
         node = factory.make_node()