Merge lp:~allenap/maas/ntp-vendor-data-routable-pairs into lp:~maas-committers/maas/trunk

[Gavin Panella]

Choose NTP servers for vendor-data by finding routable rack controller addresses from the perspective of the node being deployed.

[Andres Rodriguez (andreserl)]

lgtm! Just a comment inline but non-blocker.

review: Approve

[Gavin Panella (allenap)]

Thanks for the review!

=== modified file 'src/metadataserver/tests/test_vendor_data.py'
--- src/metadataserver/tests/test_vendor_data.py	2016-09-16 16:24:22 +0000
+++ src/metadataserver/tests/test_vendor_data.py	2016-09-20 07:36:22 +0000
@@ -7,6 +7,7 @@
 
 from unittest.mock import sentinel
 
+from maasserver.dbviews import register_view
 from maasserver.models.config import Config
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -123,13 +124,35 @@
         self.assertThat(dict(configuration), Equals({}))
 
     def test_yields_boot_cluster_address_when_machine_has_booted(self):
+        register_view("maasserver_routable_pairs")
         Config.objects.set_config("ntp_external_only", False)
+
         machine = factory.make_Machine()
-        machine.boot_cluster_ip = factory.make_ip_address()
-        machine.save()
+        address = factory.make_StaticIPAddress(
+            interface=factory.make_Interface(node=machine))
+
+        rack_primary = factory.make_RackController(subnet=address.subnet)
+        rack_primary_address = factory.make_StaticIPAddress(
+            interface=factory.make_Interface(node=rack_primary),
+            subnet=address.subnet)
+
+        rack_secondary = factory.make_RackController(subnet=address.subnet)
+        rack_secondary_address = factory.make_StaticIPAddress(
+            interface=factory.make_Interface(node=rack_secondary),
+            subnet=address.subnet)
+
+        vlan = address.subnet.vlan
+        vlan.primary_rack = rack_primary
+        vlan.secondary_rack = rack_secondary
+        vlan.dhcp_on = True
+        vlan.save()
+
         configuration = generate_ntp_configuration(machine)
         self.assertThat(dict(configuration), Equals({
             "ntp": {
-                "servers": [machine.boot_cluster_ip],
+                "servers": sorted((
+                    rack_primary_address.ip,
+                    rack_secondary_address.ip,
+                )),
             },
         }))

=== modified file 'src/metadataserver/vendor_data.py'
--- src/metadataserver/vendor_data.py	2016-09-16 16:24:22 +0000
+++ src/metadataserver/vendor_data.py	2016-09-20 07:36:22 +0000
@@ -7,9 +7,12 @@
     'get_vendor_data',
     ]
 
+from collections import defaultdict
 from itertools import chain
+import random
 
 from maasserver.models.config import Config
+from maasserver.routablepairs import find_addresses_between_nodes
 from provisioningserver.utils.text import (
     make_gecos_field,
     split_string_list,
@@ -58,14 +61,31 @@
         ntp_servers = Config.objects.get_config("ntp_servers")
         ntp_servers = set(split_string_list(ntp_servers))
     else:
-        if node.boot_cluster_ip is None:
-            # For now this means there are no NTP servers but this will change
-            # with the "routable pairs" work, at which point we'll be able to
-            # easily calculate all rack addresses routable from the node.
-            ntp_servers = set()
-        else:
-            # Point the node back to the rack it booted from.
-            ntp_servers = {node.boot_cluster_ip}
+        # Point the node back to its primary and secondary rack controllers as
+        # a source of time information.
+        rack_controllers = node.get_boot_rack_controllers()
+        routable_addrs = find_addresses_between_nodes([node], rack_controllers)
+        routable_addrs_by_rack = _group_addresses_by_right_node(routable_addrs)
+        # Choose one routable address per rack at random.
+        ntp_servers = {
+            random.choice(address).format()
+            for address in routable_addrs_by_rack.values()
+        }
 
     if len(ntp_servers) >= 1:
         yield "ntp", {"servers": sorted(ntp_servers)}
+
+
+def _group_addresses_by_right_node(addresses):
+    """Group `addresses` by the "right" node.
+
+    Effectively this assumes that there is only one "left" node and thus
+    ignores it; it's only concerned with grouping addresses on the "right".
+
+    :param addresses: The output from `find_addresses_between_nodes`.
+    :return: A dict mapping "right" nodes to a list of their IP addresses.
+    """
+    collated = defaultdict(list)
+    for _, _, right_node, right_ip in addresses:
+        collated[right_node].append(right_ip)
+    return collated