Merge lp:~frankban/juju-quickstart/maas-address into lp:juju-quickstart
- maas-address
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 103 |
Proposed branch: | lp:~frankban/juju-quickstart/maas-address |
Merge into: | lp:juju-quickstart |
Diff against target: |
1288 lines (+332/-433) 12 files modified
README.rst (+2/-0) quickstart/__init__.py (+1/-1) quickstart/app.py (+21/-31) quickstart/manage.py (+6/-11) quickstart/settings.py (+3/-0) quickstart/tests/helpers.py (+11/-0) quickstart/tests/test_app.py (+63/-183) quickstart/tests/test_manage.py (+19/-68) quickstart/tests/test_utils.py (+18/-0) quickstart/tests/test_watchers.py (+122/-108) quickstart/utils.py (+14/-0) quickstart/watchers.py (+52/-31) |
To merge this branch: | bzr merge lp:~frankban/juju-quickstart/maas-address |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju GUI Hackers | Pending | ||
Review via email: mp+241263@code.launchpad.net |
Commit message
Description of the change
Unit address from the machines watcher only
Only use the mega-watcher for machines to retrieve
the Juju GUI unit address.
This change has several consequences:
- it allows us to apply some logic on how the
right address is chosen. For instance, now
we try to resolve public hostnames before
proceeding, and this should fix the cases
where a cloud dns is not configured on the
machine running quickstart. This is the case
of many maas environments;
- it simplifies parsing the mega-watcher changes;
- more importantly, it breaks compatibility
with very old versions of juju (<1.18), in which
the mega-watcher for machines did not include
machine addresses.
For this reason, quickstart now explicitly
drops support for juju < 1.18.1
(1.18.1 is the version on trusty universe).
This also allows for removing some version
checks in the code, including sudo handling when
calling bootstrap on local envs, several special
cases on the watcher side, and other oddities.
For the reasons above, I bumped the quickstart
version up to 1.5.0.
PS: my apologies for the long diff, hope the code
is still easy to follow. Sorry.
Tests: `make check`
QA:
run quickstart as usual, on local and cloud envs,
check it works properly when run again, etc.
this branch has been already successfully QAed in
a maas environment by Adam (Landscape team).
Francesco Banconi (frankban) wrote : | # |
Brad Crittenden (bac) wrote : | # |
On 2014/11/10 12:30:34, frankban wrote:
> Please take a look.
LGTM. Have not done QA yet.
Brad Crittenden (bac) wrote : | # |
Richard Harding (rharding) wrote : | # |
LGTM ty for the updates!
https:/
File quickstart/app.py (left):
https:/
quickstart/
is this because of the new juju requirement?
Francesco Banconi (frankban) wrote : | # |
Thank you both for the reviews of this long diff!
https:/
File quickstart/app.py (left):
https:/
quickstart/
On 2014/11/10 15:25:20, rharding wrote:
> is this because of the new juju requirement?
Yes! We got rid of very old versions of juju requiring an explicit sudo
on the command line.
Francesco Banconi (frankban) wrote : | # |
*** Submitted:
Unit address from the machines watcher only
Only use the mega-watcher for machines to retrieve
the Juju GUI unit address.
This change has several consequences:
- it allows us to apply some logic on how the
right address is chosen. For instance, now
we try to resolve public hostnames before
proceeding, and this should fix the cases
where a cloud dns is not configured on the
machine running quickstart. This is the case
of many maas environments;
- it simplifies parsing the mega-watcher changes;
- more importantly, it breaks compatibility
with very old versions of juju (<1.18), in which
the mega-watcher for machines did not include
machine addresses.
For this reason, quickstart now explicitly
drops support for juju < 1.18.1
(1.18.1 is the version on trusty universe).
This also allows for removing some version
checks in the code, including sudo handling when
calling bootstrap on local envs, several special
cases on the watcher side, and other oddities.
For the reasons above, I bumped the quickstart
version up to 1.5.0.
PS: my apologies for the long diff, hope the code
is still easy to follow. Sorry.
Tests: `make check`
QA:
run quickstart as usual, on local and cloud envs,
check it works properly when run again, etc.
this branch has been already successfully QAed in
a maas environment by Adam (Landscape team).
R=bac, rharding
CC=
https:/
Preview Diff
1 | === modified file 'README.rst' |
2 | --- README.rst 2014-09-01 14:44:39 +0000 |
3 | +++ README.rst 2014-11-10 12:30:00 +0000 |
4 | @@ -36,6 +36,8 @@ |
5 | Juju Quickstart is available on Ubuntu releases 12.04 LTS (precise), 14.04 LTS |
6 | (trusty), 14.10 (utopic) and on OS X (10.7 and later). |
7 | |
8 | +Starting from version 1.5.0, Juju Quickstart only supports Juju >= 1.18.1. |
9 | + |
10 | Ubuntu Installation |
11 | ~~~~~~~~~~~~~~~~~~~ |
12 | |
13 | |
14 | === modified file 'quickstart/__init__.py' |
15 | --- quickstart/__init__.py 2014-10-01 13:43:17 +0000 |
16 | +++ quickstart/__init__.py 2014-11-10 12:30:00 +0000 |
17 | @@ -45,7 +45,7 @@ |
18 | Once Juju has been installed, the command can also be run as a juju plugin, |
19 | without the hyphen ("juju quickstart"). |
20 | """ |
21 | -VERSION = (1, 4, 4) |
22 | +VERSION = (1, 5, 0) |
23 | |
24 | |
25 | def get_version(): |
26 | |
27 | === modified file 'quickstart/app.py' |
28 | --- quickstart/app.py 2014-08-25 16:57:41 +0000 |
29 | +++ quickstart/app.py 2014-11-10 12:30:00 +0000 |
30 | @@ -116,6 +116,19 @@ |
31 | return juju_version |
32 | |
33 | |
34 | +def check_juju_supported(juju_version): |
35 | + """Ensure the given Juju version is supported by Quickstart. |
36 | + |
37 | + Raise a ProgramExit if Juju is not supported. |
38 | + """ |
39 | + if juju_version < settings.JUJU_SUPPORTED_VERSION: |
40 | + current = b'.'.join(map(bytes, juju_version)) |
41 | + supported = b'.'.join(map(bytes, settings.JUJU_SUPPORTED_VERSION)) |
42 | + raise ProgramExit( |
43 | + b'the current Juju version ({}) is not supported: ' |
44 | + b'please upgrade to Juju >= {}'.format(current, supported)) |
45 | + |
46 | + |
47 | def ensure_ssh_keys(): |
48 | """Ensure that SSH keys are available. |
49 | |
50 | @@ -163,8 +176,9 @@ |
51 | raise ProgramExit(bytes(err)) |
52 | |
53 | |
54 | -def bootstrap(env_name, juju_command, requires_sudo=False, debug=False, |
55 | - upload_tools=False, upload_series=None, constraints=None): |
56 | +def bootstrap( |
57 | + env_name, juju_command, debug=False, upload_tools=False, |
58 | + upload_series=None, constraints=None): |
59 | """Bootstrap the Juju environment with the given name. |
60 | |
61 | Do not bootstrap the environment if already bootstrapped. |
62 | @@ -185,8 +199,6 @@ |
63 | """ |
64 | already_bootstrapped = False |
65 | cmd = [juju_command, 'bootstrap', '-e', env_name] |
66 | - if requires_sudo: |
67 | - cmd.insert(0, 'sudo') |
68 | if debug: |
69 | cmd.append('--debug') |
70 | if upload_tools: |
71 | @@ -497,14 +509,10 @@ |
72 | for action, data in unit_changes: |
73 | if data['Name'] == unit_name: |
74 | try: |
75 | - data = watchers.parse_unit_change( |
76 | - action, data, unit_status, address) |
77 | + unit_status, machine_id = watchers.parse_unit_change( |
78 | + action, data, unit_status) |
79 | except ValueError as err: |
80 | raise ProgramExit(bytes(err)) |
81 | - unit_status, address, machine_id = data |
82 | - if address and unit_status == 'started': |
83 | - # We can exit this loop. |
84 | - return address |
85 | # The mega-watcher contains a single change for each specific |
86 | # unit. For this reason, we can exit the for loop here. |
87 | break |
88 | @@ -528,12 +536,12 @@ |
89 | action, data, machine_status, address) |
90 | except ValueError as err: |
91 | raise ProgramExit(bytes(err)) |
92 | - if address and unit_status == 'started': |
93 | - # We can exit this loop. |
94 | - return address |
95 | # The mega-watcher contains a single change for each specific |
96 | # machine. For this reason, we can exit the for loop here. |
97 | break |
98 | + if address and unit_status == 'started': |
99 | + # We can exit this loop. |
100 | + return address |
101 | |
102 | |
103 | def deploy_bundle(env, bundle_yaml, bundle_name, bundle_id): |
104 | @@ -549,21 +557,3 @@ |
105 | env.deploy_bundle(bundle_yaml, name=bundle_name, bundle_id=bundle_id) |
106 | except jujuclient.EnvError as err: |
107 | raise ProgramExit('bad API server response: {}'.format(err.message)) |
108 | - |
109 | - |
110 | -def juju_requires_sudo(env_type, juju_version, customized): |
111 | - """Report whether the given Juju version requires "sudo". |
112 | - |
113 | - The env_type argument is the current environment type. |
114 | - "e.g. local, ec2, azure" |
115 | - |
116 | - Raise a ProgramExit if a customized version of Juju is being used and Juju |
117 | - itself requires "sudo" |
118 | - """ |
119 | - # If the Juju version is less than 1.17.2 then use sudo for local envs. |
120 | - if env_type == 'local' and juju_version < (1, 17, 2): |
121 | - if customized: |
122 | - raise ProgramExit(b'cannot use a customized Juju when it ' |
123 | - 'requires sudo') |
124 | - return True |
125 | - return False |
126 | |
127 | === modified file 'quickstart/manage.py' |
128 | --- quickstart/manage.py 2014-08-25 16:57:41 +0000 |
129 | +++ quickstart/manage.py 2014-11-10 12:30:00 +0000 |
130 | @@ -502,9 +502,7 @@ |
131 | logging.debug('ensuring juju and dependencies are installed') |
132 | juju_version = app.ensure_dependencies( |
133 | options.distro_only, options.platform, juju_command) |
134 | - |
135 | - requires_sudo = app.juju_requires_sudo( |
136 | - options.env_type, juju_version, custom_juju) |
137 | + app.check_juju_supported(juju_version) |
138 | |
139 | logging.debug('ensuring SSH keys are available') |
140 | app.ensure_ssh_keys() |
141 | @@ -513,13 +511,12 @@ |
142 | options.env_name, options.env_type)) |
143 | if options.env_type == 'local': |
144 | # If this is a local environment, notify the user that "sudo" will be |
145 | - # required to bootstrap the application, even in newer Juju versions |
146 | - # where "sudo" is invoked by juju-core itself. |
147 | + # required by Juju to bootstrap the application. |
148 | print('sudo privileges will be required to bootstrap the environment') |
149 | |
150 | already_bootstrapped, bootstrap_node_series = app.bootstrap( |
151 | options.env_name, juju_command, |
152 | - requires_sudo=requires_sudo, debug=options.debug, |
153 | + debug=options.debug, |
154 | upload_tools=options.upload_tools, |
155 | upload_series=options.upload_series, |
156 | constraints=options.constraints) |
157 | @@ -585,9 +582,7 @@ |
158 | 'Run "juju quickstart -i" if you want to manage\n' |
159 | 'or bootstrap your Juju environments using the\n' |
160 | 'interactive session.\n' |
161 | - 'Run "{sudo}juju destroy-environment {eflag}{env_name} [-y]"\n' |
162 | - 'to destroy the environment you just bootstrapped.'.format( |
163 | - env_name=options.env_name, |
164 | - sudo='sudo ' if requires_sudo else '', |
165 | - eflag='-e ' if juju_version < (1, 17, 2) else '') |
166 | + 'Run "juju destroy-environment {env_name} [-y]"\n' |
167 | + 'to destroy the environment you just bootstrapped.' |
168 | + ''.format(env_name=options.env_name) |
169 | ) |
170 | |
171 | === modified file 'quickstart/settings.py' |
172 | --- quickstart/settings.py 2014-10-09 13:14:30 +0000 |
173 | +++ quickstart/settings.py 2014-11-10 12:30:00 +0000 |
174 | @@ -76,6 +76,9 @@ |
175 | # The set of series supported by the Juju GUI charm. |
176 | JUJU_GUI_SUPPORTED_SERIES = tuple(DEFAULT_CHARM_URLS.keys()) |
177 | |
178 | +# The minimum Juju version supported by Juju Quickstart, |
179 | +JUJU_SUPPORTED_VERSION = (1, 18, 1) |
180 | + |
181 | # The path to the MAAS command line interface. |
182 | MAAS_CMD = '/usr/bin/maas' |
183 | |
184 | |
185 | === modified file 'quickstart/tests/helpers.py' |
186 | --- quickstart/tests/helpers.py 2014-09-08 17:49:51 +0000 |
187 | +++ quickstart/tests/helpers.py 2014-11-10 12:30:00 +0000 |
188 | @@ -195,6 +195,17 @@ |
189 | mock_print = mock.patch('__builtin__.print') |
190 | |
191 | |
192 | +def patch_check_resolvable(error=None): |
193 | + """Patch the utils.check_resolvable function to return the given error. |
194 | + |
195 | + This is done so that tests do not try to resolve hostname addresses. |
196 | + """ |
197 | + return mock.patch( |
198 | + 'quickstart.utils.check_resolvable', |
199 | + lambda hostname: error, |
200 | + ) |
201 | + |
202 | + |
203 | class UrlReadTestsMixin(object): |
204 | """Expose a method to mock the quickstart.utils.urlread helper function.""" |
205 | |
206 | |
207 | === modified file 'quickstart/tests/test_app.py' |
208 | --- quickstart/tests/test_app.py 2014-08-25 16:57:41 +0000 |
209 | +++ quickstart/tests/test_app.py 2014-11-10 12:30:00 +0000 |
210 | @@ -298,6 +298,28 @@ |
211 | self.assertEqual(3, mock_call.call_count) |
212 | |
213 | |
214 | +class TestCheckJujuSupported(ProgramExitTestsMixin, unittest.TestCase): |
215 | + |
216 | + supported_versions = [(1, 18, 1), (1, 19, 0), (1, 42, 47), (2, 0, 0)] |
217 | + unsupported_versions = [(1, 18, 0), (1, 17, 42), (1, 0, 0), (0, 20, 47)] |
218 | + |
219 | + def test_supported(self): |
220 | + # No exceptions are raised if the Juju version is supported. |
221 | + for version in self.supported_versions: |
222 | + app.check_juju_supported(version) |
223 | + |
224 | + def test_not_supported(self): |
225 | + # A ProgramExit error is raised if the Juju version is not supported. |
226 | + error = ( |
227 | + 'the current Juju version ({}.{}.{}) is not supported: ' |
228 | + 'please upgrade to Juju >= ' + |
229 | + '.'.join(map(str, settings.JUJU_SUPPORTED_VERSION)) |
230 | + ) |
231 | + for version in self.unsupported_versions: |
232 | + with self.assert_program_exit(error.format(*version)): |
233 | + app.check_juju_supported(version) |
234 | + |
235 | + |
236 | @helpers.mock_print |
237 | class TestEnsureSSHKeys( |
238 | helpers.CallTestsMixin, ProgramExitTestsMixin, unittest.TestCase): |
239 | @@ -484,19 +506,6 @@ |
240 | ] + self.make_status_calls(1)) |
241 | mock_print.assert_called_once_with(self.status_message) |
242 | |
243 | - def test_success_local_provider(self, mock_print): |
244 | - # The environment is bootstrapped with sudo using the local provider. |
245 | - with self.patch_multiple_calls(self.make_side_effects()) as mock_call: |
246 | - already_bootstrapped, series = app.bootstrap( |
247 | - self.env_name, self.juju_command, requires_sudo=True) |
248 | - self.assertFalse(already_bootstrapped) |
249 | - self.assertEqual(series, 'hoary') |
250 | - mock_call.assert_has_calls([ |
251 | - mock.call( |
252 | - 'sudo', self.juju_command, 'bootstrap', '-e', self.env_name), |
253 | - ] + self.make_status_calls(1)) |
254 | - mock_print.assert_called_once_with(self.status_message) |
255 | - |
256 | def test_success_debug(self, mock_print): |
257 | # The environment is successfully bootstrapped in debug mode. |
258 | with self.patch_multiple_calls(self.make_side_effects()) as mock_call: |
259 | @@ -1275,16 +1284,16 @@ |
260 | |
261 | |
262 | @helpers.mock_print |
263 | +@helpers.patch_check_resolvable() |
264 | class TestWatch( |
265 | ProgramExitTestsMixin, helpers.ValueErrorTestsMixin, |
266 | unittest.TestCase): |
267 | |
268 | address = 'unit.example.com' |
269 | machine_pending_call = mock.call('machine 0 provisioning is pending') |
270 | - unit_placed_machine_call = mock.call('unit placed on unit.example.com') |
271 | + unit_placed_call = mock.call('unit placed on {}'.format(address)) |
272 | machine_started_call = mock.call('machine 0 is started') |
273 | unit_pending_call = mock.call('django/42 deployment is pending') |
274 | - unit_placed_unit_call = mock.call('django/42 placed on {}'.format(address)) |
275 | unit_installed_call = mock.call('django/42 is installed') |
276 | unit_started_call = mock.call('django/42 is ready on machine 0') |
277 | |
278 | @@ -1314,16 +1323,9 @@ |
279 | }] |
280 | return 'change', data |
281 | |
282 | - def make_unit_change(self, status, name='django/42', address=None): |
283 | - """Create and return a unit change. |
284 | - |
285 | - If the address argument is None, the change does not include the |
286 | - corresponding address field. |
287 | - """ |
288 | - data = {'MachineId': '0', 'Name': name, 'Status': status} |
289 | - if address is not None: |
290 | - data['PublicAddress'] = address |
291 | - return 'change', data |
292 | + def make_unit_change(self, status, name='django/42'): |
293 | + """Create and return a unit change.""" |
294 | + return 'change', {'MachineId': '0', 'Name': name, 'Status': status} |
295 | |
296 | # The following group of tests exercises both the function return value and |
297 | # the function output, even if the output is handled by sub-functions. |
298 | @@ -1334,35 +1336,10 @@ |
299 | # The glorious moments in the unit's life are properly highlighted. |
300 | # The machine achievements are also celebrated. |
301 | env = self.make_env([ |
302 | - ([self.make_unit_change('pending', address='')], |
303 | + ([self.make_unit_change('pending')], |
304 | [self.make_machine_change('pending')]), |
305 | ([], [self.make_machine_change('started')]), |
306 | - ([self.make_unit_change('pending', address=self.address)], []), |
307 | - ([self.make_unit_change('installed', address=self.address)], []), |
308 | - ([self.make_unit_change('started', address=self.address)], []), |
309 | - ]) |
310 | - address = app.watch(env, 'django/42') |
311 | - self.assertEqual(self.address, address) |
312 | - self.assertEqual(6, mock_print.call_count) |
313 | - mock_print.assert_has_calls([ |
314 | - self.unit_pending_call, |
315 | - self.machine_pending_call, |
316 | - self.machine_started_call, |
317 | - self.unit_placed_unit_call, |
318 | - self.unit_installed_call, |
319 | - self.unit_started_call, |
320 | - ]) |
321 | - |
322 | - def test_unit_life_with_machine_address(self, mock_print): |
323 | - # The glorious moments in the unit's life are properly highlighted. |
324 | - # The machine achievements are also celebrated. |
325 | - # This time the new mega-watcher behavior is simulated, in which |
326 | - # addresses are included in the machine change. |
327 | - env = self.make_env([ |
328 | - ([self.make_unit_change('pending')], |
329 | - [self.make_machine_change('pending', address='')]), |
330 | ([], [self.make_machine_change('started', address=self.address)]), |
331 | - ([self.make_unit_change('pending')], []), |
332 | ([self.make_unit_change('installed')], []), |
333 | ([self.make_unit_change('started')], []), |
334 | ]) |
335 | @@ -1372,8 +1349,8 @@ |
336 | mock_print.assert_has_calls([ |
337 | self.unit_pending_call, |
338 | self.machine_pending_call, |
339 | - self.unit_placed_machine_call, |
340 | self.machine_started_call, |
341 | + self.unit_placed_call, |
342 | self.unit_installed_call, |
343 | self.unit_started_call, |
344 | ]) |
345 | @@ -1382,21 +1359,23 @@ |
346 | # Strange unit evolutions are handled. |
347 | env = self.make_env([ |
348 | # The unit is first reachable and then pending. The machine starts |
349 | - # when the unit is already installed. All of this makes no sense |
350 | - # and should never happen, but if it does, we deal with it. |
351 | - ([self.make_unit_change('pending', address=self.address)], []), |
352 | - ([self.make_unit_change('pending', address='')], |
353 | - [self.make_machine_change('pending')]), |
354 | - ([self.make_unit_change('installed', address=self.address)], []), |
355 | - ([], [self.make_machine_change('started')]), |
356 | - ([self.make_unit_change('started', address=self.address)], []), |
357 | + # when the unit is already installed. The machine has an address |
358 | + # when still provisioning. All of this makes no sense and should |
359 | + # never happen, but if it does, we deal with it. |
360 | + ([self.make_unit_change('pending')], []), |
361 | + ([], [self.make_machine_change('pending', address=self.address)]), |
362 | + ([self.make_unit_change('pending')], |
363 | + [self.make_machine_change('pending', address='')]), |
364 | + ([self.make_unit_change('installed')], []), |
365 | + ([], [self.make_machine_change('started', address=self.address)]), |
366 | + ([self.make_unit_change('started')], []), |
367 | ]) |
368 | address = app.watch(env, 'django/42') |
369 | self.assertEqual(self.address, address) |
370 | self.assertEqual(6, mock_print.call_count) |
371 | mock_print.assert_has_calls([ |
372 | - self.unit_placed_unit_call, |
373 | self.unit_pending_call, |
374 | + self.unit_placed_call, |
375 | self.machine_pending_call, |
376 | self.unit_installed_call, |
377 | self.machine_started_call, |
378 | @@ -1404,22 +1383,8 @@ |
379 | ]) |
380 | |
381 | def test_missing_changes(self, mock_print): |
382 | - # Only the unit started change is strictly required when the unit |
383 | - # change includes the public address. |
384 | - env = self.make_env([ |
385 | - ([self.make_unit_change('started', address=self.address)], []), |
386 | - ]) |
387 | - address = app.watch(env, 'django/42') |
388 | - self.assertEqual(self.address, address) |
389 | - self.assertEqual(2, mock_print.call_count) |
390 | - mock_print.assert_has_calls([ |
391 | - self.unit_placed_unit_call, |
392 | - self.unit_started_call, |
393 | - ]) |
394 | - |
395 | - def test_missing_changes_with_machine_address(self, mock_print): |
396 | - # When using the new mega-watcher, a machine change including its |
397 | - # public address is also required. |
398 | + # Only the unit started change is strictly required when the machine |
399 | + # change includes the addresses. |
400 | env = self.make_env([ |
401 | ([self.make_unit_change('started')], []), |
402 | ([], [self.make_machine_change('started', address=self.address)]), |
403 | @@ -1429,29 +1394,12 @@ |
404 | self.assertEqual(3, mock_print.call_count) |
405 | mock_print.assert_has_calls([ |
406 | self.unit_started_call, |
407 | - self.unit_placed_machine_call, |
408 | + self.unit_placed_call, |
409 | self.machine_started_call, |
410 | ]) |
411 | |
412 | def test_ignored_machine_changes(self, mock_print): |
413 | # All machine changes are ignored until the application knows what |
414 | - # machine the unit belongs to. |
415 | - env = self.make_env([ |
416 | - ([], [self.make_machine_change('pending')]), |
417 | - ([], [self.make_machine_change('started')]), |
418 | - ([self.make_unit_change('started', address=self.address)], []), |
419 | - ]) |
420 | - address = app.watch(env, 'django/42') |
421 | - self.assertEqual(self.address, address) |
422 | - # No machine related messages have been printed. |
423 | - self.assertEqual(2, mock_print.call_count) |
424 | - mock_print.assert_has_calls([ |
425 | - self.unit_placed_unit_call, |
426 | - self.unit_started_call, |
427 | - ]) |
428 | - |
429 | - def test_ignored_machine_changes_with_machine_address(self, mock_print): |
430 | - # All machine changes are ignored until the application knows what |
431 | # machine the unit belongs to. When the above happens, previously |
432 | # collected machine changes are still parsed in the case the address |
433 | # is not yet known. |
434 | @@ -1468,7 +1416,7 @@ |
435 | self.assertEqual(3, mock_print.call_count) |
436 | mock_print.assert_has_calls([ |
437 | self.unit_started_call, |
438 | - self.unit_placed_machine_call, |
439 | + self.unit_placed_call, |
440 | self.machine_started_call, |
441 | ]) |
442 | |
443 | @@ -1477,20 +1425,6 @@ |
444 | # This happens, e.g., when executing Quickstart a second time, and both |
445 | # the unit and the machine are already started. |
446 | env = self.make_env([ |
447 | - ([self.make_unit_change('started', address=self.address)], |
448 | - [self.make_machine_change('started')]), |
449 | - ]) |
450 | - address = app.watch(env, 'django/42') |
451 | - self.assertEqual(self.address, address) |
452 | - self.assertEqual(2, mock_print.call_count) |
453 | - |
454 | - def test_unit_already_deployed_with_machine_address(self, mock_print): |
455 | - # Simulate the unit we are observing has been already deployed. |
456 | - # This happens, e.g., when executing Quickstart a second time, and both |
457 | - # the unit and the machine are already started. |
458 | - # This time the new mega-watcher behavior is simulated, in which |
459 | - # addresses are included in the machine change. |
460 | - env = self.make_env([ |
461 | ([self.make_unit_change('started')], |
462 | [self.make_machine_change('started', address=self.address)]), |
463 | ]) |
464 | @@ -1499,7 +1433,7 @@ |
465 | self.assertEqual(3, mock_print.call_count) |
466 | mock_print.assert_has_calls([ |
467 | self.unit_started_call, |
468 | - self.unit_placed_machine_call, |
469 | + self.unit_placed_call, |
470 | self.machine_started_call, |
471 | ]) |
472 | |
473 | @@ -1509,31 +1443,6 @@ |
474 | # environment type: the unit is deployed on the bootstrap node, which |
475 | # is assumed to be started. |
476 | env = self.make_env([ |
477 | - ([self.make_unit_change('pending', address='')], |
478 | - [self.make_machine_change('started')]), |
479 | - ([self.make_unit_change('pending', address=self.address)], []), |
480 | - ([self.make_unit_change('installed', address=self.address)], []), |
481 | - ([self.make_unit_change('started', address=self.address)], []), |
482 | - ]) |
483 | - address = app.watch(env, 'django/42') |
484 | - self.assertEqual(self.address, address) |
485 | - self.assertEqual(5, mock_print.call_count) |
486 | - mock_print.assert_has_calls([ |
487 | - self.unit_pending_call, |
488 | - self.machine_started_call, |
489 | - self.unit_placed_unit_call, |
490 | - self.unit_installed_call, |
491 | - self.unit_started_call, |
492 | - ]) |
493 | - |
494 | - def test_machine_already_started_with_machine_address(self, mock_print): |
495 | - # Simulate the unit is being deployed on an already started machine. |
496 | - # This happens, e.g., when running Quickstart on a non-local |
497 | - # environment type: the unit is deployed on the bootstrap node, which |
498 | - # is assumed to be started. |
499 | - # This time the new mega-watcher behavior is simulated, in which |
500 | - # addresses are included in the machine change. |
501 | - env = self.make_env([ |
502 | ([self.make_unit_change('pending')], |
503 | [self.make_machine_change('started', address=self.address)]), |
504 | ([self.make_unit_change('pending')], []), |
505 | @@ -1545,7 +1454,7 @@ |
506 | self.assertEqual(5, mock_print.call_count) |
507 | mock_print.assert_has_calls([ |
508 | self.unit_pending_call, |
509 | - self.unit_placed_machine_call, |
510 | + self.unit_placed_call, |
511 | self.machine_started_call, |
512 | self.unit_installed_call, |
513 | self.unit_started_call, |
514 | @@ -1555,24 +1464,26 @@ |
515 | # Changes to units or machines we are not observing are ignored. Also |
516 | # ensure that repeated changes to a single entity are ignored, even if |
517 | # they are unlikely to happen. |
518 | - pending_unit_change = self.make_unit_change('pending', address='') |
519 | - started_unit_change = self.make_unit_change( |
520 | - 'started', address=self.address) |
521 | + pending_unit_change = self.make_unit_change('pending') |
522 | + started_unit_change = self.make_unit_change('started') |
523 | + pending_machine_change = self.make_machine_change('pending') |
524 | env = self.make_env([ |
525 | - # Add a repeated change. |
526 | + # Add a repeated unit change. |
527 | ([pending_unit_change, pending_unit_change], |
528 | - [self.make_machine_change('pending')]), |
529 | + [pending_machine_change]), |
530 | # Add extraneous unit and machine changes. |
531 | ([self.make_unit_change('pending', name='haproxy/0')], |
532 | [self.make_machine_change('pending', name='42')]), |
533 | + # Add a repeated machine change. |
534 | + ([], [pending_machine_change, pending_machine_change]), |
535 | # Add a change to an extraneous machine. |
536 | ([], [self.make_machine_change('started', name='42'), |
537 | - self.make_machine_change('started')]), |
538 | + self.make_machine_change('started', address=self.address)]), |
539 | # Add a change to an extraneous unit. |
540 | ([self.make_unit_change('started', name='haproxy/0'), |
541 | - self.make_unit_change('pending', address=self.address)], []), |
542 | - ([self.make_unit_change('installed', address=self.address)], []), |
543 | - # Add another repeated change. |
544 | + self.make_unit_change('pending')], []), |
545 | + ([self.make_unit_change('installed')], []), |
546 | + # Add another repeated unit change. |
547 | ([started_unit_change, started_unit_change], []), |
548 | ]) |
549 | address = app.watch(env, 'django/42') |
550 | @@ -1581,8 +1492,8 @@ |
551 | mock_print.assert_has_calls([ |
552 | self.unit_pending_call, |
553 | self.machine_pending_call, |
554 | + self.unit_placed_call, |
555 | self.machine_started_call, |
556 | - self.unit_placed_unit_call, |
557 | self.unit_installed_call, |
558 | self.unit_started_call, |
559 | ]) |
560 | @@ -1590,7 +1501,7 @@ |
561 | def test_api_error(self, mock_print): |
562 | # A ProgramExit is raised if an error occurs in one of the API calls. |
563 | env = self.make_env([ |
564 | - ([self.make_unit_change('pending', address='')], []), |
565 | + ([self.make_unit_change('pending')], []), |
566 | self.make_env_error('next returned an error'), |
567 | ]) |
568 | expected = 'bad API server response: next returned an error' |
569 | @@ -1602,14 +1513,13 @@ |
570 | def test_other_errors(self, mock_print): |
571 | # Any other errors occurred during the process are not trapped. |
572 | env = self.make_env([ |
573 | - ([self.make_unit_change('installed', address=self.address)], []), |
574 | + ([self.make_unit_change('installed')], []), |
575 | ValueError('explode!'), |
576 | ]) |
577 | with self.assert_value_error('explode!'): |
578 | app.watch(env, 'django/42') |
579 | - self.assertEqual(2, mock_print.call_count) |
580 | - mock_print.assert_has_calls([ |
581 | - self.unit_placed_unit_call, self.unit_installed_call]) |
582 | + self.assertEqual(1, mock_print.call_count) |
583 | + mock_print.assert_has_calls([self.unit_installed_call]) |
584 | |
585 | def test_machine_status_error(self, mock_print): |
586 | # A ProgramExit is raised if an the machine is found in an error state. |
587 | @@ -1622,8 +1532,7 @@ |
588 | # The unit pending change is required to make the function know which |
589 | # machine to observe. |
590 | env = self.make_env([ |
591 | - ([self.make_unit_change('pending', address='')], |
592 | - [change_machine_error]), |
593 | + ([self.make_unit_change('pending')], [change_machine_error]), |
594 | ]) |
595 | expected = 'machine 0 is in an error state: error: oddities' |
596 | with self.assert_program_exit(expected): |
597 | @@ -1677,32 +1586,3 @@ |
598 | with self.assertRaises(ValueError) as context_manager: |
599 | app.deploy_bundle(env, self.yaml, self.name, None) |
600 | self.assertIs(error, context_manager.exception) |
601 | - |
602 | - |
603 | -class TestJujuRequiresSudo(ProgramExitTestsMixin, unittest.TestCase): |
604 | - no_sudo_versions = [ |
605 | - (1, 17, 2), (1, 17, 10), (1, 18, 0), (1, 18, 2), (2, 16, 1)] |
606 | - sudo_versions = [(0, 7, 9), (1, 0, 0), (1, 16, 42), (1, 17, 0), (1, 17, 1)] |
607 | - |
608 | - def test_non_local_returns_false(self): |
609 | - # A non-local provider does not require sudo. |
610 | - actual = app.juju_requires_sudo('aws', None, None) |
611 | - self.assertFalse(actual) |
612 | - |
613 | - def test_local_old_juju_returns_true(self): |
614 | - # A juju previous to 1.17.2 requires sudo. |
615 | - for version in self.sudo_versions: |
616 | - self.assertTrue(app.juju_requires_sudo('local', version, False), |
617 | - version) |
618 | - |
619 | - def test_local_newer_juju_returns_false(self): |
620 | - # A juju 1.17.2 and newer does not require sudo. |
621 | - for version in self.no_sudo_versions: |
622 | - self.assertFalse(app.juju_requires_sudo('local', version, False), |
623 | - version) |
624 | - |
625 | - def test_raises_programexit(self): |
626 | - for version in self.sudo_versions: |
627 | - with self.assert_program_exit('cannot use a customized Juju when ' |
628 | - 'it requires sudo'): |
629 | - app.juju_requires_sudo('local', version, True) |
630 | |
631 | === modified file 'quickstart/tests/test_manage.py' |
632 | --- quickstart/tests/test_manage.py 2014-08-25 16:57:41 +0000 |
633 | +++ quickstart/tests/test_manage.py 2014-11-10 12:30:00 +0000 |
634 | @@ -795,7 +795,6 @@ |
635 | mock_app.watch.return_value = '1.2.3.4' |
636 | # Make mock_app.create_auth_token return a fake authentication token. |
637 | mock_app.create_auth_token.return_value = 'AUTHTOKEN' |
638 | - mock_app.juju_requires_sudo.return_value = False |
639 | options = self.make_options() |
640 | with mock.patch('quickstart.manage.platform_support.get_juju_command', |
641 | side_effect=[(self.juju_command, False)]): |
642 | @@ -803,8 +802,9 @@ |
643 | mock_app.ensure_dependencies.assert_called() |
644 | mock_app.ensure_ssh_keys.assert_called() |
645 | mock_app.bootstrap.assert_called_once_with( |
646 | - options.env_name, self.juju_command, requires_sudo=False, |
647 | - debug=options.debug, upload_tools=options.upload_tools, |
648 | + options.env_name, self.juju_command, |
649 | + debug=options.debug, |
650 | + upload_tools=options.upload_tools, |
651 | upload_series=options.upload_series, |
652 | constraints=options.constraints) |
653 | mock_app.get_api_url.assert_called_once_with( |
654 | @@ -879,68 +879,19 @@ |
655 | # where to deploy the charm, the service and unit data. |
656 | mock_app.check_environment.return_value = ( |
657 | 'cs:precise/juju-gui-42', '0', None, None) |
658 | - mock_app.juju_requires_sudo.return_value = False |
659 | - for version in versions: |
660 | - mock_app.ensure_dependencies.return_value = version |
661 | - with mock.patch( |
662 | - 'quickstart.manage.platform_support.get_juju_command', |
663 | - side_effect=[(self.juju_command, False)]): |
664 | - manage.run(options) |
665 | - mock_app.bootstrap.assert_called_once_with( |
666 | - options.env_name, self.juju_command, requires_sudo=False, |
667 | - debug=options.debug, upload_tools=options.upload_tools, |
668 | - upload_series=options.upload_series, |
669 | - constraints=options.constraints) |
670 | - mock_app.bootstrap.reset_mock() |
671 | - |
672 | - def test_local_provider_requiring_sudo(self, mock_app, mock_open): |
673 | - # The application correctly handles working with local providers when |
674 | - # Juju requires an external "sudo" call to bootstrap the environment. |
675 | - # Sudo privileges are required if the Juju version is < 1.17.2. |
676 | - options = self.make_options(env_type='local') |
677 | - versions = [(0, 7, 9), (1, 0, 0), (1, 16, 42), (1, 17, 0), (1, 17, 1)] |
678 | - # Make mock_app.bootstrap return the already_bootstrapped flag and the |
679 | - # bootstrap node series. |
680 | - mock_app.bootstrap.return_value = (True, 'trusty') |
681 | - # Make mock_app.check_environment return the charm URL, the machine |
682 | - # where to deploy the charm, the service and unit data. |
683 | - mock_app.check_environment.return_value = ( |
684 | - 'cs:trusty/juju-gui-42', '0', None, None) |
685 | - mock_app.juju_requires_sudo.return_value = True |
686 | - for version in versions: |
687 | - mock_app.ensure_dependencies.return_value = version |
688 | - with mock.patch( |
689 | - 'quickstart.manage.platform_support.get_juju_command', |
690 | - side_effect=[(self.juju_command, False)]): |
691 | - manage.run(options) |
692 | - mock_app.bootstrap.assert_called_once_with( |
693 | - options.env_name, self.juju_command, requires_sudo=True, |
694 | - debug=options.debug, upload_tools=options.upload_tools, |
695 | - upload_series=options.upload_series, |
696 | - constraints=options.constraints) |
697 | - mock_app.bootstrap.reset_mock() |
698 | - |
699 | - def test_no_local_no_sudo(self, mock_app, mock_open): |
700 | - # Sudo privileges are never required for non-local environments. |
701 | - options = self.make_options(env_type='ec2') |
702 | - mock_app.ensure_dependencies.return_value = (1, 14, 0) |
703 | - # Make mock_app.bootstrap return the already_bootstrapped flag and the |
704 | - # bootstrap node series. |
705 | - mock_app.bootstrap.return_value = (True, 'precise') |
706 | - # Make mock_app.check_environment return the charm URL, the machine |
707 | - # where to deploy the charm, the service and unit data. |
708 | - mock_app.check_environment.return_value = ( |
709 | - 'cs:precise/juju-gui-42', '0', None, None) |
710 | - mock_app.juju_requires_sudo.return_value = False |
711 | - with mock.patch('quickstart.manage.platform_support.get_juju_command', |
712 | - side_effect=[(self.juju_command, False)]): |
713 | - manage.run(options) |
714 | - mock_app.bootstrap.assert_called_once_with( |
715 | - options.env_name, self.juju_command, |
716 | - requires_sudo=False, |
717 | - debug=options.debug, upload_tools=options.upload_tools, |
718 | - upload_series=options.upload_series, |
719 | - constraints=options.constraints) |
720 | + for version in versions: |
721 | + mock_app.ensure_dependencies.return_value = version |
722 | + with mock.patch( |
723 | + 'quickstart.manage.platform_support.get_juju_command', |
724 | + side_effect=[(self.juju_command, False)]): |
725 | + manage.run(options) |
726 | + mock_app.bootstrap.assert_called_once_with( |
727 | + options.env_name, self.juju_command, |
728 | + debug=options.debug, |
729 | + upload_tools=options.upload_tools, |
730 | + upload_series=options.upload_series, |
731 | + constraints=options.constraints) |
732 | + mock_app.bootstrap.reset_mock() |
733 | |
734 | def test_no_browser(self, mock_app, mock_open): |
735 | # It is possible to avoid opening the GUI in the browser. |
736 | @@ -1015,14 +966,14 @@ |
737 | mock_app.bootstrap.return_value = (True, 'precise') |
738 | mock_app.check_environment.return_value = ( |
739 | 'cs:precise/juju-gui-42', '0', None, None) |
740 | - mock_app.juju_requires_sudo.return_value = False |
741 | options = self.make_options(env_type='aws') |
742 | juju_command = 'some/devel/path/juju' |
743 | with mock.patch('os.environ', {'JUJU': juju_command}): |
744 | manage.run(options) |
745 | mock_app.bootstrap.assert_called_once_with( |
746 | - options.env_name, juju_command, requires_sudo=False, |
747 | - debug=options.debug, upload_tools=options.upload_tools, |
748 | + options.env_name, juju_command, |
749 | + debug=options.debug, |
750 | + upload_tools=options.upload_tools, |
751 | upload_series=options.upload_series, |
752 | constraints=options.constraints) |
753 | mock_app.get_api_url.assert_called_once_with( |
754 | |
755 | === modified file 'quickstart/tests/test_utils.py' |
756 | --- quickstart/tests/test_utils.py 2014-08-22 16:38:23 +0000 |
757 | +++ quickstart/tests/test_utils.py 2014-11-10 12:30:00 +0000 |
758 | @@ -156,6 +156,24 @@ |
759 | utils.call('echo', 'we are the borg!') |
760 | |
761 | |
762 | +class TestCheckResolvable(unittest.TestCase): |
763 | + |
764 | + def test_resolvable(self): |
765 | + # None is returned if the hostname can be resolved. |
766 | + expected_log = 'example.com resolved to 1.2.3.4' |
767 | + with helpers.assert_logs([expected_log], level='debug'): |
768 | + with mock.patch('socket.gethostbyname', return_value='1.2.3.4'): |
769 | + error = utils.check_resolvable('example.com') |
770 | + self.assertIsNone(error) |
771 | + |
772 | + def test_not_resolvable(self): |
773 | + # An error message is returned if the hostname cannot be resolved. |
774 | + exception = socket.gaierror('bad wolf') |
775 | + with mock.patch('socket.gethostbyname', side_effect=exception): |
776 | + error = utils.check_resolvable('example.com') |
777 | + self.assertEqual('bad wolf', error) |
778 | + |
779 | + |
780 | @mock.patch('__builtin__.print', mock.Mock()) |
781 | class TestParseGuiCharmUrl(unittest.TestCase): |
782 | |
783 | |
784 | === modified file 'quickstart/tests/test_watchers.py' |
785 | --- quickstart/tests/test_watchers.py 2014-07-04 13:09:11 +0000 |
786 | +++ quickstart/tests/test_watchers.py 2014-11-10 12:30:00 +0000 |
787 | @@ -63,23 +63,29 @@ |
788 | |
789 | class TestRetrievePublicAddress(unittest.TestCase): |
790 | |
791 | + def resolver(self, hostname): |
792 | + """A fake resolver returning no errors.""" |
793 | + return None |
794 | + |
795 | def test_empty_addresses(self): |
796 | # None is returned if there are no available addresses. |
797 | - self.assertIsNone(watchers.retrieve_public_adddress([])) |
798 | + address = watchers.retrieve_public_adddress([], self.resolver) |
799 | + self.assertIsNone(address) |
800 | |
801 | def test_cloud_address_not_found(self): |
802 | # None is returned if a cloud machine public address is not available. |
803 | addresses = [ |
804 | {'NetworkName': '', |
805 | - 'Scope': 'local-cloud', |
806 | - 'Type': 'hostname', |
807 | - 'Value': 'eu-west-1.example.internal'}, |
808 | + 'Scope': 'public', |
809 | + 'Type': 'ipv6', |
810 | + 'Value': 'fe80::92b8:d0ff:fe94:8f8c'}, |
811 | {'NetworkName': '', |
812 | 'Scope': 'local-cloud', |
813 | - 'Type': 'ipv4', |
814 | - 'Value': '10.42.47.10'}, |
815 | + 'Type': 'ipv6', |
816 | + 'Value': 'fe80::216:3eff:fefd:787e'}, |
817 | ] |
818 | - self.assertIsNone(watchers.retrieve_public_adddress(addresses)) |
819 | + address = watchers.retrieve_public_adddress(addresses, self.resolver) |
820 | + self.assertIsNone(address) |
821 | |
822 | def test_container_address_not_found(self): |
823 | # None is returned if an LXC public address is not available. |
824 | @@ -89,7 +95,8 @@ |
825 | 'Type': 'ipv6', |
826 | 'Value': 'fe80::216:3eff:fefd:787e', |
827 | }] |
828 | - self.assertIsNone(watchers.retrieve_public_adddress(addresses)) |
829 | + address = watchers.retrieve_public_adddress(addresses, self.resolver) |
830 | + self.assertIsNone(address) |
831 | |
832 | def test_empty_public_address(self): |
833 | # None is returned if the public address has no value. |
834 | @@ -103,17 +110,20 @@ |
835 | 'Type': 'ipv4', |
836 | 'Value': ''}, |
837 | ] |
838 | - self.assertIsNone(watchers.retrieve_public_adddress(addresses)) |
839 | + address = watchers.retrieve_public_adddress(addresses, self.resolver) |
840 | + self.assertIsNone(address) |
841 | |
842 | def test_cloud_addresses(self): |
843 | # The public address of a cloud machine is properly returned. |
844 | - public_address = watchers.retrieve_public_adddress(cloud_addresses) |
845 | - self.assertEqual('eu-west-1.compute.example.com', public_address) |
846 | + address = watchers.retrieve_public_adddress( |
847 | + cloud_addresses, self.resolver) |
848 | + self.assertEqual('eu-west-1.compute.example.com', address) |
849 | |
850 | def test_container_addresses(self): |
851 | # The public address of an LXC instance is properly returned. |
852 | - public_address = watchers.retrieve_public_adddress(container_addresses) |
853 | - self.assertEqual('10.0.3.42', public_address) |
854 | + address = watchers.retrieve_public_adddress( |
855 | + container_addresses, self.resolver) |
856 | + self.assertEqual('10.0.3.42', address) |
857 | |
858 | def test_old_juju_version(self): |
859 | # The public address is properly returned when using Juju < 1.20. |
860 | @@ -129,11 +139,32 @@ |
861 | 'Type': 'hostname', |
862 | 'Value': 'eu-west-1.example.internal'}, |
863 | ] |
864 | - public_address = watchers.retrieve_public_adddress(addresses) |
865 | - self.assertEqual('eu-west-1.compute.example.com', public_address) |
866 | - |
867 | - def test_last_unknown_address(self): |
868 | - # If the scope of multiple addresses is unknown, the last one is taken. |
869 | + address = watchers.retrieve_public_adddress(addresses, self.resolver) |
870 | + self.assertEqual('eu-west-1.compute.example.com', address) |
871 | + |
872 | + def test_local_cloud_address(self): |
873 | + # If there are no available public addresses the first ipv4 address |
874 | + # with cloud-local scope is returned. |
875 | + addresses = [ |
876 | + {'NetworkName': '', |
877 | + 'Scope': 'local-cloud', |
878 | + 'Type': 'hostname', |
879 | + 'Value': 'eu-west-1.example.internal'}, |
880 | + {'NetworkName': '', |
881 | + 'Scope': 'local-cloud', |
882 | + 'Type': 'ipv4', |
883 | + 'Value': '10.42.47.10'}, |
884 | + {'NetworkName': '', |
885 | + 'Scope': 'local-cloud', |
886 | + 'Type': 'ipv4', |
887 | + 'Value': '1.2.3.4'}, |
888 | + ] |
889 | + address = watchers.retrieve_public_adddress(addresses, self.resolver) |
890 | + self.assertEqual('10.42.47.10', address) |
891 | + |
892 | + def test_unknown_address(self): |
893 | + # If there are no available public or local-cloud addresses, the first |
894 | + # address with unknown scope is returned. |
895 | addresses = [ |
896 | {'NetworkName': '', |
897 | 'Scope': '', |
898 | @@ -144,8 +175,52 @@ |
899 | 'Type': 'ipv4', |
900 | 'Value': '10.0.3.47'}, |
901 | ] |
902 | - public_address = watchers.retrieve_public_adddress(addresses) |
903 | - self.assertEqual('10.0.3.47', public_address) |
904 | + address = watchers.retrieve_public_adddress(addresses, self.resolver) |
905 | + self.assertEqual('10.0.3.42', address) |
906 | + |
907 | + def test_preferred_fallback_address(self): |
908 | + # If there are no available public addresses the first fallback address |
909 | + # in the list is returned. |
910 | + addresses = [ |
911 | + {'NetworkName': '', |
912 | + 'Scope': '', |
913 | + 'Type': 'ipv4', |
914 | + 'Value': '10.0.3.47'}, |
915 | + {'NetworkName': '', |
916 | + 'Scope': 'local-cloud', |
917 | + 'Type': 'ipv4', |
918 | + 'Value': '10.42.47.10'}, |
919 | + |
920 | + ] |
921 | + address = watchers.retrieve_public_adddress(addresses, self.resolver) |
922 | + self.assertEqual('10.0.3.47', address) |
923 | + # Now test with a reversed order. |
924 | + address = watchers.retrieve_public_adddress( |
925 | + reversed(addresses), self.resolver) |
926 | + self.assertEqual('10.42.47.10', address) |
927 | + |
928 | + def test_unresolvable_public_address(self): |
929 | + # None is returned if a public cloud is found but it is not resolvable. |
930 | + addresses = [ |
931 | + {'NetworkName': '', |
932 | + 'Scope': 'public', |
933 | + 'Type': 'hostname', |
934 | + 'Value': 'eu-west-1.example.internal'}, |
935 | + ] |
936 | + expected_warning = ( |
937 | + 'cannot resolve public eu-west-1.example.internal address, ' |
938 | + 'looking for another candidate: bad wolf') |
939 | + with helpers.assert_logs([expected_warning], level='warn'): |
940 | + address = watchers.retrieve_public_adddress( |
941 | + addresses, lambda hostname: 'bad wolf') |
942 | + self.assertIsNone(address) |
943 | + |
944 | + def test_unresolvable_public_address_fallback(self): |
945 | + # If there are no resolvable public addresses the first ipv4 address |
946 | + # with cloud-local scope is returned. |
947 | + address = watchers.retrieve_public_adddress( |
948 | + cloud_addresses, lambda hostname: 'bad wolf') |
949 | + self.assertEqual('10.42.47.10', address) |
950 | |
951 | |
952 | class TestParseMachineChange(helpers.ValueErrorTestsMixin, unittest.TestCase): |
953 | @@ -205,8 +280,12 @@ |
954 | # A message is printed to stdout when the machine obtains a public |
955 | # address. |
956 | data = {'Addresses': cloud_addresses, 'Id': '1', 'Status': 'pending'} |
957 | - status, address = watchers.parse_machine_change( |
958 | - 'change', data, 'pending', '') |
959 | + # Patch the hostname resolver used to check hostname addresses. |
960 | + # Note that resolve error cases are properly exercised by the |
961 | + # watchers.retrieve_public_adddress tests. |
962 | + with helpers.patch_check_resolvable(): |
963 | + status, address = watchers.parse_machine_change( |
964 | + 'change', data, 'pending', '') |
965 | self.assertEqual('pending', status) |
966 | self.assertEqual('eu-west-1.compute.example.com', address) |
967 | mock_print.assert_called_once_with( |
968 | @@ -248,8 +327,8 @@ |
969 | # A ValueError is raised if the change represents a unit removal. |
970 | data = {'Name': 'django/42', 'Status': 'started'} |
971 | with self.assert_value_error('django/42 unexpectedly removed'): |
972 | - # The last two arguments are the current status and address. |
973 | - watchers.parse_unit_change('remove', data, '', '') |
974 | + # The last argument is the current status. |
975 | + watchers.parse_unit_change('remove', data, '') |
976 | |
977 | def test_unit_error(self): |
978 | # A ValueError is raised if the unit is in an error state. |
979 | @@ -260,120 +339,55 @@ |
980 | } |
981 | expected_error = 'django/0 is in an error state: start error: bad wolf' |
982 | with self.assert_value_error(expected_error): |
983 | - # The last two arguments are the current status and address. |
984 | - watchers.parse_unit_change('change', data, '', '') |
985 | - |
986 | - @helpers.mock_print |
987 | - def test_address_notified(self, mock_print): |
988 | - # A message is printed to stdout when the unit obtains a public |
989 | - # address. The function returns the status, the new address and the |
990 | - # machine identifier. |
991 | - data = { |
992 | - 'Name': 'haproxy/2', |
993 | - 'Status': 'pending', |
994 | - 'PublicAddress': 'haproxy2.example.com', |
995 | - 'MachineId': '42', |
996 | - } |
997 | - status, address, machine_id = watchers.parse_unit_change( |
998 | - 'change', data, 'pending', '') |
999 | - self.assertEqual('pending', status) |
1000 | - self.assertEqual('haproxy2.example.com', address) |
1001 | - self.assertEqual('42', machine_id) |
1002 | - mock_print.assert_called_once_with( |
1003 | - 'haproxy/2 placed on haproxy2.example.com') |
1004 | + # The last argument is the current status. |
1005 | + watchers.parse_unit_change('change', data, '') |
1006 | |
1007 | @helpers.mock_print |
1008 | def test_pending_status_notified(self, mock_print): |
1009 | # A message is printed to stdout when the unit changes its status to |
1010 | - # "pending". The function returns the new status, the address and the |
1011 | - # machine identifier. The last two values are empty strings if the unit |
1012 | - # has not yet been assigned to a machine. |
1013 | - data = {'Name': 'django/1', 'Status': 'pending', 'PublicAddress': ''} |
1014 | - # The last two arguments are the current status and address. |
1015 | - status, address, machine_id = watchers.parse_unit_change( |
1016 | - 'change', data, '', '') |
1017 | + # "pending". The function returns the new status and the machine |
1018 | + # identifier, which is empty if the unit has not yet been assigned to a |
1019 | + # machine. |
1020 | + data = {'Name': 'django/1', 'Status': 'pending'} |
1021 | + # The last argument is the current status. |
1022 | + status, machine_id = watchers.parse_unit_change('change', data, '') |
1023 | self.assertEqual('pending', status) |
1024 | - self.assertEqual('', address) |
1025 | self.assertEqual('', machine_id) |
1026 | mock_print.assert_called_once_with('django/1 deployment is pending') |
1027 | |
1028 | @helpers.mock_print |
1029 | def test_installed_status_notified(self, mock_print): |
1030 | # A message is printed to stdout when the unit changes its status to |
1031 | - # "installed". The function returns the new status, the address and the |
1032 | - # machine identifier. |
1033 | - data = { |
1034 | - 'Name': 'django/42', |
1035 | - 'Status': 'installed', |
1036 | - 'PublicAddress': 'django42.example.com', |
1037 | - 'MachineId': '1', |
1038 | - } |
1039 | - status, address, machine_id = watchers.parse_unit_change( |
1040 | - 'change', data, 'pending', 'django42.example.com') |
1041 | + # "installed". |
1042 | + data = {'Name': 'django/42', 'Status': 'installed', 'MachineId': '1'} |
1043 | + status, machine_id = watchers.parse_unit_change( |
1044 | + 'change', data, 'pending') |
1045 | self.assertEqual('installed', status) |
1046 | - self.assertEqual('django42.example.com', address) |
1047 | self.assertEqual('1', machine_id) |
1048 | mock_print.assert_called_once_with('django/42 is installed') |
1049 | |
1050 | @helpers.mock_print |
1051 | def test_started_status_notified(self, mock_print): |
1052 | # A message is printed to stdout when the unit changes its status to |
1053 | - # "started". The function returns the new status, the address and the |
1054 | - # machine identifier. |
1055 | - data = { |
1056 | - 'Name': 'wordpress/0', |
1057 | - 'Status': 'started', |
1058 | - 'PublicAddress': 'wordpress0.example.com', |
1059 | - 'MachineId': '0', |
1060 | - } |
1061 | - status, address, machine_id = watchers.parse_unit_change( |
1062 | - 'change', data, '', 'wordpress0.example.com') |
1063 | + # "started". |
1064 | + data = {'Name': 'wordpress/0', 'Status': 'started', 'MachineId': '0'} |
1065 | + # The last argument is the current status. |
1066 | + status, machine_id = watchers.parse_unit_change('change', data, '') |
1067 | self.assertEqual('started', status) |
1068 | - self.assertEqual('wordpress0.example.com', address) |
1069 | self.assertEqual('0', machine_id) |
1070 | mock_print.assert_called_once_with('wordpress/0 is ready on machine 0') |
1071 | |
1072 | @helpers.mock_print |
1073 | - def test_both_status_and_address_notified(self, mock_print): |
1074 | - # Both status and public address changes are notified if required. |
1075 | - data = { |
1076 | - 'Name': 'django/0', |
1077 | - 'Status': 'started', |
1078 | - 'PublicAddress': 'django42.example.com', |
1079 | - 'MachineId': '0', |
1080 | - } |
1081 | - # The last two arguments are the current status and address. |
1082 | - watchers.parse_unit_change('change', data, '', '') |
1083 | - self.assertEqual(2, mock_print.call_count) |
1084 | - mock_print.assert_has_calls([ |
1085 | - mock.call('django/0 placed on django42.example.com'), |
1086 | - mock.call('django/0 is ready on machine 0'), |
1087 | - ]) |
1088 | - |
1089 | - @helpers.mock_print |
1090 | def test_status_not_changed(self, mock_print): |
1091 | # If the status in the unit change and the given current status are the |
1092 | # same value, nothing is printed and the current values are returned. |
1093 | - data = {'Name': 'django/1', 'Status': 'pending', 'PublicAddress': ''} |
1094 | - status, address, machine_id = watchers.parse_unit_change( |
1095 | - 'change', data, 'pending', '') |
1096 | + data = {'Name': 'django/1', 'Status': 'pending'} |
1097 | + status, machine_id = watchers.parse_unit_change( |
1098 | + 'change', data, 'pending') |
1099 | self.assertEqual('pending', status) |
1100 | - self.assertEqual('', address) |
1101 | self.assertEqual('', machine_id) |
1102 | self.assertFalse(mock_print.called) |
1103 | |
1104 | - @helpers.mock_print |
1105 | - def test_address_not_available(self, mock_print): |
1106 | - # An empty address is returned when the public address field is not |
1107 | - # included in the change data. |
1108 | - data = {'Name': 'haproxy/2', 'Status': 'pending', 'MachineId': '42'} |
1109 | - status, address, machine_id = watchers.parse_unit_change( |
1110 | - 'change', data, 'pending', '') |
1111 | - self.assertEqual('pending', status) |
1112 | - self.assertEqual('', address) |
1113 | - self.assertEqual('42', machine_id) |
1114 | - self.assertFalse(mock_print.called) |
1115 | - |
1116 | |
1117 | class TestUnitMachineChanges(unittest.TestCase): |
1118 | |
1119 | |
1120 | === modified file 'quickstart/utils.py' |
1121 | --- quickstart/utils.py 2014-08-22 18:32:15 +0000 |
1122 | +++ quickstart/utils.py 2014-11-10 12:30:00 +0000 |
1123 | @@ -105,6 +105,20 @@ |
1124 | return retcode, output.decode('utf-8'), error.decode('utf-8') |
1125 | |
1126 | |
1127 | +def check_resolvable(hostname): |
1128 | + """Check that the hostname can be resolved to a numeric IP address. |
1129 | + |
1130 | + Return an error message if the address cannot be resolved. |
1131 | + """ |
1132 | + try: |
1133 | + address = socket.gethostbyname(hostname) |
1134 | + except socket.error as err: |
1135 | + return bytes(err).decode('utf-8') |
1136 | + logging.debug('{} resolved to {}'.format( |
1137 | + hostname, address.decode('utf-8'))) |
1138 | + return None |
1139 | + |
1140 | + |
1141 | def parse_gui_charm_url(charm_url): |
1142 | """Parse the given charm URL. |
1143 | |
1144 | |
1145 | === modified file 'quickstart/watchers.py' |
1146 | --- quickstart/watchers.py 2014-07-04 13:09:11 +0000 |
1147 | +++ quickstart/watchers.py 2014-11-10 12:30:00 +0000 |
1148 | @@ -21,15 +21,25 @@ |
1149 | unicode_literals, |
1150 | ) |
1151 | |
1152 | - |
1153 | +import logging |
1154 | + |
1155 | +from quickstart import utils |
1156 | + |
1157 | + |
1158 | +IPV4_ADDRESS = 'ipv4' |
1159 | IPV6_ADDRESS = 'ipv6' |
1160 | -NETWORK_PUBLIC = 'public' |
1161 | -NETWORK_UNKNOWN = '' |
1162 | - |
1163 | - |
1164 | -def retrieve_public_adddress(addresses): |
1165 | +SCOPE_CLOUD_LOCAL = 'local-cloud' |
1166 | +SCOPE_PUBLIC = 'public' |
1167 | +SCOPE_UNKNOWN = '' |
1168 | + |
1169 | + |
1170 | +def retrieve_public_adddress(addresses, hostname_resolver): |
1171 | """Parse the given addresses and return a public address if available. |
1172 | |
1173 | + Use the given hostname_resolver callable to ensure a candidate address can |
1174 | + be resolved. The given function must accept an hostname/ip address and |
1175 | + return None if the address is resolvable, or an error string otherwise. |
1176 | + |
1177 | The addresses argument is a list of address dictionaries. |
1178 | Cloud addresses look like the following: |
1179 | |
1180 | @@ -73,8 +83,8 @@ |
1181 | found, this function returns None. |
1182 | """ |
1183 | # This implementation reflects how the public address is retrieved in Juju: |
1184 | - # see juju-core/instance/address.go:SelectPublicAddress. |
1185 | - public_address = None |
1186 | + # see juju-core/network/address.go:SelectPublicAddress. |
1187 | + fallback_address = None |
1188 | for address in addresses: |
1189 | value = address['Value'] |
1190 | # Exclude empty values and ipv6 addresses. |
1191 | @@ -82,14 +92,33 @@ |
1192 | # The address scope was called "NetworkScope" prior to juju 1.20.0. |
1193 | scope = address.get('Scope', address.get('NetworkScope')) |
1194 | # If the scope is public then we have found the address. |
1195 | - if scope == NETWORK_PUBLIC: |
1196 | - return value |
1197 | - # If the scope is unknown then store the value. This way the last |
1198 | - # address with unknown scope will be returned, and we are able to |
1199 | - # return the right LXC address. |
1200 | - if scope == NETWORK_UNKNOWN: |
1201 | - public_address = value |
1202 | - return public_address |
1203 | + if scope == SCOPE_PUBLIC: |
1204 | + error = hostname_resolver(value) |
1205 | + if error is None: |
1206 | + return value |
1207 | + # If the address is not resolvable, fall back to the next |
1208 | + # candidate, whether it is another public address, a |
1209 | + # local-cloud one or an unknown one. The local-cloud case, for |
1210 | + # instance, would happen when using the maas provider and the |
1211 | + # maas DNS server is not configured locally. |
1212 | + logging.warn( |
1213 | + 'cannot resolve public {} address, looking for another ' |
1214 | + 'candidate: {}'.format(value, error)) |
1215 | + if fallback_address is not None: |
1216 | + # If we already have a fallback address then we can jump to the |
1217 | + # next candidate. |
1218 | + continue |
1219 | + address_type = address.get('Type') |
1220 | + if scope == SCOPE_CLOUD_LOCAL and address_type == IPV4_ADDRESS: |
1221 | + # Use the local cloud scoped address as fallback value: the |
1222 | + # current environment is probably maas and the public address |
1223 | + # of the instance is not resolvable by local DNS. |
1224 | + fallback_address = value |
1225 | + elif scope == SCOPE_UNKNOWN: |
1226 | + # Use the unknown scoped address as fallback value: the current |
1227 | + # environment is probably local. |
1228 | + fallback_address = value |
1229 | + return fallback_address |
1230 | |
1231 | |
1232 | def parse_machine_change(action, data, current_status, address): |
1233 | @@ -120,7 +149,8 @@ |
1234 | # of units hosted by a specific machine. |
1235 | if not address: |
1236 | addresses = data.get('Addresses', []) |
1237 | - public_address = retrieve_public_adddress(addresses) |
1238 | + public_address = retrieve_public_adddress( |
1239 | + addresses, utils.check_resolvable) |
1240 | if public_address is not None: |
1241 | address = public_address |
1242 | print('unit placed on {}'.format(address)) |
1243 | @@ -134,16 +164,16 @@ |
1244 | return status, address |
1245 | |
1246 | |
1247 | -def parse_unit_change(action, data, current_status, address): |
1248 | +def parse_unit_change(action, data, current_status): |
1249 | """Parse the given unit change. |
1250 | |
1251 | The change is represented by the given action/data pair. |
1252 | - Also receive the last known unit status and address, which can be empty |
1253 | - strings if those pieces of information are unknown. |
1254 | + Also receive the last known unit status which can be empty if the unit |
1255 | + status is not yet known. |
1256 | |
1257 | Output a human readable message each time a relevant change is found. |
1258 | |
1259 | - Return the unit status, address and machine identifier. |
1260 | + Return the unit status and the identifier of the machine hosting the unit. |
1261 | Raise a ValueError if the service unit is removed or in an error state. |
1262 | """ |
1263 | unit_name = data['Name'] |
1264 | @@ -157,15 +187,6 @@ |
1265 | msg = '{} is in an error state: {}: {}'.format( |
1266 | unit_name, status, data['StatusInfo']) |
1267 | raise ValueError(msg.encode('utf-8')) |
1268 | - # Notify when the unit becomes reachable. Up to juju-core 1.18, the |
1269 | - # mega-watcher for units includes the public address for each unit. This |
1270 | - # info is likely to be deprecated in favor of addresses as included in the |
1271 | - # mega-watcher for machines, but we still try to retrieve the address here |
1272 | - # for backward compatibility. |
1273 | - if not address: |
1274 | - address = data.get('PublicAddress', '') |
1275 | - if address: |
1276 | - print('{} placed on {}'.format(unit_name, address)) |
1277 | # Notify status changes. |
1278 | if status != current_status: |
1279 | if status == 'pending': |
1280 | @@ -175,7 +196,7 @@ |
1281 | elif status == 'started': |
1282 | print('{} is ready on machine {}'.format( |
1283 | unit_name, data['MachineId'])) |
1284 | - return status, address, data.get('MachineId', '') |
1285 | + return status, data.get('MachineId', '') |
1286 | |
1287 | |
1288 | def unit_machine_changes(changeset): |
Reviewers: mp+241263_ code.launchpad. net,
Message:
Please take a look.
Description:
Unit address from the machines watcher only
Only use the mega-watcher for machines to retrieve
the Juju GUI unit address.
This change has several consequences:
- it allows us to apply some logic on how the
right address is chosen. For instance, now
we try to resolve public hostnames before
proceeding, and this should fix the cases
where a cloud dns is not configured on the
machine running quickstart. This is the case
of many maas environments;
- it simplifies parsing the mega-watcher changes;
- more importantly, it breaks compatibility
with very old versions of juju (<1.18), in which
the mega-watcher for machines did not include
machine addresses.
For this reason, quickstart now explicitly
drops support for juju < 1.18.1
(1.18.1 is the version on trusty universe).
This also allows for removing some version
checks in the code, including sudo handling when
calling bootstrap on local envs, several special
cases on the watcher side, and other oddities.
For the reasons above, I bumped the quickstart
version up to 1.5.0.
PS: my apologies for the long diff, hope the code
is still easy to follow. Sorry.
Tests: `make check`
QA:
run quickstart as usual, on local and cloud envs,
check it works properly when run again, etc.
this branch has been already successfully QAed in
a maas environment by Adam (Landscape team).
https:/ /code.launchpad .net/~frankban/ juju-quickstart /maas-address/ +merge/ 241263
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/174790043/
Affected files (+334, -433 lines): __init_ _.py manage. py settings. py tests/helpers. py tests/test_ app.py tests/test_ manage. py tests/test_ utils.py tests/test_ watchers. py watchers. py
M README.rst
A [revision details]
M quickstart/
M quickstart/app.py
M quickstart/
M quickstart/
M quickstart/
M quickstart/
M quickstart/
M quickstart/
M quickstart/
M quickstart/utils.py
M quickstart/