diff --git a/glance/api/authorization.py b/glance/api/authorization.py index 3a1045af53..0b66af62b8 100644 --- a/glance/api/authorization.py +++ b/glance/api/authorization.py @@ -147,10 +147,10 @@ class ImageMemberRepoProxy(glance.domain.proxy.Repo): raise exception.Forbidden(message % self.image.image_id) - def save(self, image_member): + def save(self, image_member, from_state=None): if (self.context.is_admin or self.context.owner == image_member.member_id): - self.member_repo.save(image_member) + self.member_repo.save(image_member, from_state=from_state) else: message = _("You cannot update image member %s") raise exception.Forbidden(message % image_member.member_id) diff --git a/glance/api/policy.py b/glance/api/policy.py index 22a561dad5..1838395f82 100644 --- a/glance/api/policy.py +++ b/glance/api/policy.py @@ -182,9 +182,9 @@ class ImageRepoProxy(glance.domain.proxy.Repo): self.policy.enforce(self.context, 'get_images', {}) return super(ImageRepoProxy, self).list(*args, **kwargs) - def save(self, image): + def save(self, image, from_state=None): self.policy.enforce(self.context, 'modify_image', {}) - return super(ImageRepoProxy, self).save(image) + return super(ImageRepoProxy, self).save(image, from_state=from_state) def add(self, image): self.policy.enforce(self.context, 'add_image', {}) @@ -283,9 +283,9 @@ class ImageMemberRepoProxy(glance.domain.proxy.Repo): self.policy.enforce(self.context, 'get_member', {}) return self.member_repo.get(member_id) - def save(self, member): + def save(self, member, from_state=None): self.policy.enforce(self.context, 'modify_member', {}) - self.member_repo.save(member) + self.member_repo.save(member, from_state=from_state) def list(self, *args, **kwargs): self.policy.enforce(self.context, 'get_members', {}) diff --git a/glance/api/v1/upload_utils.py b/glance/api/v1/upload_utils.py index 8fcf76685d..f7e980bb6c 100644 --- a/glance/api/v1/upload_utils.py +++ b/glance/api/v1/upload_utils.py @@ -146,14 +146,21 @@ def upload_data_to_store(req, image_meta, image_data, store, notifier): update_data = {'checksum': checksum, 'size': size} try: - image_meta = registry.update_image_metadata(req.context, - image_id, - update_data, - from_state='saving') - - except exception.NotFound as e: - msg = _("Image %s could not be found after upload. The image may " - "have been deleted during the upload.") % image_id + try: + state = 'saving' + image_meta = registry.update_image_metadata(req.context, + image_id, + update_data, + from_state=state) + except exception.Duplicate: + image = registry.get_image_metadata(req.context, image_id) + if image['status'] == 'deleted': + raise exception.NotFound() + else: + raise + except exception.NotFound: + msg = _("Image %s could not be found after upload. The image may" + " have been deleted during the upload.") % image_id LOG.info(msg) # NOTE(jculp): we need to clean up the datastore if an image diff --git a/glance/api/v2/image_data.py b/glance/api/v2/image_data.py index 1bff3d7c5c..6253608264 100644 --- a/glance/api/v2/image_data.py +++ b/glance/api/v2/image_data.py @@ -22,6 +22,7 @@ from glance.common import wsgi import glance.db import glance.gateway import glance.notifier +from glance.openstack.common import excutils import glance.openstack.common.log as logging import glance.store @@ -66,13 +67,12 @@ class ImageDataController(object): try: image_repo.save(image) image.set_data(data, size) - image_repo.save(image) - except exception.NotFound as e: - msg = (_("Image %(id)s could not be found after upload." - "The image may have been deleted during the upload: " - "%(error)s Cleaning up the chunks uploaded") % - {'id': image_id, - 'error': e}) + image_repo.save(image, from_state='saving') + except (exception.NotFound, exception.Conflict): + msg = (_("Image %s could not be found after upload. " + "The image may have been deleted during the " + "upload, cleaning up the chunks uploaded.") % + image_id) LOG.warn(msg) # NOTE(sridevi): Cleaning up the uploaded chunks. try: @@ -131,6 +131,10 @@ class ImageDataController(object): raise webob.exc.HTTPServiceUnavailable(explanation=msg, request=req) + except webob.exc.HTTPGone as e: + with excutils.save_and_reraise_exception(): + LOG.error(_("Failed to upload image data due to HTTP error")) + except webob.exc.HTTPError as e: LOG.error(_("Failed to upload image data due to HTTP error")) self._restore(image_repo, image) diff --git a/glance/db/__init__.py b/glance/db/__init__.py index a59447de02..379cf6f698 100644 --- a/glance/db/__init__.py +++ b/glance/db/__init__.py @@ -162,7 +162,7 @@ class ImageRepo(object): image.created_at = new_values['created_at'] image.updated_at = new_values['updated_at'] - def save(self, image): + def save(self, image, from_state=None): image_values = self._format_image_to_db(image) if image_values['size'] > CONF.image_size_cap: raise exception.ImageSizeLimitExceeded @@ -170,7 +170,8 @@ class ImageRepo(object): new_values = self.db_api.image_update(self.context, image.image_id, image_values, - purge_props=True) + purge_props=True, + from_state=from_state) except (exception.NotFound, exception.Forbidden): msg = _("No image found with ID %s") % image.image_id raise exception.NotFound(msg) @@ -263,7 +264,7 @@ class ImageMemberRepo(object): msg = _("The specified member %s could not be found") raise exception.NotFound(msg % image_member.id) - def save(self, image_member): + def save(self, image_member, from_state=None): image_member_values = self._format_image_member_to_db(image_member) try: new_values = self.db_api.image_member_update(self.context, diff --git a/glance/domain/proxy.py b/glance/domain/proxy.py index 89f138c4f8..b27b448c18 100644 --- a/glance/domain/proxy.py +++ b/glance/domain/proxy.py @@ -94,9 +94,9 @@ class Repo(object): result = self.base.add(base_item) return self.helper.proxy(result) - def save(self, item): + def save(self, item, from_state=None): base_item = self.helper.unproxy(item) - result = self.base.save(base_item) + result = self.base.save(base_item, from_state=from_state) return self.helper.proxy(result) def remove(self, item): diff --git a/glance/notifier.py b/glance/notifier.py index 1eaadbf94d..1d27c07d5b 100644 --- a/glance/notifier.py +++ b/glance/notifier.py @@ -178,8 +178,8 @@ class ImageRepoProxy(glance.domain.proxy.Repo): item_proxy_class=ImageProxy, item_proxy_kwargs=proxy_kwargs) - def save(self, image): - super(ImageRepoProxy, self).save(image) + def save(self, image, from_state=None): + super(ImageRepoProxy, self).save(image, from_state=from_state) self.notifier.info('image.update', format_image_notification(image)) diff --git a/glance/quota/__init__.py b/glance/quota/__init__.py index 0fcc7926b6..a8babe1642 100644 --- a/glance/quota/__init__.py +++ b/glance/quota/__init__.py @@ -96,9 +96,9 @@ class ImageRepoProxy(glance.domain.proxy.Repo): raise exception.ImagePropertyLimitExceeded(attempted=attempted, maximum=maximum) - def save(self, image): + def save(self, image, from_state=None): self._enforce_image_property_quota(image) - super(ImageRepoProxy, self).save(image) + return super(ImageRepoProxy, self).save(image, from_state=from_state) def add(self, image): self._enforce_image_property_quota(image) diff --git a/glance/store/__init__.py b/glance/store/__init__.py index 273b7c7adc..ae3b4c83af 100644 --- a/glance/store/__init__.py +++ b/glance/store/__init__.py @@ -446,7 +446,7 @@ class ImageRepoProxy(glance.domain.proxy.Repo): self._set_acls(image) return result - def save(self, image): + def save(self, image, from_state=None): result = super(ImageRepoProxy, self).save(image) self._set_acls(image) return result diff --git a/glance/tests/unit/test_domain_proxy.py b/glance/tests/unit/test_domain_proxy.py index 86684fc05d..4260b2f8e4 100644 --- a/glance/tests/unit/test_domain_proxy.py +++ b/glance/tests/unit/test_domain_proxy.py @@ -74,7 +74,7 @@ class TestProxyRepoPlain(test_utils.BaseTestCase): self._test_method('add', 'snuff', 'enough') def test_save(self): - self._test_method('save', 'snuff', 'enough') + self._test_method('save', 'snuff', 'enough', from_state=None) def test_remove(self): self._test_method('add', None, 'flying') @@ -121,14 +121,14 @@ class TestProxyRepoWrapping(test_utils.BaseTestCase): self.assertEqual(results[i].args, tuple()) self.assertEqual(results[i].kwargs, {'a': 1}) - def _test_method_with_proxied_argument(self, name, result): + def _test_method_with_proxied_argument(self, name, result, **kwargs): self.fake_repo.result = result item = FakeProxy('snoop') method = getattr(self.proxy_repo, name) proxy_result = method(item) - self.assertEqual(self.fake_repo.args, ('snoop',)) - self.assertEqual(self.fake_repo.kwargs, {}) + self.assertEqual(('snoop',), self.fake_repo.args) + self.assertEqual(kwargs, self.fake_repo.kwargs) if result is None: self.assertTrue(proxy_result is None) @@ -145,10 +145,12 @@ class TestProxyRepoWrapping(test_utils.BaseTestCase): self._test_method_with_proxied_argument('add', None) def test_save(self): - self._test_method_with_proxied_argument('save', 'dog') + self._test_method_with_proxied_argument('save', 'dog', + from_state=None) def test_save_with_no_result(self): - self._test_method_with_proxied_argument('save', None) + self._test_method_with_proxied_argument('save', None, + from_state=None) def test_remove(self): self._test_method_with_proxied_argument('remove', 'dog') diff --git a/glance/tests/unit/test_policy.py b/glance/tests/unit/test_policy.py index 6306f1bb04..0b51e8e80d 100644 --- a/glance/tests/unit/test_policy.py +++ b/glance/tests/unit/test_policy.py @@ -69,7 +69,7 @@ class MemberRepoStub(object): def get(self, *args, **kwargs): return 'member_repo_get' - def save(self, image_member): + def save(self, image_member, from_state=None): image_member.output = 'member_repo_save' def list(self, *args, **kwargs): diff --git a/glance/tests/unit/test_quota.py b/glance/tests/unit/test_quota.py index 504cefd8b9..a66b0acf51 100644 --- a/glance/tests/unit/test_quota.py +++ b/glance/tests/unit/test_quota.py @@ -290,7 +290,8 @@ class TestImagePropertyQuotas(test_utils.BaseTestCase): self.image.extra_properties = {'foo': 'bar'} self.image_repo_proxy.save(self.image) - self.image_repo_mock.save.assert_called_once_with(self.base_image) + self.image_repo_mock.save.assert_called_once_with(self.base_image, + from_state=None) def test_save_image_too_many_image_properties(self): self.config(image_property_quota=1) @@ -306,7 +307,8 @@ class TestImagePropertyQuotas(test_utils.BaseTestCase): self.image.extra_properties = {'foo': 'bar'} self.image_repo_proxy.save(self.image) - self.image_repo_mock.save.assert_called_once_with(self.base_image) + self.image_repo_mock.save.assert_called_once_with(self.base_image, + from_state=None) def test_add_image_with_image_property(self): self.config(image_property_quota=1) diff --git a/glance/tests/unit/test_store_image.py b/glance/tests/unit/test_store_image.py index 424915b5f8..4d63ad7661 100644 --- a/glance/tests/unit/test_store_image.py +++ b/glance/tests/unit/test_store_image.py @@ -33,7 +33,7 @@ class ImageRepoStub(object): def add(self, image): return image - def save(self, image): + def save(self, image, from_state=None): return image diff --git a/glance/tests/unit/v1/test_api.py b/glance/tests/unit/v1/test_api.py index 5618cb085f..187b7f451f 100644 --- a/glance/tests/unit/v1/test_api.py +++ b/glance/tests/unit/v1/test_api.py @@ -1583,8 +1583,7 @@ class TestGlanceAPI(base.IsolatedUnitTest): self.assertEqual(1, mock_store_add_to_backend.call_count) - def test_delete_during_image_upload(self): - req = unit_test_utils.get_fake_request() + def _check_delete_during_image_upload(self, is_admin=False): fixture_headers = {'x-image-meta-store': 'file', 'x-image-meta-disk-format': 'vhd', @@ -1618,30 +1617,18 @@ class TestGlanceAPI(base.IsolatedUnitTest): mock_initiate_deletion) orig_update_image_metadata = registry.update_image_metadata - ctlr = glance.api.v1.controller.BaseController - orig_get_image_meta_or_404 = ctlr.get_image_meta_or_404 + + data = "somedata" def mock_update_image_metadata(*args, **kwargs): - if args[2].get('status', None) == 'deleted': - - # One shot. - def mock_get_image_meta_or_404(*args, **kwargs): - ret = orig_get_image_meta_or_404(*args, **kwargs) - ret['status'] = 'queued' - self.stubs.Set(ctlr, 'get_image_meta_or_404', - orig_get_image_meta_or_404) - return ret - - self.stubs.Set(ctlr, 'get_image_meta_or_404', - mock_get_image_meta_or_404) - - req = webob.Request.blank("/images/%s" % image_id) - req.method = 'PUT' - req.headers['Content-Type'] = 'application/octet-stream' - req.body = "somedata" + if args[2].get('size', None) == len(data): + path = "/images/%s" % image_id + req = unit_test_utils.get_fake_request(path=path, + method='DELETE', + is_admin=is_admin) res = req.get_response(self.api) - self.assertEqual(res.status_int, 200) + self.assertEqual(200, res.status_int) self.stubs.Set(registry, 'update_image_metadata', orig_update_image_metadata) @@ -1651,20 +1638,30 @@ class TestGlanceAPI(base.IsolatedUnitTest): self.stubs.Set(registry, 'update_image_metadata', mock_update_image_metadata) - req = webob.Request.blank("/images/%s" % image_id) - req.method = 'DELETE' + req = unit_test_utils.get_fake_request(path="/images/%s" % image_id, + method='PUT') + req.headers['Content-Type'] = 'application/octet-stream' + req.body = data res = req.get_response(self.api) - self.assertEqual(res.status_int, 200) + self.assertEqual(412, res.status_int) + self.assertFalse(res.location) self.assertTrue(called['initiate_deletion']) - req = webob.Request.blank("/images/%s" % image_id) - req.method = 'HEAD' + req = unit_test_utils.get_fake_request(path="/images/%s" % image_id, + method='HEAD', + is_admin=True) res = req.get_response(self.api) self.assertEqual(res.status_int, 200) self.assertEqual(res.headers['x-image-meta-deleted'], 'True') self.assertEqual(res.headers['x-image-meta-status'], 'deleted') + def test_delete_during_image_upload_by_normal_user(self): + self._check_delete_during_image_upload(is_admin=False) + + def test_delete_during_image_upload_by_admin(self): + self._check_delete_during_image_upload(is_admin=True) + def test_disable_purge_props(self): """ Test the special x-glance-registry-purge-props header controls diff --git a/glance/tests/unit/v2/test_image_data_resource.py b/glance/tests/unit/v2/test_image_data_resource.py index 0dfb55ecce..19fb2063da 100644 --- a/glance/tests/unit/v2/test_image_data_resource.py +++ b/glance/tests/unit/v2/test_image_data_resource.py @@ -79,7 +79,7 @@ class FakeImageRepo(object): else: return self.result - def save(self, image): + def save(self, image, from_state=None): self.saved_image = image @@ -180,17 +180,21 @@ class TestImagesController(base.StoreClearingUnitTest): request, unit_test_utils.UUID1, 'YYYY', 4) def test_upload_non_existent_image_during_save_initiates_deletion(self): - def fake_save(self): + def fake_save_not_found(self): raise exception.NotFound() - request = unit_test_utils.get_fake_request() - image = FakeImage('abcd', locations=['http://example.com/image']) - self.image_repo.result = image - self.image_repo.save = fake_save - image.delete = mock.Mock() - self.assertRaises(webob.exc.HTTPGone, self.controller.upload, - request, str(uuid.uuid4()), 'ABC', 3) - self.assertTrue(image.delete.called) + def fake_save_conflict(self): + raise exception.Conflict() + + for fun in [fake_save_not_found, fake_save_conflict]: + request = unit_test_utils.get_fake_request() + image = FakeImage('abcd', locations=['http://example.com/image']) + self.image_repo.result = image + self.image_repo.save = fun + image.delete = mock.Mock() + self.assertRaises(webob.exc.HTTPGone, self.controller.upload, + request, str(uuid.uuid4()), 'ABC', 3) + self.assertTrue(image.delete.called) def test_upload_non_existent_image_before_save(self): request = unit_test_utils.get_fake_request()