From 0551f0a9ff7b787fcb3e1b686b83e25f99cad874 Mon Sep 17 00:00:00 2001 From: Francois Deppierraz Date: Mon, 22 Oct 2018 15:33:25 +0200 Subject: [PATCH] cinder-volume: Stop masking IOError different than ENOSPC When glanceclient raises an IOError with a different errno than ENOSPC, cinder-volume silently masked it and continued its volume creation process. The result was volumes with invalid content being successfuly created. With the patch, an ImageDownloadFailed exception is raised in this case, which makes the volume creation process fail and gives enough information to operators for troubleshooting. As explained in detail below, this patch is a squash of four cherry picks to fix Bug #1799221. The cherry-picks are being squashed instead of done separately per normal cinder practice because they are dependent; this will make sure that one of them isn't further backported without the supporting patches. Here's the relationship between the four cherry-picks: (1) The purpose of this backport is to fix Bug #1799221, which was introduced in Pike. It is fixed in Stein and Rocky by Ic011fe30b4840e5098db1a594ea276ec98768bff (2) That change requires an exception introduced in Stein and backported to Rocky by If7c22ac4516f8c2a6ccd8bf6b6ed98409312b138 to fix Bug #1798147 (which defect is also present in Queens and Pike) (3) The change in (2) introduced Bug #1808443 which was fixed in Stein and Rocky by I6d8dedfd056add3414f8f4bf7f7279eae4763286 (4) The change in (2) also introduced Bug #1811184, which is fixed by I6d8dedfd056add3414f8f4bf7f7279eae4763286 in Stein and Rocky, and which adds a unit test for Bug #1798147. In short, in order to backport the fix for (1), we need to backport (2), but in order to backport (2) we need to follow up immediately with backports of (3) and (4) to fix the defects (2) introduces. The attentive reader will note that this patch smuggles in the fix for Bug #1798147. We could have left this out, but it's a very small isolated change, the defect is present in Queens and Pike (remember that Bug #1799221, the subject of this patch, was introduced in Pike), and it has a unit test (see (4), above). Finally, leaving out the fix for Bug #1798147 and backporting only the exception would still require backporting the fixes for Bug #1808443 and Bug #1811184, so it really would not simplify this patch. To summarize what's being included here: commit changeId fixed bug 805368e If7c22ac4516f8c2a6ccd8bf6b6ed98409312b138 Bug #1798147 9c696ce I2aa56da73660794c6dedcbb8a66e84bcec511a9c Bug #1808443 844b627 I6d8dedfd056add3414f8f4bf7f7279eae4763286 Bug #1811184 bf89f76 Ic011fe30b4840e5098db1a594ea276ec98768bff Bug #1799221 Change-Id: Ic011fe30b4840e5098db1a594ea276ec98768bff Closes-Bug: #1799221 (cherry picked from commit 864c074ff15db601c896dadc2842ae703861c0dd) (cherry picked from commit bf89f76fb1b7a52299c17467106018eae01608e8) Related-Bug: #1798147 (cherry picked from commit d3afc39467c6532344118e8e491409878da9c2dd) (cherry picked from commit 805368e902ada4c63e2e6e4d6692cf8cc2531a1e) Related-Bug: #1808443 (cherry picked from commit 80fdc0a71b55659d92efdda826807d60d8bd4ab9) (cherry picked from commit 9c696ce29f63275ec229a943a9b9bcf63f688f0c) Related-bug: #1811184 (cherry picked from commit 16b4346171d2027bb3c2630d5f28f6f002878870) (cherry picked from commit 844b627c38bb810abd3ef0b464577191f193ad7d) (cherry picked from commit 937af5be0e7c17b34860e25cc434a219d7143387) --- cinder/exception.py | 4 ++++ cinder/image/glance.py | 4 ++++ cinder/image/image_utils.py | 6 ++++++ cinder/tests/unit/image/test_glance.py | 15 ++++++++++++++ cinder/tests/unit/test_image_utils.py | 20 +++++++++++++++++++ ...ase-of-glance-errors-6cae19218249c3cf.yaml | 6 ++++++ 6 files changed, 55 insertions(+) create mode 100644 releasenotes/notes/bug-1799221-fix-truncated-volumes-in-case-of-glance-errors-6cae19218249c3cf.yaml diff --git a/cinder/exception.py b/cinder/exception.py index 33bddc3b188..feb05c35c90 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -716,6 +716,10 @@ class GlanceMetadataNotFound(NotFound): message = _("Glance metadata for volume/snapshot %(id)s cannot be found.") +class ImageDownloadFailed(CinderException): + message = _("Failed to download image %(image_href)s, reason: %(reason)s") + + class ExportFailure(Invalid): message = _("Failed to export for volume: %(reason)s") diff --git a/cinder/image/glance.py b/cinder/image/glance.py index ee4b2cc2ace..6792f401c86 100644 --- a/cinder/image/glance.py +++ b/cinder/image/glance.py @@ -335,6 +335,10 @@ class GlanceImageService(object): except Exception: _reraise_translated_image_exception(image_id) + if image_chunks is None: + raise exception.ImageDownloadFailed( + image_href=image_id, reason=_('image contains no data.')) + if not data: return image_chunks else: diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py index 3417196d380..da11a678f04 100644 --- a/cinder/image/image_utils.py +++ b/cinder/image/image_utils.py @@ -232,6 +232,12 @@ def fetch(context, image_service, image_id, path, _user_id, _project_id): raise exception.ImageTooBig(image_id=image_id, reason=reason) + reason = ("IOError: %(errno)s %(strerror)s" % + {'errno': e.errno, 'strerror': e.strerror}) + LOG.error(reason) + raise exception.ImageDownloadFailed(image_href=image_id, + reason=reason) + duration = timeutils.delta_seconds(start_time, timeutils.utcnow()) # NOTE(jdg): use a default of 1, mostly for unit test, but in diff --git a/cinder/tests/unit/image/test_glance.py b/cinder/tests/unit/image/test_glance.py index 0724450a659..975773f5613 100644 --- a/cinder/tests/unit/image/test_glance.py +++ b/cinder/tests/unit/image/test_glance.py @@ -16,6 +16,7 @@ import datetime import itertools +import six import ddt import glanceclient.exc @@ -556,6 +557,20 @@ class TestGlanceImageService(test.TestCase): self.flags(glance_num_retries=1) service.download(self.context, image_id, writer) + def test_download_no_data(self): + class MyGlanceStubClient(glance_stubs.StubGlanceClient): + """Returns None instead of an iterator.""" + def data(self, image_id): + return None + + client = MyGlanceStubClient() + service = self._create_image_service(client) + image_id = 'fake-image-uuid' + e = self.assertRaises(exception.ImageDownloadFailed, service.download, + self.context, image_id) + self.assertIn('image contains no data', six.text_type(e)) + self.assertIn(image_id, six.text_type(e)) + def test_client_forbidden_converts_to_imagenotauthed(self): class MyGlanceStubClient(glance_stubs.StubGlanceClient): """A client that raises a Forbidden exception.""" diff --git a/cinder/tests/unit/test_image_utils.py b/cinder/tests/unit/test_image_utils.py index 78e99727676..186df1cceab 100644 --- a/cinder/tests/unit/test_image_utils.py +++ b/cinder/tests/unit/test_image_utils.py @@ -315,6 +315,26 @@ class TestFetch(test.TestCase): context, image_service, image_id, path, _user_id, _project_id) + def test_fetch_ioerror(self): + context = mock.sentinel.context + image_service = mock.Mock() + image_id = mock.sentinel.image_id + e = IOError() + e.errno = errno.ECONNRESET + e.strerror = 'Some descriptive message' + image_service.download.side_effect = e + path = '/test_path' + _user_id = mock.sentinel._user_id + _project_id = mock.sentinel._project_id + + with mock.patch('cinder.image.image_utils.open', + new=mock.mock_open(), create=True): + self.assertRaisesRegex(exception.ImageDownloadFailed, + e.strerror, + image_utils.fetch, + context, image_service, image_id, path, + _user_id, _project_id) + class TestVerifyImage(test.TestCase): @mock.patch('cinder.image.image_utils.qemu_img_info') diff --git a/releasenotes/notes/bug-1799221-fix-truncated-volumes-in-case-of-glance-errors-6cae19218249c3cf.yaml b/releasenotes/notes/bug-1799221-fix-truncated-volumes-in-case-of-glance-errors-6cae19218249c3cf.yaml new file mode 100644 index 00000000000..d38a4cc8966 --- /dev/null +++ b/releasenotes/notes/bug-1799221-fix-truncated-volumes-in-case-of-glance-errors-6cae19218249c3cf.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixed a bug which could create volumes with invalid content in case of + unhandled errors from glance client + (Bug `#1799221 `_).