From 8d3e72f8b25ca9ae86544dc7ab249f9941dee5f1 Mon Sep 17 00:00:00 2001 From: Tom Patzig Date: Tue, 8 Mar 2016 16:55:04 +0100 Subject: [PATCH] Update quota of proper user on resource delete When deleting a resource (share, snapshot, share-network) the quota usage of the current user gets updated, which might not be the same user, who created that resource. That means the quota usage will never be reduced for initial user. This fix adds the resources user_id to the quota update statements, to update the quota_usage for the user, that created this resource, irregardless who deletes it. Change-Id: Iefe8f4d0e7c526e3ed94c1994ba62f1a2a929ba2 Closes-Bug: #1542598 --- manila/api/v1/share_networks.py | 6 +++-- manila/quota.py | 20 ++++++++++------ manila/share/api.py | 10 ++++++-- manila/share/manager.py | 6 +++-- manila/tests/api/v1/test_share_networks.py | 9 +++++++- manila/tests/share/test_api.py | 27 +++++++++++++++++++++- manila/tests/share/test_manager.py | 23 ++++++++++++++++++ 7 files changed, 86 insertions(+), 15 deletions(-) diff --git a/manila/api/v1/share_networks.py b/manila/api/v1/share_networks.py index b0d19fa28b..cde19a5bb2 100644 --- a/manila/api/v1/share_networks.py +++ b/manila/api/v1/share_networks.py @@ -97,13 +97,14 @@ class ShareNetworkController(wsgi.Controller): try: reservations = QUOTAS.reserve( context, project_id=share_network['project_id'], - share_networks=-1) + share_networks=-1, user_id=share_network['user_id']) except Exception: LOG.exception(_LE("Failed to update usages deleting " "share-network.")) else: QUOTAS.commit(context, reservations, - project_id=share_network['project_id']) + project_id=share_network['project_id'], + user_id=share_network['user_id']) return webob.Response(status_int=202) def _get_share_networks(self, req, is_detail=True): @@ -247,6 +248,7 @@ class ShareNetworkController(wsgi.Controller): values = body[RESOURCE_NAME] values['project_id'] = context.project_id + values['user_id'] = context.user_id self._verify_no_mutually_exclusive_data(values) try: diff --git a/manila/quota.py b/manila/quota.py index 7b3f620187..682c768d73 100644 --- a/manila/quota.py +++ b/manila/quota.py @@ -301,7 +301,8 @@ class DbQuotaDriver(object): common user's tenant. :param user_id: Specify the user_id if current context is admin and admin wants to impact on - common user. + common user. (Special case: user operates on + resource, owned/created by different user) """ # Filter resources @@ -358,7 +359,8 @@ class DbQuotaDriver(object): common user's tenant. :param user_id: Specify the user_id if current context is admin and admin wants to impact on - common user. + common user. (Special case: user operates on + resource, owned/created by different user) """ # Ensure no value is less than zero @@ -425,7 +427,8 @@ class DbQuotaDriver(object): common user's tenant. :param user_id: Specify the user_id if current context is admin and admin wants to impact on - common user. + common user. (Special case: user operates on + resource, owned/created by different user) """ # Set up the reservation expiration @@ -441,7 +444,7 @@ class DbQuotaDriver(object): # If project_id is None, then we use the project_id in context if project_id is None: project_id = context.project_id - # If user_id is None, then we use the project_id in context + # If user_id is None, then we use the user_id in context if user_id is None: user_id = context.user_id @@ -476,7 +479,8 @@ class DbQuotaDriver(object): common user's tenant. :param user_id: Specify the user_id if current context is admin and admin wants to impact on - common user. + common user. (Special case: user operates on + resource, owned/created by different user) """ # If project_id is None, then we use the project_id in context if project_id is None: @@ -499,7 +503,8 @@ class DbQuotaDriver(object): common user's tenant. :param user_id: Specify the user_id if current context is admin and admin wants to impact on - common user. + common user. (Special case: user operates on + resource, owned/created by different user) """ # If project_id is None, then we use the project_id in context if project_id is None: @@ -912,7 +917,8 @@ class QuotaEngine(object): common user's tenant. :param user_id: Specify the user_id if current context is admin and admin wants to impact on - common user. + common user. (Special case: user operates on + resource, owned/created by different user) """ return self._driver.limit_check(context, self._resources, values, diff --git a/manila/share/api.py b/manila/share/api.py index 439e94e8a2..5ecc79a547 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -605,10 +605,13 @@ class API(base.Base): self._check_is_share_busy(share) try: + # we give the user_id of the share, to update the quota usage + # for the user, who created the share reservations = QUOTAS.reserve(context, project_id=project_id, shares=-1, - gigabytes=-share['size']) + gigabytes=-share['size'], + user_id=share['user_id']) except Exception as e: reservations = None LOG.exception( @@ -623,7 +626,10 @@ class API(base.Base): self.db.share_instance_delete(context, share_instance['id']) if reservations: - QUOTAS.commit(context, reservations, project_id=project_id) + # we give the user_id of the share, to update the quota usage + # for the user, who created the share + QUOTAS.commit(context, reservations, project_id=project_id, + user_id=share['user_id']) def delete_instance(self, context, share_instance, force=False): policy.check_policy(context, 'share', 'delete') diff --git a/manila/share/manager.py b/manila/share/manager.py index 6d68012101..8b028bb6a6 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -1808,13 +1808,15 @@ class ShareManager(manager.SchedulerDependentManager): try: reservations = QUOTAS.reserve( context, project_id=project_id, snapshots=-1, - snapshot_gigabytes=-snapshot_ref['size']) + snapshot_gigabytes=-snapshot_ref['size'], + user_id=snapshot_ref['user_id']) except Exception: reservations = None LOG.exception(_LE("Failed to update usages deleting snapshot")) if reservations: - QUOTAS.commit(context, reservations, project_id=project_id) + QUOTAS.commit(context, reservations, project_id=project_id, + user_id=snapshot_ref['user_id']) @add_hooks @utils.require_driver_initialized diff --git a/manila/tests/api/v1/test_share_networks.py b/manila/tests/api/v1/test_share_networks.py index 4e10cb11e9..1e9cb1a0e5 100644 --- a/manila/tests/api/v1/test_share_networks.py +++ b/manila/tests/api/v1/test_share_networks.py @@ -238,6 +238,8 @@ class ShareNetworkAPITest(test.TestCase): def test_quota_delete_reservation_failed(self): share_nw = fake_share_network.copy() share_nw['share_servers'] = ['foo', 'bar'] + share_nw['user_id'] = 'fake_user_id' + self.mock_object(db_api, 'share_network_get', mock.Mock(return_value=share_nw)) self.mock_object(db_api, 'share_instances_get_all_by_share_network', @@ -262,7 +264,12 @@ class ShareNetworkAPITest(test.TestCase): mock.call(self.req.environ['manila.context'], 'bar')]) db_api.share_network_delete.assert_called_once_with( self.req.environ['manila.context'], share_nw['id']) - self.assertTrue(share_networks.QUOTAS.reserve.called) + share_networks.QUOTAS.reserve.assert_called_once_with( + self.req.environ['manila.context'], + project_id=share_nw['project_id'], + share_networks=-1, + user_id=share_nw['user_id'] + ) self.assertFalse(share_networks.QUOTAS.commit.called) def test_delete_in_use_by_share(self): diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 10be17a297..2699fbdd5e 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -1082,6 +1082,30 @@ class ShareAPITestCase(test.TestCase): db_api.share_snapshot_get_all_for_share.assert_called_once_with( utils.IsAMatcher(context.RequestContext), share['id']) + def test_delete_quota_with_different_user(self): + share = self._setup_delete_mocks(constants.STATUS_AVAILABLE) + diff_user_context = context.RequestContext( + user_id='fake2', + project_id='fake', + is_admin=False + ) + + self.api.delete(diff_user_context, share) + + quota.QUOTAS.reserve.assert_called_once_with( + diff_user_context, + project_id=share['project_id'], + shares=-1, + gigabytes=-share['size'], + user_id=share['user_id'] + ) + quota.QUOTAS.commit.assert_called_once_with( + diff_user_context, + mock.ANY, + project_id=share['project_id'], + user_id=share['user_id'] + ) + def test_delete_wrong_status(self): share = fake_share('fakeid') self.mock_object(db_api, 'share_get', mock.Mock(return_value=share)) @@ -1148,7 +1172,8 @@ class ShareAPITestCase(test.TestCase): self.context, project_id=share['project_id'], shares=-1, - gigabytes=-share['size'] + gigabytes=-share['size'], + user_id=share['user_id'] ) self.assertFalse(quota.QUOTAS.commit.called) diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index 5e6c8c84eb..1659426739 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -1308,6 +1308,29 @@ class ShareManagerTestCase(test.TestCase): utils.IsAMatcher(models.ShareSnapshotInstance), share_server=None) + def test_delete_snapshot_quota_error(self): + share = db_utils.create_share() + share_id = share['id'] + snapshot = db_utils.create_snapshot(share_id=share_id) + snapshot_id = snapshot['id'] + snapshot = db_utils.create_snapshot( + with_share=True, status=constants.STATUS_AVAILABLE) + + self.mock_object(quota.QUOTAS, 'reserve', + mock.Mock(side_effect=exception.QuotaError('fake'))) + self.mock_object(quota.QUOTAS, 'commit') + + self.share_manager.delete_snapshot(self.context, snapshot_id) + + quota.QUOTAS.reserve.assert_called_once_with( + mock.ANY, + project_id=six.text_type(snapshot['project_id']), + snapshots=-1, + snapshot_gigabytes=-snapshot['size'], + user_id=six.text_type(snapshot['user_id']) + ) + self.assertFalse(quota.QUOTAS.commit.called) + def test_delete_share_instance_if_busy(self): """Test snapshot could not be deleted if busy."""