Allow some property operations when quota exceeded
Currently in Glance v2 if, for some reason, an image has more properties than the image_propery_quota allows (e.g. the quota was lowered after the image was created with properties) then any request to modify or delete existing properties results in a 413 overlimit error. Ideally a user should be able to remove properties or any other action except for adding a property when they are over their quota for a given image. This commit does this by adding a new member to the quota.ImageProxy class to "remember" what properties were already present in an image *before* any new property operations are preformed on the image. After the new property operations are performed the quotas are checked (before writing the image info to DB) only if any new properties have been added. This commit does not use a subclass of ExtraPropertiesProxy to check property quotas (in the __setitem__ method) because Glance does not implement the JSON-patch RFC correctly - in Glance all operations in a patch are applied and the quota checked only after all operations have been applied (RFC requires that operations be applied sequentially and fail on the first failure). Therefore it is possible for the quota to be temporarily exceeded when a patch is being applied and therefore we cannot check for quotas as they are being added - we have to wait until all patch operations have been completed. Also, as per review discussions in IRC, a new file: glance/tests/integration/v2/test_property_quota_violations.py has been added to perform image property quota related tests (because the functional test framework is slow). Change-Id: Icf1b46343463791ed3d2f3ce376f11e409e792ff Closes-bug: #1258331 Author: David Koo <david.koo@huawei.com>
This commit is contained in:
parent
56625d9fab
commit
0d95e5316a
|
@ -14,6 +14,8 @@
|
||||||
|
|
||||||
import copy
|
import copy
|
||||||
|
|
||||||
|
import six
|
||||||
|
|
||||||
from oslo.config import cfg
|
from oslo.config import cfg
|
||||||
|
|
||||||
import glance.api.common
|
import glance.api.common
|
||||||
|
@ -87,24 +89,26 @@ class ImageRepoProxy(glance.domain.proxy.Repo):
|
||||||
item_proxy_class=ImageProxy,
|
item_proxy_class=ImageProxy,
|
||||||
item_proxy_kwargs=proxy_kwargs)
|
item_proxy_kwargs=proxy_kwargs)
|
||||||
|
|
||||||
def _enforce_image_property_quota(self, image):
|
def _enforce_image_property_quota(self, attempted):
|
||||||
if CONF.image_property_quota < 0:
|
if CONF.image_property_quota < 0:
|
||||||
# If value is negative, allow unlimited number of properties
|
# If value is negative, allow unlimited number of properties
|
||||||
return
|
return
|
||||||
|
|
||||||
attempted = len(image.extra_properties)
|
|
||||||
maximum = CONF.image_property_quota
|
maximum = CONF.image_property_quota
|
||||||
if attempted > maximum:
|
if attempted > maximum:
|
||||||
raise exception.ImagePropertyLimitExceeded(attempted=attempted,
|
kwargs = {'attempted': attempted, 'maximum': maximum}
|
||||||
maximum=maximum)
|
exc = exception.ImagePropertyLimitExceeded(**kwargs)
|
||||||
|
LOG.debug(six.text_type(exc))
|
||||||
|
raise exc
|
||||||
|
|
||||||
def save(self, image):
|
def save(self, image):
|
||||||
self._enforce_image_property_quota(image)
|
if image.added_new_properties():
|
||||||
super(ImageRepoProxy, self).save(image)
|
self._enforce_image_property_quota(len(image.extra_properties))
|
||||||
|
return super(ImageRepoProxy, self).save(image)
|
||||||
|
|
||||||
def add(self, image):
|
def add(self, image):
|
||||||
self._enforce_image_property_quota(image)
|
self._enforce_image_property_quota(len(image.extra_properties))
|
||||||
super(ImageRepoProxy, self).add(image)
|
return super(ImageRepoProxy, self).add(image)
|
||||||
|
|
||||||
|
|
||||||
class ImageFactoryProxy(glance.domain.proxy.ImageFactory):
|
class ImageFactoryProxy(glance.domain.proxy.ImageFactory):
|
||||||
|
@ -117,7 +121,6 @@ class ImageFactoryProxy(glance.domain.proxy.ImageFactory):
|
||||||
|
|
||||||
def new_image(self, **kwargs):
|
def new_image(self, **kwargs):
|
||||||
tags = kwargs.pop('tags', set([]))
|
tags = kwargs.pop('tags', set([]))
|
||||||
|
|
||||||
_enforce_image_tag_quota(tags)
|
_enforce_image_tag_quota(tags)
|
||||||
return super(ImageFactoryProxy, self).new_image(tags=tags, **kwargs)
|
return super(ImageFactoryProxy, self).new_image(tags=tags, **kwargs)
|
||||||
|
|
||||||
|
@ -277,6 +280,7 @@ class ImageProxy(glance.domain.proxy.Image):
|
||||||
self.db_api = db_api
|
self.db_api = db_api
|
||||||
self.store_utils = store_utils
|
self.store_utils = store_utils
|
||||||
super(ImageProxy, self).__init__(image)
|
super(ImageProxy, self).__init__(image)
|
||||||
|
self.orig_props = set(image.extra_properties.keys())
|
||||||
|
|
||||||
def set_data(self, data, size=None):
|
def set_data(self, data, size=None):
|
||||||
remaining = glance.api.common.check_quota(
|
remaining = glance.api.common.check_quota(
|
||||||
|
@ -355,3 +359,7 @@ class ImageProxy(glance.domain.proxy.Image):
|
||||||
self.context, required_size, self.db_api,
|
self.context, required_size, self.db_api,
|
||||||
image_id=self.image.image_id)
|
image_id=self.image.image_id)
|
||||||
self.image.locations = value
|
self.image.locations = value
|
||||||
|
|
||||||
|
def added_new_properties(self):
|
||||||
|
current_props = set(self.image.extra_properties.keys())
|
||||||
|
return bool(current_props.difference(self.orig_props))
|
||||||
|
|
|
@ -0,0 +1,125 @@
|
||||||
|
# Copyright 2012 OpenStack Foundation
|
||||||
|
# All Rights Reserved.
|
||||||
|
#
|
||||||
|
# Licensed under the Apache License, Version 2.0 (the "License"); you may
|
||||||
|
# not use this file except in compliance with the License. You may obtain
|
||||||
|
# a copy of the License at
|
||||||
|
#
|
||||||
|
# http://www.apache.org/licenses/LICENSE-2.0
|
||||||
|
#
|
||||||
|
# Unless required by applicable law or agreed to in writing, software
|
||||||
|
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
|
||||||
|
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
|
||||||
|
# License for the specific language governing permissions and limitations
|
||||||
|
# under the License.
|
||||||
|
|
||||||
|
from oslo.config import cfg
|
||||||
|
|
||||||
|
from glance.openstack.common import jsonutils
|
||||||
|
from glance.tests.integration.v2 import base
|
||||||
|
|
||||||
|
CONF = cfg.CONF
|
||||||
|
|
||||||
|
CONF.import_opt('backend', 'glance.openstack.common.db.api', group='database')
|
||||||
|
|
||||||
|
|
||||||
|
class TestPropertyQuotaViolations(base.ApiTest):
|
||||||
|
def __init__(self, *args, **kwargs):
|
||||||
|
super(TestPropertyQuotaViolations, self).__init__(*args, **kwargs)
|
||||||
|
self.api_flavor = 'noauth'
|
||||||
|
self.registry_flavor = 'fakeauth'
|
||||||
|
|
||||||
|
def _headers(self, custom_headers=None):
|
||||||
|
base_headers = {
|
||||||
|
'X-Identity-Status': 'Confirmed',
|
||||||
|
'X-Auth-Token': '932c5c84-02ac-4fe5-a9ba-620af0e2bb96',
|
||||||
|
'X-User-Id': 'f9a41d13-0c13-47e9-bee2-ce4e8bfe958e',
|
||||||
|
'X-Tenant-Id': "foo",
|
||||||
|
'X-Roles': 'member',
|
||||||
|
}
|
||||||
|
base_headers.update(custom_headers or {})
|
||||||
|
return base_headers
|
||||||
|
|
||||||
|
def _get(self, image_id=""):
|
||||||
|
path = ('/v2/images/%s' % image_id).rstrip('/')
|
||||||
|
rsp, content = self.http.request(path, 'GET', headers=self._headers())
|
||||||
|
self.assertEqual(200, rsp.status)
|
||||||
|
content = jsonutils.loads(content)
|
||||||
|
return content
|
||||||
|
|
||||||
|
def _create_image(self, body):
|
||||||
|
path = '/v2/images'
|
||||||
|
headers = self._headers({'content-type': 'application/json'})
|
||||||
|
rsp, content = self.http.request(path, 'POST', headers=headers,
|
||||||
|
body=jsonutils.dumps(body))
|
||||||
|
self.assertEqual(201, rsp.status)
|
||||||
|
return jsonutils.loads(content)
|
||||||
|
|
||||||
|
def _patch(self, image_id, body, expected_status):
|
||||||
|
path = '/v2/images/%s' % image_id
|
||||||
|
media_type = 'application/openstack-images-v2.1-json-patch'
|
||||||
|
headers = self._headers({'content-type': media_type})
|
||||||
|
rsp, content = self.http.request(path, 'PATCH', headers=headers,
|
||||||
|
body=jsonutils.dumps(body))
|
||||||
|
self.assertEqual(expected_status, rsp.status, content)
|
||||||
|
return content
|
||||||
|
|
||||||
|
def test_property_ops_when_quota_violated(self):
|
||||||
|
# Image list must be empty to begin with
|
||||||
|
image_list = self._get()['images']
|
||||||
|
self.assertEqual(0, len(image_list))
|
||||||
|
|
||||||
|
orig_property_quota = 10
|
||||||
|
CONF.set_override('image_property_quota', orig_property_quota)
|
||||||
|
|
||||||
|
# Create an image (with deployer-defined properties)
|
||||||
|
req_body = {'name': 'testimg',
|
||||||
|
'disk_format': 'aki',
|
||||||
|
'container_format': 'aki'}
|
||||||
|
for i in range(orig_property_quota):
|
||||||
|
req_body['k_%d' % i] = 'v_%d' % i
|
||||||
|
image = self._create_image(req_body)
|
||||||
|
image_id = image['id']
|
||||||
|
for i in range(orig_property_quota):
|
||||||
|
self.assertEqual('v_%d' % i, image['k_%d' % i])
|
||||||
|
|
||||||
|
# Now reduce property quota. We should be allowed to modify/delete
|
||||||
|
# existing properties (even if the result still exceeds property quota)
|
||||||
|
# but not add new properties nor replace existing properties with new
|
||||||
|
# properties (as long as we're over the quota)
|
||||||
|
self.config(image_property_quota=2)
|
||||||
|
|
||||||
|
patch_body = [{'op': 'replace', 'path': '/k_4', 'value': 'v_4.new'}]
|
||||||
|
image = jsonutils.loads(self._patch(image_id, patch_body, 200))
|
||||||
|
self.assertEqual('v_4.new', image['k_4'])
|
||||||
|
|
||||||
|
patch_body = [{'op': 'remove', 'path': '/k_7'}]
|
||||||
|
image = jsonutils.loads(self._patch(image_id, patch_body, 200))
|
||||||
|
self.assertNotIn('k_7', image)
|
||||||
|
|
||||||
|
patch_body = [{'op': 'add', 'path': '/k_100', 'value': 'v_100'}]
|
||||||
|
self._patch(image_id, patch_body, 413)
|
||||||
|
image = self._get(image_id)
|
||||||
|
self.assertNotIn('k_100', image)
|
||||||
|
|
||||||
|
patch_body = [
|
||||||
|
{'op': 'remove', 'path': '/k_5'},
|
||||||
|
{'op': 'add', 'path': '/k_100', 'value': 'v_100'},
|
||||||
|
]
|
||||||
|
self._patch(image_id, patch_body, 413)
|
||||||
|
image = self._get(image_id)
|
||||||
|
self.assertNotIn('k_100', image)
|
||||||
|
self.assertIn('k_5', image)
|
||||||
|
|
||||||
|
# temporary violations to property quota should be allowed as long as
|
||||||
|
# it's within one PATCH request and the end result does not violate
|
||||||
|
# quotas.
|
||||||
|
patch_body = [{'op': 'add', 'path': '/k_100', 'value': 'v_100'},
|
||||||
|
{'op': 'add', 'path': '/k_99', 'value': 'v_99'}]
|
||||||
|
to_rm = ['k_%d' % i for i in range(orig_property_quota) if i != 7]
|
||||||
|
patch_body.extend([{'op': 'remove', 'path': '/%s' % k} for k in to_rm])
|
||||||
|
image = jsonutils.loads(self._patch(image_id, patch_body, 200))
|
||||||
|
self.assertEqual('v_99', image['k_99'])
|
||||||
|
self.assertEqual('v_100', image['k_100'])
|
||||||
|
for k in to_rm:
|
||||||
|
self.assertNotIn(k, image)
|
|
@ -45,6 +45,9 @@ class FakeImage(object):
|
||||||
for d in data:
|
for d in data:
|
||||||
self.size += len(d)
|
self.size += len(d)
|
||||||
|
|
||||||
|
def __init__(self, **kwargs):
|
||||||
|
self.extra_properties = kwargs.get('extra_properties', {})
|
||||||
|
|
||||||
|
|
||||||
class TestImageQuota(test_utils.BaseTestCase):
|
class TestImageQuota(test_utils.BaseTestCase):
|
||||||
def setUp(self):
|
def setUp(self):
|
||||||
|
@ -342,13 +345,15 @@ class TestImageQuota(test_utils.BaseTestCase):
|
||||||
class TestImagePropertyQuotas(test_utils.BaseTestCase):
|
class TestImagePropertyQuotas(test_utils.BaseTestCase):
|
||||||
def setUp(self):
|
def setUp(self):
|
||||||
super(TestImagePropertyQuotas, self).setUp()
|
super(TestImagePropertyQuotas, self).setUp()
|
||||||
self.base_image = mock.Mock()
|
self.base_image = FakeImage()
|
||||||
self.image = glance.quota.ImageProxy(self.base_image,
|
self.image = glance.quota.ImageProxy(self.base_image,
|
||||||
mock.Mock(),
|
mock.Mock(),
|
||||||
mock.Mock(),
|
mock.Mock(),
|
||||||
mock.Mock())
|
mock.Mock())
|
||||||
|
|
||||||
self.image_repo_mock = mock.Mock()
|
self.image_repo_mock = mock.Mock()
|
||||||
|
self.image_repo_mock.add.return_value = self.base_image
|
||||||
|
self.image_repo_mock.save.return_value = self.base_image
|
||||||
|
|
||||||
self.image_repo_proxy = glance.quota.ImageRepoProxy(
|
self.image_repo_proxy = glance.quota.ImageRepoProxy(
|
||||||
self.image_repo_mock,
|
self.image_repo_mock,
|
||||||
|
@ -404,12 +409,75 @@ class TestImagePropertyQuotas(test_utils.BaseTestCase):
|
||||||
|
|
||||||
self.image_repo_mock.add.assert_called_once_with(self.base_image)
|
self.image_repo_mock.add.assert_called_once_with(self.base_image)
|
||||||
|
|
||||||
|
def _quota_exceed_setup(self):
|
||||||
|
self.config(image_property_quota=2)
|
||||||
|
self.base_image.extra_properties = {'foo': 'bar', 'spam': 'ham'}
|
||||||
|
self.image = glance.quota.ImageProxy(self.base_image,
|
||||||
|
mock.Mock(),
|
||||||
|
mock.Mock(),
|
||||||
|
mock.Mock())
|
||||||
|
|
||||||
|
def test_modify_image_properties_when_quota_exceeded(self):
|
||||||
|
self._quota_exceed_setup()
|
||||||
|
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.assertEqual('frob', self.base_image.extra_properties['foo'])
|
||||||
|
self.assertEqual('eggs', self.base_image.extra_properties['spam'])
|
||||||
|
|
||||||
|
def test_delete_image_properties_when_quota_exceeded(self):
|
||||||
|
self._quota_exceed_setup()
|
||||||
|
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.assertNotIn('foo', self.base_image.extra_properties)
|
||||||
|
self.assertEqual('ham', self.base_image.extra_properties['spam'])
|
||||||
|
|
||||||
|
def test_exceed_quota_during_patch_operation(self):
|
||||||
|
self._quota_exceed_setup()
|
||||||
|
self.image.extra_properties['frob'] = 'baz'
|
||||||
|
self.image.extra_properties['lorem'] = 'ipsum'
|
||||||
|
self.assertEqual('bar', self.base_image.extra_properties['foo'])
|
||||||
|
self.assertEqual('ham', self.base_image.extra_properties['spam'])
|
||||||
|
self.assertEqual('baz', self.base_image.extra_properties['frob'])
|
||||||
|
self.assertEqual('ipsum', self.base_image.extra_properties['lorem'])
|
||||||
|
|
||||||
|
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)
|
||||||
|
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'])
|
||||||
|
self.assertNotIn('frob', self.base_image.extra_properties)
|
||||||
|
self.assertNotIn('lorem', self.base_image.extra_properties)
|
||||||
|
|
||||||
|
def test_quota_exceeded_after_delete_image_properties(self):
|
||||||
|
self.config(image_property_quota=3)
|
||||||
|
self.base_image.extra_properties = {'foo': 'bar',
|
||||||
|
'spam': 'ham',
|
||||||
|
'frob': 'baz'}
|
||||||
|
self.image = glance.quota.ImageProxy(self.base_image,
|
||||||
|
mock.Mock(),
|
||||||
|
mock.Mock(),
|
||||||
|
mock.Mock())
|
||||||
|
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.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'])
|
||||||
|
|
||||||
|
|
||||||
class TestImageTagQuotas(test_utils.BaseTestCase):
|
class TestImageTagQuotas(test_utils.BaseTestCase):
|
||||||
def setUp(self):
|
def setUp(self):
|
||||||
super(TestImageTagQuotas, self).setUp()
|
super(TestImageTagQuotas, self).setUp()
|
||||||
self.base_image = mock.Mock()
|
self.base_image = mock.Mock()
|
||||||
self.base_image.tags = set([])
|
self.base_image.tags = set([])
|
||||||
|
self.base_image.extra_properties = {}
|
||||||
self.image = glance.quota.ImageProxy(self.base_image,
|
self.image = glance.quota.ImageProxy(self.base_image,
|
||||||
mock.Mock(),
|
mock.Mock(),
|
||||||
mock.Mock(),
|
mock.Mock(),
|
||||||
|
@ -549,6 +617,7 @@ class TestImageLocationQuotas(test_utils.BaseTestCase):
|
||||||
self.base_image = mock.Mock()
|
self.base_image = mock.Mock()
|
||||||
self.base_image.locations = []
|
self.base_image.locations = []
|
||||||
self.base_image.size = 1
|
self.base_image.size = 1
|
||||||
|
self.base_image.extra_properties = {}
|
||||||
self.image = glance.quota.ImageProxy(self.base_image,
|
self.image = glance.quota.ImageProxy(self.base_image,
|
||||||
mock.Mock(),
|
mock.Mock(),
|
||||||
mock.Mock(),
|
mock.Mock(),
|
||||||
|
|
Loading…
Reference in New Issue