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

Proposed by Jeroen T. Vermeulen on 2013-02-06
Status: Merged
Approved by: Jeroen T. Vermeulen on 2013-02-07
Approved revision: 1436
Merged at revision: 1430
Proposed branch: lp:~jtv/maas/bug-1116700
Merge into: lp:maas/trunk
Diff against target: 111 lines (+43/-32)
2 files modified
src/maasserver/models/dhcplease.py (+24/-24)
src/maasserver/tests/test_dhcplease.py (+19/-8)
To merge this branch: bzr merge lp:~jtv/maas/bug-1116700
Reviewer Review Type Date Requested Status
Raphaël Badin (community) 2013-02-06 Approve on 2013-02-06
Review via email: mp+146835@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
lp:~jtv/maas/bug-1116700 updated on 2013-02-06
1434. By Jeroen T. Vermeulen on 2013-02-06

Test a bit more thoroughly.

Raphaël Badin (rvb) wrote :

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
lp:~jtv/maas/bug-1116700 updated on 2013-02-07
1435. By Jeroen T. Vermeulen on 2013-02-07

Review changes.

1436. By Jeroen T. Vermeulen on 2013-02-07

Missed a spot.

Jeroen T. Vermeulen (jtv) wrote :

> 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/models/dhcplease.py'
2--- src/maasserver/models/dhcplease.py 2012-11-08 06:34:48 +0000
3+++ src/maasserver/models/dhcplease.py 2013-02-07 04:00:32 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2012 Canonical Ltd. This software is licensed under the
6+# Copyright 2013 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 """Node IP/MAC mappings as leased from the workers' DHCP servers."""
10@@ -111,32 +111,32 @@
11 """Return a mapping {hostnames -> ips} for the currently leased
12 IP addresses for the nodes in `nodegroup`.
13
14- This will consider only the first interface (i.e. the first
15- MAC Address) associated with each node withing the given
16- `nodegroup`.
17- If the hostnames contain a domain, it gets removed.
18+ For each node, this will consider only the oldest `MACAddress` that
19+ has a `DHCPLease`.
20+
21+ Any domain will be stripped from the hostnames.
22 """
23 cursor = connection.cursor()
24- # The subquery fetches the IDs of the first MAC Address for
25- # all the nodes in this nodegroup.
26- # Then the main query returns the hostname -> ip mapping for
27- # these MAC Addresses.
28+
29+ # The "DISTINCT ON" gives us the first matching row for any
30+ # given hostname, in the query's ordering.
31+ # The ordering must start with the hostname so that the database
32+ # can do this efficiently. The next ordering criterion is the
33+ # MACAddress id, so that if there are multiple rows with the
34+ # same hostname, we get the one with the oldest MACAddress.
35+ #
36+ # If this turns out to be inefficient, be sure to try selecting
37+ # on node.nodegroup_id instead of lease.nodegroup_id. It has
38+ # the same effect but may perform differently.
39 cursor.execute("""
40- SELECT node.hostname, lease.ip
41- FROM maasserver_macaddress as mac,
42- maasserver_node as node,
43- maasserver_dhcplease as lease
44- WHERE mac.id IN (
45- SELECT DISTINCT ON (node_id) mac.id
46- FROM maasserver_macaddress as mac,
47- maasserver_node as node
48- WHERE node.nodegroup_id = %s AND mac.node_id = node.id
49- ORDER BY node_id, mac.id
50- )
51- AND mac.node_id = node.id
52- AND mac.mac_address = lease.mac
53- AND lease.nodegroup_id = %s
54- """, (nodegroup.id, nodegroup.id))
55+ SELECT DISTINCT ON (node.hostname)
56+ node.hostname, lease.ip
57+ FROM maasserver_macaddress AS mac
58+ JOIN maasserver_node AS node ON node.id = mac.node_id
59+ JOIN maasserver_dhcplease AS lease ON lease.mac = mac.mac_address
60+ WHERE lease.nodegroup_id = %s
61+ ORDER BY node.hostname, mac.id
62+ """, (nodegroup.id, ))
63 return dict(
64 (strip_domain(hostname), ip)
65 for hostname, ip in cursor.fetchall()
66
67=== modified file 'src/maasserver/tests/test_dhcplease.py'
68--- src/maasserver/tests/test_dhcplease.py 2012-11-08 06:34:48 +0000
69+++ src/maasserver/tests/test_dhcplease.py 2013-02-07 04:00:32 +0000
70@@ -1,4 +1,4 @@
71-# Copyright 2012 Canonical Ltd. This software is licensed under the
72+# Copyright 2013 Canonical Ltd. This software is licensed under the
73 # GNU Affero General Public License version 3 (see the file LICENSE).
74
75 """Tests for the :class:`DHCPLease` model."""
76@@ -190,17 +190,28 @@
77 mapping = DHCPLease.objects.get_hostname_ip_mapping(nodegroup)
78 self.assertEqual({hostname: lease.ip}, mapping)
79
80- def test_get_hostname_ip_mapping_considers_only_first_mac(self):
81- nodegroup = factory.make_node_group()
82- node = factory.make_node(
83- nodegroup=nodegroup)
84+ def test_get_hostname_ip_mapping_picks_mac_with_lease(self):
85+ node = factory.make_node(hostname=factory.make_name('host'))
86 factory.make_mac_address(node=node)
87 second_mac = factory.make_mac_address(node=node)
88 # Create a lease for the second MAC Address.
89+ lease = factory.make_dhcp_lease(
90+ nodegroup=node.nodegroup, mac=second_mac.mac_address)
91+ mapping = DHCPLease.objects.get_hostname_ip_mapping(node.nodegroup)
92+ self.assertEqual({node.hostname: lease.ip}, mapping)
93+
94+ def test_get_hostname_ip_mapping_picks_oldest_mac_with_lease(self):
95+ node = factory.make_node(hostname=factory.make_name('host'))
96+ older_mac = factory.make_mac_address(node=node)
97+ newer_mac = factory.make_mac_address(node=node)
98+
99 factory.make_dhcp_lease(
100- nodegroup=nodegroup, mac=second_mac.mac_address)
101- mapping = DHCPLease.objects.get_hostname_ip_mapping(nodegroup)
102- self.assertEqual({}, mapping)
103+ nodegroup=node.nodegroup, mac=newer_mac.mac_address)
104+ lease_for_older_mac = factory.make_dhcp_lease(
105+ nodegroup=node.nodegroup, mac=older_mac.mac_address)
106+
107+ mapping = DHCPLease.objects.get_hostname_ip_mapping(node.nodegroup)
108+ self.assertEqual({node.hostname: lease_for_older_mac.ip}, mapping)
109
110 def test_get_hostname_ip_mapping_considers_given_nodegroup(self):
111 nodegroup = factory.make_node_group()