Merge into trunk : addressable-containers-assess : Code : juju-ci-tools

Status:	Merged
Merged at revision:	1173
Proposed branch:	lp:~dooferlad/juju-ci-tools/addressable-containers-assess
Merge into:	lp:juju-ci-tools
Prerequisite:	lp:~dooferlad/juju-ci-tools/addressable-containers-tools
Diff against target:	575 lines (+497/-7) 3 files modified assess_container_networking.py (+342/-5) substrate.py (+2/-2) test_assess_container_networking.py (+153/-0)
To merge this branch:	bzr merge lp:~dooferlad/juju-ci-tools/addressable-containers-assess
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Aaron Bentley (community)	2015-09-21	Approve on 2015-12-08
James Tunnicliffe (community)		Needs Resubmitting on 2015-11-26
Review via email: mp+271837@code.launchpad.net

Revision history for this message

Aaron Bentley (abentley) wrote on 2015-09-22:

#

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

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2015-09-23:

#

Download full text (14.1 KiB)

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

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 <aaron.bentley@canonical.com>
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, dest, 'echo "hello" | nc ' + address + ' 6778')
>
> This seems vulnerable to stale files.  Maybe use
> deploy_stack.get_random_string()?
>
> > +        result = ssh(client, source, 'more nc_listen.out')
> > +        if result.rstrip() != 'hello':
> > +            raise ValueError("Wrong or missing message: %r" %
> result.rstrip())
> > +
> > +
> > +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; will assert failures
> > +    """
> > +    status = client.wait_for_started().status
> > +
> > +    host = targets[0].split('/')[0]
> > +    host_address =
> socket.gethostbyname(status['machines'][host]['dns-name'])
> > +    host_subnet = find_network(client, host, host_address)
> > +
> > +    for target in targets:
> > +        vm_host = target.split('/')[0]
> > +        addr =
> status['machines'][vm_host]['containers'][target]['dns-name']
> > +        subnet = find_network(client, host, addr)
> > +        assert host_subnet == subnet, \
>
> Because python can be run with assertions disabled, they should not be
> used in important code paths.  If you really want, you can do "if foo:
> raise AssertionError".
>
> > +            '{} ({}) not on the same subnet as {} ({})'.format(
> > +                target, subnet, host, host_subnet)
> > +
> > +
> > +def assess_internet_connection(client, targets):
> > +    """Test that targets can ping Google's DNS server, google.com
> > +    :param client: Juju client
> > +    :param targets: machine IDs of machines to test
> > +    :return: None; will assert failures
> > +    """
> > +
> > +    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, args):
> > +    """Run _assessment_iteration on all useful combinations of
> containers
> > +    :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]
> > +
> > +    mg = MachineGetter(client)
> > +    hosts = mg.get(count=2)
> > +
> > +    for container_type in types:
> > +        # Test with two containers on the same host
> > +        containers = mg.get(container_type, count=2)
> > +        _assessment_iteration(client, containers)
> > +
> > +        # Now test with two containers on two different hosts
> > +        containers = [mg.get(container_type, machine=hosts[0])[0],
> > +                      mg.get(container_type, machine=hosts[1])[0]]
> > +        _assessment_iteration(client, containers)
> > +
> > +    if args.machine_type is None:
> > +        # Test with an LXC and a KVM on the same machine
> > +        containers = [mg.get(LXC_MACHINE, machine=hosts[0])[0],
> > +                      mg.get(KVM_MACHINE, machine=hosts[0])[0]]
> > +        _assessment_iteration(client, containers)
> > +
> > +        # Test with an LXC and a KVM on different machines
> > +        containers = [mg.get(LXC_MACHINE, machine=hosts[0])[0],
> > +                      mg.get(KVM_MACHINE, machine=hosts[1])[0]]
> > +        _assessment_iteration(client, 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
> > +    """
> > +    _assess_container_networking(client, args)
> > +    mg = MachineGetter(client)
> > +    hosts = mg.get(count=2)
> > +    for host in hosts:
> > +        ssh(client, host, 'sudo reboot')
> > +    client.wait_for_started()
> > +    _assess_container_networking(client, args)
> > +
> > +
> > +def main():
> > +    args = parse_args()
> > +    client = EnvJujuClient.by_version(
> > +        SimpleEnvironment.from_config(args.env),
> > +        os.path.join(args.juju_bin, 'juju'), args.debug)
>
> args.juju_bin should already end in 'juju', so you shouldn't append it
> here.
>
> > +    client.enable_container_address_allocation()
> > +    update_env(client.env, args.temp_env_name)
> > +    juju_home = get_juju_home()
> > +    bootstrap_host = None
> > +    try:
> > +        if args.clean_environment:
> > +            try:
> > +                clean_environment(client, services_only=True)
>
> Why is this True?  I thought --clean was supposed to clean out machines.
>
> > +            except Exception as e:
> > +                logging.exception(e)
> > +                with temp_bootstrap_env(juju_home, client):
> > +                    client.bootstrap(args.upload_tools)
> > +        else:
> > +            client.destroy_environment()
> > +            client = make_client(
>
> Cannot call make_client with args.juju_bin, because make_client expects a
> directory name, but args.juju_bin is a full path to a file.  I think this
> is not needed at all, because client has already been assigned.
>
> > +                args.juju_bin, args.debug, args.env, args.temp_env_name)
> > +            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:
> > +        logging.exception(e)
> > +        try:
> > +            if bootstrap_host is None:
> > +                parse_new_state_server_from_error(e)
>
> I think you want "bootstrap_host = " here.
>
> > +            else:
> > +                dump_env_logs(client, bootstrap_host, args.logs)
> > +        except Exception as e:
> > +            print_now('exception while dumping logs:\n')
> > +            logging.exception(e)
> > +        exit(1)
> > +    finally:
> > +        if args.clean_environment:
> > +            clean_environment(client, services_only=True)
> > +        else:
> > +            client.destroy_environment()
> > +
> > +
> > +if __name__ == '__main__':
> > +    main()
> >
> > === modified file 'test_container_networking.py'
> > --- test_container_networking.py      2015-09-21 16:08:26 +0000
> > +++ test_container_networking.py      2015-09-21 16:08:26 +0000
> > @@ -155,6 +157,71 @@
> >              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']
> > +
> > +        # Kill stderr so the test doesn't print anything (calling the
> tests
> > +        # with buffer=True would also do this, but I am trying not to be
> > +        # invasive)
> > +        sys.stderr = StringIO()
> > +        self.assertRaises(SystemExit, jcnet.parse_args, cmdline + opts)
> > +
> > +        # Restore stderr
> > +        sys.stderr = sys.__stderr__
>
> I suggest using "with test_utility.parse_error(self):" instead.
>
> > +
> > +    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.assertRaises(ValueError,
> > +                          jcnet.find_network, self.client, machine,
> addr)
>
> ValueError is very generic.  Please consider using assertRaisesRegexp, so
> that you ensure it's the correct ValueError.
>
> > +
> > +        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')
>
>
> --
>
> 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 updated on 2015-11-11

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

Revision history for this message

Aaron Bentley (abentley) wrote on 2015-11-13:

#

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 updated on 2015-11-16

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.

Revision history for this message

Aaron Bentley (abentley) wrote on 2015-11-23:

#

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 updated on 2015-11-26

1106. By James Tunnicliffe on 2015-11-26: Fixed subnets/<ver>/?op=reserved_ip_ranges when no IP addresses had been explicitly added.

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2015-11-26:

#

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 updated on 2015-11-26

1107. By James Tunnicliffe on 2015-11-26: Doh!
1108. By James Tunnicliffe on 2015-11-26: Fixed up reboot ordering and waits.

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2015-11-26:

#

That should do it. Please review.

review: Needs Resubmitting

Revision history for this message

Aaron Bentley (abentley) wrote on 2015-11-27:

#

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 updated on 2015-11-27

1109. By James Tunnicliffe on 2015-11-27: Fixed import spacing

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2015-11-27:

#

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.

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2015-12-01:

#

Fix pushed.

Revision history for this message

Aaron Bentley (abentley) wrote on 2015-12-08:

#

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

juju-ci-tools

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

Commit message

Description of the change

Preview Diff

Subscribers

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