Merge lp:~jtv/maas/bug-1116700 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Generate CNAME for a node's oldest MACAddress that has a lease. (We used to generate a CNAME for a node's oldest MACAddress *if it had* a lease).

We don't know why the code was originally written this way, although it was deliberate at the time (originally proposed at https://code.launchpad.net/~rvb/maas/maas-dns2/+merge/114707 ). Our best recollection now is that it was done that way to mimick how things worked with Cobbler, but it led to problems when network interfaces weren't in the expected order.

We're taking a bit of a risk with this one, because we've lost the reason for the original behaviour. We do know that we had to do things like this for the original Cobbler-based MAAS, where things worked the way they did simply because that's how they worked in Cobbler, or because it was the way we ended up interfacing with Cobbler. DHCPLease was bolted on later, so there may have been problems with incorporating it nicely.

Raphaël sees no problem with the change. It seems more sensible than what we did before.

[Raphaël Badin (rvb)]

Looks good to me.

Thanks for updating the test's name and adding a new test.

3 tiny notes:

[0]

50 + node.hostname, lease.ip, mac.id
[...]
59 - for hostname, ip in cursor.fetchall()
60 + for hostname, ip, mac in cursor.fetchall()

You probably don't need to return the 'mac' in the query since you're ignoring it later.

[1]

101 + lease_for_newer_mac = factory.make_dhcp_lease(
102 + nodegroup=node.nodegroup, mac=newer_mac.mac_address)
103 + ignore_unused(lease_for_newer_mac)

A bit far fetched :) I'd get rid of the assignment and the ignore_unused() statement because I think the name of the other variable 'lease_for_older_mac' make the whole design pretty clear.

[2]

69 -# Copyright 2012 Canonical Ltd. This software is licensed under the
70 +# Copyright 2013 Canonical Ltd. This software is licensed under the

That's a nice touch but now you got me wondering about why you did not update dhcplease.py's header ;).

review: Approve

[Jeroen T. Vermeulen (jtv)]

> 59 - for hostname, ip in cursor.fetchall()
> 60 + for hostname, ip, mac in cursor.fetchall()
>
> You probably don't need to return the 'mac' in the query since you're ignoring
> it later.

You're right. There are some requirements with DISTINCT ON that can be surprising, and I was in a hurry to knock something out that other people could work with, without such puzzling surprises. So I erred on the side of caution in the prototype for my fix, and didn't tighten it up fully later.

> 101 + lease_for_newer_mac = factory.make_dhcp_lease(
> 102 + nodegroup=node.nodegroup, mac=newer_mac.mac_address)
> 103 + ignore_unused(lease_for_newer_mac)
>
> A bit far fetched :) I'd get rid of the assignment and the ignore_unused()
> statement because I think the name of the other variable 'lease_for_older_mac'
> make the whole design pretty clear.

Changed.

> 69 -# Copyright 2012 Canonical Ltd. This software is licensed under the
> 70 +# Copyright 2013 Canonical Ltd. This software is licensed under the
>
> That's a nice touch but now you got me wondering about why you did not update
> dhcplease.py's header ;).

Done.

=== modified file 'src/maasserver/models/dhcplease.py'
--- src/maasserver/models/dhcplease.py	2012-11-08 06:34:48 +0000
+++ src/maasserver/models/dhcplease.py	2013-02-07 04:00:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Node IP/MAC mappings as leased from the workers' DHCP servers."""
@@ -111,32 +111,32 @@
         """Return a mapping {hostnames -> ips} for the currently leased
         IP addresses for the nodes in `nodegroup`.
 
-        This will consider only the first interface (i.e. the first
-        MAC Address) associated with each node withing the given
-        `nodegroup`.
-        If the hostnames contain a domain, it gets removed.
+        For each node, this will consider only the oldest `MACAddress` that
+        has a `DHCPLease`.
+
+        Any domain will be stripped from the hostnames.
         """
         cursor = connection.cursor()
-        # The subquery fetches the IDs of the first MAC Address for
-        # all the nodes in this nodegroup.
-        # Then the main query returns the hostname -> ip mapping for
-        # these MAC Addresses.
+
+        # The "DISTINCT ON" gives us the first matching row for any
+        # given hostname, in the query's ordering.
+        # The ordering must start with the hostname so that the database
+        # can do this efficiently.  The next ordering criterion is the
+        # MACAddress id, so that if there are multiple rows with the
+        # same hostname, we get the one with the oldest MACAddress.
+        #
+        # If this turns out to be inefficient, be sure to try selecting
+        # on node.nodegroup_id instead of lease.nodegroup_id.  It has
+        # the same effect but may perform differently.
         cursor.execute("""
-        SELECT node.hostname, lease.ip
-        FROM maasserver_macaddress as mac,
-             maasserver_node as node,
-             maasserver_dhcplease as lease
-        WHERE mac.id IN (
-            SELECT DISTINCT ON (node_id) mac.id
-            FROM maasserver_macaddress as mac,
-                 maasserver_node as node
-            WHERE node.nodegroup_id = %s AND mac.node_id = node.id
-            ORDER BY node_id, mac.id
-        )
-        AND mac.node_id = node.id
-        AND mac.mac_address = lease.mac
-        AND lease.nodegroup_id = %s
-        """, (nodegroup.id, nodegroup.id))
+            SELECT DISTINCT ON (node.hostname)
+                node.hostname, lease.ip
+            FROM maasserver_macaddress AS mac
+            JOIN maasserver_node AS node ON node.id = mac.node_id
+            JOIN maasserver_dhcplease AS lease ON lease.mac = mac.mac_address
+            WHERE lease.nodegroup_id = %s
+            ORDER BY node.hostname, mac.id
+            """, (nodegroup.id, ))
         return dict(
             (strip_domain(hostname), ip)
             for hostname, ip in cursor.fetchall()
 
=== modified file 'src/maasserver/tests/test_dhcplease.py'
--- src/maasserver/tests/test_dhcplease.py	2012-11-08 06:34:48 +0000
+++ src/maasserver/tests/test_dhcplease.py	2013-02-07 04:00:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the :class:`DHCPLease` model."""
@@ -190,17 +190,28 @@
         mapping = DHCPLease.objects.get_hostname_ip_mapping(nodegroup)
         self.assertEqual({hostname: lease.ip}, mapping)
 
-    def test_get_hostname_ip_mapping_considers_only_first_mac(self):
-        nodegroup = factory.make_node_group()
-        node = factory.make_node(
-            nodegroup=nodegroup)
+    def test_get_hostname_ip_mapping_picks_mac_with_lease(self):
+        node = factory.make_node(hostname=factory.make_name('host'))
         factory.make_mac_address(node=node)
         second_mac = factory.make_mac_address(node=node)
         # Create a lease for the second MAC Address.
+        lease = factory.make_dhcp_lease(
+            nodegroup=node.nodegroup, mac=second_mac.mac_address)
+        mapping = DHCPLease.objects.get_hostname_ip_mapping(node.nodegroup)
+        self.assertEqual({node.hostname: lease.ip}, mapping)
+
+    def test_get_hostname_ip_mapping_picks_oldest_mac_with_lease(self):
+        node = factory.make_node(hostname=factory.make_name('host'))
+        older_mac = factory.make_mac_address(node=node)
+        newer_mac = factory.make_mac_address(node=node)
+
         factory.make_dhcp_lease(
-            nodegroup=nodegroup, mac=second_mac.mac_address)
-        mapping = DHCPLease.objects.get_hostname_ip_mapping(nodegroup)
-        self.assertEqual({}, mapping)
+            nodegroup=node.nodegroup, mac=newer_mac.mac_address)
+        lease_for_older_mac = factory.make_dhcp_lease(
+            nodegroup=node.nodegroup, mac=older_mac.mac_address)
+
+        mapping = DHCPLease.objects.get_hostname_ip_mapping(node.nodegroup)
+        self.assertEqual({node.hostname: lease_for_older_mac.ip}, mapping)
 
     def test_get_hostname_ip_mapping_considers_given_nodegroup(self):
         nodegroup = factory.make_node_group()