Merge lp:~julian-edwards/maas/consider-static-range into lp:~maas-committers/maas/trunk

[Julian Edwards]

Consider the static IP range, if configured, when linking MAC addresses to networks and cluster interfaces. This ensures that pre-existing nodes already using the static range will get linked.

The update_mac_cluster_interfaces() function is getting a little big, but it's just inside bigjools rule-of-thumb size for a function which is a full page of my terminal.

I hesitated to break it up anyway because everything in there is closely tied. Ideally it would be namespaced in a class to indicate that fact.

Anyway, api.py is 4000 lines long now. If we're going to split things up ...

[Jeroen T. Vermeulen (jtv)]

"Everything is closely tied" is not a reason not to break things up; it's a reason to put more effort into breaking them up! Such irredeemable patches of code tend to spread like oil slicks. There's clearly stuff here that could be extracted, and make the code cleaner for it. I see loops that could do with a name, a docstring, or at least a comment.

One question about the design of this branch: why would you look just at the static range, and not at the network's full address range? Does it matter for the purpose of linking a node's network interface to a subnet whether the address is in the static range? If I reduced the static range after a node had already acquired a static address, so that the address is now outside the static range, should that affect whether this could makes the link? Wouldn't be a check against the network be more robust, as well as faster (because IPRange is so awful about “in”) and simpler?

review: Approve

[Julian Edwards (julian-edwards)]

Thank you for the review.

On Thursday 14 Aug 2014 09:41:01 you wrote:
> Review: Approve
>
> "Everything is closely tied" is not a reason not to break things up; it's a
> reason to put more effort into breaking them up! Such irredeemable patches
> of code tend to spread like oil slicks. There's clearly stuff here that
> could be extracted, and make the code cleaner for it. I see loops that
> could do with a name, a docstring, or at least a comment.

Oh I do agree, believe me. What I meant by closely tied is that I want to
namespace it. I *really* dislike interdependent functions scattered around a
module. But right now I don't have the headspace to tackle this in a way that
I would like. It's not fun having an illness that makes you feel stupid :(

> One question about the design of this branch: why would you look just at the
> static range, and not at the network's full address range? Does it matter
> for the purpose of linking a node's network interface to a subnet whether
> the address is in the static range? If I reduced the static range after a
> node had already acquired a static address, so that the address is now
> outside the static range, should that affect whether this could makes the
> link? Wouldn't be a check against the network be more robust, as well as

The existing ranges are definitive in terms of what we are serving on this
cluster. You cannot reduce a range and leave an existing address outside that
range; it is checked for and disallowed.

The point is that we can (and will) have machines appearing on this network
that we're not meant to service, so a tighter check is warranted.

> faster (because IPRange is so awful about “in”) and simpler?

Actually IPRange is ok, it's IPSet that's slow.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2014-08-12 03:05:45 +0000
+++ src/maasserver/api.py	2014-08-14 09:21:28 +0000
@@ -1717,15 +1717,21 @@
     for interface in interfaces:
         ip_range = netaddr.IPRange(
             interface.ip_range_low, interface.ip_range_high)
-        interface_ranges[interface] = ip_range
+        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 in interface_ranges.items():
-            if netaddr.IPAddress(ip) in ip_range:
+        for interface, (ip_range, static_range) in interface_ranges.items():
+            ipaddress = netaddr.IPAddress(ip)
+            if ipaddress in ip_range or ipaddress in static_range:
                 mac_address.cluster_interface = interface
                 mac_address.save()
 
@@ -1735,6 +1741,9 @@
                     try:
                         network = Network.objects.get(ip=ipnetwork.ip.format())
                         network.macaddress_set.add(mac_address)
+                        maaslog.info(
+                            "Linking %s to network %s",
+                            mac_address, network.name)
                     except Network.DoesNotExist:
                         pass
 

=== modified file 'src/maasserver/tests/test_api_nodegroup.py'
--- src/maasserver/tests/test_api_nodegroup.py	2014-08-12 03:03:53 +0000
+++ src/maasserver/tests/test_api_nodegroup.py	2014-08-14 09:21:28 +0000
@@ -79,12 +79,16 @@
     )
 
 
-def get_random_ip_from_interface_range(interface):
+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."""
-    ip_range = netaddr.IPRange(
-        interface.ip_range_low, interface.ip_range_high)
+    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)
 
@@ -834,7 +838,7 @@
 class TestUpdateMacClusterInterfaces(MAASServerTestCase):
     """Tests for `update_mac_cluster_interfaces`()."""
 
-    def make_cluster_with_macs_and_leases(self):
+    def make_cluster_with_macs_and_leases(self, use_static_range=False):
         cluster = factory.make_node_group()
         mac_addresses = {
             factory.make_mac_address(): factory.make_node_group_interface(
@@ -842,7 +846,7 @@
             for i in range(4)
             }
         leases = {
-            get_random_ip_from_interface_range(interface): (
+            get_random_ip_from_interface_range(interface, use_static_range): (
                 mac_address.mac_address)
             for mac_address, interface in mac_addresses.viewitems()
         }
@@ -859,6 +863,17 @@
             }
         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.
@@ -873,7 +888,7 @@
         # fugly, so I'm iterating.
         for net, mac in expected_relations:
             [observed_macddress] = net.macaddress_set.all()
-            self.assertEqual(mac, observed_macddress)
+            self.expectThat(mac, Equals(observed_macddress))
 
     def test_ignores_mac_not_attached_to_cluster(self):
         cluster = factory.make_node_group()