From f56cef93ea6e3746a17152bcd1850ccf4b3dad3d Mon Sep 17 00:00:00 2001 From: Mark Washenberger Date: Fri, 2 Mar 2012 15:57:55 -0500 Subject: [PATCH] Better glance exception handling - Adds a conversion step that turns glance exceptions into nova exceptions - Converts Invalid to HTTPBadRequest in /images and /images/detail, fixing bug 944846 - Makes stub glance client return glance exceptions instead of nova exceptions - Rebased off of http://review.openstack.org/4788 to pick up MissingCredentialError handling as well, and added a test - A few small, miscellaneous testing fixes for issues I noticed Change-Id: I88eebfe7a7ac21cc5cd84ad84d64b311ddccf91e --- nova/api/openstack/compute/images.py | 14 +++-- nova/image/glance.py | 59 +++++++++++++++---- .../api/openstack/compute/test_images.py | 25 +++++++- nova/tests/glance/stubs.py | 10 +++- nova/tests/image/test_glance.py | 37 +++++++++++- 5 files changed, 124 insertions(+), 21 deletions(-) diff --git a/nova/api/openstack/compute/images.py b/nova/api/openstack/compute/images.py index 1a957bc56a0a..eceee665179f 100644 --- a/nova/api/openstack/compute/images.py +++ b/nova/api/openstack/compute/images.py @@ -171,8 +171,11 @@ class Controller(wsgi.Controller): for key, val in page_params.iteritems(): params[key] = val - images = self._image_service.index(context, filters=filters, - **page_params) + try: + images = self._image_service.index(context, filters=filters, + **page_params) + except exception.Invalid as e: + raise webob.exc.HTTPBadRequest(explanation=str(e)) return self._view_builder.index(req, images) @wsgi.serializers(xml=ImagesTemplate) @@ -188,8 +191,11 @@ class Controller(wsgi.Controller): page_params = common.get_pagination_params(req) for key, val in page_params.iteritems(): params[key] = val - images = self._image_service.detail(context, filters=filters, - **page_params) + try: + images = self._image_service.detail(context, filters=filters, + **page_params) + except exception.Invalid as e: + raise webob.exc.HTTPBadRequest(explanation=str(e)) return self._view_builder.detail(req, images) diff --git a/nova/image/glance.py b/nova/image/glance.py index 5edab2655514..d924b2c410f6 100644 --- a/nova/image/glance.py +++ b/nova/image/glance.py @@ -23,6 +23,7 @@ import copy import datetime import json import random +import sys import time import urlparse @@ -202,7 +203,10 @@ class GlanceImageService(object): def _fetch_images(self, fetch_func, **kwargs): """Paginate through results from glance server""" - images = fetch_func(**kwargs) + try: + images = fetch_func(**kwargs) + except Exception: + _reraise_translated_exception() if not images: # break out of recursive loop to end pagination @@ -234,8 +238,8 @@ class GlanceImageService(object): try: image_meta = self._call_retry(context, 'get_image_meta', image_id) - except glance_exception.NotFound: - raise exception.ImageNotFound(image_id=image_id) + except Exception: + _reraise_translated_image_exception(image_id) if not self._is_image_available(context, image_meta): raise exception.ImageNotFound(image_id=image_id) @@ -256,8 +260,8 @@ class GlanceImageService(object): try: image_meta, image_chunks = self._call_retry(context, 'get_image', image_id) - except glance_exception.NotFound: - raise exception.ImageNotFound(image_id=image_id) + except Exception: + _reraise_translated_image_exception(image_id) for chunk in image_chunks: data.write(chunk) @@ -296,14 +300,11 @@ class GlanceImageService(object): # NOTE(vish): show is to check if image is available self.show(context, image_id) image_meta = self._translate_to_glance(image_meta) + client = self._get_client(context) try: - client = self._get_client(context) image_meta = client.update_image(image_id, image_meta, data) - except glance_exception.NotFound: - raise exception.ImageNotFound(image_id=image_id) - # NOTE(vish): this gets raised for public images - except glance_exception.MissingCredentialError: - raise exception.ImageNotAuthorized(image_id=image_id) + except Exception: + _reraise_translated_image_exception(image_id) base_image_meta = self._translate_from_glance(image_meta) return base_image_meta @@ -468,3 +469,39 @@ def _remove_read_only(image_meta): if attr in output: del output[attr] return output + + +def _reraise_translated_image_exception(image_id): + """Transform the exception for the image but keep its traceback intact.""" + exc_type, exc_value, exc_trace = sys.exc_info() + new_exc = _translate_image_exception(image_id, exc_type, exc_value) + raise new_exc, None, exc_trace + + +def _reraise_translated_exception(): + """Transform the exception but keep its traceback intact.""" + exc_type, exc_value, exc_trace = sys.exc_info() + new_exc = _translate_plain_exception(exc_type, exc_value) + raise new_exc, None, exc_trace + + +def _translate_image_exception(image_id, exc_type, exc_value): + if exc_type in (glance_exception.NotAuthorized, + glance_exception.MissingCredentialError): + return exception.ImageNotAuthorized(image_id=image_id) + if exc_type is glance_exception.NotFound: + return exception.ImageNotFound(image_id=image_id) + if exc_type is glance_exception.Invalid: + return exception.Invalid(exc_value) + return exc_value + + +def _translate_plain_exception(exc_type, exc_value): + if exc_type in (glance_exception.NotAuthorized, + glance_exception.MissingCredentialError): + return exception.NotAuthorized(exc_value) + if exc_type is glance_exception.NotFound: + return exception.NotFound(exc_value) + if exc_type is glance_exception.Invalid: + return exception.Invalid(exc_value) + return exc_value diff --git a/nova/tests/api/openstack/compute/test_images.py b/nova/tests/api/openstack/compute/test_images.py index 092485a10076..0bcae01ce330 100644 --- a/nova/tests/api/openstack/compute/test_images.py +++ b/nova/tests/api/openstack/compute/test_images.py @@ -25,6 +25,7 @@ import urlparse from lxml import etree import webob +from nova import exception from nova import flags from nova.api.openstack.compute import images from nova.api.openstack.compute.views import images as images_view @@ -903,10 +904,10 @@ class ImagesControllerTest(test.TestCase): request = fakes.HTTPRequest.blank('/v2/images?status=ACTIVE&' 'UNSUPPORTEDFILTER=testname') context = request.environ['nova.context'] - image_service.detail(context, filters=filters).AndReturn([]) + image_service.index(context, filters=filters).AndReturn([]) self.mox.ReplayAll() controller = images.Controller(image_service=image_service) - controller.detail(request) + controller.index(request) def test_image_no_filters(self): image_service = self.mox.CreateMockAnything() @@ -918,6 +919,16 @@ class ImagesControllerTest(test.TestCase): controller = images.Controller(image_service=image_service) controller.index(request) + def test_image_invalid_marker(self): + class InvalidImageService(object): + + def index(self, *args, **kwargs): + raise exception.Invalid('meow') + + request = fakes.HTTPRequest.blank('/v2/images?marker=invalid') + controller = images.Controller(image_service=InvalidImageService()) + self.assertRaises(webob.exc.HTTPBadRequest, controller.index, request) + def test_image_detail_filter_with_name(self): image_service = self.mox.CreateMockAnything() filters = {'name': 'testname'} @@ -1018,6 +1029,16 @@ class ImagesControllerTest(test.TestCase): controller = images.Controller(image_service=image_service) controller.detail(request) + def test_image_detail_invalid_marker(self): + class InvalidImageService(object): + + def detail(self, *args, **kwargs): + raise exception.Invalid('meow') + + request = fakes.HTTPRequest.blank('/v2/images?marker=invalid') + controller = images.Controller(image_service=InvalidImageService()) + self.assertRaises(webob.exc.HTTPBadRequest, controller.detail, request) + def test_generate_alternate_link(self): view = images_view.ViewBuilder() request = fakes.HTTPRequest.blank('/v2/fake/images/1') diff --git a/nova/tests/glance/stubs.py b/nova/tests/glance/stubs.py index 5c0f4f05e312..ccf431b69265 100644 --- a/nova/tests/glance/stubs.py +++ b/nova/tests/glance/stubs.py @@ -17,6 +17,8 @@ import copy import StringIO +from glance.common import exception as glance_exception + from nova import exception from nova.image import glance @@ -103,7 +105,7 @@ class StubGlanceClient(object): for image in self.images: if image['id'] == str(image_id): return image - raise exception.ImageNotFound(image_id=image_id) + raise glance_exception.NotFound() #TODO(bcwaldon): implement filters def get_images_detailed(self, filters=None, marker=None, limit=3): @@ -114,6 +116,8 @@ class StubGlanceClient(object): if image['id'] == str(marker): index += 1 break + else: + raise glance_exception.Invalid() return self.images[index:index + limit] @@ -143,11 +147,11 @@ class StubGlanceClient(object): metadata['id'] = str(metadata['id']) self.images[i].update(metadata) return self.images[i] - raise exception.ImageNotFound(image_id=image_id) + raise glance_exception.NotFound() def delete_image(self, image_id): for i, image in enumerate(self.images): if image['id'] == image_id: del self.images[i] return - raise exception.ImageNotFound(image_id=image_id) + raise glance_exception.NotFound() diff --git a/nova/tests/image/test_glance.py b/nova/tests/image/test_glance.py index 6d965756a909..6ed4c501f773 100644 --- a/nova/tests/image/test_glance.py +++ b/nova/tests/image/test_glance.py @@ -18,6 +18,8 @@ import datetime +import glance.common.exception as glance_exception + from nova.tests.api.openstack import fakes from nova import context from nova import exception @@ -181,7 +183,7 @@ class TestGlanceImageService(test.TestCase): image_id = self.service.create(self.context, fixture)['id'] self.assertNotEquals(None, image_id) - self.assertRaises(exception.NotFound, + self.assertRaises(exception.ImageNotFound, self.service.show, self.context, 'bad image id') @@ -260,6 +262,17 @@ class TestGlanceImageService(test.TestCase): self.assertDictMatch(meta, expected) i = i + 1 + def test_index_invalid_marker(self): + fixtures = [] + ids = [] + for i in range(10): + fixture = self._make_fixture(name='TestImage %d' % (i)) + fixtures.append(fixture) + ids.append(self.service.create(self.context, fixture)['id']) + + self.assertRaises(exception.Invalid, self.service.index, + self.context, marker='invalidmarker') + def test_index_private_image(self): fixture = self._make_fixture(name='test image') fixture['is_public'] = False @@ -354,6 +367,17 @@ class TestGlanceImageService(test.TestCase): self.assertDictMatch(meta, expected) i = i + 1 + def test_detail_invalid_marker(self): + fixtures = [] + ids = [] + for i in range(10): + fixture = self._make_fixture(name='TestImage %d' % (i)) + fixtures.append(fixture) + ids.append(self.service.create(self.context, fixture)['id']) + + self.assertRaises(exception.Invalid, self.service.detail, + self.context, marker='invalidmarker') + def test_update(self): fixture = self._make_fixture(name='test image') image_id = self.service.create(self.context, fixture)['id'] @@ -445,6 +469,17 @@ class TestGlanceImageService(test.TestCase): self.context, image_id) + def test_show_raises_on_missing_credential(self): + def raise_missing_credentials(*args, **kwargs): + raise glance_exception.MissingCredentialError() + + self.stubs.Set(glance_stubs.StubGlanceClient, 'get_image_meta', + raise_missing_credentials) + self.assertRaises(exception.ImageNotAuthorized, + self.service.show, + self.context, + 'test-image-id') + def test_detail_passes_through_to_client(self): fixture = self._make_fixture(name='image10', is_public=True) image_id = self.service.create(self.context, fixture)['id']