launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10739
[Merge] lp:~rvb/maas/dns-setting-cleanup into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/dns-setting-cleanup into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/dns-setting-cleanup/+merge/118755
This branch fixes a few things in the DNS module. It's a preparation branch for the follow-up branch which introduces a global start-up method for the MAAS application.
In this branch I did 3 things:
- add retry capabilities to the rndc_command task (and add a parameter in the caller method: write_full_dns_config).
- refactored all the code from dns.py used to connect signals with DNS config writing functions into src/maasserver/dns_connect. The main reason behind this change is that, in order to connect the signals, one has to import the module where the signals are connected in models/__init__.py (this module is imported by Django). And since this import happens very early when the application is loaded, it's best to separate this from the rest of the code in order to avoid circular import nightmares.
- move the is_dns_enabled() check further down the stack. It was only done in the methods called by the signals and I've moved that check into the DNS-conf-writing methods themselves. This is to ease the testing of the start_up method which will (among other things) call write_full_dns_config() and will now have an easy way to disconnect the DNS machinery (which is very heavy to set up).
= Notes =
- You'll note that 'test_write_full_dns_passes_reload_retry_parameter' simply checks that the 'retry' parameter is correctly passed down the stack. Testing the behavior with a real DNS set-up (i.e. testing that the rndc command is actually retired in a real DNS environment) would have been complicated and since the behavior is actually unit-tested in 'test_rndc_command_can_be_retried', I think it is fine.
- In case you wonder, 'test_rndc_command_can_be_retried' does not take forever to run because, in the test environment, tasks are retried instantly, ignoring the 'countdown' parameter.
--
https://code.launchpad.net/~rvb/maas/dns-setting-cleanup/+merge/118755
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/dns-setting-cleanup into lp:maas.
=== modified file 'src/maasserver/dns.py'
--- src/maasserver/dns.py 2012-08-06 10:11:02 +0000
+++ src/maasserver/dns.py 2012-08-08 15:05:24 +0000
@@ -23,25 +23,17 @@
import socket
from django.conf import settings
-from django.db.models.signals import (
- post_delete,
- post_save,
- )
-from django.dispatch import receiver
from maasserver.exceptions import MAASException
from maasserver.models import (
Config,
DHCPLease,
- Node,
NodeGroup,
)
-from maasserver.models.dhcplease import post_updates
from maasserver.sequence import (
INT_MAX,
Sequence,
)
from maasserver.server_address import get_maas_facing_server_address
-from maasserver.signals import connect_to_field_change
from netaddr import (
IPAddress,
IPNetwork,
@@ -101,55 +93,6 @@
return ip
-def dns_config_changed(sender, config, created, **kwargs):
- """Signal callback called when the DNS config changed."""
- if is_dns_enabled():
- write_full_dns_config(active=config.value)
-
-
-Config.objects.config_changed_connect('enable_dns', dns_config_changed)
-
-
-@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):
- """When a Node is deleted, update the Node's zone file."""
- if is_dns_enabled():
- change_dns_zones(instance.nodegroup)
-
-
-def dns_post_edit_hostname_Node(instance, old_field):
- """When a Node has been flagged, update the related zone."""
- if is_dns_enabled():
- change_dns_zones(instance.nodegroup)
-
-
-connect_to_field_change(dns_post_edit_hostname_Node, Node, 'hostname')
-
-
def get_zone(nodegroup, serial=None):
"""Create a :class:`DNSZoneConfig` object from a nodegroup.
@@ -191,6 +134,8 @@
zone should be updated.
:type nodegroups: list (or :class:`NodeGroup`)
"""
+ if not is_dns_enabled():
+ return
if not isinstance(nodegroups, collections.Iterable):
nodegroups = [nodegroups]
serial = next_zone_serial()
@@ -215,6 +160,8 @@
:param nodegroup: The nodegroup for which the zone should be added.
:type nodegroup: :class:`NodeGroup`
"""
+ if not is_dns_enabled():
+ return
zone = get_zone(nodegroup)
if zone is None:
return None
@@ -228,12 +175,19 @@
zone=zone, callback=write_dns_config_subtask)
-def write_full_dns_config(active=True):
+def write_full_dns_config(active=True, reload_retry=False):
"""Write the DNS configuration.
- If active is True, write the DNS config for all the nodegroups.
- If active is False, write an empty DNS config (with no zones).
+ :param active: If True, write the DNS config for all the nodegroups.
+ Otherwise write an empty DNS config (with no zones). Defaults
+ to `True`.
+ :type active: bool
+ :param reload_retry: Should the reload rndc command be retried in case
+ of failure? Defaults to `False`.
+ :type reload_retry: bool
"""
+ if not is_dns_enabled():
+ return
if active:
serial = next_zone_serial()
zones = get_zones(NodeGroup.objects.all(), serial)
@@ -241,4 +195,5 @@
zones = []
tasks.write_full_dns_config.delay(
zones=zones,
- callback=tasks.rndc_command.subtask(args=[['reload']]))
+ callback=tasks.rndc_command.subtask(
+ args=[['reload'], reload_retry]))
=== added file 'src/maasserver/dns_connect.py'
--- src/maasserver/dns_connect.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/dns_connect.py 2012-08-08 15:05:24 +0000
@@ -0,0 +1,78 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""DNS management module: connect DNS tasks with signals."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ ]
+
+
+from django.db.models.signals import (
+ post_delete,
+ post_save,
+ )
+from django.dispatch import receiver
+from maasserver.models import (
+ Config,
+ DHCPLease,
+ Node,
+ NodeGroup,
+ )
+from maasserver.models.dhcplease import post_updates
+from maasserver.signals import connect_to_field_change
+
+
+def dns_config_changed(sender, config, created, **kwargs):
+ """Signal callback called when the DNS config changed."""
+ from maasserver.dns import write_full_dns_config
+ write_full_dns_config(active=config.value)
+
+
+Config.objects.config_changed_connect('enable_dns', dns_config_changed)
+
+
+@receiver(post_save, sender=NodeGroup)
+def dns_post_save_NodeGroup(sender, instance, created, **kwargs):
+ """Create or update DNS zones related to the new nodegroup."""
+ from maasserver.dns import write_full_dns_config, add_zone
+ 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."""
+ from maasserver.dns import write_full_dns_config
+ write_full_dns_config()
+
+
+@receiver(post_updates, sender=DHCPLease.objects)
+def dns_updated_DHCPLeaseManager(sender, **kwargs):
+ """Update all the zone files."""
+ from maasserver.dns import change_dns_zones
+ change_dns_zones(NodeGroup.objects.all())
+
+
+@receiver(post_delete, sender=Node)
+def dns_post_delete_Node(sender, instance, **kwargs):
+ """When a Node is deleted, update the Node's zone file."""
+ from maasserver.dns import change_dns_zones
+ change_dns_zones(instance.nodegroup)
+
+
+def dns_post_edit_hostname_Node(instance, old_field):
+ """When a Node has been flagged, update the related zone."""
+ from maasserver.dns import change_dns_zones
+ change_dns_zones(instance.nodegroup)
+
+
+connect_to_field_change(dns_post_edit_hostname_Node, Node, 'hostname')
=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py 2012-07-25 15:25:42 +0000
+++ src/maasserver/models/__init__.py 2012-08-08 15:05:24 +0000
@@ -109,5 +109,5 @@
from maasserver import messages
ignore_unused(messages)
-from maasserver import dns
-ignore_unused(dns)
+from maasserver import dns_connect
+ignore_unused(dns_connect)
=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py 2012-08-06 10:11:02 +0000
+++ src/maasserver/tests/test_dns.py 2012-08-08 15:05:24 +0000
@@ -17,6 +17,7 @@
import random
import socket
+from celery.task import task
from django.conf import settings
from django.core.management import call_command
from maasserver import (
@@ -38,6 +39,7 @@
IPNetwork,
IPRange,
)
+from provisioningserver import tasks
from provisioningserver.dns.config import (
conf,
DNSZoneConfig,
@@ -194,6 +196,7 @@
def test_add_zone_loads_dns_zone(self):
nodegroup, node, lease = self.create_nodegroup_with_lease()
+ self.patch(settings, 'DNS_CONNECT', True)
dns.add_zone(nodegroup)
self.assertDNSMatches(node.hostname, nodegroup.name, lease.ip)
@@ -203,11 +206,13 @@
nodegroup = factory.make_node_group()
nodegroup.subnet_mask = None
nodegroup.save()
+ self.patch(settings, 'DNS_CONNECT', True)
dns.add_zone(nodegroup)
self.assertEqual(0, recorder.call_count)
def test_change_dns_zone_changes_dns_zone(self):
nodegroup, _, _ = self.create_nodegroup_with_lease()
+ self.patch(settings, 'DNS_CONNECT', True)
dns.write_full_dns_config()
nodegroup, new_node, new_lease = (
self.create_nodegroup_with_lease(
@@ -234,6 +239,7 @@
nodegroup = factory.make_node_group()
nodegroup.subnet_mask = None
nodegroup.save()
+ self.patch(settings, 'DNS_CONNECT', True)
dns.change_dns_zones(nodegroup)
self.assertEqual(0, recorder.call_count)
@@ -243,23 +249,40 @@
nodegroup = factory.make_node_group()
nodegroup.subnet_mask = None
nodegroup.save()
+ self.patch(settings, 'DNS_CONNECT', True)
dns.write_full_dns_config()
self.assertEqual(0, recorder.call_count)
def test_write_full_dns_loads_full_dns_config(self):
nodegroup, node, lease = self.create_nodegroup_with_lease()
+ self.patch(settings, 'DNS_CONNECT', True)
dns.write_full_dns_config()
self.assertDNSMatches(node.hostname, nodegroup.name, lease.ip)
def test_write_full_dns_can_write_inactive_config(self):
nodegroup, node, lease = self.create_nodegroup_with_lease()
+ self.patch(settings, 'DNS_CONNECT', True)
dns.write_full_dns_config(active=False)
self.assertEqual([''], self.dig_resolve(generated_hostname(lease.ip)))
+ def test_write_full_dns_passes_reload_retry_parameter(self):
+ self.patch(settings, 'DNS_CONNECT', True)
+ recorder = FakeMethod()
+
+ @task
+ def recorder_task(*args, **kwargs):
+ return recorder(*args, **kwargs)
+ self.patch(tasks, 'rndc_command', recorder_task)
+ dns.write_full_dns_config(reload_retry=True)
+ self.assertEqual(
+ (1, (['reload'], True)),
+ (recorder.call_count, recorder.extract_args()[0]))
+
def test_dns_config_has_NS_record(self):
ip = factory.getRandomIPAddress()
self.patch(settings, 'DEFAULT_MAAS_URL', 'http://%s/' % ip)
nodegroup, node, lease = self.create_nodegroup_with_lease()
+ self.patch(settings, 'DNS_CONNECT', True)
dns.write_full_dns_config()
# Get the NS record for the zone 'nodegroup.name'.
ns_record = dig_call(
=== modified file 'src/maastesting/factory.py'
--- src/maastesting/factory.py 2012-07-27 20:20:15 +0000
+++ src/maastesting/factory.py 2012-08-08 15:05:24 +0000
@@ -18,6 +18,7 @@
from functools import partial
import httplib
from itertools import (
+ chain,
imap,
islice,
repeat,
@@ -93,6 +94,28 @@
stamp = random.randrange(start, end)
return datetime.datetime.fromtimestamp(stamp)
+ def make_failure_simulator(self, exception, number_of_failures):
+ """Return a method raising an exception a fixed number of times.
+
+ When called repeatedly, the returned method will raise the given
+ exception a fixed number of times, and after that, if called
+ again, will always succeed and return None.
+
+ This is used to simulate a temporary failure.
+ """
+
+ def failure_controller(number):
+ """Return True `number` times, then always False."""
+ return chain(repeat(True, number), repeat(False))
+
+ failure_control = failure_controller(number_of_failures)
+
+ def simulate_failures(*args, **kwargs):
+ if failure_control.next():
+ raise exception
+
+ return simulate_failures
+
def make_file(self, location, name=None, contents=None):
"""Create a file, and write data to it.
=== modified file 'src/maastesting/tests/test_factory.py'
--- src/maastesting/tests/test_factory.py 2012-07-27 20:20:15 +0000
+++ src/maastesting/tests/test_factory.py 2012-08-08 15:05:24 +0000
@@ -82,6 +82,18 @@
def test_make_file_creates_file(self):
self.assertThat(factory.make_file(self.make_dir()), FileExists())
+ def test_make_failure_simulator_returns_failure_simulator(self):
+ nb_failures = randint(2, 5)
+ simulator = factory.make_failure_simulator(ValueError(), nb_failures)
+ # First we get `nb_failures` failures.
+ for index in range(nb_failures):
+ self.assertRaises(
+ ValueError, simulator, factory.make_name('random-arg'))
+ # Then only successes.
+ nb_successes = randint(2, 5)
+ self.assertEqual(
+ [None] * nb_successes, map(simulator, range(nb_successes)))
+
def test_make_file_writes_contents(self):
contents = factory.getRandomString().encode('ascii')
self.assertThat(
=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py 2012-08-08 05:54:08 +0000
+++ src/provisioningserver/tasks.py 2012-08-08 15:05:24 +0000
@@ -133,14 +133,31 @@
# DNS-related tasks
# =====================================================================
-
-@task
-def rndc_command(arguments, callback=None):
+# How many times should a rndc task be retried?
+RNDC_COMMAND_MAX_RETRY = 10
+
+# How long to wait between rndc tasks retry (in seconds)?
+RNDC_COMMAND_RETRY_DELAY = 2
+
+
+@task(max_retries=RNDC_COMMAND_MAX_RETRY)
+def rndc_command(arguments, retry=False, callback=None):
"""Use rndc to execute a command.
+ :param arguments: Argument list passed down to the rndc command.
+ :type arguments : list
+ :param retry: Should this task be retried in case of failure?
+ :type retry: bool
:param callback: Callback subtask.
:type callback: callable
"""
- execute_rndc_command(arguments)
+ try:
+ execute_rndc_command(arguments)
+ except CalledProcessError, exc:
+ if retry:
+ return rndc_command.retry(
+ exc=exc, countdown=RNDC_COMMAND_RETRY_DELAY)
+ else:
+ raise
if callback is not None:
callback.delay()
=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py 2012-08-08 05:54:08 +0000
+++ src/provisioningserver/tests/test_tasks.py 2012-08-08 15:05:24 +0000
@@ -42,6 +42,7 @@
remove_dhcp_host_map,
restart_dhcp_server,
rndc_command,
+ RNDC_COMMAND_MAX_RETRY,
setup_rndc_configuration,
write_dhcp_config,
write_dns_config,
@@ -339,6 +340,19 @@
Equals([((command,), {})]),
)))
+ def test_rndc_command_can_be_retried(self):
+ # The rndc_command task can be retried.
+ # Simulate a temporary failure.
+ number_of_failures = RNDC_COMMAND_MAX_RETRY / 2
+ simulate_failures = factory.make_failure_simulator(
+ CalledProcessError(
+ factory.make_name('exception'), random.randint(100, 200)),
+ number_of_failures)
+ self.patch(tasks, 'execute_rndc_command', simulate_failures)
+ command = factory.getRandomString()
+ result = rndc_command.delay(command, retry=True)
+ self.assertTrue(result.successful())
+
def test_write_full_dns_config_sets_up_config(self):
# write_full_dns_config writes the config file, writes
# the zone files, and reloads the dns service.
