Merge lp:~rvb/maas/connect-mac-when-parsing-leases into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Create MAC->cluster interface connection when parsing leases. This is so that one can "claim and IP address" without providing an IP address: if the device has DHCPed off MAAS: MAAS will know from which cluster interface the IP should be taken from.

[Mike Pontillo (mpontillo)]

Looks like a nice improvement; a few comments below.

It would be nice if someone else could review, since I'm not yet very qualified!

review: Approve

[Gavin Panella (allenap)]

A few comments, that's all. Looks good :)

review: Approve

[Raphaël Badin (rvb)]

Thanks both for the reviews!

=== modified file 'src/maasserver/models/dhcplease.py'
--- src/maasserver/models/dhcplease.py	2014-09-01 13:32:35 +0000
+++ src/maasserver/models/dhcplease.py	2015-03-06 11:15:40 +0000
@@ -32,7 +32,10 @@
     MACAddressField,
     )
 from maasserver.models.cleansave import CleanSave
-from maasserver.models.macaddress import MACAddress
+from maasserver.models.macaddress import (
+    MACAddress,
+    update_macs_cluster_interfaces,
+    )
 from maasserver.utils import strip_domain
 
 
@@ -97,6 +100,8 @@
 
         This deletes entries that are no longer current, adds new ones,
         and updates or replaces ones that have changed.
+        This method also updates the MAC address objects to link them to
+        their respective cluster interface.
 
         :param nodegroup: The node group that these updates are for.
         :param leases: A dict describing all current IP/MAC mappings as
@@ -108,6 +113,7 @@
         """
         self._delete_obsolete_leases(nodegroup, leases)
         new_leases = self._add_missing_leases(nodegroup, leases)
+        update_macs_cluster_interfaces(leases, nodegroup)
         return new_leases
 
     def get_hostname_ip_mapping(self, nodegroup):

=== modified file 'src/maasserver/models/macaddress.py'
--- src/maasserver/models/macaddress.py	2015-01-28 09:13:11 +0000
+++ src/maasserver/models/macaddress.py	2015-03-06 11:15:40 +0000
@@ -67,12 +67,15 @@
 
 def update_mac_cluster_interfaces(ip, mac, cluster):
     """Calculate and store which interface a MAC is attached to."""
-    try:
-        mac_address = MACAddress.objects.get(mac_address=mac)
-    except MACAddress.DoesNotExist:
-        # Silently ignore MAC addresses that we don't know about.
-        return
-
+    # Create a `leases` dict with only one lease: this is so we can re-use
+    # update_macs_cluster_interfaces() which is designed to deal with multiple
+    # leases.
+    leases = {ip: mac}
+    update_macs_cluster_interfaces(leases, cluster)
+
+
+def update_macs_cluster_interfaces(leases, cluster):
+    """Calculate and store which interface a set of MACs are attached to."""
     interface_ranges = {}
     # Only consider configured interfaces.
     interfaces = (
@@ -92,15 +95,25 @@
             static_range = []
         interface_ranges[interface] = (ip_range, static_range)
 
-    # Look through the interface ranges to see if any match the passed
-    # IP address.
-    for interface, (ip_range, static_range) in interface_ranges.items():
-        ipaddress = IPAddress(ip)
-        # Set the cluster interface only if it's new/changed.
-        # This is only an optimisation to prevent repeated logging.
-        changed = mac_address.cluster_interface != interface
-        in_range = ipaddress in ip_range or ipaddress in static_range
-        if in_range and changed:
+    for ip, mac in leases.viewitems():
+        # Look through the interface ranges to see if any match the passed
+        # IP address.
+        try:
+            mac_address = MACAddress.objects.get(mac_address=mac)
+        except MACAddress.DoesNotExist:
+            # Silently ignore MAC addresses that we don't know about.
+            continue
+
+        for interface, (ip_range, static_range) in interface_ranges.items():
+            ipaddress = IPAddress(ip)
+            # Set the cluster interface only if it's new/changed.
+            # This is only an optimisation to prevent repeated logging.
+            changed = mac_address.cluster_interface != interface
+            if not changed:
+                continue
+            in_range = ipaddress in ip_range or ipaddress in static_range
+            if not in_range:
+                continue
             mac_address.cluster_interface = interface
             mac_address.save()
             maaslog.info(
@@ -112,9 +125,10 @@
             if ipnetwork is not None:
                 try:
                     network = Network.objects.get(ip=ipnetwork.ip.format())
-                    network.macaddress_set.add(mac_address)
                 except Network.DoesNotExist:
                     pass
+                else:
+                    network.macaddress_set.add(mac_address)
 
 
 class MACAddress(CleanSave, TimestampedModel):

=== modified file 'src/maasserver/models/tests/test_dhcplease.py'
--- src/maasserver/models/tests/test_dhcplease.py	2014-12-05 12:48:49 +0000
+++ src/maasserver/models/tests/test_dhcplease.py	2015-03-06 11:15:40 +0000
@@ -23,8 +23,10 @@
     )
 from maasserver.models import DHCPLease
 from maasserver.testing.factory import factory
+from maasserver.testing.orm import reload_object
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils import ignore_unused
+from netaddr import IPRange
 
 
 def get_leases(nodegroup):
@@ -165,6 +167,22 @@
             },
             map_leases(nodegroup))
 
+    def test_update_leases_updates_macs_cluster_interfaces(self):
+        cluster = factory.make_NodeGroup()
+        mac_address = factory.make_MACAddress_with_Node()
+        interface = factory.make_NodeGroupInterface(nodegroup=cluster)
+        ip_range = IPRange(
+            interface.static_ip_range_low, interface.static_ip_range_high)
+        ip = unicode(random.choice(ip_range))
+
+        leases = {
+            factory.make_ipv4_address(): factory.make_mac_address(),
+            ip: mac_address.mac_address,
+        }
+        DHCPLease.objects.update_leases(cluster, leases)
+        self.assertEquals(
+            interface, reload_object(mac_address).cluster_interface)
+
     def test_update_leases_does_not_update_dns_zone_if_nothing_added(self):
         self.patch(dns, 'dns_update_zones')
         nodegroup = factory.make_NodeGroup()

=== modified file 'src/maasserver/models/tests/test_macaddress.py'
--- src/maasserver/models/tests/test_macaddress.py	2015-01-26 08:45:19 +0000
+++ src/maasserver/models/tests/test_macaddress.py	2015-03-06 11:15:40 +0000
@@ -32,6 +32,7 @@
     find_cluster_interface_responsible_for_ip,
     MACAddress,
     update_mac_cluster_interfaces,
+    update_macs_cluster_interfaces,
     )
 from maasserver.models.network import Network
 from maasserver.testing.factory import factory
@@ -732,42 +733,44 @@
         self.assertItemsEqual([my_interface], my_mac.get_cluster_interfaces())
 
 
+def make_cluster_with_macs_and_leases(use_static_range=False):
+    cluster = factory.make_NodeGroup()
+    mac_addresses = {
+        factory.make_MACAddress_with_Node():
+        factory.make_NodeGroupInterface(nodegroup=cluster)
+        for _ in range(4)
+        }
+    leases = {
+        get_random_ip_from_interface_range(interface, use_static_range): (
+            mac_address.mac_address)
+        for mac_address, interface in mac_addresses.viewitems()
+    }
+    return cluster, mac_addresses, leases
+
+
+def make_cluster_with_mac_and_node_and_ip(use_static_range=False):
+    cluster = factory.make_NodeGroup()
+    mac_address = factory.make_MACAddress_with_Node()
+    interface = factory.make_NodeGroupInterface(nodegroup=cluster)
+    ip = get_random_ip_from_interface_range(interface, use_static_range)
+    return cluster, interface, mac_address, ip
+
+
 class TestUpdateMacClusterInterfaces(MAASServerTestCase):
     """Tests for `update_mac_cluster_interfaces`()."""
 
-    def make_cluster_with_macs_and_leases(self, use_static_range=False):
-        cluster = factory.make_NodeGroup()
-        mac_addresses = {
-            factory.make_MACAddress_with_Node():
-            factory.make_NodeGroupInterface(nodegroup=cluster)
-            for _ in range(4)
-            }
-        leases = {
-            get_random_ip_from_interface_range(interface, use_static_range): (
-                mac_address.mac_address)
-            for mac_address, interface in mac_addresses.viewitems()
-        }
-        return cluster, mac_addresses, leases
-
-    def make_cluster_with_mac_and_node_and_ip(self, use_static_range=False):
-        cluster = factory.make_NodeGroup()
-        mac_address = factory.make_MACAddress_with_Node()
-        interface = factory.make_NodeGroupInterface(nodegroup=cluster)
-        ip = get_random_ip_from_interface_range(interface, use_static_range)
-        return cluster, interface, mac_address, ip
-
     def test_updates_mac_cluster_interfaces(self):
         cluster, interface, mac_address, ip = (
-            self.make_cluster_with_mac_and_node_and_ip())
+            make_cluster_with_mac_and_node_and_ip())
         update_mac_cluster_interfaces(ip, mac_address.mac_address, cluster)
         mac_address = reload_object(mac_address)
         self.assertEqual(interface, mac_address.cluster_interface)
 
     def test_considers_static_range_when_updating_interfaces(self):
         cluster, mac_addresses, leases = (
-            self.make_cluster_with_macs_and_leases(use_static_range=True))
+            make_cluster_with_macs_and_leases(use_static_range=True))
         cluster, interface, mac_address, ip = (
-            self.make_cluster_with_mac_and_node_and_ip(use_static_range=True))
+            make_cluster_with_mac_and_node_and_ip(use_static_range=True))
         update_mac_cluster_interfaces(ip, mac_address.mac_address, cluster)
         mac_address = reload_object(mac_address)
         self.assertEqual(interface, mac_address.cluster_interface)
@@ -776,9 +779,9 @@
         # update_mac_cluster_interfaces should also associate the mac
         # with the network on which it resides.
         cluster, mac_addresses, leases = (
-            self.make_cluster_with_macs_and_leases())
+            make_cluster_with_macs_and_leases())
         cluster, interface, mac_address, ip = (
-            self.make_cluster_with_mac_and_node_and_ip())
+            make_cluster_with_mac_and_node_and_ip())
         net = create_Network_from_NodeGroupInterface(interface)
         update_mac_cluster_interfaces(ip, mac_address.mac_address, cluster)
         [observed_macddress] = net.macaddress_set.all()
@@ -835,3 +838,30 @@
         # The real test is that update_mac_cluster_interfaces() doesn't
         # stacktrace because of the unconfigured interface (see bug
         # 1332596).
+
+
+class TestUpdateMacsClusterInterfaces(MAASServerTestCase):
+    """Tests for `update_macs_cluster_interfaces`()."""
+
+    def test_updates_macs_cluster_interfaces(self):
+        cluster, mac_addresses, leases = (
+            make_cluster_with_macs_and_leases())
+        update_macs_cluster_interfaces(leases, cluster)
+        interfaces = [inter for _, inter in mac_addresses.items()]
+        linked_interfaces = (
+            [reload_object(mac).cluster_interface
+             for mac, _ in mac_addresses.items()])
+        self.assertEquals(interfaces, linked_interfaces)
+
+    def test_ignores_unknown_macs(self):
+        mac_address = factory.make_mac_address()
+        ip = factory.make_ipv4_address()
+        cluster, mac_addresses, leases = (
+            make_cluster_with_macs_and_leases())
+        leases[ip] = mac_address
+        update_macs_cluster_interfaces(leases, cluster)
+        interfaces = [inter for _, inter in mac_addresses.items()]
+        linked_interfaces = (
+            [reload_object(mac).cluster_interface
+             for mac, _ in mac_addresses.items()])
+        self.assertEquals(interfaces, linked_interfaces)