launchpad-reviewers team mailing list archive

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

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

Commit message:
Backport trunk r1430: 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.

Requested reviews:
  MAAS Maintainers (maas-maintainers)
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/1.2-bug-1116700/+merge/147024

Straight backport, no changes needed.
-- 
https://code.launchpad.net/~jtv/maas/1.2-bug-1116700/+merge/147024
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/1.2-bug-1116700 into lp:maas/1.2.

=== modified file 'src/maasserver/models/dhcplease.py'
--- src/maasserver/models/dhcplease.py	2012-10-30 15:15:30 +0000
+++ src/maasserver/models/dhcplease.py	2013-02-07 06:34:22 +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-16 13:50:43 +0000
+++ src/maasserver/tests/test_dhcplease.py	2013-02-07 06:34:22 +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()