launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1116700 into lp:maas.

Commit message:
Generate CNAME for a node's oldest MACAddress that has a lease.  (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.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1116700 in MAAS: "CNAME not added if PXE iface is different from first one in DB"
  https://bugs.launchpad.net/maas/+bug/1116700

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1116700/+merge/146835

We're taking a bit of a risk with this one, because the reasons for the original behaviour wasn't documented.  We did have to do something like this for the original Cobbler-based MAAS, where things worked the way they did simply because that's how they worked in Cobbler, and then DHCPLease was bolted on later.

Raphaël sees no problem with the change.  It seems more sensible than what we did before.
-- 
https://code.launchpad.net/~jtv/maas/bug-1116700/+merge/146835
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-1116700 into lp:maas.

=== 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-06 12:23:25 +0000
@@ -111,35 +111,35 @@
         """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, mac.id
+            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()
+            for hostname, ip, mac 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-06 12:23:25 +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."""
@@ -191,16 +191,14 @@
         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)
+        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.
-        factory.make_dhcp_lease(
-            nodegroup=nodegroup, mac=second_mac.mac_address)
-        mapping = DHCPLease.objects.get_hostname_ip_mapping(nodegroup)
-        self.assertEqual({}, mapping)
+        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_considers_given_nodegroup(self):
         nodegroup = factory.make_node_group()