Merge lp:~sinzui/juju-release-tools/retry-swift into lp:juju-release-tools

[Curtis Hovey]

Retry swift uploads.

The publish-weekly job has failed two weeks in a row because of swift. Once for canonistack and once for hp. Modern swiftclient supports --retries, but the older version used by precise does not. This branch add a retry rule to make sync_swift more robust.

[Martin Packman (gz)]

Looks good.

I presume we're fine with maybe retrying three times per file? Not sure how many swift calls we might reach if the api is just down or something, maybe enough to hit rate limits?

review: Approve

=== modified file 'swift_sync.py'
--- swift_sync.py	2014-10-03 03:53:22 +0000
+++ swift_sync.py	2015-06-10 22:27:48 +0000
@@ -11,6 +11,9 @@
 import urllib2
 
 
+MAX_UPLOAD_ATTEMPTS = 3
+
+
 def get_account():
     cmd = ['swift', 'stat']
     output = subprocess.check_output(cmd)
@@ -37,6 +40,7 @@
 def upload_changes(args, remote_files):
     container_path = os.path.join(args.container, args.path)
     count = 0
+    uploaded_files = []
     for file_name in args.files:
         local_path = os.path.join(args.path, file_name)
         remote_file = remote_files.get(local_path)
@@ -61,10 +65,22 @@
         print("Uploading {0}/{1}".format(args.container, local_path))
         cmd = ['swift', 'upload', container_path, file_name]
         if not args.dry_run:
-            output = subprocess.check_output(cmd)
+            uploaded = False
+            attempt = 1
+            while not uploaded and attempt <= MAX_UPLOAD_ATTEMPTS:
+                # python-swiftclient on precise doesn't support --retry.
+                try:
+                    output = subprocess.check_output(cmd)
+                    uploaded = True
+                except subprocess.CalledProcessError:
+                    attempt += 1
+                    if attempt > MAX_UPLOAD_ATTEMPTS:
+                        raise
             print(' '.join(cmd))
             print(output)
+            uploaded_files.append(output)
     print('Uploaded {0} files'.format(count))
+    return uploaded_files
 
 
 def main():

=== added file 'tests/test_swift_sync.py'
--- tests/test_swift_sync.py	1970-01-01 00:00:00 +0000
+++ tests/test_swift_sync.py	2015-06-10 22:27:48 +0000
@@ -0,0 +1,83 @@
+from argparse import Namespace
+import hashlib
+from mock import patch
+import os
+from subprocess import CalledProcessError
+from unittest import TestCase
+
+from swift_sync import (
+    upload_changes,
+)
+from utils import (
+    temp_dir,
+)
+
+
+def make_local_files(base, files):
+    local_files = []
+    for name in files:
+        file_path = os.path.join(base, name)
+        local_files.append(file_path)
+        with open(file_path, 'w') as f:
+            f.write(name)
+    return local_files
+
+
+class SwiftSyncTestCase(TestCase):
+
+    def test_upload_changes(self):
+        # Only new and changed files are uploaded.
+        md5 = hashlib.md5()
+        md5.update('one')
+        one_hash = md5.hexdigest()
+        with temp_dir() as base:
+            remote_name = os.path.join(base, 'one')
+            remote_files = {
+                remote_name: {'name': remote_name, 'hash': one_hash}
+            }
+            local_files = make_local_files(base, ['one', 'two'])
+            args = Namespace(
+                container='foo', path=base, files=local_files,
+                verbose=False, dry_run=False)
+            with patch('subprocess.check_output', autospec=True,
+                       return_value='two') as co_mock:
+                uploaded_files = upload_changes(args, remote_files)
+        self.assertEqual(['two'], uploaded_files)
+        co_mock.assert_called_once_with(
+            ['swift', 'upload', base, os.path.join(base, 'two')])
+
+    def test_upload_changes_reties(self):
+        with temp_dir() as base:
+            remote_files = {}
+            local_files = make_local_files(base, ['one'])
+            args = Namespace(
+                container='foo', path=base, files=local_files,
+                verbose=False, dry_run=False)
+            outputs = [
+                CalledProcessError(1, 'a'),
+                CalledProcessError(2, 'b'),
+                'one']
+            with patch('subprocess.check_output', autospec=True,
+                       side_effect=outputs) as co_mock:
+                uploaded_files = upload_changes(args, remote_files)
+        self.assertEqual(['one'], uploaded_files)
+        self.assertEqual(3, co_mock.call_count)
+        co_mock.assert_called_with(
+            ['swift', 'upload', base, os.path.join(base, 'one')])
+
+    def test_upload_changes_error(self):
+        with temp_dir() as base:
+            remote_files = {}
+            local_files = make_local_files(base, ['one'])
+            args = Namespace(
+                container='foo', path=base, files=local_files,
+                verbose=False, dry_run=False)
+            outputs = [
+                CalledProcessError(1, 'a'),
+                CalledProcessError(2, 'b'),
+                CalledProcessError(3, 'c')]
+            with patch('subprocess.check_output', autospec=True,
+                       side_effect=outputs) as co_mock:
+                with self.assertRaises(CalledProcessError):
+                    upload_changes(args, remote_files)
+            self.assertEqual(3, co_mock.call_count)