Merge lp:~natefinch/juju-ci-tools/logrot into lp:juju-ci-tools

[Nate Finch]

Implements action do and fetch in jujupy, and uses them in tests for log rotation. Requires new charm in the repository, which will be proposed shortly.

[Aaron Bentley (abentley)]

Environment is deprecated. It should not be used for new scripts, and it should not be extended.

If you're going to remove '-e', you need to specify the environment somehow. Since "action fetch" and "action do" both support -e, I don't think you actually want to remove it. You just want to make the command handling smarter, so that it puts the -e after "do" or "fetch".

Please allow the juju under test to be specified at the commandline.

Please use temp_bootstrap_env(). At minimum, that will ensure that 'test-mode' is always True, but it will also allow us to support configuration variations such as changing the tools-url.

It would be nice if the test was self-contained. It is important that unit-fill-logs-0 and fill-machine be actions, or could they be run via "juju run" or "juju ssh"? That would allow the test script to use a generic charm instead.

Please add unit tests for log_rotation.py.

Please rename it to something that indicates it is a test. I suggest "assess_log_rotation". ("test" itself indicates that it is a unit test).

Juju 1.22 is still under development. Please ensure that these actions work with 1.22 by specifying the appropriate feature flag when the version is 1.22. See EnvJujuClient24 for an example.

review: Needs Fixing

[Nate Finch (natefinch)]

The test intentionally uses a charm, because it represents a real world situation where the charm is the thing outputting a lot of logging. I don't know that I could reasonably create a juju run command that would do the same thing. This seemed like the most easy to understand way to get the job done. I could use juju shh to get the size of the logs, but since I already had the charm and actions, it seemed easy enough to just make that another action.

[Aaron Bentley (abentley)]

> The test intentionally uses a charm, because it represents a real world
> situation where the charm is the thing outputting a lot of logging. I don't
> know that I could reasonably create a juju run command that would do the same
> thing.

You can log in a hook context and juju run has a hook context, though, right?

I'll accept a version that is not self-contained, I just wondered whether it was possible.

[Nate Finch (natefinch)]

I think I addressed all your concerns. I ended up refactoring some of the helper functions in assess_recovery, since they were needed for my test as well.

[Aaron Bentley (abentley)]

Thanks.

Please don't add things to deploy_stack unless they are used by deploy_stack (i.e. deploy_job) itself. (The history is that deploy_stack was the first script, so some of its functionality has been reused in other scripts.)

Please don't add anything at all to jujuconfig. That is essentially an import from https://launchpad.net/juju-tools, and it would be best if they don't diverge.

I see that you have not addressed the inline comments about KeyError and tuple formattting. Please go back and make sure you've addressed all the inline comments.

[Nate Finch (natefinch)]

Sorry about missing your inline comments... I haven't done a review on launchpad in about a year, so forgot that I had to scroll down to look for more comments.

I have a patch coming that will address all these issues.

[lp:~natefinch/juju-ci-tools/logrot]

961. By Nate Finch on 2015-05-23

more review changes

[Aaron Bentley (abentley)]

I think that you haven't run "make lint", because I see some lines longer than 79 characters. Please run "make lint" and fix any issues you find.

Please document all functions in assess_log_rotation.py

Please test parse_args.

See also inline comments.

review: Needs Fixing

[Aaron Bentley (abentley)]

Also, I think you missed this:
Juju 1.22 is still under development. Please ensure that these actions work with 1.22 by specifying the appropriate feature flag when the version is 1.22. See EnvJujuClient24 for an example.

[lp:~natefinch/juju-ci-tools/logrot]

962. By Nate Finch on 2015-06-01

more code review changes

963. By Nate Finch on 2015-06-02

merge

964. By Nate Finch on 2015-06-02

set longer timeout for fill log actions, and fix -e testing problem

[Nate Finch (natefinch)]

Ok, for real now, I think I addressed everything.

[lp:~natefinch/juju-ci-tools/logrot]

965. By Nate Finch on 2015-06-02

remove unneeded wait

[Aaron Bentley (abentley)]

This is almost landable, but there are still a few issues.

review: Needs Fixing

[Aaron Bentley (abentley)]

Also, TestEnvJujuClient22 needs client_class set to EnvJujuClient22.

[lp:~natefinch/juju-ci-tools/logrot]

966. By Nate Finch on 2015-06-03

more code review changes

[Nate Finch (natefinch)]

Ok, updated once again. Addressed your concerns.

[lp:~natefinch/juju-ci-tools/logrot]

967. By Nate Finch on 2015-06-03

one more fix to TestEnvJujuClient22

[Aaron Bentley (abentley)]

Thank you for you changes. This looks landable.

review: Approve

=== added file 'assess_log_rotation.py'
--- assess_log_rotation.py	1970-01-01 00:00:00 +0000
+++ assess_log_rotation.py	2015-06-03 15:41:41 +0000
@@ -0,0 +1,228 @@
+#!/usr/bin/env python
+from __future__ import print_function
+
+__metaclass__ = type
+
+from argparse import ArgumentParser
+from datetime import datetime
+import logging
+import re
+
+from deploy_stack import (
+    dump_env_logs,
+    get_machine_dns_name,
+)
+from jujuconfig import (
+    get_juju_home,
+)
+from jujupy import (
+    make_client,
+    parse_new_state_server_from_error,
+    temp_bootstrap_env,
+    yaml_loads,
+)
+from utility import (
+    print_now,
+)
+
+
+class LogRotateError(Exception):
+
+    ''' LogRotate test Exception base class. '''
+
+    def __init__(self, message):
+        super(LogRotateError, self).__init__(message)
+
+
+def test_unit_rotation(client):
+    """Tests unit log rotation."""
+    test_rotation(client,
+                  "/var/log/juju/unit-fill-logs-0.log",
+                  "unit-fill-logs-0",
+                  "fill-unit",
+                  "unit-size",
+                  "megs=300")
+
+
+def test_machine_rotation(client):
+    """Tests machine log rotation."""
+    test_rotation(client,
+                  "/var/log/juju/machine-1.log",
+                  "machine-1",
+                  "fill-machine",
+                  "machine-size", "megs=300", "machine=1")
+
+
+def test_rotation(client, logfile, prefix, fill_action, size_action, *args):
+    """A reusable help for testing log rotation.log
+
+    Deploys the fill-logs charm and uses it to fill the machine or unit log and
+    test that the logs roll over correctly.
+    """
+
+    # the rotation point should be 300 megs, so let's make sure we hit that.hit
+    # we'll obviously already have some data in the logs, so adding exactly
+    # 300megs should do the trick.
+
+    # we run do_fetch here so that we wait for fill-logs to finish.
+    client.action_do_fetch("fill-logs/0", fill_action, "3m", *args)
+    out = client.action_do_fetch("fill-logs/0", size_action)
+    action_output = yaml_loads(out)
+
+    # Now we should have one primary log file, and one backup log file.
+    # The backup should be approximately 300 megs.
+    # The primary should be below 300.
+
+    check_log0(logfile, action_output)
+    check_expected_backup("log1", prefix, action_output)
+
+    # we should only have one backup, not two.
+    check_for_extra_backup("log2", action_output)
+
+    # do it all again, this should generate a second backup.
+
+    client.action_do_fetch("fill-logs/0", fill_action, "3m", *args)
+    out = client.action_do_fetch("fill-logs/0", size_action)
+    action_output = yaml_loads(out)
+
+    # we should have two backups.
+    check_log0(logfile, action_output)
+    check_expected_backup("log1", prefix, action_output)
+    check_expected_backup("log2", prefix, action_output)
+
+    check_for_extra_backup("log3", action_output)
+
+    # one more time... we should still only have 2 backups and primary
+
+    client.action_do_fetch("fill-logs/0", fill_action, "3m", *args)
+    out = client.action_do_fetch("fill-logs/0", size_action)
+    action_output = yaml_loads(out)
+
+    check_log0(logfile, action_output)
+    check_expected_backup("log1", prefix, action_output)
+    check_expected_backup("log2", prefix, action_output)
+
+    # we should have two backups.
+    check_for_extra_backup("log3", action_output)
+
+
+def check_for_extra_backup(logname, action_output):
+    """Check that there are no extra backup files left behind."""
+    log = action_output["results"]["result-map"].get(logname)
+    if log is None:
+        # this is correct
+        return
+    # log exists.
+    name = log.get("name")
+    if name is None:
+        name = "(no name)"
+    raise LogRotateError("Extra backup log after rotation: " + name)
+
+
+def check_expected_backup(key, logprefix, action_output):
+    """Check that there the expected backup files exists and is close to 300MB.
+    """
+    log = action_output["results"]["result-map"].get(key)
+    if log is None:
+        raise LogRotateError(
+            "Missing backup log '{}' after rotation.".format(key))
+
+    backup_pattern = "/var/log/juju/%s-(.+?)\.log" % logprefix
+
+    log_name = log["name"]
+    matches = re.match(backup_pattern, log_name)
+    if matches is None:
+        raise LogRotateError(
+            "Rotated log '%s' does not match pattern '%s'." %
+            (log_name, backup_pattern))
+
+    size = int(log["size"])
+    if size < 299 or size > 301:
+        raise LogRotateError(
+            "Backup log '%s' should be ~300MB, but is %sMB." %
+            (log_name, size))
+
+    dt = matches.groups()[0]
+    dt_pattern = "%Y-%m-%dT%H-%M-%S.%f"
+
+    try:
+        # note - we have to use datetime's strptime because time's doesn't
+        # support partial seconds.
+        dt = datetime.strptime(dt, dt_pattern)
+    except Exception:
+        raise LogRotateError(
+            "Log for %s has invalid datetime appended: %s" % (log_name, dt))
+
+
+def check_log0(expected, action_output):
+    """Check that log0 exists and is not over 299MB"""
+    log = action_output["results"]["result-map"].get("log0")
+    if log is None:
+        raise LogRotateError("No log returned from size action.")
+
+    name = log["name"]
+    if name != expected:
+        raise LogRotateError(
+            "Wrong unit name: Expected: %s, actual: %s" % (expected, name))
+
+    size = int(log["size"])
+    if size > 299:
+        raise LogRotateError(
+            "Log0 too big. Expected < 300MB, got: %sMB" % size)
+
+
+def parse_args(argv=None):
+    """Parse all arguments."""
+    parser = ArgumentParser('Test log rotation.')
+    parser.add_argument(
+        '--debug', action='store_true', default=False,
+        help='Use --debug juju logging.')
+    parser.add_argument(
+        'agent',
+        help='Which agent log rotation to test.',
+        choices=['machine', 'unit'])
+    parser.add_argument(
+        'juju_path', help='Directory your juju binary lives in.')
+    parser.add_argument(
+        'env_name', help='Juju environment name to run tests in.')
+    parser.add_argument('logs', help='Directory to store logs in.')
+    parser.add_argument(
+        'temp_env_name', nargs='?',
+        help='Temporary environment name to use for this test.')
+    return parser.parse_args(argv)
+
+
+def main():
+    args = parse_args()
+    log_dir = args.logs
+
+    client = make_client(
+        args.juju_path, args.debug, args.env_name, args.temp_env_name)
+    client.destroy_environment()
+    juju_home = get_juju_home()
+    try:
+        with temp_bootstrap_env(juju_home, client):
+            client.bootstrap()
+        bootstrap_host = get_machine_dns_name(client, 0)
+        client.get_status(60)
+        client.juju("deploy", ('local:trusty/fill-logs',))
+
+        if args.agent == "unit":
+            test_unit_rotation(client)
+        if args.agent == "machine":
+            test_machine_rotation(client)
+    except Exception as e:
+        logging.exception(e)
+        try:
+            if bootstrap_host is None:
+                bootstrap_host = parse_new_state_server_from_error(e)
+            dump_env_logs(client, bootstrap_host, log_dir)
+        except Exception as e:
+            print_now("exception while dumping logs:\n")
+            logging.exception(e)
+    finally:
+        client.destroy_environment()
+
+
+if __name__ == '__main__':
+    main()
 
=== modified file 'assess_recovery.py'
--- assess_recovery.py	2015-02-11 17:42:41 +0000
+++ assess_recovery.py	2015-06-03 15:41:41 +0000
@@ -20,16 +20,16 @@
 from jujuconfig import (
     get_jenv_path,
     get_juju_home,
-    )
+)
 from jujupy import (
-    EnvJujuClient,
-    SimpleEnvironment,
     temp_bootstrap_env,
     until_timeout,
+    make_client,
+    parse_new_state_server_from_error,
 )
 from substrate import (
     terminate_instances,
-    )
+)
 from utility import (
     ensure_deleted,
     print_now,
@@ -133,14 +133,6 @@
     print_now("PASS")
 
 
-def parse_new_state_server_from_error(error):
-    output = str(error) + getattr(error, 'output', '')
-    matches = re.findall(r'Attempting to connect to (.*):22', output)
-    if matches:
-        return matches[-1]
-    return None
-
-
 def parse_args(argv=None):
     parser = ArgumentParser('Test recovery strategies.')
     parser.add_argument(
@@ -167,15 +159,6 @@
     return parser.parse_args(argv)
 
 
-def make_client(juju_path, debug, env_name, temp_env_name):
-    env = SimpleEnvironment.from_config(env_name)
-    if temp_env_name is not None:
-        env.environment = temp_env_name
-        env.config['name'] = temp_env_name
-    full_path = os.path.join(juju_path, 'juju')
-    return EnvJujuClient.by_version(env, full_path, debug)
-
-
 def main():
     args = parse_args()
     log_dir = args.logs
 
=== modified file 'check_blockers.py'
--- check_blockers.py	2015-05-15 19:45:42 +0000
+++ check_blockers.py	2015-06-03 15:41:41 +0000
@@ -51,12 +51,10 @@
     subparsers = parser.add_subparsers(help='sub-command help', dest="command")
     check_parser = subparsers.add_parser(
         'check', help='Check if merges are blocked for a branch.')
-    check_parser.add_argument(
-        'branch', default='master', nargs='?',
-        help='The branch to merge into.')
-    check_parser.add_argument(
-        'pull_request', default=None, nargs='?',
-        help='The pull request to be merged')
+    check_parser.add_argument('branch', default='master', nargs='?',
+                              help='The branch to merge into.')
+    check_parser.add_argument('pull_request', default=None, nargs='?',
+                              help='The pull request to be merged')
     update_parser = subparsers.add_parser(
         'update', help='Update blocking for a branch that passed CI.')
     update_parser.add_argument(
 
=== modified file 'jujupy.py'
--- jujupy.py	2015-06-01 21:06:33 +0000
+++ jujupy.py	2015-06-03 15:41:41 +0000
@@ -6,7 +6,7 @@
 from contextlib import (
     contextmanager,
     nested,
-    )
+)
 from cStringIO import StringIO
 from datetime import timedelta
 import errno
@@ -25,7 +25,7 @@
     get_jenv_path,
     get_juju_home,
     get_selected_environment,
-    )
+)
 from utility import (
     check_free_disk_space,
     ensure_deleted,
@@ -34,7 +34,7 @@
     scoped_environ,
     temp_dir,
     until_timeout,
-    )
+)
 
 
 WIN_JUJU_CMD = os.path.join('\\', 'Progra~2', 'Juju', 'juju.exe')
@@ -42,6 +42,14 @@
 JUJU_DEV_FEATURE_FLAGS = 'JUJU_DEV_FEATURE_FLAGS'
 
 
+def parse_new_state_server_from_error(error):
+    output = str(error) + getattr(error, 'output', '')
+    matches = re.findall(r'Attempting to connect to (.*):22', output)
+    if matches:
+        return matches[-1]
+    return None
+
+
 class ErroredUnit(Exception):
 
     def __init__(self, unit_name, state):
@@ -55,6 +63,15 @@
     return yaml.safe_load(StringIO(yaml_str))
 
 
+def make_client(juju_path, debug, env_name, temp_env_name):
+    env = SimpleEnvironment.from_config(env_name)
+    if temp_env_name is not None:
+        env.environment = temp_env_name
+        env.config['name'] = temp_env_name
+    full_path = os.path.join(juju_path, 'juju')
+    return EnvJujuClient.by_version(env, full_path, debug)
+
+
 class CannotConnectEnv(subprocess.CalledProcessError):
 
     def __init__(self, e):
@@ -156,6 +173,8 @@
             full_path = os.path.abspath(juju_path)
         if version.startswith('1.16'):
             raise Exception('Unsupported juju: %s' % version)
+        elif re.match('^1\.22[.-]', version):
+            return EnvJujuClient22(env, version, full_path, debug=debug)
         elif re.match('^1\.24[.-]', version):
             return EnvJujuClient24(env, version, full_path, debug=debug)
         elif re.match('^1\.25[.-]', version):
@@ -174,6 +193,10 @@
         else:
             prefix = ('timeout', '%.2fs' % timeout)
         logging = '--debug' if self.debug else '--show-log'
+
+        # we split the command here so that the caller can control where the -e
+        # <env> flag goes.  Everything in the command string is put before the
+        # -e flag.
         command = command.split()
         return prefix + ('juju', logging,) + tuple(command) + e_arg + args
 
@@ -197,6 +220,7 @@
                                          env['PATH'])
         if juju_home is not None:
             env['JUJU_HOME'] = juju_home
+
         return env
 
     def get_bootstrap_args(self, upload_tools):
@@ -243,8 +267,15 @@
             ensure_deleted(jenv_path)
 
     def get_juju_output(self, command, *args, **kwargs):
+        """Call a juju command and return the output.
+
+        Sub process will be called as 'juju <command> <args> <kwargs>'. Note
+        that <command> may be a space delimited list of arguments. The -e
+        <environment> flag will be placed after <command> and before args.     
+        """
         args = self._full_args(command, False, args,
-                               timeout=kwargs.get('timeout'))
+                               timeout=kwargs.get('timeout'),
+                               include_e=kwargs.get('include_e', True))
         env = self._shell_environ()
         with tempfile.TemporaryFile() as stderr:
             try:
@@ -478,6 +509,67 @@
         print_now("State-Server backup at %s" % backup_file_path)
         return backup_file_path
 
+    def action_fetch(self, id, action=None, timeout="1m"):
+        """Fetches the results of the action with the given id.
+
+        Will wait for up to 1 minute for the action results.
+        The action name here is just used for an more informational error in
+        cases where it's available.
+        Returns the yaml output of the fetched action.
+        """
+        # the command has to be "action fetch" so that the -e <env> args are
+        # placed after "fetch", since that's where action requires them to be.
+        out = self.get_juju_output("action fetch", id, "--wait", timeout)
+        status = yaml_loads(out)["status"]
+        if status != "completed":
+            name = ""
+            if action is not None:
+                name = " " + action
+            raise Exception(
+                "timed out waiting for action%s to complete during fetch" %
+                name)
+        return out
+
+    def action_do(self, unit, action, *args):
+        """Performs the given action on the given unit.
+
+        Action params should be given as args in the form foo=bar.
+        Returns the id of the queued action.
+        """
+        args = (unit, action) + args
+
+        # the command has to be "action do" so that the -e <env> args are
+        # placed after "do", since that's where action requires them to be.
+        output = self.get_juju_output("action do", *args)
+        action_id_pattern = re.compile(
+            'Action queued with id: ([a-f0-9\-]{36})')
+        match = action_id_pattern.search(output)
+        if match is None:
+            raise Exception("Action id not found in output: %s" %
+                            output)
+        return match.group(1)
+
+    def action_do_fetch(self, unit, action, timeout="1m", *args):
+        """Performs the given action on the given unit and waits for the results.
+
+        Action params should be given as args in the form foo=bar.
+        Returns the yaml output of the action.
+        """
+        id = self.action_do(unit, action, *args)
+        return self.action_fetch(id, action, timeout)
+
+
+class EnvJujuClient22(EnvJujuClient):
+
+    def _shell_environ(self, juju_home=None):
+        """Generate a suitable shell environment.
+
+        Juju's directory must be in the PATH to support plugins.
+        """
+        env = super(EnvJujuClient22, self)._shell_environ(juju_home)
+        env[JUJU_DEV_FEATURE_FLAGS] = 'actions'
+        return env
+
 
 class EnvJujuClient25(EnvJujuClient):
 
@@ -493,7 +585,8 @@
 
 
 class EnvJujuClient24(EnvJujuClient25):
-    """Currently, same feature set as juju 2.5"""
+
+    """Currently, same feature set as juju 25"""
 
 
 def get_local_root(juju_home, env):
 
=== added file 'test_assess_log_rotation.py'
--- test_assess_log_rotation.py	1970-01-01 00:00:00 +0000
+++ test_assess_log_rotation.py	2015-06-03 15:41:41 +0000
@@ -0,0 +1,154 @@
+from argparse import Namespace
+from unittest import TestCase
+from assess_log_rotation import (
+    check_for_extra_backup,
+    check_expected_backup,
+    check_log0,
+    LogRotateError,
+    parse_args,
+)
+from jujupy import yaml_loads
+
+good_yaml = \
+    """
+results:
+  result-map:
+    log0:
+      name: /var/log/juju/unit-fill-logs-0.log
+      size: "25"
+    log1:
+      name: /var/log/juju/unit-fill-logs-0-2015-05-21T09-57-03.123.log
+      size: "299"
+    log1:
+      name: /var/log/juju/unit-fill-logs-0-2015-05-22T12-57-03.123.log
+      size: "300"
+status: completed
+timing:
+  completed: 2015-05-21 09:57:03 -0400 EDT
+  enqueued: 2015-05-21 09:56:59 -0400 EDT
+  started: 2015-05-21 09:57:02 -0400 EDT
+"""
+
+good_obj = yaml_loads(good_yaml)
+
+big_yaml = \
+    """
+results:
+  result-map:
+    log0:
+      name: /var/log/juju/unit-fill-logs-0.log
+      size: "400"
+    log1:
+      name: /var/log/juju/unit-fill-logs-0-2015-05-21T09-57-03.123.log
+      size: "400"
+    log2:
+      name: /var/log/juju/unit-fill-logs-0-not-a-valid-timestamp.log
+      size: "299"
+    log3:
+      name: something-just-plain-bad.log
+      size: "299"
+status: completed
+timing:
+  completed: 2015-05-21 09:57:03 -0400 EDT
+  enqueued: 2015-05-21 09:56:59 -0400 EDT
+  started: 2015-05-21 09:57:02 -0400 EDT
+"""
+
+big_obj = yaml_loads(big_yaml)
+
+no_files_yaml = \
+    """
+results:
+  result-map:
+status: completed
+timing:
+  completed: 2015-05-21 09:57:03 -0400 EDT
+  enqueued: 2015-05-21 09:56:59 -0400 EDT
+  started: 2015-05-21 09:57:02 -0400 EDT
+"""
+
+no_files_obj = yaml_loads(no_files_yaml)
+
+
+class TestCheckForExtraBackup(TestCase):
+
+    def test_not_found(self):
+        try:
+            # log2 should not be found, and thus no exception.
+            check_for_extra_backup("log2", good_obj)
+        except Exception as e:
+            self.fail("unexpected exception: %s" % e.msg)
+
+    def test_find_extra(self):
+        with self.assertRaises(LogRotateError):
+            # log1 should be found, and thus cause an exception.
+            check_for_extra_backup("log1", good_obj)
+
+
+class TestCheckBackup(TestCase):
+
+    def test_exists(self):
+        try:
+            # log1 should be found, and thus no exception.
+            check_expected_backup("log1", "unit-fill-logs-0", good_obj)
+        except Exception as e:
+            self.fail("unexpected exception: %s" % e.msg)
+
+    def test_not_found(self):
+        with self.assertRaises(LogRotateError):
+            # log2 should not be found, and thus cause an exception.
+            check_expected_backup("log2", "unit-fill-logs-0", good_obj)
+
+    def test_too_big(self):
+        with self.assertRaises(LogRotateError):
+            # log1 is too big, and thus should cause an exception.
+            check_expected_backup("log1", "unit-fill-logs-0", big_obj)
+
+    def test_bad_timestamp(self):
+        with self.assertRaises(LogRotateError):
+            # log2 has an invalid timestamp, and thus should cause an
+            # exception.
+            check_expected_backup("log2", "unit-fill-logs-0", big_obj)
+
+    def test_bad_name(self):
+        with self.assertRaises(LogRotateError):
+            # log3 has a completely invalid name, and thus should cause an
+            # exception.
+            check_expected_backup("log3", "unit-fill-logs-0", big_obj)
+
+
+class TestCheckLog0(TestCase):
+
+    def test_exists(self):
+        try:
+            # log0 should be found, and thus no exception.
+            check_log0("/var/log/juju/unit-fill-logs-0.log", good_obj)
+        except Exception as e:
+            self.fail("unexpected exception: %s" % e.msg)
+
+    def test_not_found(self):
+        with self.assertRaises(AttributeError):
+            # There's no value under result-map, which causes the yaml parser
+            # to consider it None, and thus it'll cause an AttributeError
+            check_log0("/var/log/juju/unit-fill-logs-0.log", no_files_obj)
+
+    def test_too_big(self):
+        with self.assertRaises(LogRotateError):
+            # log0 is too big, and thus should cause an exception.
+            check_log0(
+                "/var/log/juju/unit-fill-logs-0.log", big_obj)
+
+
+class TestParseArgs(TestCase):
+
+    def test_parse_args(self):
+        args = parse_args(['machine', 'b', 'c', 'd', 'e'])
+        self.assertEqual(args, Namespace(
+            agent='machine', juju_path='b', env_name='c', logs='d',
+            temp_env_name='e', debug=False))
+
+    def test_parse_args_debug(self):
+        args = parse_args(['--debug', 'unit', 'b', 'c', 'd', 'e'])
+        self.assertEqual(args, Namespace(
+            agent='unit', juju_path='b', env_name='c', logs='d',
+            temp_env_name='e', debug=True))
 
=== modified file 'test_assess_recovery.py'
--- test_assess_recovery.py	2015-02-11 17:49:42 +0000
+++ test_assess_recovery.py	2015-06-03 15:41:41 +0000
@@ -1,68 +1,8 @@
-from contextlib import contextmanager
-import os
-from subprocess import CalledProcessError
-from textwrap import dedent
 from unittest import TestCase
 
-from mock import patch
-
 from assess_recovery import (
-    make_client,
-    parse_new_state_server_from_error,
     parse_args,
 )
-from jujupy import _temp_env as temp_env
-from utility import temp_dir
-
-
-class AssessRecoveryTestCase(TestCase):
-
-    def test_parse_new_state_server_from_error(self):
-        output = dedent("""
-            Waiting for address
-            Attempting to connect to 10.0.0.202:22
-            Attempting to connect to 1.2.3.4:22
-            The fingerprint for the ECDSA key sent by the remote host is
-            """)
-        error = CalledProcessError(1, ['foo'], output)
-        address = parse_new_state_server_from_error(error)
-        self.assertEqual('1.2.3.4', address)
-
-
-class TestMakeClient(TestCase):
-
-    @contextmanager
-    def make_client_cxt(self):
-        td = temp_dir()
-        te = temp_env({'environments': {'foo': {
-            'orig-name': 'foo', 'name': 'foo'}}})
-        with td as juju_path, te, patch('subprocess.Popen',
-                                        side_effect=ValueError):
-            with patch('subprocess.check_output') as co_mock:
-                co_mock.return_value = '1.18'
-                yield juju_path
-
-    def test_make_client(self):
-        with self.make_client_cxt() as juju_path:
-            client = make_client(juju_path, False, 'foo', 'bar')
-        self.assertEqual(client.full_path, os.path.join(juju_path, 'juju'))
-        self.assertEqual(client.debug, False)
-        self.assertEqual(client.env.config['orig-name'], 'foo')
-        self.assertEqual(client.env.config['name'], 'bar')
-        self.assertEqual(client.env.environment, 'bar')
-
-    def test_make_client_debug(self):
-        with self.make_client_cxt() as juju_path:
-            client = make_client(juju_path, True, 'foo', 'bar')
-        self.assertEqual(client.debug, True)
-
-    def test_make_client_no_temp_env_name(self):
-        with self.make_client_cxt() as juju_path:
-            client = make_client(juju_path, False, 'foo', None)
-        self.assertEqual(client.full_path, os.path.join(juju_path, 'juju'))
-        self.assertEqual(client.env.config['orig-name'], 'foo')
-        self.assertEqual(client.env.config['name'], 'foo')
-        self.assertEqual(client.env.environment, 'foo')
 
 
 class TestParseArgs(TestCase):
 
=== modified file 'test_deploy_stack.py'
--- test_deploy_stack.py	2015-05-28 22:29:53 +0000
+++ test_deploy_stack.py	2015-06-03 15:41:41 +0000
@@ -1,7 +1,7 @@
 from argparse import (
     ArgumentParser,
     Namespace,
-    )
+)
 from contextlib import contextmanager
 import json
 import logging
@@ -14,7 +14,7 @@
 from mock import (
     call,
     patch,
-    )
+)
 import yaml
 
 from deploy_stack import (
@@ -50,7 +50,7 @@
 )
 from test_jujupy import (
     assert_juju_call,
-    )
+)
 from utility import temp_dir
 
 
@@ -274,7 +274,7 @@
                        side_effect=subprocess.CalledProcessError('', '')):
                 with patch('subprocess.call', autospec=True) as c_mock:
                     with self.assertRaises(subprocess.CalledProcessError):
-                            run_instances(1, 'qux')
+                        run_instances(1, 'qux')
         c_mock.assert_called_with(['euca-terminate-instances', 'i-foo'])
 
     def test_assess_juju_run(self):
@@ -524,7 +524,7 @@
                 ('juju', '--show-log', 'status', '-e', 'foo'): status,
                 ('juju', '--show-log', 'ssh', '-e', 'foo', 'dummy-sink/0',
                  GET_TOKEN_SCRIPT): 'fake-token',
-                }
+            }
             return output[args]
 
         with patch('subprocess.check_output', side_effect=output,
@@ -582,7 +582,7 @@
             cls.RUN_UNAME: juju_run_out,
             cls.VERSION: '1.38',
             cls.GET_ENV: 'testing'
-            }
+        }
         return output[args]
 
     @contextmanager
@@ -590,11 +590,11 @@
         with patch('subprocess.check_output', side_effect=self.upgrade_output,
                    autospec=True) as co_mock:
             with patch('subprocess.check_call', autospec=True) as cc_mock:
-                    with patch('deploy_stack.check_token', autospec=True):
-                        with patch('deploy_stack.get_random_string',
-                                   return_value="FAKETOKEN", autospec=True):
-                            with patch('sys.stdout', autospec=True):
-                                yield (co_mock, cc_mock)
+                with patch('deploy_stack.check_token', autospec=True):
+                    with patch('deploy_stack.get_random_string',
+                               return_value="FAKETOKEN", autospec=True):
+                        with patch('sys.stdout', autospec=True):
+                            yield (co_mock, cc_mock)
 
     def test_assess_upgrade(self):
         env = SimpleEnvironment('foo', {'type': 'foo'})
@@ -643,7 +643,7 @@
     status = yaml.dump({
         'machines': {0: {'agent-version': '1.18.17'}},
         'services': {},
-        })
+    })
 
     def test_prepare_environment(self):
         client = self.get_client()
@@ -699,7 +699,7 @@
         client = EnvJujuClient(SimpleEnvironment(
             'foo', {'type': 'paas'}), '1.23', 'path')
         self.addContext(patch('deploy_stack.get_machine_dns_name',
-                        return_value='foo'))
+                              return_value='foo'))
         c_mock = self.addContext(patch('subprocess.call'))
         with boot_context('bar', client, None, [], None, None, None, None,
                           keep_env=False, upload_tools=False):
@@ -718,7 +718,7 @@
         client = EnvJujuClient(SimpleEnvironment(
             'foo', {'type': 'paas'}), '1.23', 'path')
         self.addContext(patch('deploy_stack.get_machine_dns_name',
-                        return_value='foo'))
+                              return_value='foo'))
         c_mock = self.addContext(patch('subprocess.call'))
         with boot_context('bar', client, None, [], None, None, None, None,
                           keep_env=True, upload_tools=False):
@@ -735,7 +735,7 @@
         client = EnvJujuClient(SimpleEnvironment(
             'foo', {'type': 'paas'}), '1.23', 'path')
         self.addContext(patch('deploy_stack.get_machine_dns_name',
-                        return_value='foo'))
+                              return_value='foo'))
         self.addContext(patch('subprocess.call'))
         with boot_context('bar', client, None, [], None, None, None, None,
                           keep_env=False, upload_tools=True):
@@ -749,7 +749,7 @@
         client = EnvJujuClient(SimpleEnvironment(
             'foo', {'type': 'paas'}), '1.23', 'path')
         self.addContext(patch('deploy_stack.get_machine_dns_name',
-                        return_value='foo'))
+                              return_value='foo'))
         self.addContext(patch('subprocess.call'))
         ue_mock = self.addContext(
             patch('deploy_stack.update_env', wraps=update_env))
@@ -783,7 +783,7 @@
             upgrade=False,
             verbose=False,
             upload_tools=False,
-            ))
+        ))
 
     def test_upload_tools(self):
         args = deploy_job_parse_args(['foo', 'bar', 'baz', '--upload-tools'])
 
=== modified file 'test_jujupy.py'
--- test_jujupy.py	2015-06-01 21:06:33 +0000
+++ test_jujupy.py	2015-06-03 15:41:41 +0000
@@ -4,7 +4,7 @@
 from datetime import (
     datetime,
     timedelta,
-    )
+)
 import os
 import shutil
 import StringIO
@@ -24,11 +24,12 @@
     get_environments_path,
     get_jenv_path,
     NoSuchEnvironment,
-    )
+)
 from jujupy import (
     CannotConnectEnv,
     Environment,
     EnvJujuClient,
+    EnvJujuClient22,
     EnvJujuClient24,
     EnvJujuClient25,
     ErroredUnit,
@@ -41,11 +42,13 @@
     temp_bootstrap_env,
     _temp_env as temp_env,
     uniquify_local,
+    make_client,
+    parse_new_state_server_from_error,
 )
 from utility import (
     scoped_environ,
     temp_dir,
-    )
+)
 
 
 def assert_juju_call(test_case, mock_method, client, expected_args,
@@ -98,7 +101,8 @@
             SimpleEnvironment('baz', {'type': 'cloudsigma'}),
             '1.25-foobar', 'path')
         env = client._shell_environ()
-        self.assertEqual(env[JUJU_DEV_FEATURE_FLAGS], 'cloudsigma')
+        "".split()
+        self.assertTrue('cloudsigma' in env[JUJU_DEV_FEATURE_FLAGS].split(","))
 
     def test__shell_environ_juju_home(self):
         client = self.client_class(
@@ -107,6 +111,23 @@
         self.assertEqual(env['JUJU_HOME'], 'asdf')
 
 
+class TestEnvJujuClient22(ClientTest):
+
+    client_class = EnvJujuClient22
+
+    def test__shell_environ(self):
+        client = self.client_class(
+            SimpleEnvironment('baz', {'type': 'ec2'}), '1.22-foobar', 'path')
+        env = client._shell_environ()
+        self.assertEqual(env.get(JUJU_DEV_FEATURE_FLAGS), 'actions')
+
+    def test__shell_environ_juju_home(self):
+        client = self.client_class(
+            SimpleEnvironment('baz', {'type': 'ec2'}), '1.22-foobar', 'path')
+        env = client._shell_environ(juju_home='asdf')
+        self.assertEqual(env['JUJU_HOME'], 'asdf')
+
+
 class TestEnvJujuClient24(TestEnvJujuClient25):
 
     client_class = EnvJujuClient25
@@ -165,6 +186,7 @@
             yield '1.16'
             yield '1.16.1'
             yield '1.15'
+            yield '1.22.1'
             yield '1.24-alpha1'
             yield '1.24.7'
             yield '1.25.1'
@@ -185,6 +207,8 @@
             self.assertIs(EnvJujuClient, type(client))
             self.assertEqual('1.15', client.version)
             client = EnvJujuClient.by_version(None)
+            self.assertIs(type(client), EnvJujuClient22)
+            client = EnvJujuClient.by_version(None)
             self.assertIs(type(client), EnvJujuClient24)
             self.assertEqual(client.version, '1.24-alpha1')
             client = EnvJujuClient.by_version(None)
@@ -231,7 +255,7 @@
         client = EnvJujuClient(env, None, 'my/juju/bin')
         full = client._full_args('action bar', False, ('baz', 'qux'))
         self.assertEqual((
-            'juju', '--show-log', 'action', 'bar', '-e', 'foo', 'baz', 'qux'),
+            'juju', '--show-log', 'action', 'bar', '-e', 'foo', 'baz', 'qux', ),
             full)
 
     def test_bootstrap_hpcloud(self):
@@ -577,9 +601,9 @@
                 '0': {'state-server-member-status': 'has-vote'},
                 '1': {'state-server-member-status': 'has-vote'},
                 '2': {'state-server-member-status': 'has-vote'},
-                },
+            },
             'services': {},
-            })
+        })
         client = EnvJujuClient(SimpleEnvironment('local'), None, None)
         with patch.object(client, 'get_juju_output', return_value=value):
             client.wait_for_ha()
@@ -590,9 +614,9 @@
                 '0': {'state-server-member-status': 'no-vote'},
                 '1': {'state-server-member-status': 'no-vote'},
                 '2': {'state-server-member-status': 'no-vote'},
-                },
+            },
             'services': {},
-            })
+        })
         client = EnvJujuClient(SimpleEnvironment('local'), None, None)
         with patch('sys.stdout'):
             with patch.object(client, 'get_juju_output', return_value=value):
@@ -606,9 +630,9 @@
             'machines': {
                 '0': {'state-server-member-status': 'has-vote'},
                 '1': {'state-server-member-status': 'has-vote'},
-                },
+            },
             'services': {},
-            })
+        })
         client = EnvJujuClient(SimpleEnvironment('local'), None, None)
         with patch('jujupy.until_timeout', lambda x: range(0)):
             with patch.object(client, 'get_juju_output', return_value=value):
@@ -621,7 +645,7 @@
         value = yaml.safe_dump({
             'machines': {
                 '0': {'agent-state': 'started'},
-                },
+            },
             'services': {
                 'jenkins': {
                     'units': {
@@ -638,9 +662,9 @@
         value = yaml.safe_dump({
             'machines': {
                 '0': {'agent-state': 'started'},
-                },
+            },
             'services': {},
-            })
+        })
         client = EnvJujuClient(SimpleEnvironment('local'), None, None)
         with patch('jujupy.until_timeout', lambda x: range(0)):
             with patch.object(client, 'get_juju_output', return_value=value):
@@ -981,7 +1005,7 @@
                     'root-dir': get_local_root(fake_home, client.env),
                     'agent-version': agent_version,
                     'test-mode': True,
-                    }}})
+                }}})
                 stub_bootstrap()
 
     def test_temp_bootstrap_env_provides_dir(self):
@@ -1052,7 +1076,7 @@
         self.assertEqual(mock_cfds.mock_calls, [
             call(os.path.join(fake_home, 'qux'), 8000000, 'MongoDB files'),
             call('/var/lib/lxc', 2000000, 'LXC containers'),
-            ])
+        ])
 
     def test_check_space_local_kvm(self):
         env = SimpleEnvironment('qux', {'type': 'local', 'container': 'kvm'})
@@ -1064,7 +1088,7 @@
         self.assertEqual(mock_cfds.mock_calls, [
             call(os.path.join(fake_home, 'qux'), 8000000, 'MongoDB files'),
             call('/var/lib/uvtool/libvirt/images', 2000000, 'KVM disk files'),
-            ])
+        ])
 
     def test_error_on_jenv(self):
         env = SimpleEnvironment('qux', {'type': 'local'})
@@ -1337,7 +1361,7 @@
         self.assertEqual(list(status.iter_machines(containers=True)), [
             ('1', status.status['machines']['1']),
             ('1/lxc/0', {'baz': 'qux'}),
-            ])
+        ])
 
     def test_agent_items_empty(self):
         status = Status({'machines': {}, 'services': {}}, '')
@@ -1483,7 +1507,7 @@
             status.get_subordinate_units('dummy-sink'), [
                 {'chaos-monkey/1': {'agent-state': 'started'}},
                 {'chaos-monkey/2': {'agent-state': 'started'}}]
-            )
+        )
         with self.assertRaisesRegexp(KeyError, 'foo'):
             status.get_subordinate_units('foo')
 
@@ -1593,7 +1617,7 @@
             'machines': {'0': {
                 'agent-state-info': failure}},
             'services': {},
-            }, '')
+        }, '')
         with self.assertRaises(ErroredUnit) as e_cxt:
             status.check_agents_started()
         e = e_cxt.exception
@@ -1653,14 +1677,14 @@
         old_status = Status({
             'machines': {
                 'bar': 'bar_info',
-                }
-            }, '')
+            }
+        }, '')
         new_status = Status({
             'machines': {
                 'foo': 'foo_info',
                 'bar': 'bar_info',
-                }
-            }, '')
+            }
+        }, '')
         self.assertItemsEqual(new_status.iter_new_machines(old_status),
                               [('foo', 'foo_info')])
 
@@ -1669,8 +1693,8 @@
             'machines': {
                 '0': {'instance-id': 'foo-bar'},
                 '1': {},
-                }
-            }, '')
+            }
+        }, '')
         self.assertEqual(status.get_instance_id('0'), 'foo-bar')
         with self.assertRaises(KeyError):
             status.get_instance_id('1')
@@ -1965,6 +1989,62 @@
         mock_get.assert_called_with(env, 'tools-metadata-url')
         self.assertEqual(0, mock_set.call_count)
 
+    def test_action_do(self):
+        client = EnvJujuClient(SimpleEnvironment(None, {'type': 'local'}),
+                               '1.23-series-arch', None)
+        with patch.object(EnvJujuClient, 'get_juju_output') as mock:
+            mock.return_value = \
+                "Action queued with id: 5a92ec93-d4be-4399-82dc-7431dbfd08f9"
+            id = client.action_do("foo/0", "myaction", "param=5")
+            self.assertEqual(id, "5a92ec93-d4be-4399-82dc-7431dbfd08f9")
+        mock.assert_called_once_with(
+            'action do', 'foo/0', 'myaction', "param=5"
+        )
+
+    def test_action_do_error(self):
+        client = EnvJujuClient(SimpleEnvironment(None, {'type': 'local'}),
+                               '1.23-series-arch', None)
+        with patch.object(EnvJujuClient, 'get_juju_output') as mock:
+            mock.return_value = "some bad text"
+            with self.assertRaisesRegexp(Exception,
+                                         "Action id not found in output"):
+                client.action_do("foo/0", "myaction", "param=5")
+
+    def test_action_fetch(self):
+        client = EnvJujuClient(SimpleEnvironment(None, {'type': 'local'}),
+                               '1.23-series-arch', None)
+        with patch.object(EnvJujuClient, 'get_juju_output') as mock:
+            ret = "status: completed\nfoo: bar"
+            mock.return_value = ret
+            out = client.action_fetch("123")
+            self.assertEqual(out, ret)
+        mock.assert_called_once_with(
+            'action fetch', '123', "--wait", "1m"
+        )
+
+    def test_action_fetch_timeout(self):
+        client = EnvJujuClient(SimpleEnvironment(None, {'type': 'local'}),
+                               '1.23-series-arch', None)
+        ret = "status: pending\nfoo: bar"
+        with patch.object(EnvJujuClient,
+                          'get_juju_output', return_value=ret):
+            with self.assertRaisesRegexp(Exception,
+                                         "timed out waiting for action"):
+                client.action_fetch("123")
+
+    def test_action_do_fetch(self):
+        client = EnvJujuClient(SimpleEnvironment(None, {'type': 'local'}),
+                               '1.23-series-arch', None)
+        with patch.object(EnvJujuClient, 'get_juju_output') as mock:
+            ret = "status: completed\nfoo: bar"
+            # setting side_effect to an iterable will return the next value
+            # from the list each time the function is called.
+            mock.side_effect = [
+                "Action queued with id: 5a92ec93-d4be-4399-82dc-7431dbfd08f9",
+                ret]
+            out = client.action_do_fetch("foo/0", "myaction", "param=5")
+            self.assertEqual(out, ret)
+
 
 class TestGroupReporter(TestCase):
 
@@ -2099,3 +2179,53 @@
         ])
         reporter.finish()
         self.assertEqual(sio.getvalue(), changes[-1] + "\n")
+
+
+class TestMakeClient(TestCase):
+
+    @contextmanager
+    def make_client_cxt(self):
+        td = temp_dir()
+        te = temp_env({'environments': {'foo': {
+            'orig-name': 'foo', 'name': 'foo'}}})
+        with td as juju_path, te, patch('subprocess.Popen',
+                                        side_effect=ValueError):
+            with patch('subprocess.check_output') as co_mock:
+                co_mock.return_value = '1.18'
+                yield juju_path
+
+    def test_make_client(self):
+        with self.make_client_cxt() as juju_path:
+            client = make_client(juju_path, False, 'foo', 'bar')
+        self.assertEqual(client.full_path, os.path.join(juju_path, 'juju'))
+        self.assertEqual(client.debug, False)
+        self.assertEqual(client.env.config['orig-name'], 'foo')
+        self.assertEqual(client.env.config['name'], 'bar')
+        self.assertEqual(client.env.environment, 'bar')
+
+    def test_make_client_debug(self):
+        with self.make_client_cxt() as juju_path:
+            client = make_client(juju_path, True, 'foo', 'bar')
+        self.assertEqual(client.debug, True)
+
+    def test_make_client_no_temp_env_name(self):
+        with self.make_client_cxt() as juju_path:
+            client = make_client(juju_path, False, 'foo', None)
+        self.assertEqual(client.full_path, os.path.join(juju_path, 'juju'))
+        self.assertEqual(client.env.config['orig-name'], 'foo')
+        self.assertEqual(client.env.config['name'], 'foo')
+        self.assertEqual(client.env.environment, 'foo')
+
+
+class AssessParseStateServerFromErrorTestCase(TestCase):
+
+    def test_parse_new_state_server_from_error(self):
+        output = dedent("""
+            Waiting for address
+            Attempting to connect to 10.0.0.202:22
+            Attempting to connect to 1.2.3.4:22
+            The fingerprint for the ECDSA key sent by the remote host is
+            """)
+        error = subprocess.CalledProcessError(1, ['foo'], output)
+        address = parse_new_state_server_from_error(error)
+        self.assertEqual('1.2.3.4', address)