Merge ~mpontillo/maas:alternate-ntp-servers-in-dhcp--bug-1753496 into maas:master

[Mike Pontillo]

LP: #1753496 - Add NTP server for peer rack, if appropriate.

[Paul Gear (paulgear)]

I'm not familiar with how racks, etc. are represented within the MAAS code, but will this produce a list of *all* NTP servers available in the region, or just one additional from a secondary rack?

NTP best practice is to have at least 4 servers, if possible: https://tools.ietf.org/html/draft-ietf-ntp-bcp-06#section-4.1, so if MAAS knows about more NTP servers, I think it should add them all.

[Andres Rodriguez (andreserl)]

Machines deployed by MAAS only have access to the rack controllers for DHCP (unless they are region/racks). In HA configurations we only have 2 rack controllers, and such we would have 2 NTP servers for NTP.

That said, these changes here are only apply to DHCP. MAAS only relies on DHCP for commissioning or ephemeral environments. For deployments the machines get statically configured from a different code path that gathers all NTP servers in the specific VLAN.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b alternate-ntp-servers-in-dhcp--bug-1753496 lp:~mpontillo/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2454/console
COMMIT: b49877a5bf10fbbeb913f8bc7b258a6a5340b56a

review: Needs Fixing

f6eb1e9... by Mike Pontillo on 2018-04-13

Fix lint.

[Andres Rodriguez (andreserl)]

+1

review: Approve

diff --git a/src/maasserver/dhcp.py b/src/maasserver/dhcp.py
index 1d07cc0..d98134c 100644
--- a/src/maasserver/dhcp.py
+++ b/src/maasserver/dhcp.py
@@ -398,7 +398,7 @@ def make_pools_for_subnet(subnet, failover_peer=None):
 def make_subnet_config(
         rack_controller, subnet, default_dns_servers: Optional[list],
         ntp_servers: Union[list, dict], default_domain, search_list=None,
-        failover_peer=None, subnets_dhcp_snippets: list=None):
+        failover_peer=None, subnets_dhcp_snippets: list=None, peer_rack=None):
     """Return DHCP subnet configuration dict for a rack interface.
 
     :param ntp_servers: Either a list of NTP server addresses or hostnames to
@@ -416,15 +416,6 @@ def make_subnet_config(
     if subnets_dhcp_snippets is None:
         subnets_dhcp_snippets = []
 
-    if isinstance(ntp_servers, dict):
-        try:
-            ntp_server = ntp_servers[
-                subnet.vlan.space_id, subnet.get_ip_version()]
-        except KeyError:
-            ntp_servers = []
-        else:
-            ntp_servers = [ntp_server]
-
     subnet_config = {
         'subnet': str(ip_network.network),
         'subnet_mask': str(ip_network.netmask),
@@ -434,7 +425,7 @@ def make_subnet_config(
             '' if not subnet.gateway_ip
             else str(subnet.gateway_ip)),
         'dns_servers': dns_servers,
-        'ntp_servers': ntp_servers,
+        'ntp_servers': get_ntp_servers(ntp_servers, subnet, peer_rack),
         'domain_name': default_domain.name,
         'pools': make_pools_for_subnet(subnet, failover_peer),
         'dhcp_snippets': [
@@ -454,18 +445,47 @@ def make_failover_peer_config(vlan, rack_controller):
     interface_ip_address = get_ip_address_for_rack_controller(
         rack_controller, vlan)
     if is_primary:
-        peer_address = get_ip_address_for_rack_controller(
-            vlan.secondary_rack, vlan)
+        peer_rack = vlan.secondary_rack
     else:
-        peer_address = get_ip_address_for_rack_controller(
-            vlan.primary_rack, vlan)
+        peer_rack = vlan.primary_rack
+    peer_address = get_ip_address_for_rack_controller(peer_rack, vlan)
     name = "failover-vlan-%d" % vlan.id
-    return name, {
-        "name": name,
-        "mode": "primary" if is_primary else "secondary",
-        "address": str(interface_ip_address.ip),
-        "peer_address": str(peer_address.ip),
-    }
+    return (
+        name,
+        {
+            "name": name,
+            "mode": "primary" if is_primary else "secondary",
+            "address": str(interface_ip_address.ip),
+            "peer_address": str(peer_address.ip),
+        },
+        peer_rack
+    )
+
+
+def get_ntp_servers(ntp_servers, subnet, peer_rack):
+    """Return the list of NTP servers, based on the initial input list of
+    servers or dictionary, the subnet the servers will be advertised on,
+    and the peer rack controller (if present).
+    """
+    if isinstance(ntp_servers, dict):
+        # If ntp_servers is a dict, that means it maps each
+        # (space_id, address_family) to the best NTP server for that space.
+        # If it's already a list, that means we're just using the external
+        # NTP server(s).
+        space_address_family = (subnet.vlan.space_id, subnet.get_ip_version())
+        ntp_server = ntp_servers.get(space_address_family)
+        if ntp_server is None:
+            return []
+        else:
+            if peer_rack is not None:
+                alternates = get_ntp_server_addresses_for_rack(peer_rack)
+                alternate_ntp_server = alternates.get(space_address_family)
+                if alternate_ntp_server is not None:
+                    return [ntp_server, alternate_ntp_server]
+            return [ntp_server]
+    else:
+        # Return the original input; it was already a list.
+        return ntp_servers
 
 
 @typed
@@ -487,12 +507,14 @@ def get_dhcp_configure_for(
         rack_controller, vlan, ip_version)
     interface = get_best_interface(interfaces)
 
-    # Generate the failover peer for this VLAN.
-    if vlan.secondary_rack_id is not None:
-        peer_name, peer_config = make_failover_peer_config(
+    has_secondary = vlan.secondary_rack_id is not None
+
+    if has_secondary:
+        # Generate the failover peer for this VLAN.
+        peer_name, peer_config, peer_rack = make_failover_peer_config(
             vlan, rack_controller)
     else:
-        peer_name, peer_config = None, None
+        peer_name, peer_config, peer_rack = None, None, None
 
     if dhcp_snippets is None:
         dhcp_snippets = []
@@ -510,7 +532,8 @@ def get_dhcp_configure_for(
         subnet_configs.append(
             make_subnet_config(
                 rack_controller, subnet, maas_dns_servers, ntp_servers,
-                domain, search_list, peer_name, subnets_dhcp_snippets))
+                domain, search_list, peer_name, subnets_dhcp_snippets,
+                peer_rack))
 
     # Generate the hosts for all subnets.
     hosts = make_hosts_for_subnets(subnets, nodes_dhcp_snippets)
diff --git a/src/maasserver/tests/test_dhcp.py b/src/maasserver/tests/test_dhcp.py
index 5680ae7..ea601bd 100644
--- a/src/maasserver/tests/test_dhcp.py
+++ b/src/maasserver/tests/test_dhcp.py
@@ -897,6 +897,35 @@ class TestMakeSubnetConfig(MAASServerTestCase):
             rack_controller, subnet, [""], ntp_servers, default_domain)
         self.expectThat(config['ntp_servers'], Equals([address.ip]))
 
+    def test__sets_ntp_from_dict_argument_with_alternates(self):
+        r1 = factory.make_RackController(interface=False)
+        r2 = factory.make_RackController(interface=False)
+        vlan = factory.make_VLAN(primary_rack=r1, secondary_rack=r2)
+        subnet = factory.make_Subnet(vlan=vlan, dns_servers=[], space=None)
+        r1_eth0 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=r1)
+        r2_eth0 = factory.make_Interface(
+            INTERFACE_TYPE.PHYSICAL, vlan=vlan, node=r2)
+        a1 = factory.make_StaticIPAddress(
+            interface=r1_eth0, subnet=subnet,
+            alloc_type=IPADDRESS_TYPE.STICKY)
+        a2 = factory.make_StaticIPAddress(
+            interface=r2_eth0, subnet=subnet,
+            alloc_type=IPADDRESS_TYPE.STICKY)
+        r1_ntp_servers = {
+            (vlan.space_id, subnet.get_ipnetwork().version): a1.ip,
+        }
+        r2_ntp_servers = {
+            (vlan.space_id, subnet.get_ipnetwork().version): a2.ip,
+        }
+        default_domain = Domain.objects.get_default_domain()
+        config = dhcp.make_subnet_config(
+            r1, subnet, [""], r1_ntp_servers, default_domain, peer_rack=r2)
+        self.expectThat(config['ntp_servers'], Equals([a1.ip, a2.ip]))
+        config = dhcp.make_subnet_config(
+            r2, subnet, [""], r2_ntp_servers, default_domain, peer_rack=r1)
+        self.expectThat(config['ntp_servers'], Equals([a2.ip, a1.ip]))
+
     def test__overrides_ipv4_dns_from_subnet(self):
         rack_controller = factory.make_RackController(interface=False)
         vlan = factory.make_VLAN()
@@ -1351,7 +1380,7 @@ class TestMakeFailoverPeerConfig(MAASServerTestCase):
             "mode": "primary",
             "address": str(primary_ip.ip),
             "peer_address": str(secondary_ip.ip),
-        }), dhcp.make_failover_peer_config(vlan, primary_rack))
+        }, secondary_rack), dhcp.make_failover_peer_config(vlan, primary_rack))
 
     def test__renders_config_for_secondary(self):
         primary_rack = factory.make_RackController()
@@ -1376,7 +1405,7 @@ class TestMakeFailoverPeerConfig(MAASServerTestCase):
             "mode": "secondary",
             "address": str(secondary_ip.ip),
             "peer_address": str(primary_ip.ip),
-        }), dhcp.make_failover_peer_config(vlan, secondary_rack))
+        }, primary_rack), dhcp.make_failover_peer_config(vlan, secondary_rack))
 
 
 class TestGetDHCPConfigureFor(MAASServerTestCase):