Merge lp:~adam-collard/landscape-client/juju-info-at-registration into lp:~landscape/landscape-client/trunk
- juju-info-at-registration
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Adam Collard | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 733 | ||||
Proposed branch: | lp:~adam-collard/landscape-client/juju-info-at-registration | ||||
Merge into: | lp:~landscape/landscape-client/trunk | ||||
Diff against target: |
614 lines (+240/-118) 13 files modified
landscape/broker/registration.py (+69/-51) landscape/broker/tests/helpers.py (+3/-0) landscape/broker/tests/test_registration.py (+38/-7) landscape/deployment.py (+5/-0) landscape/lib/juju.py (+26/-0) landscape/lib/tests/test_encoding.py (+1/-1) landscape/lib/tests/test_juju.py (+62/-0) landscape/message_schemas.py (+10/-4) landscape/monitor/config.py (+0/-7) landscape/monitor/jujuinfo.py (+4/-22) landscape/monitor/tests/test_config.py (+0/-11) landscape/monitor/tests/test_jujuinfo.py (+9/-12) landscape/tests/test_deployment.py (+13/-3) |
||||
To merge this branch: | bzr merge lp:~adam-collard/landscape-client/juju-info-at-registration | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Björn Tillenius (community) | Approve | ||
Alberto Donato (community) | Approve | ||
Review via email: mp+187836@code.launchpad.net |
Commit message
Add Juju information at registration time.
* Changed schema of JUJU_INFO message to be strict about the keys
* Re-used schema for REGISTER and REGISTER_CLOUD_VM messages
* Added l.lib.juju as resting ground for json parsing and error handling
* Moved juju_filename to main config object, not just specific to monitor
Description of the change
Add Juju information at registration time.
* Changed schema of JUJU_INFO message to be strict about the keys
* Re-used schema for REGISTER and REGISTER_CLOUD_VM messages
* Added l.lib.juju as resting ground for json parsing and error handling
* Moved juju_filename to main config object, not just specific to monitor
Björn Tillenius (bjornt) wrote : | # |
Looks good, nothing major, but still marking it as Needs Fixing,
since [6] might change the diff a bit.
[1]
29 + self._juju_data = self._get_
I think it would be cleaner to collect the data when on "run", just
like ec2 data is collected.
[2]
222 + self.assertMess
223 + [{"type": "register",
224 + "computer_title": self.config.
225 + "account_name": "account_name",
226 + "registration_
227 + "hostname": socket.getfqdn(),
228 + "vm-info": get_vm_info(),
229 + "tags": None,
230 + "juju-info": {
231 + "environment-uuid": "DEAD-BEEF",
232 + "api-addresses": ["10.0.3.1:17070"],
233 + "unit-name": "juju-unit-name"}
234 + }])
The indentation looks off here. Also, please use valid unit names,
like service/0.
[3]
264 - "tags": None}])
265 + "tags": None,
266 + "juju-info": {
267 + "environment-uuid": "DEAD-BEEF",
268 + "api-addresses": ["10.0.3.1:17070"],
269 + "unit-name": "juju-unit-
Indentation is off here as well.
[4]
365 + def test_get_
366 + """L{get_juju_info} works with sample data."""
What does "works with sample data" mean? It's implied that our
code works. Better to explain what the code is expected to do instead.
[5]
384 + self.assertTrue(
385 + "Error attempting to read JSON" in self.logfile.
Please use assertIn instead, which gives a better error message on failures.
[6]
435 + "juju-info": KeyDict(
436 + optional=["tags", "vm-info", "public_ipv4", "local_ipv4",
437 + "juju-info"])
I think we shouldn't include juju-info in the register-cloud-vm message.
By default Juju instances won't use that the register-cloud-vm message,
and the plan is to remove it altogether.
Adam Collard (adam-collard) wrote : | # |
On 27 September 2013 15:32, Björn Tillenius <email address hidden> wrote:
> [1]
>
> 29 + self._juju_data = self._get_
>
> I think it would be cleaner to collect the data when on "run", just
> like ec2 data is collected.
>
OK. Done!
>
> [2]
>
>
> 222 + self.assertMess
> 223 + [{"type": "register",
> 224 + "computer_title":
> self.config.
> 225 + "account_name": "account_name",
> 226 + "registration_
> 227 + "hostname": socket.getfqdn(),
> 228 + "vm-info": get_vm_info(),
> 229 + "tags": None,
> 230 + "juju-info": {
> 231 + "environment-uuid": "DEAD-BEEF",
> 232 + "api-addresses": ["10.0.3.1:17070"],
> 233 + "unit-name": "juju-unit-name"}
> 234 + }])
>
> The indentation looks off here. Also, please use valid unit names,
> like service/0.
>
Emacs wanted to indent it like that - have shifted things around a bit.
Done!
>
>
> [3]
> 264 - "tags": None}])
> 265 + "tags": None,
> 266 + "juju-info": {
> 267 + "environment-uuid": "DEAD-BEEF",
> 268 + "api-addresses": ["10.0.3.1:17070
> "],
> 269 + "unit-name": "juju-unit-
>
> Indentation is off here as well.
>
This became irrelevant because of [6]
>
> [4]
> 365 + def test_get_
> 366 + """L{get_juju_info} works with sample data."""
>
> What does "works with sample data" mean? It's implied that our
> code works. Better to explain what the code is expected to do instead.
>
I've tried re-phrasing it. It really is just "does it work with realistic
data"
>
> [5]
> 384 + self.assertTrue(
> 385 + "Error attempting to read JSON" in
> self.logfile.
>
> Please use assertIn instead, which gives a better error message on
> failures.
>
Argh. I thought we didn't have it because client has to work on 2.6, but
mocker implements it. Grr, I had it as assertIn and the other test using
assertIs then carefully undid them. Thanks :)
>
>
> [6]
>
> 435 + "juju-info": KeyDict(
> 436 + optional=["tags", "vm-info", "public_ipv4", "local_ipv4",
> 437 + "juju-info"])
>
> I think we shouldn't include juju-info in the register-cloud-vm message.
> By default Juju instances won't use that the register-cloud-vm message,
> and the plan is to remove it altogether.
>
OK, I wasn't sure what this was so erred on the side of caution and
included it. Removed.
Björn Tillenius (bjornt) wrote : | # |
Thanks, looks good now, +1!
[7]
358 + def test_get_
359 + """L{get_juju_info} parses realistic data properly."""
I might be nitpicking a bit :), but I'd say never use words like
"works", "properly", "as expected", etc. in test docstrings. How about?
""" L{get_juju_info} parses JSON data from the juju_filename file."""
[8]
I don't see a test for when get_juju_info parses a file having
multiple API endpoints. Is there one?
- 732. By Björn Tillenius
-
Merge unmet-dependencies-bug-1203855 [f=1203855] [r=tribaal,ack] [a=Björn Tillenius]
Don't omit the package from the package changer result text if it's
broken but we don't know why.When a package is broken we check certain dependency to see whether they
are satisfied. If all the dependencies we check are satisified, the
package wasn't reported as being broken.Ideally we should write why the package is broken, but I have been
unable to reproduce the error in the bug report. - 733. By Adam Collard
-
Merge juju-info-
at-registration [f=1229630] [r=bjornt,ack] [a=Adam Collard]
Add Juju information at registration time.* Changed schema of JUJU_INFO message to be strict about the keys
* Re-used schema for REGISTER and REGISTER_CLOUD_VM messages
* Added l.lib.juju as resting ground for json parsing and error handling
* Moved juju_filename to main config object, not just specific to monitor
Preview Diff
1 | === modified file 'landscape/broker/registration.py' | |||
2 | --- landscape/broker/registration.py 2013-07-09 15:58:36 +0000 | |||
3 | +++ landscape/broker/registration.py 2013-09-27 14:18:49 +0000 | |||
4 | @@ -5,7 +5,7 @@ | |||
5 | 5 | as a new client without providing any identification credentials, and the | 5 | as a new client without providing any identification credentials, and the |
6 | 6 | server replies with the available registration mechanisms. At this point | 6 | server replies with the available registration mechanisms. At this point |
7 | 7 | the machinery in this module will notice that we have no identification | 7 | the machinery in this module will notice that we have no identification |
9 | 8 | credentials yet and that the server accepts regiration messages, so it | 8 | credentials yet and that the server accepts registration messages, so it |
10 | 9 | will craft an appropriate one and send it out. | 9 | will craft an appropriate one and send it out. |
11 | 10 | """ | 10 | """ |
12 | 11 | import time | 11 | import time |
13 | @@ -14,8 +14,11 @@ | |||
14 | 14 | 14 | ||
15 | 15 | from twisted.internet.defer import Deferred | 15 | from twisted.internet.defer import Deferred |
16 | 16 | 16 | ||
17 | 17 | from landscape.message_schemas import juju_data | ||
18 | 18 | |||
19 | 17 | from landscape.lib.bpickle import loads | 19 | from landscape.lib.bpickle import loads |
20 | 18 | from landscape.lib.log import log_failure | 20 | from landscape.lib.log import log_failure |
21 | 21 | from landscape.lib.juju import get_juju_info | ||
22 | 19 | from landscape.lib.fetch import fetch, FetchError | 22 | from landscape.lib.fetch import fetch, FetchError |
23 | 20 | from landscape.lib.tag import is_valid_tag_list | 23 | from landscape.lib.tag import is_valid_tag_list |
24 | 21 | from landscape.lib.network import get_fqdn | 24 | from landscape.lib.network import get_fqdn |
25 | @@ -97,6 +100,7 @@ | |||
26 | 97 | self._pinger = pinger | 100 | self._pinger = pinger |
27 | 98 | self._message_store = message_store | 101 | self._message_store = message_store |
28 | 99 | self._reactor.call_on("run", self._fetch_ec2_data) | 102 | self._reactor.call_on("run", self._fetch_ec2_data) |
29 | 103 | self._reactor.call_on("run", self._get_juju_data) | ||
30 | 100 | self._reactor.call_on("pre-exchange", self._handle_pre_exchange) | 104 | self._reactor.call_on("pre-exchange", self._handle_pre_exchange) |
31 | 101 | self._reactor.call_on("exchange-done", self._handle_exchange_done) | 105 | self._reactor.call_on("exchange-done", self._handle_exchange_done) |
32 | 102 | self._exchange.register_message("set-id", self._handle_set_id) | 106 | self._exchange.register_message("set-id", self._handle_set_id) |
33 | @@ -107,6 +111,7 @@ | |||
34 | 107 | self._fetch_async = fetch_async | 111 | self._fetch_async = fetch_async |
35 | 108 | self._otp = None | 112 | self._otp = None |
36 | 109 | self._ec2_data = None | 113 | self._ec2_data = None |
37 | 114 | self._juju_data = None | ||
38 | 110 | 115 | ||
39 | 111 | def should_register(self): | 116 | def should_register(self): |
40 | 112 | id = self._identity | 117 | id = self._identity |
41 | @@ -134,6 +139,14 @@ | |||
42 | 134 | self._exchange.exchange() | 139 | self._exchange.exchange() |
43 | 135 | return result | 140 | return result |
44 | 136 | 141 | ||
45 | 142 | def _get_juju_data(self): | ||
46 | 143 | """Load Juju information.""" | ||
47 | 144 | juju_info = get_juju_info(self._config) | ||
48 | 145 | if juju_info is None: | ||
49 | 146 | return None | ||
50 | 147 | self._juju_data = dict( | ||
51 | 148 | (key, juju_info[key]) for key in juju_data) | ||
52 | 149 | |||
53 | 137 | def _get_data(self, path, accumulate): | 150 | def _get_data(self, path, accumulate): |
54 | 138 | """ | 151 | """ |
55 | 139 | Get data at C{path} on the EC2 API endpoint, and add the result to the | 152 | Get data at C{path} on the EC2 API endpoint, and add the result to the |
56 | @@ -254,65 +267,70 @@ | |||
57 | 254 | # registration. | 267 | # registration. |
58 | 255 | self._should_register = self.should_register() | 268 | self._should_register = self.should_register() |
59 | 256 | 269 | ||
97 | 257 | if self._should_register: | 270 | if not self._should_register: |
98 | 258 | id = self._identity | 271 | return |
99 | 259 | self._message_store.delete_all_messages() | 272 | id = self._identity |
100 | 260 | tags = id.tags | 273 | self._message_store.delete_all_messages() |
101 | 261 | if not is_valid_tag_list(tags): | 274 | tags = id.tags |
102 | 262 | tags = None | 275 | if not is_valid_tag_list(tags): |
103 | 263 | logging.error("Invalid tags provided for cloud " | 276 | tags = None |
104 | 264 | "registration.") | 277 | logging.error("Invalid tags provided for cloud registration.") |
105 | 265 | if self._config.cloud and self._ec2_data is not None: | 278 | if self._config.cloud and self._ec2_data is not None: |
106 | 266 | if self._otp: | 279 | if self._otp: |
107 | 267 | logging.info("Queueing message to register with OTP") | 280 | logging.info("Queueing message to register with OTP") |
108 | 268 | message = {"type": "register-cloud-vm", | 281 | message = {"type": "register-cloud-vm", |
109 | 269 | "otp": self._otp, | 282 | "otp": self._otp, |
110 | 270 | "hostname": get_fqdn(), | 283 | "hostname": get_fqdn(), |
111 | 271 | "account_name": None, | 284 | "account_name": None, |
112 | 272 | "registration_password": None, | 285 | "registration_password": None, |
113 | 273 | "tags": tags, | 286 | "tags": tags, |
114 | 274 | "vm-info": get_vm_info()} | 287 | "vm-info": get_vm_info()} |
115 | 275 | message.update(self._ec2_data) | 288 | message.update(self._ec2_data) |
116 | 276 | self._exchange.send(message) | 289 | if self._juju_data is not None: |
117 | 277 | elif id.account_name: | 290 | message["juju-info"] = self._juju_data |
118 | 278 | with_tags = ["", u"and tags %s " % tags][bool(tags)] | 291 | self._exchange.send(message) |
82 | 279 | logging.info( | ||
83 | 280 | u"Queueing message to register with account %r %s" | ||
84 | 281 | u"as an EC2 instance." % (id.account_name, with_tags)) | ||
85 | 282 | message = {"type": "register-cloud-vm", | ||
86 | 283 | "otp": None, | ||
87 | 284 | "hostname": get_fqdn(), | ||
88 | 285 | "account_name": id.account_name, | ||
89 | 286 | "registration_password": \ | ||
90 | 287 | id.registration_key, | ||
91 | 288 | "tags": tags, | ||
92 | 289 | "vm-info": get_vm_info()} | ||
93 | 290 | message.update(self._ec2_data) | ||
94 | 291 | self._exchange.send(message) | ||
95 | 292 | else: | ||
96 | 293 | self._reactor.fire("registration-failed") | ||
119 | 294 | elif id.account_name: | 292 | elif id.account_name: |
120 | 295 | with_word = ["without", "with"][bool(id.registration_key)] | ||
121 | 296 | with_tags = ["", u"and tags %s " % tags][bool(tags)] | 293 | with_tags = ["", u"and tags %s " % tags][bool(tags)] |
127 | 297 | logging.info(u"Queueing message to register with account %r %s" | 294 | logging.info( |
128 | 298 | "%s a password." % (id.account_name, with_tags, | 295 | u"Queueing message to register with account %r %s" |
129 | 299 | with_word)) | 296 | u"as an EC2 instance." % (id.account_name, with_tags)) |
130 | 300 | message = {"type": "register", | 297 | message = {"type": "register-cloud-vm", |
131 | 301 | "computer_title": id.computer_title, | 298 | "otp": None, |
132 | 299 | "hostname": get_fqdn(), | ||
133 | 302 | "account_name": id.account_name, | 300 | "account_name": id.account_name, |
134 | 303 | "registration_password": id.registration_key, | 301 | "registration_password": id.registration_key, |
135 | 304 | "hostname": get_fqdn(), | ||
136 | 305 | "tags": tags, | 302 | "tags": tags, |
137 | 306 | "vm-info": get_vm_info()} | 303 | "vm-info": get_vm_info()} |
144 | 307 | self._exchange.send(message) | 304 | message.update(self._ec2_data) |
145 | 308 | elif self._config.provisioning_otp: | 305 | if self._juju_data is not None: |
146 | 309 | logging.info(u"Queueing message to register with OTP as a" | 306 | message["juju-info"] = self._juju_data |
141 | 310 | u" newly provisioned machine.") | ||
142 | 311 | message = {"type": "register-provisioned-machine", | ||
143 | 312 | "otp": self._config.provisioning_otp} | ||
147 | 313 | self._exchange.send(message) | 307 | self._exchange.send(message) |
148 | 314 | else: | 308 | else: |
149 | 315 | self._reactor.fire("registration-failed") | 309 | self._reactor.fire("registration-failed") |
150 | 310 | elif id.account_name: | ||
151 | 311 | with_word = ["without", "with"][bool(id.registration_key)] | ||
152 | 312 | with_tags = ["", u"and tags %s " % tags][bool(tags)] | ||
153 | 313 | logging.info(u"Queueing message to register with account %r %s" | ||
154 | 314 | "%s a password." % (id.account_name, with_tags, | ||
155 | 315 | with_word)) | ||
156 | 316 | message = {"type": "register", | ||
157 | 317 | "computer_title": id.computer_title, | ||
158 | 318 | "account_name": id.account_name, | ||
159 | 319 | "registration_password": id.registration_key, | ||
160 | 320 | "hostname": get_fqdn(), | ||
161 | 321 | "tags": tags, | ||
162 | 322 | "vm-info": get_vm_info()} | ||
163 | 323 | if self._juju_data is not None: | ||
164 | 324 | message["juju-info"] = self._juju_data | ||
165 | 325 | self._exchange.send(message) | ||
166 | 326 | elif self._config.provisioning_otp: | ||
167 | 327 | logging.info(u"Queueing message to register with OTP as a" | ||
168 | 328 | u" newly provisioned machine.") | ||
169 | 329 | message = {"type": "register-provisioned-machine", | ||
170 | 330 | "otp": self._config.provisioning_otp} | ||
171 | 331 | self._exchange.send(message) | ||
172 | 332 | else: | ||
173 | 333 | self._reactor.fire("registration-failed") | ||
174 | 316 | 334 | ||
175 | 317 | def _handle_set_id(self, message): | 335 | def _handle_set_id(self, message): |
176 | 318 | """Registered handler for the C{"set-id"} event. | 336 | """Registered handler for the C{"set-id"} event. |
177 | 319 | 337 | ||
178 | === modified file 'landscape/broker/tests/helpers.py' | |||
179 | --- landscape/broker/tests/helpers.py 2013-05-07 22:47:06 +0000 | |||
180 | +++ landscape/broker/tests/helpers.py 2013-09-27 14:18:49 +0000 | |||
181 | @@ -127,6 +127,9 @@ | |||
182 | 127 | 127 | ||
183 | 128 | test_case.fetch_func = fetch_async | 128 | test_case.fetch_func = fetch_async |
184 | 129 | test_case.config.cloud = getattr(test_case, "cloud", False) | 129 | test_case.config.cloud = getattr(test_case, "cloud", False) |
185 | 130 | if hasattr(test_case, "juju_contents"): | ||
186 | 131 | test_case.makeFile( | ||
187 | 132 | test_case.juju_contents, path=test_case.config.juju_filename) | ||
188 | 130 | test_case.handler = RegistrationHandler( | 133 | test_case.handler = RegistrationHandler( |
189 | 131 | test_case.config, test_case.identity, test_case.reactor, | 134 | test_case.config, test_case.identity, test_case.reactor, |
190 | 132 | test_case.exchanger, test_case.pinger, test_case.mstore, | 135 | test_case.exchanger, test_case.pinger, test_case.mstore, |
191 | 133 | 136 | ||
192 | === modified file 'landscape/broker/tests/test_registration.py' | |||
193 | --- landscape/broker/tests/test_registration.py 2013-08-20 23:48:23 +0000 | |||
194 | +++ landscape/broker/tests/test_registration.py 2013-09-27 14:18:49 +0000 | |||
195 | @@ -1,3 +1,4 @@ | |||
196 | 1 | import json | ||
197 | 1 | import os | 2 | import os |
198 | 2 | import logging | 3 | import logging |
199 | 3 | import pycurl | 4 | import pycurl |
200 | @@ -177,7 +178,7 @@ | |||
201 | 177 | """ | 178 | """ |
202 | 178 | When a computer_title and account_name are available, no | 179 | When a computer_title and account_name are available, no |
203 | 179 | secure_id is set, and an exchange is about to happen, | 180 | secure_id is set, and an exchange is about to happen, |
205 | 180 | queue a registration message. | 181 | queue a registration message with VM information. |
206 | 181 | """ | 182 | """ |
207 | 182 | get_vm_info_mock = self.mocker.replace(get_vm_info) | 183 | get_vm_info_mock = self.mocker.replace(get_vm_info) |
208 | 183 | get_vm_info_mock() | 184 | get_vm_info_mock() |
209 | @@ -484,6 +485,38 @@ | |||
210 | 484 | "tags": None}]) | 485 | "tags": None}]) |
211 | 485 | 486 | ||
212 | 486 | 487 | ||
213 | 488 | class JujuRegistrationHandlerTest(RegistrationHandlerTestBase): | ||
214 | 489 | |||
215 | 490 | juju_contents = json.dumps({"environment-uuid": "DEAD-BEEF", | ||
216 | 491 | "unit-name": "service/0", | ||
217 | 492 | "api-addresses": "10.0.3.1:17070", | ||
218 | 493 | "private-address": "127.0.0.1"}) | ||
219 | 494 | |||
220 | 495 | def test_juju_information_added_when_present(self): | ||
221 | 496 | """ | ||
222 | 497 | When Juju information is found in $data_dir/juju-info.json, | ||
223 | 498 | key parts of it are sent in the registration message. | ||
224 | 499 | """ | ||
225 | 500 | self.mstore.set_accepted_types(["register"]) | ||
226 | 501 | self.config.account_name = "account_name" | ||
227 | 502 | self.reactor.fire("run") | ||
228 | 503 | self.reactor.fire("pre-exchange") | ||
229 | 504 | |||
230 | 505 | self.assertMessages( | ||
231 | 506 | self.mstore.get_pending_messages(), | ||
232 | 507 | [{"type": "register", | ||
233 | 508 | "computer_title": self.config.computer_title, | ||
234 | 509 | "account_name": "account_name", | ||
235 | 510 | "registration_password": None, | ||
236 | 511 | "hostname": socket.getfqdn(), | ||
237 | 512 | "vm-info": get_vm_info(), | ||
238 | 513 | "tags": None, | ||
239 | 514 | "juju-info": {"environment-uuid": "DEAD-BEEF", | ||
240 | 515 | "api-addresses": ["10.0.3.1:17070"], | ||
241 | 516 | "unit-name": "service/0"} | ||
242 | 517 | }]) | ||
243 | 518 | |||
244 | 519 | |||
245 | 487 | class CloudRegistrationHandlerTest(RegistrationHandlerTestBase): | 520 | class CloudRegistrationHandlerTest(RegistrationHandlerTestBase): |
246 | 488 | 521 | ||
247 | 489 | cloud = True | 522 | cloud = True |
248 | @@ -574,7 +607,6 @@ | |||
249 | 574 | local_ipv4=u"10.0.0.1", | 607 | local_ipv4=u"10.0.0.1", |
250 | 575 | public_ipv4=u"10.0.0.2", | 608 | public_ipv4=u"10.0.0.2", |
251 | 576 | tags=None) | 609 | tags=None) |
252 | 577 | |||
253 | 578 | # The get_vm_info() needs to be deferred to the else. If vm-info is | 610 | # The get_vm_info() needs to be deferred to the else. If vm-info is |
254 | 579 | # specified in kwargs, get_vm_info() will typically be mocked. | 611 | # specified in kwargs, get_vm_info() will typically be mocked. |
255 | 580 | if "vm_info" in kwargs: | 612 | if "vm_info" in kwargs: |
256 | @@ -912,7 +944,8 @@ | |||
257 | 912 | "registration_password": u"password", | 944 | "registration_password": u"password", |
258 | 913 | "hostname": socket.getfqdn(), | 945 | "hostname": socket.getfqdn(), |
259 | 914 | "vm-info": get_vm_info(), | 946 | "vm-info": get_vm_info(), |
261 | 915 | "tags": None}]) | 947 | "tags": None, |
262 | 948 | }]) | ||
263 | 916 | 949 | ||
264 | 917 | def test_should_register_in_cloud(self): | 950 | def test_should_register_in_cloud(self): |
265 | 918 | """ | 951 | """ |
266 | @@ -1157,7 +1190,5 @@ | |||
267 | 1157 | self.config.provisioning_otp = "" | 1190 | self.config.provisioning_otp = "" |
268 | 1158 | self.reactor.fire("pre-exchange") | 1191 | self.reactor.fire("pre-exchange") |
269 | 1159 | 1192 | ||
274 | 1160 | self.assertMessages([], | 1193 | self.assertMessages([], self.mstore.get_pending_messages()) |
275 | 1161 | self.mstore.get_pending_messages()) | 1194 | self.assertEqual(u"", self.logfile.getvalue().strip()) |
272 | 1162 | self.assertEqual(u"", | ||
273 | 1163 | self.logfile.getvalue().strip()) | ||
276 | 1164 | 1195 | ||
277 | === modified file 'landscape/deployment.py' | |||
278 | --- landscape/deployment.py 2013-09-25 08:50:25 +0000 | |||
279 | +++ landscape/deployment.py 2013-09-27 14:18:49 +0000 | |||
280 | @@ -419,6 +419,11 @@ | |||
281 | 419 | """ | 419 | """ |
282 | 420 | return os.path.join(self.data_path, "annotations.d") | 420 | return os.path.join(self.data_path, "annotations.d") |
283 | 421 | 421 | ||
284 | 422 | @property | ||
285 | 423 | def juju_filename(self): | ||
286 | 424 | """Get the path to the Juju JSON file.""" | ||
287 | 425 | return os.path.join(self.data_path, "juju-info.json") | ||
288 | 426 | |||
289 | 422 | 427 | ||
290 | 423 | def get_versioned_persist(service): | 428 | def get_versioned_persist(service): |
291 | 424 | """Get a L{Persist} database with upgrade rules applied. | 429 | """Get a L{Persist} database with upgrade rules applied. |
292 | 425 | 430 | ||
293 | === added file 'landscape/lib/juju.py' | |||
294 | --- landscape/lib/juju.py 1970-01-01 00:00:00 +0000 | |||
295 | +++ landscape/lib/juju.py 2013-09-27 14:18:49 +0000 | |||
296 | @@ -0,0 +1,26 @@ | |||
297 | 1 | import json | ||
298 | 2 | import logging | ||
299 | 3 | import os.path | ||
300 | 4 | |||
301 | 5 | from landscape.lib.fs import read_file | ||
302 | 6 | |||
303 | 7 | |||
304 | 8 | def get_juju_info(config): | ||
305 | 9 | """ | ||
306 | 10 | Returns the Juju info or C{None} if the path referenced from | ||
307 | 11 | L{config} is not a file or that file isn't valid JSON. | ||
308 | 12 | """ | ||
309 | 13 | juju_filename = config.juju_filename | ||
310 | 14 | if not os.path.isfile(juju_filename): | ||
311 | 15 | return None | ||
312 | 16 | json_contents = read_file(juju_filename) | ||
313 | 17 | try: | ||
314 | 18 | juju_info = json.loads(json_contents) | ||
315 | 19 | except Exception: | ||
316 | 20 | logging.exception( | ||
317 | 21 | "Error attempting to read JSON from %s" % juju_filename) | ||
318 | 22 | return None | ||
319 | 23 | else: | ||
320 | 24 | if "api-addresses" in juju_info: | ||
321 | 25 | juju_info["api-addresses"] = juju_info["api-addresses"].split() | ||
322 | 26 | return juju_info | ||
323 | 0 | 27 | ||
324 | === modified file 'landscape/lib/tests/test_encoding.py' | |||
325 | --- landscape/lib/tests/test_encoding.py 2013-03-20 08:58:22 +0000 | |||
326 | +++ landscape/lib/tests/test_encoding.py 2013-09-27 14:18:49 +0000 | |||
327 | @@ -4,7 +4,7 @@ | |||
328 | 4 | from landscape.lib.encoding import encode_if_needed, encode_dict_if_needed | 4 | from landscape.lib.encoding import encode_if_needed, encode_dict_if_needed |
329 | 5 | 5 | ||
330 | 6 | 6 | ||
332 | 7 | class EncodingTestTest(LandscapeTest): | 7 | class EncodingTest(LandscapeTest): |
333 | 8 | 8 | ||
334 | 9 | def test_encode_if_needed_utf_string(self): | 9 | def test_encode_if_needed_utf_string(self): |
335 | 10 | """ | 10 | """ |
336 | 11 | 11 | ||
337 | === added file 'landscape/lib/tests/test_juju.py' | |||
338 | --- landscape/lib/tests/test_juju.py 1970-01-01 00:00:00 +0000 | |||
339 | +++ landscape/lib/tests/test_juju.py 2013-09-27 14:18:49 +0000 | |||
340 | @@ -0,0 +1,62 @@ | |||
341 | 1 | from collections import namedtuple | ||
342 | 2 | import json | ||
343 | 3 | |||
344 | 4 | from landscape.tests.helpers import LandscapeTest | ||
345 | 5 | from landscape.lib.juju import get_juju_info | ||
346 | 6 | |||
347 | 7 | |||
348 | 8 | SAMPLE_JUJU_INFO = json.dumps({"environment-uuid": "DEAD-BEEF", | ||
349 | 9 | "unit-name": "service/0", | ||
350 | 10 | "api-addresses": "10.0.3.1:17070", | ||
351 | 11 | "private-address": "127.0.0.1"}) | ||
352 | 12 | |||
353 | 13 | |||
354 | 14 | class JujuTest(LandscapeTest): | ||
355 | 15 | |||
356 | 16 | Config = namedtuple("Config", "juju_filename") | ||
357 | 17 | |||
358 | 18 | def test_get_juju_info_sample_data(self): | ||
359 | 19 | """L{get_juju_info} parses JSON data from the juju_filename file.""" | ||
360 | 20 | stub_config = self.Config(self.makeFile(SAMPLE_JUJU_INFO)) | ||
361 | 21 | juju_info = get_juju_info(stub_config) | ||
362 | 22 | self.assertEqual( | ||
363 | 23 | {u"environment-uuid": "DEAD-BEEF", | ||
364 | 24 | u"unit-name": "service/0", | ||
365 | 25 | u"api-addresses": ["10.0.3.1:17070"], | ||
366 | 26 | u"private-address": "127.0.0.1"}, juju_info) | ||
367 | 27 | |||
368 | 28 | def test_get_juju_info_empty_file(self): | ||
369 | 29 | """ | ||
370 | 30 | If L{get_juju_info} is called with a configuration pointing to | ||
371 | 31 | an empty file, it returns C{None}. | ||
372 | 32 | """ | ||
373 | 33 | stub_config = self.Config(self.makeFile("")) | ||
374 | 34 | juju_info = get_juju_info(stub_config) | ||
375 | 35 | self.log_helper.ignore_errors(ValueError) | ||
376 | 36 | self.assertEqual(juju_info, None) | ||
377 | 37 | self.assertIn("Error attempting to read JSON", self.logfile.getvalue()) | ||
378 | 38 | |||
379 | 39 | def test_get_juju_info_not_a_file(self): | ||
380 | 40 | """ | ||
381 | 41 | If L{get_juju_info} is called with a configuration pointing to | ||
382 | 42 | a directory, it returns C{None}. | ||
383 | 43 | """ | ||
384 | 44 | stub_config = self.Config("/") | ||
385 | 45 | juju_info = get_juju_info(stub_config) | ||
386 | 46 | self.assertIs(juju_info, None) | ||
387 | 47 | |||
388 | 48 | def test_get_juju_info_multiple_endpoints(self): | ||
389 | 49 | """L{get_juju_info} turns space separated API addresses into a list.""" | ||
390 | 50 | juju_multiple_endpoints = json.dumps({ | ||
391 | 51 | "environment-uuid": "DEAD-BEEF", | ||
392 | 52 | "unit-name": "service/0", | ||
393 | 53 | "api-addresses": "10.0.3.1:17070 10.0.3.2:18080", | ||
394 | 54 | "private-address": "127.0.0.1"}) | ||
395 | 55 | |||
396 | 56 | stub_config = self.Config(self.makeFile(juju_multiple_endpoints)) | ||
397 | 57 | juju_info = get_juju_info(stub_config) | ||
398 | 58 | self.assertEqual( | ||
399 | 59 | {u"environment-uuid": "DEAD-BEEF", | ||
400 | 60 | u"unit-name": "service/0", | ||
401 | 61 | u"api-addresses": ["10.0.3.1:17070", "10.0.3.2:18080"], | ||
402 | 62 | u"private-address": "127.0.0.1"}, juju_info) | ||
403 | 0 | 63 | ||
404 | === modified file 'landscape/message_schemas.py' | |||
405 | --- landscape/message_schemas.py 2013-09-25 08:43:53 +0000 | |||
406 | +++ landscape/message_schemas.py 2013-09-27 14:18:49 +0000 | |||
407 | @@ -103,8 +103,12 @@ | |||
408 | 103 | HARDWARE_INFO = Message("hardware-info", { | 103 | HARDWARE_INFO = Message("hardware-info", { |
409 | 104 | "data": utf8}) | 104 | "data": utf8}) |
410 | 105 | 105 | ||
413 | 106 | JUJU_INFO = Message("juju-info", { | 106 | juju_data = {"environment-uuid": utf8, |
414 | 107 | "data": Dict(utf8, utf8)}) | 107 | "api-addresses": List(utf8), |
415 | 108 | "unit-name": utf8} | ||
416 | 109 | |||
417 | 110 | # The copy is needed because Message mutates the dictionary | ||
418 | 111 | JUJU_INFO = Message("juju-info", juju_data.copy()) | ||
419 | 108 | 112 | ||
420 | 109 | LOAD_AVERAGE = Message("load-average", { | 113 | LOAD_AVERAGE = Message("load-average", { |
421 | 110 | "load-averages": List(Tuple(Int(), Float())), | 114 | "load-averages": List(Tuple(Int(), Float())), |
422 | @@ -171,9 +175,11 @@ | |||
423 | 171 | "hostname": utf8, | 175 | "hostname": utf8, |
424 | 172 | "account_name": utf8, | 176 | "account_name": utf8, |
425 | 173 | "tags": Any(utf8, Constant(None)), | 177 | "tags": Any(utf8, Constant(None)), |
427 | 174 | "vm-info": String()}, | 178 | "vm-info": String(), |
428 | 179 | "juju-info": KeyDict(juju_data)}, | ||
429 | 175 | # hostname wasn't around in old versions | 180 | # hostname wasn't around in old versions |
431 | 176 | optional=["registration_password", "hostname", "tags", "vm-info"]) | 181 | optional=["registration_password", "hostname", "tags", "vm-info", |
432 | 182 | "juju-info"]) | ||
433 | 177 | 183 | ||
434 | 178 | 184 | ||
435 | 179 | REGISTER_PROVISIONED_MACHINE = Message( | 185 | REGISTER_PROVISIONED_MACHINE = Message( |
436 | 180 | 186 | ||
437 | === modified file 'landscape/monitor/config.py' | |||
438 | --- landscape/monitor/config.py 2013-09-25 14:06:05 +0000 | |||
439 | +++ landscape/monitor/config.py 2013-09-27 14:18:49 +0000 | |||
440 | @@ -1,5 +1,3 @@ | |||
441 | 1 | import os.path | ||
442 | 2 | |||
443 | 3 | from landscape.deployment import Configuration | 1 | from landscape.deployment import Configuration |
444 | 4 | 2 | ||
445 | 5 | 3 | ||
446 | @@ -32,8 +30,3 @@ | |||
447 | 32 | if self.monitor_plugins == "ALL": | 30 | if self.monitor_plugins == "ALL": |
448 | 33 | return ALL_PLUGINS | 31 | return ALL_PLUGINS |
449 | 34 | return [x.strip() for x in self.monitor_plugins.split(",")] | 32 | return [x.strip() for x in self.monitor_plugins.split(",")] |
450 | 35 | |||
451 | 36 | @property | ||
452 | 37 | def juju_filename(self): | ||
453 | 38 | """Get the path to the Juju JSON file.""" | ||
454 | 39 | return os.path.join(self.data_path, "juju-info.json") | ||
455 | 40 | 33 | ||
456 | === modified file 'landscape/monitor/jujuinfo.py' | |||
457 | --- landscape/monitor/jujuinfo.py 2013-09-25 14:06:05 +0000 | |||
458 | +++ landscape/monitor/jujuinfo.py 2013-09-27 14:18:49 +0000 | |||
459 | @@ -1,8 +1,6 @@ | |||
460 | 1 | import json | ||
461 | 2 | import logging | 1 | import logging |
462 | 3 | import os.path | ||
463 | 4 | 2 | ||
465 | 5 | from landscape.lib.fs import read_file | 3 | from landscape.lib.juju import get_juju_info |
466 | 6 | from landscape.monitor.plugin import MonitorPlugin | 4 | from landscape.monitor.plugin import MonitorPlugin |
467 | 7 | 5 | ||
468 | 8 | 6 | ||
469 | @@ -21,32 +19,16 @@ | |||
470 | 21 | broker.call_if_accepted("juju-info", self.send_juju_message, urgent) | 19 | broker.call_if_accepted("juju-info", self.send_juju_message, urgent) |
471 | 22 | 20 | ||
472 | 23 | def send_juju_message(self, urgent=False): | 21 | def send_juju_message(self, urgent=False): |
476 | 24 | message = {} | 22 | message = self._create_juju_info_message() |
477 | 25 | juju_data = self._create_juju_info_message() | 23 | if message: |
475 | 26 | if juju_data: | ||
478 | 27 | message["type"] = "juju-info" | 24 | message["type"] = "juju-info" |
479 | 28 | message["data"] = juju_data | ||
480 | 29 | logging.info("Queuing message with updated juju info.") | 25 | logging.info("Queuing message with updated juju info.") |
481 | 30 | self.registry.broker.send_message(message, self._session_id, | 26 | self.registry.broker.send_message(message, self._session_id, |
482 | 31 | urgent=urgent) | 27 | urgent=urgent) |
483 | 32 | 28 | ||
484 | 33 | def _create_juju_info_message(self): | 29 | def _create_juju_info_message(self): |
486 | 34 | message = self._get_juju_info() | 30 | message = get_juju_info(self.registry.config) |
487 | 35 | if message != self._persist.get("juju-info"): | 31 | if message != self._persist.get("juju-info"): |
488 | 36 | self._persist.set("juju-info", message) | 32 | self._persist.set("juju-info", message) |
489 | 37 | return message | 33 | return message |
490 | 38 | return None | 34 | return None |
491 | 39 | |||
492 | 40 | def _get_juju_info(self): | ||
493 | 41 | juju_filename = self.registry.config.juju_filename | ||
494 | 42 | if not os.path.isfile(juju_filename): | ||
495 | 43 | return None | ||
496 | 44 | json_contents = read_file(juju_filename) | ||
497 | 45 | try: | ||
498 | 46 | juju_info = json.loads(json_contents) | ||
499 | 47 | except Exception: | ||
500 | 48 | logging.exception( | ||
501 | 49 | "Error attempting to read JSON from %s" % juju_filename) | ||
502 | 50 | return None | ||
503 | 51 | else: | ||
504 | 52 | return juju_info | ||
505 | 53 | 35 | ||
506 | === modified file 'landscape/monitor/tests/test_config.py' | |||
507 | --- landscape/monitor/tests/test_config.py 2013-09-25 14:06:05 +0000 | |||
508 | +++ landscape/monitor/tests/test_config.py 2013-09-27 14:18:49 +0000 | |||
509 | @@ -1,5 +1,3 @@ | |||
510 | 1 | import os.path | ||
511 | 2 | |||
512 | 3 | from landscape.tests.helpers import LandscapeTest | 1 | from landscape.tests.helpers import LandscapeTest |
513 | 4 | from landscape.monitor.config import MonitorConfiguration, ALL_PLUGINS | 2 | from landscape.monitor.config import MonitorConfiguration, ALL_PLUGINS |
514 | 5 | 3 | ||
515 | @@ -32,12 +30,3 @@ | |||
516 | 32 | """ | 30 | """ |
517 | 33 | self.config.load(["--flush-interval", "123"]) | 31 | self.config.load(["--flush-interval", "123"]) |
518 | 34 | self.assertEqual(self.config.flush_interval, 123) | 32 | self.assertEqual(self.config.flush_interval, 123) |
519 | 35 | |||
520 | 36 | def test_juju_filename(self): | ||
521 | 37 | """ | ||
522 | 38 | Can get path to juju JSON file from config. | ||
523 | 39 | """ | ||
524 | 40 | self.config.data_path = self.makeDir() | ||
525 | 41 | self.assertEqual(self.config.juju_filename, os.path.join( | ||
526 | 42 | self.config.data_path, "juju-info.json")) | ||
527 | 43 | |||
528 | 44 | 33 | ||
529 | === modified file 'landscape/monitor/tests/test_jujuinfo.py' | |||
530 | --- landscape/monitor/tests/test_jujuinfo.py 2013-09-25 14:06:05 +0000 | |||
531 | +++ landscape/monitor/tests/test_jujuinfo.py 2013-09-27 14:18:49 +0000 | |||
532 | @@ -30,10 +30,9 @@ | |||
533 | 30 | self.plugin.exchange() | 30 | self.plugin.exchange() |
534 | 31 | message = self.mstore.get_pending_messages()[0] | 31 | message = self.mstore.get_pending_messages()[0] |
535 | 32 | self.assertEqual(message["type"], "juju-info") | 32 | self.assertEqual(message["type"], "juju-info") |
540 | 33 | self.assertEqual(message["data"]["environment-uuid"], "DEAD-BEEF") | 33 | self.assertEqual(message["environment-uuid"], "DEAD-BEEF") |
541 | 34 | self.assertEqual(message["data"]["unit-name"], "juju-unit-name") | 34 | self.assertEqual(message["unit-name"], "juju-unit-name") |
542 | 35 | self.assertEqual(message["data"]["api-addresses"], "10.0.3.1:17070") | 35 | self.assertEqual(message["api-addresses"], ["10.0.3.1:17070"]) |
539 | 36 | self.assertEqual(message["data"]["private-address"], "127.0.0.1") | ||
543 | 37 | 36 | ||
544 | 38 | def test_juju_info_reported_only_once(self): | 37 | def test_juju_info_reported_only_once(self): |
545 | 39 | """ | 38 | """ |
546 | @@ -57,10 +56,9 @@ | |||
547 | 57 | self.plugin.exchange() | 56 | self.plugin.exchange() |
548 | 58 | message = self.mstore.get_pending_messages()[0] | 57 | message = self.mstore.get_pending_messages()[0] |
549 | 59 | self.assertEqual(message["type"], "juju-info") | 58 | self.assertEqual(message["type"], "juju-info") |
554 | 60 | self.assertEqual(message["data"]["environment-uuid"], "DEAD-BEEF") | 59 | self.assertEqual(message["environment-uuid"], "DEAD-BEEF") |
555 | 61 | self.assertEqual(message["data"]["unit-name"], "juju-unit-name") | 60 | self.assertEqual(message["unit-name"], "juju-unit-name") |
556 | 62 | self.assertEqual(message["data"]["api-addresses"], "10.0.3.1:17070") | 61 | self.assertEqual(message["api-addresses"], ["10.0.3.1:17070"]) |
553 | 63 | self.assertEqual(message["data"]["private-address"], "127.0.0.1") | ||
557 | 64 | 62 | ||
558 | 65 | self.makeFile( | 63 | self.makeFile( |
559 | 66 | json.dumps({"environment-uuid": "FEED-BEEF", | 64 | json.dumps({"environment-uuid": "FEED-BEEF", |
560 | @@ -71,10 +69,9 @@ | |||
561 | 71 | self.plugin.exchange() | 69 | self.plugin.exchange() |
562 | 72 | message = self.mstore.get_pending_messages()[1] | 70 | message = self.mstore.get_pending_messages()[1] |
563 | 73 | self.assertEqual(message["type"], "juju-info") | 71 | self.assertEqual(message["type"], "juju-info") |
568 | 74 | self.assertEqual(message["data"]["environment-uuid"], "FEED-BEEF") | 72 | self.assertEqual(message["environment-uuid"], "FEED-BEEF") |
569 | 75 | self.assertEqual(message["data"]["unit-name"], "changed-unit-name") | 73 | self.assertEqual(message["unit-name"], "changed-unit-name") |
570 | 76 | self.assertEqual(message["data"]["api-addresses"], "10.0.3.2:17070") | 74 | self.assertEqual(message["api-addresses"], ["10.0.3.2:17070"]) |
567 | 77 | self.assertEqual(message["data"]["private-address"], "127.0.1.1") | ||
571 | 78 | 75 | ||
572 | 79 | def test_no_message_with_invalid_json(self): | 76 | def test_no_message_with_invalid_json(self): |
573 | 80 | """No Juju message is sent if the JSON file is invalid.""" | 77 | """No Juju message is sent if the JSON file is invalid.""" |
574 | 81 | 78 | ||
575 | === modified file 'landscape/tests/test_deployment.py' | |||
576 | --- landscape/tests/test_deployment.py 2013-09-25 09:24:17 +0000 | |||
577 | +++ landscape/tests/test_deployment.py 2013-09-27 14:18:49 +0000 | |||
578 | @@ -560,7 +560,7 @@ | |||
579 | 560 | 560 | ||
580 | 561 | If a specific config file is requested, use this instead of defaults. | 561 | If a specific config file is requested, use this instead of defaults. |
581 | 562 | 562 | ||
583 | 563 | If a cmdilne --config option is specified this should take precedence | 563 | If a cmdline --config option is specified this should take precedence |
584 | 564 | over either of the former options. | 564 | over either of the former options. |
585 | 565 | """ | 565 | """ |
586 | 566 | default_filename1 = self.makeFile("") | 566 | default_filename1 = self.makeFile("") |
587 | @@ -599,8 +599,9 @@ | |||
588 | 599 | The L{Configuration.socket_path} property returns the path to the | 599 | The L{Configuration.socket_path} property returns the path to the |
589 | 600 | socket directory. | 600 | socket directory. |
590 | 601 | """ | 601 | """ |
593 | 602 | self.assertEqual(self.config.sockets_path, | 602 | self.assertEqual( |
594 | 603 | "/var/lib/landscape/client/sockets") | 603 | "/var/lib/landscape/client/sockets", |
595 | 604 | self.config.sockets_path) | ||
596 | 604 | 605 | ||
597 | 605 | def test_annotations_path(self): | 606 | def test_annotations_path(self): |
598 | 606 | """ | 607 | """ |
599 | @@ -611,6 +612,15 @@ | |||
600 | 611 | "/var/lib/landscape/client/annotations.d", | 612 | "/var/lib/landscape/client/annotations.d", |
601 | 612 | self.config.annotations_path) | 613 | self.config.annotations_path) |
602 | 613 | 614 | ||
603 | 615 | def test_juju_filename(self): | ||
604 | 616 | """ | ||
605 | 617 | The L{Configuration.juju_filename} property returns the path to the | ||
606 | 618 | juju info file. | ||
607 | 619 | """ | ||
608 | 620 | self.assertEqual( | ||
609 | 621 | "/var/lib/landscape/client/juju-info.json", | ||
610 | 622 | self.config.juju_filename) | ||
611 | 623 | |||
612 | 614 | def test_clone(self): | 624 | def test_clone(self): |
613 | 615 | """The L{Configuration.clone} method clones a configuration.""" | 625 | """The L{Configuration.clone} method clones a configuration.""" |
614 | 616 | self.write_config_file() | 626 | self.write_config_file() |
+1, looks good!
#1: on_info = {} on_info[ "environment- uuid"] = juju_info[ on_info[ "api-addresses" ] = juju_info[ ].split( ) on_info[ "unit-name" ] = juju_info[ "unit-name" ]
+ juju_registrati
+ juju_registrati
+ "environment-uuid"]
+ juju_registrati
+ "api-addresses"
+ juju_registrati
I think the juju_info[ "api-addresses" ].split( ) should me moved inside get_juju_info(), since there are currently two places where the split is performed after getting the info.
As a result this could we written as
+ juju_registrati on_info = dict( uuid", "api-addresses", "unit-name"])
+ (key, value) for key, value in juju_info
+ if key in ["environment-
#2:
+import os.path
Please use "from os import path" instead.
#3: LandscapeTest) :
+class JujuTest(
Tests in this class don't have docstrings.
#4: l("DEAD- BEEF", juju_info[ "environment- uuid"]) l("juju- unit-name" , juju_info[ "unit-name" ]) l("10.0. 3.1:17070" , juju_info[ "api-addresses" ]) l("127. 0.0.1", juju_info[ "private- address" ])
+ self.assertEqua
+ self.assertEqua
+ self.assertEqua
+ self.assertEqua
It'd be better to compare juju_info with an expected dict, to ensure there are no extra keys.