--- /dev/null
+From 7d5d8657fd70b20518610b3c6f8e41e16c72fa31 Mon Sep 17 00:00:00 2001
+From: Zhi Yan Liu <zhiyanl@cn.ibm.com>
+Date: Tue, 30 Dec 2014 22:25:50 +0800
+Subject: [PATCH] Cleanup chunks for deleted image that was 'saving'
+
+Currently image data cannot be removed synchronously for an image that
+is in saving state. And when, the upload operation for such an image is
+completed the operator configured quota can be exceeded.
+
+This patch fixes the issue of left over chunks for an image which was
+deleted from saving status. However, by the limitation of the design we
+cannot enforce a global quota check for the image in saving status.
+
+This change introduces a inconsonance between http response codes of
+v1 and v2 APIs. The status codes which we will now see after the upload
+process completes on an image which was deleted mid way are:
+
+v1: 412 Precondition Failed
+v2: 410 Gone
+
+SecurityImpact
+UpgradeImpact
+APIImpact
+
+Closes-Bug: 1383973
+Closes-Bug: 1398830
+Closes-Bug: 1188532
+
+Conflicts:
+ glance/api/v1/upload_utils.py
+ glance/api/v2/image_data.py
+ glance/tests/unit/test_domain_proxy.py
+ glance/tests/unit/v1/test_api.py
+
+Change-Id: I47229b366c25367ec1bd48aec684e0880f3dfe60
+Signed-off-by: Zhi Yan Liu <zhiyanl@cn.ibm.com>
+(cherry picked from commit 0dc8fbb3479a53c5bba8475d14f4c7206904c5ea)
+---
+ glance/api/authorization.py | 4 +-
+ glance/api/policy.py | 8 ++--
+ glance/api/v1/upload_utils.py | 19 +++++---
+ glance/api/v2/image_data.py | 8 +++-
+ glance/db/__init__.py | 7 +--
+ glance/domain/proxy.py | 4 +-
+ glance/location.py | 4 +-
+ glance/notifier.py | 4 +-
+ glance/quota/__init__.py | 4 +-
+ glance/tests/unit/test_domain_proxy.py | 14 +++---
+ glance/tests/unit/test_policy.py | 2 +-
+ glance/tests/unit/test_quota.py | 17 ++++---
+ glance/tests/unit/test_store_image.py | 2 +-
+ glance/tests/unit/v1/test_api.py | 57 +++++++++++-------------
+ glance/tests/unit/v2/test_image_data_resource.py | 24 +++++-----
+ 15 files changed, 98 insertions(+), 80 deletions(-)
+
+diff --git a/glance/api/authorization.py b/glance/api/authorization.py
+index 149ff55..77f7ed3 100644
+--- a/glance/api/authorization.py
++++ b/glance/api/authorization.py
+@@ -158,10 +158,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 0bc8d56..e395876 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', {})
+@@ -285,9 +285,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 8a190fc..60c3d3d 100644
+--- a/glance/api/v1/upload_utils.py
++++ b/glance/api/v1/upload_utils.py
+@@ -153,12 +153,19 @@ 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:
++ 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 = _LI("Image %s could not be found after upload. The image may"
+ " have been deleted during the upload.") % image_id
+ LOG.info(msg)
+diff --git a/glance/api/v2/image_data.py b/glance/api/v2/image_data.py
+index 430ffc5..cdfa34b 100644
+--- a/glance/api/v2/image_data.py
++++ b/glance/api/v2/image_data.py
+@@ -73,8 +73,8 @@ class ImageDataController(object):
+ try:
+ image_repo.save(image)
+ image.set_data(data, size)
+- image_repo.save(image)
+- except exception.NotFound as e:
++ image_repo.save(image, from_state='saving')
++ except (exception.NotFound, exception.Conflict) 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") %
+@@ -152,6 +152,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(_LE("Failed to upload image data due to HTTP error"))
++
+ except webob.exc.HTTPError as e:
+ with excutils.save_and_reraise_exception():
+ LOG.error(_LE("Failed to upload image data due to HTTP error"))
+diff --git a/glance/db/__init__.py b/glance/db/__init__.py
+index 05db7f8..483ba21 100644
+--- a/glance/db/__init__.py
++++ b/glance/db/__init__.py
+@@ -164,7 +164,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
+@@ -172,7 +172,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)
+@@ -265,7 +266,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 5a91d34..09c0fb2 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/location.py b/glance/location.py
+index f83fa7a..b49546d 100644
+--- a/glance/location.py
++++ b/glance/location.py
+@@ -60,8 +60,8 @@ class ImageRepoProxy(glance.domain.proxy.Repo):
+ self._set_acls(image)
+ return result
+
+- def save(self, image):
+- result = super(ImageRepoProxy, self).save(image)
++ def save(self, image, from_state=None):
++ result = super(ImageRepoProxy, self).save(image, from_state=from_state)
+ self._set_acls(image)
+ return result
+
+diff --git a/glance/notifier.py b/glance/notifier.py
+index 5ec0854..21223da 100644
+--- a/glance/notifier.py
++++ b/glance/notifier.py
+@@ -122,8 +122,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 4051992..d628a8c 100644
+--- a/glance/quota/__init__.py
++++ b/glance/quota/__init__.py
+@@ -104,10 +104,10 @@ class ImageRepoProxy(glance.domain.proxy.Repo):
+ LOG.debug(six.text_type(exc))
+ raise exc
+
+- def save(self, image):
++ def save(self, image, from_state=None):
+ if image.added_new_properties():
+ self._enforce_image_property_quota(len(image.extra_properties))
+- return super(ImageRepoProxy, self).save(image)
++ return super(ImageRepoProxy, self).save(image, from_state=from_state)
+
+ def add(self, image):
+ self._enforce_image_property_quota(len(image.extra_properties))
+diff --git a/glance/tests/unit/test_domain_proxy.py b/glance/tests/unit/test_domain_proxy.py
+index 1bb6863..2b8f792 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.assertIsNone(proxy_result)
+@@ -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 5b5e870..44546a0 100644
+--- a/glance/tests/unit/test_policy.py
++++ b/glance/tests/unit/test_policy.py
+@@ -78,7 +78,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 c12eda2..1c40fb4 100644
+--- a/glance/tests/unit/test_quota.py
++++ b/glance/tests/unit/test_quota.py
+@@ -367,7 +367,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)
+@@ -383,7 +384,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)
+@@ -422,7 +424,8 @@ class TestImagePropertyQuotas(test_utils.BaseTestCase):
+ self.config(image_property_quota=1)
+ self.image.extra_properties = {'foo': 'frob', 'spam': 'eggs'}
+ 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)
+ self.assertEqual('frob', self.base_image.extra_properties['foo'])
+ self.assertEqual('eggs', self.base_image.extra_properties['spam'])
+
+@@ -431,7 +434,8 @@ class TestImagePropertyQuotas(test_utils.BaseTestCase):
+ self.config(image_property_quota=1)
+ del self.image.extra_properties['foo']
+ 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)
+ self.assertNotIn('foo', self.base_image.extra_properties)
+ self.assertEqual('ham', self.base_image.extra_properties['spam'])
+
+@@ -447,7 +451,7 @@ class TestImagePropertyQuotas(test_utils.BaseTestCase):
+ del self.image.extra_properties['frob']
+ del self.image.extra_properties['lorem']
+ self.image_repo_proxy.save(self.image)
+- call_args = mock.call(self.base_image)
++ call_args = mock.call(self.base_image, from_state=None)
+ self.assertEqual(call_args, self.image_repo_mock.save.call_args)
+ self.assertEqual('bar', self.base_image.extra_properties['foo'])
+ self.assertEqual('ham', self.base_image.extra_properties['spam'])
+@@ -466,7 +470,8 @@ class TestImagePropertyQuotas(test_utils.BaseTestCase):
+ self.config(image_property_quota=1)
+ del self.image.extra_properties['foo']
+ 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)
+ self.assertNotIn('foo', self.base_image.extra_properties)
+ self.assertEqual('ham', self.base_image.extra_properties['spam'])
+ self.assertEqual('baz', self.base_image.extra_properties['frob'])
+diff --git a/glance/tests/unit/test_store_image.py b/glance/tests/unit/test_store_image.py
+index 8b334ab..b656454 100644
+--- a/glance/tests/unit/test_store_image.py
++++ b/glance/tests/unit/test_store_image.py
+@@ -36,7 +36,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 39e9a44..7dc0737 100644
+--- a/glance/tests/unit/v1/test_api.py
++++ b/glance/tests/unit/v1/test_api.py
+@@ -39,7 +39,6 @@ from glance.db.sqlalchemy import api as db_api
+ from glance.db.sqlalchemy import models as db_models
+ from glance.openstack.common import jsonutils
+ from glance.openstack.common import timeutils
+-
+ import glance.registry.client.v1.api as registry
+ from glance.tests.unit import base
+ import glance.tests.unit.utils as unit_test_utils
+@@ -1735,8 +1734,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',
+@@ -1744,8 +1742,8 @@ class TestGlanceAPI(base.IsolatedUnitTest):
+ 'x-image-meta-name': 'fake image #3',
+ 'x-image-meta-property-key1': 'value1'}
+
+- req = webob.Request.blank("/images")
+- req.method = 'POST'
++ req = unit_test_utils.get_fake_request(path="/images",
++ is_admin=is_admin)
+ for k, v in six.iteritems(fixture_headers):
+ req.headers[k] = v
+
+@@ -1770,31 +1768,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
+-
+- def mock_update_image_metadata(*args, **kwargs):
+
+- if args[2].get('status', None) == 'deleted':
++ data = "somedata"
+
+- # 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)
++ def mock_update_image_metadata(*args, **kwargs):
+
+- 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.assertFalse(res.location)
++ self.assertEqual(200, res.status_int)
+
+ self.stubs.Set(registry, 'update_image_metadata',
+ orig_update_image_metadata)
+@@ -1804,20 +1789,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 cc8148a..a121e82 100644
+--- a/glance/tests/unit/v2/test_image_data_resource.py
++++ b/glance/tests/unit/v2/test_image_data_resource.py
+@@ -81,7 +81,7 @@ class FakeImageRepo(object):
+ else:
+ return self.result
+
+- def save(self, image):
++ def save(self, image, from_state=None):
+ self.saved_image = image
+
+
+@@ -184,17 +184,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_raises_not_found_exception(self):
+ def fake_save(self):
+--
+2.0.5
+
projects / gentoo.git / commitdiff