Merge lp:~vila/uci-engine/delete-containers into lp:uci-engine

[Vincent Ladeuil]

Safely delete swift containers

This builds on top of https://code.launchpad.net/~vila/uci-engine/cleanup-data-store/+merge/242218 and extends the swift client retry policy to the delete container request.

This allows us to reliably delete swift containers whne they become useless (in-production tests, uci-britney, test themselves, etc).

I've tuned the overall timeout to ~5 minutes based on previous experimentations which showed real life inconsistency windows going up to 2 minutes.

[lp:~vila/uci-engine/delete-containers]

909. By Vincent Ladeuil on 2014-11-20

One more test to ensure we fail after all retries (with a test reproducing the bug).

Tested with: ./run-tests ^tests.test_style ^tests.test_data_store ^ci-utils.*test_fixtures ^ci-utils.*test_data_store

910. By Vincent Ladeuil on 2014-11-20

One more test for the retries=0 case to address review comment.

911. By Vincent Ladeuil on 2014-11-20

Fix pyflakes issue.

912. By Vincent Ladeuil on 2014-11-20

Fix pep8 issue.

[Evan (ev)]

review: Needs Fixing

Unmerged revisions

912. By Vincent Ladeuil on 2014-11-20

Fix pep8 issue.

911. By Vincent Ladeuil on 2014-11-20

Fix pyflakes issue.

910. By Vincent Ladeuil on 2014-11-20

One more test for the retries=0 case to address review comment.

909. By Vincent Ladeuil on 2014-11-20

One more test to ensure we fail after all retries (with a test reproducing the bug).

Tested with: ./run-tests ^tests.test_style ^tests.test_data_store ^ci-utils.*test_fixtures ^ci-utils.*test_data_store

908. By Vincent Ladeuil on 2014-11-20

Extend swiftclient retry policy to container deletion with tests.

907. By Vincent Ladeuil on 2014-11-19

Meh, we want to test the real data store with a fake swift client, not the fake data store ;)

906. By Vincent Ladeuil on 2014-11-19

One less FIXME and a test plan.

905. By Vincent Ladeuil on 2014-11-19

Tested with ./run-tests ^tests.test_style ^tests.test_data_store befor submission.

(and some cosmetic changes)

904. By Vincent Ladeuil on 2014-11-19

This test pollutes the run output.

903. By Vincent Ladeuil on 2014-11-19

Log auth info (except for the password).

Rewrite exception in list_files.

=== modified file 'britney_proxy/britney/process_results.py'
--- britney_proxy/britney/process_results.py	2014-10-30 17:30:49 +0000
+++ britney_proxy/britney/process_results.py	2014-11-20 08:47:10 +0000
@@ -51,11 +51,7 @@
 # container. This won't scale but we'll revisit the layout when we get a better
 # understanding of the britney needs (and numbers !) -- vila 2014-10-03
 class BritneyDataStore(data_store.DataStore):
-
-    def file_name(self, filename):
-        # Allow paths to be used as object names in the container by overriding
-        # the filtering in the base class.
-        return filename
+    pass
 
 
 class BritneyResultHandler(object):

=== modified file 'ci-utils/ci_utils/data_store.py'
--- ci-utils/ci_utils/data_store.py	2014-10-07 10:04:01 +0000
+++ ci-utils/ci_utils/data_store.py	2014-11-20 08:47:10 +0000
@@ -14,10 +14,10 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import logging
-import os
+import time
+
 
 from swiftclient import client
-from swiftclient.client import ClientException
 
 
 class DataStoreException(Exception):
@@ -31,11 +31,26 @@
 class DataStore(object):
     """Store and retrieve files into a swift container."""
 
-    def __init__(self, component, auth_config, public=False):
+    def __init__(self, component, auth_config, public=False,
+                 # FIXME: These should really be config options as all data
+                 # stores will use the same but in the mean time, having them
+                 # available here address the test needs -- vila 2014-11-19
+                 starting_backoff=1, max_backoff=64, retries=10):
         self.container_id = component
         self.auth_config = auth_config
         self.public = public
         self.client = None
+        # The swiftclient requests are retried with increasing timeouts based
+        # on 'starting_backoff' and 'max_backoff'. For simplicity, we apply the
+        # same policy to delete_container() on 409 errors (Conflict: container
+        # not empty) which swiftclient doesn't take into account.
+        # FIXME: This implementation misses the random part in the timeout
+        # calculation -- vila 2014-11-20
+        self.starting_backoff = starting_backoff
+        self.max_backoff = max_backoff
+        # The 7 first backoff values will be: 1, 2, 4... 64 => 127secs, so 10
+        # retries will give us 5 minutes
+        self.retries = retries
 
     @staticmethod
     def validate_auth_config(auth_config):
@@ -50,33 +65,45 @@
 
         if len(missing_fields) > 0:
             raise DataStoreException(
-                "missing fields: {}".format(', '.join(missing_fields))
-            )
+                "missing fields: {}".format(', '.join(missing_fields)))
 
     def ensure_swift_client(self):
+        """Ensures we have a valid swift client and a container.
+
+        This allows callers of the DataStore to call any part of the API
+        without worrying about creating the container.
+        """
         if self.client is not None:
             return self.client
 
         self.validate_auth_config(self.auth_config)
         config = self.auth_config
-        self.client = client.Connection(
-            authurl=config.get('auth_url'),
-            user=config.get('auth_user'),
-            key=config.get('auth_password'),
-            os_options={'tenant_name': config.get('auth_tenant_name'),
-                        'region_name': config.get('auth_region')},
-            auth_version=('/v1.' in config.get('auth_url') and '1.0' or '2.0'))
+        # All authentication data bar the password so we can log them.
+        auth = dict(authurl=config.get('auth_url'),
+                    user=config.get('auth_user'),
+                    os_options={'tenant_name': config.get('auth_tenant_name'),
+                                'region_name': config.get('auth_region')},
+                    auth_version=('/v1.' in config.get('auth_url') and '1.0'
+                                  or '2.0'))
+        self.client = client.Connection(key=config.get('auth_password'),
+                                        **auth)
         try:
+            # Creating a container is idempotent and can be repeated
             self._create_container(self.container_id)
         except client.ClientException:
+            # This is the first connection attempt in the object life
             logging.exception('Unable to connect to swift')
-            raise DataStoreException("Missing or invalid authentication info.")
+            raise DataStoreException(
+                "Invalid authentication info: {}".format(auth))
         self.change_visibility(public=self.public)
         self.container_url = "{}/{}".format(self.client.url, self.container_id)
 
     def list_files(self):
         self.ensure_swift_client()
-        _, objects = self.client.get_container(self.container_id)
+        try:
+            _, objects = self.client.get_container(self.container_id)
+        except client.ClientException as e:
+            raise DataStoreException("Failed to get container: {}".format(e))
         return [x['name'] for x in objects]
 
     def put_file(self, filename, contents, content_type=None):
@@ -86,10 +113,9 @@
             self.client.put_object(self.container_id, obj=name,
                                    contents=contents,
                                    content_type=content_type)
-        except ClientException as e:
+        except client.ClientException as e:
             raise DataStoreException(
-                "Failed to upload file: {}, Error: {}".format(filename, e)
-            )
+                "Failed to upload file: {}, Error: {}".format(filename, e))
 
         return self.file_path(filename)
 
@@ -100,10 +126,9 @@
         contents = None
         try:
             hdr, contents = self.client.get_object(self.container_id, obj=name)
-        except ClientException as e:
+        except client.ClientException as e:
             raise DataStoreException(
-                "Failed to get file: {}, Error: {}".format(filename, e)
-            )
+                "Failed to get file: {}, Error: {}".format(filename, e))
         return contents
 
     def change_visibility(self, public=False):
@@ -121,40 +146,44 @@
         name = self.file_name(filename)
         try:
             self.client.delete_object(self.container_id, obj=name)
-        except ClientException as e:
+        except client.ClientException as e:
             raise DataStoreException(
-                "Failed to delete file: {}, Error: {}".format(filename, e)
-            )
+                "Failed to delete file: {}, Error: {}".format(filename, e))
 
     def clear(self):
+        files = self.list_files()
+        for fname in files:
+            self.delete_file(fname)
+
+    def delete(self, recursive=False):
         self.ensure_swift_client()
+        if recursive:
+            self.clear()
         try:
-            files = self.client.get_container(self.container_id)[1]
-
-            for f in files:
+            attempts = 0
+            backoff = self.starting_backoff
+            while attempts < self.retries:
                 try:
-                    self.client.delete_object(self.container_id,
-                                              obj=f['name'])
-                except ClientException as e:
-                    raise DataStoreException(
-                        "Failed to delete file: {}".format(e)
-                    )
-        except ClientException as e:
-            raise DataStoreException(
-                "Failed to get container: {}".format(e)
-            )
-
-    def delete(self, recursive=False):
-        self.ensure_swift_client()
-        try:
-            if recursive:
-                self.clear()
-
-            self.client.delete_container(self.container_id)
-        except ClientException as e:
-            raise DataStoreException(
-                "Failed to delete container: {}".format(e)
-            )
+                    self.client.delete_container(self.container_id)
+                    break
+                except client.ClientException as e:
+                    if e.http_status == 409:
+                        # The container is not empty yet
+                        pass
+                    elif attempts > 0 and e.http_status == 404:
+                        # The deletion succeeded while we slept, we're done,
+                        # the container is gone
+                        break
+                    else:
+                        msg = 'Failed to delete container (attempts: {}): {}'
+                        raise DataStoreException(msg.format(attempts, e))
+                # Take a nap before retrying
+                time.sleep(backoff)
+                backoff = min(backoff * 2, self.max_backoff)
+                attempts += 1
+        except client.ClientException as e:
+            raise DataStoreException(
+                "Failed to delete container: {}".format(e))
 
     def file_name(self, filename):
         """Returns the file name that is used inside the data store.
@@ -162,7 +191,7 @@
         :param filename: The file name used to build the object name inside the
             container.
         """
-        return os.path.basename(filename)
+        return filename
 
     def file_path(self, filename):
         name = self.file_name(filename)

=== modified file 'ci-utils/ci_utils/testing/fixtures.py'
--- ci-utils/ci_utils/testing/fixtures.py	2014-10-29 11:42:52 +0000
+++ ci-utils/ci_utils/testing/fixtures.py	2014-11-20 08:47:10 +0000
@@ -89,7 +89,12 @@
     def delete(self, recursive=False):
         if recursive:
             self.clear()
-        os.rmdir(self.container_id)
+        try:
+            os.rmdir(self.container_id)
+        except OSError as e:
+            if e.errno == errno.ENOTEMPTY:
+                raise data_store.DataStoreException(
+                    "Failed to delete container: {}".format(e))
 
     def file_path(self, filename):
         return '{}{}/{}'.format('file://', self.container_id, filename)

=== modified file 'ci-utils/ci_utils/tests/test_data_store.py'
--- ci-utils/ci_utils/tests/test_data_store.py	2014-10-07 10:04:01 +0000
+++ ci-utils/ci_utils/tests/test_data_store.py	2014-11-20 08:47:10 +0000
@@ -13,9 +13,14 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import os
 import unittest
 
 
+from ucitests import fixtures
+from swiftclient import client
+
+
 from ci_utils import data_store
 
 
@@ -35,23 +40,64 @@
             data_store.DataStore.validate_auth_config({})
 
 
-class TestDataStoreFileName(unittest.TestCase):
-
-    def setUp(self):
-        super(TestDataStoreFileName, self).setUp()
-        # We won't use any method that requires the auth config so we don't
-        # provide any.
-        self.ds = data_store.DataStore('foo', None)
-
-    def test_abspath(self):
-        self.assertEquals('foo', self.ds.file_name('/baz/bar/foo'))
-
-    def test_relpath(self):
-        self.assertEquals('foo', self.ds.file_name('../baz/bar/foo'))
-
-    def test_basename(self):
-        self.assertEquals('foo', self.ds.file_name('foo'))
-
-
-if __name__ == "__main__":
-    unittest.main()
+class DeletingContainerClient(object):
+    """Fake swift client to emulate scenarios for container deletion.
+
+    This is injected into a DataStore object to execute a scenario defined by a
+    list of calls.
+    """
+
+    def __init__(self, calls):
+        # Reverse the list order so we can pop() calls
+        self.calls = list(reversed(calls))
+
+    def delete_container(self, container, *args, **kwargs):
+        call = self.calls.pop()
+        return call(container, *args, **kwargs)
+
+
+class DeletingContainerDataStore(data_store.DataStore):
+    """An instrumented DataStore to emulate errors in container deletion."""
+
+    def __init__(self, component, calls):
+        # We don't use the config so we inject a fake but valid one
+        auth_config = {'auth_url': 'http://example.com',
+                       'auth_user': 'user',
+                       'auth_password': 'pass',
+                       'auth_tenant_name': 'tenant',
+                       'auth_region': 'region',}
+        super(DeletingContainerDataStore, self).__init__(
+            component, auth_config,
+            # We don't want to wait
+            starting_backoff=0, max_backoff=0)
+        self.client = DeletingContainerClient(calls)
+
+
+class TestDataStoreDelete(unittest.TestCase):
+    """Test the container deletion.
+
+    We don't have a swift server to inject errors into so we're faking that
+    part to exercise the error handling in the DataStore.delete()
+    implementation.
+    """
+    def raise_409(self, container, *args, **kwargs):
+        raise client.ClientException('409', http_status=409)
+
+    def raise_404(self, container, *args, **kwargs):
+        raise client.ClientException('404', http_status=404)
+
+    def test_delete_retried_on_409(self):
+        calls = [self.raise_409,
+                 lambda container, *args, **kwargs: None]
+        ds = DeletingContainerDataStore('ignored', calls)
+        ds.delete()
+        # All the calls have been issued
+        self.assertEqual([], ds.client.calls)
+
+    def test_delete_retried_once_then_succeeds(self):
+        calls = [self.raise_409,
+                 self.raise_404]
+        ds = DeletingContainerDataStore('ignored', calls)
+        ds.delete()
+        # All the calls have been issued
+        self.assertEqual([], ds.client.calls)

=== modified file 'ci-utils/ci_utils/tests/test_fixtures.py'
--- ci-utils/ci_utils/tests/test_fixtures.py	2014-10-29 13:55:35 +0000
+++ ci-utils/ci_utils/tests/test_fixtures.py	2014-11-20 08:47:10 +0000
@@ -19,6 +19,7 @@
 
 from ucitests import fixtures as uci_fixtures
 
+from ci_utils import data_store
 from ci_utils.testing import fixtures
 
 
@@ -91,11 +92,10 @@
     def test_delete_not_empty(self):
         self.assertTrue(os.path.exists(self.ds_path))
         self.ds.put_file('foo', 'foo content\n')
-        with self.assertRaises(OSError) as cm:
+        with self.assertRaises(data_store.DataStoreException) as cm:
             self.ds.delete()
-        # FIXME: Ideally the fake data store and the real one should throw the
-        # same exception in this case, not a big deal -- vila 2014-10-29
-        self.assertEqual(errno.ENOTEMPTY, cm.exception.args[0])
+        self.assertTrue(cm.exception.args[0].endswith("'{}'".format(
+            self.ds_path)))
 
 
 class TestUsingFakeDataStore(unittest.TestCase):

=== modified file 'tests/test_data_store.py'
--- tests/test_data_store.py	2014-10-08 09:54:56 +0000
+++ tests/test_data_store.py	2014-11-20 08:47:10 +0000
@@ -59,7 +59,6 @@
         """ Test adding a file. """
 
         self.ds.put_file(self.filename, self.contents)
-        self.addCleanup(self.ds.delete_file, self.filename)
         files = self.ds.list_files()
         self.assertEqual([self.filename], files)
 
@@ -67,18 +66,20 @@
         """ Test adding a file has correct contents. """
 
         self.ds.put_file(self.filename, self.contents)
-        self.addCleanup(self.ds.delete_file, self.filename)
         contents = self.ds.get_file(self.filename)
 
         self.assertEqual(contents, self.contents)
 
-    def test_public_container(self):
-        """ Test accesssing a public container. """
+    def test_not_public_fails(self):
+        link = self.ds.put_file(self.filename, self.contents)
+        self.addCleanup(self.ds.delete_file, self.filename)
+        with self.assertRaises(urllib2.HTTPError) as cm:
+            urllib2.urlopen(link)
+        self.assertEqual(401, cm.exception.code)
 
+    def test_public_succeeds(self):
         self.ds.put_file(self.filename, self.contents)
-        self.addCleanup(self.ds.delete_file, self.filename)
         self.ds.change_visibility(public=True)
-
         response = urllib2.urlopen(self.ds.file_path(self.filename))
         self.assertEqual(response.getcode(), 200, response.read())
 
@@ -93,18 +94,6 @@
         files = self.ds.list_files()
         self.assertEqual([], files)
 
-    def test_put_file_link(self):
-        """ Test put file returns a valid link. """
-
-        link = self.ds.put_file(self.filename, self.contents)
-        self.addCleanup(self.ds.delete_file, self.filename)
-        with self.assertRaises(urllib2.HTTPError) as cm:
-            resp = urllib2.urlopen(link)
-        self.assertEqual(401, cm.exception.code)
-        self.ds.change_visibility(public=True)
-        resp = urllib2.urlopen(link)
-        self.assertEqual(200, resp.getcode(), resp.read())
-
 
 class TestDataStoreBadConfigs(unittest.TestCase):
 
@@ -132,6 +121,9 @@
             ds = data_store.DataStore("test", auth_config=conf)
             ds.ensure_swift_client()
 
+    # FIXME: This needs log_on_failure from
+    # test_runner/tstrun/tests/__init__.py. The needed refactoring would make
+    # the current MP too big -- vila 2014-11-19
     def test_bad_region(self):
         conf = self.get_config()
         conf['auth_region'] = "I don't exist"