Merge lp:~jtv/maas/custom-dhcp into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Use our own dhcpd instance, so we can tell it which network interfaces to service.

This must be accompanied by a matching packaging change which installs the maas-dhcp-server Upstart script as part of maas-dhcp.

As a drive-by, I added the “-n” option to a sudo command. Makes sudo fail if it would need to prompt for a password.

The “sudo -n” was Raphaël's suggestion. The accompanying packaging branch is at https://code.launchpad.net/~jtv/maas/packaging-custom-dhcpd/+merge/124594

Jeroen

[John A Meinel (jameinel)]

Would it make sense to gather up all the files that need to be written under sudo and do them with one call rather than several? Do we get a decent error message up to users if sudo fails because of '-n' (or just a stacktrace in python because the command failed?)

Otherwise looks fine to me.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Good questions — and luckily I provided answers in my predecessor branch! In that preparatory branch I extracted the sudo_write_file helper, and in view of Raphaël's painful debugging experience, made it check sudo's return code and report sudo's error message. I haven't looked into how that message gets displayed, really, so there may be separate work to be done on that.

I see some value in bundling up the rewriting of files, but there isn't much to bundle anyway. Plus it's all done synchronously to service restarts, so arguably the real value would be in ensuring that we retry on failure.

[MAAS Lander (maas-lander)]

Attempt to merge into lp:maas failed due to conflicts:

text conflict in src/maasserver/models/nodegroup.py
text conflict in src/maasserver/tests/test_nodegroup.py

=== modified file 'HACKING.txt'
--- HACKING.txt	2012-09-17 14:25:54 +0000
+++ HACKING.txt	2012-09-24 06:50:26 +0000
@@ -270,23 +270,32 @@
 ^^^^^^^^^^^^^^^^
 
 MAAS requires a properly configured DHCP server so it can boot machines using
-PXE.  MAAS works with the ISC DHCP server; install it if it's not already
-installed::
-
-    $ sudo apt-get install isc-dhcp-server
-
-There is a tool to generate a configuration that will work with MAAS::
+PXE.  MAAS can work with its own instance of the ISC DHCP server, if you
+install the maas-dhcp package::
+
+    $ sudo apt-get install maas-dhcp
+
+If you choose to run your own ISC DHCP server, there is a bit more
+configuration to do.  First, run this tool to generate a configuration that
+will work with MAAS::
 
     $ maas-provision generate-dhcp-config [options]
 
 Run ``maas-provision generate-dhcp-config -h`` to see the options.  You will
 need to provide various IP details such as the range of IP addresses to assign
 to clients.  You can use the generated output to configure your system's ISC
-DHCP server, by putting the configuration in the ``/etc/dhcp/dhcpd.conf`` file.
+DHCP server, by inserting the configuration in the ``/etc/dhcp/dhcpd.conf``
+file.
+
+Also, edit /etc/default/isc-dhcp-server to set the INTERFACES variable to just
+the network interfaces that should be serviced by this DHCP server.
 
 Now restart dhcpd::
 
-    $ sudo /etc/init.d/isc-dhcp-server restart
+    $ sudo service isc-dhcp-server restart
+
+None of this work is needed if you let MAAS run its own DHCP server by
+installing ``maas-dhcp``.
 
 
 Development services

=== modified file 'etc/celeryconfig.py'
--- etc/celeryconfig.py	2012-09-20 12:53:00 +0000
+++ etc/celeryconfig.py	2012-09-24 06:50:26 +0000
@@ -35,10 +35,13 @@
 DNS_RNDC_PORT = 954
 
 # DHCP leases file, as maintained by ISC dhcpd.
-DHCP_LEASES_FILE = '/var/lib/dhcp/dhcpd.leases'
+DHCP_LEASES_FILE = '/var/lib/maas/dhcpd.leases'
 
 # ISC dhcpd configuration file.
-DHCP_CONFIG_FILE = '/etc/dhcp/dhcpd.conf'
+DHCP_CONFIG_FILE = '/etc/maas/dhcpd.conf'
+
+# List of interfaces that the dhcpd should service (if managed by MAAS).
+DHCP_INTERFACES_FILE = '/var/lib/maas/dhcpd-interfaces'
 
 # Broken connection information.
 # Format: transport://userid:password@hostname:port/virtual_host

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-09-20 15:10:21 +0000
+++ src/maasserver/models/nodegroup.py	2012-09-24 06:50:26 +0000
@@ -30,13 +30,18 @@
 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
+from provisioningserver.tasks import (
+    add_new_dhcp_host_map,
+    write_dhcp_config,
+    )
 
 
 class NodeGroupManager(Manager):

=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-09-19 13:48:42 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-09-24 06:50:26 +0000
@@ -12,11 +12,15 @@
 __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

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2012-09-17 06:46:55 +0000
+++ src/provisioningserver/tasks.py	2012-09-24 06:50:26 +0000
@@ -29,7 +29,10 @@
     )
 
 from celery.task import task
-from celeryconfig import DHCP_CONFIG_FILE
+from celeryconfig import (
+    DHCP_CONFIG_FILE,
+    DHCP_INTERFACES_FILE,
+    )
 from provisioningserver import boot_images
 from provisioningserver.auth import (
     record_api_credentials,
@@ -306,18 +309,19 @@
 def write_dhcp_config(**kwargs):
     """Write out the DHCP configuration file and restart the DHCP server.
 
+    :param dhcp_interfaces: Space-separated list of interfaces that the
+        DHCP server should listen on.
     :param **kwargs: Keyword args passed to dhcp.config.get_config()
     """
-    sudo_write_file(
-        DHCP_CONFIG_FILE, config.get_config(**kwargs), encoding='ascii',
-        mode=0744)
+    sudo_write_file(DHCP_CONFIG_FILE, config.get_config(**kwargs))
+    sudo_write_file(DHCP_INTERFACES_FILE, kwargs.get('dhcp_interfaces', ''))
     restart_dhcp_server()
 
 
 @task
 def restart_dhcp_server():
     """Restart the DHCP server."""
-    check_call(['sudo', 'service', 'isc-dhcp-server', 'restart'])
+    check_call(['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
 
 
 # =====================================================================

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2012-09-17 10:55:11 +0000
+++ src/provisioningserver/tests/test_tasks.py	2012-09-24 06:50:26 +0000
@@ -24,7 +24,10 @@
 from apiclient.creds import convert_tuple_to_string
 from apiclient.maas_client import MAASClient
 from apiclient.testing.credentials import make_api_credentials
-from celeryconfig import DHCP_CONFIG_FILE
+from celeryconfig import (
+    DHCP_CONFIG_FILE,
+    DHCP_INTERFACES_FILE,
+    )
 from maastesting.celery import CeleryFixture
 from maastesting.factory import factory
 from maastesting.fakemethod import (
@@ -255,18 +258,24 @@
         content = config.get_config(**config_params).encode("ascii")
         mocked_proc.communicate.assert_any_call(content)
 
+        # Similarly, it also writes the DHCPD interfaces to
+        # /var/lib/maas/dhcpd-interfaces.
+        mocked_popen.assert_any_call(
+            ["sudo", "-n", "maas-provision", "atomic-write", "--filename",
+            DHCP_INTERFACES_FILE, "--mode", "0744"], stdin=PIPE)
+
         # Finally it should restart the dhcp server.
         check_call_args = mocked_check_call.call_args
         self.assertEqual(
             check_call_args[0][0],
-            ['sudo', 'service', 'isc-dhcp-server', 'restart'])
+            ['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
 
     def test_restart_dhcp_server_sends_command(self):
         recorder = FakeMethod()
         self.patch(tasks, 'check_call', recorder)
         restart_dhcp_server()
         self.assertEqual(
-            (1, (['sudo', 'service', 'isc-dhcp-server', 'restart'],)),
+            (1, (['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'],)),
             (recorder.call_count, recorder.extract_args()[0]))