Merge lp:~allenap/maas/ntp-service-iburst-orphan-et-al into lp:~maas-committers/maas/trunk

[Gavin Panella]

For NTP, use iburst for all servers, configure orphan mode, and add hostnames as pools.

These options have been gleaned from much experimentation and discussion with Paul Gear.

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

=== modified file 'src/maasserver/regiondservices/ntp.py'
--- src/maasserver/regiondservices/ntp.py	2016-09-23 15:20:23 +0000
+++ src/maasserver/regiondservices/ntp.py	2016-09-30 16:34:44 +0000
@@ -17,7 +17,7 @@
 from maasserver.service_monitor import service_monitor
 from maasserver.utils.orm import transactional
 from maasserver.utils.threads import deferToDatabase
-from provisioningserver.ntp.config import configure
+from provisioningserver.ntp.config import configure_region
 from provisioningserver.utils.text import split_string_list
 from provisioningserver.utils.twisted import (
     callOut,
@@ -106,7 +106,7 @@
             `_getConfiguration`.
         """
         d = deferToThread(
-            configure, configuration.references, configuration.peers)
+            configure_region, configuration.references, configuration.peers)
         d.addCallback(
             callOut, service_monitor.restartService, "ntp")
         return d

=== modified file 'src/maasserver/regiondservices/tests/test_ntp.py'
--- src/maasserver/regiondservices/tests/test_ntp.py	2016-09-23 15:20:23 +0000
+++ src/maasserver/regiondservices/tests/test_ntp.py	2016-09-30 16:34:44 +0000
@@ -90,16 +90,16 @@
     def test__tryUpdate_updates_ntp_server(self):
         service = ntp.RegionNetworkTimeProtocolService(reactor)
         configuration = yield deferToDatabase(self.make_example_configuration)
-        configure = self.patch_autospec(ntp, "configure")
+        configure_region = self.patch_autospec(ntp, "configure_region")
         restartService = self.patch_autospec(service_monitor, "restartService")
         yield service._tryUpdate()
-        self.assertThat(configure, MockCalledOnceWith(
+        self.assertThat(configure_region, MockCalledOnceWith(
             configuration.references, configuration.peers))
         self.assertThat(restartService, MockCalledOnceWith("ntp"))
         # If the configuration has not changed then a second call to
-        # `_tryUpdate` does not result in another call to `configure`.
+        # `_tryUpdate` does not result in another call to `configure_region`.
         yield service._tryUpdate()
-        self.assertThat(configure, MockCalledOnceWith(
+        self.assertThat(configure_region, MockCalledOnceWith(
             configuration.references, configuration.peers))
         self.assertThat(restartService, MockCalledOnceWith("ntp"))
 

=== modified file 'src/provisioningserver/ntp/config.py'
--- src/provisioningserver/ntp/config.py	2016-09-07 17:12:32 +0000
+++ src/provisioningserver/ntp/config.py	2016-09-30 16:34:44 +0000
@@ -4,9 +4,11 @@
 """NTP service configuration."""
 
 __all__ = [
-    "configure",
+    "configure_rack",
+    "configure_region",
 ]
 
+from functools import partial
 from itertools import (
     dropwhile,
     groupby,
@@ -25,7 +27,7 @@
 _ntp_maas_conf_name = "ntp/maas.conf"
 
 
-def configure(servers, peers):
+def configure(servers, peers, offset):
     """Configure the local NTP server with the given time references.
 
     This writes new ``ntp.conf`` and ``ntp.maas.conf`` files, using ``sudo``
@@ -33,13 +35,27 @@
 
     :param servers: An iterable of server addresses -- IPv4, IPv6, hostnames
         -- to use as time references.
+    :param peers: An iterable of peer addresses -- IPv4, IPv6, hostnames -- to
+        use as time references.
+    :param offset: A relative stratum within MAAS's world. A region controller
+        would be 0 and a rack controller would be 1.
     """
-    ntp_maas_conf = _render_ntp_maas_conf(servers, peers).encode("ascii")
+    ntp_maas_conf = _render_ntp_maas_conf(servers, peers, offset)
     ntp_maas_conf_path = get_tentative_path("etc", _ntp_maas_conf_name)
-    sudo_write_file(ntp_maas_conf_path, ntp_maas_conf, mode=0o644)
-    ntp_conf = _render_ntp_conf(ntp_maas_conf_path).encode("ascii")
+    sudo_write_file(
+        ntp_maas_conf_path,
+        ntp_maas_conf.encode("ascii"),
+        mode=0o644)
+    ntp_conf = _render_ntp_conf(ntp_maas_conf_path)
     ntp_conf_path = get_tentative_path("etc", _ntp_conf_name)
-    sudo_write_file(ntp_conf_path, ntp_conf, mode=0o644)
+    sudo_write_file(
+        ntp_conf_path,
+        ntp_conf.encode("ascii"),
+        mode=0o644)
+
+
+configure_region = partial(configure, offset=0)
+configure_rack = partial(configure, offset=1)
 
 
 def _render_ntp_conf(includefile):
@@ -66,17 +82,25 @@
     yield "includefile %s\n" % includefile
 
 
-def _render_ntp_maas_conf(servers, peers):
+def _render_ntp_maas_conf(servers, peers, offset):
     """Render ``ntp.maas.conf`` for the given time references.
 
     :param servers: An iterable of server addresses -- IPv4, IPv6, hostnames
         -- to use as time references.
+    :param peers: An iterable of peer addresses -- IPv4, IPv6, hostnames -- to
+        use as time references.
+    :param offset: A relative stratum used when calculating the stratum for
+        orphan mode (see http://support.ntp.org/bin/view/Support/OrphanMode).
     """
     lines = ["# MAAS NTP configuration."]
     servers = map(_normalise_address, servers)
-    lines.extend("server %s" % server for server in servers)
+    lines.extend(
+        "%s %s iburst" % (
+            ("server" if isinstance(server, IPAddress) else "pool"), server)
+        for server in servers)
     peers = map(_normalise_address, peers)
     lines.extend("peer %s" % peer for peer in peers)
+    lines.append("tos orphan {:d}".format(offset + 8))
     lines.append("")  # Add newline at end.
     return "\n".join(lines)
 

=== modified file 'src/provisioningserver/ntp/tests/test_config.py'
--- src/provisioningserver/ntp/tests/test_config.py	2016-09-07 14:57:19 +0000
+++ src/provisioningserver/ntp/tests/test_config.py	2016-09-30 16:34:44 +0000
@@ -5,6 +5,9 @@
 
 __all__ = []
 
+from functools import partial
+from itertools import chain
+from random import randrange
 import re
 
 from maastesting.factory import factory
@@ -15,6 +18,9 @@
 from provisioningserver.path import get_path
 from testtools.matchers import (
     Equals,
+    Is,
+    IsInstance,
+    MatchesStructure,
     StartsWith,
 )
 
@@ -25,20 +31,34 @@
 
 
 def extract_servers_and_pools(configuration):
+    return [
+        address for _, address, _ in
+        extract_servers_and_pools_full(configuration)
+    ]
+
+
+def extract_servers_and_pools_full(configuration):
     return re.findall(
-        r" ^ \s* (?: server | pool ) \s+ (.+) $ ", configuration,
-        re.VERBOSE | re.MULTILINE)
+        r"^ *(server|pool) +(\S+)(?: +([^\r\n]+))?$",
+        configuration, re.MULTILINE)
 
 
 def extract_peers(configuration):
+    return [
+        address for _, address, _ in
+        extract_peers_full(configuration)
+    ]
+
+
+def extract_peers_full(configuration):
     return re.findall(
-        r" ^ \s* peer \s+ (.+) $ ", configuration,
-        re.VERBOSE | re.MULTILINE)
+        r"^ *(peer) +(\S+)(?: +([^\r\n]+))?$",
+        configuration, re.MULTILINE)
 
 
 def extract_included_files(configuration):
     return re.findall(
-        r" ^ \s* includefile \s+ (.*) $ ", configuration,
+        r" ^ \s* includefile \s+ (\S*) $ ", configuration,
         re.VERBOSE | re.MULTILINE)
 
 
@@ -60,7 +80,8 @@
             factory.make_ipv6_address(),
             factory.make_hostname(),
         ]
-        config.configure(servers, peers)
+        offset = randrange(0, 5)
+        config.configure(servers, peers, offset)
         ntp_conf_path = get_path("etc", config._ntp_conf_name)
         ntp_maas_conf_path = get_path("etc", config._ntp_maas_conf_name)
         ntp_conf = read_configuration(ntp_conf_path)
@@ -74,6 +95,28 @@
             extract_servers_and_pools(ntp_maas_conf), Equals(servers))
         self.assertThat(
             extract_peers(ntp_maas_conf), Equals(peers))
+        self.assertThat(
+            extract_tos_options(ntp_maas_conf),
+            Equals(["orphan", str(offset + 8)]))
+
+    def test_configure_region_is_alias(self):
+        self.assertThat(config.configure_region, IsInstance(partial))
+        self.assertThat(
+            config.configure_region, MatchesStructure(
+                func=Is(config.configure), args=Equals(()),
+                keywords=Equals({"offset": 0})))
+
+    def test_configure_rack_is_alias(self):
+        self.assertThat(config.configure_rack, IsInstance(partial))
+        self.assertThat(
+            config.configure_rack, MatchesStructure(
+                func=Is(config.configure), args=Equals(()),
+                keywords=Equals({"offset": 1})))
+
+
+def extract_tos_options(configuration):
+    commands = re.findall(r"^ *tos +([^\r\n]+)$", configuration, re.MULTILINE)
+    return list(chain.from_iterable(map(str.split, commands)))
 
 
 class TestRenderNTPConfFromSource(MAASTestCase):
@@ -152,11 +195,17 @@
             factory.make_ipv6_address(),
             factory.make_hostname(),
         ]
-        ntp_maas_conf = config._render_ntp_maas_conf(servers, [])
+        ntp_maas_conf = config._render_ntp_maas_conf(servers, [], 0)
         self.assertThat(ntp_maas_conf, StartsWith(
             '# MAAS NTP configuration.\n'))
-        servers_or_pools = extract_servers_and_pools(ntp_maas_conf)
-        self.assertThat(servers_or_pools, Equals(servers))
+        servers_or_pools = extract_servers_and_pools_full(ntp_maas_conf)
+        # Hostnames are rendered as `pool` commands so that all IP addresses
+        # resolved via DNS are included in clock selection.
+        self.assertThat(servers_or_pools, Equals([
+            ("server", servers[0], "iburst"),
+            ("server", servers[1], "iburst"),
+            ("pool", servers[2], "iburst"),
+        ]))
 
     def test_renders_the_given_peers(self):
         peers = [
@@ -164,11 +213,12 @@
             factory.make_ipv6_address(),
             factory.make_hostname(),
         ]
-        ntp_maas_conf = config._render_ntp_maas_conf([], peers)
+        ntp_maas_conf = config._render_ntp_maas_conf([], peers, 0)
         self.assertThat(ntp_maas_conf, StartsWith(
             '# MAAS NTP configuration.\n'))
-        observed_peers = extract_peers(ntp_maas_conf)
-        self.assertThat(observed_peers, Equals(peers))
+        observed_peers = extract_peers_full(ntp_maas_conf)
+        self.assertThat(observed_peers, Equals(
+            [("peer", peer, "") for peer in peers]))
 
     def test_renders_ipv6_mapped_ipv4_addresses_as_plain_ipv4(self):
         server_as_ipv4 = factory.make_ipv4_address()
@@ -176,12 +226,19 @@
         peer_as_ipv4 = factory.make_ipv4_address()
         peer_as_ipv6 = str(IPAddress(peer_as_ipv4).ipv6())
         ntp_maas_conf = config._render_ntp_maas_conf(
-            [server_as_ipv6], [peer_as_ipv6])
+            [server_as_ipv6], [peer_as_ipv6], 0)
         observed_servers = extract_servers_and_pools(ntp_maas_conf)
         self.assertThat(observed_servers, Equals([server_as_ipv4]))
         observed_peers = extract_peers(ntp_maas_conf)
         self.assertThat(observed_peers, Equals([peer_as_ipv4]))
 
+    def test_configures_orphan_mode(self):
+        offset = randrange(0, 5)
+        ntp_maas_conf = config._render_ntp_maas_conf([], [], offset)
+        self.assertThat(
+            extract_tos_options(ntp_maas_conf),
+            Equals(["orphan", str(offset + 8)]))
+
 
 example_ntp_conf = """\
 # /etc/ntp.conf, configuration for ntpd; see ntp.conf(5) for help

=== modified file 'src/provisioningserver/rackdservices/ntp.py'
--- src/provisioningserver/rackdservices/ntp.py	2016-08-19 09:38:08 +0000
+++ src/provisioningserver/rackdservices/ntp.py	2016-09-30 16:34:44 +0000
@@ -10,7 +10,7 @@
 from datetime import timedelta
 
 import attr
-from provisioningserver.ntp.config import configure
+from provisioningserver.ntp.config import configure_rack
 from provisioningserver.rpc import exceptions
 from provisioningserver.rpc.region import GetControllerType
 from provisioningserver.service_monitor import service_monitor
@@ -98,7 +98,7 @@
             `_getConfiguration`.
         """
         if configuration.is_rack and not configuration.is_region:
-            d = deferToThread(configure, configuration.references, ())
+            d = deferToThread(configure_rack, configuration.references, ())
             d.addCallback(callOut, service_monitor.restartService, "ntp")
             return d
 

=== modified file 'src/provisioningserver/rackdservices/tests/test_ntp.py'
--- src/provisioningserver/rackdservices/tests/test_ntp.py	2016-09-07 15:40:52 +0000
+++ src/provisioningserver/rackdservices/tests/test_ntp.py	2016-09-30 16:34:44 +0000
@@ -116,16 +116,16 @@
         rpc_service, _ = yield prepareRegionForGetControllerType(self)
         servers = {c.address[0] for c in rpc_service.getAllClients()}
         service = ntp.RackNetworkTimeProtocolService(rpc_service, reactor)
-        configure = self.patch_autospec(ntp, "configure")
+        configure_rack = self.patch_autospec(ntp, "configure_rack")
         restartService = self.patch_autospec(service_monitor, "restartService")
 
         yield service._tryUpdate()
-        self.assertThat(configure, MockCalledOnceWith(servers, ()))
+        self.assertThat(configure_rack, MockCalledOnceWith(servers, ()))
         self.assertThat(restartService, MockCalledOnceWith("ntp"))
         # If the configuration has not changed then a second call to
         # `_tryUpdate` does not result in another call to `configure`.
         yield service._tryUpdate()
-        self.assertThat(configure, MockCalledOnceWith(servers, ()))
+        self.assertThat(configure_rack, MockCalledOnceWith(servers, ()))
         self.assertThat(restartService, MockCalledOnceWith("ntp"))
 
     @inlineCallbacks
@@ -160,7 +160,7 @@
         self.useFixture(MAASRootFixture())
         rpc_service, _ = yield prepareRegionForGetControllerType(self, True)
         service = ntp.RackNetworkTimeProtocolService(rpc_service, reactor)
-        self.patch_autospec(ntp, "configure")  # No-op configuration.
+        self.patch_autospec(ntp, "configure_rack")  # No-op configuration.
 
         # There is no most recently applied configuration.
         self.assertThat(service._configuration, Is(None))
@@ -192,7 +192,7 @@
     @inlineCallbacks
     def test__tryUpdate_logs_errors_from_broken_method(self):
         rpc_service, _ = yield prepareRegionForGetControllerType(self)
-        self.patch_autospec(ntp, "configure")  # No-op configuration.
+        self.patch_autospec(ntp, "configure_rack")  # No-op configuration.
 
         service = ntp.RackNetworkTimeProtocolService(rpc_service, reactor)
         broken_method = self.patch_autospec(service, self.method)