From 95e00c9247d5c56c1184d788ca8c1f9b165a25ba Mon Sep 17 00:00:00 2001 From: Paul Bourke Date: Mon, 3 Sep 2012 11:17:54 +0100 Subject: [PATCH] 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 | 5 +- glance/image_cache/__init__.py | 21 ++++--- glance/tests/unit/test_image_cache.py | 91 ++++++++++++++++----------- 3 files changed, 72 insertions(+), 45 deletions(-) diff --git a/glance/api/common.py b/glance/api/common.py index e026b691b5..0841d12f18 100644 --- a/glance/api/common.py +++ b/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,8 +50,8 @@ 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 " - "image %(image_id)s") % locals()) + raise exception.GlanceException(_("Corrupt image download for " + "image %(image_id)s") % locals()) def image_send_notification(bytes_written, expected_size, image_meta, request, diff --git a/glance/image_cache/__init__.py b/glance/image_cache/__init__.py index 270343062c..47712d3693 100644 --- a/glance/image_cache/__init__.py +++ b/glance/image_cache/__init__.py @@ -237,16 +237,21 @@ class ImageCache(object): yield chunk cache_file.flush() - if image_checksum and \ - image_checksum != current_checksum.hexdigest(): - msg = _("Checksum verification failed. Aborted caching " - "of image %s." % image_id) - raise exception.GlanceException(msg) + if (image_checksum and + image_checksum != current_checksum.hexdigest()): + msg = _("Checksum verification failed. Aborted " + "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 " - "with response.") % image_id) + "image '%s' into cache: %s. Continuing " + "with response.") % (image_id, e)) # NOTE(markwash): continue responding even if caching failed for chunk in image_iter: diff --git a/glance/tests/unit/test_image_cache.py b/glance/tests/unit/test_image_cache.py index ac992de002..eca2a03401 100644 --- a/glance/tests/unit/test_image_cache.py +++ b/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, - registry_host='127.0.0.1', - registry_port=9191) + image_cache_max_size=1024 * 5) 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, - registry_host='127.0.0.1', - registry_port=9191) + image_cache_max_size=1024 * 5) 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):