Merge lp:~jtv/maas/mac-claim_static_ips-plural into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

With IPv6, MACAddress.claim_static_ip can allocate more than one address, so rename it to claim_static_ips and make it return a list.

This does not make the actual change where a MACAddress can be connected to multiple cluster interfaces, and so allocate multiple static addresses. I'll leave that to a separate branch, although a helper for identifying those cluster interfaces is already up for review.

On the other hand, the branch does take care of the propagated implications: we no longer do things with the address if we get one, we do things for every address in the list.

Jeroen

[Graham Binns (gmb)]

review: Approve

[Julian Edwards (julian-edwards)]

I see Graham already approved, but I have some suggestions for improvements inline!

=== modified file 'src/maasserver/api/api.py'
--- src/maasserver/api/api.py	2014-08-18 04:41:45 +0000
+++ src/maasserver/api/api.py	2014-08-18 09:56:11 +0000
@@ -474,7 +474,7 @@
             assign the sticky IP address.  If not passed, defaults to the
             primary MAC for the node.
         :param requested_address: Optional IP address to claim.  Must be in
-            the range defined on the cluster interface to which the context
+            the range defined on a cluster interface to which the context
             MAC is related, or 403 Forbidden is returned.  If the requested
             address is unavailable for use, 404 Not Found is returned.
 
@@ -500,11 +500,12 @@
                 raise MAASAPIBadRequest(
                     "mac_address %s not found on the node" % raw_mac)
         requested_address = request.POST.get('requested_address', None)
-        sticky_ip = mac_address.claim_static_ip(
+        sticky_ips = mac_address.claim_static_ips(
             alloc_type=IPADDRESS_TYPE.STICKY,
             requested_address=requested_address)
         maaslog.info(
-            "%s: Sticky IP address %s allocated", node.hostname, sticky_ip.ip)
+            "%s: Sticky IP address(es) allocated: %s", node.hostname,
+            ', '.join(allocation.ip for allocation in sticky_ips))
         return node
 
     @operation(idempotent=False)
@@ -1966,9 +1967,9 @@
     def reserve(self, request):
         """Reserve an IP address for use outside of MAAS.
 
-        Returns an IP adddress for which MAAS will not allow any of its
-        known devices and Nodes to use; it is free for use by the requesting
-        user until released by the user.
+        Returns an IP adddress which MAAS will not allow any of its known
+        devices and Nodes to use; it is free for use by the requesting user
+        until released by the user.
 
         :param network: CIDR representation of the network on which the IP
             reservation is required. e.g. 10.1.2.0/24

=== modified file 'src/maasserver/api/tests/test_node.py'
--- src/maasserver/api/tests/test_node.py	2014-08-18 04:41:45 +0000
+++ src/maasserver/api/tests/test_node.py	2014-08-18 09:56:11 +0000
@@ -1011,7 +1011,7 @@
     def test_claim_sticky_ip_address_returns_existing_if_already_exists(self):
         self.become_admin()
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
-        existing_ip = node.get_primary_mac().claim_static_ip(
+        [existing_ip] = node.get_primary_mac().claim_static_ips(
             alloc_type=IPADDRESS_TYPE.STICKY)
         response = self.client.post(
             self.get_node_uri(node), {'op': 'claim_sticky_ip_address'})
@@ -1029,7 +1029,7 @@
         random_alloc_type = factory.pick_enum(
             IPADDRESS_TYPE,
             but_not=[IPADDRESS_TYPE.STICKY, IPADDRESS_TYPE.USER_RESERVED])
-        node.get_primary_mac().claim_static_ip(alloc_type=random_alloc_type)
+        node.get_primary_mac().claim_static_ips(alloc_type=random_alloc_type)
         response = self.client.post(
             self.get_node_uri(node), {'op': 'claim_sticky_ip_address'})
         self.assertEqual(
@@ -1128,7 +1128,7 @@
         # Allocate an IP to one of the nodes.
         requested_address = IPAddress(ngi.static_ip_range_low) + 1
         requested_address = requested_address.format()
-        other_node.get_primary_mac().claim_static_ip(
+        other_node.get_primary_mac().claim_static_ips(
             requested_address=requested_address)
 
         # Use the API to try to duplicate the same IP on the other node.

=== modified file 'src/maasserver/models/macaddress.py'
--- src/maasserver/models/macaddress.py	2014-08-15 22:48:42 +0000
+++ src/maasserver/models/macaddress.py	2014-08-18 09:56:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """MACAddress model and friends."""
@@ -89,9 +89,13 @@
         """Return networks to which this MAC is connected, sorted by name."""
         return self.networks.all().order_by('name')
 
-    def claim_static_ip(self, alloc_type=IPADDRESS_TYPE.AUTO,
-                        requested_address=None):
-        """Assign a static IP to this MAC.
+    def claim_static_ips(self, alloc_type=IPADDRESS_TYPE.AUTO,
+                         requested_address=None):
+        """Assign static IP addresses to this MAC.
+
+        Allocates one address per managed cluster interface connected to this
+        MAC.  Typically this will be either just one IPv4 address, or an IPv4
+        address and an IPv6 address.
 
         It is the caller's responsibility to create a celery Task that will
         write the dhcp host.  It is not done here because celery doesn't
@@ -101,13 +105,14 @@
         :param alloc_type: See :class:`StaticIPAddress`.alloc_type.
             This parameter musn't be IPADDRESS_TYPE.USER_RESERVED.
         :param requested_address: Optional IP address to claim.  Must be in
-            the range defined on the cluster interface to which this MACAddress
-            is related.
-        :return: A :class:`StaticIPAddress` object. Returns None if
+            the range defined on a cluster interface to which this MACAddress
+            is related.  If given, no allocations will be made on any other
+            cluster interfaces the MAC may be connected to.
+        :return: A list of :class:`StaticIPAddress`.  Returns empty if
             the cluster_interface is not yet known, or the
             static_ip_range_low/high values values are not set on the
-            cluster_interface. If an IP already exists for this type, it
-            is always returned with no further allocation.
+            cluster_interface.  If an IP address was already allocated, the
+            function will return it rather than allocate a new one.
         :raises: StaticIPAddressExhaustion if there are not enough IPs left.
         :raises: StaticIPAddressTypeClash if an IP already exists with a
             different type.
@@ -129,20 +134,20 @@
 
         # Check to see if an IP with the same type already exists.
         try:
-            return StaticIPAddress.objects.get(
-                macaddress=self, alloc_type=alloc_type)
+            return [StaticIPAddress.objects.get(
+                macaddress=self, alloc_type=alloc_type)]
         except StaticIPAddress.DoesNotExist:
             pass
 
         if self.cluster_interface is None:
             # We need to know this to allocate an IP, so return nothing.
-            return None
+            return []
 
         low = self.cluster_interface.static_ip_range_low
         high = self.cluster_interface.static_ip_range_high
         if not low or not high:
             # low/high can be None or blank if not defined yet.
-            return None
+            return []
 
         # Avoid circular imports.
         from maasserver.models import (
@@ -152,4 +157,4 @@
         sip = StaticIPAddress.objects.allocate_new(
             low, high, alloc_type, requested_address=requested_address)
         MACStaticIPAddressLink(mac_address=self, ip_address=sip).save()
-        return sip
+        return [sip]

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-08-18 04:41:45 +0000
+++ src/maasserver/models/node.py	2014-08-18 09:56:11 +0000
@@ -701,7 +701,7 @@
         macs = self.mac_addresses_on_managed_interfaces()
         for mac in macs:
             try:
-                sip = mac.claim_static_ip()
+                sips = mac.claim_static_ips()
             except StaticIPAddressTypeClash:
                 # There's already a non-AUTO IP, so nothing to do.
                 continue
@@ -709,7 +709,7 @@
             # defined, which will be the case when migrating from older
             # versions of the code.  If it is None we just ignore this
             # MAC.
-            if sip is not None:
+            for sip in sips:
                 tasks.append(self._create_hostmap_task(mac, sip))
                 maaslog.info(
                     "%s: Claimed static IP %s on %s", self.hostname,

=== modified file 'src/maasserver/models/tests/test_macaddress.py'
--- src/maasserver/models/tests/test_macaddress.py	2014-08-13 21:49:35 +0000
+++ src/maasserver/models/tests/test_macaddress.py	2014-08-18 09:56:11 +0000
@@ -85,55 +85,56 @@
 
 class TestMACAddressForStaticIPClaiming(MAASServerTestCase):
 
-    def test_claim_static_ip_returns_none_if_no_cluster_interface(self):
+    def test_claim_statics_ip_returns_empty_if_no_cluster_interface(self):
         # If mac.cluster_interface is None, we can't allocate any IP.
         mac = factory.make_mac_address()
-        self.assertIsNone(mac.claim_static_ip())
+        self.assertEquals([], mac.claim_static_ips())
 
-    def test_claim_static_ip_reserves_an_ip_address(self):
+    def test_claim_static_ips_reserves_an_ip_address(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
-        claimed_ip = mac.claim_static_ip()
+        [claimed_ip] = mac.claim_static_ips()
         self.assertIsInstance(claimed_ip, StaticIPAddress)
         self.assertNotEqual([], list(node.static_ip_addresses()))
         self.assertEqual(
             IPADDRESS_TYPE.AUTO, StaticIPAddress.objects.all()[0].alloc_type)
 
-    def test_claim_static_ip_sets_type_as_required(self):
+    def test_claim_static_ips_sets_type_as_required(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
-        claimed_ip = mac.claim_static_ip(alloc_type=IPADDRESS_TYPE.STICKY)
+        [claimed_ip] = mac.claim_static_ips(alloc_type=IPADDRESS_TYPE.STICKY)
         self.assertEqual(IPADDRESS_TYPE.STICKY, claimed_ip.alloc_type)
 
-    def test_claim_static_ip_returns_none_if_no_static_range_defined(self):
+    def test_claim_static_ips_returns_none_if_no_static_range_defined(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
         mac.cluster_interface.static_ip_range_low = None
         mac.cluster_interface.static_ip_range_high = None
-        self.assertIsNone(mac.claim_static_ip())
+        self.assertEqual([], mac.claim_static_ips())
 
-    def test_claim_static_ip_raises_if_clashing_type(self):
+    def test_claim_static_ips_raises_if_clashing_type(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
         iptype = factory.pick_enum(
             IPADDRESS_TYPE, but_not=[IPADDRESS_TYPE.USER_RESERVED])
         iptype2 = factory.pick_enum(IPADDRESS_TYPE, but_not=[iptype])
-        mac.claim_static_ip(alloc_type=iptype)
+        mac.claim_static_ips(alloc_type=iptype)
         self.assertRaises(
             StaticIPAddressTypeClash,
-            mac.claim_static_ip, alloc_type=iptype2)
+            mac.claim_static_ips, alloc_type=iptype2)
 
-    def test_claim_static_ip_returns_existing_if_claiming_same_type(self):
+    def test_claim_static_ips_returns_existing_if_claiming_same_type(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
         iptype = factory.pick_enum(
             IPADDRESS_TYPE, but_not=[IPADDRESS_TYPE.USER_RESERVED])
-        ip = mac.claim_static_ip(alloc_type=iptype)
+        [ip] = mac.claim_static_ips(alloc_type=iptype)
         self.assertEqual(
-            ip, mac.claim_static_ip(alloc_type=iptype))
+            [ip], mac.claim_static_ips(alloc_type=iptype))
 
     def test_passes_requested_ip(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         mac = node.get_primary_mac()
         ip = node.get_primary_mac().cluster_interface.static_ip_range_high
-        self.assertEqual(ip, mac.claim_static_ip(requested_address=ip).ip)
+        [allocation] = mac.claim_static_ips(requested_address=ip)
+        self.assertEqual(ip, allocation.ip)

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-08-18 04:41:45 +0000
+++ src/maasserver/models/tests/test_node.py	2014-08-18 09:56:11 +0000
@@ -326,7 +326,7 @@
         primary_mac = node.get_primary_mac()
         random_alloc_type = factory.pick_enum(
             IPADDRESS_TYPE, but_not=[IPADDRESS_TYPE.USER_RESERVED])
-        primary_mac.claim_static_ip(alloc_type=random_alloc_type)
+        primary_mac.claim_static_ips(alloc_type=random_alloc_type)
         node.delete()
         self.assertItemsEqual([], StaticIPAddress.objects.all())
 
@@ -335,7 +335,7 @@
         self.patch(Omshell, 'remove')
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         primary_mac = node.get_primary_mac()
-        primary_mac.claim_static_ip(alloc_type=IPADDRESS_TYPE.STICKY)
+        primary_mac.claim_static_ips(alloc_type=IPADDRESS_TYPE.STICKY)
         node.delete()
         self.assertEqual(
             ['provisioningserver.tasks.remove_dhcp_host_map'],
@@ -649,17 +649,17 @@
         user = factory.make_user()
         node = factory.make_node_with_mac_attached_to_nodegroupinterface(
             owner=user, status=NODE_STATUS.ALLOCATED)
-        sip = node.get_primary_mac().claim_static_ip()
+        sips = node.get_primary_mac().claim_static_ips()
         delete_static_host_maps = self.patch(node, 'delete_static_host_maps')
         node.release()
-        expected = [sip.ip.format()]
+        expected = [sip.ip.format() for sip in sips]
         self.assertThat(delete_static_host_maps, MockCalledOnceWith(expected))
 
     def test_delete_static_host_maps(self):
         user = factory.make_user()
         node = factory.make_node_with_mac_attached_to_nodegroupinterface(
             owner=user, status=NODE_STATUS.ALLOCATED)
-        sip = node.get_primary_mac().claim_static_ip()
+        [sip] = node.get_primary_mac().claim_static_ips()
         self.patch(Omshell, 'remove')
         set_call = self.patch(celery.canvas.Signature, 'set')
         node.delete_static_host_maps([sip.ip.format()])
@@ -1476,7 +1476,7 @@
                 self.celery.tasks[0]['kwargs']['mac_address'],
             ))
 
-    def test_start_nodes_does_not_claim_static_ip_unless_allocated(self):
+    def test_start_nodes_does_not_claim_static_ips_unless_allocated(self):
         user = factory.make_user()
         node = factory.make_node_with_mac_attached_to_nodegroupinterface(
             owner=user, power_type='ether_wake')
@@ -1747,7 +1747,7 @@
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         self.patch(
             MACAddress,
-            'claim_static_ip').side_effect = StaticIPAddressExhaustion
+            'claim_static_ips').side_effect = StaticIPAddressExhaustion
         deallocate_call = self.patch(
             StaticIPAddressManager, 'deallocate_by_node')
         self.assertRaises(StaticIPAddressExhaustion, node.claim_static_ips)
@@ -1772,7 +1772,7 @@
     def test_claim_static_ips_creates_no_tasks_if_existing_IP(self):
         node = factory.make_node_with_mac_attached_to_nodegroupinterface()
         primary_mac = node.get_primary_mac()
-        primary_mac.claim_static_ip(alloc_type=IPADDRESS_TYPE.STICKY)
+        primary_mac.claim_static_ips(alloc_type=IPADDRESS_TYPE.STICKY)
         ngi = factory.make_node_group_interface(
             node.nodegroup, management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
         second_mac = factory.make_mac_address(node=node, cluster_interface=ngi)