Fix cache not handling backend failures

1) caching_iter doesn't handle backend exceptions:

caching_iter assumes any exception that occurs is the result of being
unable to cache.  Hence the IOError raised from size_checked_iter, which
indicates a problem with the backend, means the caching_iter will
continuing trying to serve non-existent data.  The exception was not
been re-raised in this case, making wsgi keep the connection open and
clients stuck forever waiting for more data.

Raising a GlanceException in size_checked_iter rather than an IOError
allows caching_iter to distinguish between a problem fetching data, and
a problem writing to the cache.

2) Checksum verification happens after cache commit rather than before:

This block was outside the context manager block which meant the
GlanceException was not caught by open_for_write and the rollback didn't
happen.  This resulted in an error been logged, but the bad image still
placed in and subsequently served from the cache.

Also:

* Fix test_gate_caching_iter_bad_checksum - the loop to consume the
  iterator in was in a subroutine that never got called.

* Move test_gate_caching_iter_(good|bad)_checksum into
  ImageCacheTestCase to excercise both the sql and xattr drivers.

* Remove invalid registry_host/registry_port params from
  TestImageCacheXattr/TestImageCacheSqlite setup which caused a failure
  when testing the file on it's own using nosetests.

Fixes bug 1045792

Change-Id: I8aedec347e7f50566c44c5b6c6db424573c5ebaf

glance/api/common.py

@@ -15,6 +15,7 @@
 
 import errno
 
+from glance.common import exception
 from glance.openstack.common import log as logging
 
 LOG = logging.getLogger(__name__)
@@ -49,7 +50,7 @@ def size_checked_iter(response, image_meta, expected_size, image_iter,
                 "disconnected after writing only %(bytes_written)d "
                 "bytes") % locals()
         LOG.error(msg)
-        raise IOError(errno.EPIPE, _("Corrupt image download for "
+        raise exception.GlanceException(_("Corrupt image download for "
                                           "image %(image_id)s") % locals())
 
 

glance/image_cache/__init__.py

@@ -237,16 +237,21 @@ class ImageCache(object):
                             yield chunk
                     cache_file.flush()
 
-                if image_checksum and \
+                    if (image_checksum and
-                        image_checksum != current_checksum.hexdigest():
+                            image_checksum != current_checksum.hexdigest()):
-                    msg = _("Checksum verification failed.  Aborted caching "
+                        msg = _("Checksum verification failed. Aborted "
-                            "of image %s." % image_id)
+                                "caching of image '%s'." % image_id)
                         raise exception.GlanceException(msg)
 
-            except Exception:
+            except exception.GlanceException as e:
+                # image_iter has given us bad, (size_checked_iter has found a
+                # bad length), or corrupt data (checksum is wrong).
+                LOG.exception(e)
+                raise
+            except Exception as e:
                 LOG.exception(_("Exception encountered while tee'ing "
-                                "image '%s' into cache. Continuing "
+                                "image '%s' into cache: %s. Continuing "
-                                "with response.") % image_id)
+                                "with response.") % (image_id, e))
 
             # NOTE(markwash): continue responding even if caching failed
             for chunk in image_iter:

glance/tests/unit/test_image_cache.py

@@ -24,6 +24,7 @@ import StringIO
 
 import stubout
 
+from glance.common import exception
 from glance.common import utils
 from glance import image_cache
 #NOTE(bcwaldon): This is imported to load the registry config options
@@ -319,6 +320,29 @@ class ImageCacheTestCase(object):
         self.assertFalse(os.path.exists(incomplete_file_path))
         self.assertFalse(os.path.exists(invalid_file_path))
 
+    def test_caching_iterator_handles_backend_failure(self):
+        """
+        Test that when the backend fails, caching_iter does not continue trying
+        to consume data, and rolls back the cache.
+        """
+        def faulty_backend():
+            data = ['a', 'b', 'c', 'Fail', 'd', 'e', 'f']
+            for d in data:
+                if d == 'Fail':
+                    raise exception.GlanceException('Backend failure')
+                yield d
+
+        def consume(image_id):
+            caching_iter = self.cache.get_caching_iter(image_id, None,
+                                                       faulty_backend())
+            # excercise the caching_iter
+            list(caching_iter)
+
+        image_id = '1'
+        self.assertRaises(exception.GlanceException, consume, image_id)
+        # make sure bad image was not cached
+        self.assertFalse(self.cache.is_cached(image_id))
+
     def test_caching_iterator_falloffend(self):
         """
         Test to see if the caching iterator interacts properly with the driver
@@ -347,6 +371,36 @@ class ImageCacheTestCase(object):
         self.assertFalse(os.path.exists(incomplete_file_path))
         self.assertTrue(os.path.exists(invalid_file_path))
 
+    def test_gate_caching_iter_good_checksum(self):
+        image = "12345678990abcdefghijklmnop"
+        image_id = 123
+
+        md5 = hashlib.md5()
+        md5.update(image)
+        checksum = md5.hexdigest()
+
+        cache = image_cache.ImageCache()
+        img_iter = cache.get_caching_iter(image_id, checksum, image)
+        for chunk in img_iter:
+            pass
+        # checksum is valid, fake image should be cached:
+        self.assertTrue(cache.is_cached(image_id))
+
+    def test_gate_caching_iter_bad_checksum(self):
+        image = "12345678990abcdefghijklmnop"
+        image_id = 123
+        checksum = "foobar"  # bad.
+
+        cache = image_cache.ImageCache()
+        img_iter = cache.get_caching_iter(image_id, checksum, image)
+
+        def reader():
+            for chunk in img_iter:
+                pass
+        self.assertRaises(exception.GlanceException, reader)
+        # checksum is invalid, caching will fail:
+        self.assertFalse(cache.is_cached(image_id))
+
 
 class TestImageCacheXattr(test_utils.BaseTestCase,
                           ImageCacheTestCase):
@@ -381,9 +435,7 @@ class TestImageCacheXattr(test_utils.BaseTestCase,
         self.disabled = False
         self.config(image_cache_dir=self.cache_dir,
                     image_cache_driver='xattr',
-                    image_cache_max_size=1024 * 5,
+                    image_cache_max_size=1024 * 5)
-                    registry_host='127.0.0.1',
-                    registry_port=9191)
         self.cache = image_cache.ImageCache()
 
         if not xattr_writes_supported(self.cache_dir):
@@ -428,9 +480,7 @@ class TestImageCacheSqlite(test_utils.BaseTestCase,
                                       random.randint(0, 1000000))
         self.config(image_cache_dir=self.cache_dir,
                     image_cache_driver='sqlite',
-                    image_cache_max_size=1024 * 5,
+                    image_cache_max_size=1024 * 5)
-                    registry_host='127.0.0.1',
-                    registry_port=9191)
         self.cache = image_cache.ImageCache()
 
     def tearDown(self):
@@ -438,35 +488,6 @@ class TestImageCacheSqlite(test_utils.BaseTestCase,
         if os.path.exists(self.cache_dir):
             shutil.rmtree(self.cache_dir)
 
-    def test_gate_caching_iter_good_checksum(self):
-        image = "12345678990abcdefghijklmnop"
-        image_id = 123
-
-        md5 = hashlib.md5()
-        md5.update(image)
-        checksum = md5.hexdigest()
-
-        cache = image_cache.ImageCache()
-        img_iter = cache.get_caching_iter(image_id, checksum, image)
-        for chunk in img_iter:
-            pass
-        # checksum is valid, fake image should be cached:
-        self.assertTrue(cache.is_cached(image_id))
-
-    def test_gate_caching_iter_bad_checksum(self):
-        image = "12345678990abcdefghijklmnop"
-        image_id = 123
-        checksum = "foobar"  # bad.
-
-        cache = image_cache.ImageCache()
-        img_iter = cache.get_caching_iter(image_id, checksum, image)
-
-        def reader():
-            for chunk in img_iter:
-                pass
-        # checksum is invalid, caching will fail:
-        self.assertFalse(cache.is_cached(image_id))
-
 
 class TestImageCacheNoDep(test_utils.BaseTestCase):