Merge lp:~dooferlad/juju-ci-tools/addressable-containers-assess into lp:juju-ci-tools

[James Tunnicliffe]

https://code.launchpad.net/~dooferlad/juju-ci-tools/juju-ci-tools-addressable-containers/+merge/267494 has been split into two changes. This is the second.

This adds tests that check container networking as set up by the Juju addressable containers feature.

[Aaron Bentley (abentley)]

Thanks.

The file for testing assess_container_networking should be named test_assess_container_networking.

You are treating args.juju_bin as if it's the path to the directory, but it's the path to the binary.

I'm still having trouble understanding why we need something as complicated as MachineGetter. Here's a patch that I think could remove the need for it:
https://pastebin.canonical.com/140360/

Does that seem sane? Just make a bunch of machines and containers and then use them?

See also inline comments.

review: Needs Information

[James Tunnicliffe (dooferlad)]

Thanks Aaron,

Yes, your patch seems entirely reasonable. What I had evolved into
something much more complicated than was needed! Thanks for calling me out
on it.

On Tue, Sep 22, 2015 at 8:02 PM, Aaron Bentley <email address hidden>
wrote:

> Review: Needs Information
>
> Thanks.
>
> The file for testing assess_container_networking should be named
> test_assess_container_networking.
>
> You are treating args.juju_bin as if it's the path to the directory, but
> it's the path to the binary.
>
> I'm still having trouble understanding why we need something as
> complicated as MachineGetter. Here's a patch that I think could remove the
> need for it:
> https://pastebin.canonical.com/140360/
>
> Does that seem sane? Just make a bunch of machines and containers and
> then use them?
>
> See also inline comments.
>
> Diff comments:
>
> > === modified file 'assess_container_networking.py'
> > --- assess_container_networking.py 2015-09-21 16:08:26 +0000
> > +++ assess_container_networking.py 2015-09-21 16:08:26 +0000
> > @@ -148,3 +218,210 @@
> > self.requested_machines.append(machine)
> > if self._new_machine_count() == 0:
> > return
> > +
> > +
> > +def find_network(client, machine, addr):
> > + """Find a connected subnet containing the given address.
> > +
> > + When using this to find the subnet of a container, don't use the
> container
> > + as the machine to run the ip route show command on ("machine"), use
> a real
> > + box because lxc will just send everything to its host machine, so
> it is on
> > + a subnet containing itself. Not much use.
> > + :param client: A Juju client
> > + :param machine: ID of the machine on which to run a command
> > + :param addr: find the connected subnet containing this address
> > + :return: CIDR containing the address if found, else, None
> > + """
> > + ip_cmd = ' '.join(['ip', 'route', 'show', 'to', 'match', addr])
> > + routes = ssh(client, machine, ip_cmd)
> > +
> > + for route in re.findall(r'^(\S+).*[\d\.]+/\d+', routes,
> re.MULTILINE):
> > + if route != 'default':
> > + return route
> > +
> > + raise ValueError("Unable to find route to %r" % addr)
> > +
> > +
> > +def assess_network_traffic(client, targets):
> > + """Test that all containers in target can talk to target[0]
> > + :param client: Juju client
> > + :param targets: machine IDs of machines to test
> > + :return: None;
> > + """
> > + status = client.wait_for_started().status
> > + source = targets[0]
> > + dests = targets[1:]
> > +
> > + with tempfile.NamedTemporaryFile(delete=False) as f:
> > + f.write('tmux new-session -d -s test "nc -l 6778 >
> nc_listen.out"')
> > + client.juju('scp', (f.name, source + ':/home/ubuntu/listen.sh'))
> > + os.remove(f.name)
> > +
> > + # Containers are named 'x/type/y' where x is the host of the
> container. We
> > + host = source.split('/')[0]
> > + address = status['machines'][host]['containers'][source]['dns-name']
> > +
> > + for dest in dests:
> > + ssh(client, source, 'rm nc_listen.out; bash ./listen.sh')
> > + ssh(client...

[lp:~dooferlad/juju-ci-tools/addressable-containers-assess]

1094. By James Tunnicliffe on 2015-09-24

retry ssh commands if they fail with code 255, which seems to happen on heavily loaded machines.

1095. By James Tunnicliffe on 2015-09-24

Fixed missing import

1096. By James Tunnicliffe on 2015-09-25

SSH wrapper now trying even harder to retry on connection errors.
clean_environment is more able to cope with Juju getting confused and retrying.
Use random data in assess_network_traffic to avoid stale data triggering false positive in tests.
Not using asserts.
Clean out machines from the environment again (accidental check in of debug code).
Fixed dumping logs at the end of the test.
Better fix for EnvJujuClient.get_status.

1097. By James Tunnicliffe on 2015-09-25

Merge from addressable-containers-tools

1098. By James Tunnicliffe on 2015-11-10

A bit more reliability hacking around ssh and reboots.
Find a private address to ping for testing rather than 8.8.8.8.
Generate the correct exit code on failure.
join URLs using urljoin, not string concatination (copes with if a / is needed or not). This may need to be expanded to more areas of the code.

1099. By James Tunnicliffe on 2015-11-10

merge upstream changes

1100. By James Tunnicliffe on 2015-11-11

Merged upstream

[Aaron Bentley (abentley)]

This looks pretty good.

There are bunch of files that you've removed the execute bit from. Please don't do that.

See other comments inline.

review: Needs Fixing

[lp:~dooferlad/juju-ci-tools/addressable-containers-assess]

1101. By James Tunnicliffe on 2015-11-13

Merge latest addressable-containers-tools

1102. By James Tunnicliffe on 2015-11-13

Fixed up tests.

1103. By James Tunnicliffe on 2015-11-13

Removed unused imports

1104. By James Tunnicliffe on 2015-11-13

Removed unused variable

1105. By James Tunnicliffe on 2015-11-16

Fixed up tests.
Use wait_for_port instead of a simple delay where possible.

[Aaron Bentley (abentley)]

You haven't addressed these comments from 2015-11-13:
+ for host in hosts:
280 + ssh(client, host, 'sudo reboot')
281 +
282 + # If wait_for_started is called too early the machines still appear to be
283 + # up, so we wait a few seconds for the reboot.
284 + time.sleep(10)
Aaron Bentley (abentley) wrote on 2015-11-13:
I think it would be more reliable to use wait_for_port(22, closed=True) here.

365 --- substrate.py 2015-09-17 20:39:25 +0000
366 +++ substrate.py 2015-11-11 19:51:39 +0000
367 @@ -1,3 +1,4 @@
368 +__metaclass__ = type
Aaron Bentley (abentley) wrote on 2015-11-13:
Assigning metaclass must go below any imports or it will break lint on wily.

372 @@ -6,7 +7,7 @@
373 import os
374 import subprocess
375 from time import sleep
376 -
377 +import urlparse
378 from utility import temp_dir
Aaron Bentley (abentley) wrote on 2015-11-13:
There should be a blank line between standard library imports and local application imports.

[lp:~dooferlad/juju-ci-tools/addressable-containers-assess]

1106. By James Tunnicliffe on 2015-11-26

Fixed subnets/<ver>/?op=reserved_ip_ranges when no IP addresses had been explicitly added.

[James Tunnicliffe (dooferlad)]

Sorry, tried wait_for_port(hostname, 22, closed=True) but it was flaky
- should have commented on the merge proposal. Of course today when I
try again it has worked perfectly three times in a row and failed with
a DNS error once, which I am not going to blame it for. Whatever I am
doing differently today seems to be fine.

On Mon, Nov 23, 2015 at 5:15 PM, Aaron Bentley
<email address hidden> wrote:
> You haven't addressed these comments from 2015-11-13:
> + for host in hosts:
> 280 + ssh(client, host, 'sudo reboot')
> 281 +
> 282 + # If wait_for_started is called too early the machines still appear to be
> 283 + # up, so we wait a few seconds for the reboot.
> 284 + time.sleep(10)
> Aaron Bentley (abentley) wrote on 2015-11-13:
> I think it would be more reliable to use wait_for_port(22, closed=True) here.
>
> 365 --- substrate.py 2015-09-17 20:39:25 +0000
> 366 +++ substrate.py 2015-11-11 19:51:39 +0000
> 367 @@ -1,3 +1,4 @@
> 368 +__metaclass__ = type
> Aaron Bentley (abentley) wrote on 2015-11-13:
> Assigning metaclass must go below any imports or it will break lint on wily.
>
>
> 372 @@ -6,7 +7,7 @@
> 373 import os
> 374 import subprocess
> 375 from time import sleep
> 376 -
> 377 +import urlparse
> 378 from utility import temp_dir
> Aaron Bentley (abentley) wrote on 2015-11-13:
> There should be a blank line between standard library imports and local application imports.
> --
> https://code.launchpad.net/~dooferlad/juju-ci-tools/addressable-containers-assess/+merge/271837
> You are the owner of lp:~dooferlad/juju-ci-tools/addressable-containers-assess.

[lp:~dooferlad/juju-ci-tools/addressable-containers-assess]

1107. By James Tunnicliffe on 2015-11-26

Doh!

1108. By James Tunnicliffe on 2015-11-26

Fixed up reboot ordering and waits.

[James Tunnicliffe (dooferlad)]

That should do it. Please review.

review: Resubmit

[Aaron Bentley (abentley)]

Thanks for your changes. You haven't addressed this comment yet:

372 @@ -6,7 +7,7 @@
373 import os
374 import subprocess
375 from time import sleep
376 -
377 +import urlparse
378 from utility import temp_dir
Aaron Bentley (abentley) wrote on 2015-11-13:
There should be a blank line between standard library imports and local application imports.

[lp:~dooferlad/juju-ci-tools/addressable-containers-assess]

1109. By James Tunnicliffe on 2015-11-27

Fixed import spacing

[James Tunnicliffe (dooferlad)]

Bother. Sorry. Fix on its way.

On Fri, Nov 27, 2015 at 3:06 PM, Aaron Bentley
<email address hidden> wrote:
> Thanks for your changes. You haven't addressed this comment yet:
>
> 372 @@ -6,7 +7,7 @@
> 373 import os
> 374 import subprocess
> 375 from time import sleep
> 376 -
> 377 +import urlparse
> 378 from utility import temp_dir
> Aaron Bentley (abentley) wrote on 2015-11-13:
> There should be a blank line between standard library imports and local application imports.
> --
> https://code.launchpad.net/~dooferlad/juju-ci-tools/addressable-containers-assess/+merge/271837
> You are the owner of lp:~dooferlad/juju-ci-tools/addressable-containers-assess.

[James Tunnicliffe (dooferlad)]

Fix pushed.

[Aaron Bentley (abentley)]

You have added a blank line between "standard library imports" and "local application imports", but you have also removed the blank line between "related third party imports" and "local application imports". I will fix this, but in the future, please try to follow PEP8 style more closely for imports.

review: Approve

=== modified file 'assess_container_networking.py'
--- assess_container_networking.py	2015-09-25 15:52:47 +0000
+++ assess_container_networking.py	2015-11-27 15:17:35 +0000
@@ -1,18 +1,99 @@
 #!/usr/bin/env python
 from __future__ import print_function
-import subprocess
 from copy import (
     copy,
     deepcopy,
 )
+import logging
 import time
-
-__metaclass__ = type
-
+import re
+import tempfile
+import os
+import subprocess
+from textwrap import dedent
+from argparse import ArgumentParser
+
+from jujuconfig import (
+    get_juju_home,
+)
+from jujupy import (
+    parse_new_state_server_from_error,
+    temp_bootstrap_env,
+    SimpleEnvironment,
+    EnvJujuClient,
+)
+from utility import (
+    wait_for_port,
+    print_now,
+    add_basic_testing_arguments,
+)
+from deploy_stack import (
+    update_env,
+    dump_env_logs,
+    get_random_string,
+)
 
 KVM_MACHINE = 'kvm'
 LXC_MACHINE = 'lxc'
 
+__metaclass__ = type
+
+
+def parse_args(argv=None):
+    """Parse all arguments."""
+
+    description = dedent("""\
+    Test container address allocation.
+    For LXC and KVM, create machines of each type and test the network
+    between LXC <--> LXC, KVM <--> KVM and LXC <--> KVM. Also test machine
+    to outside world, DNS and that these tests still pass after a reboot. In
+    case of failure pull logs and configuration files from the machine that
+    we detected a problem on for later analysis.
+    """)
+    parser = add_basic_testing_arguments(ArgumentParser(
+        description=description
+    ))
+    parser.add_argument(
+        '--machine-type',
+        help='Which virtual machine/container type to test. Defaults to all.',
+        choices=[KVM_MACHINE, LXC_MACHINE])
+    parser.add_argument(
+        '--clean-environment', action='store_true', help=dedent("""\
+        Attempts to re-use an existing environment rather than destroying it
+        and creating a new one.
+
+        On launch, if an environment exists, clean out services and machines
+        from it rather than destroying it. If an environment doesn't exist,
+        create one and use it.
+
+        At termination, clean out services and machines from the environment
+        rather than destroying it."""))
+    return parser.parse_args(argv)
+
+
+def ssh(client, machine, cmd):
+    """Convenience function: run a juju ssh command and get back the output
+    :param client: A Juju client
+    :param machine: ID of the machine on which to run a command
+    :param cmd: the command to run
+    :return: text output of the command
+    """
+    back_off = 2
+    attempts = 4
+    for attempt in range(attempts):
+        try:
+            return client.get_juju_output('ssh', machine, cmd)
+        except subprocess.CalledProcessError as e:
+            # If the connection to the host failed, try again in a couple of
+            # seconds. This is usually due to heavy load.
+            if(attempt < attempts-1 and
+               re.search('ssh_exchange_identification: '
+                         'Connection closed by remote host', e.stderr)):
+                time.sleep(back_off)
+                back_off *= 2
+            else:
+                raise
+
 
 def clean_environment(client, services_only=False):
     """Remove all the services and, optionally, machines from an environment.
@@ -49,7 +130,7 @@
                     # containers on it. Normally a small pause and trying
                     # again is all that is needed to resolve this issue.
                     time.sleep(2)
-                    s = client.wait_for_started()
+                    client.wait_for_started()
                     client.juju('remove-machine', m)
 
         client.wait_for('machines-not-0', 'none')
@@ -102,3 +183,259 @@
             containers[type][host].append(c[0])
 
     return hosts, containers
+
+
+def find_network(client, machine, addr):
+    """Find a connected subnet containing the given address.
+
+    When using this to find the subnet of a container, don't use the container
+    as the machine to run the ip route show command on ("machine"), use a real
+    box because lxc will just send everything to its host machine, so it is on
+    a subnet containing itself. Not much use.
+    :param client: A Juju client
+    :param machine: ID of the machine on which to run a command
+    :param addr: find the connected subnet containing this address
+    :return: CIDR containing the address if found, else, None
+    """
+    ip_cmd = ' '.join(['ip', 'route', 'show', 'to', 'match', addr])
+    routes = ssh(client, machine, ip_cmd)
+
+    for route in re.findall(r'^(\S+).*[\d\.]+/\d+', routes, re.MULTILINE):
+        if route != 'default':
+            return route
+
+    raise ValueError("Unable to find route to %r" % addr)
+
+
+def assess_network_traffic(client, targets):
+    """Test that all containers in target can talk to target[0]
+    :param client: Juju client
+    :param targets: machine IDs of machines to test
+    :return: None;
+    """
+    status = client.wait_for_started().status
+    source = targets[0]
+    dests = targets[1:]
+
+    with tempfile.NamedTemporaryFile(delete=False) as f:
+        f.write('tmux new-session -d -s test "nc -l 6778 > nc_listen.out"')
+    client.juju('scp', (f.name, source + ':/home/ubuntu/listen.sh'))
+    os.remove(f.name)
+
+    # Containers are named 'x/type/y' where x is the host of the container. We
+    host = source.split('/')[0]
+    address = status['machines'][host]['containers'][source]['dns-name']
+
+    for dest in dests:
+        msg = get_random_string()
+        ssh(client, source, 'rm nc_listen.out; bash ./listen.sh')
+        ssh(client, dest,
+            'echo "{msg}" | nc {addr} 6778'.format(msg=msg, addr=address))
+        result = ssh(client, source, 'more nc_listen.out')
+        if result.rstrip() != msg:
+            raise ValueError("Wrong or missing message: %r" % result.rstrip())
+
+
+def private_address(client, host):
+    default_route = ssh(client, host, 'ip -4 -o route list 0/0')
+    device = re.search(r'(\w+)\s*$', default_route).group(1)
+    device_ip = ssh(client, host, 'ip -4 -o addr show {}'.format(device))
+    return re.search(r'inet\s+(\S+)/\d+\s', device_ip).group(1)
+
+
+def assess_address_range(client, targets):
+    """Test that two containers are in the same subnet as their host
+    :param client: Juju client
+    :param targets: machine IDs of machines to test
+    :return: None; raises ValueError on failure
+    """
+    status = client.wait_for_started().status
+
+    host_subnet_cache = {}
+
+    for target in targets:
+        host = target.split('/')[0]
+
+        if host in host_subnet_cache:
+            host_subnet = host_subnet_cache[host]
+        else:
+            host_address = private_address(client, host)
+            host_subnet = find_network(client, host, host_address)
+            host_subnet_cache[host] = host_subnet
+
+        addr = status['machines'][host]['containers'][target]['dns-name']
+        subnet = find_network(client, host, addr)
+        if host_subnet != subnet:
+            raise ValueError(
+                '{} ({}) not on the same subnet as {} ({})'.format(
+                    target, subnet, host, host_subnet))
+
+
+def assess_internet_connection(client, targets):
+    """Test that targets can ping their default route
+    :param client: Juju client
+    :param targets: machine IDs of machines to test
+    :return: None; raises ValueError on failure
+    """
+
+    for target in targets:
+        routes = ssh(client, target, 'ip route show')
+
+        d = re.search(r'^default\s+via\s+([\d\.]+)\s+', routes, re.MULTILINE)
+        if d:
+            rc = client.juju('ssh', (target, 'ping -c1 -q ' + d.group(1)),
+                             check=False)
+            if rc != 0:
+                raise ValueError('%s unable to ping default route' % target)
+        else:
+            raise ValueError("Default route not found")
+
+
+def _assessment_iteration(client, containers):
+    """Run the network tests on this collection of machines and containers
+    :param client: Juju client
+    :param hosts: list of hosts of containers
+    :param containers: list of containers to run tests between
+    :return: None
+    """
+    assess_internet_connection(client, containers)
+    assess_address_range(client, containers)
+    assess_network_traffic(client, containers)
+
+
+def _assess_container_networking(client, types, hosts, containers):
+    """Run _assessment_iteration on all useful combinations of containers
+    :param client: Juju client
+    :param args: Parsed command line arguments
+    :return: None
+    """
+    for container_type in types:
+        # Test with two containers on the same host
+        _assessment_iteration(client, containers[container_type][hosts[0]])
+
+        # Now test with two containers on two different hosts
+        test_containers = [
+            containers[container_type][hosts[0]][0],
+            containers[container_type][hosts[1]][0],
+            ]
+        _assessment_iteration(client, test_containers)
+
+    if KVM_MACHINE in types and LXC_MACHINE in types:
+        test_containers = [
+            containers[LXC_MACHINE][hosts[0]][0],
+            containers[KVM_MACHINE][hosts[0]][0],
+            ]
+        _assessment_iteration(client, test_containers)
+
+        # Test with an LXC and a KVM on different machines
+        test_containers = [
+            containers[LXC_MACHINE][hosts[0]][0],
+            containers[KVM_MACHINE][hosts[1]][0],
+            ]
+        _assessment_iteration(client, test_containers)
+
+
+def assess_container_networking(client, args):
+    """Runs _assess_address_allocation, reboots hosts, repeat.
+    :param client: Juju client
+    :param args: Parsed command line arguments
+    :return: None
+    """
+    # Only test the containers we were asked to test
+    if args.machine_type:
+        types = [args.machine_type]
+    else:
+        types = [KVM_MACHINE, LXC_MACHINE]
+
+    hosts, containers = make_machines(client, types)
+    status = client.wait_for_started().status
+
+    _assess_container_networking(client, types, hosts, containers)
+
+    # Reboot all hosts apart from machine 0 because we use machine 0 to jump
+    # through for some hosts.
+    for host in hosts[1:]:
+        ssh(client, host, 'sudo reboot')
+
+    # Finally reboot machine 0
+    ssh(client, hosts[0], 'sudo reboot')
+
+    # Wait for the state server to shut down. This prevents us from calling
+    # wait_for_started before machine 0 has shut down, which can cause us
+    # to think that we have finished rebooting before we actually have.
+    hostname = status['machines'][hosts[0]]['dns-name']
+    wait_for_port(hostname, 22, closed=True)
+
+    client.wait_for_started()
+
+    # Once Juju is up it can take a little while before ssh responds.
+    for host in hosts:
+        hostname = status['machines'][host]['dns-name']
+        wait_for_port(hostname, 22, timeout=240)
+
+    _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
+    )
+    client.enable_container_address_allocation()
+    update_env(client.env, args.temp_env_name)
+    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):
+                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)
+
+if __name__ == '__main__':
+    main()

=== modified file 'substrate.py'
--- substrate.py	2015-11-13 16:27:48 +0000
+++ substrate.py	2015-11-27 15:17:35 +0000
@@ -6,9 +6,9 @@
 import os
 import subprocess
 from time import sleep
+import urlparse
 
 from utility import temp_dir
-
 from boto import ec2
 from boto.exception import EC2ResponseError
 
@@ -395,7 +395,7 @@
 
     def __init__(self, profile, url, oauth):
         self.profile = profile
-        self.url = url + 'api/1.0/'
+        self.url = urlparse.urljoin(url, 'api/1.0/')
         self.oauth = oauth
 
     @classmethod

=== modified file 'test_assess_container_networking.py'
--- test_assess_container_networking.py	2015-11-13 16:35:35 +0000
+++ test_assess_container_networking.py	2015-11-27 15:17:35 +0000
@@ -12,6 +12,8 @@
 import assess_container_networking as jcnet
 from copy import deepcopy
 from contextlib import contextmanager
+from tests import parse_error
+
 
 __metaclass__ = type
 
@@ -162,6 +164,68 @@
             patcher.start()
             self.addCleanup(patcher.stop)
 
+    def assert_ssh(self, args, machine, cmd):
+        self.assertEqual(args, [('ssh', machine, cmd), ])
+
+    def test_parse_args(self):
+        # Test a simple command line that should work
+        cmdline = ['env', 'juju_bin', 'logs', 'ten']
+        args = jcnet.parse_args(cmdline)
+        self.assertEqual(args.machine_type, None)
+        self.assertEqual(args.juju_bin, 'juju_bin')
+        self.assertEqual(args.env, 'env')
+        self.assertEqual(args.logs, 'logs')
+        self.assertEqual(args.temp_env_name, 'ten')
+        self.assertEqual(args.debug, False)
+        self.assertEqual(args.upload_tools, False)
+        self.assertEqual(args.clean_environment, False)
+
+        # check the optional arguments
+        opts = ['--machine-type', jcnet.KVM_MACHINE, '--debug',
+                '--upload-tools', '--clean-environment']
+        args = jcnet.parse_args(cmdline + opts)
+        self.assertEqual(args.machine_type, jcnet.KVM_MACHINE)
+        self.assertEqual(args.debug, True)
+        self.assertEqual(args.upload_tools, True)
+        self.assertEqual(args.clean_environment, True)
+
+        # Now check that we can only set machine_type to kvm or lxc
+        opts = ['--machine-type', jcnet.LXC_MACHINE]
+        args = jcnet.parse_args(cmdline + opts)
+        self.assertEqual(args.machine_type, jcnet.LXC_MACHINE)
+
+        # Set up an error (bob is invalid)
+        opts = ['--machine-type', 'bob']
+        with parse_error(self) as stderr:
+            jcnet.parse_args(cmdline + opts)
+        self.assertRegexpMatches(
+            stderr.getvalue(),
+            ".*error: argument --machine-type: invalid choice: 'bob'.*")
+
+    def test_ssh(self):
+        machine, addr = '0', 'foobar'
+        with patch.object(self.client, 'get_juju_output',
+                          autospec=True) as ssh_mock:
+            jcnet.ssh(self.client, machine, addr)
+            self.assertEqual(1, ssh_mock.call_count)
+            self.assert_ssh(ssh_mock.call_args, machine, addr)
+
+    def test_find_network(self):
+        machine, addr = '0', '1.1.1.1'
+        self.assertRaisesRegexp(
+            ValueError, "Unable to find route to '1.1.1.1'",
+            jcnet.find_network, self.client, machine, addr)
+
+        self.juju_mock.set_ssh_output([
+            'default via 192.168.0.1 dev eth3\n'
+            '1.1.1.0/24 dev eth3  proto kernel  scope link  src 1.1.1.22',
+        ])
+        self.juju_mock.commands = []
+        jcnet.find_network(self.client, machine, addr)
+        self.assertItemsEqual(self.juju_mock.commands,
+                              [('ssh', (
+                               machine, 'ip route show to match ' + addr))])
+
     def test_clean_environment(self):
         self.juju_mock.add_machine('1')
         self.juju_mock.add_machine('2')
@@ -229,3 +293,92 @@
                     '1': ['1/kvm/0']},
         }
         self.assertDictEqual(containers, expected)
+
+    def test_test_network_traffic(self):
+        targets = ['0/lxc/0', '0/lxc/1']
+        self.juju_mock.set_status({'machines': {'0': {
+            'containers': {targets[0]: {'dns-name': '0-dns-name'}}}}})
+
+        with patch('assess_container_networking.get_random_string',
+                   lambda *args, **kw: 'hello'):
+
+            self.juju_mock.set_ssh_output(['', '', 'hello'])
+            jcnet.assess_network_traffic(self.client, targets)
+
+            self.juju_mock.reset_calls()
+            self.juju_mock.set_ssh_output(['', '', 'fail'])
+            self.assertRaisesRegexp(
+                ValueError, "Wrong or missing message: 'fail'",
+                jcnet.assess_network_traffic, self.client, targets)
+
+    def test_test_address_range(self):
+        targets = ['0/lxc/0', '0/lxc/1']
+        self.juju_mock.set_status({'machines': {'0': {
+            'containers': {
+                targets[0]: {'dns-name': 'lxc0-dns-name'},
+                targets[1]: {'dns-name': 'lxc1-dns-name'},
+            },
+            'dns-name': '0-dns-name',
+        }}})
+        self.juju_mock.set_ssh_output([
+            'default via 192.168.0.1 dev eth3',
+            '2: eth3    inet 192.168.0.22/24 brd 192.168.0.255 scope '
+            'global eth3\       valid_lft forever preferred_lft forever',
+            '192.168.0.0/24',
+        ])
+
+        jcnet.assess_address_range(self.client, targets)
+
+    def test_test_address_range_fail(self):
+        targets = ['0/lxc/0', '0/lxc/1']
+        self.juju_mock.set_status({'machines': {'0': {
+            'containers': {
+                targets[0]: {'dns-name': 'lxc0-dns-name'},
+                targets[1]: {'dns-name': 'lxc1-dns-name'},
+            },
+            'dns-name': '0-dns-name',
+        }}})
+        self.juju_mock.set_ssh_output([
+            'default via 192.168.0.1 dev eth3',
+            '2: eth3    inet 192.168.0.22/24 brd 192.168.0.255 scope '
+            'global eth3\       valid_lft forever preferred_lft forever',
+            '192.168.0.0/24',
+            '192.168.1.0/24',
+            '192.168.2.0/24',
+            '192.168.3.0/24',
+        ])
+
+        self.assertRaisesRegexp(
+            ValueError, '0/lxc/0 \S+ not on the same subnet as 0 \S+',
+            jcnet.assess_address_range, self.client, targets)
+
+    def test_test_internet_connection(self):
+        targets = ['0/lxc/0', '0/lxc/1']
+        self.juju_mock.set_status({'machines': {'0': {
+            'containers': {
+                targets[0]: {'dns-name': 'lxc0-dns-name'},
+                targets[1]: {'dns-name': 'lxc1-dns-name'},
+            },
+            'dns-name': '0-dns-name',
+        }}})
+
+        # Can ping default route
+        self.juju_mock.set_ssh_output([
+            'default via 192.168.0.1 dev eth3', 0,
+            'default via 192.168.0.1 dev eth3', 0])
+        jcnet.assess_internet_connection(self.client, targets)
+
+        # Can't ping default route
+        self.juju_mock.set_ssh_output([
+            'default via 192.168.0.1 dev eth3', 1])
+        self.juju_mock.reset_calls()
+        self.assertRaisesRegexp(
+            ValueError, "0/lxc/0 unable to ping default route",
+            jcnet.assess_internet_connection, self.client, targets)
+
+        # Can't find default route
+        self.juju_mock.set_ssh_output(['', 1])
+        self.juju_mock.reset_calls()
+        self.assertRaisesRegexp(
+            ValueError, "Default route not found",
+            jcnet.assess_internet_connection, self.client, targets)