Merge lp:~gz/juju-ci-tools/common_container_networking into lp:juju-ci-tools

[Martin Packman]

Make assess_container_networking function with jes

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)]

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).

review: Needs Information

[Aaron Bentley (abentley)]

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.

review: Approve

[Martin Packman (gz)]

Right, I think what I really want is an alternative first step method on BootstrapManager that loads the tempenv rather than does the bootstrap.

=== modified file 'assess_container_networking.py'
--- assess_container_networking.py	2015-12-12 00:40:50 +0000
+++ assess_container_networking.py	2016-01-14 17:48:44 +0000
@@ -9,28 +9,24 @@
 import re
 import os
 import subprocess
+import sys
 import tempfile
 from textwrap import dedent
 import time
 
 from deploy_stack import (
+    BootstrapManager,
+    get_random_string,
     update_env,
-    dump_env_logs,
-    get_random_string,
-)
-from jujuconfig import (
-    get_juju_home,
 )
 from jujupy import (
-    parse_new_state_server_from_error,
-    temp_bootstrap_env,
     SimpleEnvironment,
     EnvJujuClient,
 )
 from utility import (
+    add_basic_testing_arguments,
+    configure_logging,
     wait_for_port,
-    print_now,
-    add_basic_testing_arguments,
 )
 
 
@@ -40,6 +36,9 @@
 __metaclass__ = type
 
 
+log = logging.getLogger("assess_container_networking")
+
+
 def parse_args(argv=None):
     """Parse all arguments."""
 
@@ -69,7 +68,11 @@
 
         At termination, clean out services and machines from the environment
         rather than destroying it."""))
-    return parser.parse_args(argv)
+    args = parser.parse_args(argv)
+    # Passing --clean-environment implies --keep-env
+    if args.clean_environment:
+        args.keep_env = True
+    return args
 
 
 def ssh(client, machine, cmd):
@@ -107,9 +110,14 @@
     # A short timeout is used for get_status here because if we don't get a
     # response from  get_status quickly then the environment almost
     # certainly doesn't exist or needs recreating.
-    status = client.get_status(5)
+    try:
+        status = client.get_status(5)
+    except Exception as e:
+        # TODO(gz): get_status should return a more specific error type.
+        log.info("Could not clean existing env: %s", e)
+        return False
 
-    for service in status.status['services']:
+    for service in status.status.get('services', {}):
         client.juju('remove-service', service)
 
     if not services_only:
@@ -214,6 +222,7 @@
     :param targets: machine IDs of machines to test
     :return: None;
     """
+    log.info('Waiting for the bootstrap machine agent to start.')
     status = client.wait_for_started().status
     source = targets[0]
     dests = targets[1:]
@@ -336,15 +345,15 @@
         _assessment_iteration(client, test_containers)
 
 
-def assess_container_networking(client, args):
+def assess_container_networking(client, machine_type):
     """Runs _assess_address_allocation, reboots hosts, repeat.
     :param client: Juju client
-    :param args: Parsed command line arguments
+    :param machine_type: Container types to test
     :return: None
     """
     # Only test the containers we were asked to test
-    if args.machine_type:
-        types = [args.machine_type]
+    if machine_type:
+        types = [machine_type]
     else:
         types = [KVM_MACHINE, LXC_MACHINE]
 
@@ -377,69 +386,30 @@
     _assess_container_networking(client, types, hosts, containers)
 
 
-def get_client(args):
-    client = EnvJujuClient.by_version(
-        SimpleEnvironment.from_config(args.env),
-        args.juju_bin, args.debug
-    )
+def main(argv=None):
+    args = parse_args(argv)
+    configure_logging(args.verbose)
+    bs_manager = BootstrapManager.from_args(args)
+    client = bs_manager.client
     client.enable_container_address_allocation()
-    update_env(client.env, args.temp_env_name,
-               series=args.series, bootstrap_host=args.bootstrap_host,
-               agent_url=args.agent_url, agent_stream=args.agent_stream,
-               region=args.region)
-    return client
-
-
-def main():
-    args = parse_args()
-    client = get_client(args)
-    juju_home = get_juju_home()
-    bootstrap_host = None
-    success = True
-    try:
-        if args.clean_environment:
-            try:
-                if not clean_environment(client):
-                    with temp_bootstrap_env(juju_home, client):
-                        client.bootstrap(args.upload_tools)
-            except Exception as e:
-                logging.exception(e)
-                client.destroy_environment()
-                client = get_client(args)
-                with temp_bootstrap_env(juju_home, client):
-                    client.bootstrap(args.upload_tools)
-        else:
-            client.destroy_environment()
-            client = get_client(args)
-            with temp_bootstrap_env(juju_home, client):
+    # TODO(gz): Having to manipulate client env state here to get the temp env
+    #           is ugly, would ideally be captured in an explicit scope.
+    update_env(client.env, bs_manager.temp_env_name, series=bs_manager.series,
+               agent_url=bs_manager.agent_url,
+               agent_stream=bs_manager.agent_stream, region=bs_manager.region)
+    with bs_manager.top_context() as machines:
+        bootstrap_required = True
+        if args.clean_environment and clean_environment(client):
+            bootstrap_required = False
+        if bootstrap_required:
+            with bs_manager.bootstrap_context(machines):
                 client.bootstrap(args.upload_tools)
-
-        logging.info('Waiting for the bootstrap machine agent to start.')
-        status = client.wait_for_started()
-        mid, data = list(status.iter_machines())[0]
-        bootstrap_host = data['dns-name']
-
-        assess_container_networking(client, args)
-
-    except Exception as e:
-        success = False
-        logging.exception(e)
-        try:
-            if bootstrap_host is None:
-                bootstrap_host = parse_new_state_server_from_error(e)
-        except Exception as e:
-            print_now('exception while dumping logs:\n')
-            logging.exception(e)
-    finally:
-        if bootstrap_host is not None:
-            dump_env_logs(client, bootstrap_host, args.logs)
-
-        if args.clean_environment:
-            clean_environment(client)
-        else:
-            client.destroy_environment()
-        if not success:
-            exit(1)
+        with bs_manager.runtime_context(machines):
+            assess_container_networking(client, args.machine_type)
+        if args.clean_environment and not clean_environment(client):
+            return 1
+    return 0
+
 
 if __name__ == '__main__':
-    main()
+    sys.exit(main())

=== modified file 'tests/test_assess_container_networking.py'
--- tests/test_assess_container_networking.py	2015-12-14 16:23:40 +0000
+++ tests/test_assess_container_networking.py	2016-01-14 17:48:44 +0000
@@ -1,6 +1,7 @@
 from argparse import Namespace
 from copy import deepcopy
 from contextlib import contextmanager
+import logging
 
 from mock import (
     patch,
@@ -16,6 +17,7 @@
 
 import assess_container_networking as jcnet
 from tests import (
+    FakeHomeTestCase,
     parse_error,
     TestCase,
 )
@@ -389,18 +391,101 @@
             ValueError, "Default route not found",
             jcnet.assess_internet_connection, self.client, targets)
 
-    def test_get_client(self):
-        args = Namespace(
-            env="e", juju_bin="jb", debug=False,
-            agent_stream="http://tools.testing/agents", temp_env_name="te",
-            series="s", bootstrap_host="bh", agent_url="au", region="r")
-
-        upenv = MagicMock()
-        with patch.object(EnvJujuClient, "by_version"), \
-            patch.object(SimpleEnvironment, "from_config"), \
-                patch("assess_container_networking.update_env", upenv):
-
-                jcnet.get_client(args)
-                self.assertEqual(upenv.call_args[0][1], args.temp_env_name)
-                for key, value in upenv.call_args[1].iteritems():
-                    self.assertEqual(vars(args)[key], value)
+
+class TestMain(FakeHomeTestCase):
+
+    @contextmanager
+    def patch_main(self, argv, client, log_level, debug=False):
+        env = SimpleEnvironment(argv[0], {"type": "ec2"})
+        client.env = env
+        with patch("assess_container_networking.configure_logging",
+                   autospec=True) as mock_cl:
+            with patch("jujupy.SimpleEnvironment.from_config",
+                       return_value=env) as mock_e:
+                with patch("jujupy.EnvJujuClient.by_version",
+                           return_value=client) as mock_c:
+                    yield
+        mock_cl.assert_called_once_with(log_level)
+        mock_e.assert_called_once_with(argv[0])
+        mock_c.assert_called_once_with(env, argv[1], debug=debug)
+
+    @contextmanager
+    def patch_bootstrap_manager(self, runs=True):
+        with patch("deploy_stack.BootstrapManager.top_context",
+                   autospec=True) as mock_tc:
+            with patch("deploy_stack.BootstrapManager.bootstrap_context",
+                       autospec=True) as mock_bc:
+                with patch("deploy_stack.BootstrapManager.runtime_context",
+                           autospec=True) as mock_rc:
+                    yield mock_bc
+        self.assertEqual(mock_tc.call_count, 1)
+        if runs:
+            self.assertEqual(mock_rc.call_count, 1)
+
+    def test_bootstrap_required(self):
+        argv = ["an-env", "/bin/juju", "/tmp/logs", "an-env-mod", "--verbose"]
+        client = Mock(spec=["bootstrap", "enable_container_address_allocation",
+                            "is_jes_enabled"])
+        with patch("assess_container_networking.assess_container_networking",
+                   autospec=True) as mock_assess:
+            with self.patch_bootstrap_manager() as mock_bc:
+                with self.patch_main(argv, client, logging.DEBUG):
+                    ret = jcnet.main(argv)
+        client.enable_container_address_allocation.assert_called_once_with()
+        client.bootstrap.assert_called_once_with(False)
+        self.assertEqual("", self.log_stream.getvalue())
+        self.assertEqual(mock_bc.call_count, 1)
+        mock_assess.assert_called_once_with(client, None)
+        self.assertEqual(ret, 0)
+
+    def test_clean_existing_env(self):
+        argv = ["an-env", "/bin/juju", "/tmp/logs", "an-env-mod",
+                "--clean-environment"]
+        client = Mock(spec=["enable_container_address_allocation", "env",
+                            "get_status", "is_jes_enabled", "wait_for",
+                            "wait_for_started"])
+        client.get_status.return_value = Status.from_text("""
+            machines:
+                "0":
+                    agent-state: started
+        """)
+        with patch("assess_container_networking.assess_container_networking",
+                   autospec=True) as mock_assess:
+            with self.patch_bootstrap_manager() as mock_bc:
+                with self.patch_main(argv, client, logging.INFO):
+                    ret = jcnet.main(argv)
+        client.enable_container_address_allocation.assert_called_once_with()
+        self.assertEqual(client.env.environment, "an-env-mod")
+        self.assertEqual("", self.log_stream.getvalue())
+        self.assertEqual(mock_bc.call_count, 0)
+        mock_assess.assert_called_once_with(client, None)
+        self.assertEqual(ret, 0)
+
+    def test_clean_missing_env(self):
+        argv = ["an-env", "/bin/juju", "/tmp/logs", "an-env-mod",
+                "--clean-environment"]
+        client = Mock(spec=["bootstrap", "enable_container_address_allocation",
+                            "env", "get_status", "is_jes_enabled", "wait_for",
+                            "wait_for_started"])
+        client.get_status.side_effect = [
+            Exception("Timeout"),
+            Status.from_text("""
+                machines:
+                    "0":
+                        agent-state: started
+            """),
+        ]
+        with patch("assess_container_networking.assess_container_networking",
+                   autospec=True) as mock_assess:
+            with self.patch_bootstrap_manager() as mock_bc:
+                with self.patch_main(argv, client, logging.INFO):
+                    ret = jcnet.main(argv)
+        client.enable_container_address_allocation.assert_called_once_with()
+        self.assertEqual(client.env.environment, "an-env-mod")
+        client.bootstrap.assert_called_once_with(False)
+        self.assertEqual(
+            "INFO Could not clean existing env: Timeout\n",
+            self.log_stream.getvalue())
+        self.assertEqual(mock_bc.call_count, 1)
+        mock_assess.assert_called_once_with(client, None)
+        self.assertEqual(ret, 0)