launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10281
[Merge] lp:~rvb/maas/hook-up-dns-conf-updates into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/hook-up-dns-conf-updates into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/hook-up-dns-conf-updates/+merge/116681
This branch hooks up the DNS conf writing methods to signals so that the DNS server conf is updated each time a nodegroup is created/modified/updated and each time a lease record is changed.
= Pre-imp =
This was discussed with Julian.
= Notes =
- I've added a config option DNS_CONNECT used to connect disconnect the DNS machinery. It serves two purposes: 1) be able to run most of the tests (unrelated to DNS stuff) without writing DNS conf files 2) I'm sure it will come in handy when we will want to debug things. Sure, I'll add a user level config option to disable the DNS as well, but this is intended to be a more lower level switch.
- I've fixed factory.make_node_group. It now can work in two different ways: either you pass it a network (IPNetwork) object or all the parameters directly. This is useful to create a nodegroup with a coherent network configuration.
- You'll note that I've change the number of times dig tries to resolve a hostname. This is to fix the spurious failures I started to see: sometimes reloading a zone takes more time than it should.
- I've changed DHCPLease.objects.update_leases so that it now sends a signal to indicate that the lease record has changed. I could have been more clever here and compute the zone in need of a refresh instead of re-writing all the zones. After a talk with the team on IRC, this seemed like a premature optimization. Also, maybe it's not such a great idea to slow down the execution of 'update_leases'. Right now, it simply fires a signal which, in turn, fires a few asynchronous celery tasks.
- I agree that assertDNSMatches is a little bit of a mess. That's why I've added more explicit failure messages. Still I think that method abstracts quite a bit of boiler-plate code so I'd like to keep it.
--
https://code.launchpad.net/~rvb/maas/hook-up-dns-conf-updates/+merge/116681
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/hook-up-dns-conf-updates into lp:maas.
=== modified file 'src/maas/demo.py'
--- src/maas/demo.py 2012-06-21 20:49:18 +0000
+++ src/maas/demo.py 2012-07-25 15:52:18 +0000
@@ -36,6 +36,9 @@
# Enable longpoll. Set LONGPOLL_PATH to None to disable it.
LONGPOLL_PATH = '/longpoll/'
+# Connect to the DNS server.
+DNS_CONNECT = True
+
# For demo purposes, use a real provisioning server.
USE_REAL_PSERV = True
=== modified file 'src/maas/development.py'
--- src/maas/development.py 2012-07-23 21:22:32 +0000
+++ src/maas/development.py 2012-07-25 15:52:18 +0000
@@ -40,6 +40,10 @@
# Use a fake provisioning server for test/demo purposes.
USE_REAL_PSERV = False
+# Don't connect to the DNS server in tests, this will be enabled on a case per
+# cqse basis.
+DNS_CONNECT = False
+
# Invalid strings should be visible.
TEMPLATE_STRING_IF_INVALID = '#### INVALID STRING ####'
=== modified file 'src/maas/settings.py'
--- src/maas/settings.py 2012-07-23 21:22:32 +0000
+++ src/maas/settings.py 2012-07-25 15:52:18 +0000
@@ -47,6 +47,12 @@
LOGIN_REDIRECT_URL = '/'
LOGIN_URL = '/accounts/login/'
+# Should the DNS features be enabled?. Note that the MAAS' DNS features can
+# be enabled/disabled by an admin using a config option. Having this config
+# option is a debugging/testing feature to be able to quickly disconnect the
+# DNS machinery.
+DNS_CONNECT = True
+
# The MAAS CLI.
MAAS_CLI = 'sudo maas'
=== modified file 'src/maasserver/dns.py'
--- src/maasserver/dns.py 2012-07-25 09:36:03 +0000
+++ src/maasserver/dns.py 2012-07-25 15:52:18 +0000
@@ -12,16 +12,26 @@
__metaclass__ = type
__all__ = [
'add_zone',
- 'change_dns_zone',
+ 'change_dns_zones',
'next_zone_serial',
'write_full_dns_config',
]
+import collections
+
+from django.conf import settings
+from django.db.models.signals import (
+ post_delete,
+ post_save,
+ )
+from django.dispatch import receiver
from maasserver.models import (
DHCPLease,
+ Node,
NodeGroup,
)
+from maasserver.models.dhcplease import post_updates
from maasserver.sequence import (
INT_MAX,
Sequence,
@@ -40,6 +50,41 @@
return '%0.10d' % zone_serial.nextval()
+def is_dns_enabled():
+ return settings.DNS_CONNECT
+
+
+@receiver(post_save, sender=NodeGroup)
+def dns_post_save_NodeGroup(sender, instance, created, **kwargs):
+ """Create or update DNS zones related to the new nodegroup."""
+ if is_dns_enabled():
+ if created:
+ add_zone(instance)
+ else:
+ write_full_dns_config()
+
+
+@receiver(post_delete, sender=NodeGroup)
+def dns_post_delete_NodeGroup(sender, instance, **kwargs):
+ """Delete DNS zones related to the nodegroup."""
+ if is_dns_enabled():
+ write_full_dns_config()
+
+
+@receiver(post_updates, sender=DHCPLease.objects)
+def dns_updated_DHCPLeaseManager(sender, **kwargs):
+ """Update all the zone files."""
+ if is_dns_enabled():
+ change_dns_zones(NodeGroup.objects.all())
+
+
+@receiver(post_delete, sender=Node)
+def dns_post_delete_Node(sender, instance, **kwargs):
+ """Update the Node's zone file."""
+ if is_dns_enabled():
+ change_dns_zones(instance.nodegroup)
+
+
def get_zone(nodegroup, serial=None):
"""Create a :class:`DNSZoneConfig` object from a nodegroup.
@@ -56,20 +101,25 @@
mapping=DHCPLease.objects.get_hostname_ip_mapping(nodegroup))
-def change_dns_zone(nodegroup):
- """Update the zone configurtion for the given `nodegroup`.
+def change_dns_zones(nodegroups):
+ """Update the zone configuration for the given list of Nodegroups.
- :param nodegroup: The nodegroup for which the zone should be updated.
- :type nodegroup: :class:`NodeGroup`
+ :param nodegroup: The list of nodegroups (or the nodegroup) for which the
+ zone should be updated.
+ :type nodegroup: list (or :class:`NodeGroup`)
"""
- zone = get_zone(nodegroup)
- reverse_zone_reload_subtask = tasks.rndc_command.subtask(
- args=[['reload', zone.reverse_zone_name]])
- zone_reload_subtask = tasks.rndc_command.subtask(
- args=[['reload', zone.zone_name]],
- callback=reverse_zone_reload_subtask)
- tasks.write_dns_zone_config.delay(
- zone=zone, callback=zone_reload_subtask)
+ if not isinstance(nodegroups, collections.Iterable):
+ nodegroups = [nodegroups]
+ serial = next_zone_serial()
+ for nodegroup in nodegroups:
+ zone = get_zone(nodegroup, serial)
+ reverse_zone_reload_subtask = tasks.rndc_command.subtask(
+ args=[['reload', zone.reverse_zone_name]])
+ zone_reload_subtask = tasks.rndc_command.subtask(
+ args=[['reload', zone.zone_name]],
+ callback=reverse_zone_reload_subtask)
+ tasks.write_dns_zone_config.delay(
+ zone=zone, callback=zone_reload_subtask)
def add_zone(nodegroup):
=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py 2012-07-10 11:31:58 +0000
+++ src/maasserver/models/__init__.py 2012-07-25 15:52:18 +0000
@@ -108,3 +108,6 @@
from maasserver import messages
ignore_unused(messages)
+
+from maasserver import dns
+ignore_unused(dns)
=== modified file 'src/maasserver/models/dhcplease.py'
--- src/maasserver/models/dhcplease.py 2012-07-13 10:50:23 +0000
+++ src/maasserver/models/dhcplease.py 2012-07-25 15:52:18 +0000
@@ -12,6 +12,7 @@
__metaclass__ = type
__all__ = [
'DHCPLease',
+ 'post_updates'
]
@@ -22,10 +23,14 @@
Manager,
Model,
)
+from django.dispatch import Signal
from maasserver import DefaultMeta
from maasserver.fields import MACAddressField
from maasserver.models.cleansave import CleanSave
+# A signal indicating that the record of leases has changed.
+post_updates = Signal()
+
class DHCPLeaseManager(Manager):
"""Utility that manages :class:`DHCPLease` objects.
@@ -95,6 +100,7 @@
"""
self._delete_obsolete_leases(nodegroup, leases)
self._add_missing_leases(nodegroup, leases)
+ post_updates.send(sender=self)
def get_hostname_ip_mapping(self, nodegroup):
"""Return a mapping {hostnames -> ips} for the currently leased
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py 2012-07-24 17:47:48 +0000
+++ src/maasserver/testing/factory.py 2012-07-25 15:52:18 +0000
@@ -110,22 +110,44 @@
return reload_object(node)
def make_node_group(self, name=None, worker_ip=None, router_ip=None,
- api_token=None, network=None, **kwargs):
+ api_token=None, network=None, subnet_mask=None,
+ broadcast_ip=None, ip_range_low=None,
+ ip_range_high=None, **kwargs):
+ """Create a :class:`NodeGroup`.
+
+ If network (an instance of IPNetwork) is provided, use it to populate
+ subnet_mask, broadcast_ip, ip_range_low, ip_range_high, router_ip and
+ worker_ip. This is a convenience to setup a coherent network all in
+ one go.
+
+ Otherwise, use the provided values for these values or use random IP
+ addresses if they are not provided.
+ """
if name is None:
name = self.make_name('nodegroup')
if api_token is None:
user = self.make_user()
api_token = create_auth_token(user)
- if network is None:
- network = factory.getRandomNetwork()
- subnet_mask = str(network.netmask)
- broadcast_ip = str(network.broadcast)
- ip_range_low = str(IPAddress(network.first))
- ip_range_high = str(IPAddress(network.last))
- if router_ip is None:
+ if network is not None:
+ subnet_mask = str(network.netmask)
+ broadcast_ip = str(network.broadcast)
+ ip_range_low = str(IPAddress(network.first))
+ ip_range_high = str(IPAddress(network.last))
router_ip = factory.getRandomIPInNetwork(network)
- if worker_ip is None:
worker_ip = factory.getRandomIPInNetwork(network)
+ else:
+ if subnet_mask is None:
+ subnet_mask = self.getRandomIPAddress()
+ if broadcast_ip is None:
+ broadcast_ip = self.getRandomIPAddress()
+ if ip_range_low is None:
+ ip_range_low = self.getRandomIPAddress()
+ if ip_range_high is None:
+ ip_range_high = self.getRandomIPAddress()
+ if router_ip is None:
+ router_ip = self.getRandomIPAddress()
+ if worker_ip is None:
+ worker_ip = self.getRandomIPAddress()
ng = NodeGroup(
name=name, api_token=api_token, api_key=api_token.key,
worker_ip=worker_ip, subnet_mask=subnet_mask,
=== modified file 'src/maasserver/tests/test_dhcplease.py'
--- src/maasserver/tests/test_dhcplease.py 2012-07-12 19:03:39 +0000
+++ src/maasserver/tests/test_dhcplease.py 2012-07-25 15:52:18 +0000
@@ -12,10 +12,14 @@
__metaclass__ = type
__all__ = []
-from maasserver.models.dhcplease import DHCPLease
+from maasserver.models.dhcplease import (
+ DHCPLease,
+ post_updates,
+ )
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
from maasserver.utils import ignore_unused
+from maastesting.fakemethod import FakeMethod
def get_leases(nodegroup):
@@ -135,6 +139,17 @@
},
map_leases(nodegroup))
+ def test_update_leases_fires_signal(self):
+ # A call to DHCPLease.objects.update_leases fires the 'post_updates'
+ # signal.
+ recorder = FakeMethod()
+ post_updates.connect(recorder, sender=DHCPLease.objects)
+ self.addCleanup(
+ post_updates.disconnect, recorder, sender=DHCPLease.objects)
+ nodegroup = factory.make_node_group()
+ DHCPLease.objects.update_leases(nodegroup, {})
+ self.assertEqual(1, recorder.call_count)
+
def test_get_hostname_ip_mapping_returns_mapping(self):
nodegroup = factory.make_node_group()
expected_mapping = {}
=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py 2012-07-25 09:36:03 +0000
+++ src/maasserver/tests/test_dns.py 2012-07-25 15:52:18 +0000
@@ -15,14 +15,20 @@
from itertools import islice
+from django.conf import settings
from django.core.management import call_command
from maasserver.dns import (
add_zone,
- change_dns_zone,
+ change_dns_zones,
+ is_dns_enabled,
next_zone_serial,
write_full_dns_config,
zone_serial,
)
+from maasserver.models.dhcplease import (
+ DHCPLease,
+ post_updates,
+ )
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
from maastesting.bindfixture import BINDServer
@@ -91,6 +97,10 @@
lease_ip = str(islice(ips, lease_number, lease_number + 1).next())
lease = factory.make_dhcp_lease(
nodegroup=nodegroup, mac=mac.mac_address, ip=lease_ip)
+ # Simulate that this lease was created by
+ # DHCPLease.objects.update_leases: fire the 'post_updates'
+ # signal.
+ post_updates.send(sender=DHCPLease.objects)
return nodegroup, node, lease
def dig_resolve(self, fqdn):
@@ -108,15 +118,27 @@
def assertDNSMatches(self, hostname, domain, ip):
fqdn = "%s.%s" % (hostname, domain)
autogenerated_hostname = '%s.' % generated_hostname(ip, domain)
- # The fqdn resolves to the autogenerated hostname (CNAME record) and
- # the IP address (A record).
- self.assertItemsEqual(
- [autogenerated_hostname, ip],
- self.dig_resolve(fqdn))
+ forward_lookup_result = self.dig_resolve(fqdn)
+ if '%s.' % fqdn == autogenerated_hostname:
+ # If the fqdn is an autogenerated hostname, it resolves to the IP
+ # address (A record).
+ expected_results = [ip]
+ else:
+ # If the fqdn is a custom hostname, it resolves to the
+ # autogenerated hostname (CNAME record) and the IP address
+ # (A record).
+ expected_results = [autogenerated_hostname, ip]
+ self.assertEqual(
+ expected_results, forward_lookup_result,
+ "Failed to resolve '%s' (results: '%s')." % (
+ fqdn, ','.join(forward_lookup_result)))
# A reverse lookup on the IP returns the autogenerated
# hostname.
+ reverse_lookup_result = self.dig_reverse_resolve(ip)
self.assertEqual(
- [autogenerated_hostname], self.dig_reverse_resolve(ip))
+ [autogenerated_hostname], reverse_lookup_result,
+ "Failed to reverse resolve '%s' (results: '%s')." % (
+ fqdn, ','.join(reverse_lookup_result)))
def test_add_zone_loads_dns_zone(self):
nodegroup, node, lease = self.create_nodegroup_with_lease()
@@ -129,10 +151,61 @@
nodegroup, new_node, new_lease = (
self.create_nodegroup_with_lease(
nodegroup=nodegroup, lease_number=2))
- change_dns_zone(nodegroup)
+ change_dns_zones(nodegroup)
self.assertDNSMatches(new_node.hostname, nodegroup.name, new_lease.ip)
def test_write_full_dns_loads_full_dns_config(self):
nodegroup, node, lease = self.create_nodegroup_with_lease()
write_full_dns_config()
self.assertDNSMatches(node.hostname, nodegroup.name, lease.ip)
+
+ def test_is_dns_enabled_follows_DNS_CONNECT(self):
+ rand_bool = factory.getRandomBoolean()
+ self.patch(settings, "DNS_CONNECT", rand_bool)
+ self.assertEqual(rand_bool, is_dns_enabled())
+
+ def test_add_nodegroup_creates_DNS_zone(self):
+ self.patch(settings, "DNS_CONNECT", True)
+ network = IPNetwork('192.168.7.1/24')
+ ip = factory.getRandomIPInNetwork(network)
+ nodegroup = factory.make_node_group(network=network)
+ self.assertDNSMatches(generated_hostname(ip), nodegroup.name, ip)
+
+ def test_edit_nodegroup_updates_DNS_zone(self):
+ self.patch(settings, "DNS_CONNECT", True)
+ old_network = IPNetwork('192.168.7.1/24')
+ old_ip = factory.getRandomIPInNetwork(old_network)
+ nodegroup = factory.make_node_group(network=old_network)
+ # Edit nodegroup's network information to '192.168.44.1/24'
+ nodegroup.broadcast_ip = '192.168.44.255'
+ nodegroup.netmask = '255.255.255.0'
+ nodegroup.ip_range_low = '192.168.44.0'
+ nodegroup.ip_range_high = '192.168.44.255'
+ nodegroup.save()
+ ip = factory.getRandomIPInNetwork(IPNetwork('192.168.44.1/24'))
+ # The ip from the old network does not resolve anymore.
+ self.assertEqual([''], self.dig_resolve(generated_hostname(old_ip)))
+ self.assertEqual([''], self.dig_reverse_resolve(old_ip))
+ # The ip from the new network resolves.
+ self.assertDNSMatches(generated_hostname(ip), nodegroup.name, ip)
+
+ def test_delete_nodegroup_disables_DNS_zone(self):
+ self.patch(settings, "DNS_CONNECT", True)
+ network = IPNetwork('192.168.7.1/24')
+ ip = factory.getRandomIPInNetwork(network)
+ nodegroup = factory.make_node_group(network=network)
+ nodegroup.delete()
+ self.assertEqual([''], self.dig_resolve(generated_hostname(ip)))
+ self.assertEqual([''], self.dig_reverse_resolve(ip))
+
+ def test_add_node_updates_zone(self):
+ self.patch(settings, "DNS_CONNECT", True)
+ nodegroup, node, lease = self.create_nodegroup_with_lease()
+ self.assertDNSMatches(node.hostname, nodegroup.name, lease.ip)
+
+ def test_remove_node_updates_zone(self):
+ self.patch(settings, "DNS_CONNECT", True)
+ nodegroup, node, lease = self.create_nodegroup_with_lease()
+ node.delete()
+ fqdn = "%s.%s" % (node.hostname, nodegroup.name)
+ self.assertEqual([''], self.dig_resolve(fqdn))
=== modified file 'src/maastesting/tests/test_bindfixture.py'
--- src/maastesting/tests/test_bindfixture.py 2012-07-17 09:08:49 +0000
+++ src/maastesting/tests/test_bindfixture.py 2012-07-25 15:52:18 +0000
@@ -48,7 +48,7 @@
:rtype: basestring
"""
cmd = [
- 'dig', '+time=1', '+tries=1', '@%s' % server, '-p',
+ 'dig', '+time=1', '+tries=5', '@%s' % server, '-p',
'%d' % port]
if commands is not None:
if not isinstance(commands, list):
