Merge lp:~blake-rouse/maas/external-dhcp-probe into lp:~maas-committers/maas/trunk
- external-dhcp-probe
- Merge into trunk
Proposed by
Blake Rouse
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Blake Rouse | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 4884 | ||||
Proposed branch: | lp:~blake-rouse/maas/external-dhcp-probe | ||||
Merge into: | lp:~maas-committers/maas/trunk | ||||
Diff against target: |
1353 lines (+240/-688) 12 files modified
Makefile (+1/-2) buildout.cfg (+0/-2) src/maasserver/rpc/rackcontrollers.py (+28/-47) src/maasserver/rpc/regionservice.py (+5/-18) src/maasserver/rpc/tests/test_rackcontrollers.py (+54/-1) src/maasserver/rpc/tests/test_regionservice.py (+36/-90) src/provisioningserver/dhcp/detect.py (+4/-127) src/provisioningserver/dhcp/probe.py (+0/-41) src/provisioningserver/dhcp/tests/test_detect.py (+4/-213) src/provisioningserver/pserv_services/dhcp_probe_service.py (+36/-47) src/provisioningserver/pserv_services/tests/test_dhcp_probe_service.py (+68/-77) src/provisioningserver/rpc/region.py (+4/-23) |
||||
To merge this branch: | bzr merge lp:~blake-rouse/maas/external-dhcp-probe | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+291145@code.launchpad.net |
Commit message
Update the external_dhcp field on the VLAN for discovered DHCP servers. Remove code that is no longer be used for DHCP probe.
Description of the change
To post a comment you must log in.
Revision history for this message
Blake Rouse (blake-rouse) wrote : | # |
Thanks for the review. I fixed you inline comment.
Believe it or not the bulk of this code could have been removed back in 1.7. When MAAS was switched from celery to RPC most of this code stopped being used. I have tested this on a installed system and MAAS has permission. Really I didn't change any of the probing code, I just removed what is no longer used.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'Makefile' |
2 | --- Makefile 2016-03-28 13:54:47 +0000 |
3 | +++ Makefile 2016-04-07 13:22:34 +0000 |
4 | @@ -54,7 +54,6 @@ |
5 | bin/buildout \ |
6 | bin/database \ |
7 | bin/maas \ |
8 | - bin/maas-probe-dhcp \ |
9 | bin/maas-rack \ |
10 | bin/maas-region \ |
11 | bin/twistd.rack \ |
12 | @@ -135,7 +134,7 @@ |
13 | $(buildout) install testing-test |
14 | @touch --no-create $@ |
15 | |
16 | -bin/maas-probe-dhcp bin/maas-rack bin/twistd.rack: \ |
17 | +bin/maas-rack bin/twistd.rack: \ |
18 | bin/buildout buildout.cfg versions.cfg setup.py |
19 | $(buildout) install rack |
20 | @touch --no-create $@ |
21 | |
22 | === modified file 'buildout.cfg' |
23 | --- buildout.cfg 2016-03-28 13:54:47 +0000 |
24 | +++ buildout.cfg 2016-04-07 13:22:34 +0000 |
25 | @@ -189,13 +189,11 @@ |
26 | recipe = zc.recipe.egg |
27 | eggs = |
28 | entry-points = |
29 | - maas-probe-dhcp=provisioningserver.dhcp.probe:main |
30 | maas-rack=provisioningserver.__main__:main |
31 | twistd.rack=twisted.scripts.twistd:run |
32 | extra-paths = |
33 | ${common:extra-paths} |
34 | scripts = |
35 | - maas-probe-dhcp |
36 | maas-rack |
37 | twistd.rack |
38 | initialization = |
39 | |
40 | === modified file 'src/maasserver/rpc/rackcontrollers.py' |
41 | --- src/maasserver/rpc/rackcontrollers.py 2016-03-28 13:54:47 +0000 |
42 | +++ src/maasserver/rpc/rackcontrollers.py 2016-04-07 13:22:34 +0000 |
43 | @@ -16,10 +16,10 @@ |
44 | from maasserver import worker_user |
45 | from maasserver.enum import NODE_TYPE |
46 | from maasserver.models import ( |
47 | - Interface, |
48 | Node, |
49 | NodeGroupToRackController, |
50 | RackController, |
51 | + StaticIPAddress, |
52 | ) |
53 | from maasserver.models.node import typecast_node |
54 | from maasserver.utils.orm import transactional |
55 | @@ -163,52 +163,33 @@ |
56 | |
57 | |
58 | @transactional |
59 | -def update_foreign_dhcp_ip(cluster_uuid, interface_name, foreign_dhcp_ip): |
60 | - """Update the foreign_dhcp_ip field of a given interface on a cluster. |
61 | - |
62 | - Note: We do this through an update, not a read/modify/write. |
63 | - Updating NodeGroupInterface client-side may inadvertently trigger |
64 | - Django signals that cause a rewrite of the DHCP config, plus restart |
65 | - of the DHCP server. The inadvertent triggering has been known to |
66 | - happen because of race conditions between read/modify/write |
67 | - transactions that were enabled by Django defaulting to, and being |
68 | - designed for, the READ COMMITTED isolation level; the ORM writing |
69 | - back even unmodified fields; and GenericIPAddressField's default |
70 | - value being prone to problems where NULL is sometimes represented as |
71 | - None, sometimes as an empty string, and the difference being enough |
72 | - to convince the signal machinery that these fields have changed when |
73 | - in fact they have not. |
74 | - |
75 | - :param cluster_uuid: Cluster's UUID. |
76 | - :param interface_name: The name of the cluster interface on which the |
77 | - foreign DHCP server was (or wasn't) discovered. |
78 | - :param foreign_dhcp_ip: IP address of foreign DCHP server, if any. |
79 | - """ |
80 | - # XXX 2016-01-20 blake_r - Currently no where to place this information. |
81 | - # Need to add to the model to store this information. |
82 | - pass |
83 | - |
84 | - |
85 | -@transactional |
86 | -def get_rack_controllers_interfaces_as_dicts(system_id): |
87 | - """Return all the interfaces on a given rack controller as a list of dicts. |
88 | - |
89 | - :return: A list of dicts in the form {'name': interface.name, |
90 | - 'interface': interface.interface, 'ip': interface.ip}, one dict per |
91 | - interface on the cluster. |
92 | - """ |
93 | - interfaces = Interface.objects.filter(node__system_id=system_id) |
94 | - # XXX 2016-01-20 blake_r - Currently not passing any IP address as it now |
95 | - # should take a list of IP addresses and not just one IP address. To make |
96 | - # it work for now nothing its filtered out. |
97 | - return [ |
98 | - { |
99 | - 'name': interface.name, |
100 | - 'interface': interface.name, |
101 | - 'ip': '', |
102 | - } |
103 | - for interface in interfaces |
104 | - ] |
105 | +def update_foreign_dhcp(system_id, interface_name, dhcp_ip=None): |
106 | + """Update the external_dhcp field of the VLAN for the interface. |
107 | + |
108 | + :param system_id: Rack controller system_id. |
109 | + :param interface_name: The name of the interface. |
110 | + :param dhcp_ip: The IP address of the responding DHCP server. |
111 | + """ |
112 | + rack_controller = RackController.objects.get(system_id=system_id) |
113 | + interface = rack_controller.interface_set.filter( |
114 | + name=interface_name).select_related("vlan").first() |
115 | + if interface is not None: |
116 | + if dhcp_ip is not None: |
117 | + sip = StaticIPAddress.objects.filter(ip=dhcp_ip).first() |
118 | + if sip is not None: |
119 | + # Check that its not an IP address of a rack controller |
120 | + # providing that DHCP service. |
121 | + rack_interfaces_serving_dhcp = sip.interface_set.filter( |
122 | + node__node_type__in=[ |
123 | + NODE_TYPE.RACK_CONTROLLER, |
124 | + NODE_TYPE.REGION_AND_RACK_CONTROLLER], |
125 | + vlan__dhcp_on=True) |
126 | + if rack_interfaces_serving_dhcp.exists(): |
127 | + # Not external. It's a MAAS DHCP server. |
128 | + dhcp_ip = None |
129 | + if interface.vlan.external_dhcp != dhcp_ip: |
130 | + interface.vlan.external_dhcp = dhcp_ip |
131 | + interface.vlan.save() |
132 | |
133 | |
134 | @synchronous |
135 | |
136 | === modified file 'src/maasserver/rpc/regionservice.py' |
137 | --- src/maasserver/rpc/regionservice.py 2016-03-31 23:34:55 +0000 |
138 | +++ src/maasserver/rpc/regionservice.py 2016-04-07 13:22:34 +0000 |
139 | @@ -345,32 +345,19 @@ |
140 | return succeed({}) |
141 | |
142 | @region.ReportForeignDHCPServer.responder |
143 | - def report_foreign_dhcp_server(self, cluster_uuid, interface_name, |
144 | - foreign_dhcp_ip): |
145 | + def report_foreign_dhcp_server( |
146 | + self, system_id, interface_name, dhcp_ip=None): |
147 | """report_foreign_dhcp_server() |
148 | |
149 | Implementation of |
150 | - :py:class:`~provisioningserver.rpc.region.SendEvent`. |
151 | + :py:class:`~provisioningserver.rpc.region.ReportForeignDHCPServer`. |
152 | """ |
153 | d = deferToDatabase( |
154 | - rackcontrollers.update_foreign_dhcp_ip, |
155 | - cluster_uuid, interface_name, foreign_dhcp_ip) |
156 | + rackcontrollers.update_foreign_dhcp, |
157 | + system_id, interface_name, dhcp_ip) |
158 | d.addCallback(lambda _: {}) |
159 | return d |
160 | |
161 | - @region.GetClusterInterfaces.responder |
162 | - def get_cluster_interfaces(self, cluster_uuid): |
163 | - """get_cluster_interfaces() |
164 | - |
165 | - Implementation of |
166 | - :py:class:`~provisioningserver.rpc.region.GetClusterInterfaces`. |
167 | - """ |
168 | - d = deferToDatabase( |
169 | - rackcontrollers.get_rack_controllers_interfaces_as_dicts, |
170 | - cluster_uuid) |
171 | - d.addCallback(lambda interfaces: {'interfaces': interfaces}) |
172 | - return d |
173 | - |
174 | @region.CreateNode.responder |
175 | def create_node(self, architecture, power_type, power_parameters, |
176 | mac_addresses, domain=None, hostname=None): |
177 | |
178 | === modified file 'src/maasserver/rpc/tests/test_rackcontrollers.py' |
179 | --- src/maasserver/rpc/tests/test_rackcontrollers.py 2016-03-28 13:54:47 +0000 |
180 | +++ src/maasserver/rpc/tests/test_rackcontrollers.py 2016-04-07 13:22:34 +0000 |
181 | @@ -10,7 +10,11 @@ |
182 | from django.db import IntegrityError |
183 | from fixtures import FakeLogger |
184 | from maasserver import worker_user |
185 | -from maasserver.enum import NODE_TYPE |
186 | +from maasserver.enum import ( |
187 | + INTERFACE_TYPE, |
188 | + IPADDRESS_TYPE, |
189 | + NODE_TYPE, |
190 | +) |
191 | from maasserver.models import ( |
192 | Node, |
193 | NodeGroupToRackController, |
194 | @@ -20,6 +24,7 @@ |
195 | handle_upgrade, |
196 | register_new_rackcontroller, |
197 | register_rackcontroller, |
198 | + update_foreign_dhcp, |
199 | update_interfaces, |
200 | ) |
201 | from maasserver.testing.factory import factory |
202 | @@ -248,6 +253,54 @@ |
203 | hostname, logger.output.strip()) |
204 | |
205 | |
206 | +class TestUpdateForeignDHCP(MAASServerTestCase): |
207 | + |
208 | + def test__doesnt_fail_if_interface_missing(self): |
209 | + rack_controller = factory.make_RackController() |
210 | + # No error should be raised. |
211 | + update_foreign_dhcp( |
212 | + rack_controller.system_id, factory.make_name("eth"), None) |
213 | + |
214 | + def test__clears_external_dhcp_on_vlan(self): |
215 | + rack_controller = factory.make_RackController(interface=False) |
216 | + interface = factory.make_Interface( |
217 | + INTERFACE_TYPE.PHYSICAL, node=rack_controller) |
218 | + interface.vlan.external_dhcp = factory.make_ip_address() |
219 | + interface.vlan.save() |
220 | + update_foreign_dhcp( |
221 | + rack_controller.system_id, interface.name, None) |
222 | + self.assertIsNone(reload_object(interface.vlan).external_dhcp) |
223 | + |
224 | + def test__sets_external_dhcp_when_not_managed_vlan(self): |
225 | + rack_controller = factory.make_RackController(interface=False) |
226 | + interface = factory.make_Interface( |
227 | + INTERFACE_TYPE.PHYSICAL, node=rack_controller) |
228 | + dhcp_ip = factory.make_ip_address() |
229 | + update_foreign_dhcp( |
230 | + rack_controller.system_id, interface.name, dhcp_ip) |
231 | + self.assertEquals( |
232 | + dhcp_ip, reload_object(interface.vlan).external_dhcp) |
233 | + |
234 | + def test__clears_external_dhcp_when_managed_vlan(self): |
235 | + rack_controller = factory.make_RackController(interface=False) |
236 | + fabric = factory.make_Fabric() |
237 | + vlan = fabric.get_default_vlan() |
238 | + interface = factory.make_Interface( |
239 | + INTERFACE_TYPE.PHYSICAL, node=rack_controller, vlan=vlan) |
240 | + subnet = factory.make_Subnet() |
241 | + dhcp_ip = factory.pick_ip_in_Subnet(subnet) |
242 | + vlan.dhcp_on = True |
243 | + vlan.primary_rack = rack_controller |
244 | + vlan.external_dhcp = dhcp_ip |
245 | + vlan.save() |
246 | + factory.make_StaticIPAddress( |
247 | + alloc_type=IPADDRESS_TYPE.STICKY, ip=dhcp_ip, |
248 | + subnet=subnet, interface=interface) |
249 | + update_foreign_dhcp( |
250 | + rack_controller.system_id, interface.name, dhcp_ip) |
251 | + self.assertIsNone(reload_object(interface.vlan).external_dhcp) |
252 | + |
253 | + |
254 | class TestUpdateInterfaces(MAASServerTestCase): |
255 | |
256 | def test__calls_update_interfaces_on_rack_controller(self): |
257 | |
258 | === modified file 'src/maasserver/rpc/tests/test_regionservice.py' |
259 | --- src/maasserver/rpc/tests/test_regionservice.py 2016-03-31 23:34:55 +0000 |
260 | +++ src/maasserver/rpc/tests/test_regionservice.py 2016-04-07 13:22:34 +0000 |
261 | @@ -121,7 +121,6 @@ |
262 | GetBootConfig, |
263 | GetBootSources, |
264 | GetBootSourcesV2, |
265 | - GetClusterInterfaces, |
266 | GetProxies, |
267 | Identify, |
268 | ListNodePowerParameters, |
269 | @@ -129,6 +128,7 @@ |
270 | RegisterEventType, |
271 | RegisterRackController, |
272 | ReportBootImages, |
273 | + ReportForeignDHCPServer, |
274 | RequestNodeInfoByMACAddress, |
275 | SendEvent, |
276 | SendEventMACAddress, |
277 | @@ -2590,101 +2590,47 @@ |
278 | # If the RPC service is down, _get_addresses() returns nothing. |
279 | self.assertItemsEqual([], service._get_addresses()) |
280 | |
281 | -# NODE_GROUP_REMOVAL - blake_r - Fix. |
282 | -#class TestRegionProtocol_ReportForeignDHCPServer( |
283 | -# MAASTransactionServerTestCase): |
284 | -# |
285 | -# def test_create_node_is_registered(self): |
286 | -# protocol = Region() |
287 | -# responder = protocol.locateResponder( |
288 | -# ReportForeignDHCPServer.commandName) |
289 | -# self.assertIsNotNone(responder) |
290 | -# |
291 | -# @transactional |
292 | -# def create_cluster_interface(self): |
293 | -# cluster = factory.make_NodeGroup() |
294 | -# return factory.make_NodeGroupInterface(cluster) |
295 | -# |
296 | -# @wait_for_reactor |
297 | -# @inlineCallbacks |
298 | -# def test_sets_foreign_dhcp_value(self): |
299 | -# foreign_dhcp_ip = factory.make_ipv4_address() |
300 | -# cluster_interface = yield deferToDatabase( |
301 | -# self.create_cluster_interface) |
302 | -# cluster = cluster_interface.nodegroup |
303 | -# |
304 | -# response = yield call_responder( |
305 | -# Region(), ReportForeignDHCPServer, |
306 | -# { |
307 | -# 'cluster_uuid': cluster.uuid, |
308 | -# 'interface_name': cluster_interface.name, |
309 | -# 'foreign_dhcp_ip': foreign_dhcp_ip, |
310 | -# }) |
311 | -# |
312 | -# self.assertEqual({}, response) |
313 | -# cluster_interface = yield deferToDatabase( |
314 | -# transactional_reload_object, cluster_interface) |
315 | -# |
316 | -# self.assertEqual( |
317 | -# foreign_dhcp_ip, cluster_interface.foreign_dhcp_ip) |
318 | -# |
319 | -# @wait_for_reactor |
320 | -# @inlineCallbacks |
321 | -# def test_does_not_trigger_update_signal(self): |
322 | -# configure_dhcp = self.patch_autospec(dhcp, "configure_dhcp") |
323 | -# |
324 | -# foreign_dhcp_ip = factory.make_ipv4_address() |
325 | -# cluster_interface = yield deferToDatabase( |
326 | -# self.create_cluster_interface) |
327 | -# cluster = cluster_interface.nodegroup |
328 | -# |
329 | -# response = yield call_responder( |
330 | -# Region(), ReportForeignDHCPServer, |
331 | -# { |
332 | -# 'cluster_uuid': cluster.uuid, |
333 | -# 'interface_name': cluster_interface.name, |
334 | -# 'foreign_dhcp_ip': foreign_dhcp_ip, |
335 | -# }) |
336 | -# |
337 | -# self.assertEqual({}, response) |
338 | -# self.assertThat(configure_dhcp, MockNotCalled()) |
339 | - |
340 | - |
341 | -class TestRegionProtocol_GetClusterInterfaces(MAASTransactionServerTestCase): |
342 | - |
343 | - def test_create_node_is_registered(self): |
344 | + |
345 | +class TestRegionProtocol_ReportForeignDHCPServer( |
346 | + MAASTransactionServerTestCase): |
347 | + |
348 | + def test_is_registered(self): |
349 | protocol = Region() |
350 | responder = protocol.locateResponder( |
351 | - GetClusterInterfaces.commandName) |
352 | + ReportForeignDHCPServer.commandName) |
353 | self.assertIsNotNone(responder) |
354 | |
355 | @transactional |
356 | - def create_controller_and_interfaces(self): |
357 | - controller = factory.make_RackController() |
358 | - for _ in range(3): |
359 | - factory.make_Interface(INTERFACE_TYPE.PHYSICAL, node=controller) |
360 | - interfaces = [ |
361 | + def create_rack_interface(self): |
362 | + rack_controller = factory.make_RackController(interface=False) |
363 | + interface = factory.make_Interface( |
364 | + INTERFACE_TYPE.PHYSICAL, node=rack_controller) |
365 | + return rack_controller, interface |
366 | + |
367 | + @transactional |
368 | + def get_vlan_for_interface(self, interface): |
369 | + return reload_object(interface.vlan) |
370 | + |
371 | + @wait_for_reactor |
372 | + @inlineCallbacks |
373 | + def test_sets_external_dhcp_value(self): |
374 | + dhcp_ip = factory.make_ipv4_address() |
375 | + rack, interface = yield deferToDatabase( |
376 | + self.create_rack_interface) |
377 | + |
378 | + response = yield call_responder( |
379 | + Region(), ReportForeignDHCPServer, |
380 | { |
381 | - 'name': interface.name, |
382 | - 'interface': interface.name, |
383 | - 'ip': '', |
384 | - } |
385 | - for interface in controller.interface_set.all()] |
386 | - return controller, interfaces |
387 | - |
388 | - @wait_for_reactor |
389 | - @inlineCallbacks |
390 | - def test_returns_all_cluster_interfaces(self): |
391 | - controller, expected_interfaces = yield deferToDatabase( |
392 | - self.create_controller_and_interfaces) |
393 | - |
394 | - response = yield call_responder( |
395 | - Region(), GetClusterInterfaces, |
396 | - {'cluster_uuid': controller.system_id}) |
397 | - |
398 | - self.assertIsNot(None, response) |
399 | - self.assertItemsEqual( |
400 | - expected_interfaces, response["interfaces"]) |
401 | + 'system_id': rack.system_id, |
402 | + 'interface_name': interface.name, |
403 | + 'dhcp_ip': dhcp_ip, |
404 | + }) |
405 | + |
406 | + self.assertEqual({}, response) |
407 | + vlan = yield deferToDatabase( |
408 | + self.get_vlan_for_interface, interface) |
409 | + self.assertEqual( |
410 | + dhcp_ip, vlan.external_dhcp) |
411 | |
412 | |
413 | class TestRegionProtocol_CreateNode(MAASTransactionServerTestCase): |
414 | |
415 | === modified file 'src/provisioningserver/dhcp/detect.py' |
416 | --- src/provisioningserver/dhcp/detect.py 2016-03-28 13:54:47 +0000 |
417 | +++ src/provisioningserver/dhcp/detect.py 2016-04-07 13:22:34 +0000 |
418 | @@ -1,3 +1,4 @@ |
419 | + |
420 | # Copyright 2013-2016 Canonical Ltd. This software is licensed under the |
421 | # GNU Affero General Public License version 3 (see the file LICENSE). |
422 | |
423 | @@ -10,21 +11,10 @@ |
424 | from contextlib import contextmanager |
425 | import errno |
426 | import fcntl |
427 | -import http.client |
428 | -import json |
429 | from random import randint |
430 | import socket |
431 | import struct |
432 | -from urllib.error import ( |
433 | - HTTPError, |
434 | - URLError, |
435 | -) |
436 | |
437 | -from apiclient.maas_client import ( |
438 | - MAASClient, |
439 | - MAASDispatcher, |
440 | - MAASOAuth, |
441 | -) |
442 | from provisioningserver.logger import get_maas_logger |
443 | |
444 | |
445 | @@ -204,64 +194,10 @@ |
446 | return receive_offers(transaction_id) |
447 | |
448 | |
449 | -def process_request(client_func, *args, **kwargs): |
450 | - """Run a MAASClient query and check for common errors. |
451 | - |
452 | - :return: None if there is an error, otherwise the decoded response body. |
453 | - """ |
454 | - try: |
455 | - response = client_func(*args, **kwargs) |
456 | - except (HTTPError, URLError) as e: |
457 | - maaslog.warning("Failed to contact region controller:\n%s", e) |
458 | - return None |
459 | - code = response.getcode() |
460 | - if code != http.client.OK: |
461 | - maaslog.error( |
462 | - "Failed talking to region controller, it returned:\n%s\n%s", |
463 | - code, response.read()) |
464 | - return None |
465 | - try: |
466 | - raw_data = response.read() |
467 | - if len(raw_data) > 0: |
468 | - data = json.loads(raw_data) |
469 | - else: |
470 | - return None |
471 | - except ValueError as e: |
472 | - maaslog.error( |
473 | - "Failed to decode response from region controller:\n%s", e) |
474 | - return None |
475 | - return data |
476 | - |
477 | - |
478 | -def determine_cluster_interfaces(knowledge): |
479 | - """Given server knowledge, determine network interfaces on this cluster. |
480 | - |
481 | - :return: a list of tuples of (interface name, ip) for all interfaces. |
482 | - |
483 | - :note: this uses an API call and not local probing because the |
484 | - region controller has the definitive and final say in what does and |
485 | - doesn't exist. |
486 | - """ |
487 | - api_path = ( |
488 | - 'api/2.0/nodegroups/%s/interfaces/' % knowledge['nodegroup_uuid']) |
489 | - oauth = MAASOAuth(*knowledge['api_credentials']) |
490 | - client = MAASClient(oauth, MAASDispatcher(), knowledge['maas_url']) |
491 | - interfaces = process_request(client.get, api_path, 'list') |
492 | - if interfaces is None: |
493 | - return None |
494 | - |
495 | - interface_names = sorted( |
496 | - (interface['interface'], interface['ip']) |
497 | - for interface in interfaces |
498 | - if interface['interface'] != '') |
499 | - return interface_names |
500 | - |
501 | - |
502 | -def probe_interface(interface, ip): |
503 | +def probe_interface(interface): |
504 | """Probe the given interface for DHCP servers. |
505 | |
506 | - :param interface: interface as returned from determine_cluster_interfaces |
507 | - :param ip: ip as returned from determine_cluster_interfaces |
508 | + :param interface: interface name |
509 | :return: A set of IP addresses of detected servers. |
510 | |
511 | :note: Any servers running on the IP address of the local host are |
512 | @@ -287,63 +223,4 @@ |
513 | "your cluster interfaces configuration.", interface) |
514 | else: |
515 | raise |
516 | - # Using servers.discard(ip) here breaks Mock in the tests, so |
517 | - # we're creating a copy of the set instead. |
518 | - results = servers.difference([ip]) |
519 | - return results |
520 | - |
521 | - |
522 | -def update_region_controller(knowledge, interface, server): |
523 | - """Update the region controller with the status of the probe. |
524 | - |
525 | - :param knowledge: dictionary of server info |
526 | - :param interface: name of interface, e.g. eth0 |
527 | - :param server: IP address of detected DHCP server, or None |
528 | - """ |
529 | - api_path = 'api/2.0/nodegroups/%s/interfaces/%s/' % ( |
530 | - knowledge['nodegroup_uuid'], interface) |
531 | - oauth = MAASOAuth(*knowledge['api_credentials']) |
532 | - client = MAASClient(oauth, MAASDispatcher(), knowledge['maas_url']) |
533 | - if server is None: |
534 | - server = '' |
535 | - process_request( |
536 | - client.post, api_path, 'report_foreign_dhcp', foreign_dhcp_ip=server) |
537 | - |
538 | - |
539 | -def periodic_probe_task(api_knowledge): |
540 | - """Probe for DHCP servers and set NodeGroupInterface.foriegn_dhcp. |
541 | - |
542 | - This should be run periodically so that the database has an up-to-date |
543 | - view of any rogue DHCP servers on the network. |
544 | - |
545 | - NOTE: This uses blocking I/O with sequential polling of interfaces, and |
546 | - hence doesn't scale well. It's a future improvement to make |
547 | - to throw it in parallel threads or async I/O. |
548 | - |
549 | - :param api_knowledge: A dict of the information needed to be able to |
550 | - make requests to the region's REST API. |
551 | - """ |
552 | - # Determine all the active interfaces on this cluster (nodegroup). |
553 | - interfaces = determine_cluster_interfaces(api_knowledge) |
554 | - if interfaces is None: |
555 | - maaslog.info("No interfaces on cluster, not probing DHCP.") |
556 | - return |
557 | - |
558 | - # Iterate over interfaces and probe each one. |
559 | - for interface, ip in interfaces: |
560 | - try: |
561 | - servers = probe_interface(interface, ip) |
562 | - except socket.error: |
563 | - maaslog.error( |
564 | - "Failed to probe sockets; did you configure authbind as per " |
565 | - "HACKING.txt?") |
566 | - return |
567 | - else: |
568 | - if len(servers) > 0: |
569 | - # Only send one, if it gets cleared out then the |
570 | - # next detection pass will send a different one, if it |
571 | - # still exists. |
572 | - update_region_controller( |
573 | - api_knowledge, interface, servers.pop()) |
574 | - else: |
575 | - update_region_controller(api_knowledge, interface, None) |
576 | + return servers |
577 | |
578 | === removed file 'src/provisioningserver/dhcp/probe.py' |
579 | --- src/provisioningserver/dhcp/probe.py 2015-12-01 18:12:59 +0000 |
580 | +++ src/provisioningserver/dhcp/probe.py 1970-01-01 00:00:00 +0000 |
581 | @@ -1,41 +0,0 @@ |
582 | -#!/usr/bin/env python2.7 |
583 | -# Copyright 2013-2015 Canonical Ltd. This software is licensed under the |
584 | -# GNU Affero General Public License version 3 (see the file LICENSE). |
585 | - |
586 | -"""Probe network on given network interface for a DHCP server. |
587 | - |
588 | -This needs to be run as root, in order to be allowed to broadcast on the |
589 | -BOOTP port. |
590 | - |
591 | -Exit code is zero ("success") if no servers were detected, or the number of |
592 | -DHCP servers that were found. |
593 | -""" |
594 | - |
595 | -import argparse |
596 | -from sys import exit |
597 | - |
598 | -from provisioningserver.dhcp.detect import probe_dhcp |
599 | - |
600 | - |
601 | -argument_parser = argparse.ArgumentParser(description=__doc__) |
602 | - |
603 | - |
604 | -def main(): |
605 | - argument_parser.add_argument( |
606 | - 'interface', |
607 | - help="Probe network on this network interface.") |
608 | - |
609 | - args = argument_parser.parse_args() |
610 | - |
611 | - servers = probe_dhcp(args.interface) |
612 | - |
613 | - num_servers = len(servers) |
614 | - if num_servers == 0: |
615 | - print("No DHCP servers detected.") |
616 | - exit(0) |
617 | - else: |
618 | - print("DHCP servers detected: %s" % ', '.join(sorted(servers))) |
619 | - exit(num_servers) |
620 | - |
621 | -if __name__ == "__main__": |
622 | - main() |
623 | |
624 | === modified file 'src/provisioningserver/dhcp/tests/test_detect.py' |
625 | --- src/provisioningserver/dhcp/tests/test_detect.py 2016-03-28 13:54:47 +0000 |
626 | +++ src/provisioningserver/dhcp/tests/test_detect.py 2016-04-07 13:22:34 +0000 |
627 | @@ -7,39 +7,25 @@ |
628 | |
629 | import errno |
630 | import fcntl |
631 | -import http.client |
632 | import socket |
633 | -import textwrap |
634 | -import urllib.error |
635 | -import urllib.parse |
636 | -import urllib.request |
637 | |
638 | -from apiclient.maas_client import MAASClient |
639 | -from apiclient.testing.credentials import make_api_credentials |
640 | -from fixtures import FakeLogger |
641 | from maastesting.factory import factory |
642 | -from maastesting.matchers import MockCalledOnceWith |
643 | from maastesting.testcase import MAASTestCase |
644 | import mock |
645 | -from mock import sentinel |
646 | from provisioningserver.dhcp.detect import ( |
647 | BOOTP_CLIENT_PORT, |
648 | BOOTP_SERVER_PORT, |
649 | - determine_cluster_interfaces, |
650 | DHCPDiscoverPacket, |
651 | DHCPOfferPacket, |
652 | get_interface_IP, |
653 | get_interface_MAC, |
654 | make_transaction_ID, |
655 | - periodic_probe_task, |
656 | probe_interface, |
657 | receive_offers, |
658 | request_dhcp, |
659 | udp_socket, |
660 | - update_region_controller, |
661 | ) |
662 | import provisioningserver.dhcp.detect as detect_module |
663 | -from provisioningserver.testing.config import ClusterConfigurationFixture |
664 | from provisioningserver.testing.testcase import PservTestCase |
665 | |
666 | |
667 | @@ -306,230 +292,35 @@ |
668 | receive_offers, factory.make_bytes(4)) |
669 | |
670 | |
671 | -class MockResponse: |
672 | - # This implements just enough to look lke a urllib2 response object. |
673 | - def __init__(self, code=None, response=None): |
674 | - if code is None: |
675 | - code = http.client.OK |
676 | - self.code = code |
677 | - if response is None: |
678 | - response = "" |
679 | - self.response = response |
680 | - |
681 | - def getcode(self): |
682 | - return self.code |
683 | - |
684 | - def read(self): |
685 | - return self.response |
686 | - |
687 | - |
688 | class TestPeriodicTask(PservTestCase): |
689 | |
690 | - def setUp(self): |
691 | - # Initialise the knowledge cache. |
692 | - super(TestPeriodicTask, self).setUp() |
693 | - self.maaslog = self.useFixture(FakeLogger("maas.dhcp.detect")) |
694 | - uuid = factory.make_UUID() |
695 | - maas_url = 'http://%s.example.com/%s/' % ( |
696 | - factory.make_name('host'), |
697 | - factory.make_string(), |
698 | - ) |
699 | - api_credentials = make_api_credentials() |
700 | - self.useFixture(ClusterConfigurationFixture(maas_url=maas_url)) |
701 | - self.knowledge = dict( |
702 | - nodegroup_uuid=uuid, |
703 | - api_credentials=api_credentials, |
704 | - maas_url=maas_url) |
705 | - |
706 | - def make_fake_interfaces_response(self, interfaces_pairs): |
707 | - stanzas = [] |
708 | - for interfaces_pair in interfaces_pairs: |
709 | - stanza = textwrap.dedent(""" |
710 | - {{ |
711 | - "ip_range_high": null, |
712 | - "ip_range_low": null, |
713 | - "broadcast_ip": null, |
714 | - "ip": "{1}", |
715 | - "subnet_mask": "255.255.255.0", |
716 | - "management": 0, |
717 | - "interface": "{0}" |
718 | - }}""").format(*interfaces_pair) |
719 | - stanzas.append(stanza) |
720 | - interfaces_json = "[" |
721 | - interfaces_json += ",".join(stanzas) |
722 | - interfaces_json += "]" |
723 | - return interfaces_json |
724 | - |
725 | - def patch_fake_interfaces_list(self, interfaces_pairs): |
726 | - # Set up the api client to return a fake set of interfaces. |
727 | - # Determine_cluster_interfaces calls the API to discover what |
728 | - # interfaces are available, so any test code that calls it |
729 | - # should first call this helper to set up the required fake response. |
730 | - interfaces_json = self.make_fake_interfaces_response(interfaces_pairs) |
731 | - self.patch(MAASClient, 'get').return_value = MockResponse( |
732 | - http.client.OK, interfaces_json) |
733 | - |
734 | - def test_determine_cluster_interfaces_returns_interface_names(self): |
735 | - eth0_addr = factory.make_ipv4_address() |
736 | - wlan0_addr = factory.make_ipv4_address() |
737 | - self.patch_fake_interfaces_list( |
738 | - [("eth0", eth0_addr), ("wlan0", wlan0_addr)]) |
739 | - self.assertEqual( |
740 | - [("eth0", eth0_addr), ("wlan0", wlan0_addr)], |
741 | - determine_cluster_interfaces(self.knowledge)) |
742 | - |
743 | def test_probe_interface_returns_empty_set_when_nothing_detected(self): |
744 | - eth0_addr = factory.make_ipv4_address() |
745 | - self.patch_fake_interfaces_list([("eth0", eth0_addr)]) |
746 | self.patch(detect_module, 'probe_dhcp').return_value = set() |
747 | - interfaces = determine_cluster_interfaces(self.knowledge) |
748 | - results = probe_interface(*interfaces[0]) |
749 | + results = probe_interface("eth0") |
750 | self.assertEqual(set(), results) |
751 | |
752 | def test_probe_interface_returns_empty_set_when_IP_missing(self): |
753 | # If the interface being probed has no IP address, the |
754 | # request_dhcr() method will raise IOError with errno 99. Make |
755 | # sure this is caught and ignored. |
756 | - eth0_addr = factory.make_ipv4_address() |
757 | - self.patch_fake_interfaces_list([("eth0", eth0_addr)]) |
758 | ioerror = IOError( |
759 | errno.EADDRNOTAVAIL, "Cannot assign requested address") |
760 | self.patch(fcntl, 'ioctl').side_effect = ioerror |
761 | - interfaces = determine_cluster_interfaces(self.knowledge) |
762 | - results = probe_interface(*interfaces[0]) |
763 | + results = probe_interface("eth0") |
764 | self.assertEqual(set(), results) |
765 | |
766 | def test_probe_interface_returns_empty_set_when_device_missing(self): |
767 | # If the interface being probed does not exist, the |
768 | # request_dhcp() method will raise IOError with errno 19. Make |
769 | # sure this is caught and ignored. |
770 | - eth0_addr = factory.make_ipv4_address() |
771 | - self.patch_fake_interfaces_list([("eth0", eth0_addr)]) |
772 | ioerror = IOError(errno.ENODEV, "No such device") |
773 | self.patch(fcntl, 'ioctl').side_effect = ioerror |
774 | - interfaces = determine_cluster_interfaces(self.knowledge) |
775 | - results = probe_interface(*interfaces[0]) |
776 | + results = probe_interface("eth0") |
777 | self.assertEqual(set(), results) |
778 | |
779 | def test_probe_interface_returns_populated_set(self): |
780 | # Test that the detected DHCP server is returned. |
781 | - eth0_addr = factory.make_ipv4_address() |
782 | - self.patch_fake_interfaces_list([("eth0", eth0_addr)]) |
783 | self.patch( |
784 | detect_module, 'probe_dhcp').return_value = {'10.2.2.2'} |
785 | - interfaces = determine_cluster_interfaces(self.knowledge) |
786 | - results = probe_interface(*interfaces[0]) |
787 | + results = probe_interface("eth0") |
788 | self.assertEqual({'10.2.2.2'}, results) |
789 | - |
790 | - def test_probe_interface_filters_interface_own_ip(self): |
791 | - # Test that the interface shows the detected DHCP server except |
792 | - # if it is the same IP as the interface's. |
793 | - eth0_addr = factory.make_ipv4_address() |
794 | - self.patch_fake_interfaces_list([("eth0", eth0_addr)]) |
795 | - detected_dhcp = eth0_addr |
796 | - self.patch(detect_module, 'probe_dhcp').return_value = {detected_dhcp} |
797 | - interfaces = determine_cluster_interfaces(self.knowledge) |
798 | - results = probe_interface(*interfaces[0]) |
799 | - self.assertEqual(set(), results) |
800 | - |
801 | - def test_determine_cluster_interfaces_catchs_HTTPError_in_MAASClient(self): |
802 | - self.patch(MAASClient, 'get').side_effect = urllib.error.HTTPError( |
803 | - sentinel.url, sentinel.code, sentinel.msg, sentinel.hdrs, None) |
804 | - determine_cluster_interfaces(self.knowledge) |
805 | - self.assertIn( |
806 | - "Failed to contact region controller:", self.maaslog.output) |
807 | - |
808 | - def test_determine_cluster_interfaces_catches_URLError_in_MAASClient(self): |
809 | - self.patch(MAASClient, 'get').side_effect = urllib.error.URLError( |
810 | - sentinel.arg1) |
811 | - determine_cluster_interfaces(self.knowledge) |
812 | - self.assertIn( |
813 | - "Failed to contact region controller:", self.maaslog.output) |
814 | - |
815 | - def test_determine_cluster_interfaces_catches_non_OK_response(self): |
816 | - self.patch(MAASClient, 'get').return_value = MockResponse( |
817 | - http.client.NOT_FOUND, "error text") |
818 | - determine_cluster_interfaces(self.knowledge) |
819 | - self.assertIn( |
820 | - "Failed talking to region controller, it returned:", |
821 | - self.maaslog.output) |
822 | - |
823 | - def test_update_region_controller_sets_detected_dhcp(self): |
824 | - mocked_post = self.patch(MAASClient, 'post') |
825 | - mocked_post.return_value = MockResponse() |
826 | - detected_server = factory.make_ipv4_address() |
827 | - update_region_controller(self.knowledge, "eth0", detected_server) |
828 | - uuid = self.knowledge['nodegroup_uuid'] |
829 | - self.assertThat(mocked_post, MockCalledOnceWith( |
830 | - 'api/2.0/nodegroups/%s/interfaces/eth0/' % uuid, |
831 | - 'report_foreign_dhcp', foreign_dhcp_ip=detected_server)) |
832 | - |
833 | - def test_update_region_controller_clears_detected_dhcp(self): |
834 | - mocked_post = self.patch(MAASClient, 'post') |
835 | - mocked_post.return_value = MockResponse() |
836 | - detected_server = None |
837 | - update_region_controller(self.knowledge, "eth0", detected_server) |
838 | - uuid = self.knowledge['nodegroup_uuid'] |
839 | - self.assertThat(mocked_post, MockCalledOnceWith( |
840 | - 'api/2.0/nodegroups/%s/interfaces/eth0/' % uuid, |
841 | - 'report_foreign_dhcp', foreign_dhcp_ip='')) |
842 | - |
843 | - def test_update_region_controller_catches_HTTPError_in_MAASClient(self): |
844 | - self.patch(MAASClient, 'post').side_effect = urllib.error.HTTPError( |
845 | - sentinel.url, sentinel.code, sentinel.msg, sentinel.hdrs, None) |
846 | - update_region_controller(self.knowledge, "eth0", None) |
847 | - self.assertIn( |
848 | - "Failed to contact region controller:", self.maaslog.output) |
849 | - |
850 | - def test_update_region_controller_catches_URLError_in_MAASClient(self): |
851 | - self.patch(MAASClient, 'post').side_effect = urllib.error.URLError( |
852 | - sentinel.arg1) |
853 | - update_region_controller(self.knowledge, "eth0", None) |
854 | - self.assertIn( |
855 | - "Failed to contact region controller:", self.maaslog.output) |
856 | - |
857 | - def test_update_region_controller_catches_non_OK_response(self): |
858 | - mock_response = MockResponse(http.client.NOT_FOUND, "error text") |
859 | - self.patch(MAASClient, 'post').return_value = mock_response |
860 | - update_region_controller(self.knowledge, "eth0", None) |
861 | - self.assertIn( |
862 | - "Failed talking to region controller, it returned:", |
863 | - self.maaslog.output) |
864 | - |
865 | - def test_periodic_probe_task_exits_if_no_interfaces(self): |
866 | - mocked = self.patch(detect_module, 'probe_interface') |
867 | - self.patch( |
868 | - detect_module, 'determine_cluster_interfaces').return_value = None |
869 | - periodic_probe_task(self.knowledge) |
870 | - self.assertFalse(mocked.called) |
871 | - |
872 | - def test_periodic_probe_task_updates_region_with_detected_server(self): |
873 | - eth0_addr = factory.make_ipv4_address() |
874 | - wlan0_addr = factory.make_ipv4_address() |
875 | - detected_server = factory.make_ipv4_address() |
876 | - self.patch_fake_interfaces_list( |
877 | - [("eth0", eth0_addr), ("wlan0", wlan0_addr)]) |
878 | - self.patch( |
879 | - detect_module, 'probe_dhcp').return_value = {detected_server} |
880 | - mocked_update = self.patch(detect_module, 'update_region_controller') |
881 | - periodic_probe_task(self.knowledge) |
882 | - calls = [ |
883 | - mock.call(self.knowledge, 'eth0', detected_server), |
884 | - mock.call(self.knowledge, 'wlan0', detected_server), |
885 | - ] |
886 | - mocked_update.assert_has_calls(calls, any_order=True) |
887 | - |
888 | - def test_periodic_probe_task_updates_region_with_no_detected_server(self): |
889 | - eth0_addr = factory.make_ipv4_address() |
890 | - wlan0_addr = factory.make_ipv4_address() |
891 | - self.patch_fake_interfaces_list( |
892 | - [("eth0", eth0_addr), ("wlan0", wlan0_addr)]) |
893 | - self.patch( |
894 | - detect_module, 'probe_dhcp').return_value = set() |
895 | - mocked_update = self.patch(detect_module, 'update_region_controller') |
896 | - periodic_probe_task(self.knowledge) |
897 | - calls = [ |
898 | - mock.call(self.knowledge, 'eth0', None), |
899 | - mock.call(self.knowledge, 'wlan0', None), |
900 | - ] |
901 | - mocked_update.assert_has_calls(calls, any_order=True) |
902 | |
903 | === modified file 'src/provisioningserver/pserv_services/dhcp_probe_service.py' |
904 | --- src/provisioningserver/pserv_services/dhcp_probe_service.py 2016-03-28 13:54:47 +0000 |
905 | +++ src/provisioningserver/pserv_services/dhcp_probe_service.py 2016-04-07 13:22:34 +0000 |
906 | @@ -14,19 +14,14 @@ |
907 | from provisioningserver.dhcp.detect import probe_interface |
908 | from provisioningserver.logger.log import get_maas_logger |
909 | from provisioningserver.rpc.exceptions import NoConnectionsAvailable |
910 | -from provisioningserver.rpc.region import ( |
911 | - GetClusterInterfaces, |
912 | - ReportForeignDHCPServer, |
913 | -) |
914 | +from provisioningserver.rpc.region import ReportForeignDHCPServer |
915 | +from provisioningserver.utils.network import get_all_interfaces_definition |
916 | from provisioningserver.utils.twisted import ( |
917 | pause, |
918 | retries, |
919 | ) |
920 | from twisted.application.internet import TimerService |
921 | -from twisted.internet.defer import ( |
922 | - inlineCallbacks, |
923 | - returnValue, |
924 | -) |
925 | +from twisted.internet.defer import inlineCallbacks |
926 | from twisted.internet.threads import deferToThread |
927 | from twisted.protocols.amp import UnhandledCommand |
928 | |
929 | @@ -35,12 +30,11 @@ |
930 | |
931 | |
932 | class DHCPProbeService(TimerService, object): |
933 | - """Service to probe for DHCP servers on this cluster's network. |
934 | + """Service to probe for DHCP servers on the rack controller interface's. |
935 | |
936 | Built on top of Twisted's `TimerService`. |
937 | |
938 | :param reactor: An `IReactor` instance. |
939 | - :param cluster_uuid: This cluster's UUID. |
940 | """ |
941 | |
942 | check_interval = timedelta(minutes=10).total_seconds() |
943 | @@ -52,48 +46,44 @@ |
944 | self.clock = reactor |
945 | self.client_service = client_service |
946 | |
947 | - @inlineCallbacks |
948 | - def _get_cluster_interfaces(self, client): |
949 | - """Return the interfaces for this cluster.""" |
950 | - try: |
951 | - response = yield client( |
952 | - GetClusterInterfaces, cluster_uuid=client.localIdent) |
953 | - except UnhandledCommand: |
954 | - # The region hasn't been upgraded to support this method |
955 | - # yet, so give up. Returning an empty dict means that this |
956 | - # run will end, since there are no interfaces to check. |
957 | - maaslog.error( |
958 | - "Unable to query region for interfaces: Region does not " |
959 | - "support the GetClusterInterfaces RPC method.") |
960 | - returnValue({}) |
961 | - else: |
962 | - returnValue(response['interfaces']) |
963 | + def _get_interfaces(self): |
964 | + """Return the interfaces for this rack controller.""" |
965 | + d = deferToThread(get_all_interfaces_definition) |
966 | + d.addCallback(lambda interfaces: [ |
967 | + name |
968 | + for name, info in interfaces.items() |
969 | + if info["enabled"] |
970 | + ]) |
971 | + return d |
972 | |
973 | - @inlineCallbacks |
974 | - def _inform_region_of_foreign_dhcp(self, client, name, |
975 | - foreign_dhcp_ip): |
976 | - """Tell the region that there's a rogue DHCP server. |
977 | + def _inform_region_of_dhcp(self, client, name, dhcp_ip): |
978 | + """Tell the region about the DHCP server. |
979 | |
980 | :param client: The RPC client to use. |
981 | :param name: The name of the network interface where the rogue |
982 | DHCP server was found. |
983 | - :param foreign_dhcp_ip: The IP address of the rogue server. |
984 | + :param dhcp_ip: The IP address of the DHCP server. |
985 | """ |
986 | - try: |
987 | - yield client( |
988 | - ReportForeignDHCPServer, cluster_uuid=client.localIdent, |
989 | - interface_name=name, foreign_dhcp_ip=foreign_dhcp_ip) |
990 | - except UnhandledCommand: |
991 | + |
992 | + def eb_unhandled(failure): |
993 | + failure.trap(UnhandledCommand) |
994 | # Not a lot we can do here... The region doesn't support |
995 | # this method yet. |
996 | maaslog.error( |
997 | - "Unable to inform region of rogue DHCP server: the region " |
998 | + "Unable to inform region of DHCP server: the region " |
999 | "does not yet support the ReportForeignDHCPServer RPC " |
1000 | "method.") |
1001 | |
1002 | + d = client( |
1003 | + ReportForeignDHCPServer, system_id=client.localIdent, |
1004 | + interface_name=name, dhcp_ip=dhcp_ip) |
1005 | + d.addErrback(eb_unhandled) |
1006 | + return d |
1007 | + |
1008 | @inlineCallbacks |
1009 | def probe_dhcp(self): |
1010 | - """Find all the interfaces on this cluster and probe for DHCP servers. |
1011 | + """Find all the interfaces on this rack controller and probe for |
1012 | + DHCP servers. |
1013 | """ |
1014 | client = None |
1015 | for elapsed, remaining, wait in retries(15, 5, self.clock): |
1016 | @@ -107,12 +97,11 @@ |
1017 | "Can't initiate DHCP probe, no RPC connection to region.") |
1018 | return |
1019 | |
1020 | - cluster_interfaces = yield self._get_cluster_interfaces(client) |
1021 | # Iterate over interfaces and probe each one. |
1022 | - for interface in cluster_interfaces: |
1023 | + interfaces = yield self._get_interfaces() |
1024 | + for interface in interfaces: |
1025 | try: |
1026 | - servers = yield deferToThread( |
1027 | - probe_interface, interface['interface'], interface['ip']) |
1028 | + servers = yield deferToThread(probe_interface, interface) |
1029 | except socket.error: |
1030 | maaslog.error( |
1031 | "Failed to probe sockets; did you configure authbind as " |
1032 | @@ -123,11 +112,11 @@ |
1033 | # Only send one, if it gets cleared out then the |
1034 | # next detection pass will send a different one, if it |
1035 | # still exists. |
1036 | - yield self._inform_region_of_foreign_dhcp( |
1037 | - client, interface['name'], servers.pop()) |
1038 | + yield self._inform_region_of_dhcp( |
1039 | + client, interface, servers.pop()) |
1040 | else: |
1041 | - yield self._inform_region_of_foreign_dhcp( |
1042 | - client, interface['name'], None) |
1043 | + yield self._inform_region_of_dhcp( |
1044 | + client, interface, None) |
1045 | |
1046 | @inlineCallbacks |
1047 | def try_probe_dhcp(self): |
1048 | @@ -136,7 +125,7 @@ |
1049 | yield self.probe_dhcp() |
1050 | except Exception as error: |
1051 | maaslog.error( |
1052 | - "Unable to probe for rogue DHCP servers: %s", |
1053 | + "Unable to probe for DHCP servers: %s", |
1054 | str(error)) |
1055 | else: |
1056 | maaslog.debug("Finished periodic DHCP probe.") |
1057 | |
1058 | === modified file 'src/provisioningserver/pserv_services/tests/test_dhcp_probe_service.py' |
1059 | --- src/provisioningserver/pserv_services/tests/test_dhcp_probe_service.py 2016-03-28 13:54:47 +0000 |
1060 | +++ src/provisioningserver/pserv_services/tests/test_dhcp_probe_service.py 2016-04-07 13:22:34 +0000 |
1061 | @@ -11,10 +11,12 @@ |
1062 | get_mock_calls, |
1063 | HasLength, |
1064 | MockCalledOnceWith, |
1065 | + MockCallsMatch, |
1066 | MockNotCalled, |
1067 | ) |
1068 | from maastesting.testcase import MAASTwistedRunTest |
1069 | from mock import ( |
1070 | + call, |
1071 | Mock, |
1072 | sentinel, |
1073 | ) |
1074 | @@ -39,19 +41,9 @@ |
1075 | def patch_rpc_methods(self): |
1076 | fixture = self.useFixture(MockLiveClusterToRegionRPCFixture()) |
1077 | protocol, connecting = fixture.makeEventLoop( |
1078 | - region.GetClusterInterfaces, region.ReportForeignDHCPServer) |
1079 | + region.ReportForeignDHCPServer) |
1080 | return protocol, connecting |
1081 | |
1082 | - def make_cluster_interface_values(self, ip=None): |
1083 | - """Return a dict describing a cluster interface.""" |
1084 | - if ip is None: |
1085 | - ip = factory.make_ipv4_address() |
1086 | - return { |
1087 | - 'name': factory.make_name('interface'), |
1088 | - 'interface': factory.make_name('eth'), |
1089 | - 'ip': ip, |
1090 | - } |
1091 | - |
1092 | def test_is_called_every_interval(self): |
1093 | clock = Clock() |
1094 | service = DHCPProbeService( |
1095 | @@ -82,67 +74,47 @@ |
1096 | |
1097 | def test_probe_is_initiated_in_new_thread(self): |
1098 | clock = Clock() |
1099 | - interface = self.make_cluster_interface_values() |
1100 | - rpc_service = Mock() |
1101 | - rpc_client = rpc_service.getClient.return_value |
1102 | - rpc_client.side_effect = [ |
1103 | - defer.succeed(dict(interfaces=[interface])), |
1104 | - ] |
1105 | + interface_name = factory.make_name("eth") |
1106 | + interfaces = { |
1107 | + interface_name: { |
1108 | + "enabled": True, |
1109 | + } |
1110 | + } |
1111 | |
1112 | - # We could patch out 'periodic_probe_task' instead here but this |
1113 | - # is better because: |
1114 | - # 1. The former requires spinning the reactor again before being |
1115 | - # able to test the result. |
1116 | - # 2. This way there's no thread to clean up after the test. |
1117 | deferToThread = self.patch(dhcp_probe_service, 'deferToThread') |
1118 | - deferToThread.return_value = defer.succeed(None) |
1119 | - service = DHCPProbeService( |
1120 | - rpc_service, clock) |
1121 | + deferToThread.side_effect = [ |
1122 | + defer.succeed(interfaces), |
1123 | + defer.succeed(None), |
1124 | + ] |
1125 | + service = DHCPProbeService(Mock(), clock) |
1126 | service.startService() |
1127 | self.assertThat( |
1128 | - deferToThread, MockCalledOnceWith( |
1129 | - dhcp_probe_service.probe_interface, |
1130 | - interface['interface'], interface['ip'])) |
1131 | - |
1132 | - @defer.inlineCallbacks |
1133 | - def test_exits_gracefully_if_cant_get_interfaces(self): |
1134 | - clock = Clock() |
1135 | - maaslog = self.patch(dhcp_probe_service, 'maaslog') |
1136 | - |
1137 | - protocol, connecting = self.patch_rpc_methods() |
1138 | - self.addCleanup((yield connecting)) |
1139 | - |
1140 | - del protocol._commandDispatch[ |
1141 | - region.GetClusterInterfaces.commandName] |
1142 | - rpc_service = Mock() |
1143 | - rpc_service.getClient.return_value = getRegionClient() |
1144 | - service = DHCPProbeService( |
1145 | - rpc_service, clock) |
1146 | - yield service.startService() |
1147 | - yield service.stopService() |
1148 | - |
1149 | - self.assertThat( |
1150 | - maaslog.error, MockCalledOnceWith( |
1151 | - "Unable to query region for interfaces: Region does not " |
1152 | - "support the GetClusterInterfaces RPC method.")) |
1153 | + deferToThread, MockCallsMatch( |
1154 | + call(dhcp_probe_service.get_all_interfaces_definition), |
1155 | + call(dhcp_probe_service.probe_interface, interface_name))) |
1156 | |
1157 | @defer.inlineCallbacks |
1158 | def test_exits_gracefully_if_cant_report_foreign_dhcp_server(self): |
1159 | clock = Clock() |
1160 | + interface_name = factory.make_name("eth") |
1161 | + interfaces = { |
1162 | + interface_name: { |
1163 | + "enabled": True, |
1164 | + } |
1165 | + } |
1166 | + |
1167 | maaslog = self.patch(dhcp_probe_service, 'maaslog') |
1168 | deferToThread = self.patch( |
1169 | dhcp_probe_service, 'deferToThread') |
1170 | - deferToThread.return_value = defer.succeed(['192.168.0.100']) |
1171 | + deferToThread.side_effect = [ |
1172 | + defer.succeed(interfaces), |
1173 | + defer.succeed(['192.168.0.100']), |
1174 | + ] |
1175 | protocol, connecting = self.patch_rpc_methods() |
1176 | self.addCleanup((yield connecting)) |
1177 | |
1178 | del protocol._commandDispatch[ |
1179 | region.ReportForeignDHCPServer.commandName] |
1180 | - protocol.GetClusterInterfaces.return_value = { |
1181 | - 'interfaces': [ |
1182 | - self.make_cluster_interface_values(ip='192.168.0.1'), |
1183 | - ], |
1184 | - } |
1185 | |
1186 | rpc_service = Mock() |
1187 | rpc_service.getClient.return_value = getRegionClient() |
1188 | @@ -153,13 +125,23 @@ |
1189 | |
1190 | self.assertThat( |
1191 | maaslog.error, MockCalledOnceWith( |
1192 | - "Unable to inform region of rogue DHCP server: the region " |
1193 | + "Unable to inform region of DHCP server: the region " |
1194 | "does not yet support the ReportForeignDHCPServer RPC " |
1195 | "method.")) |
1196 | |
1197 | def test_logs_errors(self): |
1198 | clock = Clock() |
1199 | + interface_name = factory.make_name("eth") |
1200 | + interfaces = { |
1201 | + interface_name: { |
1202 | + "enabled": True, |
1203 | + } |
1204 | + } |
1205 | + |
1206 | maaslog = self.patch(dhcp_probe_service, 'maaslog') |
1207 | + mock_interfaces = self.patch( |
1208 | + dhcp_probe_service, 'get_all_interfaces_definition') |
1209 | + mock_interfaces.return_value = interfaces |
1210 | service = DHCPProbeService( |
1211 | sentinel.service, clock) |
1212 | error_message = factory.make_string() |
1213 | @@ -168,25 +150,29 @@ |
1214 | service.startService() |
1215 | self.assertThat( |
1216 | maaslog.error, MockCalledOnceWith( |
1217 | - "Unable to probe for rogue DHCP servers: %s", |
1218 | + "Unable to probe for DHCP servers: %s", |
1219 | error_message)) |
1220 | |
1221 | @defer.inlineCallbacks |
1222 | def test_reports_foreign_dhcp_servers_to_region(self): |
1223 | clock = Clock() |
1224 | + interface_name = factory.make_name("eth") |
1225 | + interfaces = { |
1226 | + interface_name: { |
1227 | + "enabled": True, |
1228 | + } |
1229 | + } |
1230 | + |
1231 | protocol, connecting = self.patch_rpc_methods() |
1232 | self.addCleanup((yield connecting)) |
1233 | |
1234 | deferToThread = self.patch( |
1235 | dhcp_probe_service, 'deferToThread') |
1236 | foreign_dhcp_ip = factory.make_ipv4_address() |
1237 | - deferToThread.return_value = defer.succeed( |
1238 | - [foreign_dhcp_ip]) |
1239 | - |
1240 | - interface = self.make_cluster_interface_values() |
1241 | - protocol.GetClusterInterfaces.return_value = { |
1242 | - 'interfaces': [interface], |
1243 | - } |
1244 | + deferToThread.side_effect = [ |
1245 | + defer.succeed(interfaces), |
1246 | + defer.succeed([foreign_dhcp_ip]), |
1247 | + ] |
1248 | |
1249 | client = getRegionClient() |
1250 | rpc_service = Mock() |
1251 | @@ -201,24 +187,29 @@ |
1252 | protocol.ReportForeignDHCPServer, |
1253 | MockCalledOnceWith( |
1254 | protocol, |
1255 | - cluster_uuid=client.localIdent, |
1256 | - interface_name=interface['name'], |
1257 | - foreign_dhcp_ip=foreign_dhcp_ip)) |
1258 | + system_id=client.localIdent, |
1259 | + interface_name=interface_name, |
1260 | + dhcp_ip=foreign_dhcp_ip)) |
1261 | |
1262 | @defer.inlineCallbacks |
1263 | def test_reports_lack_of_foreign_dhcp_servers_to_region(self): |
1264 | clock = Clock() |
1265 | + interface_name = factory.make_name("eth") |
1266 | + interfaces = { |
1267 | + interface_name: { |
1268 | + "enabled": True, |
1269 | + } |
1270 | + } |
1271 | + |
1272 | protocol, connecting = self.patch_rpc_methods() |
1273 | self.addCleanup((yield connecting)) |
1274 | |
1275 | deferToThread = self.patch( |
1276 | dhcp_probe_service, 'deferToThread') |
1277 | - deferToThread.return_value = defer.succeed([]) |
1278 | - |
1279 | - interface = self.make_cluster_interface_values() |
1280 | - protocol.GetClusterInterfaces.return_value = { |
1281 | - 'interfaces': [interface], |
1282 | - } |
1283 | + deferToThread.side_effect = [ |
1284 | + defer.succeed(interfaces), |
1285 | + defer.succeed([]), |
1286 | + ] |
1287 | |
1288 | client = getRegionClient() |
1289 | rpc_service = Mock() |
1290 | @@ -232,6 +223,6 @@ |
1291 | protocol.ReportForeignDHCPServer, |
1292 | MockCalledOnceWith( |
1293 | protocol, |
1294 | - cluster_uuid=client.localIdent, |
1295 | - interface_name=interface['name'], |
1296 | - foreign_dhcp_ip=None)) |
1297 | + system_id=client.localIdent, |
1298 | + interface_name=interface_name, |
1299 | + dhcp_ip=None)) |
1300 | |
1301 | === modified file 'src/provisioningserver/rpc/region.py' |
1302 | --- src/provisioningserver/rpc/region.py 2016-03-28 13:54:47 +0000 |
1303 | +++ src/provisioningserver/rpc/region.py 2016-04-07 13:22:34 +0000 |
1304 | @@ -14,7 +14,6 @@ |
1305 | "GetBootConfig", |
1306 | "GetBootSources", |
1307 | "GetBootSourcesV2", |
1308 | - "GetClusterInterfaces", |
1309 | "GetProxies", |
1310 | "Identify", |
1311 | "ListNodePowerParameters", |
1312 | @@ -337,38 +336,20 @@ |
1313 | |
1314 | |
1315 | class ReportForeignDHCPServer(amp.Command): |
1316 | - """Report a foreign DHCP server on the cluster's network. |
1317 | + """Report a foreign DHCP server on a rack controller's interface. |
1318 | |
1319 | - :since: 1.7 |
1320 | + :since: 2.0 |
1321 | """ |
1322 | |
1323 | arguments = [ |
1324 | - (b"cluster_uuid", amp.Unicode()), |
1325 | + (b"system_id", amp.Unicode()), |
1326 | (b"interface_name", amp.Unicode()), |
1327 | - (b"foreign_dhcp_ip", amp.Unicode(optional=True)), |
1328 | + (b"dhcp_ip", amp.Unicode(optional=True)), |
1329 | ] |
1330 | response = [] |
1331 | errors = [] |
1332 | |
1333 | |
1334 | -class GetClusterInterfaces(amp.Command): |
1335 | - """Fetch the known interfaces for a cluster from the region. |
1336 | - |
1337 | - :since: 1.7 |
1338 | - """ |
1339 | - |
1340 | - arguments = [ |
1341 | - (b"cluster_uuid", amp.Unicode()), |
1342 | - ] |
1343 | - response = [ |
1344 | - (b"interfaces", AmpList( |
1345 | - [(b"name", amp.Unicode()), |
1346 | - (b"interface", amp.Unicode()), |
1347 | - (b"ip", amp.Unicode())])) |
1348 | - ] |
1349 | - errors = [] |
1350 | - |
1351 | - |
1352 | class CreateNode(amp.Command): |
1353 | """Create a node on a given cluster. |
1354 |
This looks like a welcome simplification. What's made it possible now?
How is rackd given permission to access the necessary port?
I think you can also remove p.dhcp. detect. process_ request( ) and its imports.
Good stuff though.