Merge lp:~gz/juju-ci-tools/common_container_networking into lp:juju-ci-tools
- common_container_networking
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 1226 |
Proposed branch: | lp:~gz/juju-ci-tools/common_container_networking |
Merge into: | lp:juju-ci-tools |
Diff against target: |
335 lines (+148/-93) 2 files modified
assess_container_networking.py (+48/-78) tests/test_assess_container_networking.py (+100/-15) |
To merge this branch: | bzr merge lp:~gz/juju-ci-tools/common_container_networking |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Aaron Bentley (community) | Approve | ||
Review via email: mp+282640@code.launchpad.net |
Commit message
Description of the change
Make assess_
Switches to use BootstrapManager but retains --clean-environment as a special case of --keep-env. Pulls some of the parts of BootstrapManager out into the local test file for now, still saves a bunch of code.
Aaron Bentley (abentley) wrote : | # |
Okay, I think I get this better now. Because you're not hitting bootstrap_context, you're not writing client.env to disk-- you're just ignoring it. Its only value is the name, so you're using update_env to change the name.
I think this is a borderline case-- it might be better to load the proper client.env from disk, but this works. update_env is overkill, but it respects DRY.
Martin Packman (gz) wrote : | # |
Right, I think what I really want is an alternative first step method on BootstrapManager that loads the tempenv rather than does the bootstrap.
Preview Diff
1 | === modified file 'assess_container_networking.py' | |||
2 | --- assess_container_networking.py 2015-12-12 00:40:50 +0000 | |||
3 | +++ assess_container_networking.py 2016-01-14 17:48:44 +0000 | |||
4 | @@ -9,28 +9,24 @@ | |||
5 | 9 | import re | 9 | import re |
6 | 10 | import os | 10 | import os |
7 | 11 | import subprocess | 11 | import subprocess |
8 | 12 | import sys | ||
9 | 12 | import tempfile | 13 | import tempfile |
10 | 13 | from textwrap import dedent | 14 | from textwrap import dedent |
11 | 14 | import time | 15 | import time |
12 | 15 | 16 | ||
13 | 16 | from deploy_stack import ( | 17 | from deploy_stack import ( |
14 | 18 | BootstrapManager, | ||
15 | 19 | get_random_string, | ||
16 | 17 | update_env, | 20 | update_env, |
17 | 18 | dump_env_logs, | ||
18 | 19 | get_random_string, | ||
19 | 20 | ) | ||
20 | 21 | from jujuconfig import ( | ||
21 | 22 | get_juju_home, | ||
22 | 23 | ) | 21 | ) |
23 | 24 | from jujupy import ( | 22 | from jujupy import ( |
24 | 25 | parse_new_state_server_from_error, | ||
25 | 26 | temp_bootstrap_env, | ||
26 | 27 | SimpleEnvironment, | 23 | SimpleEnvironment, |
27 | 28 | EnvJujuClient, | 24 | EnvJujuClient, |
28 | 29 | ) | 25 | ) |
29 | 30 | from utility import ( | 26 | from utility import ( |
30 | 27 | add_basic_testing_arguments, | ||
31 | 28 | configure_logging, | ||
32 | 31 | wait_for_port, | 29 | wait_for_port, |
33 | 32 | print_now, | ||
34 | 33 | add_basic_testing_arguments, | ||
35 | 34 | ) | 30 | ) |
36 | 35 | 31 | ||
37 | 36 | 32 | ||
38 | @@ -40,6 +36,9 @@ | |||
39 | 40 | __metaclass__ = type | 36 | __metaclass__ = type |
40 | 41 | 37 | ||
41 | 42 | 38 | ||
42 | 39 | log = logging.getLogger("assess_container_networking") | ||
43 | 40 | |||
44 | 41 | |||
45 | 43 | def parse_args(argv=None): | 42 | def parse_args(argv=None): |
46 | 44 | """Parse all arguments.""" | 43 | """Parse all arguments.""" |
47 | 45 | 44 | ||
48 | @@ -69,7 +68,11 @@ | |||
49 | 69 | 68 | ||
50 | 70 | At termination, clean out services and machines from the environment | 69 | At termination, clean out services and machines from the environment |
51 | 71 | rather than destroying it.""")) | 70 | rather than destroying it.""")) |
53 | 72 | return parser.parse_args(argv) | 71 | args = parser.parse_args(argv) |
54 | 72 | # Passing --clean-environment implies --keep-env | ||
55 | 73 | if args.clean_environment: | ||
56 | 74 | args.keep_env = True | ||
57 | 75 | return args | ||
58 | 73 | 76 | ||
59 | 74 | 77 | ||
60 | 75 | def ssh(client, machine, cmd): | 78 | def ssh(client, machine, cmd): |
61 | @@ -107,9 +110,14 @@ | |||
62 | 107 | # A short timeout is used for get_status here because if we don't get a | 110 | # A short timeout is used for get_status here because if we don't get a |
63 | 108 | # response from get_status quickly then the environment almost | 111 | # response from get_status quickly then the environment almost |
64 | 109 | # certainly doesn't exist or needs recreating. | 112 | # certainly doesn't exist or needs recreating. |
66 | 110 | status = client.get_status(5) | 113 | try: |
67 | 114 | status = client.get_status(5) | ||
68 | 115 | except Exception as e: | ||
69 | 116 | # TODO(gz): get_status should return a more specific error type. | ||
70 | 117 | log.info("Could not clean existing env: %s", e) | ||
71 | 118 | return False | ||
72 | 111 | 119 | ||
74 | 112 | for service in status.status['services']: | 120 | for service in status.status.get('services', {}): |
75 | 113 | client.juju('remove-service', service) | 121 | client.juju('remove-service', service) |
76 | 114 | 122 | ||
77 | 115 | if not services_only: | 123 | if not services_only: |
78 | @@ -214,6 +222,7 @@ | |||
79 | 214 | :param targets: machine IDs of machines to test | 222 | :param targets: machine IDs of machines to test |
80 | 215 | :return: None; | 223 | :return: None; |
81 | 216 | """ | 224 | """ |
82 | 225 | log.info('Waiting for the bootstrap machine agent to start.') | ||
83 | 217 | status = client.wait_for_started().status | 226 | status = client.wait_for_started().status |
84 | 218 | source = targets[0] | 227 | source = targets[0] |
85 | 219 | dests = targets[1:] | 228 | dests = targets[1:] |
86 | @@ -336,15 +345,15 @@ | |||
87 | 336 | _assessment_iteration(client, test_containers) | 345 | _assessment_iteration(client, test_containers) |
88 | 337 | 346 | ||
89 | 338 | 347 | ||
91 | 339 | def assess_container_networking(client, args): | 348 | def assess_container_networking(client, machine_type): |
92 | 340 | """Runs _assess_address_allocation, reboots hosts, repeat. | 349 | """Runs _assess_address_allocation, reboots hosts, repeat. |
93 | 341 | :param client: Juju client | 350 | :param client: Juju client |
95 | 342 | :param args: Parsed command line arguments | 351 | :param machine_type: Container types to test |
96 | 343 | :return: None | 352 | :return: None |
97 | 344 | """ | 353 | """ |
98 | 345 | # Only test the containers we were asked to test | 354 | # Only test the containers we were asked to test |
101 | 346 | if args.machine_type: | 355 | if machine_type: |
102 | 347 | types = [args.machine_type] | 356 | types = [machine_type] |
103 | 348 | else: | 357 | else: |
104 | 349 | types = [KVM_MACHINE, LXC_MACHINE] | 358 | types = [KVM_MACHINE, LXC_MACHINE] |
105 | 350 | 359 | ||
106 | @@ -377,69 +386,30 @@ | |||
107 | 377 | _assess_container_networking(client, types, hosts, containers) | 386 | _assess_container_networking(client, types, hosts, containers) |
108 | 378 | 387 | ||
109 | 379 | 388 | ||
115 | 380 | def get_client(args): | 389 | def main(argv=None): |
116 | 381 | client = EnvJujuClient.by_version( | 390 | args = parse_args(argv) |
117 | 382 | SimpleEnvironment.from_config(args.env), | 391 | configure_logging(args.verbose) |
118 | 383 | args.juju_bin, args.debug | 392 | bs_manager = BootstrapManager.from_args(args) |
119 | 384 | ) | 393 | client = bs_manager.client |
120 | 385 | client.enable_container_address_allocation() | 394 | client.enable_container_address_allocation() |
150 | 386 | update_env(client.env, args.temp_env_name, | 395 | # TODO(gz): Having to manipulate client env state here to get the temp env |
151 | 387 | series=args.series, bootstrap_host=args.bootstrap_host, | 396 | # is ugly, would ideally be captured in an explicit scope. |
152 | 388 | agent_url=args.agent_url, agent_stream=args.agent_stream, | 397 | update_env(client.env, bs_manager.temp_env_name, series=bs_manager.series, |
153 | 389 | region=args.region) | 398 | agent_url=bs_manager.agent_url, |
154 | 390 | return client | 399 | agent_stream=bs_manager.agent_stream, region=bs_manager.region) |
155 | 391 | 400 | with bs_manager.top_context() as machines: | |
156 | 392 | 401 | bootstrap_required = True | |
157 | 393 | def main(): | 402 | if args.clean_environment and clean_environment(client): |
158 | 394 | args = parse_args() | 403 | bootstrap_required = False |
159 | 395 | client = get_client(args) | 404 | if bootstrap_required: |
160 | 396 | juju_home = get_juju_home() | 405 | with bs_manager.bootstrap_context(machines): |
132 | 397 | bootstrap_host = None | ||
133 | 398 | success = True | ||
134 | 399 | try: | ||
135 | 400 | if args.clean_environment: | ||
136 | 401 | try: | ||
137 | 402 | if not clean_environment(client): | ||
138 | 403 | with temp_bootstrap_env(juju_home, client): | ||
139 | 404 | client.bootstrap(args.upload_tools) | ||
140 | 405 | except Exception as e: | ||
141 | 406 | logging.exception(e) | ||
142 | 407 | client.destroy_environment() | ||
143 | 408 | client = get_client(args) | ||
144 | 409 | with temp_bootstrap_env(juju_home, client): | ||
145 | 410 | client.bootstrap(args.upload_tools) | ||
146 | 411 | else: | ||
147 | 412 | client.destroy_environment() | ||
148 | 413 | client = get_client(args) | ||
149 | 414 | with temp_bootstrap_env(juju_home, client): | ||
161 | 415 | client.bootstrap(args.upload_tools) | 406 | client.bootstrap(args.upload_tools) |
189 | 416 | 407 | with bs_manager.runtime_context(machines): | |
190 | 417 | logging.info('Waiting for the bootstrap machine agent to start.') | 408 | assess_container_networking(client, args.machine_type) |
191 | 418 | status = client.wait_for_started() | 409 | if args.clean_environment and not clean_environment(client): |
192 | 419 | mid, data = list(status.iter_machines())[0] | 410 | return 1 |
193 | 420 | bootstrap_host = data['dns-name'] | 411 | return 0 |
194 | 421 | 412 | ||
168 | 422 | assess_container_networking(client, args) | ||
169 | 423 | |||
170 | 424 | except Exception as e: | ||
171 | 425 | success = False | ||
172 | 426 | logging.exception(e) | ||
173 | 427 | try: | ||
174 | 428 | if bootstrap_host is None: | ||
175 | 429 | bootstrap_host = parse_new_state_server_from_error(e) | ||
176 | 430 | except Exception as e: | ||
177 | 431 | print_now('exception while dumping logs:\n') | ||
178 | 432 | logging.exception(e) | ||
179 | 433 | finally: | ||
180 | 434 | if bootstrap_host is not None: | ||
181 | 435 | dump_env_logs(client, bootstrap_host, args.logs) | ||
182 | 436 | |||
183 | 437 | if args.clean_environment: | ||
184 | 438 | clean_environment(client) | ||
185 | 439 | else: | ||
186 | 440 | client.destroy_environment() | ||
187 | 441 | if not success: | ||
188 | 442 | exit(1) | ||
195 | 443 | 413 | ||
196 | 444 | if __name__ == '__main__': | 414 | if __name__ == '__main__': |
198 | 445 | main() | 415 | sys.exit(main()) |
199 | 446 | 416 | ||
200 | === modified file 'tests/test_assess_container_networking.py' | |||
201 | --- tests/test_assess_container_networking.py 2015-12-14 16:23:40 +0000 | |||
202 | +++ tests/test_assess_container_networking.py 2016-01-14 17:48:44 +0000 | |||
203 | @@ -1,6 +1,7 @@ | |||
204 | 1 | from argparse import Namespace | 1 | from argparse import Namespace |
205 | 2 | from copy import deepcopy | 2 | from copy import deepcopy |
206 | 3 | from contextlib import contextmanager | 3 | from contextlib import contextmanager |
207 | 4 | import logging | ||
208 | 4 | 5 | ||
209 | 5 | from mock import ( | 6 | from mock import ( |
210 | 6 | patch, | 7 | patch, |
211 | @@ -16,6 +17,7 @@ | |||
212 | 16 | 17 | ||
213 | 17 | import assess_container_networking as jcnet | 18 | import assess_container_networking as jcnet |
214 | 18 | from tests import ( | 19 | from tests import ( |
215 | 20 | FakeHomeTestCase, | ||
216 | 19 | parse_error, | 21 | parse_error, |
217 | 20 | TestCase, | 22 | TestCase, |
218 | 21 | ) | 23 | ) |
219 | @@ -389,18 +391,101 @@ | |||
220 | 389 | ValueError, "Default route not found", | 391 | ValueError, "Default route not found", |
221 | 390 | jcnet.assess_internet_connection, self.client, targets) | 392 | jcnet.assess_internet_connection, self.client, targets) |
222 | 391 | 393 | ||
238 | 392 | def test_get_client(self): | 394 | |
239 | 393 | args = Namespace( | 395 | class TestMain(FakeHomeTestCase): |
240 | 394 | env="e", juju_bin="jb", debug=False, | 396 | |
241 | 395 | agent_stream="http://tools.testing/agents", temp_env_name="te", | 397 | @contextmanager |
242 | 396 | series="s", bootstrap_host="bh", agent_url="au", region="r") | 398 | def patch_main(self, argv, client, log_level, debug=False): |
243 | 397 | 399 | env = SimpleEnvironment(argv[0], {"type": "ec2"}) | |
244 | 398 | upenv = MagicMock() | 400 | client.env = env |
245 | 399 | with patch.object(EnvJujuClient, "by_version"), \ | 401 | with patch("assess_container_networking.configure_logging", |
246 | 400 | patch.object(SimpleEnvironment, "from_config"), \ | 402 | autospec=True) as mock_cl: |
247 | 401 | patch("assess_container_networking.update_env", upenv): | 403 | with patch("jujupy.SimpleEnvironment.from_config", |
248 | 402 | 404 | return_value=env) as mock_e: | |
249 | 403 | jcnet.get_client(args) | 405 | with patch("jujupy.EnvJujuClient.by_version", |
250 | 404 | self.assertEqual(upenv.call_args[0][1], args.temp_env_name) | 406 | return_value=client) as mock_c: |
251 | 405 | for key, value in upenv.call_args[1].iteritems(): | 407 | yield |
252 | 406 | self.assertEqual(vars(args)[key], value) | 408 | mock_cl.assert_called_once_with(log_level) |
253 | 409 | mock_e.assert_called_once_with(argv[0]) | ||
254 | 410 | mock_c.assert_called_once_with(env, argv[1], debug=debug) | ||
255 | 411 | |||
256 | 412 | @contextmanager | ||
257 | 413 | def patch_bootstrap_manager(self, runs=True): | ||
258 | 414 | with patch("deploy_stack.BootstrapManager.top_context", | ||
259 | 415 | autospec=True) as mock_tc: | ||
260 | 416 | with patch("deploy_stack.BootstrapManager.bootstrap_context", | ||
261 | 417 | autospec=True) as mock_bc: | ||
262 | 418 | with patch("deploy_stack.BootstrapManager.runtime_context", | ||
263 | 419 | autospec=True) as mock_rc: | ||
264 | 420 | yield mock_bc | ||
265 | 421 | self.assertEqual(mock_tc.call_count, 1) | ||
266 | 422 | if runs: | ||
267 | 423 | self.assertEqual(mock_rc.call_count, 1) | ||
268 | 424 | |||
269 | 425 | def test_bootstrap_required(self): | ||
270 | 426 | argv = ["an-env", "/bin/juju", "/tmp/logs", "an-env-mod", "--verbose"] | ||
271 | 427 | client = Mock(spec=["bootstrap", "enable_container_address_allocation", | ||
272 | 428 | "is_jes_enabled"]) | ||
273 | 429 | with patch("assess_container_networking.assess_container_networking", | ||
274 | 430 | autospec=True) as mock_assess: | ||
275 | 431 | with self.patch_bootstrap_manager() as mock_bc: | ||
276 | 432 | with self.patch_main(argv, client, logging.DEBUG): | ||
277 | 433 | ret = jcnet.main(argv) | ||
278 | 434 | client.enable_container_address_allocation.assert_called_once_with() | ||
279 | 435 | client.bootstrap.assert_called_once_with(False) | ||
280 | 436 | self.assertEqual("", self.log_stream.getvalue()) | ||
281 | 437 | self.assertEqual(mock_bc.call_count, 1) | ||
282 | 438 | mock_assess.assert_called_once_with(client, None) | ||
283 | 439 | self.assertEqual(ret, 0) | ||
284 | 440 | |||
285 | 441 | def test_clean_existing_env(self): | ||
286 | 442 | argv = ["an-env", "/bin/juju", "/tmp/logs", "an-env-mod", | ||
287 | 443 | "--clean-environment"] | ||
288 | 444 | client = Mock(spec=["enable_container_address_allocation", "env", | ||
289 | 445 | "get_status", "is_jes_enabled", "wait_for", | ||
290 | 446 | "wait_for_started"]) | ||
291 | 447 | client.get_status.return_value = Status.from_text(""" | ||
292 | 448 | machines: | ||
293 | 449 | "0": | ||
294 | 450 | agent-state: started | ||
295 | 451 | """) | ||
296 | 452 | with patch("assess_container_networking.assess_container_networking", | ||
297 | 453 | autospec=True) as mock_assess: | ||
298 | 454 | with self.patch_bootstrap_manager() as mock_bc: | ||
299 | 455 | with self.patch_main(argv, client, logging.INFO): | ||
300 | 456 | ret = jcnet.main(argv) | ||
301 | 457 | client.enable_container_address_allocation.assert_called_once_with() | ||
302 | 458 | self.assertEqual(client.env.environment, "an-env-mod") | ||
303 | 459 | self.assertEqual("", self.log_stream.getvalue()) | ||
304 | 460 | self.assertEqual(mock_bc.call_count, 0) | ||
305 | 461 | mock_assess.assert_called_once_with(client, None) | ||
306 | 462 | self.assertEqual(ret, 0) | ||
307 | 463 | |||
308 | 464 | def test_clean_missing_env(self): | ||
309 | 465 | argv = ["an-env", "/bin/juju", "/tmp/logs", "an-env-mod", | ||
310 | 466 | "--clean-environment"] | ||
311 | 467 | client = Mock(spec=["bootstrap", "enable_container_address_allocation", | ||
312 | 468 | "env", "get_status", "is_jes_enabled", "wait_for", | ||
313 | 469 | "wait_for_started"]) | ||
314 | 470 | client.get_status.side_effect = [ | ||
315 | 471 | Exception("Timeout"), | ||
316 | 472 | Status.from_text(""" | ||
317 | 473 | machines: | ||
318 | 474 | "0": | ||
319 | 475 | agent-state: started | ||
320 | 476 | """), | ||
321 | 477 | ] | ||
322 | 478 | with patch("assess_container_networking.assess_container_networking", | ||
323 | 479 | autospec=True) as mock_assess: | ||
324 | 480 | with self.patch_bootstrap_manager() as mock_bc: | ||
325 | 481 | with self.patch_main(argv, client, logging.INFO): | ||
326 | 482 | ret = jcnet.main(argv) | ||
327 | 483 | client.enable_container_address_allocation.assert_called_once_with() | ||
328 | 484 | self.assertEqual(client.env.environment, "an-env-mod") | ||
329 | 485 | client.bootstrap.assert_called_once_with(False) | ||
330 | 486 | self.assertEqual( | ||
331 | 487 | "INFO Could not clean existing env: Timeout\n", | ||
332 | 488 | self.log_stream.getvalue()) | ||
333 | 489 | self.assertEqual(mock_bc.call_count, 1) | ||
334 | 490 | mock_assess.assert_called_once_with(client, None) | ||
335 | 491 | self.assertEqual(ret, 0) |
Looks reasonable to me, except I don't understand the update_env call. Shame I didn't ask about why you needed it on the call, but now I can't see why you'd need it.
When you create the client for --clean- environment, you want to be using the actual previous configuration, so that you get the right API server addresses. Since that actual previous configuration used the temp-env-name, presumably you'd need to load the environment using the temp-env-name. And in that case, the environment shouldn't need updating at all (because it was created using update_env).