Merge lp:~julian-edwards/maas/ntp-server-dhcp into lp:~maas-committers/maas/trunk

[Julian Edwards]

Add a global config for NTP server, which sets the ntp-servers option on DHCP responses to nodes.

I made the default NTP address ntp.ubuntu.com as a convenience. More to the point, I made it the IP address because the DHCP server config requires an IP not a domain name.

Everything else here is pretty self-explanatory, I hope.

[Graham Binns (gmb)]

Minor nitpick, otherwise looks good.

[1]

+ "e.g. 91.189.94.4 (ntp.ubuntu.com)")

It's not clear from this what the example is. Should I copy just the IP, or the IP and what's inside the parens, or can I just use what's inside the parens?

I'd say something like: "e.g. for ntp.ubuntu.com: '91.189.94.4'".

review: Approve

[Julian Edwards (julian-edwards)]

On 11/11/13 18:25, Graham Binns wrote:
> Review: Approve
>
> Minor nitpick, otherwise looks good.
>
> [1]
>
> + "e.g. 91.189.94.4 (ntp.ubuntu.com)")
>
> It's not clear from this what the example is. Should I copy just the IP, or the IP and what's inside the parens, or can I just use what's inside the parens?
>
> I'd say something like: "e.g. for ntp.ubuntu.com: '91.189.94.4'".

Good call, thanks. I was hand-wringing over that one a bit :)

=== modified file 'etc/maas/templates/dhcp/dhcpd.conf.template'
--- etc/maas/templates/dhcp/dhcpd.conf.template	2013-10-17 20:52:59 +0000
+++ etc/maas/templates/dhcp/dhcpd.conf.template	2013-11-11 09:25:24 +0000
@@ -13,6 +13,9 @@
        option domain-name-servers {{dns_servers}};
        option domain-name "{{domain_name}}";
        option routers {{router_ip}};
+       {{if ntp_server}}
+       option ntp-servers {{ntp_server}};
+       {{endif}}
        range dynamic-bootp {{ip_range_low}} {{ip_range_high}};
        class "PXE" {
           match if substring (option vendor-class-identifier, 0, 3) = "PXE";

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/dhcp.py	2013-11-11 09:25:24 +0000
@@ -22,6 +22,7 @@
     NODEGROUP_STATUS,
     NODEGROUPINTERFACE_MANAGEMENT,
     )
+from maasserver.models import Config
 from netaddr import IPAddress
 from provisioningserver.tasks import (
     restart_dhcp_server,
@@ -67,6 +68,7 @@
         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"),
         domain_name=nodegroup.name,
         ip_range_low=interface.ip_range_low,
         ip_range_high=interface.ip_range_high,

=== modified file 'src/maasserver/dhcp_connect.py'
--- src/maasserver/dhcp_connect.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/dhcp_connect.py	2013-11-11 09:25:24 +0000
@@ -19,6 +19,7 @@
 from django.db.models.signals import post_save
 from django.dispatch import receiver
 from maasserver.models import (
+    Config,
     NodeGroup,
     NodeGroupInterface,
     )
@@ -43,3 +44,13 @@
 
 
 connect_to_field_change(dhcp_post_edit_status_NodeGroup, NodeGroup, 'status')
+
+
+def ntp_server_changed(sender, instance, created, **kwargs):
+    """The ntp_server config item changed, so write new DHCP configs."""
+    from maasserver.dhcp import configure_dhcp
+    for nodegroup in NodeGroup.objects.all():
+        configure_dhcp(nodegroup)
+
+
+Config.objects.config_changed_connect("ntp_server", ntp_server_changed)

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2013-11-05 03:47:04 +0000
+++ src/maasserver/forms.py	2013-11-11 09:25:24 +0000
@@ -627,6 +627,7 @@
     enlistment_domain = get_config_field('enlistment_domain')
     http_proxy = get_config_field('http_proxy')
     upstream_dns = get_config_field('upstream_dns')
+    ntp_server = get_config_field('ntp_server')
 
 
 class CommissioningForm(ConfigForm):

=== modified file 'src/maasserver/forms_settings.py'
--- src/maasserver/forms_settings.py	2013-11-08 06:26:55 +0000
+++ src/maasserver/forms_settings.py	2013-11-11 09:25:24 +0000
@@ -184,6 +184,17 @@
                 "server config.")
         }
     },
+    'ntp_server': {
+        'default': None,
+        'form': forms.GenericIPAddressField,
+        'form_kwargs': {
+            'label': "Address of NTP server for nodes",
+            'required': False,
+            'help_text': (
+                "NTP server address passed to nodes via a DHCP response. "
+                "e.g. for ntp.ubuntu.com: '91.189.94.4'")
+        }
+    },
     'default_distro_series': {
         'default': DISTRO_SERIES.precise,
         'form': forms.ChoiceField,

=== modified file 'src/maasserver/models/config.py'
--- src/maasserver/models/config.py	2013-11-05 03:47:04 +0000
+++ src/maasserver/models/config.py	2013-11-11 09:25:24 +0000
@@ -57,6 +57,7 @@
         'commissioning_distro_series': DISTRO_SERIES.precise,
         'http_proxy': None,
         'upstream_dns': None,
+        'ntp_server': '91.189.94.4',  # ntp.ubuntu.com
         ## /settings
         }
 

=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2013-10-15 06:52:13 +0000
+++ src/maasserver/tests/test_dhcp.py	2013-11-11 09:25:24 +0000
@@ -15,6 +15,7 @@
 __all__ = []
 
 from functools import partial
+import random
 
 from django.conf import settings
 from maasserver import dhcp
@@ -24,6 +25,8 @@
     )
 from maasserver.dns import get_dns_server_address
 from maasserver.enum import NODEGROUP_STATUS
+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 maastesting.celery import CeleryFixture
@@ -84,6 +87,7 @@
 
         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
@@ -174,3 +178,22 @@
                 dhcp.write_dhcp_config.apply_async.call_count,
                 kwargs['kwargs']['router_ip'],
             ))
+
+    def test_dhcp_config_gets_written_when_ntp_server_changes(self):
+        # When the "ntp_server" Config item is changed, check that all
+        # nodegroups get their DHCP config re-written.
+        num_active_nodegroups = random.randint(1, 10)
+        num_inactive_nodegroups = random.randint(1, 10)
+        for x in range(num_active_nodegroups):
+            factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
+        for x in range(num_inactive_nodegroups):
+            factory.make_node_group(status=NODEGROUP_STATUS.PENDING)
+
+        self.patch(settings, "DHCP_CONNECT", True)
+        self.patch(dhcp, 'write_dhcp_config')
+
+        Config.objects.set_config("ntp_server", factory.getRandomIPAddress())
+
+        self.assertEqual(
+            num_active_nodegroups,
+            dhcp.write_dhcp_config.apply_async.call_count)

=== modified file 'src/provisioningserver/dhcp/config.py'
--- src/provisioningserver/dhcp/config.py	2013-10-07 09:12:40 +0000
+++ src/provisioningserver/dhcp/config.py	2013-11-11 09:25:24 +0000
@@ -41,6 +41,7 @@
         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.
@@ -52,6 +53,7 @@
     template_file = locate_config(TEMPLATES_DIR, 'dhcpd.conf.template')
     params['bootloader'] = compose_bootloader_path()
     params['platform_codename'] = linux_distribution()[2]
+    params.setdefault("ntp_server")
     try:
         template = tempita.Template.from_filename(
             template_file, encoding="UTF-8")

=== modified file 'src/provisioningserver/dhcp/tests/test_config.py'
--- src/provisioningserver/dhcp/tests/test_config.py	2013-10-30 01:21:13 +0000
+++ src/provisioningserver/dhcp/tests/test_config.py	2013-11-11 09:25:24 +0000
@@ -55,6 +55,7 @@
         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",
@@ -109,3 +110,11 @@
         params = make_sample_params()
         output = config.get_config(**params)
         self.assertThat(output, Contains(compose_bootloader_path()))
+
+    def test_renders_without_ntp_servers_set(self):
+        params = make_sample_params()
+        del params['ntp_server']
+        template = self.patch_template()
+        rendered = template.substitute(params)
+        self.assertEqual(rendered, config.get_config(**params))
+        self.assertNotIn("ntp-servers", rendered)

=== modified file 'src/provisioningserver/dhcp/tests/test_writer.py'
--- src/provisioningserver/dhcp/tests/test_writer.py	2013-10-07 09:12:40 +0000
+++ src/provisioningserver/dhcp/tests/test_writer.py	2013-11-11 09:25:24 +0000
@@ -38,6 +38,7 @@
         '--subnet-mask', 'subnet-mask',
         '--broadcast-ip', 'broadcast-ip',
         '--dns-servers', 'dns-servers',
+        '--ntp-server', 'ntp-server',
         '--domain-name', 'domain-name',
         '--router-ip', 'router-ip',
         '--ip-range-low', 'ip-range-low',
@@ -53,8 +54,8 @@
         output, err = cmd.communicate()
         contains_all_params = ContainsAll(
             ['subnet', 'subnet-mask', 'broadcast-ip',
-             'omapi-key', 'dns-servers', 'domain-name', 'router-ip',
-             'ip-range-low', 'ip-range-high'])
+             'omapi-key', 'dns-servers', 'ntp-server', 'domain-name',
+             'router-ip', 'ip-range-low', 'ip-range-high'])
         self.assertThat(output, contains_all_params)
 
     def test_arg_setup(self):
@@ -67,6 +68,7 @@
                 subnet_mask='subnet-mask',
                 broadcast_ip='broadcast-ip',
                 dns_servers='dns-servers',
+                ntp_server='ntp-server',
                 domain_name='domain-name',
                 router_ip='router-ip',
                 omapi_key='omapi-key',
@@ -86,6 +88,7 @@
             'broadcast-ip',
             'omapi-key',
             'dns-servers',
+            'ntp-server',
             'domain-name',
             'router-ip',
             'ip-range-low',
@@ -108,6 +111,7 @@
             'broadcast-ip',
             'omapi-key',
             'dns-servers',
+            'ntp-server',
             'domain-name',
             'router-ip',
             'ip-range-low',

=== modified file 'src/provisioningserver/dhcp/writer.py'
--- src/provisioningserver/dhcp/writer.py	2013-10-15 11:35:10 +0000
+++ src/provisioningserver/dhcp/writer.py	2013-11-11 09:25:24 +0000
@@ -41,6 +41,10 @@
             "One or more IP addresses of the DNS server for the subnet "
             "separated by spaces."))
     parser.add_argument(
+        "--ntp-server", action="store", required=True, help=(
+            "IP address of the NTP server to pass to nodes in the DHCP "
+            "response."))
+    parser.add_argument(
         "--domain-name", action="store", required=True, help=(
             "The domain name that will be appended to the client's hostname "
             "to form a fully-qualified domain-name (FQDN)."))