Merge lp:~julian-edwards/maas/revert-2819 into lp:~maas-committers/maas/trunk
- revert-2819
- Merge into trunk
Proposed by
Julian Edwards
Status: | Merged |
---|---|
Approved by: | Julian Edwards |
Approved revision: | no longer in the source branch. |
Merged at revision: | 2822 |
Proposed branch: | lp:~julian-edwards/maas/revert-2819 |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
468 lines (+62/-254) 8 files modified
etc/celeryconfig_cluster.py (+8/-0) src/provisioningserver/dhcp/detect.py (+26/-8) src/provisioningserver/dhcp/dhcp_probe_service.py (+0/-85) src/provisioningserver/dhcp/tests/test_detect.py (+13/-4) src/provisioningserver/dhcp/tests/test_dhcp_probe_service.py (+0/-128) src/provisioningserver/plugin.py (+0/-12) src/provisioningserver/tasks.py (+12/-1) src/provisioningserver/tests/test_plugin.py (+3/-16) |
To merge this branch: | bzr merge lp:~julian-edwards/maas/revert-2819 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Julian Edwards (community) | selfie | Approve | |
Review via email:
|
Commit message
Revert r2819 (pserv-based dhcp probing service), which could never work as it was using API credentials that are not sent to pserv.
Description of the change
To post a comment you must log in.
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Julian Edwards (julian-edwards) : | # |
review:
Approve
(selfie)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'etc/celeryconfig_cluster.py' |
2 | --- etc/celeryconfig_cluster.py 2014-08-22 14:47:15 +0000 |
3 | +++ etc/celeryconfig_cluster.py 2014-08-26 23:57:04 +0000 |
4 | @@ -56,4 +56,12 @@ |
5 | 'expires': int(REPORT_BOOT_IMAGES_SCHEDULE.total_seconds()), |
6 | }, |
7 | }, |
8 | + 'probe-dhcp-servers': { |
9 | + 'task': 'provisioningserver.tasks.periodic_probe_dhcp', |
10 | + 'schedule': PROBE_DHCP_SERVERS_SCHEDULE, |
11 | + 'options': { |
12 | + 'queue': CLUSTER_UUID, |
13 | + 'expires': int(PROBE_DHCP_SERVERS_SCHEDULE.total_seconds()), |
14 | + }, |
15 | + }, |
16 | } |
17 | |
18 | === modified file 'src/provisioningserver/dhcp/detect.py' |
19 | --- src/provisioningserver/dhcp/detect.py 2014-08-26 10:19:58 +0000 |
20 | +++ src/provisioningserver/dhcp/detect.py 2014-08-26 23:57:04 +0000 |
21 | @@ -33,6 +33,11 @@ |
22 | MAASDispatcher, |
23 | MAASOAuth, |
24 | ) |
25 | +from provisioningserver.auth import ( |
26 | + get_recorded_api_credentials, |
27 | + get_recorded_nodegroup_uuid, |
28 | + ) |
29 | +from provisioningserver.cluster_config import get_maas_url |
30 | from provisioningserver.logger import get_maas_logger |
31 | |
32 | |
33 | @@ -316,7 +321,7 @@ |
34 | client.post, api_path, 'report_foreign_dhcp', foreign_dhcp_ip=server) |
35 | |
36 | |
37 | -def periodic_probe_task(api_knowledge): |
38 | +def periodic_probe_task(): |
39 | """Probe for DHCP servers and set NodeGroupInterface.foriegn_dhcp. |
40 | |
41 | This should be run periodically so that the database has an up-to-date |
42 | @@ -325,12 +330,26 @@ |
43 | NOTE: This uses blocking I/O with sequential polling of interfaces, and |
44 | hence doesn't scale well. It's a future improvement to make |
45 | to throw it in parallel threads or async I/O. |
46 | - |
47 | - :param api_knowledge: A dict of the information needed to be able to |
48 | - make requests to the region's REST API. |
49 | """ |
50 | + # Items that the server must have sent us before we can do this. |
51 | + knowledge = { |
52 | + 'maas_url': get_maas_url(), |
53 | + 'api_credentials': get_recorded_api_credentials(), |
54 | + 'nodegroup_uuid': get_recorded_nodegroup_uuid(), |
55 | + } |
56 | + |
57 | + if None in knowledge.values(): |
58 | + # The MAAS server hasn't sent us enough information for us to do |
59 | + # this yet. Leave it for another time. |
60 | + maaslog.info( |
61 | + "Not probing for rogue DHCP servers; not all required knowledge " |
62 | + "received from server yet. " |
63 | + "Missing: %s" % ', '.join(sorted( |
64 | + name for name, value in knowledge.items() if value is None))) |
65 | + return |
66 | + |
67 | # Determine all the active interfaces on this cluster (nodegroup). |
68 | - interfaces = determine_cluster_interfaces(api_knowledge) |
69 | + interfaces = determine_cluster_interfaces(knowledge) |
70 | if interfaces is None: |
71 | maaslog.info("No interfaces on cluster, not probing DHCP.") |
72 | return |
73 | @@ -349,7 +368,6 @@ |
74 | # Only send one, if it gets cleared out then the |
75 | # next detection pass will send a different one, if it |
76 | # still exists. |
77 | - update_region_controller( |
78 | - api_knowledge, interface, servers.pop()) |
79 | + update_region_controller(knowledge, interface, servers.pop()) |
80 | else: |
81 | - update_region_controller(api_knowledge, interface, None) |
82 | + update_region_controller(knowledge, interface, None) |
83 | |
84 | === removed file 'src/provisioningserver/dhcp/dhcp_probe_service.py' |
85 | --- src/provisioningserver/dhcp/dhcp_probe_service.py 2014-08-26 15:47:52 +0000 |
86 | +++ src/provisioningserver/dhcp/dhcp_probe_service.py 1970-01-01 00:00:00 +0000 |
87 | @@ -1,85 +0,0 @@ |
88 | -# Copyright 2014 Canonical Ltd. This software is licensed under the |
89 | -# GNU Affero General Public License version 3 (see the file LICENSE). |
90 | - |
91 | -"""Periodic DHCP probing service.""" |
92 | - |
93 | -from __future__ import ( |
94 | - absolute_import, |
95 | - print_function, |
96 | - unicode_literals, |
97 | - ) |
98 | - |
99 | -str = None |
100 | - |
101 | -__metaclass__ = type |
102 | -__all__ = [ |
103 | - "PeriodicDHCPProbeService", |
104 | - ] |
105 | - |
106 | - |
107 | -from datetime import timedelta |
108 | - |
109 | -from provisioningserver.auth import get_recorded_api_credentials |
110 | -from provisioningserver.cluster_config import get_maas_url |
111 | -from provisioningserver.dhcp import detect |
112 | -from provisioningserver.logger.log import get_maas_logger |
113 | -from twisted.application.internet import TimerService |
114 | -from twisted.internet.defer import inlineCallbacks |
115 | -from twisted.internet.threads import deferToThread |
116 | - |
117 | - |
118 | -maaslog = get_maas_logger("dhcp.probe") |
119 | - |
120 | - |
121 | -class PeriodicDHCPProbeService(TimerService, object): |
122 | - """Service to probe for DHCP servers on this cluster's network. |
123 | - |
124 | - Built on top of Twisted's `TimerService`. |
125 | - |
126 | - :param reactor: An `IReactor` instance. |
127 | - :param cluster_uuid: This cluster's UUID. |
128 | - """ |
129 | - |
130 | - check_interval = timedelta(minutes=1).total_seconds() |
131 | - |
132 | - def __init__(self, reactor, cluster_uuid): |
133 | - # Call self.check() every self.check_interval. |
134 | - super(PeriodicDHCPProbeService, self).__init__( |
135 | - self.check_interval, self.try_probe_dhcp) |
136 | - self.clock = reactor |
137 | - self.uuid = cluster_uuid |
138 | - |
139 | - def _probe_dhcp(self, knowledge): |
140 | - """Probe for DHCP servers.""" |
141 | - return deferToThread(detect.periodic_probe_task, knowledge) |
142 | - |
143 | - @inlineCallbacks |
144 | - def try_probe_dhcp(self): |
145 | - # Items that the server must have sent us before we can do |
146 | - # this. |
147 | - knowledge = { |
148 | - 'api_credentials': get_recorded_api_credentials(), |
149 | - 'maas_url': get_maas_url(), |
150 | - 'nodegroup_uuid': self.uuid, |
151 | - } |
152 | - |
153 | - if None in knowledge.values(): |
154 | - # The MAAS server hasn't sent us enough information for |
155 | - # us to do this yet. Leave it for another time. |
156 | - maaslog.info( |
157 | - "Not probing for rogue DHCP servers; not all " |
158 | - "required knowledge received from server yet. " |
159 | - "Missing: %s" % ', '.join(sorted( |
160 | - name for name, value in knowledge.items() |
161 | - if value is None))) |
162 | - return |
163 | - else: |
164 | - maaslog.debug("Running periodic DHCP probe.") |
165 | - try: |
166 | - d = self._probe_dhcp(knowledge) |
167 | - yield d |
168 | - maaslog.debug("Finished periodic DHCP probe.") |
169 | - except Exception as error: |
170 | - maaslog.error( |
171 | - "Unable to probe for rogue DHCP servers: %s" |
172 | - % unicode(error)) |
173 | |
174 | === modified file 'src/provisioningserver/dhcp/tests/test_detect.py' |
175 | --- src/provisioningserver/dhcp/tests/test_detect.py 2014-08-26 10:19:58 +0000 |
176 | +++ src/provisioningserver/dhcp/tests/test_detect.py 2014-08-26 23:57:04 +0000 |
177 | @@ -30,7 +30,10 @@ |
178 | from maastesting.testcase import MAASTestCase |
179 | import mock |
180 | from provisioningserver import cache |
181 | -from provisioningserver.auth import NODEGROUP_UUID_CACHE_KEY |
182 | +from provisioningserver.auth import ( |
183 | + NODEGROUP_UUID_CACHE_KEY, |
184 | + record_api_credentials, |
185 | + ) |
186 | from provisioningserver.dhcp.detect import ( |
187 | BOOTP_CLIENT_PORT, |
188 | BOOTP_SERVER_PORT, |
189 | @@ -510,11 +513,17 @@ |
190 | "Failed talking to region controller, it returned:", |
191 | self.maaslog.output) |
192 | |
193 | + def test_periodic_probe_task_exits_with_not_enough_knowledge(self): |
194 | + mocked = self.patch(detect_module, 'determine_cluster_interfaces') |
195 | + record_api_credentials(None) |
196 | + periodic_probe_task() |
197 | + self.assertFalse(mocked.called) |
198 | + |
199 | def test_periodic_probe_task_exits_if_no_interfaces(self): |
200 | mocked = self.patch(detect_module, 'probe_interface') |
201 | self.patch( |
202 | detect_module, 'determine_cluster_interfaces').return_value = None |
203 | - periodic_probe_task(self.knowledge) |
204 | + periodic_probe_task() |
205 | self.assertFalse(mocked.called) |
206 | |
207 | def test_periodic_probe_task_updates_region_with_detected_server(self): |
208 | @@ -526,7 +535,7 @@ |
209 | self.patch( |
210 | detect_module, 'probe_dhcp').return_value = {detected_server} |
211 | mocked_update = self.patch(detect_module, 'update_region_controller') |
212 | - periodic_probe_task(self.knowledge) |
213 | + periodic_probe_task() |
214 | calls = [ |
215 | mock.call(self.knowledge, 'eth0', detected_server), |
216 | mock.call(self.knowledge, 'wlan0', detected_server), |
217 | @@ -541,7 +550,7 @@ |
218 | self.patch( |
219 | detect_module, 'probe_dhcp').return_value = set() |
220 | mocked_update = self.patch(detect_module, 'update_region_controller') |
221 | - periodic_probe_task(self.knowledge) |
222 | + periodic_probe_task() |
223 | calls = [ |
224 | mock.call(self.knowledge, 'eth0', None), |
225 | mock.call(self.knowledge, 'wlan0', None), |
226 | |
227 | === removed file 'src/provisioningserver/dhcp/tests/test_dhcp_probe_service.py' |
228 | --- src/provisioningserver/dhcp/tests/test_dhcp_probe_service.py 2014-08-26 15:45:21 +0000 |
229 | +++ src/provisioningserver/dhcp/tests/test_dhcp_probe_service.py 1970-01-01 00:00:00 +0000 |
230 | @@ -1,128 +0,0 @@ |
231 | -# Copyright 2014 Canonical Ltd. This software is licensed under the |
232 | -# GNU Affero General Public License version 3 (see the file LICENSE). |
233 | - |
234 | -"""Tests for periodic DHCP prober.""" |
235 | - |
236 | -from __future__ import ( |
237 | - absolute_import, |
238 | - print_function, |
239 | - unicode_literals, |
240 | - ) |
241 | - |
242 | -str = None |
243 | - |
244 | -__metaclass__ = type |
245 | -__all__ = [] |
246 | - |
247 | -import os |
248 | - |
249 | -from apiclient.testing.credentials import make_api_credentials |
250 | -from maastesting.factory import factory |
251 | -from maastesting.matchers import ( |
252 | - get_mock_calls, |
253 | - HasLength, |
254 | - MockCalledOnceWith, |
255 | - MockNotCalled, |
256 | - ) |
257 | -from mock import ANY |
258 | -from provisioningserver import cache |
259 | -from provisioningserver.auth import ( |
260 | - NODEGROUP_UUID_CACHE_KEY, |
261 | - record_api_credentials, |
262 | - ) |
263 | -from provisioningserver.dhcp import dhcp_probe_service |
264 | -from provisioningserver.dhcp.dhcp_probe_service import ( |
265 | - PeriodicDHCPProbeService, |
266 | - ) |
267 | -from provisioningserver.testing.testcase import PservTestCase |
268 | -from testtools.deferredruntest import AsynchronousDeferredRunTest |
269 | -from twisted.internet import defer |
270 | -from twisted.internet.task import Clock |
271 | - |
272 | - |
273 | -class TestDHCPProbeService(PservTestCase): |
274 | - |
275 | - run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5) |
276 | - |
277 | - def setUp(self): |
278 | - super(TestDHCPProbeService, self).setUp() |
279 | - self.cluster_uuid = factory.make_UUID() |
280 | - maas_url = 'http://%s.example.com/%s/' % ( |
281 | - factory.make_name('host'), |
282 | - factory.make_string(), |
283 | - ) |
284 | - api_credentials = make_api_credentials() |
285 | - |
286 | - cache.cache.set(NODEGROUP_UUID_CACHE_KEY, self.cluster_uuid) |
287 | - os.environ["MAAS_URL"] = maas_url |
288 | - cache.cache.set('api_credentials', ':'.join(api_credentials)) |
289 | - |
290 | - self.knowledge = dict( |
291 | - api_credentials=api_credentials, |
292 | - maas_url=maas_url, |
293 | - nodegroup_uuid=self.cluster_uuid) |
294 | - |
295 | - def test_is_called_every_interval(self): |
296 | - clock = Clock() |
297 | - service = PeriodicDHCPProbeService(clock, self.cluster_uuid) |
298 | - |
299 | - # Avoid actually probing |
300 | - _probe_dhcp = self.patch(service, '_probe_dhcp') |
301 | - |
302 | - # Until the service has started, periodic_probe_dhcp() won't |
303 | - # be called. |
304 | - self.assertThat(_probe_dhcp, MockNotCalled()) |
305 | - |
306 | - # The first call is issued at startup. |
307 | - service.startService() |
308 | - self.assertThat(_probe_dhcp, MockCalledOnceWith(ANY)) |
309 | - |
310 | - # Wind clock forward one second less than the desired interval. |
311 | - clock.advance(service.check_interval - 1) |
312 | - |
313 | - # No more periodic calls made. |
314 | - self.assertEqual(1, len(get_mock_calls(_probe_dhcp))) |
315 | - |
316 | - # Wind clock forward one second, past the interval. |
317 | - clock.advance(1) |
318 | - |
319 | - # Now there were two calls. |
320 | - self.assertThat(get_mock_calls(_probe_dhcp), HasLength(2)) |
321 | - |
322 | - def test_download_is_initiated_in_new_thread(self): |
323 | - clock = Clock() |
324 | - |
325 | - # We could patch out 'periodic_probe_task' instead here but this |
326 | - # is better because: |
327 | - # 1. The former requires spinning the reactor again before being |
328 | - # able to test the result. |
329 | - # 2. This way there's no thread to clean up after the test. |
330 | - deferToThread = self.patch(dhcp_probe_service, 'deferToThread') |
331 | - deferToThread.return_value = defer.succeed(None) |
332 | - service = PeriodicDHCPProbeService(clock, self.cluster_uuid) |
333 | - service.startService() |
334 | - self.assertThat( |
335 | - deferToThread, MockCalledOnceWith( |
336 | - dhcp_probe_service.detect.periodic_probe_task, |
337 | - self.knowledge)) |
338 | - |
339 | - def test_no_probe_if_api_credentials_not_set(self): |
340 | - record_api_credentials(None) |
341 | - clock = Clock() |
342 | - service = PeriodicDHCPProbeService(clock, self.cluster_uuid) |
343 | - _probe_dhcp = self.patch(service, '_probe_dhcp') |
344 | - service.startService() |
345 | - self.assertThat(_probe_dhcp, MockNotCalled()) |
346 | - |
347 | - def test_logs_errors(self): |
348 | - clock = Clock() |
349 | - maaslog = self.patch(dhcp_probe_service, 'maaslog') |
350 | - service = PeriodicDHCPProbeService(clock, self.cluster_uuid) |
351 | - error_message = factory.make_string() |
352 | - self.patch(service, '_probe_dhcp').side_effect = Exception( |
353 | - error_message) |
354 | - service.startService() |
355 | - self.assertThat( |
356 | - maaslog.error, MockCalledOnceWith( |
357 | - "Unable to probe for rogue DHCP servers: %s" % |
358 | - error_message)) |
359 | |
360 | === modified file 'src/provisioningserver/plugin.py' |
361 | --- src/provisioningserver/plugin.py 2014-08-26 17:23:16 +0000 |
362 | +++ src/provisioningserver/plugin.py 2014-08-26 23:57:04 +0000 |
363 | @@ -31,9 +31,6 @@ |
364 | import provisioningserver |
365 | from provisioningserver.cluster_config import get_cluster_uuid |
366 | from provisioningserver.config import Config |
367 | -from provisioningserver.dhcp.dhcp_probe_service import ( |
368 | - PeriodicDHCPProbeService, |
369 | - ) |
370 | from provisioningserver.rpc.boot_images import PeriodicImageDownloadService |
371 | from provisioningserver.rpc.clusterservice import ClusterClientService |
372 | from provisioningserver.rpc.power import NodePowerMonitorService |
373 | @@ -242,12 +239,6 @@ |
374 | rpc_service.setName("rpc") |
375 | return rpc_service |
376 | |
377 | - def _makePeriodicDHCPProbeService(self, rpc_service): |
378 | - dhcp_probe_service = PeriodicDHCPProbeService( |
379 | - reactor, get_cluster_uuid()) |
380 | - dhcp_probe_service.setName("dhcp_probe") |
381 | - return dhcp_probe_service |
382 | - |
383 | def makeService(self, options): |
384 | """Construct a service.""" |
385 | services = provisioningserver.services |
386 | @@ -272,7 +263,4 @@ |
387 | rpc_service) |
388 | image_download_service.setServiceParent(services) |
389 | |
390 | - dhcp_probe_service = self._makePeriodicDHCPProbeService(rpc_service) |
391 | - dhcp_probe_service.setServiceParent(services) |
392 | - |
393 | return services |
394 | |
395 | === modified file 'src/provisioningserver/tasks.py' |
396 | --- src/provisioningserver/tasks.py 2014-08-26 10:23:23 +0000 |
397 | +++ src/provisioningserver/tasks.py 2014-08-26 23:57:04 +0000 |
398 | @@ -43,7 +43,10 @@ |
399 | record_api_credentials, |
400 | record_nodegroup_uuid, |
401 | ) |
402 | -from provisioningserver.dhcp import config |
403 | +from provisioningserver.dhcp import ( |
404 | + config, |
405 | + detect, |
406 | + ) |
407 | from provisioningserver.dhcp.control import ( |
408 | restart_dhcpv4, |
409 | stop_dhcpv4, |
410 | @@ -445,6 +448,14 @@ |
411 | raise |
412 | |
413 | |
414 | +@task |
415 | +@log_task_events(level=logging.DEBUG) |
416 | +@log_exception_text |
417 | +def periodic_probe_dhcp(): |
418 | + """Probe for foreign DHCP servers.""" |
419 | + detect.periodic_probe_task() |
420 | + |
421 | + |
422 | # ===================================================================== |
423 | # Boot images-related tasks |
424 | # ===================================================================== |
425 | |
426 | === modified file 'src/provisioningserver/tests/test_plugin.py' |
427 | --- src/provisioningserver/tests/test_plugin.py 2014-08-26 17:23:16 +0000 |
428 | +++ src/provisioningserver/tests/test_plugin.py 2014-08-26 23:57:04 +0000 |
429 | @@ -21,9 +21,6 @@ |
430 | from maastesting.testcase import MAASTestCase |
431 | import provisioningserver |
432 | from provisioningserver import plugin as plugin_module |
433 | -from provisioningserver.dhcp.dhcp_probe_service import ( |
434 | - PeriodicDHCPProbeService, |
435 | - ) |
436 | from provisioningserver.plugin import ( |
437 | Options, |
438 | ProvisioningRealm, |
439 | @@ -112,11 +109,9 @@ |
440 | service_maker = ProvisioningServiceMaker("Harry", "Hill") |
441 | service = service_maker.makeService(options) |
442 | self.assertIsInstance(service, MultiService) |
443 | - expected_services = [ |
444 | - "dhcp_probe", "image_download", "log", "node_monitor", |
445 | - "oops", "rpc", "tftp", |
446 | - ] |
447 | - self.assertItemsEqual(expected_services, service.namedServices) |
448 | + self.assertSequenceEqual( |
449 | + ["image_download", "log", "node_monitor", "oops", "rpc", "tftp"], |
450 | + sorted(service.namedServices)) |
451 | self.assertEqual( |
452 | len(service.namedServices), len(service.services), |
453 | "Not all services are named.") |
454 | @@ -138,14 +133,6 @@ |
455 | node_monitor = service.getServiceNamed("node_monitor") |
456 | self.assertIsInstance(node_monitor, NodePowerMonitorService) |
457 | |
458 | - def test_dhcp_probe_service(self): |
459 | - options = Options() |
460 | - options["config-file"] = self.write_config({}) |
461 | - service_maker = ProvisioningServiceMaker("Spike", "Milligan") |
462 | - service = service_maker.makeService(options) |
463 | - dhcp_probe = service.getServiceNamed("dhcp_probe") |
464 | - self.assertIsInstance(dhcp_probe, PeriodicDHCPProbeService) |
465 | - |
466 | def test_tftp_service(self): |
467 | # A TFTP service is configured and added to the top-level service. |
468 | config = { |