Merge lp:~chad.smith/charm-helpers/retry-add-apt-repository into lp:charm-helpers

[Chad Smith]

Add 3 retries and logging messages to add-apt-repository attempts.

This branch involves some slight refactoring of _run_apt_command so that the common code _retry_command() can be reused by both _run_apt_command and add_source.

Note this branch also includes the local environment now in add-apt-repository calls which should also help in environments with proxy settings present.

[lp:~chad.smith/charm-helpers/retry-add-apt-repository]

708. By Chad Smith on 2017-03-03

comment fix

[David Britton (dpb)]

Small nits inline. Code tests OK, lints OK. Thanks for the contribution. +1 with the small fixes addressed!

review: Approve

[lp:~chad.smith/charm-helpers/retry-add-apt-repository]

709. By Chad Smith on 2017-03-03

- rename -APT_NO_LOCK_RETRY_COUNT -> CMD_RETRY_COUNT
- Retry add-apt-repository CMD_RETRY_COUNT (30) times instead of 3
- update unit tests

710. By Chad Smith on 2017-03-03

lints

[Eric Snow (ericsnowcurrently)]

review: Needs Fixing

[lp:~chad.smith/charm-helpers/retry-add-apt-repository]

711. By Chad Smith on 2017-03-03

address review comments, sensible defaults, drop fatal from from _run_with_retries to simplify logic

[Eric Snow (ericsnowcurrently)]

Thanks! That's exactly how I was thinking it should look. :)

review: Approve

=== modified file 'charmhelpers/fetch/ubuntu.py'
--- charmhelpers/fetch/ubuntu.py	2016-11-14 16:28:24 +0000
+++ charmhelpers/fetch/ubuntu.py	2017-03-03 20:38:43 +0000
@@ -116,8 +116,8 @@
 }
 
 APT_NO_LOCK = 100  # The return code for "couldn't acquire lock" in APT.
-APT_NO_LOCK_RETRY_DELAY = 10  # Wait 10 seconds between apt lock checks.
-APT_NO_LOCK_RETRY_COUNT = 30  # Retry to acquire the lock X times.
+CMD_RETRY_DELAY = 10  # Wait 10 seconds between command retries.
+CMD_RETRY_COUNT = 30  # Retry a failing fatal command X times.
 
 
 def filter_installed_packages(packages):
@@ -249,7 +249,8 @@
         source.startswith('http') or
         source.startswith('deb ') or
             source.startswith('cloud-archive:')):
-        subprocess.check_call(['add-apt-repository', '--yes', source])
+        cmd = ['add-apt-repository', '--yes', source]
+        _run_with_retries(cmd)
     elif source.startswith('cloud:'):
         install(filter_installed_packages(['ubuntu-cloud-keyring']),
                 fatal=True)
@@ -286,41 +287,60 @@
                                    key])
 
 
+def _run_with_retries(cmd, max_retries=CMD_RETRY_COUNT, retry_exitcodes=(1,),
+                      retry_message="", cmd_env=None):
+    """Run a command and retry until success or max_retries is reached.
+
+    :param: cmd: str: The apt command to run.
+    :param: max_retries: int: The number of retries to attempt on a fatal
+        command. Defaults to CMD_RETRY_COUNT.
+    :param: retry_exitcodes: tuple: Optional additional exit codes to retry.
+        Defaults to retry on exit code 1.
+    :param: retry_message: str: Optional log prefix emitted during retries.
+    :param: cmd_env: dict: Environment variables to add to the command run.
+    """
+
+    env = os.environ.copy()
+    if cmd_env:
+        env.update(cmd_env)
+
+    if not retry_message:
+        retry_message = "Failed executing '{}'".format(" ".join(cmd))
+    retry_message += ". Will retry in {} seconds".format(CMD_RETRY_DELAY)
+
+    retry_count = 0
+    result = None
+
+    retry_results = (None,) + retry_exitcodes
+    while result in retry_results:
+        try:
+            result = subprocess.check_call(cmd, env=env)
+        except subprocess.CalledProcessError as e:
+            retry_count = retry_count + 1
+            if retry_count > max_retries:
+                raise
+            result = e.returncode
+            log(retry_message)
+            time.sleep(CMD_RETRY_DELAY)
+
+
 def _run_apt_command(cmd, fatal=False):
-    """Run an APT command.
-
-    Checks the output and retries if the fatal flag is set
-    to True.
-
-    :param: cmd: str: The apt command to run.
+    """Run an apt command with optional retries.
+
     :param: fatal: bool: Whether the command's output should be checked and
         retried.
     """
-    env = os.environ.copy()
-
-    if 'DEBIAN_FRONTEND' not in env:
-        env['DEBIAN_FRONTEND'] = 'noninteractive'
+    # Provide DEBIAN_FRONTEND=noninteractive if not present in the environment.
+    cmd_env = {
+        'DEBIAN_FRONTEND': os.environ.get('DEBIAN_FRONTEND', 'noninteractive')}
 
     if fatal:
-        retry_count = 0
-        result = None
-
-        # If the command is considered "fatal", we need to retry if the apt
-        # lock was not acquired.
-
-        while result is None or result == APT_NO_LOCK:
-            try:
-                result = subprocess.check_call(cmd, env=env)
-            except subprocess.CalledProcessError as e:
-                retry_count = retry_count + 1
-                if retry_count > APT_NO_LOCK_RETRY_COUNT:
-                    raise
-                result = e.returncode
-                log("Couldn't acquire DPKG lock. Will retry in {} seconds."
-                    "".format(APT_NO_LOCK_RETRY_DELAY))
-                time.sleep(APT_NO_LOCK_RETRY_DELAY)
-
+        _run_with_retries(
+            cmd, cmd_env=cmd_env, retry_exitcodes=(1, APT_NO_LOCK,),
+            retry_message="Couldn't acquire DPKG lock")
     else:
+        env = os.environ.copy()
+        env.update(cmd_env)
         subprocess.call(cmd, env=env)
 
 

=== modified file 'tests/fetch/test_fetch.py'
--- tests/fetch/test_fetch.py	2016-09-20 17:07:51 +0000
+++ tests/fetch/test_fetch.py	2017-03-03 20:38:43 +0000
@@ -192,7 +192,35 @@
         source = "ppa:test-ppa"
         fetch.add_source(source=source)
         check_call.assert_called_with(
-            ['add-apt-repository', '--yes', source])
+            ['add-apt-repository', '--yes', source], env=getenv())
+
+    @patch("charmhelpers.fetch.ubuntu.log")
+    @patch.object(osplatform, 'get_platform')
+    @patch('subprocess.check_call')
+    @patch('time.sleep')
+    def test_add_source_ppa_retries_30_times(self, sleep, check_call,
+                                             platform, log):
+        platform.return_value = 'ubuntu'
+        imp.reload(fetch)
+
+        self.call_count = 0
+
+        def side_effect(*args, **kwargs):
+            """Raise an 3 times, then return 0 """
+            self.call_count += 1
+            if self.call_count <= fetch.ubuntu.CMD_RETRY_COUNT:
+                raise subprocess.CalledProcessError(
+                    returncode=1, cmd="some add-apt-repository command")
+            else:
+                return 0
+        check_call.side_effect = side_effect
+
+        source = "ppa:test-ppa"
+        fetch.add_source(source=source)
+        check_call.assert_called_with(
+            ['add-apt-repository', '--yes', source], env=getenv())
+        sleep.assert_called_with(10)
+        self.assertTrue(fetch.ubuntu.CMD_RETRY_COUNT, sleep.call_count)
 
     @patch('charmhelpers.fetch.ubuntu.log')
     @patch.object(osplatform, 'get_platform')
@@ -204,7 +232,7 @@
         source = "http://archive.ubuntu.com/ubuntu raring-backports main"
         fetch.add_source(source=source)
         check_call.assert_called_with(
-            ['add-apt-repository', '--yes', source])
+            ['add-apt-repository', '--yes', source], env=getenv())
 
     @patch('charmhelpers.fetch.centos.log')
     @patch.object(osplatform, 'get_platform')
@@ -233,7 +261,7 @@
         source = "https://example.com"
         fetch.add_source(source=source)
         check_call.assert_called_with(
-            ['add-apt-repository', '--yes', source])
+            ['add-apt-repository', '--yes', source], env=getenv())
 
     @patch('charmhelpers.fetch.ubuntu.log')
     @patch.object(osplatform, 'get_platform')
@@ -261,7 +289,7 @@
         source = "deb http://archive.ubuntu.com/ubuntu raring-backports main"
         fetch.add_source(source=source)
         check_call.assert_called_with(
-            ['add-apt-repository', '--yes', source])
+            ['add-apt-repository', '--yes', source], env=getenv())
 
     @patch.object(osplatform, 'get_platform')
     @patch.object(fetch.ubuntu, 'filter_installed_packages')
@@ -357,9 +385,10 @@
 
         source = "http://archive.ubuntu.com/ubuntu raring-backports main"
         key_id = "akey"
+        check_call.return_value = 0  # Successful exit code
         fetch.add_source(source=source, key=key_id)
         check_call.assert_has_calls([
-            call(['add-apt-repository', '--yes', source]),
+            call(['add-apt-repository', '--yes', source], env=getenv()),
             call(['apt-key', 'adv', '--keyserver',
                   'hkp://keyserver.ubuntu.com:80', '--recv', key_id])
         ])
@@ -390,6 +419,7 @@
     @patch('subprocess.check_call')
     def test_add_source_https_and_key_id_ubuntu(self, check_call,
                                                 platform, log):
+        check_call.return_value = 0  # Success from both calls
         platform.return_value = 'ubuntu'
         imp.reload(fetch)
 
@@ -397,7 +427,7 @@
         key_id = "GPGPGP"
         fetch.add_source(source=source, key=key_id)
         check_call.assert_has_calls([
-            call(['add-apt-repository', '--yes', source]),
+            call(['add-apt-repository', '--yes', source], env=getenv()),
             call(['apt-key', 'adv', '--keyserver',
                   'hkp://keyserver.ubuntu.com:80', '--recv', key_id])
         ])
@@ -442,16 +472,18 @@
         received_args = []
         received_key = StringIO()
 
-        def _check_call(arg, stdin=None):
+        def _check_call(arg, stdin=None, env=None):
             '''side_effect to store the stdin passed to check_call process.'''
             if stdin is not None:
                 received_args.extend(arg)
                 received_key.write(stdin.read())
+            return 0  # Successful return code checked by add-apt-repository
 
         with patch('subprocess.check_call',
                    side_effect=_check_call) as check_call:
             fetch.add_source(source=source, key=key)
-            check_call.assert_any_call(['add-apt-repository', '--yes', source])
+            check_call.assert_any_call(
+                ['add-apt-repository', '--yes', source], env=getenv())
             self.assertEqual(['apt-key', 'add', '-'], received_args)
             self.assertEqual(key, received_key.getvalue())