Merge lp:~julian-edwards/maas/deallocate-static-ip into lp:~maas-committers/maas/trunk

[Julian Edwards]

Add StaticIPAddress.deallocate(), StaticIPAddressManager.deallocate_by_node() and improve StaticIPAddress.__unicode__ to show its type.

[Julian Edwards (julian-edwards)]

So damn trivial I am going to selfie this one.

review: Approve

[Julian Edwards (julian-edwards)]

Since the lander is borked and canonistack is not responding, I'm going to do some more work in this branch.

[Julian Edwards (julian-edwards)]

review: Abstain

[Raphaël Badin (rvb)]

Looks good. Couple of inline comments though.

review: Approve

[Julian Edwards (julian-edwards)]

Thanks for the review! I replied inline, and implemented some of your suggestions or explained why I rejected the others.

Cheers.

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'src/maasserver/models/staticipaddress.py'
--- src/maasserver/models/staticipaddress.py	2014-06-03 00:35:04 +0000
+++ src/maasserver/models/staticipaddress.py	2014-06-05 01:29:18 +0000
@@ -87,6 +87,13 @@
         ipaddress.save()
         return ipaddress
 
+    def deallocate_by_node(self, node):
+        """Given a node, deallocate all of its AUTO StaticIPAddresses."""
+        qs = self.filter(
+            alloc_type=IPADDRESS_TYPE.AUTO).filter(
+            macaddress__node=node)
+        qs.delete()
+
 
 class StaticIPAddress(CleanSave, TimestampedModel):
 
@@ -97,8 +104,10 @@
     ip = GenericIPAddressField(
         unique=True, null=False, editable=False, blank=False)
 
-    # The MACIPAddressLink table is used to link IPAddress to
-    # MACAddress.  See MACAddress.ip_addresses.
+    # The MACStaticIPAddressLink table is used to link StaticIPAddress to
+    # MACAddress.  See MACAddress.ip_addresses, and the reverse relation
+    # self.macaddress_set (which will only ever contain one MAC due to
+    # the unique FK restriction on the link table).
 
     alloc_type = IntegerField(
         editable=False, null=False, blank=False, default=IPADDRESS_TYPE.AUTO)
@@ -106,4 +115,24 @@
     objects = StaticIPAddressManager()
 
     def __unicode__(self):
-        return "<IPAddress %s>" % self.ip
+        # Attempt to show the symbolic alloc_type name if possible.
+
+        # __iter__ does not work here for some reason, so using
+        # iteritems().
+        # XXX: convert this into a reverse_map_enum in maasserver.utils.
+        for k,v in IPADDRESS_TYPE.__dict__.iteritems():
+            if v == self.alloc_type:
+                strtype = k
+                break
+        else:
+            # Should never get here, but defensive coding FTW.
+            strtype = "%s" % self.alloc_type
+        return "<StaticIPAddress: <%s:type=%s>>" % (self.ip, strtype)
+
+    def deallocate(self):
+        """Mark this IP address as no longer in use.
+
+        After return, this object is no longer valid.
+        """
+        self.delete()
+

=== modified file 'src/maasserver/models/tests/test_staticipaddress.py'
--- src/maasserver/models/tests/test_staticipaddress.py	2014-06-03 00:35:04 +0000
+++ src/maasserver/models/tests/test_staticipaddress.py	2014-06-05 01:29:18 +0000
@@ -16,12 +16,14 @@
 
 
 from django.core.exceptions import ValidationError
+from maasserver.enum import IPADDRESS_TYPE
 from maasserver.models.staticipaddress import (
     StaticIPAddress,
     StaticIPAddressExhaustion,
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
+from maasserver.utils import map_enum
 from netaddr import (
     IPAddress,
     IPRange,
@@ -44,9 +46,56 @@
             StaticIPAddressExhaustion,
             StaticIPAddress.objects.allocate_new, low, high)
 
+    def test_deallocate_by_node_removes_addresses(self):
+        node = factory.make_node()
+        [mac1, mac2] = [
+            factory.make_mac_address(node=node) for _ in range(2)]
+        factory.make_staticipaddress(mac=mac1)
+        factory.make_staticipaddress(mac=mac2)
+        StaticIPAddress.objects.deallocate_by_node(node)
+        # Check the DB is cleared.
+        self.assertEqual([], list(StaticIPAddress.objects.all()))
+        # Check the link table is cleared.
+        self.assertEqual([], list(node.static_ip_addresses()))
+
+    def test_deallocate_by_node_ignores_other_nodes(self):
+        node1 = factory.make_node()
+        mac1 = factory.make_mac_address(node=node1)
+        factory.make_staticipaddress(mac=mac1)
+        node2 = factory.make_node()
+        mac2 = factory.make_mac_address(node=node2)
+        ip2 = factory.make_staticipaddress(mac=mac2)
+
+        StaticIPAddress.objects.deallocate_by_node(node1)
+        self.assertItemsEqual([ip2.ip], node2.static_ip_addresses())
+
+    def test_deallocate_only_deletes_auto_types(self):
+        node = factory.make_node()
+        mac = factory.make_mac_address(node=node)
+        alloc_types = map_enum(IPADDRESS_TYPE).values()
+        for alloc_type in alloc_types:
+            factory.make_staticipaddress(mac=mac, alloc_type=alloc_type)
+
+        StaticIPAddress.objects.deallocate_by_node(node)
+        types_without_auto = set(alloc_types)
+        types_without_auto.discard(IPADDRESS_TYPE.AUTO)
+        self.assertItemsEqual(
+            types_without_auto,
+            [ip.alloc_type for ip in StaticIPAddress.objects.all()])
+
 
 class StaticIPAddressTest(MAASServerTestCase):
 
+    def test_repr_with_valid_type(self):
+        actual = "%s" % factory.make_staticipaddress(
+            ip="10.0.0.1", alloc_type=IPADDRESS_TYPE.AUTO)
+        self.assertEqual("<StaticIPAddress: <10.0.0.1:type=AUTO>>", actual)
+
+    def test_repr_with_invalid_type(self):
+        actual = "%s" % factory.make_staticipaddress(
+            ip="10.0.0.1", alloc_type=99999)
+        self.assertEqual("<StaticIPAddress: <10.0.0.1:type=99999>>", actual)
+
     def test_stores_to_database(self):
         ipaddress = factory.make_staticipaddress()
         self.assertEqual([ipaddress], list(StaticIPAddress.objects.all()))
@@ -54,3 +103,14 @@
     def test_invalid_address_raises_validation_error(self):
         ip = StaticIPAddress(ip='256.0.0.0.0')
         self.assertRaises(ValidationError, ip.full_clean)
+
+    def test_deallocate_removes_object(self):
+        ipaddress = factory.make_staticipaddress()
+        ipaddress.deallocate()
+        self.assertEqual([], list(StaticIPAddress.objects.all()))
+
+    def test_deallocate_ignores_other_objects(self):
+        ipaddress = factory.make_staticipaddress()
+        ipaddress2 = factory.make_staticipaddress()
+        ipaddress.deallocate()
+        self.assertEqual([ipaddress2], list(StaticIPAddress.objects.all()))