Merge lp:~free.ekanayaka/landscape-client/juju-machine-registration into lp:~landscape/landscape-client/trunk

Proposed by Free Ekanayaka on 2014-08-28
Status: Merged
Approved by: Free Ekanayaka on 2014-09-02
Approved revision: 789
Merged at revision: 784
Proposed branch: lp:~free.ekanayaka/landscape-client/juju-machine-registration
Merge into: lp:~landscape/landscape-client/trunk
Diff against target: 476 lines (+105/-71)
9 files modified
landscape/__init__.py (+3/-1)
landscape/broker/registration.py (+16/-7)
landscape/broker/tests/test_exchange.py (+4/-4)
landscape/broker/tests/test_registration.py (+38/-18)
landscape/broker/tests/test_store.py (+3/-4)
landscape/manager/tests/test_customgraph.py (+8/-10)
landscape/manager/tests/test_shutdownmanager.py (+2/-3)
landscape/message_schemas.py (+29/-21)
landscape/monitor/tests/test_activeprocessinfo.py (+2/-3)
To merge this branch: bzr merge lp:~free.ekanayaka/landscape-client/juju-machine-registration
Reviewer Review Type Date Requested Status
Adam Collard (community) 2014-08-28 Approve on 2014-09-01
Alberto Donato 2014-08-28 Approve on 2014-08-29
Review via email: mp+232531@code.launchpad.net

Commit message

This branch:

- Bumps the server API to 3.3: this will let us introduce a new schema
  for the juju-info registration field, since we have released clients
  out there that could potentially send the old juju-info schema (although
  they'd need to be deployed with an older version of the client charm,
  so it's more a theoretical situation than one that can happen in practice).
  A server branch has been pushed to bump the server to API 3.3. as well.

- Include the machine ID information in the registration message if the server
  is capable of handling it.

Description of the change

This branch:

- Bumps the server API to 3.3: this will let us introduce a new schema for the juju-info registration field, since we have released clients out there that could potentially send the old juju-info schema (although they'd need to be deployed with an older version of the client charm, so it's more a theoretical situation than one that can happen in practice). A server branch has been pushed to bump the server to API 3.3. as well. See:

https://code.launchpad.net/~free.ekanayaka/landscape/handle-machine-id-registration/+merge/232535

- Include the machine ID information in the registration message if the server is capable of handling it.

Just to be clear, after the server-side branch lands too what happen will be that new clients will send a 3.3 registration message with the new juju info schema, which the server knows how to handle. In case old clients send the old juju-info field, the server will just ignore it.

To post a comment you must log in.
Alberto Donato (ack) wrote :

Looks good, +1

A few comments inline.

review: Approve
Free Ekanayaka (free.ekanayaka) wrote :

Thanks Albert, all fixed.

788. By Free Ekanayaka on 2014-08-29

Address review comments

Adam Collard (adam-collard) wrote :

Lint issue:

landscape/monitor/tests/test_activeprocessinfo.py:13: 'SERVER_API' imported but unused

Adam Collard (adam-collard) wrote :

Needs Fixing because there are several changes needed and I'd like to give it another pass.

7 inline comments for your perusal below!

review: Needs Fixing
Free Ekanayaka (free.ekanayaka) wrote :

Thanks Adam, should be all fixed/replied.

789. By Free Ekanayaka on 2014-09-01

Address review comments

Adam Collard (adam-collard) wrote :

LGTM! +1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'landscape/__init__.py'
2--- landscape/__init__.py 2014-07-16 17:15:13 +0000
3+++ landscape/__init__.py 2014-09-01 13:16:19 +0000
4@@ -13,10 +13,12 @@
5 #
6 # Changelog:
7 #
8+# 3.3:
9+# * Add new schema for the "registration" message, providing Juju information
10 # 3.2:
11 # * Add new "eucalyptus-info" and "eucalyptus-info-error" messages.
12 #
13-SERVER_API = "3.2"
14+SERVER_API = "3.3"
15
16 # XXX This is needed for backward compatibility in the server code importing
17 # the API variable. We should eventually replace it in the server code.
18
19=== modified file 'landscape/broker/registration.py'
20--- landscape/broker/registration.py 2014-08-22 14:00:17 +0000
21+++ landscape/broker/registration.py 2014-09-01 13:16:19 +0000
22@@ -16,6 +16,7 @@
23 from landscape.lib.tag import is_valid_tag_list
24 from landscape.lib.network import get_fqdn
25 from landscape.lib.vm_info import get_vm_info, get_container_info
26+from landscape.lib.versioning import is_version_higher
27
28
29 class InvalidCredentialsError(Exception):
30@@ -126,10 +127,10 @@
31
32 def _get_juju_data(self):
33 """Load Juju information."""
34- juju_info = get_juju_info(self._config)[0]
35+ juju_info = get_juju_info(self._config)
36 if juju_info is None:
37 return None
38- self._juju_data = juju_info # A list of dicts
39+ self._juju_data = juju_info # (list of dicts, new juju info struct)
40
41 def _handle_exchange_done(self):
42 """Registered handler for the C{"exchange-done"} event.
43@@ -188,11 +189,19 @@
44 if group:
45 message["access_group"] = group
46
47- if self._juju_data is not None:
48- # For backwards compatibility, set the juju-info to be one of
49- # the juju infos (it used not to be a list).
50- message["juju-info"] = self._juju_data[0]
51- message["juju-info-list"] = self._juju_data
52+ server_api = self._message_store.get_server_api()
53+ # If we have juju data to send and if the server is recent enough to
54+ # know how to handle juju data, then we include it in the registration
55+ # message. We want to trigger the 3.3 server handler because client
56+ # version 14.01 has a different format for the juju-info field,
57+ # so this makes sure that the correct schema is used by the server
58+ # when validating our message.
59+ if self._juju_data and is_version_higher(server_api, "3.3"):
60+ juju_info_list, juju_info = self._juju_data
61+ message["juju-info"] = juju_info
62+ # XXX the juju-info-list field will be dropped once the migration
63+ # to computer-machine association is complete
64+ message["juju-info-list"] = juju_info_list
65
66 # The computer is a normal computer, possibly a container.
67 with_word = "with" if bool(registration_key) else "without"
68
69=== modified file 'landscape/broker/tests/test_exchange.py'
70--- landscape/broker/tests/test_exchange.py 2014-08-22 13:53:51 +0000
71+++ landscape/broker/tests/test_exchange.py 2014-09-01 13:16:19 +0000
72@@ -1,4 +1,4 @@
73-from landscape import SERVER_API, CLIENT_API
74+from landscape import CLIENT_API
75 from landscape.lib.persist import Persist
76 from landscape.lib.hashlib import md5
77 from landscape.schema import Message, Int
78@@ -111,7 +111,7 @@
79 messages = self.transport.payloads[0]["messages"]
80 self.assertEqual(messages, [{"type": "empty",
81 "timestamp": 0,
82- "api": SERVER_API}])
83+ "api": "3.2"}])
84
85 def test_send_urgent(self):
86 """
87@@ -528,8 +528,8 @@
88 payload = self.transport.payloads[-1]
89 self.assertMessages(payload["messages"], [])
90 self.assertEqual(payload.get("client-api"), CLIENT_API)
91- self.assertEqual(payload.get("server-api"), SERVER_API)
92- self.assertEqual(self.transport.message_api, SERVER_API)
93+ self.assertEqual(payload.get("server-api"), "3.2")
94+ self.assertEqual(self.transport.message_api, "3.2")
95
96 self.mstore.add({"type": "a", "api": "1.0"})
97 self.mstore.add({"type": "b", "api": "1.0"})
98
99=== modified file 'landscape/broker/tests/test_registration.py'
100--- landscape/broker/tests/test_registration.py 2014-08-28 07:36:58 +0000
101+++ landscape/broker/tests/test_registration.py 2014-09-01 13:16:19 +0000
102@@ -498,15 +498,36 @@
103 class JujuRegistrationHandlerTest(RegistrationHandlerTestBase):
104
105 juju_contents = json.dumps({"environment-uuid": "DEAD-BEEF",
106+ "machine-id": "1",
107 "unit-name": "service/0",
108 "api-addresses": "10.0.3.1:17070"})
109
110- def test_juju_information_added_when_present(self):
111+ def test_juju_info_added_when_present(self):
112+ """
113+ When information about the Juju environment is found in
114+ the $data_dir/juju-info.d/ directory, it's included in
115+ the registration message.
116+ """
117+ self.mstore.set_accepted_types(["register"])
118+ self.mstore.set_server_api("3.3")
119+ self.config.account_name = "account_name"
120+ self.reactor.fire("run")
121+ self.reactor.fire("pre-exchange")
122+
123+ messages = self.mstore.get_pending_messages()
124+ self.assertEqual(
125+ {"environment-uuid": "DEAD-BEEF",
126+ "machine-id": "1",
127+ "api-addresses": ["10.0.3.1:17070"]},
128+ messages[0]["juju-info"])
129+
130+ def test_juju_info_list_added_when_present(self):
131 """
132 When Juju information is found in $data_dir/juju-info.d/*.json,
133 key parts of it are sent in the registration message.
134 """
135 self.mstore.set_accepted_types(["register"])
136+ self.mstore.set_server_api("3.3")
137 self.config.account_name = "account_name"
138 self.reactor.fire("run")
139 self.reactor.fire("pre-exchange")
140@@ -517,23 +538,6 @@
141 "unit-name": "service/0"}
142 self.assertEqual(expected, messages[0]["juju-info-list"][0])
143
144- def test_juju_info_compatibility_present(self):
145- """
146- When Juju information is found in $data_dir/juju-info.d/*.json,
147- the registration message also contains a "juju-info" key for
148- backwards compatibility with older servers.
149- """
150- self.mstore.set_accepted_types(["register"])
151- self.config.account_name = "account_name"
152- self.reactor.fire("run")
153- self.reactor.fire("pre-exchange")
154-
155- messages = self.mstore.get_pending_messages()
156- expected = {"environment-uuid": "DEAD-BEEF",
157- "api-addresses": ["10.0.3.1:17070"],
158- "unit-name": "service/0"}
159- self.assertEqual(expected, messages[0]["juju-info"])
160-
161 def test_multiple_juju_information_added_when_present(self):
162 """
163 When Juju information is found in $data_dir/juju-info.json,
164@@ -541,6 +545,7 @@
165 """
166 # Write a second file in the config directory
167 contents = json.dumps({"environment-uuid": "DEAD-BEEF",
168+ "machine-id": "1",
169 "unit-name": "service-2/0",
170 "api-addresses": "10.0.3.2:17070",
171 "private-address": "127.0.0.1"})
172@@ -549,6 +554,7 @@
173 dirname=self.config.juju_directory, suffix=".json")
174
175 self.mstore.set_accepted_types(["register"])
176+ self.mstore.set_server_api("3.3")
177 self.config.account_name = "account_name"
178 self.reactor.fire("run")
179 self.reactor.fire("pre-exchange")
180@@ -567,3 +573,17 @@
181 "unit-name": "service-2/0",
182 "private-address": "127.0.0.1"}
183 self.assertIn(expected2, juju_info)
184+
185+ def test_juju_info_skipped_with_old_server(self):
186+ """
187+ If a server doesn't speak at least 3.3, the juju-info field is
188+ isn't included in the message.
189+ """
190+ self.mstore.set_accepted_types(["register"])
191+ self.mstore.set_server_api("3.2")
192+ self.config.account_name = "account_name"
193+ self.reactor.fire("run")
194+ self.reactor.fire("pre-exchange")
195+
196+ messages = self.mstore.get_pending_messages()
197+ self.assertNotIn("juju-info", messages[0])
198
199=== modified file 'landscape/broker/tests/test_store.py'
200--- landscape/broker/tests/test_store.py 2014-08-21 09:43:26 +0000
201+++ landscape/broker/tests/test_store.py 2014-09-01 13:16:19 +0000
202@@ -8,7 +8,6 @@
203
204 from landscape.tests.helpers import LandscapeTest
205 from landscape.tests.mocker import ANY
206-from landscape import SERVER_API
207
208
209 class MessageStoreTest(LandscapeTest):
210@@ -133,7 +132,7 @@
211 self.assertMessages(messages,
212 [{"type": "data",
213 "data": "A thing",
214- "api": SERVER_API}])
215+ "api": "3.2"}])
216
217 def test_max_pending(self):
218 for i in range(10):
219@@ -271,7 +270,7 @@
220 messages = self.store.get_pending_messages()
221
222 self.assertEqual(messages, [{"type": "data", "data": "2",
223- "api": SERVER_API}])
224+ "api": "3.2"}])
225
226 self.store.set_pending_offset(len(messages))
227
228@@ -302,7 +301,7 @@
229 self.mocker.verify()
230 self.mocker.reset()
231 self.assertEqual(self.store.get_pending_messages(),
232- [{"type": "data", "data": 1, "api": SERVER_API}])
233+ [{"type": "data", "data": 1, "api": "3.2"}])
234
235 def test_get_server_api_default(self):
236 """
237
238=== modified file 'landscape/manager/tests/test_customgraph.py'
239--- landscape/manager/tests/test_customgraph.py 2011-07-05 05:09:11 +0000
240+++ landscape/manager/tests/test_customgraph.py 2014-09-01 13:16:19 +0000
241@@ -5,8 +5,6 @@
242 from twisted.internet.error import ProcessDone
243 from twisted.python.failure import Failure
244
245-from landscape import SERVER_API
246-
247 from landscape.manager.customgraph import CustomGraphPlugin
248 from landscape.manager.store import ManagerStore
249
250@@ -458,7 +456,7 @@
251 self.graph_manager.exchange()
252 self.assertMessages(
253 self.broker_service.message_store.get_pending_messages(),
254- [{"api": SERVER_API,
255+ [{"api": "3.2",
256 "data": {123: {"error": u"",
257 "script-hash": "e00a2f44dbc7b6710ce32af2348aec9b",
258 "values": []}},
259@@ -481,7 +479,7 @@
260 self.graph_manager.exchange()
261 self.assertMessages(
262 self.broker_service.message_store.get_pending_messages(),
263- [{"api": SERVER_API,
264+ [{"api": "3.2",
265 "data": {},
266 "timestamp": 0,
267 "type": "custom-graph"}])
268@@ -506,13 +504,13 @@
269 self.graph_manager.exchange()
270 self.assertMessages(
271 self.broker_service.message_store.get_pending_messages(),
272- [{"api": SERVER_API,
273+ [{"api": "3.2",
274 "data": {123: {"error": u"",
275 "script-hash": "e00a2f44dbc7b6710ce32af2348aec9b",
276 "values": []}},
277 "timestamp": 0,
278 "type": "custom-graph"},
279- {"api": SERVER_API,
280+ {"api": "3.2",
281 "data": {123: {"error": u"",
282 "script-hash": "e00a2f44dbc7b6710ce32af2348aec9b",
283 "values": []}},
284@@ -540,13 +538,13 @@
285 self.graph_manager.exchange()
286 self.assertMessages(
287 self.broker_service.message_store.get_pending_messages(),
288- [{"api": SERVER_API,
289+ [{"api": "3.2",
290 "data": {123: {"error": u"",
291 "script-hash": "e00a2f44dbc7b6710ce32af2348aec9b",
292 "values": []}},
293 "timestamp": 0,
294 "type": "custom-graph"},
295- {"api": SERVER_API,
296+ {"api": "3.2",
297 "data": {123: {"error": u"",
298 "script-hash": "d483816dc0fbb51ede42502a709b0e2a",
299 "values": []}},
300@@ -589,7 +587,7 @@
301 self.graph_manager.exchange()
302 self.assertMessages(
303 self.broker_service.message_store.get_pending_messages(),
304- [{"api": SERVER_API,
305+ [{"api": "3.2",
306 "data": {123: {"error": u"",
307 "script-hash":
308 "991e15a81929c79fe1d243b2afd99c62",
309@@ -631,7 +629,7 @@
310 self.graph_manager.exchange()
311 self.assertMessages(
312 self.broker_service.message_store.get_pending_messages(),
313- [{"api": SERVER_API, "data": {}, "timestamp": 0, "type":
314+ [{"api": "3.2", "data": {}, "timestamp": 0, "type":
315 "custom-graph"}])
316 return result.addCallback(check)
317
318
319=== modified file 'landscape/manager/tests/test_shutdownmanager.py'
320--- landscape/manager/tests/test_shutdownmanager.py 2013-03-21 14:02:06 +0000
321+++ landscape/manager/tests/test_shutdownmanager.py 2014-09-01 13:16:19 +0000
322@@ -1,7 +1,6 @@
323 from twisted.python.failure import Failure
324 from twisted.internet.error import ProcessTerminated, ProcessDone
325
326-from landscape import SERVER_API
327 from landscape.manager.plugin import SUCCEEDED, FAILED
328 from landscape.manager.shutdownmanager import (
329 ShutdownManager, ShutdownProcessProtocol)
330@@ -44,7 +43,7 @@
331 self.assertTrue(self.broker_service.exchanger.is_urgent())
332 self.assertEqual(
333 self.broker_service.message_store.get_pending_messages(),
334- [{"type": "operation-result", "api": SERVER_API,
335+ [{"type": "operation-result", "api": "3.2",
336 "operation-id": 100, "timestamp": 10, "status": SUCCEEDED,
337 "result-text": u"Data may arrive in batches."}])
338
339@@ -84,7 +83,7 @@
340 self.assertTrue(self.broker_service.exchanger.is_urgent())
341 self.assertEqual(
342 self.broker_service.message_store.get_pending_messages(),
343- [{"type": "operation-result", "api": SERVER_API,
344+ [{"type": "operation-result", "api": "3.2",
345 "operation-id": 100, "timestamp": 0, "status": FAILED,
346 "result-text": u"Failure text is reported."}])
347
348
349=== modified file 'landscape/message_schemas.py'
350--- landscape/message_schemas.py 2014-08-21 09:43:26 +0000
351+++ landscape/message_schemas.py 2014-09-01 13:16:19 +0000
352@@ -54,14 +54,6 @@
353 "result-text": Unicode()},
354 optional=["result-code", "result-text"])
355
356-#ACTION_INFO is obsolete.
357-ACTION_INFO = Message(
358- "action-info",
359- {"response-id": Int(),
360- "success": Bool(),
361- "kind": Bytes(),
362- "parameters": Bytes()})
363-
364 COMPUTER_INFO = Message(
365 "computer-info",
366 {"hostname": Unicode(),
367@@ -112,11 +104,7 @@
368 "unit-name": Unicode(),
369 "private-address": Unicode()}
370
371-# The copy is needed because Message mutates the dictionary
372-JUJU_INFO = Message("juju-info",
373- juju_data.copy(),
374- optional=["private-address"])
375-
376+# The copy of juju_data is needed because Message mutates the dictionary
377 JUJU_UNITS_INFO = Message("juju-units-info", {
378 "juju-info-list": List(KeyDict(juju_data.copy(),
379 optional=["private-address"]))
380@@ -196,18 +184,37 @@
381 "tags": Any(Unicode(), Constant(None)),
382 "vm-info": Bytes(),
383 "container-info": Unicode(),
384- "juju-info": KeyDict(juju_data, optional=["private-address"]),
385- # Because of backwards compatibility we need another member with the list
386- # of juju-info, so it can safely be ignored by old servers.
387+ "access_group": Unicode()},
388+ optional=["registration_password", "hostname", "tags", "vm-info",
389+ "container-info", "access_group"])
390+
391+
392+REGISTER_3_3 = Message(
393+ "register",
394+ # The term used in the UI is actually 'registration_key', but we keep
395+ # the message schema field as 'registration_password' in case a new
396+ # client contacts an older server.
397+ {"registration_password": Any(Unicode(), Constant(None)),
398+ "computer_title": Unicode(),
399+ "hostname": Unicode(),
400+ "account_name": Unicode(),
401+ "tags": Any(Unicode(), Constant(None)),
402+ "vm-info": Bytes(),
403+ "container-info": Unicode(),
404+ "juju-info": KeyDict({"environment-uuid": Unicode(),
405+ "api-addresses": List(Unicode()),
406+ "machine-id": Unicode()}),
407+ # XXX temporary field with unit info, will be dropped when we complete
408+ # the migration to machine info.
409 "juju-info-list": List(KeyDict(juju_data, optional=["private-address"])),
410 "access_group": Unicode()},
411+ api="3.3",
412 optional=["registration_password", "hostname", "tags", "vm-info",
413- "container-info", "juju-info", "juju-info-list", "unicode",
414- "access_group"])
415+ "container-info", "access_group", "juju-info", "juju-info-list"])
416
417
418 # XXX The register-provisioned-machine message is obsolete, it's kept around
419-# just to not break older LDS releases that import it the last LDS release
420+# just to not break older LDS releases that import it (the last LDS release
421 # to have it is 14.07). Eventually it shall be dropped.
422 REGISTER_PROVISIONED_MACHINE = Message(
423 "register-provisioned-machine",
424@@ -238,6 +245,7 @@
425 "access_group": Unicode()},
426 optional=["tags", "vm-info", "public_ipv4", "local_ipv4", "access_group"])
427
428+
429 TEMPERATURE = Message("temperature", {
430 "thermal-zone": Unicode(),
431 "temperatures": List(Tuple(Int(), Float())),
432@@ -482,7 +490,7 @@
433 OPERATION_RESULT, COMPUTER_INFO, DISTRIBUTION_INFO,
434 HARDWARE_INVENTORY, HARDWARE_INFO, LOAD_AVERAGE, MEMORY_INFO,
435 RESYNCHRONIZE, MOUNT_ACTIVITY, MOUNT_INFO, FREE_SPACE,
436- REGISTER,
437+ REGISTER, REGISTER_3_3,
438 TEMPERATURE, PROCESSOR_INFO, USERS, PACKAGES, PACKAGE_LOCKS,
439 CHANGE_PACKAGES_RESULT, UNKNOWN_PACKAGE_HASHES,
440 ADD_PACKAGES, PACKAGE_REPORTER_RESULT, TEXT_MESSAGE, TEST,
441@@ -490,4 +498,4 @@
442 NETWORK_DEVICE, NETWORK_ACTIVITY,
443 REBOOT_REQUIRED_INFO, UPDATE_MANAGER_INFO, CPU_USAGE,
444 CEPH_USAGE, SWIFT_USAGE, SWIFT_DEVICE_INFO, KEYSTONE_TOKEN,
445- CHANGE_HA_SERVICE, JUJU_INFO, JUJU_UNITS_INFO, CLOUD_METADATA)
446+ CHANGE_HA_SERVICE, JUJU_UNITS_INFO, CLOUD_METADATA)
447
448=== modified file 'landscape/monitor/tests/test_activeprocessinfo.py'
449--- landscape/monitor/tests/test_activeprocessinfo.py 2013-07-17 07:26:31 +0000
450+++ landscape/monitor/tests/test_activeprocessinfo.py 2014-09-01 13:16:19 +0000
451@@ -10,7 +10,6 @@
452 from landscape.tests.helpers import (LandscapeTest, MonitorHelper,
453 ProcessDataBuilder)
454 from landscape.tests.mocker import ANY
455-from landscape import SERVER_API
456
457
458 class ActiveProcessInfoTest(LandscapeTest):
459@@ -577,7 +576,7 @@
460 messages = self.mstore.get_pending_messages()
461 self.assertEqual(len(messages), 2)
462 self.assertMessages(messages, [{"timestamp": 0,
463- "api": SERVER_API,
464+ "api": "3.2",
465 "type": "active-process-info",
466 "kill-all-processes": True,
467 "add-processes": [{"start-time": 110,
468@@ -589,7 +588,7 @@
469 "vm-size": 11676,
470 "uid": 0}]},
471 {"timestamp": 0,
472- "api": SERVER_API,
473+ "api": "3.2",
474 "type": "active-process-info",
475 "update-processes": [
476 {"start-time": 110,

Subscribers

People subscribed via source and target branches

to all changes: