Merge lp:~blake-rouse/maas/fix-1403909 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Don't remove static ip address assignment from node until it has been completely powered off.

[Raphaël Badin (rvb)]

Looks good!

review: Approve

[Blake Rouse (blake-rouse)]

Thanks for the review.

I will wait for the CI to be back online to test this before it lands.

[Blake Rouse (blake-rouse)]

Passed CI.

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-12-15 08:07:20 +0000
+++ src/maasserver/models/node.py	2014-12-19 23:02:23 +0000
@@ -1228,6 +1228,7 @@
                 unicode(ex))
             raise
 
+        deallocate_ip_address = True
         if self.power_state == POWER_STATE.OFF:
             # Node is already off.
             self.status = NODE_STATUS.READY
@@ -1235,7 +1236,9 @@
         elif self.get_effective_power_info().can_be_queried:
             # Controlled power type (one for which we can query the power
             # state): update_power_state() will take care of making the node
-            # READY and unowned when the power is finally off.
+            # READY, remove the owned, and deallocate the assigned static ip
+            # address when the power is finally off.
+            deallocate_ip_address = False
             self.status = NODE_STATUS.RELEASING
             self.start_transition_monitor(self.get_releasing_time())
         else:
@@ -1259,10 +1262,8 @@
 
         # Do these after updating the node to avoid creating deadlocks with
         # other node editing operations.
-        deallocated_ips = StaticIPAddress.objects.deallocate_by_node(self)
-        self.delete_host_maps(deallocated_ips)
-        from maasserver.dns.config import change_dns_zones
-        change_dns_zones([self.nodegroup])
+        if deallocate_ip_address:
+            self.deallocate_static_ip_addresses()
 
     def release_or_erase(self):
         """Either release the node or erase the node then release it, depending
@@ -1365,6 +1366,7 @@
             self.status = NODE_STATUS.READY
             self.owner = None
             self.stop_transition_monitor()
+            self.deallocate_static_ip_addresses()
         self.save()
 
     def claim_static_ip_addresses(self):
@@ -1388,6 +1390,17 @@
         # because it's all-or-nothing (hence the atomic context).
         return [(static_ip.ip, unicode(mac)) for static_ip in static_ips]
 
+    def deallocate_static_ip_addresses(self):
+        """Release the `StaticIPAddress` that is assigned to this node and
+        remove the host mapping on the cluster.
+
+        This should only be done when the node is in an unused state.
+        """
+        deallocated_ips = StaticIPAddress.objects.deallocate_by_node(self)
+        self.delete_host_maps(deallocated_ips)
+        from maasserver.dns.config import change_dns_zones
+        change_dns_zones([self.nodegroup])
+
     def get_boot_purpose(self):
         """
         Return a suitable "purpose" for this boot, e.g. "install".
 
=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-12-15 08:07:20 +0000
+++ src/maasserver/models/tests/test_node.py	2014-12-19 23:02:23 +0000
@@ -55,10 +55,7 @@
     PowerInfo,
     validate_hostname,
     )
-from maasserver.models.staticipaddress import (
-    StaticIPAddress,
-    StaticIPAddressManager,
-    )
+from maasserver.models.staticipaddress import StaticIPAddress
 from maasserver.models.user import create_auth_token
 from maasserver.node_status import (
     get_failed_status,
@@ -920,20 +917,6 @@
         self.expectThat(node.distro_series, Equals(''))
         self.expectThat(node.license_key, Equals(''))
 
-    def test_release_deletes_static_ip_host_maps(self):
-        remove_host_maps = self.patch_autospec(
-            node_module, "remove_host_maps")
-        user = factory.make_User()
-        node = factory.make_node_with_mac_attached_to_nodegroupinterface(
-            owner=user, status=NODE_STATUS.ALLOCATED)
-        self.patch(node, 'start_transition_monitor')
-        sips = node.get_primary_mac().claim_static_ips()
-        node.release()
-        expected = {sip.ip.format() for sip in sips}
-        self.assertThat(
-            remove_host_maps, MockCalledOnceWith(
-                {node.nodegroup: expected}))
-
     def test_release_clears_installation_results(self):
         agent_name = factory.make_name('agent-name')
         owner = factory.make_User()
@@ -1172,17 +1155,57 @@
         self.assertThat(
             node_stop, MockCalledOnceWith(user))
 
-    def test_release_deallocates_static_ips(self):
-        deallocate = self.patch(StaticIPAddressManager, 'deallocate_by_node')
-        deallocate.return_value = set()
-        node = factory.make_Node(
-            status=NODE_STATUS.ALLOCATED, owner=factory.make_User(),
-            power_type='ether_wake')
-        node.release()
-        self.assertThat(deallocate, MockCalledOnceWith(node))
-
-    def test_release_updates_dns(self):
-        self.patch(node_module, 'wait_for_power_command')
+    def test_release_deallocates_static_ip_when_node_is_off(self):
+        user = factory.make_User()
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface(
+            owner=user, status=NODE_STATUS.ALLOCATED,
+            power_state=POWER_STATE.OFF)
+        deallocate_static_ip_addresses = self.patch_autospec(
+            node, "deallocate_static_ip_addresses")
+        self.patch(node, 'start_transition_monitor')
+        node.release()
+        self.assertThat(
+            deallocate_static_ip_addresses, MockCalledOnceWith())
+
+    def test_release_deallocates_static_ip_when_node_cannot_be_queried(self):
+        user = factory.make_User()
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface(
+            owner=user, status=NODE_STATUS.ALLOCATED,
+            power_state=POWER_STATE.ON, power_type='ether_wake')
+        deallocate_static_ip_addresses = self.patch_autospec(
+            node, "deallocate_static_ip_addresses")
+        self.patch(node, 'start_transition_monitor')
+        node.release()
+        self.assertThat(
+            deallocate_static_ip_addresses, MockCalledOnceWith())
+
+    def test_release_doesnt_deallocate_static_ip_when_node_releasing(self):
+        user = factory.make_User()
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface(
+            owner=user, status=NODE_STATUS.ALLOCATED,
+            power_state=POWER_STATE.ON, power_type='virsh')
+        deallocate_static_ip_addresses = self.patch_autospec(
+            node, "deallocate_static_ip_addresses")
+        self.patch_autospec(node, 'stop')
+        self.patch(node, 'start_transition_monitor')
+        node.release()
+        self.assertThat(
+            deallocate_static_ip_addresses, MockNotCalled())
+
+    def test_deallocate_static_ip_deletes_static_ip_host_maps(self):
+        remove_host_maps = self.patch_autospec(
+            node_module, "remove_host_maps")
+        user = factory.make_User()
+        node = factory.make_node_with_mac_attached_to_nodegroupinterface(
+            owner=user, status=NODE_STATUS.ALLOCATED)
+        sips = node.get_primary_mac().claim_static_ips()
+        node.release()
+        expected = {sip.ip.format() for sip in sips}
+        self.assertThat(
+            remove_host_maps, MockCalledOnceWith(
+                {node.nodegroup: expected}))
+
+    def test_deallocate_static_ip_updates_dns(self):
         change_dns_zones = self.patch(dns_config, 'change_dns_zones')
         nodegroup = factory.make_NodeGroup(
             management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
@@ -1701,6 +1724,24 @@
         node.update_power_state(POWER_STATE.ON)
         self.expectThat(node.status, Equals(NODE_STATUS.ALLOCATED))
 
+    def test_update_power_state_deallocates_static_ips_if_releasing(self):
+        node = factory.make_Node(
+            power_state=POWER_STATE.ON, status=NODE_STATUS.RELEASING,
+            owner=None)
+        deallocate_static_ip_addresses = self.patch_autospec(
+            node, 'deallocate_static_ip_addresses')
+        self.patch(node, 'stop_transition_monitor')
+        node.update_power_state(POWER_STATE.OFF)
+        self.assertThat(deallocate_static_ip_addresses, MockCalledOnceWith())
+
+    def test_update_power_state_doesnt_deallocates_static_ips_if_not_off(self):
+        node = factory.make_Node(
+            power_state=POWER_STATE.OFF, status=NODE_STATUS.ALLOCATED)
+        deallocate_static_ip_addresses = self.patch_autospec(
+            node, 'deallocate_static_ip_addresses')
+        node.update_power_state(POWER_STATE.ON)
+        self.assertThat(deallocate_static_ip_addresses, MockNotCalled())
+
     def test_end_deployment_changes_state(self):
         node = factory.make_Node(status=NODE_STATUS.DEPLOYING)
         node.end_deployment()