launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13634
[Merge] lp:~rvb/maas/bug-1067977-dns-conf into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/bug-1067977-dns-conf into lp:maas.
Commit message:
Write the DNS config only if at least one of the interfaces is configured to manage DNS.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1067977 in MAAS: "write_full_dns_config runs on a fresh installation even before dns is installed or configured"
https://bugs.launchpad.net/maas/+bug/1067977
For more details, see:
https://code.launchpad.net/~rvb/maas/bug-1067977-dns-conf/+merge/130958
This branch changes the DNS machinery so that it will ony attempt to write the DNS configuration is there is at lease one interface configured to handle DNS.
This makes the code a little bit more complex than blindly writing the config because when the last DNS-configured interface is deleted or if the DNS-management is switched off, this is an exception to the rule: there is no DNS-active interface and the config still needs to be written.
= Notes =
The main changes in this branch are:
- change is_dns_enabled so that it checks if at least one interface is configured to manage DNS.
- improve connect_to_field_change so that it can also track if an object has been deleted.
- use connect_to_field_change to track if an interface has been deleted or DNS-management switched off on an interface instead of simply listening to the signal indicating the deletion of a NodeGroup.
Drive-by fix 1: Change the warning message in warn_loopback (src/maasserver/dns.py) so that it mentions the correct location of the production settings file.
Drive-by fix 2: Add a test named test_get_dns_server_address_logs_warning_if_ip_is_localhost (now src/maasserver/dns.py has a test coverage of 100%).
Drive-by fix 3: Use Mock instead of FakeMethod in src/maasserver/tests/test_signals.py.
--
https://code.launchpad.net/~rvb/maas/bug-1067977-dns-conf/+merge/130958
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/bug-1067977-dns-conf into lp:maas.
=== modified file 'src/maasserver/dhcp_connect.py'
--- src/maasserver/dhcp_connect.py 2012-10-03 07:48:03 +0000
+++ src/maasserver/dhcp_connect.py 2012-10-23 14:56:23 +0000
@@ -31,7 +31,7 @@
configure_dhcp(instance.nodegroup)
-def dhcp_post_edit_status_NodeGroup(instance, old_field):
+def dhcp_post_edit_status_NodeGroup(instance, old_field, **kwargs):
"""The status of a NodeGroup changed."""
# This could be optimized a bit by detecting if the status change is
# actually a change from 'do not manage DHCP' to 'manage DHCP'.
=== modified file 'src/maasserver/dns.py'
--- src/maasserver/dns.py 2012-10-01 16:34:43 +0000
+++ src/maasserver/dns.py 2012-10-23 14:56:23 +0000
@@ -37,6 +37,7 @@
from maasserver.models import (
DHCPLease,
NodeGroup,
+ NodeGroupInterface,
)
from maasserver.sequence import (
INT_MAX,
@@ -64,9 +65,19 @@
return '%0.10d' % zone_serial.nextval()
-def is_dns_enabled():
- """Is MAAS configured to manage DNS?"""
- return settings.DNS_CONNECT
+def is_dns_enabled(check_interfaces=True):
+ """Is MAAS configured to manage DNS?
+
+ :param check_interfaces: Check that this MAAS server has at least
+ one interface configured to manage DNS.
+ :type check_interfaces: bool
+ """
+ interfaces_with_dns = (
+ NodeGroupInterface.objects.filter(
+ management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS))
+ return (
+ settings.DNS_CONNECT and
+ (not check_interfaces or interfaces_with_dns.exists()))
class DNSException(MAASException):
@@ -90,8 +101,7 @@
"loopback network. This may not be a problem if you're not using "
"MAAS's DNS features or if you don't rely on this information. "
"Be sure to configure the DEFAULT_MAAS_URL setting in MAAS's "
- "settings.py (or demo.py/development.py if you are running a "
- "development system)."
+ "/etc/maas/maas_local_settings.py."
% ip)
@@ -286,7 +296,8 @@
zones=zones_to_write, callback=write_dns_config_subtask)
-def write_full_dns_config(active=True, reload_retry=False):
+def write_full_dns_config(active=True, reload_retry=False,
+ force=False):
"""Write the DNS configuration.
:param active: If True, write the DNS config for all the nodegroups.
@@ -296,8 +307,11 @@
:param reload_retry: Should the reload rndc command be retried in case
of failure? Defaults to `False`.
:type reload_retry: bool
+ :param force: Write the configuration even if no interface is
+ configured to manage DNS.
+ :type check_interfaces: bool
"""
- if not is_dns_enabled():
+ if not is_dns_enabled(check_interfaces=not force):
return
if active:
zones = ZoneGenerator(NodeGroup.objects.all()).as_list()
=== modified file 'src/maasserver/dns_connect.py'
--- src/maasserver/dns_connect.py 2012-10-08 12:50:32 +0000
+++ src/maasserver/dns_connect.py 2012-10-23 14:56:23 +0000
@@ -19,6 +19,7 @@
post_save,
)
from django.dispatch import receiver
+from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT
from maasserver.models import (
Node,
NodeGroup,
@@ -50,11 +51,19 @@
write_full_dns_config()
-@receiver(post_delete, sender=NodeGroup)
-def dns_post_delete_NodeGroup(sender, instance, **kwargs):
- """Delete DNS zones related to the nodegroup."""
+def dns_post_edit_management_NodeGroupInterface(instance, old_field, deleted):
+ """Delete DNS zones related to the interface."""
from maasserver.dns import write_full_dns_config
- write_full_dns_config()
+ if old_field == NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS:
+ # Force the dns config to be written as this might have been
+ # triggered by the last DNS-enabled interface being deleted
+ # or switched off (i.e. management set to DHCP or UNMANAGED).
+ write_full_dns_config(force=True)
+
+
+connect_to_field_change(
+ dns_post_edit_management_NodeGroupInterface,
+ NodeGroupInterface, 'management', delete=True)
@receiver(post_delete, sender=Node)
@@ -70,7 +79,7 @@
pass
-def dns_post_edit_hostname_Node(instance, old_field):
+def dns_post_edit_hostname_Node(instance, old_field, **kwargs):
"""When a Node has been flagged, update the related zone."""
from maasserver.dns import change_dns_zones
change_dns_zones(instance.nodegroup)
=== modified file 'src/maasserver/signals.py'
--- src/maasserver/signals.py 2012-08-03 18:57:08 +0000
+++ src/maasserver/signals.py 2012-10-23 14:56:23 +0000
@@ -15,13 +15,15 @@
]
from django.db.models.signals import (
+ post_delete,
post_init,
post_save,
+ pre_delete,
pre_save,
)
-def connect_to_field_change(callback, model, field_name):
+def connect_to_field_change(callback, model, field_name, delete=False):
"""Call the provided callback when a field is modified on a model.
The provided `callback` method will be called when the value of the field
@@ -29,13 +31,24 @@
The signature of the callback method should be the following:
- >>> def callback(instance, old_value):
+ >>> def callback(instance, old_value, deleted):
...
pass
- Where `instance` is the object which has just being saved to the database
- and `old_value` is the old value of the field (different from the value of
- the field in `instance`).
+ Where `instance` is the object which has just being saved to the
+ database, `old_value` is the old value of the field (different from the
+ value of the field in `instance`) and `deleted` indicates whether or not
+ the callback was called because the object was deleted.
+
+ :param callback: The callback function.
+ :type callback: callable
+ :param model: Specifies a particular sender to receive signals from.
+ :type model: class
+ :param field_name: Name of the field to monitor.
+ :type field_name: basestring
+ :param delete: Should the deletion of an object be considered a change
+ in the field?
+ :type delete: bool
"""
last_seen_flag = '_field_last_seen_value_%s' % field_name
delta_flag = '_field_delta_%s' % field_name
@@ -47,17 +60,27 @@
post_init.connect(post_init_callback, sender=model, weak=False)
# Set 'delta_flag' with the new and the old value of the field.
- def pre_save_callback(sender, instance, **kwargs):
+ def record_delta_flag(sender, instance, **kwargs):
original_value = getattr(instance, last_seen_flag)
new_value = getattr(instance, field_name)
setattr(instance, delta_flag, (new_value, original_value))
- pre_save.connect(pre_save_callback, sender=model, weak=False)
+ pre_save.connect(record_delta_flag, sender=model, weak=False)
# Call the `callback` if the field has changed.
def post_save_callback(sender, instance, created, **kwargs):
(new_value, original_value) = getattr(instance, delta_flag)
# Call the callback method is the field has changed.
if original_value != new_value:
- callback(instance, original_value)
+ callback(instance, original_value, deleted=False)
setattr(instance, last_seen_flag, new_value)
+
+ if delete:
+ pre_delete.connect(record_delta_flag, sender=model, weak=False)
+
+ def post_delete_callback(sender, instance, **kwargs):
+ (new_value, original_value) = getattr(instance, delta_flag)
+ callback(instance, original_value, deleted=True)
+
+ post_delete.connect(post_delete_callback, sender=model, weak=False)
+
post_save.connect(post_save_callback, sender=model, weak=False)
=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py 2012-10-11 01:17:24 +0000
+++ src/maasserver/tests/test_dns.py 2012-10-23 14:56:23 +0000
@@ -15,6 +15,7 @@
from functools import partial
from itertools import islice
+import logging
import socket
from celery.task import task
@@ -35,6 +36,7 @@
from maastesting.celery import CeleryFixture
from maastesting.fakemethod import FakeMethod
from maastesting.tests.test_bindfixture import dig_call
+from mock import Mock
from netaddr import (
IPNetwork,
IPRange,
@@ -50,6 +52,8 @@
from rabbitfixture.server import allocate_ports
from testresources import FixtureResource
from testtools.matchers import (
+ Contains,
+ FileContains,
IsInstance,
MatchesAll,
MatchesListwise,
@@ -99,6 +103,19 @@
self.patch_DEFAULT_MAAS_URL_with_random_values()
self.assertRaises(dns.DNSException, dns.get_dns_server_address)
+ def test_get_dns_server_address_logs_warning_if_ip_is_localhost(self):
+ logger = logging.getLogger('maas')
+ logfile = self.make_file(contents="")
+ handler = logging.handlers.RotatingFileHandler(logfile)
+ logger.addHandler(handler)
+ self.addCleanup(logger.removeHandler, handler)
+ self.patch(
+ dns, 'get_maas_facing_server_address',
+ Mock(return_value='127.0.0.1'))
+ dns.get_dns_server_address()
+ warning_text = "The DNS server will use the address '127.0.0.1'"
+ self.assertThat(logfile, FileContains(matcher=Contains(warning_text)))
+
def test_is_dns_managed(self):
nodegroups_with_expected_results = {
factory.make_node_group(
@@ -149,12 +166,15 @@
# Reload BIND.
self.bind.runner.rndc('reload')
+ def create_managed_nodegroup(self):
+ return factory.make_node_group(
+ network=IPNetwork('192.168.0.1/24'),
+ status=NODEGROUP_STATUS.ACCEPTED,
+ management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+
def create_nodegroup_with_lease(self, lease_number=1, nodegroup=None):
if nodegroup is None:
- nodegroup = factory.make_node_group(
- network=IPNetwork('192.168.0.1/24'),
- status=NODEGROUP_STATUS.ACCEPTED,
- management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+ nodegroup = self.create_managed_nodegroup()
interface = nodegroup.get_managed_interface()
node = factory.make_node(
nodegroup=nodegroup, set_hostname=True)
@@ -223,10 +243,17 @@
def test_is_dns_enabled_return_false_if_DNS_CONNECT_False(self):
self.patch(settings, 'DNS_CONNECT', False)
+ self.create_managed_nodegroup()
self.assertFalse(dns.is_dns_enabled())
- def test_is_dns_enabled_return_True(self):
- self.patch(settings, 'DNS_CONNECT', True)
+ def test_is_dns_enabled_return_True_if_DNS_CONNECT_True(self):
+ self.patch(settings, 'DNS_CONNECT', True)
+ self.create_managed_nodegroup()
+ self.assertTrue(dns.is_dns_enabled())
+
+ def test_is_dns_enabled_return_False_no_configured_interface(self):
+ self.patch(settings, 'DNS_CONNECT', True)
+ self.create_managed_nodegroup()
self.assertTrue(dns.is_dns_enabled())
def test_write_full_dns_loads_full_dns_config(self):
@@ -244,6 +271,7 @@
def test_write_full_dns_passes_reload_retry_parameter(self):
self.patch(settings, 'DNS_CONNECT', True)
recorder = FakeMethod()
+ self.create_managed_nodegroup()
@task
def recorder_task(*args, **kwargs):
@@ -253,6 +281,12 @@
self.assertEqual(
([(['reload'], True)]), recorder.extract_args())
+ def test_write_full_dns_doesnt_call_task_it_no_interface_configured(self):
+ self.patch(settings, 'DNS_CONNECT', True)
+ patched_task = self.patch(tasks, 'write_full_dns_config')
+ dns.write_full_dns_config()
+ self.assertEqual(0, patched_task.call_count)
+
def test_dns_config_has_NS_record(self):
ip = factory.getRandomIPAddress()
self.patch(settings, 'DEFAULT_MAAS_URL', 'http://%s/' % ip)
@@ -268,11 +302,6 @@
port=self.bind.config.port, commands=[ns_record, '+short'])
self.assertEqual(ip, ip_of_ns_record)
- def test_is_dns_enabled_follows_DNS_CONNECT(self):
- rand_bool = factory.getRandomBoolean()
- self.patch(settings, "DNS_CONNECT", rand_bool)
- self.assertEqual(rand_bool, dns.is_dns_enabled())
-
def test_add_nodegroup_creates_DNS_zone(self):
self.patch(settings, "DNS_CONNECT", True)
network = IPNetwork('192.168.7.1/24')
@@ -305,6 +334,19 @@
# The ip from the new network resolves.
self.assertDNSMatches(generated_hostname(ip), nodegroup.name, ip)
+ def test_changing_interface_management_updates_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, status=NODEGROUP_STATUS.ACCEPTED,
+ management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+ interface = nodegroup.get_managed_interface()
+ interface.management = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
+ interface.save()
+ self.assertEqual([''], self.dig_resolve(generated_hostname(ip)))
+ self.assertEqual([''], self.dig_reverse_resolve(ip))
+
def test_delete_nodegroup_disables_DNS_zone(self):
self.patch(settings, "DNS_CONNECT", True)
network = IPNetwork('192.168.7.1/24')
=== modified file 'src/maasserver/tests/test_signals.py'
--- src/maasserver/tests/test_signals.py 2012-08-03 18:57:08 +0000
+++ src/maasserver/tests/test_signals.py 2012-10-23 14:56:23 +0000
@@ -16,7 +16,10 @@
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestModelTestCase
from maasserver.tests.models import FieldChangeTestModel
-from maastesting.fakemethod import FakeMethod
+from mock import (
+ call,
+ Mock,
+ )
class ConnectToFieldChangeTest(TestModelTestCase):
@@ -25,7 +28,7 @@
app = 'maasserver.tests'
def test_connect_to_field_change_calls_callback(self):
- callback = FakeMethod()
+ callback = Mock()
connect_to_field_change(callback, FieldChangeTestModel, 'name1')
old_name1_value = factory.getRandomString()
obj = FieldChangeTestModel(name1=old_name1_value)
@@ -33,11 +36,11 @@
obj.name1 = factory.getRandomString()
obj.save()
self.assertEqual(
- (1, [(obj, old_name1_value)]),
- (callback.call_count, callback.extract_args()))
+ (1, call(obj, old_name1_value, deleted=False)),
+ (callback.call_count, callback.call_args))
def test_connect_to_field_change_calls_callback_for_each_save(self):
- callback = FakeMethod()
+ callback = Mock()
connect_to_field_change(callback, FieldChangeTestModel, 'name1')
old_name1_value = factory.getRandomString()
obj = FieldChangeTestModel(name1=old_name1_value)
@@ -49,7 +52,7 @@
self.assertEqual(2, callback.call_count)
def test_connect_to_field_change_calls_callback_for_each_real_save(self):
- callback = FakeMethod()
+ callback = Mock()
connect_to_field_change(callback, FieldChangeTestModel, 'name1')
old_name1_value = factory.getRandomString()
obj = FieldChangeTestModel(name1=old_name1_value)
@@ -60,9 +63,9 @@
self.assertEqual(1, callback.call_count)
def test_connect_to_field_change_calls_multiple_callbacks(self):
- callback1 = FakeMethod()
+ callback1 = Mock()
connect_to_field_change(callback1, FieldChangeTestModel, 'name1')
- callback2 = FakeMethod()
+ callback2 = Mock()
connect_to_field_change(callback2, FieldChangeTestModel, 'name1')
old_name1_value = factory.getRandomString()
obj = FieldChangeTestModel(name1=old_name1_value)
@@ -74,15 +77,35 @@
def test_connect_to_field_change_ignores_changes_to_other_fields(self):
obj = FieldChangeTestModel(name2=factory.getRandomString())
obj.save()
- callback = FakeMethod()
+ callback = Mock()
connect_to_field_change(callback, FieldChangeTestModel, 'name1')
obj.name2 = factory.getRandomString()
obj.save()
self.assertEqual(0, callback.call_count)
def test_connect_to_field_change_ignores_object_creation(self):
- callback = FakeMethod()
+ callback = Mock()
connect_to_field_change(callback, FieldChangeTestModel, 'name1')
obj = FieldChangeTestModel(name1=factory.getRandomString())
obj.save()
self.assertEqual(0, callback.call_count)
+
+ def test_connect_to_field_change_ignores_deletion_by_default(self):
+ obj = FieldChangeTestModel(name2=factory.getRandomString())
+ obj.save()
+ callback = Mock()
+ connect_to_field_change(callback, FieldChangeTestModel, 'name1')
+ obj.delete()
+ self.assertEqual(0, callback.call_count)
+
+ def test_connect_to_field_change_listens_to_deletion_if_delete_True(self):
+ old_name1_value = factory.getRandomString()
+ obj = FieldChangeTestModel(name1=old_name1_value)
+ obj.save()
+ callback = Mock()
+ connect_to_field_change(
+ callback, FieldChangeTestModel, 'name1', delete=True)
+ obj.delete()
+ self.assertEqual(
+ (1, call(obj, old_name1_value, deleted=True)),
+ (callback.call_count, callback.call_args))
