Merge lp:~allenap/maas/edit-disconnected-cluster--bug-1439476 into lp:~maas-committers/maas/trunk
- edit-disconnected-cluster--bug-1439476
- Merge into trunk
Proposed by
Gavin Panella
Status: | Merged |
---|---|
Approved by: | Gavin Panella |
Approved revision: | no longer in the source branch. |
Merged at revision: | 4479 |
Proposed branch: | lp:~allenap/maas/edit-disconnected-cluster--bug-1439476 |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
475 lines (+204/-39) 4 files modified
src/maasserver/dhcp.py (+31/-12) src/maasserver/rpc/regionservice.py (+35/-3) src/maasserver/rpc/tests/test_regionservice.py (+76/-0) src/maasserver/tests/test_dhcp.py (+62/-24) |
To merge this branch: | bzr merge lp:~allenap/maas/edit-disconnected-cluster--bug-1439476 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Blake Rouse (community) | Approve | ||
Review via email: mp+276996@code.launchpad.net |
Commit message
Ignore disconnected clusters when configuring DHCP.
However, configure DHCP when the cluster does reconnect to the region.
Description of the change
To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote : | # |
Thanks for the review!
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'src/maasserver/dhcp.py' |
2 | --- src/maasserver/dhcp.py 2015-09-24 16:22:12 +0000 |
3 | +++ src/maasserver/dhcp.py 2015-11-09 12:39:07 +0000 |
4 | @@ -18,6 +18,7 @@ |
5 | ] |
6 | |
7 | from collections import defaultdict |
8 | +import logging |
9 | import threading |
10 | |
11 | from django.conf import settings |
12 | @@ -35,12 +36,16 @@ |
13 | ConfigureDHCPv4, |
14 | ConfigureDHCPv6, |
15 | ) |
16 | +from provisioningserver.rpc.exceptions import NoConnectionsAvailable |
17 | from provisioningserver.utils.twisted import ( |
18 | callOut, |
19 | synchronous, |
20 | ) |
21 | |
22 | |
23 | +logger = logging.getLogger(__name__) |
24 | + |
25 | + |
26 | def split_ipv4_ipv6_interfaces(interfaces): |
27 | """Divide `interfaces` into IPv4 ones and IPv6 ones. |
28 | |
29 | @@ -80,13 +85,14 @@ |
30 | |
31 | |
32 | @synchronous |
33 | -def do_configure_dhcp(ip_version, nodegroup, interfaces, ntp_server): |
34 | +def do_configure_dhcp(ip_version, nodegroup, interfaces, ntp_server, client): |
35 | """Write DHCP configuration and restart the DHCP server. |
36 | |
37 | Delegates the work to the cluster controller, and waits for it |
38 | to complete. |
39 | |
40 | :param ip_version: The IP version to configure for, either 4 or 6. |
41 | + :param client: An RPC client for the given cluster. |
42 | |
43 | :raise NoConnectionsAvailable: if the region controller could not get |
44 | an RPC connection to the cluster controller. |
45 | @@ -122,30 +128,33 @@ |
46 | make_subnet_config(interface, dns_servers, ntp_server) |
47 | for interface in interfaces |
48 | ] |
49 | - client = getClientFor(nodegroup.uuid) |
50 | # XXX jtv 2014-08-26 bug=1361673: If this fails remotely, the error |
51 | # needs to be reported gracefully to the caller. |
52 | client(command, omapi_key=omapi_key, subnet_configs=subnets).wait(60) |
53 | |
54 | |
55 | -def configure_dhcpv4(nodegroup, interfaces, ntp_server): |
56 | +def configure_dhcpv4(nodegroup, interfaces, ntp_server, client): |
57 | """Call `do_configure_dhcp` for IPv4. |
58 | |
59 | This serves mainly as a convenience for testing. |
60 | """ |
61 | - return do_configure_dhcp(4, nodegroup, interfaces, ntp_server) |
62 | - |
63 | - |
64 | -def configure_dhcpv6(nodegroup, interfaces, ntp_server): |
65 | + return do_configure_dhcp(4, nodegroup, interfaces, ntp_server, client) |
66 | + |
67 | + |
68 | +def configure_dhcpv6(nodegroup, interfaces, ntp_server, client): |
69 | """Call `do_configure_dhcp` for IPv6. |
70 | |
71 | This serves mainly as a convenience for testing. |
72 | """ |
73 | - return do_configure_dhcp(6, nodegroup, interfaces, ntp_server) |
74 | + return do_configure_dhcp(6, nodegroup, interfaces, ntp_server, client) |
75 | |
76 | |
77 | def configure_dhcp_now(nodegroup): |
78 | - """Write the DHCP configuration files and restart the DHCP servers.""" |
79 | + """Write the DHCP configuration files and restart the DHCP servers. |
80 | + |
81 | + :raises: :py:class:`~.exceptions.NoConnectionsAvailable` when there |
82 | + are no open connections to the specified cluster controller. |
83 | + """ |
84 | # Let's get this out of the way first up shall we? |
85 | if not settings.DHCP_CONNECT: |
86 | # For the uninitiated, DHCP_CONNECT is set, by default, to False |
87 | @@ -153,6 +162,10 @@ |
88 | # calls to async tasks. |
89 | return |
90 | |
91 | + # Get the client early; it's a cheap operation that may raise an |
92 | + # exception, meaning we can avoid some work if it fails. |
93 | + client = getClientFor(nodegroup.uuid) |
94 | + |
95 | if nodegroup.status == NODEGROUP_STATUS.ENABLED: |
96 | # Cluster is an accepted one. Control DHCP for its managed interfaces. |
97 | interfaces = nodegroup.get_managed_interfaces() |
98 | @@ -168,8 +181,8 @@ |
99 | |
100 | ipv4_interfaces, ipv6_interfaces = split_ipv4_ipv6_interfaces(interfaces) |
101 | |
102 | - configure_dhcpv4(nodegroup, ipv4_interfaces, ntp_server) |
103 | - configure_dhcpv6(nodegroup, ipv6_interfaces, ntp_server) |
104 | + configure_dhcpv4(nodegroup, ipv4_interfaces, ntp_server, client) |
105 | + configure_dhcpv6(nodegroup, ipv6_interfaces, ntp_server, client) |
106 | |
107 | |
108 | class Changes: |
109 | @@ -206,7 +219,13 @@ |
110 | """Apply all requested changes.""" |
111 | clusters = {cluster.id: cluster for cluster in self.clusters} |
112 | for cluster in clusters.viewvalues(): |
113 | - configure_dhcp_now(cluster) |
114 | + try: |
115 | + configure_dhcp_now(cluster) |
116 | + except NoConnectionsAvailable: |
117 | + logger.info( |
118 | + "Cluster %s (%s) is not connected at present so cannot " |
119 | + "be configured; it will catch up when it next connects.", |
120 | + cluster.cluster_name, cluster.uuid) |
121 | |
122 | |
123 | class ChangeConsolidator(threading.local): |
124 | |
125 | === modified file 'src/maasserver/rpc/regionservice.py' |
126 | --- src/maasserver/rpc/regionservice.py 2015-10-20 21:25:57 +0000 |
127 | +++ src/maasserver/rpc/regionservice.py 2015-11-09 12:39:07 +0000 |
128 | @@ -34,6 +34,8 @@ |
129 | locks, |
130 | ) |
131 | from maasserver.bootresources import get_simplestream_endpoint |
132 | +from maasserver.dhcp import configure_dhcp_now |
133 | +from maasserver.models.nodegroup import NodeGroup |
134 | from maasserver.rpc import ( |
135 | clusters, |
136 | configuration, |
137 | @@ -383,6 +385,31 @@ |
138 | return d |
139 | |
140 | |
141 | +@transactional |
142 | +def configureCluster(ident): |
143 | + """Configure the cluster. |
144 | + |
145 | + Typically this is called once, as soon as a successful handshake is made |
146 | + between the region and cluster. After that configuration can be done at |
147 | + the moment the change is made. In essence, this is "catches-up" the |
148 | + cluster with the region's current idea of how it should be. |
149 | + |
150 | + Don't do anything particularly energetic in here because it will be called |
151 | + once for each region<-->cluster connection, of which each cluster can have |
152 | + many. |
153 | + |
154 | + :param ident: The cluster's UUID. |
155 | + """ |
156 | + try: |
157 | + cluster = NodeGroup.objects.get(uuid=ident) |
158 | + except NodeGroup.DoesNotExist: |
159 | + log.msg("Cluster '%s' is not recognised; cannot configure." % (ident,)) |
160 | + else: |
161 | + configure_dhcp_now(cluster) |
162 | + log.msg("Cluster %s (%s) has been configured." % ( |
163 | + cluster.cluster_name, cluster.uuid)) |
164 | + |
165 | + |
166 | @implementer(IConnection) |
167 | class RegionServer(Region): |
168 | """The RPC protocol supported by a region controller, server version. |
169 | @@ -428,13 +455,18 @@ |
170 | "Cluster '%s' FAILED authentication; " |
171 | "dropping connection." % self.ident) |
172 | yield self.transport.loseConnection() |
173 | + returnValue(authenticated) |
174 | |
175 | - def handshakeSucceeded(self, result): |
176 | + def handshakeSucceeded(self, authenticated): |
177 | """The handshake (identify and authenticate) succeeded. |
178 | |
179 | - This does *NOT* mean that the cluster was successfully authenticated, |
180 | - merely that the process of authentication did not encounter an error. |
181 | + :param authenticated: True if the remote cluster has successfully |
182 | + authenticated. |
183 | + :type authenticated: bool |
184 | """ |
185 | + if authenticated: |
186 | + return deferToDatabase(configureCluster, self.ident).addErrback( |
187 | + log.err, "Failed to configure DHCP on %s." % (self.ident,)) |
188 | |
189 | def handshakeFailed(self, failure): |
190 | """The handshake (identify and authenticate) failed.""" |
191 | |
192 | === modified file 'src/maasserver/rpc/tests/test_regionservice.py' |
193 | --- src/maasserver/rpc/tests/test_regionservice.py 2015-10-21 00:21:05 +0000 |
194 | +++ src/maasserver/rpc/tests/test_regionservice.py 2015-11-09 12:39:07 +0000 |
195 | @@ -1104,6 +1104,36 @@ |
196 | "'%s'.", name, event_description, mac_address)) |
197 | |
198 | |
199 | +class TestConfigureCluster(MAASServerTestCase): |
200 | + """Tests for the `configureCluster` function.""" |
201 | + |
202 | + def test__logs_when_cluster_is_not_known(self): |
203 | + ident = factory.make_UUID() |
204 | + with TwistedLoggerFixture() as logger: |
205 | + regionservice.configureCluster(ident) |
206 | + self.assertDocTestMatches( |
207 | + "Cluster '...' is not recognised; cannot configure.", |
208 | + logger.output) |
209 | + |
210 | + def test__logs_when_cluster_has_been_configured(self): |
211 | + ident = factory.make_NodeGroup().uuid |
212 | + with TwistedLoggerFixture() as logger: |
213 | + regionservice.configureCluster(ident) |
214 | + self.assertDocTestMatches( |
215 | + "Cluster ... (...) has been configured.", |
216 | + logger.output) |
217 | + |
218 | + def test__configures_dhcp(self): |
219 | + ident = factory.make_NodeGroup().uuid |
220 | + self.patch_autospec(regionservice, "configure_dhcp_now") |
221 | + regionservice.configureCluster(ident) |
222 | + self.assertThat( |
223 | + regionservice.configure_dhcp_now, |
224 | + MockCalledOnceWith(ANY)) |
225 | + [cluster] = regionservice.configure_dhcp_now.call_args[0] |
226 | + self.assertThat(cluster.uuid, Equals(ident)) |
227 | + |
228 | + |
229 | class TestRegionServer(MAASServerTestCase): |
230 | |
231 | def test_interfaces(self): |
232 | @@ -1246,6 +1276,52 @@ |
233 | # Nothing was logged. |
234 | self.assertEqual("", logger.output) |
235 | |
236 | + def make_handshaking_server(self): |
237 | + service = RegionService() |
238 | + service.running = True # Pretend it's running. |
239 | + service.factory.protocol = HandshakingRegionServer |
240 | + return service.factory.buildProtocol(addr=None) # addr is unused. |
241 | + |
242 | + def test_connectionMade_configures_cluster_if_auth_succeeds(self): |
243 | + self.patch_autospec(regionservice, "configureCluster") |
244 | + protocol = self.make_handshaking_server() |
245 | + |
246 | + connectionMade = wait_for_reactor(protocol.connectionMade) |
247 | + connectionMade() |
248 | + |
249 | + self.assertThat( |
250 | + regionservice.configureCluster, |
251 | + MockCalledOnceWith(protocol.ident)) |
252 | + |
253 | + def test_connectionMade_does_not_configure_cluster_if_auth_fails(self): |
254 | + self.patch_autospec(regionservice, "configureCluster") |
255 | + protocol = self.make_handshaking_server() |
256 | + self.patch_authenticate_for_failure(protocol) |
257 | + self.patch(protocol, "transport") |
258 | + |
259 | + connectionMade = wait_for_reactor(protocol.connectionMade) |
260 | + connectionMade() |
261 | + |
262 | + self.assertThat(regionservice.configureCluster, MockNotCalled()) |
263 | + |
264 | + def test_connectionMade_logs_cluster_configuration_failure(self): |
265 | + self.patch_autospec(regionservice, "configureCluster") |
266 | + regionservice.configureCluster.side_effect = ZeroDivisionError |
267 | + protocol = self.make_handshaking_server() |
268 | + |
269 | + with TwistedLoggerFixture() as logger: |
270 | + connectionMade = wait_for_reactor(protocol.connectionMade) |
271 | + connectionMade() |
272 | + |
273 | + self.assertDocTestMatches( |
274 | + """\ |
275 | + ... |
276 | + --- |
277 | + Failed to configure DHCP on ... |
278 | + Traceback (most recent call last): ... |
279 | + """, |
280 | + logger.output) |
281 | + |
282 | def make_running_server(self): |
283 | service = RegionService() |
284 | service.running = True # Pretend it's running. |
285 | |
286 | === modified file 'src/maasserver/tests/test_dhcp.py' |
287 | --- src/maasserver/tests/test_dhcp.py 2015-09-24 16:22:12 +0000 |
288 | +++ src/maasserver/tests/test_dhcp.py 2015-11-09 12:39:07 +0000 |
289 | @@ -18,6 +18,7 @@ |
290 | |
291 | from django.conf import settings |
292 | from django.db import transaction |
293 | +from fixtures import LoggerFixture |
294 | from maasserver import dhcp |
295 | from maasserver.dhcp import ( |
296 | configure_dhcp, |
297 | @@ -34,6 +35,7 @@ |
298 | Config, |
299 | NodeGroup, |
300 | ) |
301 | +from maasserver.rpc import getClientFor |
302 | from maasserver.rpc.testing.fixtures import MockLiveRegionToClusterRPCFixture |
303 | from maasserver.testing.eventloop import ( |
304 | RegionEventLoopFixture, |
305 | @@ -270,7 +272,9 @@ |
306 | |
307 | # Although the above nodegroup has managed interfaces, we pass the |
308 | # empty list here; do_configure_dhcp() dutifully believes us. |
309 | - do_configure_dhcp(self.ip_version, nodegroup, [], ntp_server) |
310 | + do_configure_dhcp( |
311 | + self.ip_version, nodegroup, [], ntp_server, |
312 | + getClientFor(nodegroup.uuid)) |
313 | |
314 | self.assertThat( |
315 | command_stub, MockCalledOnceWith( |
316 | @@ -297,7 +301,9 @@ |
317 | protocol, command_stub = self.prepare_rpc(nodegroup) |
318 | command_stub.side_effect = always_succeed_with({}) |
319 | |
320 | - do_configure_dhcp(self.ip_version, nodegroup, interfaces, ntp_server) |
321 | + do_configure_dhcp( |
322 | + self.ip_version, nodegroup, interfaces, ntp_server, |
323 | + getClientFor(nodegroup.uuid)) |
324 | |
325 | expected_subnet_configs = [ |
326 | make_subnet_config(interface, dns_server, ntp_server) |
327 | @@ -317,16 +323,20 @@ |
328 | def test_configure_dhcpv4_calls_do_configure_dhcp(self): |
329 | do_configure_dhcp = self.patch_autospec(dhcp, "do_configure_dhcp") |
330 | dhcp.configure_dhcpv4( |
331 | - sentinel.nodegroup, sentinel.interfaces, sentinel.ntp_server) |
332 | + sentinel.nodegroup, sentinel.interfaces, sentinel.ntp_server, |
333 | + sentinel.client) |
334 | self.assertThat(do_configure_dhcp, MockCalledOnceWith( |
335 | - 4, sentinel.nodegroup, sentinel.interfaces, sentinel.ntp_server)) |
336 | + 4, sentinel.nodegroup, sentinel.interfaces, sentinel.ntp_server, |
337 | + sentinel.client)) |
338 | |
339 | def test_configure_dhcpv6_calls_do_configure_dhcp(self): |
340 | do_configure_dhcp = self.patch_autospec(dhcp, "do_configure_dhcp") |
341 | dhcp.configure_dhcpv6( |
342 | - sentinel.nodegroup, sentinel.interfaces, sentinel.ntp_server) |
343 | + sentinel.nodegroup, sentinel.interfaces, sentinel.ntp_server, |
344 | + sentinel.client) |
345 | self.assertThat(do_configure_dhcp, MockCalledOnceWith( |
346 | - 6, sentinel.nodegroup, sentinel.interfaces, sentinel.ntp_server)) |
347 | + 6, sentinel.nodegroup, sentinel.interfaces, sentinel.ntp_server, |
348 | + sentinel.client)) |
349 | |
350 | |
351 | def patch_configure_funcs(test): |
352 | @@ -383,6 +393,11 @@ |
353 | class TestConfigureDHCP(MAASServerTestCase): |
354 | """Tests for `configure_dhcp`.""" |
355 | |
356 | + def setUp(self): |
357 | + super(TestConfigureDHCP, self).setUp() |
358 | + # Suppress checks for cluster availability. |
359 | + self.patch_autospec(dhcp, "getClientFor") |
360 | + |
361 | def test__obeys_DHCP_CONNECT(self): |
362 | configure_dhcpv4, configure_dhcpv6 = patch_configure_funcs(self) |
363 | cluster = make_cluster(self) |
364 | @@ -406,11 +421,12 @@ |
365 | with post_commit_hooks: |
366 | configure_dhcp(cluster) |
367 | |
368 | - self.expectThat(configure_dhcpv4, MockCalledOnceWith(cluster, [], ANY)) |
369 | - self.expectThat(configure_dhcpv6, MockCalledOnceWith(cluster, [], ANY)) |
370 | + self.expectThat(configure_dhcpv4, MockCalledOnceWith( |
371 | + cluster, [], ANY, dhcp.getClientFor.return_value)) |
372 | + self.expectThat(configure_dhcpv6, MockCalledOnceWith( |
373 | + cluster, [], ANY, dhcp.getClientFor.return_value)) |
374 | |
375 | def test__configures_dhcpv4(self): |
376 | - getClientFor = self.patch_autospec(dhcp, "getClientFor") |
377 | ip = factory.make_ipv4_address() |
378 | cluster = make_cluster(self, maas_url='http://%s/' % ip) |
379 | make_ipv4_interface(self, cluster) |
380 | @@ -419,9 +435,8 @@ |
381 | with post_commit_hooks: |
382 | configure_dhcp(cluster) |
383 | |
384 | - self.assertThat(getClientFor, MockCallsMatch( |
385 | - call(cluster.uuid), call(cluster.uuid))) |
386 | - client = getClientFor.return_value |
387 | + self.assertThat(dhcp.getClientFor, MockCalledOnceWith(cluster.uuid)) |
388 | + client = dhcp.getClientFor.return_value |
389 | self.assertThat(client, MockCallsMatch( |
390 | call(ANY, omapi_key=ANY, subnet_configs=ANY), |
391 | call(ANY, omapi_key=ANY, subnet_configs=ANY), |
392 | @@ -447,11 +462,30 @@ |
393 | |
394 | ntp_server = Config.objects.get_config('ntp_server') |
395 | self.assertThat( |
396 | - configure_dhcpv4, |
397 | - MockCalledOnceWith(ANY, ANY, ntp_server)) |
398 | + configure_dhcpv4, MockCalledOnceWith( |
399 | + ANY, ANY, ntp_server, dhcp.getClientFor.return_value)) |
400 | self.assertThat( |
401 | - configure_dhcpv6, |
402 | - MockCalledOnceWith(ANY, ANY, ntp_server)) |
403 | + configure_dhcpv6, MockCalledOnceWith( |
404 | + ANY, ANY, ntp_server, dhcp.getClientFor.return_value)) |
405 | + |
406 | + |
407 | +class TestConfigureDHCPWithDisconnectedCluster(MAASServerTestCase): |
408 | + """Behaviour when the target cluster is not connected.""" |
409 | + |
410 | + def test__logs_about_disconnected_cluster(self): |
411 | + cluster = make_cluster(self) |
412 | + self.patch(settings, "DHCP_CONNECT", True) |
413 | + |
414 | + with LoggerFixture(dhcp.__name__) as logger: |
415 | + with post_commit_hooks: |
416 | + configure_dhcp(cluster) |
417 | + |
418 | + self.assertDocTestMatches( |
419 | + """\ |
420 | + Cluster ... (...) is not connected at present so cannot be |
421 | + configured; it will catch up when it next connects. |
422 | + """, |
423 | + logger.output) |
424 | |
425 | |
426 | class TestConfigureDHCPTransactional(MAASTransactionServerTestCase): |
427 | @@ -461,6 +495,14 @@ |
428 | must be committed to the database in order that they're visible elsewhere. |
429 | """ |
430 | |
431 | + def setUp(self): |
432 | + super(TestConfigureDHCPTransactional, self).setUp() |
433 | + # Suppress checks for cluster availability. |
434 | + getClientFor = self.patch_autospec(dhcp, "getClientFor") |
435 | + getClientFor.return_value = sentinel.client |
436 | + # Connect DHCP changes. |
437 | + self.patch(settings, "DHCP_CONNECT", True) |
438 | + |
439 | def test__passes_only_IPv4_interfaces_to_DHCPv4(self): |
440 | configure_dhcpv4, _ = patch_configure_funcs(self) |
441 | |
442 | @@ -468,14 +510,12 @@ |
443 | cluster = make_cluster(self) |
444 | ipv4_interface = make_ipv4_interface(self, cluster) |
445 | make_ipv6_interface(self, cluster) |
446 | - self.patch(settings, "DHCP_CONNECT", True) |
447 | |
448 | with post_commit_hooks: |
449 | configure_dhcp(cluster) |
450 | |
451 | - self.assertThat( |
452 | - configure_dhcpv4, |
453 | - MockCalledOnceWith(cluster, [ipv4_interface], ANY)) |
454 | + self.assertThat(configure_dhcpv4, MockCalledOnceWith( |
455 | + cluster, [ipv4_interface], ANY, sentinel.client)) |
456 | |
457 | def test__passes_only_IPv6_interfaces_to_DHCPv6(self): |
458 | _, configure_dhcpv6 = patch_configure_funcs(self) |
459 | @@ -484,14 +524,12 @@ |
460 | cluster = make_cluster(self) |
461 | ipv6_interface = make_ipv6_interface(self, cluster) |
462 | make_ipv4_interface(self, cluster) |
463 | - self.patch(settings, "DHCP_CONNECT", True) |
464 | |
465 | with post_commit_hooks: |
466 | configure_dhcp(cluster) |
467 | |
468 | - self.assertThat( |
469 | - configure_dhcpv6, |
470 | - MockCalledOnceWith(cluster, [ipv6_interface], ANY)) |
471 | + self.assertThat(configure_dhcpv6, MockCalledOnceWith( |
472 | + cluster, [ipv6_interface], ANY, sentinel.client)) |
473 | |
474 | |
475 | class TestDHCPConnect(MAASServerTestCase): |
Code looks good and the implementation is okay. It solves the bug for 1.9 but I think in 2.0 we will need to do this better as HA will change this logic, I believe.