Merge lp:~rvb/maas/dhcp-multiple-intf into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Generate a multiple-network DHCP config.

This branch changes the DHCP-config writing code so that it can write multiple-network DHCP configs. I've tested that there is no regression by building a package from this branch and testing it in the lab.

Now, we're still in the process of testing if it's worth adding an "interface <intf>" statement inside each subnet definition. If it works, we will fix this as a follow-up branch.

[Jeroen T. Vermeulen (jtv)]

Thanks! In a way I think the code got simpler here — the network information is now more encapsulated.

A small note: in rc/provisioningserver/dhcp/config.py (starting around line 45) you now have...

    except KeyError as error:
        raise DHCPConfigError(*error.args)
    except NameError as error:
        raise DHCPConfigError(*error.args)

You can write that as simply:

    except (KeyError, NameError) as error:
        raise DHCPConfigError(*error.args)

Python's authors have thought of these things. :-)

The big loop in src/maasserver/dhcp.py (starting around line 60) looks like it now wants to be a list comprehension, with its body extracted into a separate function. But no biggie.

review: Approve

[Raphaël Badin (rvb)]

Thanks for the review!

> A small note: in rc/provisioningserver/dhcp/config.py (starting around line
> 45) you now have...
>
> except KeyError as error:
> raise DHCPConfigError(*error.args)
> except NameError as error:
> raise DHCPConfigError(*error.args)
>
> You can write that as simply:
>
> except (KeyError, NameError) as error:
> raise DHCPConfigError(*error.args)
>

Right, fixed (I think I was a bit tired when I wrote this code ;))

> The big loop in src/maasserver/dhcp.py (starting around line 60) looks like it
> now wants to be a list comprehension, with its body extracted into a separate
> function. But no biggie.

Actually, a list comprehension will be enough to make the code a tad more simple. Fixed.

=== modified file 'etc/maas/templates/dhcp/dhcpd.conf.template'
--- etc/maas/templates/dhcp/dhcpd.conf.template	2013-11-11 06:31:34 +0000
+++ etc/maas/templates/dhcp/dhcpd.conf.template	2014-01-29 08:28:06 +0000
@@ -5,24 +5,27 @@
 # will be present whenever MAAS rewrites the DHCP configuration.  Edit and save
 # the nodegroup's configuration in MAAS to trigger an update.
 
-subnet {{subnet}} netmask {{subnet_mask}} {
+{{for dhcp_subnet in dhcp_subnets}}
+subnet {{dhcp_subnet['subnet']}} netmask {{dhcp_subnet['subnet_mask']}} {
        filename "{{bootloader}}";
        ignore-client-uids true;
-       option subnet-mask {{subnet_mask}};
-       option broadcast-address {{broadcast_ip}};
-       option domain-name-servers {{dns_servers}};
-       option domain-name "{{domain_name}}";
-       option routers {{router_ip}};
-       {{if ntp_server}}
-       option ntp-servers {{ntp_server}};
+       option subnet-mask {{dhcp_subnet['subnet_mask']}};
+       option broadcast-address {{dhcp_subnet['broadcast_ip']}};
+       option domain-name-servers {{dhcp_subnet['dns_servers']}};
+       option domain-name "{{dhcp_subnet['domain_name']}}";
+       option routers {{dhcp_subnet['router_ip']}};
+       {{if dhcp_subnet.get('ntp_server')}}
+       option ntp-servers {{dhcp_subnet['ntp_server']}};
        {{endif}}
-       range dynamic-bootp {{ip_range_low}} {{ip_range_high}};
+       range dynamic-bootp {{dhcp_subnet['ip_range_low']}} {{dhcp_subnet['ip_range_high']}};
        class "PXE" {
           match if substring (option vendor-class-identifier, 0, 3) = "PXE";
           default-lease-time 30;
           max-lease-time 30;
        }
 }
+{{endfor}}
+
 omapi-port 7911;
 key omapi_key {
     algorithm HMAC-MD5;

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2014-01-17 09:57:37 +0000
+++ src/maasserver/dhcp.py	2014-01-29 08:28:06 +0000
@@ -17,9 +17,7 @@
     ]
 
 from django.conf import settings
-from maasserver.enum import (
-    NODEGROUP_STATUS,
-    )
+from maasserver.enum import NODEGROUP_STATUS
 from maasserver.models import Config
 from netaddr import IPAddress
 from provisioningserver.tasks import (
@@ -55,25 +53,33 @@
     # server.
     nodegroup.ensure_dhcp_key()
 
-    for interface in interfaces:
-        subnet = unicode(
-            IPAddress(interface.ip_range_low) &
-            IPAddress(interface.subnet_mask))
-        reload_dhcp_server_subtask = restart_dhcp_server.subtask(
-            options={'queue': nodegroup.work_queue})
-        task_kwargs = dict(
-            subnet=subnet,
-            omapi_key=nodegroup.dhcp_key,
+    dns_server = get_dns_server_address(nodegroup)
+    ntp_server = Config.objects.get_config("ntp_server")
+    dhcp_subnet_configs = []
+    dhcp_subnet_configs = [
+        dict(
+            subnet=unicode(
+                IPAddress(interface.ip_range_low) &
+                IPAddress(interface.subnet_mask)),
             subnet_mask=interface.subnet_mask,
-            dhcp_interfaces=interface.interface,
             broadcast_ip=interface.broadcast_ip,
             router_ip=interface.router_ip,
-            dns_servers=get_dns_server_address(nodegroup),
-            ntp_server=Config.objects.get_config("ntp_server"),
+            dns_servers=dns_server,
+            ntp_server=ntp_server,
             domain_name=nodegroup.name,
             ip_range_low=interface.ip_range_low,
             ip_range_high=interface.ip_range_high,
-            callback=reload_dhcp_server_subtask,
         )
-        write_dhcp_config.apply_async(
-            queue=nodegroup.work_queue, kwargs=task_kwargs)
+        for interface in interfaces]
+
+    reload_dhcp_server_subtask = restart_dhcp_server.subtask(
+        options={'queue': nodegroup.work_queue})
+    task_kwargs = dict(
+        dhcp_subnets=dhcp_subnet_configs,
+        omapi_key=nodegroup.dhcp_key,
+        dhcp_interfaces=' '.join(
+            [interface.interface for interface in interfaces]),
+        callback=reload_dhcp_server_subtask,
+    )
+    write_dhcp_config.apply_async(
+        queue=nodegroup.work_queue, kwargs=task_kwargs)

=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2014-01-27 12:48:10 +0000
+++ src/maasserver/tests/test_dhcp.py	2014-01-29 08:28:06 +0000
@@ -24,14 +24,20 @@
     get_interfaces_managed_by,
     )
 from maasserver.dns import get_dns_server_address
-from maasserver.enum import NODEGROUP_STATUS
+from maasserver.enum import (
+    NODEGROUP_STATUS,
+    NODEGROUPINTERFACE_MANAGEMENT,
+    )
 from maasserver.models import Config
 from maasserver.models.config import get_default_config
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils import map_enum
 from maastesting.celery import CeleryFixture
-from netaddr import IPNetwork
+from netaddr import (
+    IPAddress,
+    IPNetwork,
+    )
 from provisioningserver import tasks
 from testresources import FixtureResource
 from testtools.matchers import EndsWith
@@ -72,38 +78,62 @@
             {status: [] for status in unaccepted_statuses},
             managed_interfaces)
 
+    def enable_multiple_managed_interfaces(self):
+        # XXX: rvb 2012-09-18 bug=1052339: Only one "managed" interface
+        # is supported per NodeGroup.
+        # This can be removed once NodeGroupInterface.clean_management
+        # goes away.
+        from maasserver.models.nodegroupinterface import NodeGroupInterface
+        self.patch(NodeGroupInterface, 'clean_management')
+
     def test_configure_dhcp_writes_dhcp_config(self):
+        self.enable_multiple_managed_interfaces()
         mocked_task = self.patch(dhcp, 'write_dhcp_config')
         self.patch(
             settings, 'DEFAULT_MAAS_URL',
             'http://%s/' % factory.getRandomIPAddress())
+        self.patch(settings, "DHCP_CONNECT", True)
         nodegroup = factory.make_node_group(
             status=NODEGROUP_STATUS.ACCEPTED,
             dhcp_key=factory.getRandomString(),
             interface=factory.make_name('eth'),
             network=IPNetwork("192.168.102.0/22"))
+        # Create a second DHCP-managed interface.
+        factory.make_node_group_interface(
+            nodegroup=nodegroup,
+            interface=factory.make_name('eth'),
+            network=IPNetwork("10.1.1/24"),
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP,
+            )
 
-        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_interfaces()
-        expected_params = {
-            param: getattr(interface, param)
-            for param in dhcp_params}
-
+        dhcp_subnets = []
+        for interface in nodegroup.get_managed_interfaces():
+            dhcp_params = [
+                'subnet_mask',
+                'broadcast_ip',
+                'router_ip',
+                'ip_range_low',
+                'ip_range_high',
+                ]
+
+            dhcp_subnet = {
+                param: getattr(interface, param)
+                for param in dhcp_params}
+            dhcp_subnet["dns_servers"] = get_dns_server_address()
+            dhcp_subnet["ntp_server"] = get_default_config()['ntp_server']
+            dhcp_subnet["domain_name"] = nodegroup.name
+            dhcp_subnet["subnet"] = unicode(
+                IPAddress(interface.ip_range_low) &
+                IPAddress(interface.subnet_mask))
+            dhcp_subnets.append(dhcp_subnet)
+
+        expected_params = {}
         expected_params["omapi_key"] = nodegroup.dhcp_key
-        expected_params["dns_servers"] = get_dns_server_address()
-        expected_params["ntp_server"] = get_default_config()['ntp_server']
-        expected_params["domain_name"] = nodegroup.name
-        expected_params["subnet"] = '192.168.100.0'
-        expected_params["dhcp_interfaces"] = interface.interface
+        expected_params["dhcp_interfaces"] = ' '.join([
+            interface.interface
+            for interface in nodegroup.get_managed_interfaces()])
+        expected_params["dhcp_subnets"] = dhcp_subnets
 
         args, kwargs = mocked_task.apply_async.call_args
         result_params = kwargs['kwargs']
@@ -127,7 +157,7 @@
         configure_dhcp(nodegroup)
         kwargs = mocked_task.apply_async.call_args[1]['kwargs']
 
-        self.assertEqual(ip, kwargs['dns_servers'])
+        self.assertEqual(ip, kwargs['dhcp_subnets'][0]['dns_servers'])
 
     def test_configure_dhcp_restart_dhcp_server(self):
         self.patch(tasks, "sudo_write_file")
@@ -200,7 +230,7 @@
             (1, new_router_ip),
             (
                 dhcp.write_dhcp_config.apply_async.call_count,
-                kwargs['kwargs']['router_ip'],
+                kwargs['kwargs']['dhcp_subnets'][0]['router_ip'],
             ))
 
     def test_dhcp_config_gets_written_when_ntp_server_changes(self):

=== modified file 'src/provisioningserver/dhcp/config.py'
--- src/provisioningserver/dhcp/config.py	2013-11-11 06:31:34 +0000
+++ src/provisioningserver/dhcp/config.py	2014-01-29 08:28:06 +0000
@@ -33,23 +33,7 @@
 
 
 def get_config(**params):
-    """Return a DHCP config file based on the supplied parameters.
-
-    :param subnet: The base subnet declaration. e.g. 192.168.1.0
-    :param subnet_mask: The mask for the above subnet, e.g. 255.255.255.0
-    :param broadcast_address: The broadcast IP address for the subnet,
-        e.g. 192.168.1.255
-    :param dns_servers: One or more IP addresses of the DNS server for the
-        subnet
-    :param ntp_server: IP address of the NTP server for the nodes
-    :param domain_name: name that will be appended to the client's hostname to
-        form a fully-qualified domain-name
-    :param gateway: The router/gateway IP address for the subnet.
-    :param low_range: The first IP address in the range of IP addresses to
-        allocate
-    :param high_range: The last IP address in the range of IP addresses to
-        allocate
-    """
+    """Return a DHCP config file based on the supplied parameters."""
     template_file = locate_config(TEMPLATES_DIR, 'dhcpd.conf.template')
     params['bootloader'] = compose_bootloader_path()
     params['platform_codename'] = linux_distribution()[2]
@@ -57,6 +41,6 @@
     try:
         template = tempita.Template.from_filename(
             template_file, encoding="UTF-8")
-        return template.substitute(params)
-    except NameError as error:
+        return template.substitute(**params)
+    except (KeyError, NameError) as error:
         raise DHCPConfigError(*error.args)

=== modified file 'src/provisioningserver/dhcp/tests/test_config.py'
--- src/provisioningserver/dhcp/tests/test_config.py	2014-01-21 08:28:39 +0000
+++ src/provisioningserver/dhcp/tests/test_config.py	2014-01-29 08:28:06 +0000
@@ -35,14 +35,16 @@
 # substitutions, but none that aren't also in the real template.
 sample_template = dedent("""\
     {{omapi_key}}
-    {{subnet}}
-    {{subnet_mask}}
-    {{broadcast_ip}}
-    {{dns_servers}}
-    {{domain_name}}
-    {{router_ip}}
-    {{ip_range_low}}
-    {{ip_range_high}}
+    {{for dhcp_subnet in dhcp_subnets}}
+        {{dhcp_subnet['subnet']}}
+        {{dhcp_subnet['subnet_mask']}}
+        {{dhcp_subnet['broadcast_ip']}}
+        {{dhcp_subnet['dns_servers']}}
+        {{dhcp_subnet['domain_name']}}
+        {{dhcp_subnet['router_ip']}}
+        {{dhcp_subnet['ip_range_low']}}
+        {{dhcp_subnet['ip_range_high']}}
+    {{endfor}}
 """)
 
 
@@ -53,15 +55,17 @@
     """
     return dict(
         omapi_key="random",
-        subnet="10.0.0.0",
-        subnet_mask="255.0.0.0",
-        broadcast_ip="10.255.255.255",
-        dns_servers="10.1.0.1 10.1.0.2",
-        ntp_server="8.8.8.8",
-        domain_name="example.com",
-        router_ip="10.0.0.2",
-        ip_range_low="10.0.0.3",
-        ip_range_high="10.0.0.254",
+        dhcp_subnets=[dict(
+            subnet="10.0.0.0",
+            subnet_mask="255.0.0.0",
+            broadcast_ip="10.255.255.255",
+            dns_servers="10.1.0.1 10.1.0.2",
+            ntp_server="8.8.8.8",
+            domain_name="example.com",
+            router_ip="10.0.0.2",
+            ip_range_low="10.0.0.3",
+            ip_range_high="10.0.0.254",
+            )]
         )
 
 
@@ -98,14 +102,14 @@
     def test_get_config_with_too_few_parameters(self):
         template = self.patch_template()
         params = make_sample_params()
-        del params['subnet']
+        del params['dhcp_subnets'][0]['subnet']
 
         e = self.assertRaises(
             config.DHCPConfigError, config.get_config, **params)
 
         self.assertThat(
             unicode(e), MatchesRegex(
-                "name 'subnet' is not defined at line \d+ column \d+ "
+                "subnet at line \d+ column \d+ "
                 "in file %s" % template.name))
 
     def test_config_refers_to_bootloader(self):
@@ -115,7 +119,7 @@
 
     def test_renders_without_ntp_servers_set(self):
         params = make_sample_params()
-        del params['ntp_server']
+        del params['dhcp_subnets'][0]['ntp_server']
         template = self.patch_template()
         rendered = template.substitute(params)
         self.assertEqual(rendered, config.get_config(**params))

=== modified file 'src/provisioningserver/dhcp/writer.py'
--- src/provisioningserver/dhcp/writer.py	2013-11-11 06:24:19 +0000
+++ src/provisioningserver/dhcp/writer.py	2014-01-29 08:28:06 +0000
@@ -72,9 +72,15 @@
 def run(args):
     """Generate a DHCP server configuration, and write it to stdout."""
     params = vars(args)
-    output = config.get_config(**params).encode("ascii")
-    if args.outfile is None:
+    omapi_key = params.pop('omapi_key')
+    outfile = params.pop('outfile')
+    kwargs = {
+        'dhcp_subnets': [params],
+        'omapi_key': omapi_key,
+    }
+    output = config.get_config(**kwargs).encode("ascii")
+    if outfile is None:
         sys.stdout.write(output)
     else:
-        with open(args.outfile, "wb") as out:
+        with open(outfile, "wb") as out:
             out.write(output)

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2014-01-22 03:54:03 +0000
+++ src/provisioningserver/tests/test_tasks.py	2014-01-29 08:28:06 +0000
@@ -177,7 +177,6 @@
         """Fake up a dict of dhcp configuration parameters."""
         param_names = [
             'interface',
-            'omapi_key',
             'subnet',
             'subnet_mask',
             'broadcast_ip',
@@ -186,8 +185,13 @@
             'router_ip',
             'ip_range_low',
             'ip_range_high',
-            ]
-        return {param: factory.getRandomString() for param in param_names}
+        ]
+        return {
+            'dhcp_subnets': [
+                {param: factory.getRandomString() for param in param_names}
+            ],
+            'omapi_key': factory.getRandomString(),
+        }
 
     def test_upload_dhcp_leases(self):
         self.patch(