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

Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 5898
Proposed branch: lp:~allenap/maas/ntp-prefer-nearby-servers--bug-1675095
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 683 lines (+355/-72)
9 files modified
src/maasserver/dbviews.py (+19/-2)
src/maasserver/ntp.py (+5/-4)
src/maasserver/regiondservices/tests/test_ntp.py (+15/-11)
src/maasserver/routablepairs.py (+12/-0)
src/maasserver/tests/test_dbviews.py (+91/-22)
src/maasserver/tests/test_ntp.py (+54/-32)
src/maasserver/tests/test_routablepairs.py (+130/-1)
src/maastesting/matchers.py (+18/-0)
src/maastesting/tests/test_matchers.py (+11/-0)
To merge this branch: bzr merge lp:~allenap/maas/ntp-prefer-nearby-servers--bug-1675095
Reviewer Review Type Date Requested Status
Andres Rodriguez (community) Approve
Review via email: mp+321319@code.launchpad.net

Commit message

Calculated routes between nodes are now prioritised.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Andres Rodriguez (andreserl) wrote :

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
Revision history for this message
Gavin Panella (allenap) wrote :

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.

Revision history for this message
Andres Rodriguez (andreserl) wrote :

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

Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (1.8 MiB)

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...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/dbviews.py'
2--- src/maasserver/dbviews.py 2017-03-07 16:26:47 +0000
3+++ src/maasserver/dbviews.py 2017-03-31 09:18:27 +0000
4@@ -36,6 +36,7 @@
5 with closing(connection.cursor()) as cursor:
6 cursor.execute(view_sql)
7
8+
9 # Note that the `Discovery` model object is backed by this view. Any
10 # changes made to this view should be reflected there.
11 maasserver_discovery = dedent("""\
12@@ -102,17 +103,33 @@
13 # should not be used without constraining, say, the sets of nodes, to find
14 # addresses that are mutually routable between region controllers for example.
15 maasserver_routable_pairs = dedent("""\
16- SELECT if_left.node_id AS left_node_id,
17+ SELECT
18+ -- "Left" node.
19+ if_left.node_id AS left_node_id,
20 if_left.id AS left_interface_id,
21 subnet_left.id AS left_subnet_id,
22 vlan_left.id AS left_vlan_id,
23 sip_left.ip AS left_ip,
24+
25+ -- "Right" node.
26 if_right.node_id AS right_node_id,
27 if_right.id AS right_interface_id,
28 subnet_right.id AS right_subnet_id,
29 vlan_right.id AS right_vlan_id,
30 sip_right.ip AS right_ip,
31- vlan_left.space_id AS space_id
32+
33+ -- Space that left and right have in commmon. Can be NULL.
34+ vlan_left.space_id AS space_id,
35+
36+ -- Relative metric; lower is better.
37+ CASE
38+ WHEN if_left.node_id = if_right.node_id THEN 0
39+ WHEN subnet_left.id = subnet_right.id THEN 1
40+ WHEN vlan_left.id = vlan_right.id THEN 2
41+ WHEN vlan_left.space_id IS NOT NULL THEN 3
42+ ELSE 4 -- The NULL space.
43+ END AS metric
44+
45 FROM maasserver_interface AS if_left
46 JOIN maasserver_interface_ip_addresses AS ifia_left
47 ON if_left.id = ifia_left.interface_id
48
49=== modified file 'src/maasserver/ntp.py'
50--- src/maasserver/ntp.py 2016-10-03 20:10:53 +0000
51+++ src/maasserver/ntp.py 2017-03-31 09:18:27 +0000
52@@ -124,8 +124,9 @@
53 routable_addrs_map: AddressMap) -> Iterable[IPAddress]:
54 """Choose one routable address per destination node.
55
56- The result is stable, preferring IPv6 over IPv4, and then the highest
57- address numberically. (In fact it relies on the ordering provided by
58- `netaddr.IPAddress`, which is basically that.)
59+ The addresses are in preference order (see `find_addresses_between_nodes`
60+ for information on how that's derived) so this simply chooses the first.
61 """
62- return map(max, routable_addrs_map.values())
63+ for addresses in routable_addrs_map.values():
64+ assert len(addresses) >= 1
65+ yield addresses[0]
66
67=== modified file 'src/maasserver/regiondservices/tests/test_ntp.py'
68--- src/maasserver/regiondservices/tests/test_ntp.py 2017-01-28 00:51:47 +0000
69+++ src/maasserver/regiondservices/tests/test_ntp.py 2017-03-31 09:18:27 +0000
70@@ -18,13 +18,17 @@
71 from maasserver.utils.threads import deferToDatabase
72 from maastesting.fixtures import MAASRootFixture
73 from maastesting.matchers import (
74+ ContainedBy,
75 DocTestMatches,
76+ Matches,
77+ MockCalledOnce,
78 MockCalledOnceWith,
79 )
80 from maastesting.testcase import MAASTestCase
81 from maastesting.twisted import TwistedLoggerFixture
82 from provisioningserver.utils.testing import MAASIDFixture
83 from testtools.matchers import (
84+ AllMatch,
85 Equals,
86 IsInstance,
87 MatchesStructure,
88@@ -78,25 +82,25 @@
89 self.useFixture(MAASIDFixture(region.system_id))
90 peer1, addr1_4, addr1_6 = make_region_with_address(space)
91 peer2, addr2_4, addr2_6 = make_region_with_address(space)
92- # Create a configuration object.
93- return ntp._Configuration(ntp_servers, {addr1_6.ip, addr2_6.ip})
94+ # Return the servers and all possible peer IP addresses.
95+ return ntp_servers, {addr1_4.ip, addr1_6.ip, addr2_4.ip, addr2_6.ip}
96
97 @wait_for_reactor
98 @inlineCallbacks
99 def test__tryUpdate_updates_ntp_server(self):
100 service = ntp.RegionNetworkTimeProtocolService(reactor)
101- configuration = yield deferToDatabase(self.make_example_configuration)
102+ refs, peers = yield deferToDatabase(self.make_example_configuration)
103 configure_region = self.patch_autospec(ntp, "configure_region")
104 restartService = self.patch_autospec(service_monitor, "restartService")
105 yield service._tryUpdate()
106- self.assertThat(configure_region, MockCalledOnceWith(
107- configuration.references, configuration.peers))
108+ self.assertThat(
109+ configure_region, MockCalledOnceWith(
110+ refs, Matches(AllMatch(ContainedBy(peers)))))
111 self.assertThat(restartService, MockCalledOnceWith("ntp_region"))
112 # If the configuration has not changed then a second call to
113 # `_tryUpdate` does not result in another call to `configure_region`.
114 yield service._tryUpdate()
115- self.assertThat(configure_region, MockCalledOnceWith(
116- configuration.references, configuration.peers))
117+ self.assertThat(configure_region, MockCalledOnce())
118 self.assertThat(restartService, MockCalledOnceWith("ntp_region"))
119
120
121@@ -147,15 +151,15 @@
122 # Put all addresses in the same space so they're mutually routable.
123 space = factory.make_Space()
124 # Populate the database with "this" region and an example peer.
125- region, addr4, addr6 = make_region_with_address(space)
126+ region, _, _ = make_region_with_address(space)
127 self.useFixture(MAASIDFixture(region.system_id))
128 peer, addr4, addr6 = make_region_with_address(space)
129
130 observed = service._getConfiguration()
131 self.assertThat(observed, IsInstance(ntp._Configuration))
132
133- expected_references = frozenset(ntp_servers)
134- expected_peers = {addr6.ip} # IPv6 is preferred.
135+ expected_references = Equals(frozenset(ntp_servers))
136+ expected_peers = AllMatch(ContainedBy({addr4.ip, addr6.ip}))
137
138- self.assertThat(observed, MatchesStructure.byEquality(
139+ self.assertThat(observed, MatchesStructure(
140 references=expected_references, peers=expected_peers))
141
142=== modified file 'src/maasserver/routablepairs.py'
143--- src/maasserver/routablepairs.py 2016-09-19 14:50:27 +0000
144+++ src/maasserver/routablepairs.py 2017-03-31 09:18:27 +0000
145@@ -24,6 +24,17 @@
146 ``node-left`` and ``node-right`` are one of the nodes passed in and
147 ``addr-left`` and ``addr-right`` are :class:`netaddr.IPAddress` instances
148 corresponding to an active address on their respective nodes.
149+
150+ The results are sorted, lowest metric first, meaning that earlier
151+ addresses are likely to have a lower overall communications cost, lower
152+ latency, and so on. The order of preference is:
153+
154+ - Same node
155+ - Same subnet
156+ - Same VLAN
157+ - Same space
158+
159+ An explicitly defined space is preferred to the default / null space.
160 """
161 nodes_left = {node.id: node for node in nodes_left}
162 nodes_right = {node.id: node for node in nodes_right}
163@@ -48,6 +59,7 @@
164 FROM maasserver_routable_pairs
165 WHERE left_node_id IN (%s)
166 AND right_node_id IN (%s)
167+ ORDER BY metric ASC
168 """)
169
170
171
172=== modified file 'src/maasserver/tests/test_dbviews.py'
173--- src/maasserver/tests/test_dbviews.py 2017-01-28 00:51:47 +0000
174+++ src/maasserver/tests/test_dbviews.py 2017-03-31 09:18:27 +0000
175@@ -10,6 +10,7 @@
176 _ALL_VIEWS,
177 register_all_views,
178 )
179+from maasserver.models.subnet import Subnet
180 from maasserver.testing.factory import factory
181 from maasserver.testing.testcase import MAASServerTestCase
182 from testtools.matchers import HasLength
183@@ -39,29 +40,97 @@
184 cursor.execute("SELECT * from maasserver_routable_pairs")
185 self.assertThat(cursor.fetchall(), HasLength(0))
186
187- def make_node_with_address(self, space, cidr):
188+ def make_node_with_address(self, cidr, space=None, vlan=None):
189 node = factory.make_Node()
190 iface = factory.make_Interface(node=node)
191- subnet = factory.make_Subnet(space=space, cidr=cidr)
192+ try:
193+ subnet = Subnet.objects.get(cidr=cidr, vlan__space=space)
194+ except Subnet.DoesNotExist:
195+ subnet = factory.make_Subnet(cidr=cidr, space=space, vlan=vlan)
196 sip = factory.make_StaticIPAddress(interface=iface, subnet=subnet)
197 return node, iface, subnet, sip
198
199+ def test__contains_routes_between_nodes_on_same_subnet(self):
200+ network = factory.make_ip4_or_6_network()
201+ node1, if1, sn1, sip1 = self.make_node_with_address(network)
202+ node2, if2, sn2, sip2 = self.make_node_with_address(network)
203+
204+ # Routes between all addresses are found, even back to themselves.
205+ left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
206+ right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
207+ row = lambda ent1, ent2, metric: (*ent1, *ent2, None, metric)
208+ expected = [
209+ row(left, left, 0),
210+ row(left, right, 1), # Same space, hence metric of 3.
211+ row(right, right, 0),
212+ row(right, left, 1), # Same space, hence metric of 3.
213+ ]
214+
215+ with connection.cursor() as cursor:
216+ cursor.execute("SELECT * from maasserver_routable_pairs")
217+ self.assertItemsEqual(expected, cursor.fetchall())
218+
219+ def test__contains_routes_between_nodes_on_same_vlan(self):
220+ vlan = factory.make_VLAN()
221+ network1 = factory.make_ip4_or_6_network()
222+ network2 = factory.make_ip4_or_6_network(version=network1.version)
223+ node1, if1, sn1, sip1 = self.make_node_with_address(
224+ network1, vlan=vlan)
225+ node2, if2, sn2, sip2 = self.make_node_with_address(
226+ network2, vlan=vlan)
227+
228+ # Routes between all addresses are found, even back to themselves.
229+ left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
230+ right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
231+ row = lambda ent1, ent2, metric: (*ent1, *ent2, None, metric)
232+ expected = [
233+ row(left, left, 0),
234+ row(left, right, 2), # Same VLAN, hence metric of 2.
235+ row(right, right, 0),
236+ row(right, left, 2), # Same VLAN, hence metric of 2.
237+ ]
238+
239+ with connection.cursor() as cursor:
240+ cursor.execute("SELECT * from maasserver_routable_pairs")
241+ self.assertItemsEqual(expected, cursor.fetchall())
242+
243 def test__contains_routes_between_nodes_on_same_space(self):
244 space = factory.make_Space()
245 network1 = factory.make_ip4_or_6_network()
246 network2 = factory.make_ip4_or_6_network(version=network1.version)
247- node1, if1, sn1, sip1 = self.make_node_with_address(space, network1)
248- node2, if2, sn2, sip2 = self.make_node_with_address(space, network2)
249-
250- # Routes between all addresses are found, even back to themselves.
251- left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
252- right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
253- row = lambda ent1, ent2: ent1 + ent2 + (space.id, )
254- expected = [
255- row(left, left),
256- row(left, right),
257- row(right, right),
258- row(right, left),
259+ node1, if1, sn1, sip1 = self.make_node_with_address(network1, space)
260+ node2, if2, sn2, sip2 = self.make_node_with_address(network2, space)
261+
262+ # Routes between all addresses are found, even back to themselves.
263+ left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
264+ right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
265+ row = lambda ent1, ent2, metric: (*ent1, *ent2, space.id, metric)
266+ expected = [
267+ row(left, left, 0),
268+ row(left, right, 3), # Same space, hence metric of 3.
269+ row(right, right, 0),
270+ row(right, left, 3), # Same space, hence metric of 3.
271+ ]
272+
273+ with connection.cursor() as cursor:
274+ cursor.execute("SELECT * from maasserver_routable_pairs")
275+ self.assertItemsEqual(expected, cursor.fetchall())
276+
277+ def test__contains_routes_between_nodes_via_null_space(self):
278+ network1 = factory.make_ip4_or_6_network()
279+ network2 = factory.make_ip4_or_6_network(version=network1.version)
280+ node1, if1, sn1, sip1 = self.make_node_with_address(network1)
281+ node2, if2, sn2, sip2 = self.make_node_with_address(network2)
282+
283+ # Routes between all addresses are found, even back to themselves.
284+ left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
285+ right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
286+ row = lambda ent1, ent2, metric: (*ent1, *ent2, None, metric)
287+ expected = [
288+ row(left, left, 0),
289+ row(left, right, 4), # The NULL space, hence metric of 4.
290+ row(right, right, 0),
291+ row(right, left, 4), # The NULL space, hence metric of 4.
292 ]
293
294 with connection.cursor() as cursor:
295@@ -73,8 +142,8 @@
296 space2 = factory.make_Space()
297 network1 = factory.make_ip4_or_6_network()
298 network2 = factory.make_ip4_or_6_network(version=network1.version)
299- node1, if1, sn1, sip1 = self.make_node_with_address(space1, network1)
300- node2, if2, sn2, sip2 = self.make_node_with_address(space2, network2)
301+ node1, if1, sn1, sip1 = self.make_node_with_address(network1, space1)
302+ node2, if2, sn2, sip2 = self.make_node_with_address(network2, space2)
303
304 # Only routes from left to left and right to right are found: right is
305 # not routable from left, and left is not routable from right because
306@@ -82,8 +151,8 @@
307 left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
308 right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
309 expected = [
310- left + left + (space1.id,),
311- right + right + (space2.id,),
312+ (*left, *left, space1.id, 0),
313+ (*right, *right, space2.id, 0),
314 ]
315
316 with connection.cursor() as cursor:
317@@ -95,8 +164,8 @@
318 network1 = factory.make_ip4_or_6_network()
319 network2 = factory.make_ip4_or_6_network(
320 version=(4 if network1.version == 6 else 6))
321- node1, if1, sn1, sip1 = self.make_node_with_address(space, network1)
322- node2, if2, sn2, sip2 = self.make_node_with_address(space, network2)
323+ node1, if1, sn1, sip1 = self.make_node_with_address(network1, space)
324+ node2, if2, sn2, sip2 = self.make_node_with_address(network2, space)
325
326 # Only routes from left to left and right to right are found: right is
327 # not routable from left, and left is not routable from right because
328@@ -104,8 +173,8 @@
329 left = node1.id, if1.id, sn1.id, sn1.vlan.id, sip1.ip
330 right = node2.id, if2.id, sn2.id, sn2.vlan.id, sip2.ip
331 expected = [
332- left + left + (space.id,),
333- right + right + (space.id,),
334+ (*left, *left, space.id, 0),
335+ (*right, *right, space.id, 0),
336 ]
337
338 with connection.cursor() as cursor:
339
340=== modified file 'src/maasserver/tests/test_ntp.py'
341--- src/maasserver/tests/test_ntp.py 2017-01-28 00:51:47 +0000
342+++ src/maasserver/tests/test_ntp.py 2017-03-31 09:18:27 +0000
343@@ -5,8 +5,6 @@
344
345 __all__ = []
346
347-from operator import methodcaller
348-
349 from maasserver.models.config import Config
350 from maasserver.ntp import (
351 get_peers_for,
352@@ -14,10 +12,12 @@
353 )
354 from maasserver.testing.factory import factory
355 from maasserver.testing.testcase import MAASServerTestCase
356-from netaddr import IPAddress
357+from netaddr import (
358+ IPAddress,
359+ IPSet,
360+)
361 from testtools.matchers import (
362 AfterPreprocessing,
363- AllMatch,
364 ContainsAll,
365 Equals,
366 HasLength,
367@@ -231,7 +231,7 @@
368
369 For racks, machines, and devices, a selection process takes place to
370 determine which of several candidate addresses per server to choose. This
371- result is stable, i.e. it will choose the same address each time.
372+ result is semi-stable, i.e. it will always prefer "closer" addresses.
373 """
374
375 scenarios = (
376@@ -253,23 +253,31 @@
377 super(TestGetServersFor_Selection, self).setUp()
378 Config.objects.set_config("ntp_external_only", False)
379
380- def test_prefers_ipv6_to_ipv4_peers_then_highest_numerically(self):
381+ def test_prefers_closest_addresses(self):
382 subnet4 = factory.make_Subnet(version=4)
383- subnet6a = factory.make_Subnet(version=6)
384- subnet6b = factory.make_Subnet(version=6)
385- # Ensure that addresses in subnet6a < those in subnet6b.
386- subnet6a, subnet6b = sorted(
387- {subnet6a, subnet6b}, key=methodcaller("get_ipnetwork"))
388- # Create a node and server with an address in each subnet.
389- node, server = self.make_node(), self.make_server()
390- subnets = {subnet4, subnet6a, subnet6b}
391- populate_node_with_addresses(node, subnets)
392- populate_node_with_addresses(server, subnets)
393+ subnet6 = factory.make_Subnet(version=6)
394+ # Separate subnets but sharing the VLAN, hence routable.
395+ subnet4v = factory.make_Subnet(version=4, vlan=subnet4.vlan)
396+ subnet6v = factory.make_Subnet(version=6, vlan=subnet6.vlan)
397+
398+ # Create a node with an address in the first two subnets...
399+ node = self.make_node()
400+ populate_node_with_addresses(node, {subnet4, subnet6})
401+ # ... and a server with an address in every subnet.
402+ server = self.make_server()
403+ populate_node_with_addresses(
404+ server, {subnet4, subnet6, subnet4v, subnet6v})
405+
406+ # The NTP server addresses chosen will be those that are "closest" to
407+ # the node, and same-subnet wins in this over same-VLAN. No additional
408+ # preference is made between IPv4 or IPv6, hence we allow for either.
409+ preferred_subnets = subnet4, subnet6
410+ preferred_networks = IPSet(
411+ subnet.get_ipnetwork() for subnet in preferred_subnets)
412
413 servers = get_servers_for(node)
414 self.assertThat(servers, Not(HasLength(0)))
415- self.assertThat(servers, AllMatch(IsIPv6Address))
416- self.assertThat(subnet6b.get_ipnetwork(), ContainsAll(servers))
417+ self.assertThat(preferred_networks, ContainsAll(servers))
418
419
420 class TestGetPeersFor_Region_RegionRack(MAASServerTestCase):
421@@ -296,23 +304,37 @@
422 get_peers_for(node2),
423 IsSetOfServers({node1_address.ip}))
424
425- def test_prefers_ipv6_to_ipv4_peers_then_highest_numerically(self):
426+ def test_prefers_closest_addresses(self):
427 subnet4 = factory.make_Subnet(version=4)
428- subnet6a = factory.make_Subnet(version=6)
429- subnet6b = factory.make_Subnet(version=6)
430- # Ensure that addresses in subnet6a < those in subnet6b.
431- subnet6a, subnet6b = sorted(
432- {subnet6a, subnet6b}, key=methodcaller("get_ipnetwork"))
433- # Create some peers, each with an address in each subnet.
434- nodes = self.make_node(), self.make_node()
435- subnets = {subnet4, subnet6a, subnet6b}
436- for node in nodes:
437- populate_node_with_addresses(node, subnets)
438- for node in nodes:
439+ subnet6 = factory.make_Subnet(version=6)
440+ # Separate subnets but sharing the VLAN, hence routable.
441+ subnet4v1 = factory.make_Subnet(version=4, vlan=subnet4.vlan)
442+ subnet6v1 = factory.make_Subnet(version=6, vlan=subnet6.vlan)
443+ subnet4v2 = factory.make_Subnet(version=4, vlan=subnet4.vlan)
444+ subnet6v2 = factory.make_Subnet(version=6, vlan=subnet6.vlan)
445+
446+ # Create a node with an address in the first two subnets and the first
447+ # two same-VLAN subnets.
448+ node1 = self.make_node()
449+ populate_node_with_addresses(
450+ node1, {subnet4, subnet6, subnet4v1, subnet6v1})
451+ # Create a node with an address in the first two subnets and the
452+ # second two same-VLAN subnets.
453+ node2 = self.make_node()
454+ populate_node_with_addresses(
455+ node2, {subnet4, subnet6, subnet4v2, subnet6v2})
456+
457+ # The NTP server addresses chosen will be those that are "closest" to
458+ # the node, and same-subnet wins in this over same-VLAN. No additional
459+ # preference is made between IPv4 or IPv6, hence we allow for either.
460+ preferred_subnets = subnet4, subnet6
461+ preferred_networks = IPSet(
462+ subnet.get_ipnetwork() for subnet in preferred_subnets)
463+
464+ for node in (node1, node2):
465 peers = get_peers_for(node)
466 self.assertThat(peers, Not(HasLength(0)))
467- self.assertThat(peers, AllMatch(IsIPv6Address))
468- self.assertThat(subnet6b.get_ipnetwork(), ContainsAll(peers))
469+ self.assertThat(preferred_networks, ContainsAll(peers))
470
471
472 class TestGetPeersFor_Other(MAASServerTestCase):
473
474=== modified file 'src/maasserver/tests/test_routablepairs.py'
475--- src/maasserver/tests/test_routablepairs.py 2017-01-28 00:51:47 +0000
476+++ src/maasserver/tests/test_routablepairs.py 2017-03-31 09:18:27 +0000
477@@ -5,13 +5,21 @@
478
479 __all__ = []
480
481-from itertools import product
482+from itertools import (
483+ product,
484+ takewhile,
485+)
486+import random
487
488 from maasserver.models.node import Node
489 from maasserver.routablepairs import find_addresses_between_nodes
490 from maasserver.testing.factory import factory
491 from maasserver.testing.testcase import MAASServerTestCase
492 from testtools import ExpectedException
493+from testtools.matchers import (
494+ AfterPreprocessing,
495+ Equals,
496+)
497
498
499 class TestFindAddressesBetweenNodes(MAASServerTestCase):
500@@ -102,3 +110,124 @@
501 # No routable addresses are found.
502 self.assertItemsEqual(
503 expected, find_addresses_between_nodes([node1], [node2]))
504+
505+ def gen_disjoint_networks(self):
506+ """Generate disjoint networks.
507+
508+ Can be IPv4 or IPv6, but once generation has begun they'll all be the
509+ same family.
510+ """
511+ make_network = random.choice([
512+ factory.make_ipv4_network,
513+ factory.make_ipv6_network]
514+ )
515+ networks = []
516+ while True:
517+ network = make_network(disjoint_from=networks)
518+ networks.append(network)
519+ yield network
520+
521+ def test__yields_routes_with_lowest_metrics_first(self):
522+ space = factory.make_Space()
523+ # Ensure networks are disjoint but of the same family.
524+ networks = self.gen_disjoint_networks()
525+
526+ # Create the node for the "left" that has two IP addresses, one in the
527+ # null space, one in a non-null space.
528+ origin = factory.make_Node(hostname="origin")
529+ origin_iface = factory.make_Interface(
530+ node=origin, disconnected=True)
531+ origin_subnet = factory.make_Subnet(
532+ space=space, cidr=next(networks))
533+ origin_subnet_null_space = factory.make_Subnet(
534+ space=None, cidr=next(networks))
535+ origin_sip = factory.make_StaticIPAddress(
536+ interface=origin_iface, subnet=origin_subnet)
537+ origin_sip_null_space = factory.make_StaticIPAddress(
538+ interface=origin_iface, subnet=origin_subnet_null_space)
539+
540+ # Same subnet, different node.
541+ node_same_subnet = factory.make_Node(hostname="same-subnet")
542+ sip_same_subnet = factory.make_StaticIPAddress(
543+ interface=factory.make_Interface(
544+ node=node_same_subnet, disconnected=True),
545+ subnet=origin_subnet)
546+
547+ # Same VLAN, different subnet, different node.
548+ node_same_vlan = factory.make_Node(hostname="same-vlan")
549+ sip_same_vlan = factory.make_StaticIPAddress(
550+ interface=factory.make_Interface(
551+ node=node_same_vlan, disconnected=True),
552+ subnet=factory.make_Subnet(
553+ space=space, vlan=origin_subnet.vlan,
554+ cidr=next(networks)))
555+
556+ # Same space, different VLAN, subnet, and node.
557+ node_same_space = factory.make_Node(hostname="same-space")
558+ sip_same_space = factory.make_StaticIPAddress(
559+ interface=factory.make_Interface(
560+ node=node_same_space, disconnected=True),
561+ subnet=factory.make_Subnet(
562+ space=space, cidr=next(networks)))
563+
564+ # Null space, different VLAN, subnet, and node.
565+ node_null_space = factory.make_Node(hostname="null-space")
566+ sip_null_space = factory.make_StaticIPAddress(
567+ interface=factory.make_Interface(
568+ node=node_null_space, disconnected=True),
569+ subnet=factory.make_Subnet(
570+ space=None, cidr=next(networks)))
571+
572+ # We'll search for routes between `lefts` and `rights`.
573+ lefts = [
574+ origin,
575+ ]
576+ rights = [
577+ node_same_subnet,
578+ node_same_vlan,
579+ node_same_space,
580+ node_null_space,
581+ ]
582+
583+ # This is in order, lowest "metric" first.
584+ expected = [
585+ (origin, origin_sip.get_ipaddress(),
586+ node_same_subnet, sip_same_subnet.get_ipaddress()),
587+ (origin, origin_sip.get_ipaddress(),
588+ node_same_vlan, sip_same_vlan.get_ipaddress()),
589+ (origin, origin_sip.get_ipaddress(),
590+ node_same_space, sip_same_space.get_ipaddress()),
591+ (origin, origin_sip_null_space.get_ipaddress(),
592+ node_null_space, sip_null_space.get_ipaddress()),
593+ ]
594+ self.assertThat(
595+ find_addresses_between_nodes(lefts, rights),
596+ AfterPreprocessing(list, Equals(expected)))
597+
598+ # Same node, same space, different VLAN and subnet. We did not add
599+ # this earlier because its existence allows for a large number of
600+ # additional routes between the origin and the other nodes, which
601+ # would have obscured the test.
602+ origin_sip_2 = factory.make_StaticIPAddress(
603+ interface=factory.make_Interface(
604+ node=origin, disconnected=True),
605+ subnet=factory.make_Subnet(
606+ space=space, cidr=next(networks)))
607+
608+ # Now the first addresses returned are between those addresses we
609+ # created on the same node, in no particular order.
610+ origin_ips = origin_sip.get_ipaddress(), origin_sip_2.get_ipaddress()
611+ expected_mutual = {
612+ (origin, ip1, origin, ip2)
613+ for ip1, ip2 in product(origin_ips, origin_ips)
614+ }
615+ # There's a mutual route for the null-space IP address too.
616+ expected_mutual.add(
617+ (origin, origin_sip_null_space.get_ipaddress(),
618+ origin, origin_sip_null_space.get_ipaddress()))
619+ observed_mutual = takewhile(
620+ (lambda route: route[0] == route[2]), # Route is mutual.
621+ find_addresses_between_nodes(lefts, [origin, *rights]),
622+ )
623+ self.assertItemsEqual(
624+ expected_mutual, observed_mutual)
625
626=== modified file 'src/maastesting/matchers.py'
627--- src/maastesting/matchers.py 2016-12-07 12:46:14 +0000
628+++ src/maastesting/matchers.py 2017-03-31 09:18:27 +0000
629@@ -4,6 +4,7 @@
630 """testtools custom matchers"""
631
632 __all__ = [
633+ 'ContainedBy',
634 'DocTestMatches',
635 'FileContains',
636 'GreaterThanOrEqual',
637@@ -481,3 +482,20 @@
638 (lambda observed: not observed.isspace()),
639 "%r is whitespace"),
640 first_only=True)
641+
642+
643+class ContainedBy(Matcher):
644+ """Test if the matchee is in the given container."""
645+
646+ def __init__(self, haystack):
647+ super(ContainedBy, self).__init__()
648+ self.haystack = haystack
649+
650+ def __str__(self):
651+ return "%s(%r)" % (
652+ self.__class__.__name__, self.haystack)
653+
654+ def match(self, needle):
655+ if needle not in self.haystack:
656+ return Mismatch(
657+ "%r not in %r" % (needle, self.haystack))
658
659=== modified file 'src/maastesting/tests/test_matchers.py'
660--- src/maastesting/tests/test_matchers.py 2016-12-07 12:46:14 +0000
661+++ src/maastesting/tests/test_matchers.py 2017-03-31 09:18:27 +0000
662@@ -18,6 +18,7 @@
663 from maastesting import matchers
664 from maastesting.factory import factory
665 from maastesting.matchers import (
666+ ContainedBy,
667 FileContains,
668 GreaterThanOrEqual,
669 HasAttribute,
670@@ -588,3 +589,13 @@
671 def test_does_not_match_non_strings(self):
672 self.assertMismatch(
673 IsNonEmptyString.match(1234), "1234 is not a string")
674+
675+
676+class TestContainedBy(MAASTestCase, MockTestMixin):
677+ """Tests for the `ContainedBy` matcher."""
678+
679+ def test_matches_needle_in_haystack(self):
680+ self.assertThat("foo", ContainedBy({"foo", "bar"}))
681+
682+ def test_does_not_match_needle_not_in_haystack(self):
683+ self.assertMismatch(ContainedBy([]).match("foo"), "'foo' not in []")