Merge lp:~rvb/maas/bug-1064210-dns into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Strip out the domain part of the hostnames used to write out the DNS config files.

This branch is a band aid fix: strip out the domain part of the hostnames used to write out the DNS config files. Note that we can't really remove the domain part of the hostnames altogether because we need to be able to name nodes like 'hostname.local'.

[Jeroen T. Vermeulen (jtv)]

Besides the miscellaneous code notes (see below), I do have to wonder where we use the domain name that's in Node.hostname at all. We also ignore the domain name in pxeconfig(), using the nodegroup's domain instead. Giving users the option to edit the domain name isn't much good if it doesn't go anywhere!

The only use I can find for the domain is in get_preseed_filenames, and I'm not sure it's essential there. So maybe we should either leave the domain name out of Node.hostname altogether, or make it optional (and by default either leave it out, or use the nodegroup's domain).

.

In src/maasserver/models/dhcplease.py, around l.30:

8 +def strip_out_domain(hostname):
9 + """Remove the domain part of a hostname, if any."""
10 + return hostname.split('.', 1)[0]

I'd omit the “out” from the name — just “strip” has a clear meaning in python, well established by convention. Saying “strip out” instead makes me think you may mean something different, such as returning the domain instead of the host part.

To follow the conventions even more, maybe the docstring could say “Return `hostname` with the domain part removed”?

.

In src/maasserver/models/dhcplease.py, l.144 or so:

28 - return dict(cursor.fetchall())
29 + return dict([
30 + (strip_out_domain(hostname), ip)
31 + for hostname, ip in cursor.fetchall()
32 + ])

You don't need the square brackets.

Jeroen

review: Approve

[Raphaël Badin (rvb)]

Thanks for the review!

> Besides the miscellaneous code notes (see below), I do have to wonder where we
> use the domain name that's in Node.hostname at all. We also ignore the domain
> name in pxeconfig(), using the nodegroup's domain instead. Giving users the
> option to edit the domain name isn't much good if it doesn't go anywhere!
>
> The only use I can find for the domain is in get_preseed_filenames, and I'm
> not sure it's essential there. So maybe we should either leave the domain
> name out of Node.hostname altogether, or make it optional (and by default
> either leave it out, or use the nodegroup's domain).

That would be the long term solution indeed.

But we need to cope with the mDNS model and in that case, we want to be able to name nodes like "hostname.local".

> 8 +def strip_out_domain(hostname):
> 9 + """Remove the domain part of a hostname, if any."""
> 10 + return hostname.split('.', 1)[0]
>
> I'd omit the “out” from the name — just “strip” has a clear meaning in python,
> well established by convention. Saying “strip out” instead makes me think you
> may mean something different, such as returning the domain instead of the host
> part.
>
> To follow the conventions even more, maybe the docstring could say “Return
> `hostname` with the domain part removed”?

Ok, done.

> In src/maasserver/models/dhcplease.py, l.144 or so:
>
> 28 - return dict(cursor.fetchall())
> 29 + return dict([
> 30 + (strip_out_domain(hostname), ip)
> 31 + for hostname, ip in cursor.fetchall()
> 32 + ])
>
> You don't need the square brackets.

Fixed.

=== modified file 'src/maasserver/models/dhcplease.py'
--- src/maasserver/models/dhcplease.py	2012-09-03 11:08:32 +0000
+++ src/maasserver/models/dhcplease.py	2012-10-09 10:33:21 +0000
@@ -27,6 +27,11 @@
 from maasserver.models.cleansave import CleanSave
 
 
+def strip_domain(hostname):
+    """Return `hostname` with the domain part removed."""
+    return hostname.split('.', 1)[0]
+
+
 class DHCPLeaseManager(Manager):
     """Utility that manages :class:`DHCPLease` objects.
 
@@ -113,6 +118,7 @@
         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.
         """
         cursor = connection.cursor()
         # The subquery fetches the IDs of the first MAC Address for
@@ -135,7 +141,10 @@
         AND mac.mac_address = lease.mac
         AND lease.nodegroup_id = %s
         """, (nodegroup.id, nodegroup.id))
-        return dict(cursor.fetchall())
+        return dict(
+            (strip_domain(hostname), ip)
+            for hostname, ip in cursor.fetchall()
+            )
 
 
 class DHCPLease(CleanSave, Model):

=== modified file 'src/maasserver/tests/test_dhcplease.py'
--- src/maasserver/tests/test_dhcplease.py	2012-09-03 11:07:49 +0000
+++ src/maasserver/tests/test_dhcplease.py	2012-10-09 10:33:21 +0000
@@ -14,6 +14,7 @@
 
 from maasserver import dns
 from maasserver.models import DHCPLease
+from maasserver.models.dhcplease import strip_domain
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maasserver.utils import ignore_unused
@@ -46,6 +47,20 @@
         self.assertEqual(mac, lease.mac)
 
 
+class TestUtitilies(TestCase):
+
+    def test_strip_domain(self):
+        input_and_results = [
+            ('name.domain',  'name'),
+            ('name', 'name'),
+            ('name.domain.what', 'name'),
+            ('name..domain', 'name'),
+            ]
+        inputs = [input for input, _ in input_and_results]
+        results = [result for _, result in input_and_results]
+        self.assertEqual(results, map(strip_domain, inputs))
+
+
 class TestDHCPLeaseManager(TestCase):
     """Tests for :class:`DHCPLeaseManager`."""
 
@@ -177,6 +192,19 @@
         mapping = DHCPLease.objects.get_hostname_ip_mapping(nodegroup)
         self.assertEqual(expected_mapping, mapping)
 
+    def test_get_hostname_ip_mapping_strips_out_domain(self):
+        nodegroup = factory.make_node_group()
+        hostname = factory.make_name('hostname')
+        domain = factory.make_name('domain')
+        node = factory.make_node(
+            nodegroup=nodegroup,
+            hostname='%s.%s' % (hostname, domain))
+        mac = factory.make_mac_address(node=node)
+        lease = factory.make_dhcp_lease(
+            nodegroup=nodegroup, mac=mac.mac_address)
+        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(