Merge lp:~blake-rouse/maas/boot-images-rpc-v2 into lp:~maas-committers/maas/trunk
- boot-images-rpc-v2
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Blake Rouse |
Approved revision: | no longer in the source branch. |
Merged at revision: | 4151 |
Proposed branch: | lp:~blake-rouse/maas/boot-images-rpc-v2 |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
442 lines (+222/-36) 8 files modified
src/maasserver/bootresources.py (+13/-2) src/maasserver/clusterrpc/boot_images.py (+25/-5) src/maasserver/clusterrpc/tests/test_boot_images.py (+75/-1) src/maasserver/tests/test_bootresources.py (+65/-23) src/provisioningserver/rpc/cluster.py (+24/-0) src/provisioningserver/rpc/clusterservice.py (+9/-0) src/provisioningserver/rpc/tests/test_clusterservice.py (+9/-4) src/provisioningserver/rpc/tests/test_docs.py (+2/-1) |
To merge this branch: | bzr merge lp:~blake-rouse/maas/boot-images-rpc-v2 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+266728@code.launchpad.net |
Commit message
Add ListBootImagesV2 RPC command. Fallback to using ListBootImages RPC when the ListBootImagesV2 is not handled on the cluster.
The signature of the ListBootImages RPC call was changed to support more data in the response. This change will cause connected clusters that are not updated the same time as the region to stop communicating and error. This allows both methods to exist on the cluster where V2 will be called first then fallback to the older call in error.
Description of the change
Blake Rouse (blake-rouse) wrote : | # |
Thanks for the quick review. Fixed all your points! Will backport this all the way to 1.7 so the previous backport wont affect others.
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~blake-rouse/maas/boot-images-rpc-v2 into lp:maas failed. Below is the output from the failed tests.
Ign http://
Ign http://
Hit http://
Ign http://
Hit http://
Hit http://
Get:1 http://
Hit http://
Get:2 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Hit http://
Hit http://
Ign http://
Ign http://
Fetched 1,322 kB in 3s (411 kB/s)
Reading package lists...
sudo DEBIAN_
--
Preview Diff
1 | === modified file 'src/maasserver/bootresources.py' |
2 | --- src/maasserver/bootresources.py 2015-07-08 20:47:12 +0000 |
3 | +++ src/maasserver/bootresources.py 2015-08-03 16:36:18 +0000 |
4 | @@ -86,7 +86,10 @@ |
5 | from provisioningserver.import_images.keyrings import write_all_keyrings |
6 | from provisioningserver.import_images.product_mapping import map_products |
7 | from provisioningserver.logger import get_maas_logger |
8 | -from provisioningserver.rpc.cluster import ListBootImages |
9 | +from provisioningserver.rpc.cluster import ( |
10 | + ListBootImages, |
11 | + ListBootImagesV2, |
12 | +) |
13 | from provisioningserver.utils.fs import tempdir |
14 | from provisioningserver.utils.shell import ExternalProcessError |
15 | from provisioningserver.utils.twisted import ( |
16 | @@ -103,6 +106,7 @@ |
17 | from twisted.internet import reactor |
18 | from twisted.internet.defer import DeferredList |
19 | from twisted.internet.threads import deferToThread |
20 | +from twisted.protocols.amp import UnhandledCommand |
21 | from twisted.python import log |
22 | |
23 | |
24 | @@ -1122,7 +1126,14 @@ |
25 | clients = getAllClients() |
26 | |
27 | def get_images(client): |
28 | - return client(ListBootImages).addCallback(itemgetter("images")) |
29 | + def fallback_v1(failure): |
30 | + failure.trap(UnhandledCommand) |
31 | + return client(ListBootImages) |
32 | + |
33 | + d = client(ListBootImagesV2) |
34 | + d.addErrback(fallback_v1) |
35 | + d.addCallback(itemgetter("images")) |
36 | + return d |
37 | |
38 | d = DeferredList(imap(get_images, clients), consumeErrors=True) |
39 | |
40 | |
41 | === modified file 'src/maasserver/clusterrpc/boot_images.py' |
42 | --- src/maasserver/clusterrpc/boot_images.py 2015-07-09 09:10:51 +0000 |
43 | +++ src/maasserver/clusterrpc/boot_images.py 2015-08-03 16:36:18 +0000 |
44 | @@ -32,8 +32,10 @@ |
45 | from provisioningserver.rpc.cluster import ( |
46 | IsImportBootImagesRunning, |
47 | ListBootImages, |
48 | + ListBootImagesV2, |
49 | ) |
50 | from provisioningserver.utils.twisted import synchronous |
51 | +from twisted.protocols.amp import UnhandledCommand |
52 | from twisted.python.failure import Failure |
53 | |
54 | |
55 | @@ -91,16 +93,34 @@ |
56 | 30 seconds. |
57 | """ |
58 | client = getClientFor(nodegroup.uuid, timeout=1) |
59 | - call = client(ListBootImages) |
60 | - return call.wait(30).get("images") |
61 | + try: |
62 | + call = client(ListBootImagesV2) |
63 | + return call.wait(30).get("images") |
64 | + except UnhandledCommand: |
65 | + call = client(ListBootImages) |
66 | + return call.wait(30).get("images") |
67 | |
68 | |
69 | @synchronous |
70 | def _get_available_boot_images(): |
71 | """Obtain boot images available on connected clusters.""" |
72 | - listimages = lambda client: partial(client, ListBootImages) |
73 | - responses = async.gather(imap(listimages, getAllClients())) |
74 | - for response in suppress_failures(responses): |
75 | + listimages_v1 = lambda client: partial(client, ListBootImages) |
76 | + listimages_v2 = lambda client: partial(client, ListBootImagesV2) |
77 | + clients_v2 = getAllClients() |
78 | + responses_v2 = async.gather(imap(listimages_v2, clients_v2)) |
79 | + clients_v1 = [] |
80 | + for i, response in enumerate(responses_v2): |
81 | + if (isinstance(response, Failure) and |
82 | + response.check(UnhandledCommand) is not None): |
83 | + clients_v1.append(clients_v2[i]) |
84 | + elif not isinstance(response, Failure): |
85 | + # Convert each image to a frozenset of its items. |
86 | + yield frozenset( |
87 | + frozenset(image.viewitems()) |
88 | + for image in response["images"] |
89 | + ) |
90 | + responses_v1 = async.gather(imap(listimages_v1, clients_v1)) |
91 | + for response in suppress_failures(responses_v1): |
92 | # Convert each image to a frozenset of its items. |
93 | yield frozenset( |
94 | frozenset(image.viewitems()) |
95 | |
96 | === modified file 'src/maasserver/clusterrpc/tests/test_boot_images.py' |
97 | --- src/maasserver/clusterrpc/tests/test_boot_images.py 2015-07-09 09:16:20 +0000 |
98 | +++ src/maasserver/clusterrpc/tests/test_boot_images.py 2015-08-03 16:36:18 +0000 |
99 | @@ -16,6 +16,7 @@ |
100 | |
101 | import os |
102 | |
103 | +from maasserver.clusterrpc import boot_images as boot_images_module |
104 | from maasserver.clusterrpc.boot_images import ( |
105 | get_all_available_boot_images, |
106 | get_boot_images, |
107 | @@ -30,9 +31,24 @@ |
108 | NODEGROUP_STATUS, |
109 | ) |
110 | from maasserver.rpc import getAllClients |
111 | -from maasserver.rpc.testing.fixtures import RunningClusterRPCFixture |
112 | +from maasserver.rpc.testing.fixtures import ( |
113 | + MockLiveRegionToClusterRPCFixture, |
114 | + RunningClusterRPCFixture, |
115 | +) |
116 | +from maasserver.testing.eventloop import ( |
117 | + RegionEventLoopFixture, |
118 | + RunningEventLoopFixture, |
119 | +) |
120 | from maasserver.testing.factory import factory |
121 | from maasserver.testing.testcase import MAASServerTestCase |
122 | +from maastesting.matchers import ( |
123 | + MockCalledOnceWith, |
124 | + MockCallsMatch, |
125 | +) |
126 | +from mock import ( |
127 | + call, |
128 | + MagicMock, |
129 | +) |
130 | from provisioningserver.boot.tests import test_tftppath |
131 | from provisioningserver.boot.tftppath import ( |
132 | compose_image_path, |
133 | @@ -42,12 +58,17 @@ |
134 | boot_images, |
135 | clusterservice, |
136 | ) |
137 | +from provisioningserver.rpc.cluster import ( |
138 | + ListBootImages, |
139 | + ListBootImagesV2, |
140 | +) |
141 | from provisioningserver.testing.boot_images import ( |
142 | make_boot_image_storage_params, |
143 | make_image, |
144 | ) |
145 | from provisioningserver.testing.config import ClusterConfigurationFixture |
146 | from twisted.internet.defer import succeed |
147 | +from twisted.protocols.amp import UnhandledCommand |
148 | |
149 | |
150 | def make_image_dir(image_params, tftp_root): |
151 | @@ -171,6 +192,28 @@ |
152 | ], |
153 | get_boot_images(nodegroup)) |
154 | |
155 | + def test_calls_ListBootImagesV2_before_ListBootImages(self): |
156 | + nodegroup = factory.make_NodeGroup(status=NODEGROUP_STATUS.ENABLED) |
157 | + mock_client = MagicMock() |
158 | + self.patch_autospec( |
159 | + boot_images_module, "getClientFor").return_value = mock_client |
160 | + get_boot_images(nodegroup) |
161 | + self.assertThat(mock_client, MockCalledOnceWith(ListBootImagesV2)) |
162 | + |
163 | + def test_calls_ListBootImages_if_raised_UnhandledCommand(self): |
164 | + nodegroup = factory.make_NodeGroup(status=NODEGROUP_STATUS.ENABLED) |
165 | + mock_client = MagicMock() |
166 | + self.patch_autospec( |
167 | + boot_images_module, "getClientFor").return_value = mock_client |
168 | + mock_client.return_value.wait.side_effect = [ |
169 | + UnhandledCommand(), |
170 | + {"images": []}, |
171 | + ] |
172 | + get_boot_images(nodegroup) |
173 | + self.assertThat(mock_client, MockCallsMatch( |
174 | + call(ListBootImagesV2), |
175 | + call(ListBootImages))) |
176 | + |
177 | |
178 | class TestGetAvailableBootImages(MAASServerTestCase): |
179 | """Tests for `get_common_available_boot_images` and |
180 | @@ -251,6 +294,37 @@ |
181 | |
182 | self.assertItemsEqual(images, self.get()) |
183 | |
184 | + def test_fallback_to_ListBootImages_on_old_clusters(self): |
185 | + nodegroup_1 = factory.make_NodeGroup() |
186 | + nodegroup_1.accept() |
187 | + nodegroup_2 = factory.make_NodeGroup() |
188 | + nodegroup_2.accept() |
189 | + nodegroup_3 = factory.make_NodeGroup() |
190 | + nodegroup_3.accept() |
191 | + |
192 | + images = [make_rpc_boot_image() for _ in range(3)] |
193 | + |
194 | + # Limit the region's event loop to only the "rpc" service. |
195 | + self.useFixture(RegionEventLoopFixture("rpc")) |
196 | + # Now start the region's event loop. |
197 | + self.useFixture(RunningEventLoopFixture()) |
198 | + # This fixture allows us to simulate mock clusters. |
199 | + rpc = self.useFixture(MockLiveRegionToClusterRPCFixture()) |
200 | + |
201 | + # This simulates an older cluster, one without ListBootImagesV2. |
202 | + cluster_1 = rpc.makeCluster(nodegroup_1, ListBootImages) |
203 | + cluster_1.ListBootImages.return_value = succeed({'images': images}) |
204 | + |
205 | + # This simulates a newer cluster, one with ListBootImagesV2. |
206 | + cluster_2 = rpc.makeCluster(nodegroup_2, ListBootImagesV2) |
207 | + cluster_2.ListBootImagesV2.return_value = succeed({'images': images}) |
208 | + |
209 | + # This simulates a broken cluster. |
210 | + cluster_3 = rpc.makeCluster(nodegroup_3, ListBootImagesV2) |
211 | + cluster_3.ListBootImagesV2.side_effect = ZeroDivisionError |
212 | + |
213 | + self.assertItemsEqual(images, self.get()) |
214 | + |
215 | def test_returns_empty_list_when_all_clusters_fail(self): |
216 | factory.make_NodeGroup().accept() |
217 | factory.make_NodeGroup().accept() |
218 | |
219 | === modified file 'src/maasserver/tests/test_bootresources.py' |
220 | --- src/maasserver/tests/test_bootresources.py 2015-07-10 10:55:53 +0000 |
221 | +++ src/maasserver/tests/test_bootresources.py 2015-08-03 16:36:18 +0000 |
222 | @@ -85,7 +85,10 @@ |
223 | ) |
224 | from provisioningserver.auth import get_maas_user_gpghome |
225 | from provisioningserver.import_images.product_mapping import ProductMapping |
226 | -from provisioningserver.rpc.cluster import ListBootImages |
227 | +from provisioningserver.rpc.cluster import ( |
228 | + ListBootImages, |
229 | + ListBootImagesV2, |
230 | +) |
231 | from provisioningserver.utils.text import normalise_whitespace |
232 | from provisioningserver.utils.twisted import asynchronous |
233 | from testtools.deferredruntest import extract_result |
234 | @@ -100,6 +103,7 @@ |
235 | fail, |
236 | succeed, |
237 | ) |
238 | +from twisted.protocols.amp import UnhandledCommand |
239 | |
240 | |
241 | def make_boot_resource_file_with_stream(): |
242 | @@ -1460,28 +1464,66 @@ |
243 | factory.make_BootResource() |
244 | self.assertTrue(service.are_boot_images_available_in_the_region()) |
245 | |
246 | - def test__are_boot_images_available_in_any_cluster(self): |
247 | - # Import the websocket handlers now: merely defining DeviceHandler, |
248 | - # e.g., causes a database access, which will crash if it happens |
249 | - # inside the reactor thread where database access is forbidden and |
250 | - # prevented. My own opinion is that a class definition should not |
251 | - # cause a database access and we ought to fix that. |
252 | - import maasserver.websockets.handlers # noqa |
253 | - |
254 | - cluster = factory.make_NodeGroup() |
255 | - service = bootresources.ImportResourcesProgressService() |
256 | - |
257 | - self.useFixture(RegionEventLoopFixture("rpc")) |
258 | - self.useFixture(RunningEventLoopFixture()) |
259 | - region_rpc = MockLiveRegionToClusterRPCFixture() |
260 | - self.useFixture(region_rpc) |
261 | - |
262 | - # are_boot_images_available_in_the_region() returns False when there |
263 | - # are no clusters connected. |
264 | - self.assertFalse(service.are_boot_images_available_in_any_cluster()) |
265 | - |
266 | - # Connect a cluster to the region via RPC. |
267 | - cluster_rpc = region_rpc.makeCluster(cluster, ListBootImages) |
268 | + def test__are_boot_images_available_in_any_cluster_v2(self): |
269 | + # Import the websocket handlers now: merely defining DeviceHandler, |
270 | + # e.g., causes a database access, which will crash if it happens |
271 | + # inside the reactor thread where database access is forbidden and |
272 | + # prevented. My own opinion is that a class definition should not |
273 | + # cause a database access and we ought to fix that. |
274 | + import maasserver.websockets.handlers # noqa |
275 | + |
276 | + cluster = factory.make_NodeGroup() |
277 | + service = bootresources.ImportResourcesProgressService() |
278 | + |
279 | + self.useFixture(RegionEventLoopFixture("rpc")) |
280 | + self.useFixture(RunningEventLoopFixture()) |
281 | + region_rpc = MockLiveRegionToClusterRPCFixture() |
282 | + self.useFixture(region_rpc) |
283 | + |
284 | + # are_boot_images_available_in_the_region() returns False when there |
285 | + # are no clusters connected. |
286 | + self.assertFalse(service.are_boot_images_available_in_any_cluster()) |
287 | + |
288 | + # Connect a cluster to the region via RPC. |
289 | + cluster_rpc = region_rpc.makeCluster(cluster, ListBootImagesV2) |
290 | + |
291 | + # are_boot_images_available_in_the_region() returns False when none of |
292 | + # the clusters have any images. |
293 | + cluster_rpc.ListBootImagesV2.return_value = succeed({"images": []}) |
294 | + self.assertFalse(service.are_boot_images_available_in_any_cluster()) |
295 | + |
296 | + # are_boot_images_available_in_the_region() returns True when a |
297 | + # cluster has an imported boot image. |
298 | + response = {"images": [make_rpc_boot_image()]} |
299 | + cluster_rpc.ListBootImagesV2.return_value = succeed(response) |
300 | + self.assertTrue(service.are_boot_images_available_in_any_cluster()) |
301 | + |
302 | + def test__are_boot_images_available_in_any_cluster_v1(self): |
303 | + # Import the websocket handlers now: merely defining DeviceHandler, |
304 | + # e.g., causes a database access, which will crash if it happens |
305 | + # inside the reactor thread where database access is forbidden and |
306 | + # prevented. My own opinion is that a class definition should not |
307 | + # cause a database access and we ought to fix that. |
308 | + import maasserver.websockets.handlers # noqa |
309 | + |
310 | + cluster = factory.make_NodeGroup() |
311 | + service = bootresources.ImportResourcesProgressService() |
312 | + |
313 | + self.useFixture(RegionEventLoopFixture("rpc")) |
314 | + self.useFixture(RunningEventLoopFixture()) |
315 | + region_rpc = MockLiveRegionToClusterRPCFixture() |
316 | + self.useFixture(region_rpc) |
317 | + |
318 | + # are_boot_images_available_in_the_region() returns False when there |
319 | + # are no clusters connected. |
320 | + self.assertFalse(service.are_boot_images_available_in_any_cluster()) |
321 | + |
322 | + # Connect a cluster to the region via RPC. |
323 | + cluster_rpc = region_rpc.makeCluster( |
324 | + cluster, ListBootImagesV2, ListBootImages) |
325 | + |
326 | + # All calls to ListBootImagesV2 raises a UnhandledCommand. |
327 | + cluster_rpc.ListBootImagesV2.side_effect = UnhandledCommand |
328 | |
329 | # are_boot_images_available_in_the_region() returns False when none of |
330 | # the clusters have any images. |
331 | |
332 | === modified file 'src/provisioningserver/rpc/cluster.py' |
333 | --- src/provisioningserver/rpc/cluster.py 2015-07-31 20:07:44 +0000 |
334 | +++ src/provisioningserver/rpc/cluster.py 2015-08-03 16:36:18 +0000 |
335 | @@ -59,6 +59,30 @@ |
336 | |
337 | arguments = [] |
338 | response = [ |
339 | + (b"images", amp.AmpList( |
340 | + [(b"osystem", amp.Unicode()), |
341 | + (b"architecture", amp.Unicode()), |
342 | + (b"subarchitecture", amp.Unicode()), |
343 | + (b"release", amp.Unicode()), |
344 | + (b"label", amp.Unicode()), |
345 | + (b"purpose", amp.Unicode()), |
346 | + (b"xinstall_type", amp.Unicode()), |
347 | + (b"xinstall_path", amp.Unicode())])) |
348 | + ] |
349 | + errors = [] |
350 | + |
351 | + |
352 | +class ListBootImagesV2(amp.Command): |
353 | + """List the boot images available on this cluster controller. |
354 | + |
355 | + This command compresses the images list to allow more images in the |
356 | + response and to remove the amp.TooLong error. |
357 | + |
358 | + :since: 1.7.6 |
359 | + """ |
360 | + |
361 | + arguments = [] |
362 | + response = [ |
363 | (b"images", CompressedAmpList( |
364 | [(b"osystem", amp.Unicode()), |
365 | (b"architecture", amp.Unicode()), |
366 | |
367 | === modified file 'src/provisioningserver/rpc/clusterservice.py' |
368 | --- src/provisioningserver/rpc/clusterservice.py 2015-06-25 22:33:37 +0000 |
369 | +++ src/provisioningserver/rpc/clusterservice.py 2015-08-03 16:36:18 +0000 |
370 | @@ -151,6 +151,15 @@ |
371 | """ |
372 | return {"images": list_boot_images()} |
373 | |
374 | + @cluster.ListBootImagesV2.responder |
375 | + def list_boot_images_v2(self): |
376 | + """list_boot_images_v2() |
377 | + |
378 | + Implementation of |
379 | + :py:class:`~provisioningserver.rpc.cluster.ListBootImagesV2`. |
380 | + """ |
381 | + return {"images": list_boot_images()} |
382 | + |
383 | @cluster.ImportBootImages.responder |
384 | def import_boot_images(self, sources, http_proxy=None, https_proxy=None): |
385 | """import_boot_images() |
386 | |
387 | === modified file 'src/provisioningserver/rpc/tests/test_clusterservice.py' |
388 | --- src/provisioningserver/rpc/tests/test_clusterservice.py 2015-06-26 15:36:26 +0000 |
389 | +++ src/provisioningserver/rpc/tests/test_clusterservice.py 2015-08-03 16:36:18 +0000 |
390 | @@ -215,14 +215,19 @@ |
391 | return d.addCallback(check) |
392 | |
393 | |
394 | -class TestClusterProtocol_ListBootImages(MAASTestCase): |
395 | +class TestClusterProtocol_ListBootImages_and_ListBootImagesV2(MAASTestCase): |
396 | |
397 | run_tests_with = MAASTwistedRunTest.make_factory(timeout=5) |
398 | |
399 | + scenarios = ( |
400 | + ("ListBootImages", {"rpc_call": cluster.ListBootImages}), |
401 | + ("ListBootImagesV2", {"rpc_call": cluster.ListBootImagesV2}), |
402 | + ) |
403 | + |
404 | def test_list_boot_images_is_registered(self): |
405 | protocol = Cluster() |
406 | responder = protocol.locateResponder( |
407 | - cluster.ListBootImages.commandName) |
408 | + self.rpc_call.commandName) |
409 | self.assertIsNotNone(responder) |
410 | |
411 | @inlineCallbacks |
412 | @@ -232,7 +237,7 @@ |
413 | list_boot_images = self.patch(tftppath, "list_boot_images") |
414 | list_boot_images.return_value = [] |
415 | |
416 | - response = yield call_responder(Cluster(), cluster.ListBootImages, {}) |
417 | + response = yield call_responder(Cluster(), self.rpc_call, {}) |
418 | |
419 | self.assertEqual({"images": []}, response) |
420 | |
421 | @@ -282,7 +287,7 @@ |
422 | expected_image['xinstall_path'] = '' |
423 | expected_image['xinstall_type'] = '' |
424 | |
425 | - response = yield call_responder(Cluster(), cluster.ListBootImages, {}) |
426 | + response = yield call_responder(Cluster(), self.rpc_call, {}) |
427 | |
428 | self.assertThat(response, KeysEqual("images")) |
429 | self.assertItemsEqual(expected_images, response["images"]) |
430 | |
431 | === modified file 'src/provisioningserver/rpc/tests/test_docs.py' |
432 | --- src/provisioningserver/rpc/tests/test_docs.py 2015-05-07 18:14:38 +0000 |
433 | +++ src/provisioningserver/rpc/tests/test_docs.py 2015-08-03 16:36:18 +0000 |
434 | @@ -70,6 +70,7 @@ |
435 | self.since_clause_missing_message, Contains(":since:")) |
436 | since_clause_contains_version = Annotate( |
437 | self.since_clause_version_not_recognised, MatchesRegex( |
438 | - ".*^:since: *[1-9][.][0-9]+$", re.DOTALL | re.MULTILINE)) |
439 | + ".*^:since: *[1-9][.][0-9]+([.][0-9]+)?$", |
440 | + re.DOTALL | re.MULTILINE)) |
441 | self.assertThat(getdoc(self.command), MatchesAll( |
442 | contains_since_clause, since_clause_contains_version)) |
Thanks for doing this. I have several suggestions. Please consider those before landing.