Merge lp:~allenap/maas/ntp-prefer-nearby-servers--bug-1675095 into lp:~maas-committers/maas/trunk

[Gavin Panella]

Calculated routes between nodes are now prioritised.

This is necessarily hand-wavy because it's nearly impossible to anticipate the cost exactly. Instead a cost is calculated based on a few expectations:

- Sending traffic within a single host is fastest.
- Sending traffic within the same subnet is next.
- Then, sending traffic within the same VLAN.
- Then, sending traffic within the same space.
- Then, sending traffic within the null space.

These costs are relative to one another only, and they do not convey relative _magnitudes_ of cost. They are useful for ordering only.

[Andres Rodriguez (andreserl)]

So, I've tested this and seems to work as expected. I only, however, tested for commissioning and deployment and didn't test for enlistment.

IIRC, for enlistment, we would use the Region IP address the rack is connected to instead of the rack controller IP address.

This doesn't seem to change that, but I just want to make sure that is the case.

review: Approve

[Gavin Panella (allenap)]

Where we have no prior knowledge of which racks a machine has booted from — i.e. during enlistment — we configure NTP to talk to all _rack_ controllers. One address per rack is chosen according to the new metric calculation that this branch introduces. Only rack controllers talk NTP to region controllers, never machines or devices.

Thanks for testing this out. I'll give it a run through Jenkins anyway before I land.

[Andres Rodriguez (andreserl)]

I already did run it through the CI :). I'm gonna go ahead and land.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~allenap/maas/ntp-prefer-nearby-servers--bug-1675095 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [102 kB]
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease [102 kB]
Get:4 http://security.ubuntu.com/ubuntu xenial-security InRelease [102 kB]
Fetched 306 kB in 0s (667 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libnss-wrapper libpq-dev make nodejs-legacy npm postgresql psmisc pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
psmisc is already the newest version (22.21-2.1build1).
pxelinux is already the newest version (3:6.03+dfsg-11ubuntu1).
python-formencode is already the newest version (1.3.0-0ubuntu5).
python-lxml is already the newest version (3.5.0-1build1).
python-netaddr is already the newest version (0.7.18...

=== modified file 'src/maasserver/dbviews.py'
--- src/maasserver/dbviews.py	2017-03-07 16:26:47 +0000
+++ src/maasserver/dbviews.py	2017-03-31 09:18:27 +0000
@@ -36,6 +36,7 @@
     with closing(connection.cursor()) as cursor:
         cursor.execute(view_sql)
 
+
 # Note that the `Discovery` model object is backed by this view. Any
 # changes made to this view should be reflected there.
 maasserver_discovery = dedent("""\
@@ -102,17 +103,33 @@
 # should not be used without constraining, say, the sets of nodes, to find
 # addresses that are mutually routable between region controllers for example.
 maasserver_routable_pairs = dedent("""\
-    SELECT if_left.node_id AS left_node_id,
+    SELECT
+           -- "Left" node.
+           if_left.node_id AS left_node_id,
            if_left.id AS left_interface_id,
            subnet_left.id AS left_subnet_id,
            vlan_left.id AS left_vlan_id,
            sip_left.ip AS left_ip,
+
+           -- "Right" node.
            if_right.node_id AS right_node_id,
            if_right.id AS right_interface_id,
            subnet_right.id AS right_subnet_id,
            vlan_right.id AS right_vlan_id,
            sip_right.ip AS right_ip,
-           vlan_left.space_id AS space_id
+
+           -- Space that left and right have in commmon. Can be NULL.
+           vlan_left.space_id AS space_id,
+
+           -- Relative metric; lower is better.
+           CASE
+             WHEN if_left.node_id = if_right.node_id THEN 0
+             WHEN subnet_left.id = subnet_right.id THEN 1
+             WHEN vlan_left.id = vlan_right.id THEN 2
+             WHEN vlan_left.space_id IS NOT NULL THEN 3
+             ELSE 4  -- The NULL space.
+           END AS metric
+
       FROM maasserver_interface AS if_left
       JOIN maasserver_interface_ip_addresses AS ifia_left
         ON if_left.id = ifia_left.interface_id
 
=== modified file 'src/maasserver/ntp.py'
--- src/maasserver/ntp.py	2016-10-03 20:10:53 +0000
+++ src/maasserver/ntp.py	2017-03-31 09:18:27 +0000
@@ -124,8 +124,9 @@
         routable_addrs_map: AddressMap) -> Iterable[IPAddress]:
     """Choose one routable address per destination node.
 
-    The result is stable, preferring IPv6 over IPv4, and then the highest
-    address numberically. (In fact it relies on the ordering provided by
-    `netaddr.IPAddress`, which is basically that.)
+    The addresses are in preference order (see `find_addresses_between_nodes`
+    for information on how that's derived) so this simply chooses the first.
     """
-    return map(max, routable_addrs_map.values())
+    for addresses in routable_addrs_map.values():
+        assert len(addresses) >= 1
+        yield addresses[0]
 
=== modified file 'src/maasserver/regiondservices/tests/test_ntp.py'
--- src/maasserver/regiondservices/tests/test_ntp.py	2017-01-28 00:51:47 +0000
+++ src/maasserver/regiondservices/tests/test_ntp.py	2017-03-31 09:18:27 +0000
@@ -18,13 +18,17 @@
 from maasserver.utils.threads import deferToDatabase
 from maastesting.fixtures import MAASRootFixture
 from maastesting.matchers import (
+    ContainedBy,
     DocTestMatches,
+    Matches,
+    MockCalledOnce,
     MockCalledOnceWith,
 )
 from maastesting.testcase import MAASTestCase
 from maastesting.twisted import TwistedLoggerFixture
 from provisioningserver.utils.testing import MAASIDFixture
 from testtools.matchers import (
+    AllMatch,
     Equals,
     IsInstance,
     MatchesStructure,
@@ -78,25 +82,25 @@
         self.useFixture(MAASIDFixture(region.system_id))
         peer1, addr1_4, addr1_6 = make_region_with_address(space)
         peer2, addr2_4, addr2_6 = make_region_with_address(space)
-        # Create a configuration object.
-        return ntp._Configuration(ntp_servers, {addr1_6.ip, addr2_6.ip})
+        # Return the servers and all possible peer IP addresses.
+        return ntp_servers, {addr1_4.ip, addr1_6.ip, addr2_4.ip, addr2_6.ip}
 
     @wait_for_reactor
     @inlineCallbacks
     def test__tryUpdate_updates_ntp_server(self):
         service = ntp.RegionNetworkTimeProtocolService(reactor)
-        configuration = yield deferToDatabase(self.make_example_configuration)
+        refs, peers = yield deferToDatabase(self.make_example_configuration)
         configure_region = self.patch_autospec(ntp, "configure_region")
         restartService = self.patch_autospec(service_monitor, "restartService")
         yield service._tryUpdate()
-        self.assertThat(configure_region, MockCalledOnceWith(
-            configuration.references, configuration.peers))
+        self.assertThat(
+            configure_region, MockCalledOnceWith(
+                refs, Matches(AllMatch(ContainedBy(peers)))))
         self.assertThat(restartService, MockCalledOnceWith("ntp_region"))
         # If the configuration has not changed then a second call to
         # `_tryUpdate` does not result in another call to `configure_region`.
         yield service._tryUpdate()
-        self.assertThat(configure_region, MockCalledOnceWith(
-            configuration.references, configuration.peers))
+        self.assertThat(configure_region, MockCalledOnce())
         self.assertThat(restartService, MockCalledOnceWith("ntp_region"))
 
 
@@ -147,15 +151,15 @@
         # Put all addresses in the same space so they're mutually routable.
         space = factory.make_Space()
         # Populate the database with "this" region and an example peer.
-        region, addr4, addr6 = make_region_with_address(space)
+        region, _, _ = make_region_with_address(space)
         self.useFixture(MAASIDFixture(region.system_id))
         peer, addr4, addr6 = make_region_with_address(space)
 
         observed = service._getConfiguration()
         self.assertThat(observed, IsInstance(ntp._Configuration))
 
-        expected_references = frozenset(ntp_servers)
-        expected_peers = {addr6.ip}  # IPv6 is preferred.
+        expected_references = Equals(frozenset(ntp_servers))
+        expected_peers = AllMatch(ContainedBy({addr4.ip, addr6.ip}))
 
-        self.assertThat(observed, MatchesStructure.byEquality(
+        self.assertThat(observed, MatchesStructure(
             references=expected_references, peers=expected_peers))
 
=== modified file 'src/maasserver/routablepairs.py'
--- src/maasserver/routablepairs.py	2016-09-19 14:50:27 +0000
+++ src/maasserver/routablepairs.py	2017-03-31 09:18:27 +0000
@@ -24,6 +24,17 @@
     ``node-left`` and ``node-right`` are one of the nodes passed in and
     ``addr-left`` and ``addr-right`` are :class:`netaddr.IPAddress` instances
     corresponding to an active address on their respective nodes.
+
+    The results are sorted, lowest metric first, meaning that earlier
+    addresses are likely to have a lower overall communications cost, lower
+    latency, and so on. The order of preference is:
+
+    - Same node
+    - Same subnet
+    - Same VLAN
+    - Same space
+
+    An explicitly defined space is preferred to the default / null space.
     """
     nodes_left = {node.id: node for node in nodes_left}
     nodes_right = {node.id: node for node in nodes_right}
@@ -48,6 +59,7 @@
       FROM maasserver_routable_pairs
      WHERE left_node_id IN (%s)
        AND right_node_id IN (%s)
+     ORDER BY metric ASC
 """)
 
 
 
=== modified file 'src/maasserver/tests/test_dbviews.py'
--- src/maasserver/tests/test_dbviews.py	2017-01-28 00:51:47 +0000
+++ src/maasserver/tests/test_dbviews.py	2017-03-31 09:18:27 +0000
@@ -10,6 +10,7 @@
     _ALL_VIEWS,
     register_all_views,
 )
+from maasserver.models.subnet import Subnet
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from testtools.matchers import HasLength
@@ -39,29 +40,97 @@
             cursor.execute("SELECT * from maasserver_routable_pairs")
             self.assertThat(cursor.fetchall(), HasLength(0))
 
-    def make_node_with_address(self, space, cidr):
+    def make_node_with_address(self, cidr, space=None, vlan=None):
         node = factory.make_Node()
         iface = factory.make_Interface(node=node)
-        subnet = factory.make_Subnet(space=space, cidr=cidr)
+        try:
+            subnet = Subnet.objects.get(cidr=cidr, vlan__space=space)
+        except Subnet.DoesNotExist:
+            subnet = factory.make_Subnet(cidr=cidr, space=space, vlan=vlan)
         sip = factory.make_StaticIPAddress(interface=iface, subnet=subnet)
         return node, iface, subnet, sip
 
+    def test__contains_routes_between_nodes_on_same_subnet(self):
+        network = factory.make_ip4_or_6_network()
+        node1, if1, sn1, sip1 = self.make_node_with_address(network)
+        node2, if2, sn2, sip2 = self.make_node_with_address(network)
+
+        # Routes between all addresses are found, even back to themselves.
+        left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
+        right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
+        row = lambda ent1, ent2, metric: (*ent1, *ent2, None, metric)
+        expected = [
+            row(left, left, 0),
+            row(left, right, 1),  # Same space, hence metric of 3.
+            row(right, right, 0),
+            row(right, left, 1),  # Same space, hence metric of 3.
+        ]
+
+        with connection.cursor() as cursor:
+            cursor.execute("SELECT * from maasserver_routable_pairs")
+            self.assertItemsEqual(expected, cursor.fetchall())
+
+    def test__contains_routes_between_nodes_on_same_vlan(self):
+        vlan = factory.make_VLAN()
+        network1 = factory.make_ip4_or_6_network()
+        network2 = factory.make_ip4_or_6_network(version=network1.version)
+        node1, if1, sn1, sip1 = self.make_node_with_address(
+            network1, vlan=vlan)
+        node2, if2, sn2, sip2 = self.make_node_with_address(
+            network2, vlan=vlan)
+
+        # Routes between all addresses are found, even back to themselves.
+        left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
+        right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
+        row = lambda ent1, ent2, metric: (*ent1, *ent2, None, metric)
+        expected = [
+            row(left, left, 0),
+            row(left, right, 2),  # Same VLAN, hence metric of 2.
+            row(right, right, 0),
+            row(right, left, 2),  # Same VLAN, hence metric of 2.
+        ]
+
+        with connection.cursor() as cursor:
+            cursor.execute("SELECT * from maasserver_routable_pairs")
+            self.assertItemsEqual(expected, cursor.fetchall())
+
     def test__contains_routes_between_nodes_on_same_space(self):
         space = factory.make_Space()
         network1 = factory.make_ip4_or_6_network()
         network2 = factory.make_ip4_or_6_network(version=network1.version)
-        node1, if1, sn1, sip1 = self.make_node_with_address(space, network1)
-        node2, if2, sn2, sip2 = self.make_node_with_address(space, network2)
-
-        # Routes between all addresses are found, even back to themselves.
-        left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
-        right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
-        row = lambda ent1, ent2: ent1 + ent2 + (space.id, )
-        expected = [
-            row(left, left),
-            row(left, right),
-            row(right, right),
-            row(right, left),
+        node1, if1, sn1, sip1 = self.make_node_with_address(network1, space)
+        node2, if2, sn2, sip2 = self.make_node_with_address(network2, space)
+
+        # Routes between all addresses are found, even back to themselves.
+        left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
+        right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
+        row = lambda ent1, ent2, metric: (*ent1, *ent2, space.id, metric)
+        expected = [
+            row(left, left, 0),
+            row(left, right, 3),  # Same space, hence metric of 3.
+            row(right, right, 0),
+            row(right, left, 3),  # Same space, hence metric of 3.
+        ]
+
+        with connection.cursor() as cursor:
+            cursor.execute("SELECT * from maasserver_routable_pairs")
+            self.assertItemsEqual(expected, cursor.fetchall())
+
+    def test__contains_routes_between_nodes_via_null_space(self):
+        network1 = factory.make_ip4_or_6_network()
+        network2 = factory.make_ip4_or_6_network(version=network1.version)
+        node1, if1, sn1, sip1 = self.make_node_with_address(network1)
+        node2, if2, sn2, sip2 = self.make_node_with_address(network2)
+
+        # Routes between all addresses are found, even back to themselves.
+        left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
+        right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
+        row = lambda ent1, ent2, metric: (*ent1, *ent2, None, metric)
+        expected = [
+            row(left, left, 0),
+            row(left, right, 4),  # The NULL space, hence metric of 4.
+            row(right, right, 0),
+            row(right, left, 4),  # The NULL space, hence metric of 4.
         ]
 
         with connection.cursor() as cursor:
@@ -73,8 +142,8 @@
         space2 = factory.make_Space()
         network1 = factory.make_ip4_or_6_network()
         network2 = factory.make_ip4_or_6_network(version=network1.version)
-        node1, if1, sn1, sip1 = self.make_node_with_address(space1, network1)
-        node2, if2, sn2, sip2 = self.make_node_with_address(space2, network2)
+        node1, if1, sn1, sip1 = self.make_node_with_address(network1, space1)
+        node2, if2, sn2, sip2 = self.make_node_with_address(network2, space2)
 
         # Only routes from left to left and right to right are found: right is
         # not routable from left, and left is not routable from right because
@@ -82,8 +151,8 @@
         left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
         right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
         expected = [
-            left + left + (space1.id,),
-            right + right + (space2.id,),
+            (*left, *left, space1.id, 0),
+            (*right, *right, space2.id, 0),
         ]
 
         with connection.cursor() as cursor:
@@ -95,8 +164,8 @@
         network1 = factory.make_ip4_or_6_network()
         network2 = factory.make_ip4_or_6_network(
             version=(4 if network1.version == 6 else 6))
-        node1, if1, sn1, sip1 = self.make_node_with_address(space, network1)
-        node2, if2, sn2, sip2 = self.make_node_with_address(space, network2)
+        node1, if1, sn1, sip1 = self.make_node_with_address(network1, space)
+        node2, if2, sn2, sip2 = self.make_node_with_address(network2, space)
 
         # Only routes from left to left and right to right are found: right is
         # not routable from left, and left is not routable from right because
@@ -104,8 +173,8 @@
         left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
         right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
         expected = [
-            left + left + (space.id,),
-            right + right + (space.id,),
+            (*left, *left, space.id, 0),
+            (*right, *right, space.id, 0),
         ]
 
         with connection.cursor() as cursor:
 
=== modified file 'src/maasserver/tests/test_ntp.py'
--- src/maasserver/tests/test_ntp.py	2017-01-28 00:51:47 +0000
+++ src/maasserver/tests/test_ntp.py	2017-03-31 09:18:27 +0000
@@ -5,8 +5,6 @@
 
 __all__ = []
 
-from operator import methodcaller
-
 from maasserver.models.config import Config
 from maasserver.ntp import (
     get_peers_for,
@@ -14,10 +12,12 @@
 )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from netaddr import IPAddress
+from netaddr import (
+    IPAddress,
+    IPSet,
+)
 from testtools.matchers import (
     AfterPreprocessing,
-    AllMatch,
     ContainsAll,
     Equals,
     HasLength,
@@ -231,7 +231,7 @@
 
     For racks, machines, and devices, a selection process takes place to
     determine which of several candidate addresses per server to choose. This
-    result is stable, i.e. it will choose the same address each time.
+    result is semi-stable, i.e. it will always prefer "closer" addresses.
     """
 
     scenarios = (
@@ -253,23 +253,31 @@
         super(TestGetServersFor_Selection, self).setUp()
         Config.objects.set_config("ntp_external_only", False)
 
-    def test_prefers_ipv6_to_ipv4_peers_then_highest_numerically(self):
+    def test_prefers_closest_addresses(self):
         subnet4 = factory.make_Subnet(version=4)
-        subnet6a = factory.make_Subnet(version=6)
-        subnet6b = factory.make_Subnet(version=6)
-        # Ensure that addresses in subnet6a < those in subnet6b.
-        subnet6a, subnet6b = sorted(
-            {subnet6a, subnet6b}, key=methodcaller("get_ipnetwork"))
-        # Create a node and server with an address in each subnet.
-        node, server = self.make_node(), self.make_server()
-        subnets = {subnet4, subnet6a, subnet6b}
-        populate_node_with_addresses(node, subnets)
-        populate_node_with_addresses(server, subnets)
+        subnet6 = factory.make_Subnet(version=6)
+        # Separate subnets but sharing the VLAN, hence routable.
+        subnet4v = factory.make_Subnet(version=4, vlan=subnet4.vlan)
+        subnet6v = factory.make_Subnet(version=6, vlan=subnet6.vlan)
+
+        # Create a node with an address in the first two subnets...
+        node = self.make_node()
+        populate_node_with_addresses(node, {subnet4, subnet6})
+        # ... and a server with an address in every subnet.
+        server = self.make_server()
+        populate_node_with_addresses(
+            server, {subnet4, subnet6, subnet4v, subnet6v})
+
+        # The NTP server addresses chosen will be those that are "closest" to
+        # the node, and same-subnet wins in this over same-VLAN. No additional
+        # preference is made between IPv4 or IPv6, hence we allow for either.
+        preferred_subnets = subnet4, subnet6
+        preferred_networks = IPSet(
+            subnet.get_ipnetwork() for subnet in preferred_subnets)
 
         servers = get_servers_for(node)
         self.assertThat(servers, Not(HasLength(0)))
-        self.assertThat(servers, AllMatch(IsIPv6Address))
-        self.assertThat(subnet6b.get_ipnetwork(), ContainsAll(servers))
+        self.assertThat(preferred_networks, ContainsAll(servers))
 
 
 class TestGetPeersFor_Region_RegionRack(MAASServerTestCase):
@@ -296,23 +304,37 @@
             get_peers_for(node2),
             IsSetOfServers({node1_address.ip}))
 
-    def test_prefers_ipv6_to_ipv4_peers_then_highest_numerically(self):
+    def test_prefers_closest_addresses(self):
         subnet4 = factory.make_Subnet(version=4)
-        subnet6a = factory.make_Subnet(version=6)
-        subnet6b = factory.make_Subnet(version=6)
-        # Ensure that addresses in subnet6a < those in subnet6b.
-        subnet6a, subnet6b = sorted(
-            {subnet6a, subnet6b}, key=methodcaller("get_ipnetwork"))
-        # Create some peers, each with an address in each subnet.
-        nodes = self.make_node(), self.make_node()
-        subnets = {subnet4, subnet6a, subnet6b}
-        for node in nodes:
-            populate_node_with_addresses(node, subnets)
-        for node in nodes:
+        subnet6 = factory.make_Subnet(version=6)
+        # Separate subnets but sharing the VLAN, hence routable.
+        subnet4v1 = factory.make_Subnet(version=4, vlan=subnet4.vlan)
+        subnet6v1 = factory.make_Subnet(version=6, vlan=subnet6.vlan)
+        subnet4v2 = factory.make_Subnet(version=4, vlan=subnet4.vlan)
+        subnet6v2 = factory.make_Subnet(version=6, vlan=subnet6.vlan)
+
+        # Create a node with an address in the first two subnets and the first
+        # two same-VLAN subnets.
+        node1 = self.make_node()
+        populate_node_with_addresses(
+            node1, {subnet4, subnet6, subnet4v1, subnet6v1})
+        # Create a node with an address in the first two subnets and the
+        # second two same-VLAN subnets.
+        node2 = self.make_node()
+        populate_node_with_addresses(
+            node2, {subnet4, subnet6, subnet4v2, subnet6v2})
+
+        # The NTP server addresses chosen will be those that are "closest" to
+        # the node, and same-subnet wins in this over same-VLAN. No additional
+        # preference is made between IPv4 or IPv6, hence we allow for either.
+        preferred_subnets = subnet4, subnet6
+        preferred_networks = IPSet(
+            subnet.get_ipnetwork() for subnet in preferred_subnets)
+
+        for node in (node1, node2):
             peers = get_peers_for(node)
             self.assertThat(peers, Not(HasLength(0)))
-            self.assertThat(peers, AllMatch(IsIPv6Address))
-            self.assertThat(subnet6b.get_ipnetwork(), ContainsAll(peers))
+            self.assertThat(preferred_networks, ContainsAll(peers))
 
 
 class TestGetPeersFor_Other(MAASServerTestCase):
 
=== modified file 'src/maasserver/tests/test_routablepairs.py'
--- src/maasserver/tests/test_routablepairs.py	2017-01-28 00:51:47 +0000
+++ src/maasserver/tests/test_routablepairs.py	2017-03-31 09:18:27 +0000
@@ -5,13 +5,21 @@
 
 __all__ = []
 
-from itertools import product
+from itertools import (
+    product,
+    takewhile,
+)
+import random
 
 from maasserver.models.node import Node
 from maasserver.routablepairs import find_addresses_between_nodes
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from testtools import ExpectedException
+from testtools.matchers import (
+    AfterPreprocessing,
+    Equals,
+)
 
 
 class TestFindAddressesBetweenNodes(MAASServerTestCase):
@@ -102,3 +110,124 @@
         # No routable addresses are found.
         self.assertItemsEqual(
             expected, find_addresses_between_nodes([node1], [node2]))
+
+    def gen_disjoint_networks(self):
+        """Generate disjoint networks.
+
+        Can be IPv4 or IPv6, but once generation has begun they'll all be the
+        same family.
+        """
+        make_network = random.choice([
+            factory.make_ipv4_network,
+            factory.make_ipv6_network]
+        )
+        networks = []
+        while True:
+            network = make_network(disjoint_from=networks)
+            networks.append(network)
+            yield network
+
+    def test__yields_routes_with_lowest_metrics_first(self):
+        space = factory.make_Space()
+        # Ensure networks are disjoint but of the same family.
+        networks = self.gen_disjoint_networks()
+
+        # Create the node for the "left" that has two IP addresses, one in the
+        # null space, one in a non-null space.
+        origin = factory.make_Node(hostname="origin")
+        origin_iface = factory.make_Interface(
+            node=origin, disconnected=True)
+        origin_subnet = factory.make_Subnet(
+            space=space, cidr=next(networks))
+        origin_subnet_null_space = factory.make_Subnet(
+            space=None, cidr=next(networks))
+        origin_sip = factory.make_StaticIPAddress(
+            interface=origin_iface, subnet=origin_subnet)
+        origin_sip_null_space = factory.make_StaticIPAddress(
+            interface=origin_iface, subnet=origin_subnet_null_space)
+
+        # Same subnet, different node.
+        node_same_subnet = factory.make_Node(hostname="same-subnet")
+        sip_same_subnet = factory.make_StaticIPAddress(
+            interface=factory.make_Interface(
+                node=node_same_subnet, disconnected=True),
+            subnet=origin_subnet)
+
+        # Same VLAN, different subnet, different node.
+        node_same_vlan = factory.make_Node(hostname="same-vlan")
+        sip_same_vlan = factory.make_StaticIPAddress(
+            interface=factory.make_Interface(
+                node=node_same_vlan, disconnected=True),
+            subnet=factory.make_Subnet(
+                space=space, vlan=origin_subnet.vlan,
+                cidr=next(networks)))
+
+        # Same space, different VLAN, subnet, and node.
+        node_same_space = factory.make_Node(hostname="same-space")
+        sip_same_space = factory.make_StaticIPAddress(
+            interface=factory.make_Interface(
+                node=node_same_space, disconnected=True),
+            subnet=factory.make_Subnet(
+                space=space, cidr=next(networks)))
+
+        # Null space, different VLAN, subnet, and node.
+        node_null_space = factory.make_Node(hostname="null-space")
+        sip_null_space = factory.make_StaticIPAddress(
+            interface=factory.make_Interface(
+                node=node_null_space, disconnected=True),
+            subnet=factory.make_Subnet(
+                space=None, cidr=next(networks)))
+
+        # We'll search for routes between `lefts` and `rights`.
+        lefts = [
+            origin,
+        ]
+        rights = [
+            node_same_subnet,
+            node_same_vlan,
+            node_same_space,
+            node_null_space,
+        ]
+
+        # This is in order, lowest "metric" first.
+        expected = [
+            (origin, origin_sip.get_ipaddress(),
+             node_same_subnet, sip_same_subnet.get_ipaddress()),
+            (origin, origin_sip.get_ipaddress(),
+             node_same_vlan, sip_same_vlan.get_ipaddress()),
+            (origin, origin_sip.get_ipaddress(),
+             node_same_space, sip_same_space.get_ipaddress()),
+            (origin, origin_sip_null_space.get_ipaddress(),
+             node_null_space, sip_null_space.get_ipaddress()),
+        ]
+        self.assertThat(
+            find_addresses_between_nodes(lefts, rights),
+            AfterPreprocessing(list, Equals(expected)))
+
+        # Same node, same space, different VLAN and subnet. We did not add
+        # this earlier because its existence allows for a large number of
+        # additional routes between the origin and the other nodes, which
+        # would have obscured the test.
+        origin_sip_2 = factory.make_StaticIPAddress(
+            interface=factory.make_Interface(
+                node=origin, disconnected=True),
+            subnet=factory.make_Subnet(
+                space=space, cidr=next(networks)))
+
+        # Now the first addresses returned are between those addresses we
+        # created on the same node, in no particular order.
+        origin_ips = origin_sip.get_ipaddress(), origin_sip_2.get_ipaddress()
+        expected_mutual = {
+            (origin, ip1, origin, ip2)
+            for ip1, ip2 in product(origin_ips, origin_ips)
+        }
+        # There's a mutual route for the null-space IP address too.
+        expected_mutual.add(
+            (origin, origin_sip_null_space.get_ipaddress(),
+             origin, origin_sip_null_space.get_ipaddress()))
+        observed_mutual = takewhile(
+            (lambda route: route[0] == route[2]),  # Route is mutual.
+            find_addresses_between_nodes(lefts, [origin, *rights]),
+        )
+        self.assertItemsEqual(
+            expected_mutual, observed_mutual)
 
=== modified file 'src/maastesting/matchers.py'
--- src/maastesting/matchers.py	2016-12-07 12:46:14 +0000
+++ src/maastesting/matchers.py	2017-03-31 09:18:27 +0000
@@ -4,6 +4,7 @@
 """testtools custom matchers"""
 
 __all__ = [
+    'ContainedBy',
     'DocTestMatches',
     'FileContains',
     'GreaterThanOrEqual',
@@ -481,3 +482,20 @@
         (lambda observed: not observed.isspace()),
         "%r is whitespace"),
     first_only=True)
+
+
+class ContainedBy(Matcher):
+    """Test if the matchee is in the given container."""
+
+    def __init__(self, haystack):
+        super(ContainedBy, self).__init__()
+        self.haystack = haystack
+
+    def __str__(self):
+        return "%s(%r)" % (
+            self.__class__.__name__, self.haystack)
+
+    def match(self, needle):
+        if needle not in self.haystack:
+            return Mismatch(
+                "%r not in %r" % (needle, self.haystack))
 
=== modified file 'src/maastesting/tests/test_matchers.py'
--- src/maastesting/tests/test_matchers.py	2016-12-07 12:46:14 +0000
+++ src/maastesting/tests/test_matchers.py	2017-03-31 09:18:27 +0000
@@ -18,6 +18,7 @@
 from maastesting import matchers
 from maastesting.factory import factory
 from maastesting.matchers import (
+    ContainedBy,
     FileContains,
     GreaterThanOrEqual,
     HasAttribute,
@@ -588,3 +589,13 @@
     def test_does_not_match_non_strings(self):
         self.assertMismatch(
             IsNonEmptyString.match(1234), "1234 is not a string")
+
+
+class TestContainedBy(MAASTestCase, MockTestMixin):
+    """Tests for the `ContainedBy` matcher."""
+
+    def test_matches_needle_in_haystack(self):
+        self.assertThat("foo", ContainedBy({"foo", "bar"}))
+
+    def test_does_not_match_needle_not_in_haystack(self):
+        self.assertMismatch(ContainedBy([]).match("foo"), "'foo' not in []")