Merge lp:~julian-edwards/maas/pxeconfig-updates-cluster-interface-bug-1387859 into lp:~maas-committers/maas/trunk

[Julian Edwards]

Set the mac.cluster_interface when receiving a pxeconfig API request, instead of doing it when leases are parsed. Decoupling this makes it more resilient in the face of lease parser failures, and removes the update latency inherent in the parser.

Thanks to whomever wrote the Node.add_mac tests which don't use patching, and instead test the desired behaviour — it meant that I could just change the underlying code and watch it pass.

[Graham Binns (gmb)]

LGTM. Nice work!

review: Approve

=== modified file 'src/maasserver/api/pxeconfig.py'
--- src/maasserver/api/pxeconfig.py	2014-12-05 16:13:38 +0000
+++ src/maasserver/api/pxeconfig.py	2014-12-08 02:56:05 +0000
@@ -34,6 +34,7 @@
     MACAddress,
     NodeGroup,
     )
+from maasserver.models.macaddress import update_mac_cluster_interfaces
 from maasserver.preseed import (
     compose_enlistment_preseed_url,
     compose_preseed_url,
@@ -163,12 +164,15 @@
         is preferred.
     """
     request_mac = request.GET.get('mac', None)
+    request_ip = request.GET.get('remote', None)
     node = get_node_from_mac_string(request_mac)
 
     if node is not None:
-        # Update the record of which MAC address this node uses to PXE boot.
+        # Update the record of which MAC address and cluster interface
+        # this node uses to PXE boot.
         node.pxe_mac = MACAddress.objects.get(mac_address=request_mac)
         node.save()
+        update_mac_cluster_interfaces(request_ip, request_mac, node.nodegroup)
 
     if node is None or node.get_boot_purpose() == "commissioning":
         osystem = Config.objects.get_config('commissioning_osystem')

=== modified file 'src/maasserver/api/tests/test_pxeconfig.py'
--- src/maasserver/api/tests/test_pxeconfig.py	2014-12-05 16:13:38 +0000
+++ src/maasserver/api/tests/test_pxeconfig.py	2014-12-08 02:56:05 +0000
@@ -576,3 +576,17 @@
         params_out = self.get_pxeconfig(params)
         self.assertEqual(commissioning_osystem, params_out['osystem'])
         self.assertEqual(commissioning_series, params_out['release'])
+
+    def test_pxeconfig_updates_cluster_interface_for_request_mac(self):
+        params = self.get_mac_params()
+        node = factory.make_Node(mac=True)
+        params['cluster_uuid'] = node.nodegroup.uuid
+        params['mac'] = node.get_primary_mac()
+        update_mac_cluster_interfaces = self.patch(
+            pxeconfig_module, 'update_mac_cluster_interfaces')
+        response = self.client.get(reverse('pxeconfig'), params)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        self.assertThat(
+            update_mac_cluster_interfaces,
+            MockCalledOnceWith(
+                params['remote'], unicode(params['mac']), node.nodegroup))

=== modified file 'src/maasserver/models/macaddress.py'
--- src/maasserver/models/macaddress.py	2014-11-27 11:06:23 +0000
+++ src/maasserver/models/macaddress.py	2014-12-08 02:56:05 +0000
@@ -65,8 +65,14 @@
     return None
 
 
-def update_mac_cluster_interfaces(leases, cluster):
+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
+
     interface_ranges = {}
     # Only consider configured interfaces.
     interfaces = (
@@ -74,6 +80,8 @@
         .exclude(ip_range_low__isnull=True)
         .exclude(ip_range_high__isnull=True)
     )
+    # Pre-calculate a dict of interface ranges, keyed by cluster
+    # interface.
     for interface in interfaces:
         ip_range = IPRange(
             interface.ip_range_low, interface.ip_range_high)
@@ -83,37 +91,30 @@
         else:
             static_range = []
         interface_ranges[interface] = (ip_range, static_range)
-    for ip, mac in leases.items():
-        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
-            in_range = ipaddress in ip_range or ipaddress in static_range
-            if in_range and changed:
-                mac_address.cluster_interface = interface
-                mac_address.save()
-                maaslog.info(
-                    "%s %s linked to cluster interface %s",
-                    mac_address.node.hostname, mac_address, interface.name)
-
-                # Locate the Network to which this MAC belongs.
-                ipnetwork = interface.network
-                if ipnetwork is not None:
-                    try:
-                        network = Network.objects.get(ip=ipnetwork.ip.format())
-                        network.macaddress_set.add(mac_address)
-                    except Network.DoesNotExist:
-                        pass
-
-                # Cheap optimisation. No other interfaces will match, so
-                # break out of the loop.
-                break
+
+    # 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:
+            mac_address.cluster_interface = interface
+            mac_address.save()
+            maaslog.info(
+                "%s %s linked to cluster interface %s",
+                mac_address.node.hostname, mac_address, interface.name)
+
+            # Locate the Network to which this MAC belongs and link it.
+            ipnetwork = interface.network
+            if ipnetwork is not None:
+                try:
+                    network = Network.objects.get(ip=ipnetwork.ip.format())
+                    network.macaddress_set.add(mac_address)
+                except Network.DoesNotExist:
+                    pass
 
 
 class MACAddress(CleanSave, TimestampedModel):

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-12-05 21:32:33 +0000
+++ src/maasserver/models/node.py	2014-12-08 02:56:05 +0000
@@ -105,7 +105,10 @@
     get_db_state,
     strip_domain,
     )
-from maasserver.utils.orm import commit_within_atomic_block
+from maasserver.utils.orm import (
+    commit_within_atomic_block,
+    get_one,
+    )
 from metadataserver.enum import RESULT_TYPE
 from metadataserver.models import NodeResult
 from netaddr import IPAddress
@@ -783,10 +786,11 @@
 
         # See if there's a lease for this MAC and set its
         # cluster_interface if so.
-        nodegroup_leases = {
-            lease.ip: lease.mac
-            for lease in DHCPLease.objects.filter(nodegroup=self.nodegroup)}
-        update_mac_cluster_interfaces(nodegroup_leases, self.nodegroup)
+        leases = DHCPLease.objects.filter(
+            nodegroup=self.nodegroup, mac=mac.mac_address)
+        lease = get_one(leases)
+        if lease is not None:
+            update_mac_cluster_interfaces(lease.ip, lease.mac, self.nodegroup)
 
         return mac
 

=== modified file 'src/maasserver/models/tests/test_macaddress.py'
--- src/maasserver/models/tests/test_macaddress.py	2014-11-27 11:06:23 +0000
+++ src/maasserver/models/tests/test_macaddress.py	2014-12-08 02:56:05 +0000
@@ -736,49 +736,45 @@
         }
         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, mac_addresses, leases = (
-            self.make_cluster_with_macs_and_leases())
-        update_mac_cluster_interfaces(leases, cluster)
-        results = {
-            mac_address: mac_address.cluster_interface
-            for mac_address in MACAddress.objects.filter(
-                mac_address__in=leases.values())
-            }
-        self.assertEqual(mac_addresses, results)
+        cluster, interface, mac_address, ip = (
+            self.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))
-        update_mac_cluster_interfaces(leases, cluster)
-        results = {
-            mac_address: mac_address.cluster_interface
-            for mac_address in MACAddress.objects.filter(
-                mac_address__in=leases.values())
-            }
-        self.assertEqual(mac_addresses, results)
+        cluster, interface, mac_address, ip = (
+            self.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)
 
     def test_updates_network_relations(self):
         # 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())
-        expected_relations = []
-        for mac, interface in mac_addresses.viewitems():
-            net = create_Network_from_NodeGroupInterface(interface)
-            expected_relations.append((net, mac))
-        update_mac_cluster_interfaces(leases, cluster)
-        # Doing a single giant comparison here would be unintuitive and
-        # fugly, so I'm iterating.
-        for net, mac in expected_relations:
-            [observed_macddress] = net.macaddress_set.all()
-            self.expectThat(mac, Equals(observed_macddress))
-            interface = mac_addresses[mac]
-            self.expectThat(
-                net, MatchesStructure.byEquality(
-                    default_gateway=interface.router_ip,
-                    netmask=interface.subnet_mask,
-                ))
+        cluster, interface, mac_address, ip = (
+            self.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()
+        self.expectThat(mac_address, Equals(observed_macddress))
+        self.expectThat(
+            net, MatchesStructure.byEquality(
+                default_gateway=interface.router_ip,
+                netmask=interface.subnet_mask,
+            ))
 
     def test_does_not_overwrite_network_with_same_name(self):
         cluster = factory.make_NodeGroup()
@@ -794,10 +790,8 @@
     def test_ignores_mac_not_attached_to_cluster(self):
         cluster = factory.make_NodeGroup()
         mac_address = factory.make_MACAddress_with_Node()
-        leases = {
-            factory.make_ipv4_address(): mac_address.mac_address
-            }
-        update_mac_cluster_interfaces(leases, cluster)
+        ip = factory.make_ipv4_address()
+        update_mac_cluster_interfaces(ip, mac_address.mac_address, cluster)
         mac_address = MACAddress.objects.get(
             id=mac_address.id)
         self.assertIsNone(mac_address.cluster_interface)
@@ -805,13 +799,11 @@
     def test_ignores_unknown_macs(self):
         cluster = factory.make_NodeGroup()
         mac_address = factory.make_mac_address()
-        leases = {
-            factory.make_ipv4_address(): mac_address
-            }
+        ip = factory.make_ipv4_address()
         # This is a test to show that update_mac_cluster_interfaces()
         # doesn't raise an Http404 when it comes across something it
         # doesn't know, hence the lack of meaningful assertions.
-        update_mac_cluster_interfaces(leases, cluster)
+        update_mac_cluster_interfaces(ip, mac_address, cluster)
         self.assertFalse(
             MACAddress.objects.filter(mac_address=mac_address).exists())
 
@@ -824,10 +816,9 @@
             ip=factory.make_ipv4_address(),
             management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
         mac_address = factory.make_mac_address()
-        leases = {
-            factory.make_ipv4_address(): mac_address
-            }
-        self.assertIsNone(update_mac_cluster_interfaces(leases, cluster))
+        ip = factory.make_ipv4_address()
+        self.assertIsNone(
+            update_mac_cluster_interfaces(ip, mac_address, cluster))
         # The real test is that update_mac_cluster_interfaces() doesn't
         # stacktrace because of the unconfigured interface (see bug
         # 1332596).

=== modified file 'src/maasserver/rpc/leases.py'
--- src/maasserver/rpc/leases.py	2014-10-01 23:07:50 +0000
+++ src/maasserver/rpc/leases.py	2014-12-08 02:56:05 +0000
@@ -17,7 +17,6 @@
 ]
 
 from maasserver.models.dhcplease import DHCPLease
-from maasserver.models.macaddress import update_mac_cluster_interfaces
 from maasserver.models.nodegroup import NodeGroup
 from maasserver.utils.async import transactional
 from provisioningserver.pserv_services.lease_upload_service import (
@@ -50,5 +49,4 @@
     else:
         leases = convert_mappings_to_leases(mappings)
         DHCPLease.objects.update_leases(nodegroup, leases)
-        update_mac_cluster_interfaces(leases, nodegroup)
         return {}

=== modified file 'src/maasserver/rpc/tests/test_regionservice.py'
--- src/maasserver/rpc/tests/test_regionservice.py	2014-11-25 13:06:34 +0000
+++ src/maasserver/rpc/tests/test_regionservice.py	2014-12-08 02:56:05 +0000
@@ -420,31 +420,6 @@
         self.expectThat(mac, Equals(mapping["mac"]))
 
     @wait_for_reactor
-    @inlineCallbacks
-    def test__updates_mac_to_cluster_links(self):
-        uuid = factory.make_name("uuid")
-        nodegroup = yield deferToThread(self.make_node_group, uuid)
-        cluster_interface = yield deferToThread(
-            self.make_node_group_interface, nodegroup)
-        mac_address = yield deferToThread(self.make_mac_address)
-
-        mapping = {
-            "ip": cluster_interface.ip_range_low,
-            "mac": mac_address.mac_address.get_raw(),
-        }
-
-        response = yield call_responder(Region(), UpdateLeases, {
-            b"uuid": uuid, b"mappings": [mapping]})
-        self.assertThat(response, Equals({}))
-
-        @transactional
-        def get_cluster_interface():
-            return reload_object(mac_address).cluster_interface
-
-        observed = yield deferToThread(get_cluster_interface)
-        self.assertThat(observed, Equals(cluster_interface))
-
-    @wait_for_reactor
     def test__raises_NoSuchCluster_if_cluster_not_found(self):
         uuid = factory.make_name("uuid")
         d = call_responder(