From 576c9b0880c32bfc8053550b8f25a76df98c60e4 Mon Sep 17 00:00:00 2001 From: Yug Suo Date: Mon, 11 Mar 2013 16:30:07 +0800 Subject: [PATCH] Fix quota updating when admin deletes common user's volume 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 --- cinder/db/api.py | 14 ++++--- cinder/db/sqlalchemy/api.py | 27 ++++++------ cinder/quota.py | 83 ++++++++++++++++++++++++++++--------- cinder/tests/test_quota.py | 55 +++++++++++++++++------- cinder/tests/test_volume.py | 24 +++++------ cinder/volume/api.py | 11 ++++- cinder/volume/manager.py | 24 +++++++++-- 7 files changed, 168 insertions(+), 70 deletions(-) diff --git a/cinder/db/api.py b/cinder/db/api.py index 900119330..dea3b5ec1 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -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): diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 68b9671eb..7f0e2c49a 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -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] diff --git a/cinder/quota.py b/cinder/quota.py index 34196c2df..a14318724 100644 --- a/cinder/quota.py +++ b/cinder/quota.py @@ -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 diff --git a/cinder/tests/test_quota.py b/cinder/tests/test_quota.py index 20b956335..9f3434a1b 100644 --- a/cinder/tests/test_quota.py +++ b/cinder/tests/test_quota.py @@ -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 reserve(self, context, resources, deltas, expire=None): - self.called.append(('reserve', context, resources, deltas, expire)) + 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, + 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, diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index 278e10584..91474f1ce 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -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) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 8c3d394b0..543f1cf9d 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -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"]: diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index f8e9b28ee..9c2402d73 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -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): -- 2.45.2