openstack
/
glance
Code Issues Proposed changes

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

Change-Id: I47229b366c25367ec1bd48aec684e0880f3dfe60
Signed-off-by: Zhi Yan Liu <zhiyanl@cn.ibm.com>
This commit is contained in:
Zhi Yan Liu 2014-12-30 22:25:50 +08:00 committed by nikhil komawar
parent 9e55118515
commit 0dc8fbb347
15 changed files with 101 additions and 84 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
glance/api/authorization.py
View File

@ -161,10 +161,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)

8
glance/api/policy.py
View File

@ -118,9 +118,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', {})
@ -221,9 +221,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', {})

23
glance/api/v1/upload_utils.py
View File

@ -153,14 +153,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 = _LI("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

16
glance/api/v2/image_data.py
View File

@ -72,14 +72,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."
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: %(error)s Cleaning up the chunks "
"uploaded") %
{'id': image_id,
'error': utils.exception_to_str(e)})
"upload, cleaning up the chunks uploaded.") %
image_id)
LOG.warn(msg)
# NOTE(sridevi): Cleaning up the uploaded chunks.
try:
@ -152,6 +150,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"))

7
glance/db/__init__.py
View File

@ -166,7 +166,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
@ -174,7 +174,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)
@ -267,7 +268,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,

4
glance/domain/proxy.py
View File

@ -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):

4
glance/location.py
View File

@ -61,8 +61,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

4
glance/notifier.py
View File

@ -125,8 +125,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))

4
glance/quota/__init__.py
View File

@ -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))

12
glance/tests/unit/test_domain_proxy.py
View File

@ -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(tuple(), results[i].args)
self.assertEqual({'a': 1}, results[i].kwargs)
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(('snoop',), self.fake_repo.args)
self.assertEqual({}, self.fake_repo.kwargs)
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')

2
glance/tests/unit/test_policy.py
View File

@ -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):

17
glance/tests/unit/test_quota.py
View File

@ -366,7 +366,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)
@ -382,7 +383,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)
@ -421,7 +423,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'])
@ -430,7 +433,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'])
@ -452,7 +456,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'])
@ -471,7 +475,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'])

2
glance/tests/unit/test_store_image.py
View File

@ -35,7 +35,7 @@ class ImageRepoStub(object):
def add(self, image):
return image
def save(self, image):
def save(self, image, from_state=None):
return image

54
glance/tests/unit/v1/test_api.py
View File

@ -1741,8 +1741,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',
@ -1750,8 +1749,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
@ -1776,31 +1775,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(200, res.status_int)
self.assertFalse(res.location)
self.stubs.Set(registry, 'update_image_metadata',
orig_update_image_metadata)
@ -1810,20 +1796,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(200, res.status_int)
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(200, res.status_int)
self.assertEqual('True', res.headers['x-image-meta-deleted'])
self.assertEqual('deleted', res.headers['x-image-meta-status'])
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

24
glance/tests/unit/v2/test_image_data_resource.py
View File

@ -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
@ -182,17 +182,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):