Merge lp:~veebers/juju-ci-tools/introduce-commandtime into lp:juju-ci-tools

[Christopher Lee]

Introduce CommandTime to ModelClient.juju().

This is the next step in getting CommandTime and CommandComplete introduced.
Included in this MP is an update to tests (due to the slightly different ModelClient.juju return behaviour) as well as consideration for any existing code that store the return from .juju() or any of the wrapper commands (e.g. deploy)

[Curtis Hovey (sinzui)]

Thank you. I don't like the name actual_completion() I have some ideas inline. I leave it to you to decide the name you like. I also have a suggestion for a a test.

review: Approve (code)

[Aaron Bentley (abentley)]

I think maybe Chris forgot to make lp:~veebers/juju-ci-tools/poc-juju-ci-timing a prerequisite.

=== modified file 'assess_container_networking.py'
--- assess_container_networking.py	2017-02-24 18:28:04 +0000
+++ assess_container_networking.py	2017-03-23 19:29:02 +0000
@@ -248,7 +248,7 @@
 
         d = re.search(r'^default\s+via\s+([\d\.]+)\s+', routes, re.MULTILINE)
         if d:
-            rc = client.juju('ssh', ('--proxy', target,
+            rc, _ = client.juju('ssh', ('--proxy', target,
                                      'ping -c1 -q ' + d.group(1)), check=False)
             if rc != 0:
                 raise ValueError('%s unable to ping default route' % target)

=== modified file 'chaos.py'
--- chaos.py	2016-06-27 11:41:04 +0000
+++ chaos.py	2017-03-23 19:29:02 +0000
@@ -146,8 +146,9 @@
         check_cmd += '/chaos_monkey.' + self.monkey_ids[unit_name]
         check_cmd += '/chaos_runner.lock'
         check_cmd += ' ]'
-        if self.client.juju('run', ('--unit', unit_name, check_cmd),
-                            check=False):
+        retvar, _ = self.client.juju(
+            'run', ('--unit', unit_name, check_cmd), check=False)
+        if retvar != 0:
             return 'done'
         return 'running'
 

=== modified file 'jujupy/client.py'
--- jujupy/client.py	2017-03-23 19:29:02 +0000
+++ jujupy/client.py	2017-03-23 19:29:02 +0000
@@ -1079,7 +1079,7 @@
         self.feature_flags = feature_flags
         self.debug = debug
         self._timeout_path = get_timeout_path()
-        self.juju_timings = {}
+        self.juju_timings = []
         self.soft_deadline = soft_deadline
         self._ignore_soft_deadline = False
 
@@ -1181,7 +1181,11 @@
     def juju(self, command, args, used_feature_flags,
              juju_home, model=None, check=True, timeout=None, extra_env=None,
              suppress_err=False):
-        """Run a command under juju for the current environment."""
+        """Run a command under juju for the current environment.
+
+        :return: Tuple rval, CommandTime rval being the commands exit code and
+          a CommandTime object used for storing command timing data.
+        """
         args = self.full_args(command, args, model, timeout)
         log.info(' '.join(args))
         env = self.shell_environ(used_feature_flags, juju_home)
@@ -1191,17 +1195,17 @@
             call_func = subprocess.check_call
         else:
             call_func = subprocess.call
-        start_time = time.time()
         # Mutate os.environ instead of supplying env parameter so Windows can
         # search env['PATH']
         stderr = subprocess.PIPE if suppress_err else None
+        # Keep track of commands and how long the take.
+        command_time = CommandTime(command, args, env)
         with scoped_environ(env):
             log.debug('Running juju with env: {}'.format(env))
             with self._check_timeouts():
                 rval = call_func(args, stderr=stderr)
-        self.juju_timings.setdefault(args, []).append(
-            (time.time() - start_time))
-        return rval
+        self.juju_timings.append(command_time)
+        return rval, command_time
 
     def expect(self, command, args, used_feature_flags, juju_home, model=None,
                timeout=None, extra_env=None):
@@ -1926,7 +1930,7 @@
             '--config', config_file))
 
     def destroy_model(self):
-        exit_status = self.juju(
+        exit_status, _ = self.juju(
             'destroy-model', (self.env.environment, '-y',),
             include_e=False, timeout=get_teardown_timeout(self))
         return exit_status
@@ -1960,7 +1964,7 @@
             self.destroy_controller(all_models=True)
         except subprocess.CalledProcessError:
             logging.warning('tear_down destroy-controller failed')
-            retval = self.kill_controller()
+            retval, _ = self.kill_controller()
             message = 'tear_down kill-controller result={}'.format(retval)
             if retval == 0:
                 logging.info(message)

=== modified file 'jujupy/fake.py'
--- jujupy/fake.py	2017-03-17 20:43:14 +0000
+++ jujupy/fake.py	2017-03-23 19:29:02 +0000
@@ -19,6 +19,7 @@
     JujuData,
     SoftDeadlineExceeded,
 )
+from jujupy.client import CommandTime
 
 __metaclass__ = type
 
@@ -977,6 +978,7 @@
                     self.controller_state.shares.remove(username)
             if command == 'restore-backup':
                 model_state.restore_backup()
+            return 0, CommandTime(command, args)
 
     @contextmanager
     def juju_async(self, command, args, used_feature_flags,

=== modified file 'jujupy/tests/test_client.py'
--- jujupy/tests/test_client.py	2017-03-23 19:29:02 +0000
+++ jujupy/tests/test_client.py	2017-03-23 19:29:02 +0000
@@ -104,6 +104,7 @@
     FakeHomeTestCase,
     FakePopen,
     observable_temp_file,
+    patch_juju_call,
     TestCase,
     )
 from jujupy.utility import (
@@ -904,7 +905,7 @@
     def test_destroy_model(self):
         env = JujuData('foo', {'type': 'ec2'})
         client = ModelClient(env, None, None)
-        with patch.object(client, 'juju') as mock:
+        with patch_juju_call(client) as mock:
             client.destroy_model()
         mock.assert_called_with(
             'destroy-model', ('foo', '-y'),
@@ -913,7 +914,7 @@
     def test_destroy_model_azure(self):
         env = JujuData('foo', {'type': 'azure'})
         client = ModelClient(env, None, None)
-        with patch.object(client, 'juju') as mock:
+        with patch_juju_call(client) as mock:
             client.destroy_model()
         mock.assert_called_with(
             'destroy-model', ('foo', '-y'),
@@ -922,7 +923,7 @@
     def test_destroy_model_gce(self):
         env = JujuData('foo', {'type': 'gce'})
         client = ModelClient(env, None, None)
-        with patch.object(client, 'juju') as mock:
+        with patch_juju_call(client) as mock:
             client.destroy_model()
         mock.assert_called_with(
             'destroy-model', ('foo', '-y'),

=== modified file 'jujupy/tests/test_version_client.py'
--- jujupy/tests/test_version_client.py	2017-03-10 21:15:12 +0000
+++ jujupy/tests/test_version_client.py	2017-03-23 19:29:02 +0000
@@ -573,7 +573,8 @@
     def test_destroy_environment(self):
         env = SimpleEnvironment('foo', {'type': 'ec2'})
         client = EnvJujuClient1X(env, None, None)
-        with patch.object(client, 'juju') as mock:
+        with patch.object(
+                client, 'juju', autospec=True, return_value=(0, None)) as mock:
             client.destroy_environment()
         mock.assert_called_with(
             'destroy-environment', ('foo', '--force', '-y'),
@@ -582,7 +583,9 @@
     def test_destroy_environment_no_force(self):
         env = SimpleEnvironment('foo', {'type': 'ec2'})
         client = EnvJujuClient1X(env, None, None)
-        with patch.object(client, 'juju') as mock:
+        with patch.object(
+                client, 'juju',
+                autospec=True, return_value=(0, None)) as mock:
             client.destroy_environment(force=False)
             mock.assert_called_with(
                 'destroy-environment', ('foo', '-y'),
@@ -591,7 +594,8 @@
     def test_destroy_environment_azure(self):
         env = SimpleEnvironment('foo', {'type': 'azure'})
         client = EnvJujuClient1X(env, None, None)
-        with patch.object(client, 'juju') as mock:
+        with patch.object(
+                client, 'juju', autospec=True, return_value=(0, None)) as mock:
             client.destroy_environment(force=False)
             mock.assert_called_with(
                 'destroy-environment', ('foo', '-y'),
@@ -600,7 +604,9 @@
     def test_destroy_environment_gce(self):
         env = SimpleEnvironment('foo', {'type': 'gce'})
         client = EnvJujuClient1X(env, None, None)
-        with patch.object(client, 'juju') as mock:
+        with patch.object(
+                client, 'juju',
+                autospec=True, return_value=(0, None)) as mock:
             client.destroy_environment(force=False)
             mock.assert_called_with(
                 'destroy-environment', ('foo', '-y'),
@@ -609,7 +615,8 @@
     def test_destroy_environment_delete_jenv(self):
         env = SimpleEnvironment('foo', {'type': 'ec2'})
         client = EnvJujuClient1X(env, None, None)
-        with patch.object(client, 'juju'):
+        with patch.object(
+                client, 'juju', autospec=True, return_value=(0, None)):
             with temp_env({}) as juju_home:
                 client.env.juju_home = juju_home
                 jenv_path = get_jenv_path(juju_home, 'foo')
@@ -622,7 +629,8 @@
     def test_destroy_model(self):
         env = SimpleEnvironment('foo', {'type': 'ec2'})
         client = EnvJujuClient1X(env, None, None)
-        with patch.object(client, 'juju') as mock:
+        with patch.object(
+                client, 'juju', autospec=True, return_value=(0, None)) as mock:
             client.destroy_model()
         mock.assert_called_with(
             'destroy-environment', ('foo', '-y'),
@@ -631,7 +639,9 @@
     def test_kill_controller(self):
         client = EnvJujuClient1X(
             SimpleEnvironment('foo', {'type': 'ec2'}), None, None)
-        with patch.object(client, 'juju') as juju_mock:
+        with patch.object(
+                client, 'juju',
+                autospec=True, return_value=(0, None)) as juju_mock:
             client.kill_controller()
         juju_mock.assert_called_once_with(
             'destroy-environment', ('foo', '--force', '-y'), check=False,

=== modified file 'jujupy/version_client.py'
--- jujupy/version_client.py	2017-03-10 19:54:30 +0000
+++ jujupy/version_client.py	2017-03-23 19:29:02 +0000
@@ -498,7 +498,7 @@
             force_arg = ('--force',)
         else:
             force_arg = ()
-        exit_status = self.juju(
+        exit_status, _ = self.juju(
             'destroy-environment',
             (self.env.environment,) + force_arg + ('-y',),
             check=False, include_e=False,

=== modified file 'tests/__init__.py'
--- tests/__init__.py	2017-03-10 19:46:30 +0000
+++ tests/__init__.py	2017-03-23 19:29:02 +0000
@@ -152,6 +152,17 @@
         os.environ[key] = org_value
 
 
+@contextmanager
+def patch_juju_call(client, return_value=(0, None)):
+    """Simple patch for client.juju call.
+
+    :param return_value: A tuple to return representing the retvar and
+      CommandTime object
+    """
+    with patch.object(client, 'juju', return_value=return_value) as mock:
+        yield mock
+
+
 def assert_juju_call(test_case, mock_method, client, expected_args,
                      call_index=None):
     """Check a mock's positional arguments.

=== modified file 'tests/test_assess_container_networking.py'
--- tests/test_assess_container_networking.py	2017-01-20 20:58:41 +0000
+++ tests/test_assess_container_networking.py	2017-03-23 19:29:02 +0000
@@ -17,6 +17,7 @@
     LXD_MACHINE,
     SimpleEnvironment,
     )
+from jujupy.client import CommandTime
 
 import assess_container_networking as jcnet
 from tests import (
@@ -78,6 +79,29 @@
         if cmd != 'bootstrap':
             self.commands.append((cmd, args))
         if cmd == 'ssh':
+            ct = CommandTime(cmd, args)
+            if len(self._ssh_output) == 0:
+                return "", ct
+
+            try:
+                ct = CommandTime(cmd, args)
+                return self._ssh_output[self._call_number()], ct
+            except IndexError:
+                # If we ran out of values, just return the last one
+                return self._ssh_output[-1], ct
+        else:
+            return super(JujuMock, self).juju(cmd, *rargs, **kwargs)
+
+    def get_juju_output(self, cmd, *rargs, **kwargs):
+        # Almost exactly like juju() except get_juju_output doesn't return
+        # a CommandTime
+        if len(rargs) == 1:
+            args = rargs[0]
+        else:
+            args = rargs
+        if cmd != 'bootstrap':
+            self.commands.append((cmd, args))
+        if cmd == 'ssh':
             if len(self._ssh_output) == 0:
                 return ""
 
@@ -87,7 +111,7 @@
                 # If we ran out of values, just return the last one
                 return self._ssh_output[-1]
         else:
-            return super(JujuMock, self).juju(cmd, *rargs, **kwargs)
+            return super(JujuMock, self).get_juju_output(cmd, *rargs, **kwargs)
 
     def _call_number(self):
         call_number = self._call_n
@@ -117,7 +141,9 @@
             patch.object(self.client, 'wait_for', lambda *args, **kw: None),
             patch.object(self.client, 'wait_for_started',
                          self.juju_mock.get_status),
-            patch.object(self.client, 'get_juju_output', self.juju_mock.juju),
+            patch.object(
+                self.client, 'get_juju_output',
+                self.juju_mock.get_juju_output),
         ]
 
         for patcher in patches: