Merge lp:~allenap/maas/ntp-prefer-nearby-servers--bug-1675095 into lp:~maas-committers/maas/trunk
- ntp-prefer-nearby-servers--bug-1675095
- Merge into trunk
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 |
Related bugs: |
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.
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.
Andres Rodriguez (andreserl) wrote : | # |
I already did run it through the CI :). I'm gonna go ahead and land.
MAAS Lander (maas-lander) wrote : | # |
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://
Get:2 http://
Get:3 http://
Get:4 http://
Fetched 306 kB in 0s (667 kB/s)
Reading package lists...
sudo DEBIAN_
--no-
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~
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubun
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+
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
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 []") |
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.