Merge lp:~julian-edwards/maas/always-send-leases into lp:~maas-committers/maas/trunk

[Julian Edwards]

Attempt to populate the cluster_interface when a MAC is added to a node. This avoids the problem where a lease is already present after a node and/or MAC address is added because that linkage was previously only attempted at lease upload time. That's problematic because leases are not re-sent to the region unless changes are noticed.

[Julian Edwards (julian-edwards)]

The diff is much bigger than really needed because I had to relocate update_mac_cluster_interfaces() to somewhere better so to avoid circular imports.

[Jeroen T. Vermeulen (jtv)]

Just the changes in the imports are enough to suggest that you moved update_mac_cluster_interfaces() to a better place. Aren't there any tests to move though?

It seems heavy-handed to run all of the cluster's leases through update_mac_cluster_interfaces just because you're adding one MAC to one node... Would it not be enough to pass just the leases for that one MAC?

review: Approve

[Julian Edwards (julian-edwards)]

On Wednesday 10 September 2014 06:24:16 you wrote:
> Review: Approve
>
> Just the changes in the imports are enough to suggest that you moved
> update_mac_cluster_interfaces() to a better place. Aren't there any tests
> to move though?

Ah good point, I forgot those, thanks.

> It seems heavy-handed to run all of the cluster's leases through
> update_mac_cluster_interfaces just because you're adding one MAC to one
> node... Would it not be enough to pass just the leases for that one MAC?

I thought long and hard about this, and decided that it might not be a bad
idea to let them all refresh. Let's revisit if it's obviously becoming a
problem.

Thanks for the review!

[Jeroen T. Vermeulen (jtv)]

Careful: if you update other MACs that aren't obviously related, you may end up hiding some other, similar bug from our testing.

[Julian Edwards (julian-edwards)]

On Wednesday 10 September 2014 06:35:03 you wrote:
> Careful: if you update other MACs that aren't obviously related, you may end
> up hiding some other, similar bug from our testing.

That's a fair point. I'm going to land this now anyway so we can unfuck the
CI suite. Let's have a think about this at a more leisurely pace and see what
needs to be changed.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~julian-edwards/maas/always-send-leases into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mimeparse python-mock python-netaddr python-netifaces python-nose python-oauth python-oops python-oops-amqp python-oops-datedir-repo python-oops-twisted python-oops-wsgi python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-seamicroclient python-simplejson python-simplestreams python-sphi...

=== modified file 'src/maasserver/api/node_groups.py'
--- src/maasserver/api/node_groups.py	2014-09-09 11:21:41 +0000
+++ src/maasserver/api/node_groups.py	2014-09-10 07:41:56 +0000
@@ -53,19 +53,14 @@
     NodeGroupDefineForm,
     NodeGroupEdit,
     )
-from maasserver.models import (
-    MACAddress,
-    Network,
-    Node,
-    NodeGroup,
-    )
+from maasserver.models.node import Node
+from maasserver.models.nodegroup import NodeGroup
 from maasserver.models.nodeprobeddetails import get_probed_details
 from maasserver.utils import (
     build_absolute_uri,
     get_local_cluster_UUID,
     )
 from maasserver.utils.orm import get_one
-import netaddr
 
 
 DISPLAYED_NODEGROUP_FIELDS = ('uuid', 'status', 'name', 'cluster_name')
@@ -230,57 +225,6 @@
         nodegroup.save()
 
 
-def update_mac_cluster_interfaces(leases, cluster):
-    """Calculate and store which interface a MAC is attached to."""
-    interface_ranges = {}
-    # Only consider configured interfaces.
-    interfaces = (
-        cluster.nodegroupinterface_set
-        .exclude(ip_range_low__isnull=True)
-        .exclude(ip_range_high__isnull=True)
-    )
-    for interface in interfaces:
-        ip_range = netaddr.IPRange(
-            interface.ip_range_low, interface.ip_range_high)
-        if interface.static_ip_range_low and interface.static_ip_range_high:
-            static_range = netaddr.IPRange(
-                interface.static_ip_range_low, interface.static_ip_range_high)
-        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 = netaddr.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
-
-
 class NodeGroupsHandler(OperationsHandler):
     """Manage the collection of all the nodegroups in this MAAS."""
 

=== modified file 'src/maasserver/api/tests/test_nodegroup.py'
--- src/maasserver/api/tests/test_nodegroup.py	2014-09-09 07:21:43 +0000
+++ src/maasserver/api/tests/test_nodegroup.py	2014-09-10 07:41:56 +0000
@@ -16,24 +16,18 @@
 
 import httplib
 import json
-import random
 from textwrap import dedent
 
 import bson
 from django.contrib.auth.models import AnonymousUser
 from django.core.urlresolvers import reverse
-from maasserver.api.node_groups import update_mac_cluster_interfaces
 from maasserver.bootresources import get_simplestream_endpoint
 from maasserver.enum import (
     NODEGROUP_STATUS,
     NODEGROUP_STATUS_CHOICES,
-    NODEGROUPINTERFACE_MANAGEMENT,
     )
-from maasserver.forms import create_Network_from_NodeGroupInterface
 from maasserver.models import (
     DownloadProgress,
-    MACAddress,
-    Network,
     NodeGroup,
     nodegroup as nodegroup_module,
     )
@@ -60,31 +54,15 @@
     NodeResult,
     )
 from mock import Mock
-import netaddr
 from provisioningserver.auth import get_recorded_nodegroup_uuid
 from provisioningserver.rpc.cluster import ImportBootImages
 from testresources import FixtureResource
 from testtools.matchers import (
     AllMatch,
     Equals,
-    MatchesStructure,
     )
 
 
-def get_random_ip_from_interface_range(interface, use_static_range=None):
-    """Return a random IP from the pool available to an interface.
-
-    :return: An IP address as a string."""
-    if use_static_range:
-        ip_range = netaddr.IPRange(
-            interface.static_ip_range_low, interface.static_ip_range_high)
-    else:
-        ip_range = netaddr.IPRange(
-            interface.ip_range_low, interface.ip_range_high)
-    chosen_ip = random.choice(ip_range)
-    return unicode(chosen_ip)
-
-
 class TestNodeGroupsAPI(MultipleUsersScenarios,
                         MAASServerTestCase):
     scenarios = [
@@ -694,117 +672,3 @@
         self.assertEqual(
             httplib.FORBIDDEN, response.status_code,
             explain_unexpected_response(httplib.FORBIDDEN, response))
-
-
-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(): factory.make_NodeGroupInterface(
-                nodegroup=cluster)
-            for i 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 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)
-
-    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)
-
-    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,
-                ))
-
-    def test_does_not_overwrite_network_with_same_name(self):
-        cluster = factory.make_NodeGroup()
-        ngi = factory.make_NodeGroupInterface(nodegroup=cluster)
-        net1 = create_Network_from_NodeGroupInterface(ngi)
-
-        other_ngi = factory.make_NodeGroupInterface(nodegroup=cluster)
-        other_ngi.interface = ngi.interface
-        net2 = create_Network_from_NodeGroupInterface(ngi)
-        self.assertEqual(None, net2)
-        self.assertItemsEqual([net1], Network.objects.all())
-
-    def test_ignores_mac_not_attached_to_cluster(self):
-        cluster = factory.make_NodeGroup()
-        mac_address = factory.make_MACAddress()
-        leases = {
-            factory.getRandomIPAddress(): mac_address.mac_address
-            }
-        update_mac_cluster_interfaces(leases, cluster)
-        mac_address = MACAddress.objects.get(
-            id=mac_address.id)
-        self.assertIsNone(mac_address.cluster_interface)
-
-    def test_ignores_unknown_macs(self):
-        cluster = factory.make_NodeGroup()
-        mac_address = factory.getRandomMACAddress()
-        leases = {
-            factory.getRandomIPAddress(): mac_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)
-        self.assertFalse(
-            MACAddress.objects.filter(mac_address=mac_address).exists())
-
-    def test_ignores_unconfigured_interfaces(self):
-        cluster = factory.make_NodeGroup()
-        factory.make_NodeGroupInterface(
-            nodegroup=cluster, subnet_mask='', broadcast_ip='',
-            static_ip_range_low='', static_ip_range_high='',
-            ip_range_low='', ip_range_high='', router_ip='',
-            ip=factory.getRandomIPAddress(),
-            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
-        mac_address = factory.getRandomMACAddress()
-        leases = {
-            factory.getRandomIPAddress(): mac_address
-            }
-        self.assertIsNone(update_mac_cluster_interfaces(leases, 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/models/macaddress.py'
--- src/maasserver/models/macaddress.py	2014-09-09 11:06:20 +0000
+++ src/maasserver/models/macaddress.py	2014-09-10 07:41:56 +0000
@@ -35,9 +35,13 @@
     )
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.managers import BulkManager
+from maasserver.models.network import Network
 from maasserver.models.nodegroupinterface import NodeGroupInterface
 from maasserver.models.timestampedmodel import TimestampedModel
-from netaddr import IPAddress
+from netaddr import (
+    IPAddress,
+    IPRange,
+    )
 from provisioningserver.logger import get_maas_logger
 
 
@@ -59,6 +63,57 @@
     return None
 
 
+def update_mac_cluster_interfaces(leases, cluster):
+    """Calculate and store which interface a MAC is attached to."""
+    interface_ranges = {}
+    # Only consider configured interfaces.
+    interfaces = (
+        cluster.nodegroupinterface_set
+        .exclude(ip_range_low__isnull=True)
+        .exclude(ip_range_high__isnull=True)
+    )
+    for interface in interfaces:
+        ip_range = IPRange(
+            interface.ip_range_low, interface.ip_range_high)
+        if interface.static_ip_range_low and interface.static_ip_range_high:
+            static_range = IPRange(
+                interface.static_ip_range_low, interface.static_ip_range_high)
+        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
+
+
 class MACAddress(CleanSave, TimestampedModel):
     """A `MACAddress` represents a `MAC address`_ attached to a :class:`Node`.
 

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-09-04 22:46:11 +0000
+++ src/maasserver/models/node.py	2014-09-10 07:41:56 +0000
@@ -78,6 +78,7 @@
 from maasserver.models.config import Config
 from maasserver.models.dhcplease import DHCPLease
 from maasserver.models.licensekey import LicenseKey
+from maasserver.models.macaddress import update_mac_cluster_interfaces
 from maasserver.models.staticipaddress import (
     StaticIPAddress,
     StaticIPAddressExhaustion,
@@ -882,6 +883,14 @@
 
         mac = MACAddress(mac_address=mac_address, node=self)
         mac.save()
+
+        # 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)
+
         return mac
 
     def remove_mac_address(self, mac_address):

=== modified file 'src/maasserver/models/tests/test_macaddress.py'
--- src/maasserver/models/tests/test_macaddress.py	2014-09-05 16:38:32 +0000
+++ src/maasserver/models/tests/test_macaddress.py	2014-09-10 07:41:56 +0000
@@ -15,6 +15,7 @@
 __all__ = []
 
 from operator import attrgetter
+import random
 
 from django.core.exceptions import ValidationError
 from maasserver.enum import (
@@ -22,27 +23,46 @@
     NODEGROUPINTERFACE_MANAGEMENT,
     )
 from maasserver.exceptions import StaticIPAddressTypeClash
+from maasserver.forms import create_Network_from_NodeGroupInterface
 from maasserver.models import (
-    MACAddress,
     NodeGroupInterface,
     StaticIPAddress,
     )
 from maasserver.models.macaddress import (
     find_cluster_interface_responsible_for_ip,
+    MACAddress,
+    update_mac_cluster_interfaces,
     )
+from maasserver.models.network import Network
 from maasserver.testing.factory import factory
 from maasserver.testing.orm import reload_object
 from maasserver.testing.testcase import MAASServerTestCase
 from netaddr import (
     IPAddress,
     IPNetwork,
+    IPRange,
     )
 from testtools.matchers import (
     Equals,
     HasLength,
+    MatchesStructure,
     )
 
 
+def get_random_ip_from_interface_range(interface, use_static_range=None):
+    """Return a random IP from the pool available to an interface.
+
+    :return: An IP address as a string."""
+    if use_static_range:
+        ip_range = IPRange(
+            interface.static_ip_range_low, interface.static_ip_range_high)
+    else:
+        ip_range = IPRange(
+            interface.ip_range_low, interface.ip_range_high)
+    chosen_ip = random.choice(ip_range)
+    return unicode(chosen_ip)
+
+
 class MACAddressTest(MAASServerTestCase):
 
     def test_stores_to_database(self):
@@ -675,3 +695,117 @@
         my_mac = factory.make_MACAddress(
             node=my_node, cluster_interface=my_interface)
         self.assertItemsEqual([my_interface], my_mac.get_cluster_interfaces())
+
+
+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(): factory.make_NodeGroupInterface(
+                nodegroup=cluster)
+            for i 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 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)
+
+    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)
+
+    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,
+                ))
+
+    def test_does_not_overwrite_network_with_same_name(self):
+        cluster = factory.make_NodeGroup()
+        ngi = factory.make_NodeGroupInterface(nodegroup=cluster)
+        net1 = create_Network_from_NodeGroupInterface(ngi)
+
+        other_ngi = factory.make_NodeGroupInterface(nodegroup=cluster)
+        other_ngi.interface = ngi.interface
+        net2 = create_Network_from_NodeGroupInterface(ngi)
+        self.assertEqual(None, net2)
+        self.assertItemsEqual([net1], Network.objects.all())
+
+    def test_ignores_mac_not_attached_to_cluster(self):
+        cluster = factory.make_NodeGroup()
+        mac_address = factory.make_MACAddress()
+        leases = {
+            factory.getRandomIPAddress(): mac_address.mac_address
+            }
+        update_mac_cluster_interfaces(leases, cluster)
+        mac_address = MACAddress.objects.get(
+            id=mac_address.id)
+        self.assertIsNone(mac_address.cluster_interface)
+
+    def test_ignores_unknown_macs(self):
+        cluster = factory.make_NodeGroup()
+        mac_address = factory.getRandomMACAddress()
+        leases = {
+            factory.getRandomIPAddress(): mac_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)
+        self.assertFalse(
+            MACAddress.objects.filter(mac_address=mac_address).exists())
+
+    def test_ignores_unconfigured_interfaces(self):
+        cluster = factory.make_NodeGroup()
+        factory.make_NodeGroupInterface(
+            nodegroup=cluster, subnet_mask='', broadcast_ip='',
+            static_ip_range_low='', static_ip_range_high='',
+            ip_range_low='', ip_range_high='', router_ip='',
+            ip=factory.getRandomIPAddress(),
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+        mac_address = factory.getRandomMACAddress()
+        leases = {
+            factory.getRandomIPAddress(): mac_address
+            }
+        self.assertIsNone(update_mac_cluster_interfaces(leases, 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/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-09-09 07:21:43 +0000
+++ src/maasserver/models/tests/test_node.py	2014-09-10 07:41:56 +0000
@@ -81,7 +81,7 @@
     ANY,
     sentinel,
     )
-from provisioningserver.rpc import cluster
+from provisioningserver.rpc import cluster as cluster_module
 from provisioningserver.rpc.exceptions import MultipleFailures
 from provisioningserver.rpc.testing import (
     always_succeed_with,
@@ -273,6 +273,21 @@
         macs = MACAddress.objects.filter(node=node, mac_address=mac).count()
         self.assertEqual(1, macs)
 
+    def test_add_mac_address_sets_cluster_interface(self):
+        # If a DHCPLease exists for this mac, ensure the
+        # cluster_interface is set on the basis of that lease.
+        cluster = factory.make_NodeGroup()
+        cluster_interface = factory.make_NodeGroupInterface(nodegroup=cluster)
+        ip_in_range = cluster_interface.static_ip_range_low
+        mac_address = factory.getRandomMACAddress()
+        factory.make_DHCPLease(
+            mac=mac_address, ip=ip_in_range, nodegroup=cluster)
+        node = factory.make_Node(nodegroup=cluster)
+
+        node.add_mac_address(mac_address)
+        self.assertEqual(
+            cluster_interface, node.get_primary_mac().cluster_interface)
+
     def test_remove_mac_address(self):
         mac = factory.getRandomMACAddress()
         node = factory.make_Node()
@@ -1459,7 +1474,7 @@
 
     def prepare_rpc_to_cluster(self, nodegroup):
         protocol = self.rpc_fixture.makeCluster(
-            nodegroup, cluster.CreateHostMaps, cluster.PowerOn)
+            nodegroup, cluster_module.CreateHostMaps, cluster_module.PowerOn)
         protocol.CreateHostMaps.side_effect = always_succeed_with({})
         protocol.PowerOn.side_effect = always_succeed_with({})
         return protocol

=== modified file 'src/maasserver/rpc/leases.py'
--- src/maasserver/rpc/leases.py	2014-09-02 02:18:28 +0000
+++ src/maasserver/rpc/leases.py	2014-09-10 07:41:56 +0000
@@ -17,8 +17,8 @@
 ]
 
 from django.shortcuts import get_object_or_404
-from maasserver.api.node_groups import update_mac_cluster_interfaces
 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 (