launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12216
[Merge] lp:~rvb/maas/manage-dhcp-confs into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/manage-dhcp-confs into lp:maas.
Requested reviews:
MAAS Maintainers (maas-maintainers)
For more details, see:
https://code.launchpad.net/~rvb/maas/manage-dhcp-confs/+merge/125213
The goal of this branch is to get DHCP config files to be written when nodegroup/nodegroupinterface are edited.
Right now, the only time where the DHCP-conf-writing task is fired is when the command config_master_dhcp is run.
= Pre-imp =
This was discussed with Jeroen.
= Notes =
- I added a DHCP_CONNECT flag to be able to disconnect/enable the DHCP machinery in tests.
- The hooking up is done with signals. This approach (which was discussed with Jeroen) has its pros and cons but fits well with the particular goal that we're trying to achieve here: hooking up an external system (a DHCP server) to that any change made to the data in MAAS (via the API or the UI) trigger the appropriate task to handle the changes to the external system.
- All the DHCP-related code is now in src/maasserver/dhcp.py and I've moved nodegroup.set_up_dhcp there (and renamed it 'configure_dhcp').
- drive-by fix: the master nodegroup should be accepted by default.
- Right now, the tasks are not yet routed to the appropriate worker. So, all the DHCP-conf-writing tasks will be sent off to the master worker. That's why I had to stop that from happening and to that effect, I've create the ugly 'is_dhcp_disabled_until_task_routing_in_place' which will stop all the DHCP-conf-writing tasks from being fired but the ones related to the master nodegroup. Once tasks are routed correctly (bug 1039366), that method will need to be removed.
--
https://code.launchpad.net/~rvb/maas/manage-dhcp-confs/+merge/125213
Your team MAAS Maintainers is requested to review the proposed merge of lp:~rvb/maas/manage-dhcp-confs into lp:maas.
=== modified file 'src/maas/demo.py'
--- src/maas/demo.py 2012-08-16 08:24:49 +0000
+++ src/maas/demo.py 2012-09-19 14:13:23 +0000
@@ -39,6 +39,8 @@
# Connect to the DNS server.
DNS_CONNECT = True
+DHCP_CONNECT = True
+
MAAS_CLI = abspath("bin/maas")
RABBITMQ_PUBLISH = True
=== modified file 'src/maas/development.py'
--- src/maas/development.py 2012-08-16 08:24:49 +0000
+++ src/maas/development.py 2012-09-19 14:13:23 +0000
@@ -41,6 +41,10 @@
# case basis.
DNS_CONNECT = False
+# Don't setup DHCP servers in tests, this will be enabled on a case per case
+# basis.
+DHCP_CONNECT = False
+
# Invalid strings should be visible.
TEMPLATE_STRING_IF_INVALID = '#### INVALID STRING ####'
=== modified file 'src/maas/settings.py'
--- src/maas/settings.py 2012-09-12 06:59:40 +0000
+++ src/maas/settings.py 2012-09-19 14:13:23 +0000
@@ -46,12 +46,16 @@
LOGIN_REDIRECT_URL = '/'
LOGIN_URL = '/accounts/login/'
-# Should the DNS features be enabled? Note that the MAAS' DNS features can
-# also 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.
+# Should the DNS features be enabled? Having this config option is a
+# debugging/testing feature to be able to quickly disconnect the DNS
+# machinery.
DNS_CONNECT = True
+# Should the DHCP features be enabled? Having this config option is a
+# debugging/testing feature to be able to quickly disconnect the DNS
+# machinery.
+DHCP_CONNECT = True
+
# The MAAS CLI.
MAAS_CLI = 'sudo maas'
=== added file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/dhcp.py 2012-09-19 14:13:23 +0000
@@ -0,0 +1,86 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""DHCP management module."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ 'configure_dhcp',
+ ]
+
+from django.conf import settings
+from maasserver.enum import (
+ NODEGROUP_STATUS,
+ NODEGROUPINTERFACE_MANAGEMENT,
+ )
+from maasserver.models import NodeGroup
+from maasserver.server_address import get_maas_facing_server_address
+from netaddr import IPAddress
+from provisioningserver.tasks import write_dhcp_config
+
+
+def is_dhcp_disabled_until_task_routing_in_place(nodegroup):
+ """Until proper task routing is in place, disable DHCP for non-master
+ nodegroups.
+
+ # XXX: rvb 2012-09-19 bug=1039366: Tasks are not routed yet.
+ Until proper task routing is in place, the only DHCP config which can be
+ written is the one for the master nodegroup.
+ """
+ if nodegroup == NodeGroup.objects.ensure_master():
+ return False
+ else:
+ return True
+
+
+def is_dhcp_managed(nodegroup):
+ """Does MAAS manage the DHCP server for this Nodegroup?"""
+ interface = nodegroup.get_managed_interface()
+ return (
+ settings.DHCP_CONNECT and
+ nodegroup.status == NODEGROUP_STATUS.ACCEPTED and
+ interface is not None and
+ interface.management in (
+ NODEGROUPINTERFACE_MANAGEMENT.DHCP,
+ NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS))
+
+
+def configure_dhcp(nodegroup):
+ """Write the DHCP configuration file and restart the DHCP server."""
+ if not is_dhcp_managed(nodegroup):
+ return
+
+ # XXX: rvb 2012-09-19 bug=1039366: Tasks are not routed yet.
+ if is_dhcp_disabled_until_task_routing_in_place(nodegroup):
+ return
+
+ # Circular imports.
+ from maasserver.dns import get_dns_server_address
+
+ # Use the server's address (which is where the central TFTP
+ # server is) for the next_server setting. We'll want to proxy
+ # it on the local worker later, and then we can use
+ # next_server=self.worker_ip.
+ next_server = get_maas_facing_server_address()
+
+ interface = nodegroup.get_managed_interface()
+ subnet = str(
+ IPAddress(interface.ip_range_low) &
+ IPAddress(interface.subnet_mask))
+ # XXX: rvb 2012-09-19 bug=1039366: Tasks are not routed yet.
+ # Once task routing is in place, the method
+ # is_dhcp_disabled_until_task_routing_in_place should be removed.
+ write_dhcp_config.delay(
+ subnet=subnet, next_server=next_server, omapi_key=nodegroup.dhcp_key,
+ subnet_mask=interface.subnet_mask,
+ broadcast_ip=interface.broadcast_ip,
+ router_ip=interface.router_ip,
+ dns_servers=get_dns_server_address(),
+ ip_range_low=interface.ip_range_low,
+ ip_range_high=interface.ip_range_high)
=== added file 'src/maasserver/dhcp_connect.py'
--- src/maasserver/dhcp_connect.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/dhcp_connect.py 2012-09-19 14:13:23 +0000
@@ -0,0 +1,41 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""DHCP management module: connect DHCP tasks with signals."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ ]
+
+
+from django.db.models.signals import post_save
+from django.dispatch import receiver
+from maasserver.models import (
+ NodeGroup,
+ NodeGroupInterface,
+ )
+from maasserver.signals import connect_to_field_change
+
+
+@receiver(post_save, sender=NodeGroupInterface)
+def dns_post_save_NodeGroupInterface(sender, instance, created, **kwargs):
+ """Update the DHCP config related to the saved nodegroupinterface."""
+ from maasserver.dhcp import configure_dhcp
+ configure_dhcp(instance.nodegroup)
+
+
+def dhcp_post_edit_status_NodeGroup(instance, old_field):
+ """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'.
+ from maasserver.dhcp import configure_dhcp
+ configure_dhcp(instance)
+
+
+connect_to_field_change(dhcp_post_edit_status_NodeGroup, NodeGroup, 'status')
=== modified file 'src/maasserver/management/commands/config_master_dhcp.py'
--- src/maasserver/management/commands/config_master_dhcp.py 2012-09-18 16:36:51 +0000
+++ src/maasserver/management/commands/config_master_dhcp.py 2012-09-19 14:13:23 +0000
@@ -105,6 +105,3 @@
for item, value in settings.items():
setattr(interface, item, value)
interface.save()
-
- # Create a Task that will write the config out.
- master_nodegroup.set_up_dhcp()
=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py 2012-09-13 10:16:33 +0000
+++ src/maasserver/models/__init__.py 2012-09-19 14:13:23 +0000
@@ -111,3 +111,6 @@
from maasserver import dns_connect
ignore_unused(dns_connect)
+
+from maasserver import dhcp_connect
+ignore_unused(dhcp_connect)
=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py 2012-09-18 16:36:51 +0000
+++ src/maasserver/models/nodegroup.py 2012-09-19 14:13:23 +0000
@@ -30,18 +30,13 @@
from maasserver.models.nodegroupinterface import NodeGroupInterface
from maasserver.models.timestampedmodel import TimestampedModel
from maasserver.refresh_worker import refresh_worker
-from maasserver.server_address import get_maas_facing_server_address
from maasserver.utils.orm import get_one
-from netaddr import IPAddress
from piston.models import (
KEY_SIZE,
Token,
)
from provisioningserver.omshell import generate_omapi_key
-from provisioningserver.tasks import (
- add_new_dhcp_host_map,
- write_dhcp_config,
- )
+from provisioningserver.tasks import add_new_dhcp_host_map
class NodeGroupManager(Manager):
@@ -54,6 +49,7 @@
def new(self, name, uuid, ip, subnet_mask=None,
broadcast_ip=None, router_ip=None, ip_range_low=None,
ip_range_high=None, dhcp_key='', interface='',
+ status=NODEGROUP_STATUS.DEFAULT_STATUS,
management=NODEGROUPINTERFACE_MANAGEMENT.DEFAULT):
"""Create a :class:`NodeGroup` with the given parameters.
@@ -72,7 +68,8 @@
assert all(dhcp_values) or not any(dhcp_values), (
"Provide all DHCP settings, or none at all.")
- nodegroup = NodeGroup(name=name, uuid=uuid, dhcp_key=dhcp_key)
+ nodegroup = NodeGroup(
+ name=name, uuid=uuid, dhcp_key=dhcp_key, status=status)
nodegroup.save()
nginterface = NodeGroupInterface(
nodegroup=nodegroup, ip=ip, subnet_mask=subnet_mask,
@@ -92,7 +89,8 @@
except NodeGroup.DoesNotExist:
# The master did not exist yet; create it on demand.
master = self.new(
- 'master', 'master', '127.0.0.1', dhcp_key=generate_omapi_key())
+ 'master', 'master', '127.0.0.1', dhcp_key=generate_omapi_key(),
+ status=NODEGROUP_STATUS.ACCEPTED)
# If any legacy nodes were still not associated with a node
# group, enroll them in the master node group.
@@ -173,30 +171,6 @@
nodegroup=self).exclude(
management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED))
- def set_up_dhcp(self):
- """Write the DHCP configuration file and restart the DHCP server."""
- # Circular imports.
- from maasserver.dns import get_dns_server_address
-
- # Use the server's address (which is where the central TFTP
- # server is) for the next_server setting. We'll want to proxy
- # it on the local worker later, and then we can use
- # next_server=self.worker_ip.
- next_server = get_maas_facing_server_address()
-
- interface = self.get_managed_interface()
- subnet = str(
- IPAddress(interface.ip_range_low) &
- IPAddress(interface.subnet_mask))
- write_dhcp_config.delay(
- subnet=subnet, next_server=next_server, omapi_key=self.dhcp_key,
- subnet_mask=interface.subnet_mask,
- broadcast_ip=interface.broadcast_ip,
- router_ip=interface.router_ip,
- dns_servers=get_dns_server_address(),
- ip_range_low=interface.ip_range_low,
- ip_range_high=interface.ip_range_high)
-
def add_dhcp_host_maps(self, new_leases):
if self.get_managed_interface() is not None and len(new_leases) > 0:
# XXX JeroenVermeulen 2012-08-21, bug=1039362: the DHCP
=== modified file 'src/maasserver/tests/test_commands_config_master_dhcp.py'
--- src/maasserver/tests/test_commands_config_master_dhcp.py 2012-09-18 16:36:51 +0000
+++ src/maasserver/tests/test_commands_config_master_dhcp.py 2012-09-19 14:13:23 +0000
@@ -14,7 +14,9 @@
from optparse import OptionValueError
+from django.conf import settings
from django.core.management import call_command
+from maasserver import dhcp
from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT
from maasserver.management.commands.config_master_dhcp import name_option
from maasserver.models import NodeGroup
@@ -55,7 +57,8 @@
def setUp(self):
super(TestConfigMasterDHCP, self).setUp()
# Make sure any attempts to write a dhcp config do nothing.
- self.patch(NodeGroup, 'set_up_dhcp')
+ self.patch(dhcp, 'configure_dhcp')
+ self.patch(settings, 'DHCP_CONNECT', True)
def test_configures_dhcp_for_master_nodegroup(self):
settings = make_dhcp_settings()
@@ -134,8 +137,9 @@
def test_name_option_turns_dhcp_setting_name_into_option(self):
self.assertEqual('--subnet-mask', name_option('subnet_mask'))
- def test_sets_up_dhcp(self):
- master = NodeGroup.objects.ensure_master()
+ def test_configures_dhcp(self):
+ NodeGroup.objects.ensure_master()
+ self.patch(dhcp, 'configure_dhcp')
settings = make_dhcp_settings()
call_command('config_master_dhcp', **settings)
- self.assertEqual(1, master.set_up_dhcp.call_count)
+ self.assertEqual(1, dhcp.configure_dhcp.call_count)
=== added file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_dhcp.py 2012-09-19 14:13:23 +0000
@@ -0,0 +1,138 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for DHCP management."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = []
+
+from django.conf import settings
+from maasserver import dhcp
+from maasserver.dhcp import (
+ configure_dhcp,
+ is_dhcp_managed,
+ )
+from maasserver.dns import get_dns_server_address
+from maasserver.enum import (
+ NODEGROUP_STATUS,
+ NODEGROUPINTERFACE_MANAGEMENT,
+ )
+from maasserver.models import NodeGroup
+from maasserver.server_address import get_maas_facing_server_address
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase
+from maastesting.celery import CeleryFixture
+from mock import Mock
+from testresources import FixtureResource
+
+
+class TestDHCP(TestCase):
+
+ resources = (
+ ('celery', FixtureResource(CeleryFixture())),
+ )
+
+ def setUp(self):
+ super(TestDHCP, self).setUp()
+ # XXX: rvb 2012-09-19 bug=1039366: Tasks are not routed yet.
+ # This call to self.patch() can be removed once the bug referenced
+ # above will be fixed.
+ self.patch(
+ dhcp, 'is_dhcp_disabled_until_task_routing_in_place',
+ Mock(return_value=False))
+
+ def test_is_dhcp_managed_returns_False_for_pending_nodegroup(self):
+ nodegroup = factory.make_node_group(
+ status=NODEGROUP_STATUS.PENDING)
+ self.assertFalse(is_dhcp_managed(nodegroup))
+
+ def test_is_dhcp_managed_returns_False_for_rejected_nodegroup(self):
+ nodegroup = factory.make_node_group(
+ status=NODEGROUP_STATUS.REJECTED)
+ self.assertFalse(is_dhcp_managed(nodegroup))
+
+ def test_configure_dhcp_writes_dhcp_config(self):
+ mocked_task = self.patch(dhcp, 'write_dhcp_config')
+ self.patch(
+ settings, 'DEFAULT_MAAS_URL',
+ 'http://%s/' % factory.getRandomIPAddress())
+ nodegroup = factory.make_node_group(
+ status=NODEGROUP_STATUS.ACCEPTED,
+ dhcp_key=factory.getRandomString(),
+ ip_range_low='192.168.102.1', ip_range_high='192.168.103.254',
+ subnet_mask='255.255.252.0', broadcast_ip='192.168.103.255')
+
+ self.patch(settings, "DHCP_CONNECT", True)
+ configure_dhcp(nodegroup)
+ dhcp_params = [
+ 'subnet_mask', 'broadcast_ip', 'router_ip',
+ 'ip_range_low', 'ip_range_high']
+
+ interface = nodegroup.get_managed_interface()
+ expected_params = {
+ param: getattr(interface, param)
+ for param in dhcp_params}
+
+ # Currently all nodes use the central TFTP server. This will be
+ # decentralized to use NodeGroup.worker_ip later.
+ expected_params["next_server"] = get_maas_facing_server_address()
+
+ expected_params["omapi_key"] = nodegroup.dhcp_key
+ expected_params["dns_servers"] = get_dns_server_address()
+ expected_params["subnet"] = '192.168.100.0'
+
+ mocked_task.delay.assert_called_once_with(**expected_params)
+
+ def test_dhcp_config_gets_written_when_nodegroup_becomes_active(self):
+ nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.PENDING)
+ self.patch(settings, "DHCP_CONNECT", True)
+ self.patch(dhcp, 'write_dhcp_config')
+ nodegroup.accept()
+ self.assertEqual(1, dhcp.write_dhcp_config.delay.call_count)
+
+ def test_dhcp_config_gets_written_when_nodegroupinterface_changes(self):
+ nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
+ interface = nodegroup.get_managed_interface()
+ self.patch(settings, "DHCP_CONNECT", True)
+ self.patch(dhcp, 'write_dhcp_config')
+ new_router_ip = factory.getRandomIPAddress()
+ interface.router_ip = new_router_ip
+ interface.save()
+ self.assertEqual(
+ (1, new_router_ip),
+ (
+ dhcp.write_dhcp_config.delay.call_count,
+ dhcp.write_dhcp_config.delay.call_args[1]['router_ip'],
+ ))
+
+
+class TestDHCPDisabledMultipleNodegroup(TestCase):
+ """Writing DHCP config files is disabled for non-master Nodegroups.
+
+ # XXX: rvb 2012-09-19 bug=1039366: Tasks are not routed yet.
+ These tests could be removed once proper routing is in place.
+ """
+
+ def test_dhcp_config_does_not_get_written_for_non_master_nodegroup(self):
+ nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.PENDING)
+ self.patch(settings, "DHCP_CONNECT", True)
+ self.patch(dhcp, 'write_dhcp_config')
+ nodegroup.accept()
+ self.assertEqual(0, dhcp.write_dhcp_config.delay.call_count)
+
+ def test_dhcp_config_gets_written_for_master_nodegroup(self):
+ nodegroup = NodeGroup.objects.ensure_master()
+ interface = nodegroup.nodegroupinterface_set.all()[0]
+ self.patch(settings, "DHCP_CONNECT", True)
+ self.patch(dhcp, 'write_dhcp_config')
+ interface.management = NODEGROUPINTERFACE_MANAGEMENT.DHCP
+ interface.ip_range_low = factory.getRandomIPAddress()
+ interface.subnet_mask = factory.getRandomIPAddress()
+ interface.save()
+ self.assertEqual(1, dhcp.write_dhcp_config.delay.call_count)
=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py 2012-09-18 16:36:51 +0000
+++ src/maasserver/tests/test_nodegroup.py 2012-09-19 14:13:23 +0000
@@ -12,15 +12,11 @@
__metaclass__ = type
__all__ = []
-from django.conf import settings
-import maasserver
-from maasserver.dns import get_dns_server_address
from maasserver.enum import (
NODEGROUP_STATUS,
NODEGROUPINTERFACE_MANAGEMENT,
)
from maasserver.models import NodeGroup
-from maasserver.server_address import get_maas_facing_server_address
from maasserver.testing import reload_object
from maasserver.testing.factory import factory
from maasserver.testing.testcase import TestCase
@@ -156,6 +152,10 @@
master.save()
self.assertEqual(key, NodeGroup.objects.ensure_master().dhcp_key)
+ def test_ensure_master_creates_accepted_nodegroup(self):
+ master = NodeGroup.objects.ensure_master()
+ self.assertEqual(NODEGROUP_STATUS.ACCEPTED, master.status)
+
def test_get_by_natural_key_looks_up_by_uuid(self):
nodegroup = factory.make_node_group()
self.assertEqual(
@@ -176,36 +176,6 @@
('celery', FixtureResource(CeleryFixture())),
)
- def test_set_up_dhcp_writes_dhcp_config(self):
- mocked_task = self.patch(
- maasserver.models.nodegroup, 'write_dhcp_config')
- self.patch(
- settings, 'DEFAULT_MAAS_URL',
- 'http://%s/' % factory.getRandomIPAddress())
- nodegroup = factory.make_node_group(
- dhcp_key=factory.getRandomString(),
- ip_range_low='192.168.102.1', ip_range_high='192.168.103.254',
- subnet_mask='255.255.252.0', broadcast_ip='192.168.103.255')
- nodegroup.set_up_dhcp()
- dhcp_params = [
- 'subnet_mask', 'broadcast_ip', 'router_ip',
- 'ip_range_low', 'ip_range_high']
-
- interface = nodegroup.get_managed_interface()
- expected_params = {
- param: getattr(interface, param)
- for param in dhcp_params}
-
- # Currently all nodes use the central TFTP server. This will be
- # decentralized to use NodeGroup.worker_ip later.
- expected_params["next_server"] = get_maas_facing_server_address()
-
- expected_params["omapi_key"] = nodegroup.dhcp_key
- expected_params["dns_servers"] = get_dns_server_address()
- expected_params["subnet"] = '192.168.100.0'
-
- mocked_task.delay.assert_called_once_with(**expected_params)
-
def test_add_dhcp_host_maps_adds_maps_if_managing_dhcp(self):
self.patch(Omshell, 'create', FakeMethod())
nodegroup = factory.make_node_group()
