Merge lp:~gandelman-a/juju-deployer/retry_cli into lp:~hazmat/juju-deployer/refactor

[Adam Gandelman]

Allows for failed CLI commands to be optionally retried. We seem to hit transient provider errors with a long running MAAS environments, and the previous deployer plowed on thru via retries. Shares the same retry_count used when resolving unit errors.

[lp:~gandelman-a/juju-deployer/retry_cli]

111. By Adam Gandelman on 2013-07-31

Fix AttributeError in original retry_count usage.

=== modified file 'deployer/action/importer.py'
--- deployer/action/importer.py	2013-07-22 15:29:31 +0000
+++ deployer/action/importer.py	2013-07-31 01:56:27 +0000
@@ -163,5 +163,5 @@
         if self.options.retry_count:
             self.log.info("Looking for errors to auto-retry")
             self.env.resolve_errors(
-                self.retry_count,
+                self.options.retry_count,
                 self.options.timeout - time.time() - self.start_time)

=== modified file 'deployer/cli.py'
--- deployer/cli.py	2013-07-23 20:33:37 +0000
+++ deployer/cli.py	2013-07-31 01:56:27 +0000
@@ -89,12 +89,12 @@
         dest="watch", action="store_true", default=False)
     parser.add_argument(
         '-r', "--retry", default=0, type=int, dest="retry_count",
-        help=("Resolve unit errors via retry."
+        help=("Resolve CLI and unit errors via number of retries (default: 0)."
               " Either standalone or in a deployment"))
     parser.add_argument(
         "--diff", action="store_true", default=False,
-        help=("Resolve unit errors via retry."
-              " Either standalone or in a deployment"))
+        help=("Generate a delta between a configured deployment and a running"
+             " environment."))
     parser.add_argument(
         '-w', '--relation-wait', action='store', dest='rel_wait',
         default=60, type=int,
@@ -127,7 +127,7 @@
     log = logging.getLogger("deployer.cli")
     start_time = time.time()
 
-    env = select_runtime(options.juju_env)
+    env = select_runtime(options.juju_env, options)
     log.debug('Using runtime %s', env.__class__.__name__)
 
     config = ConfigStack(options.configs or [])

=== modified file 'deployer/env/__init__.py'
--- deployer/env/__init__.py	2013-07-22 15:29:31 +0000
+++ deployer/env/__init__.py	2013-07-31 01:56:27 +0000
@@ -4,9 +4,10 @@
 from ..utils import _check_call
 
 
-def select_runtime(env_name):
+def select_runtime(name, options):
     # pyjuju does juju --version
     result = _check_call(["juju", "version"], None, ignoreerr=True)
     if result is None:
-        return PyEnvironment(env_name)
-    return GoEnvironment(env_name)
+        return PyEnvironment(name, options)
+    return GoEnvironment(name, options)
+

=== modified file 'deployer/env/base.py'
--- deployer/env/base.py	2013-07-22 15:29:31 +0000
+++ deployer/env/base.py	2013-07-31 01:56:27 +0000
@@ -15,6 +15,11 @@
         env_config_path = path_join(jhome, 'environments.yaml')
         return env_config_path
 
+    def _check_call(self, *args, **kwargs):
+        if self.options and self.options.retry_count:
+            kwargs['max_retry'] = self.options.retry_count
+        return _check_call(*args, **kwargs)
+
     def _named_env(self, params):
         if self.name:
             params.extend(["-e", self.name])
@@ -80,7 +85,8 @@
                 params.extend["--force-machine=%d" % force_machine]
 
             params.extend([charm_url, name])
-            _check_call(params, self.log, "Error deploying service %r", name)
+            self._check_call(
+                params, self.log, "Error deploying service %r", name)
 
     def terminate_machine(self, mid, wait=False):
         """Terminate a machine.
@@ -95,7 +101,8 @@
         params = self._named_env(["juju", "terminate-machine"])
         params.append(mid)
         try:
-            _check_call(params, self.log, "Error terminating machine %r" % mid)
+            self._check_call(
+                params, self.log, "Error terminating machine %r" % mid)
         except ErrorExit, e:
             if ("machine %s does not exist" % mid) in e.error.output:
                 return
@@ -115,7 +122,7 @@
     def get_cli_status(self):
         params = self._named_env(["juju", "status"])
         with open('/dev/null', 'w') as fh:
-            output = _check_call(
+            output = self._check_call(
                 params, self.log, "Error getting status, is it bootstrapped?",
                 stderr=fh)
         status = yaml_load(output)

=== modified file 'deployer/env/go.py'
--- deployer/env/go.py	2013-07-22 15:29:31 +0000
+++ deployer/env/go.py	2013-07-31 01:56:27 +0000
@@ -10,8 +10,9 @@
 
 class GoEnvironment(BaseEnvironment):
 
-    def __init__(self, name, endpoint=None):
+    def __init__(self, name, options=None, endpoint=None):
         self.name = name
+        self.options = options
         self.api_endpoint = endpoint
         self.client = None
 

=== modified file 'deployer/env/py.py'
--- deployer/env/py.py	2013-07-22 15:29:31 +0000
+++ deployer/env/py.py	2013-07-31 01:56:27 +0000
@@ -1,28 +1,29 @@
 import time
 
 from deployer.errors import UnitErrors
-from deployer.utils import _check_call, ErrorExit
+from deployer.utils import ErrorExit
 
 from .base import BaseEnvironment
 
 
 class PyEnvironment(BaseEnvironment):
 
-    def __init__(self, name):
+    def __init__(self, name, options=None):
         self.name = name
+        self.options = options
 
     def add_units(self, service_name, num_units):
         params = self._named_env(["juju", "add-unit"])
         if num_units > 1:
             params.extend(["-n", str(num_units)])
         params.append(service_name)
-        _check_call(
+        self._check_call(
             params, self.log, "Error adding units to %s", service_name)
 
     def add_relation(self, endpoint_a, endpoint_b):
         params = self._named_env(["juju", "add-relation"])
         params.extend([endpoint_a, endpoint_b])
-        _check_call(
+        self._check_call(
             params, self.log, "Error adding relation to %s %s",
             endpoint_a, endpoint_b)
 
@@ -35,19 +36,19 @@
     def _destroy_service(self, service_name):
         params = self._named_env(["juju", "destroy-service"])
         params.append(service_name)
-        _check_call(
+        self._check_call(
             params, self.log, "Error destroying service %s" % service_name)
 
     def get_config(self, svc_name):
         params = self._named_env(["juju", "get"])
         params.append(svc_name)
-        return _check_call(
+        return self._check_call(
             params, self.log, "Error retrieving config for %r", svc_name)
 
     def get_constraints(self, svc_name):
         params = self._named_env(["juju", "get-constraints"])
         params.append(svc_name)
-        return _check_call(
+        return self._check_call(
             params, self.log, "Error retrieving constraints for %r", svc_name)
 
     def reset(self,

=== modified file 'deployer/tests/test_utils.py'
--- deployer/tests/test_utils.py	2013-07-22 15:29:31 +0000
+++ deployer/tests/test_utils.py	2013-07-31 01:56:27 +0000
@@ -1,5 +1,7 @@
+from mock import patch, MagicMock
+from subprocess import CalledProcessError
 from .base import Base
-from deployer.utils import dict_merge
+from deployer.utils import dict_merge, _check_call, ErrorExit
 
 
 class UtilTests(Base):
@@ -17,3 +19,32 @@
                 {'a': 1},
                 {'relations': [['m1', 'x1'], ['m2', 'x2']]}),
             {'a': 1, 'relations': [['m1', 'x1'], ['m2', 'x2']]})
+
+    @patch('subprocess.check_output')
+    def test_check_call_fail_no_retry(self, check_output):
+        _e = CalledProcessError(returncode=1, cmd=['fail'])
+        check_output.side_effect = _e
+        self.assertRaises(
+            ErrorExit, _check_call, params=['fail'], log=MagicMock())
+
+    @patch('time.sleep')
+    @patch('subprocess.check_output')
+    def test_check_call_fail_retry(self, check_output, sleep):
+        _e = CalledProcessError(returncode=1, cmd=['fail'])
+        check_output.side_effect = _e
+        self.assertRaises(
+            ErrorExit, _check_call, params=['fail'], log=MagicMock(), max_retry=3)
+        # 1 failure + 3 retries
+        self.assertEquals(len(check_output.call_args_list), 4)
+
+    @patch('time.sleep')
+    @patch('subprocess.check_output')
+    def test_check_call_succeed_after_retry(self, check_output, sleep):
+        # call succeeds after the 3rd try
+        _e = CalledProcessError(returncode=1, cmd=['maybe_fail'])
+        check_output.side_effect = [
+            _e, _e, 'good', _e ]
+        output = _check_call(params=['magybe_fail'], log=MagicMock(), max_retry=3)
+        self.assertEquals(output, 'good')
+        # 1 failure + 3 retries
+        self.assertEquals(len(check_output.call_args_list), 3)

=== modified file 'deployer/utils.py'
--- deployer/utils.py	2013-07-22 15:29:31 +0000
+++ deployer/utils.py	2013-07-31 01:56:27 +0000
@@ -11,6 +11,7 @@
 
 import stat
 import subprocess
+import time
 import tempfile
 import zipfile
 
@@ -150,6 +151,8 @@
 
 
 def _check_call(params, log, *args, **kw):
+    max_retry = kw.get('max_retry', None)
+    cur = kw.get('cur_try', 1)
     try:
         cwd = abspath(".")
         if 'cwd' in kw:
@@ -169,7 +172,12 @@
         #print e.output
         log.error(*args)
         log.error("Command (%s) Output:\n\n %s", " ".join(params), e.output)
-        raise ErrorExit(e)
+        if not max_retry or cur > max_retry:
+            raise ErrorExit(e)
+        kw['cur_try'] = cur + 1
+        log.error("Retrying (%s of %s)" % (cur, max_retry))
+        time.sleep(1)
+        output = _check_call(params, log, args, **kw)
     return output