openstack/cinder
Code Issues Proposed changes

Fix quota updating when admin deletes common user's volume

Browse Source 
Add 'project_id=None' optional argument in QUOTAS related methods.

There are two problems when admin deletes volumes of common user
before fixing this bug:
Firstly, It will get tenant quotas using context.project_id which
results in getting admin's quotas not common user's.
Secondly, It will also get wrong quota usages using context.project_id
when updating common user's quota_usages.

So we could pass into QUOTAS related methods an optional argument to
specify the tenant ID to be affected. And if it's not set, then we
default to drawing the tenant ID from the context.

Fixes: bug 1078668
Change-Id: Ie250da84726d1c5c3280430038184000dfd1e9e2
This commit is contained in:
Yug Suo
2013-03-11 16:30:07 +08:00
committed by Guangyu Suo
parent f53d48ebd9
commit 576c9b0880
7 changed files with 167 additions and 69 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
cinder/db/api.py
View File

@@ -680,20 +680,22 @@ def reservation_destroy(context, uuid):
def quota_reserve(context, resources, quotas, deltas, expire,
until_refresh, max_age):
until_refresh, max_age, project_id=None):
"""Check quotas and create appropriate reservations."""
return IMPL.quota_reserve(context, resources, quotas, deltas, expire,
until_refresh, max_age)
until_refresh, max_age, project_id=project_id)
def reservation_commit(context, reservations):
def reservation_commit(context, reservations, project_id=None):
"""Commit quota reservations."""
return IMPL.reservation_commit(context, reservations)
return IMPL.reservation_commit(context, reservations,
project_id=project_id)
def reservation_rollback(context, reservations):
def reservation_rollback(context, reservations, project_id=None):
"""Roll back quota reservations."""
return IMPL.reservation_rollback(context, reservations)
return IMPL.reservation_rollback(context, reservations,
project_id=project_id)
def quota_destroy_all_by_project(context, project_id):

27
cinder/db/sqlalchemy/api.py
View File

@@ -648,12 +648,12 @@ def reservation_destroy(context, uuid):
# code always acquires the lock on quota_usages before acquiring the lock
# on reservations.
def _get_quota_usages(context, session):
def _get_quota_usages(context, session, project_id):
# Broken out for testability
rows = model_query(context, models.QuotaUsage,
read_deleted="no",
session=session).\
filter_by(project_id=context.project_id).\
filter_by(project_id=project_id).\
with_lockmode('update').\
all()
return dict((row.resource, row) for row in rows)
@@ -661,12 +661,15 @@ def _get_quota_usages(context, session):
@require_context
def quota_reserve(context, resources, quotas, deltas, expire,
until_refresh, max_age):
until_refresh, max_age, project_id=None):
elevated = context.elevated()
session = get_session()
with session.begin():
if project_id is None:
project_id = context.project_id
# Get the current usages
usages = _get_quota_usages(context, session)
usages = _get_quota_usages(context, session, project_id)
# Handle usage refresh
work = set(deltas.keys())
@@ -677,7 +680,7 @@ def quota_reserve(context, resources, quotas, deltas, expire,
refresh = False
if resource not in usages:
usages[resource] = quota_usage_create(elevated,
context.project_id,
project_id,
resource,
0, 0,
until_refresh or None,
@@ -700,12 +703,12 @@ def quota_reserve(context, resources, quotas, deltas, expire,
# Grab the sync routine
sync = resources[resource].sync
updates = sync(elevated, context.project_id, session)
updates = sync(elevated, project_id, session)
for res, in_use in updates.items():
# Make sure we have a destination for the usage!
if res not in usages:
usages[res] = quota_usage_create(elevated,
context.project_id,
project_id,
res,
0, 0,
until_refresh or None,
@@ -755,7 +758,7 @@ def quota_reserve(context, resources, quotas, deltas, expire,
reservation = reservation_create(elevated,
str(uuid.uuid4()),
usages[resource],
context.project_id,
project_id,
resource, delta, expire,
session=session)
reservations.append(reservation.uuid)
@@ -804,10 +807,10 @@ def _quota_reservations(session, context, reservations):
@require_context
def reservation_commit(context, reservations):
def reservation_commit(context, reservations, project_id=None):
session = get_session()
with session.begin():
usages = _get_quota_usages(context, session)
usages = _get_quota_usages(context, session, project_id)
for reservation in _quota_reservations(session, context, reservations):
usage = usages[reservation.resource]
@@ -822,10 +825,10 @@ def reservation_commit(context, reservations):
@require_context
def reservation_rollback(context, reservations):
def reservation_rollback(context, reservations, project_id=None):
session = get_session()
with session.begin():
usages = _get_quota_usages(context, session)
usages = _get_quota_usages(context, session, project_id)
for reservation in _quota_reservations(session, context, reservations):
usage = usages[reservation.resource]

83
cinder/quota.py
View File

@@ -174,7 +174,7 @@ class DbQuotaDriver(object):
return quotas
def _get_quotas(self, context, resources, keys, has_sync):
def _get_quotas(self, context, resources, keys, has_sync, project_id=None):
"""
A helper method which retrieves the quotas for the specific
resources identified by keys, and which apply to the current
@@ -187,6 +187,9 @@ class DbQuotaDriver(object):
have a sync attribute; if False, indicates
that the resource must NOT have a sync
attribute.
:param project_id: Specify the project_id if current context
is admin and admin wants to impact on
common user's tenant.
"""
# Filter resources
@@ -205,12 +208,12 @@ class DbQuotaDriver(object):
# Grab and return the quotas (without usages)
quotas = self.get_project_quotas(context, sub_resources,
context.project_id,
project_id,
context.quota_class, usages=False)
return dict((k, v['limit']) for k, v in quotas.items())
def limit_check(self, context, resources, values):
def limit_check(self, context, resources, values, project_id=None):
"""Check simple quota limits.
For limits--those quotas for which there is no usage
@@ -230,6 +233,9 @@ class DbQuotaDriver(object):
:param resources: A dictionary of the registered resources.
:param values: A dictionary of the values to check against the
quota.
:param project_id: Specify the project_id if current context
is admin and admin wants to impact on
common user's tenant.
"""
# Ensure no value is less than zero
@@ -237,9 +243,13 @@ class DbQuotaDriver(object):
if unders:
raise exception.InvalidQuotaValue(unders=sorted(unders))
# If project_id is None, then we use the project_id in context
if project_id is None:
project_id = context.project_id
# Get the applicable quotas
quotas = self._get_quotas(context, resources, values.keys(),
has_sync=False)
has_sync=False, project_id=project_id)
# Check the quotas and construct a list of the resources that
# would be put over limit by the desired values
overs = [key for key, val in values.items()
@@ -248,7 +258,8 @@ class DbQuotaDriver(object):
raise exception.OverQuota(overs=sorted(overs), quotas=quotas,
usages={})
def reserve(self, context, resources, deltas, expire=None):
def reserve(self, context, resources, deltas, expire=None,
project_id=None):
"""Check quotas and reserve resources.
For counting quotas--those quotas for which there is a usage
@@ -278,6 +289,9 @@ class DbQuotaDriver(object):
default expiration time set by
--default-reservation-expire will be used (this
value will be treated as a number of seconds).
:param project_id: Specify the project_id if current context
is admin and admin wants to impact on
common user's tenant.
"""
# Set up the reservation expiration
@@ -290,12 +304,16 @@ class DbQuotaDriver(object):
if not isinstance(expire, datetime.datetime):
raise exception.InvalidReservationExpiration(expire=expire)
# If project_id is None, then we use the project_id in context
if project_id is None:
project_id = context.project_id
# Get the applicable quotas.
# NOTE(Vek): We're not worried about races at this point.
# Yes, the admin may be in the process of reducing
# quotas, but that's a pretty rare thing.
quotas = self._get_quotas(context, resources, deltas.keys(),
has_sync=True)
has_sync=True, project_id=project_id)
# NOTE(Vek): Most of the work here has to be done in the DB
# API, because we have to do it in a transaction,
@@ -303,27 +321,40 @@ class DbQuotaDriver(object):
# session isn't available outside the DBAPI, we
# have to do the work there.
return db.quota_reserve(context, resources, quotas, deltas, expire,
FLAGS.until_refresh, FLAGS.max_age)
FLAGS.until_refresh, FLAGS.max_age,
project_id=project_id)
def commit(self, context, reservations):
def commit(self, context, reservations, project_id=None):
"""Commit reservations.
:param context: The request context, for access checks.
:param reservations: A list of the reservation UUIDs, as
returned by the reserve() method.
:param project_id: Specify the project_id if current context
is admin and admin wants to impact on
common user's tenant.
"""
# If project_id is None, then we use the project_id in context
if project_id is None:
project_id = context.project_id
db.reservation_commit(context, reservations)
db.reservation_commit(context, reservations, project_id=project_id)
def rollback(self, context, reservations):
def rollback(self, context, reservations, project_id=None):
"""Roll back reservations.
:param context: The request context, for access checks.
:param reservations: A list of the reservation UUIDs, as
returned by the reserve() method.
:param project_id: Specify the project_id if current context
is admin and admin wants to impact on
common user's tenant.
"""
# If project_id is None, then we use the project_id in context
if project_id is None:
project_id = context.project_id
db.reservation_rollback(context, reservations)
db.reservation_rollback(context, reservations, project_id=project_id)
def destroy_all_by_project(self, context, project_id):
"""
@@ -603,7 +634,7 @@ class QuotaEngine(object):
return res.count(context, *args, **kwargs)
def limit_check(self, context, **values):
def limit_check(self, context, project_id=None, **values):
"""Check simple quota limits.
For limits--those quotas for which there is no usage
@@ -623,11 +654,15 @@ class QuotaEngine(object):
nothing.
:param context: The request context, for access checks.
:param project_id: Specify the project_id if current context
is admin and admin wants to impact on
common user's tenant.
"""
return self._driver.limit_check(context, self._resources, values)
return self._driver.limit_check(context, self._resources, values,
project_id=project_id)
def reserve(self, context, expire=None, **deltas):
def reserve(self, context, expire=None, project_id=None, **deltas):
"""Check quotas and reserve resources.
For counting quotas--those quotas for which there is a usage
@@ -657,25 +692,32 @@ class QuotaEngine(object):
default expiration time set by
--default-reservation-expire will be used (this
value will be treated as a number of seconds).
:param project_id: Specify the project_id if current context
is admin and admin wants to impact on
common user's tenant.
"""
reservations = self._driver.reserve(context, self._resources, deltas,
expire=expire)
expire=expire,
project_id=project_id)
LOG.debug(_("Created reservations %(reservations)s") % locals())
return reservations
def commit(self, context, reservations):
def commit(self, context, reservations, project_id=None):
"""Commit reservations.
:param context: The request context, for access checks.
:param reservations: A list of the reservation UUIDs, as
returned by the reserve() method.
:param project_id: Specify the project_id if current context
is admin and admin wants to impact on
common user's tenant.
"""
try:
self._driver.commit(context, reservations)
self._driver.commit(context, reservations, project_id=project_id)
except Exception:
# NOTE(Vek): Ignoring exceptions here is safe, because the
# usage resynchronization and the reservation expiration
@@ -684,16 +726,19 @@ class QuotaEngine(object):
LOG.exception(_("Failed to commit reservations "
"%(reservations)s") % locals())
def rollback(self, context, reservations):
def rollback(self, context, reservations, project_id=None):
"""Roll back reservations.
:param context: The request context, for access checks.
:param reservations: A list of the reservation UUIDs, as
returned by the reserve() method.
:param project_id: Specify the project_id if current context
is admin and admin wants to impact on
common user's tenant.
"""
try:
self._driver.rollback(context, reservations)
self._driver.rollback(context, reservations, project_id=project_id)
except Exception:
# NOTE(Vek): Ignoring exceptions here is safe, because the
# usage resynchronization and the reservation expiration

53
cinder/tests/test_quota.py
View File

@@ -143,18 +143,21 @@ class FakeDriver(object):
project_id, quota_class, defaults, usages))
return resources
def limit_check(self, context, resources, values):
self.called.append(('limit_check', context, resources, values))
def limit_check(self, context, resources, values, project_id=None):
self.called.append(('limit_check', context, resources,
values, project_id))
def reserve(self, context, resources, deltas, expire=None):
self.called.append(('reserve', context, resources, deltas, expire))
def reserve(self, context, resources, deltas, expire=None,
project_id=None):
self.called.append(('reserve', context, resources, deltas,
expire, project_id))
return self.reservations
def commit(self, context, reservations):
self.called.append(('commit', context, reservations))
def commit(self, context, reservations, project_id=None):
self.called.append(('commit', context, reservations, project_id))
def rollback(self, context, reservations):
self.called.append(('rollback', context, reservations))
def rollback(self, context, reservations, project_id=None):
self.called.append(('rollback', context, reservations, project_id))
def destroy_all_by_project(self, context, project_id):
self.called.append(('destroy_all_by_project', context, project_id))
@@ -468,7 +471,8 @@ class QuotaEngineTestCase(test.TestCase):
test_resource1=4,
test_resource2=3,
test_resource3=2,
test_resource4=1,)), ])
test_resource4=1,),
None), ])
def test_reserve(self):
context = FakeContext(None, None)
@@ -483,6 +487,9 @@ class QuotaEngineTestCase(test.TestCase):
result2 = quota_obj.reserve(context, expire=3600,
test_resource1=1, test_resource2=2,
test_resource3=3, test_resource4=4)
result3 = quota_obj.reserve(context, project_id='fake_project',
test_resource1=1, test_resource2=2,
test_resource3=3, test_resource4=4)
self.assertEqual(driver.called, [
('reserve',
@@ -493,6 +500,7 @@ class QuotaEngineTestCase(test.TestCase):
test_resource2=3,
test_resource3=2,
test_resource4=1, ),
None,
None),
('reserve',
context,
@@ -502,7 +510,18 @@ class QuotaEngineTestCase(test.TestCase):
test_resource2=2,
test_resource3=3,
test_resource4=4, ),
3600), ])
3600,
None),
('reserve',
context,
quota_obj._resources,
dict(
test_resource1=1,
test_resource2=2,
test_resource3=3,
test_resource4=4, ),
None,
'fake_project'), ])
self.assertEqual(result1, ['resv-01',
'resv-02',
'resv-03',
@@ -511,6 +530,10 @@ class QuotaEngineTestCase(test.TestCase):
'resv-02',
'resv-03',
'resv-04', ])
self.assertEqual(result3, ['resv-01',
'resv-02',
'resv-03',
'resv-04', ])
def test_commit(self):
context = FakeContext(None, None)
@@ -523,7 +546,8 @@ class QuotaEngineTestCase(test.TestCase):
context,
['resv-01',
'resv-02',
'resv-03']), ])
'resv-03'],
None), ])
def test_rollback(self):
context = FakeContext(None, None)
@@ -536,7 +560,8 @@ class QuotaEngineTestCase(test.TestCase):
context,
['resv-01',
'resv-02',
'resv-03']), ])
'resv-03'],
None), ])
def test_destroy_all_by_project(self):
context = FakeContext(None, None)
@@ -788,7 +813,7 @@ class DbQuotaDriverTestCase(test.TestCase):
def _stub_quota_reserve(self):
def fake_quota_reserve(context, resources, quotas, deltas, expire,
until_refresh, max_age):
until_refresh, max_age, project_id=None):
self.calls.append(('quota_reserve', expire, until_refresh,
max_age))
return ['resv-1', 'resv-2', 'resv-3']
@@ -946,7 +971,7 @@ class QuotaReserveSqlAlchemyTestCase(test.TestCase):
def fake_get_session():
return FakeSession()
def fake_get_quota_usages(context, session):
def fake_get_quota_usages(context, session, project_id):
return self.usages.copy()
def fake_quota_usage_create(context, project_id, resource, in_use,

24
cinder/tests/test_volume.py
View File

@@ -96,13 +96,13 @@ class VolumeTestCase(test.TestCase):
def test_create_delete_volume(self):
"""Test volume can be created and deleted."""
# Need to stub out reserve, commit, and rollback
def fake_reserve(context, expire=None, **deltas):
def fake_reserve(context, expire=None, project_id=None, **deltas):
return ["RESERVATION"]
def fake_commit(context, reservations):
def fake_commit(context, reservations, project_id=None):
pass
def fake_rollback(context, reservations):
def fake_rollback(context, reservations, project_id=None):
pass
self.stubs.Set(QUOTAS, "reserve", fake_reserve)
@@ -160,13 +160,13 @@ class VolumeTestCase(test.TestCase):
def test_create_volume_with_volume_type(self):
"""Test volume creation with default volume type."""
def fake_reserve(context, expire=None, **deltas):
def fake_reserve(context, expire=None, project_id=None, **deltas):
return ["RESERVATION"]
def fake_commit(context, reservations):
def fake_commit(context, reservations, project_id=None):
pass
def fake_rollback(context, reservations):
def fake_rollback(context, reservations, project_id=None):
pass
self.stubs.Set(QUOTAS, "reserve", fake_reserve)
@@ -746,13 +746,13 @@ class VolumeTestCase(test.TestCase):
'name', 'description', image_id=1)
def _do_test_create_volume_with_size(self, size):
def fake_reserve(context, expire=None, **deltas):
def fake_reserve(context, expire=None, project_id=None, **deltas):
return ["RESERVATION"]
def fake_commit(context, reservations):
def fake_commit(context, reservations, project_id=None):
pass
def fake_rollback(context, reservations):
def fake_rollback(context, reservations, project_id=None):
pass
self.stubs.Set(QUOTAS, "reserve", fake_reserve)
@@ -776,13 +776,13 @@ class VolumeTestCase(test.TestCase):
self._do_test_create_volume_with_size('2')
def test_create_volume_with_bad_size(self):
def fake_reserve(context, expire=None, **deltas):
def fake_reserve(context, expire=None, project_id=None, **deltas):
return ["RESERVATION"]
def fake_commit(context, reservations):
def fake_commit(context, reservations, project_id=None):
pass
def fake_rollback(context, reservations):
def fake_rollback(context, reservations, project_id=None):
pass
self.stubs.Set(QUOTAS, "reserve", fake_reserve)

11
cinder/volume/api.py
View File

@@ -277,12 +277,19 @@ class API(base.Base):
@wrap_check_policy
def delete(self, context, volume, force=False):
if context.is_admin and context.project_id != volume['project_id']:
project_id = volume['project_id']
else:
project_id = context.project_id
volume_id = volume['id']
if not volume['host']:
# NOTE(vish): scheduling failed, so delete it
# Note(zhiteng): update volume quota reservation
try:
reservations = QUOTAS.reserve(context, volumes=-1,
reservations = QUOTAS.reserve(context,
project_id=project_id,
volumes=-1,
gigabytes=-volume['size'])
except Exception:
reservations = None
@@ -290,7 +297,7 @@ class API(base.Base):
self.db.volume_destroy(context.elevated(), volume_id)
if reservations:
QUOTAS.commit(context, reservations)
QUOTAS.commit(context, reservations, project_id=project_id)
return
if not force and volume['status'] not in ["available", "error",
"error_restoring"]:

24
cinder/volume/manager.py
View File

@@ -393,6 +393,12 @@ class VolumeManager(manager.SchedulerDependentManager):
"""Deletes and unexports volume."""
context = context.elevated()
volume_ref = self.db.volume_get(context, volume_id)
if context.project_id != volume_ref['project_id']:
project_id = volume_ref['project_id']
else:
project_id = context.project_id
LOG.info(_("volume %s: deleting"), volume_ref['name'])
if volume_ref['attach_status'] == "attached":
# Volume is still attached, need to detach first
@@ -422,7 +428,9 @@ class VolumeManager(manager.SchedulerDependentManager):
# Get reservations
try:
reservations = QUOTAS.reserve(context, volumes=-1,
reservations = QUOTAS.reserve(context,
project_id=project_id,
volumes=-1,
gigabytes=-volume_ref['size'])
except Exception:
reservations = None
@@ -435,7 +443,7 @@ class VolumeManager(manager.SchedulerDependentManager):
# Commit the reservations
if reservations:
QUOTAS.commit(context, reservations)
QUOTAS.commit(context, reservations, project_id=project_id)
return True
@@ -474,6 +482,11 @@ class VolumeManager(manager.SchedulerDependentManager):
snapshot_ref = self.db.snapshot_get(context, snapshot_id)
LOG.info(_("snapshot %s: deleting"), snapshot_ref['name'])
if context.project_id != snapshot_ref['project_id']:
project_id = snapshot_ref['project_id']
else:
project_id = context.project_id
try:
LOG.debug(_("snapshot %s: deleting"), snapshot_ref['name'])
self.driver.delete_snapshot(snapshot_ref)
@@ -492,10 +505,13 @@ class VolumeManager(manager.SchedulerDependentManager):
# Get reservations
try:
if CONF.no_snapshot_gb_quota:
reservations = QUOTAS.reserve(context, snapshots=-1)
reservations = QUOTAS.reserve(context,
project_id=project_id,
snapshots=-1)
else:
reservations = QUOTAS.reserve(
context,
project_id=project_id,
snapshots=-1,
gigabytes=-snapshot_ref['volume_size'])
except Exception:
@@ -507,7 +523,7 @@ class VolumeManager(manager.SchedulerDependentManager):
# Commit the reservations
if reservations:
QUOTAS.commit(context, reservations)
QUOTAS.commit(context, reservations, project_id=project_id)
return True
def attach_volume(self, context, volume_id, instance_uuid, mountpoint):