From 3c596304991927f62290fd940a7f518ec1cae4a2 Mon Sep 17 00:00:00 2001 From: Valeriy Ponomaryov Date: Fri, 11 Aug 2017 19:16:16 +0300 Subject: [PATCH] Fix quota usages update deleting same share from several API endpoints It is possible to update quota usages multiple times sending share deletion request to several API endpoints concurrently. So, move quota usages update logic that is triggered by share deletion, to DB functions level, which will be able to be executed only when share deletion succeded. So, all concurrent requests, that failed to delete DB record, won't commit quota usages updates. Change-Id: If7d52e08d00d435f2e26c30654f0d2180b17b81a Closes-Bug: #1707379 Closes-bug: #1707377 --- manila/db/api.py | 7 +++-- manila/db/sqlalchemy/api.py | 42 +++++++++++++++++++++----- manila/share/api.py | 34 ++------------------- manila/share/manager.py | 4 ++- manila/tests/db/sqlalchemy/test_api.py | 36 ++++++++++++++++++++++ manila/tests/share/test_api.py | 32 ++------------------ 6 files changed, 83 insertions(+), 72 deletions(-) diff --git a/manila/db/api.py b/manila/db/api.py index 364988ff26..aedf0fc4ca 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -311,9 +311,12 @@ def share_instance_create(context, share_id, values): return IMPL.share_instance_create(context, share_id, values) -def share_instance_delete(context, instance_id): +def share_instance_delete(context, instance_id, session=None, + need_to_update_usages=False): """Delete share instance.""" - return IMPL.share_instance_delete(context, instance_id) + return IMPL.share_instance_delete( + context, instance_id, session=session, + need_to_update_usages=need_to_update_usages) def share_instance_update(context, instance_id, values, with_share_data=False): diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 06ffea5f61..9368c3131b 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -49,10 +49,12 @@ from manila.common import constants from manila.db.sqlalchemy import models from manila import exception from manila.i18n import _ +from manila import quota CONF = cfg.CONF LOG = log.getLogger(__name__) +QUOTAS = quota.QUOTAS _DEFAULT_QUOTA_NAME = 'default' PER_PROJECT_QUOTAS = [] @@ -493,8 +495,8 @@ def quota_get_all_by_project_and_user(context, project_id, user_id): ).all() result = {'project_id': project_id, 'user_id': user_id} - for quota in user_quotas: - result[quota.resource] = quota.hard_limit + for u_quota in user_quotas: + result[u_quota.resource] = u_quota.hard_limit return result @@ -516,8 +518,8 @@ def quota_get_all_by_project_and_share_type(context, project_id, 'project_id': project_id, 'share_type_id': share_type_id, } - for quota in share_type_quotas: - result[quota.resource] = quota.hard_limit + for st_quota in share_type_quotas: + result[st_quota.resource] = st_quota.hard_limit return result @@ -531,8 +533,8 @@ def quota_get_all_by_project(context, project_id): ).all() result = {'project_id': project_id} - for quota in project_quotas: - result[quota.resource] = quota.hard_limit + for p_quota in project_quotas: + result[p_quota.resource] = p_quota.hard_limit return result @@ -1416,7 +1418,8 @@ def share_instances_get_all(context, filters=None): @require_context -def share_instance_delete(context, instance_id, session=None): +def share_instance_delete(context, instance_id, session=None, + need_to_update_usages=False): if session is None: session = get_session() @@ -1432,6 +1435,31 @@ def share_instance_delete(context, instance_id, session=None): share_id=share['id']).soft_delete() share.soft_delete(session=session) + if need_to_update_usages: + reservations = None + 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=share['project_id'], + shares=-1, + gigabytes=-share['size'], + user_id=share['user_id'], + share_type_id=instance_ref['share_type_id']) + QUOTAS.commit( + context, reservations, project_id=share['project_id'], + user_id=share['user_id'], + share_type_id=instance_ref['share_type_id']) + except Exception: + LOG.exception( + "Failed to update usages deleting share '%s'.", + share["id"]) + if reservations: + QUOTAS.rollback( + context, reservations, + share_type_id=instance_ref['share_type_id']) + def _set_instances_share_data(context, instances, session): if instances and not isinstance(instances, list): diff --git a/manila/share/api.py b/manila/share/api.py index 21d3531719..d0ce4948ce 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -886,13 +886,7 @@ class API(base.Base): def delete(self, context, share, force=False): """Delete share.""" share = self.db.share_get(context, share['id']) - if context.is_admin and context.project_id != share['project_id']: - project_id = share['project_id'] - else: - project_id = context.project_id - share_id = share['id'] - statuses = (constants.STATUS_AVAILABLE, constants.STATUS_ERROR, constants.STATUS_INACTIVE) if not (force or share['status'] in statuses): @@ -922,36 +916,12 @@ class API(base.Base): raise exception.InvalidShare(reason=msg) 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'], - user_id=share['user_id'], - share_type_id=share['instance']['share_type_id']) - except Exception as e: - reservations = None - LOG.exception( - ("Failed to update quota for deleting share: %s"), e) - for share_instance in share.instances: if share_instance['host']: self.delete_instance(context, share_instance, force=force) else: - self.db.share_instance_delete(context, share_instance['id']) - - if reservations: - # 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'], - share_type_id=share['instance']['share_type_id'], - ) + self.db.share_instance_delete( + context, share_instance['id'], need_to_update_usages=True) 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 8d3c0ba551..8c353f22b2 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -2754,7 +2754,9 @@ class ShareManager(manager.SchedulerDependentManager): resource_id=share_instance_id, exception=excep) - self.db.share_instance_delete(context, share_instance_id) + self.db.share_instance_delete( + context, share_instance_id, need_to_update_usages=True) + LOG.info("Share instance %s: deleted successfully.", share_instance_id) diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index 6549fe624a..b810a29741 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -33,9 +33,12 @@ from manila import context from manila.db.sqlalchemy import api as db_api from manila.db.sqlalchemy import models from manila import exception +from manila import quota from manila import test from manila.tests import db_utils +QUOTAS = quota.QUOTAS + security_service_dict = { 'id': 'fake id', 'project_id': 'fake project', @@ -289,6 +292,39 @@ class ShareDatabaseAPITestCase(test.TestCase): self.assertRaises(exception.NotFound, db_api.share_metadata_get, self.ctxt, share['id']) + def test_share_instance_delete_with_share_need_to_update_usages(self): + share = db_utils.create_share() + + self.assertIsNotNone(db_api.share_get(self.ctxt, share['id'])) + self.assertIsNotNone(db_api.share_metadata_get(self.ctxt, share['id'])) + + self.mock_object(quota.QUOTAS, 'reserve', + mock.Mock(return_value='reservation')) + self.mock_object(quota.QUOTAS, 'commit') + + db_api.share_instance_delete( + self.ctxt, share.instance['id'], need_to_update_usages=True) + + self.assertRaises(exception.NotFound, db_api.share_get, + self.ctxt, share['id']) + self.assertRaises(exception.NotFound, db_api.share_metadata_get, + self.ctxt, share['id']) + quota.QUOTAS.reserve.assert_called_once_with( + self.ctxt, + project_id=share['project_id'], + shares=-1, + gigabytes=-share['size'], + share_type_id=None, + user_id=share['user_id'] + ) + quota.QUOTAS.commit.assert_called_once_with( + self.ctxt, + mock.ANY, + project_id=share['project_id'], + share_type_id=None, + user_id=share['user_id'] + ) + def test_share_instance_get(self): share = db_utils.create_share() diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 6975e7ddba..3f36506737 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -212,9 +212,6 @@ class ShareAPITestCase(test.TestCase): self.mock_object(db_api, 'share_snapshot_get_all_for_share', mock.Mock(return_value=snapshots)) self.mock_object(self.api, 'delete_instance') - self.mock_object(quota.QUOTAS, 'reserve', - mock.Mock(return_value='reservation')) - self.mock_object(quota.QUOTAS, 'commit') return share def _setup_delete_share_instance_mocks(self, **kwargs): @@ -1758,22 +1755,6 @@ class ShareAPITestCase(test.TestCase): 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'], - share_type_id=None, - user_id=share['user_id'] - ) - quota.QUOTAS.commit.assert_called_once_with( - diff_user_context, - mock.ANY, - project_id=share['project_id'], - share_type_id=None, - user_id=share['user_id'] - ) - def test_delete_wrong_status(self): share = fakes.fake_share(status='wrongstatus') self.mock_object(db_api, 'share_get', mock.Mock(return_value=share)) @@ -1807,7 +1788,8 @@ class ShareAPITestCase(test.TestCase): self.api.delete(self.context, share) db_api.share_instance_delete.assert_called_once_with( - utils.IsAMatcher(context.RequestContext), share.instance['id']) + utils.IsAMatcher(context.RequestContext), share.instance['id'], + need_to_update_usages=True) def test_delete_share_with_snapshots(self): share = self._setup_delete_mocks(constants.STATUS_AVAILABLE, @@ -1836,16 +1818,6 @@ class ShareAPITestCase(test.TestCase): self.api.delete(self.context, share) - quota.QUOTAS.reserve.assert_called_once_with( - self.context, - project_id=share['project_id'], - shares=-1, - gigabytes=-share['size'], - share_type_id=None, - user_id=share['user_id'] - ) - self.assertFalse(quota.QUOTAS.commit.called) - @ddt.data({'status': constants.STATUS_AVAILABLE, 'force': False}, {'status': constants.STATUS_ERROR, 'force': True}) @ddt.unpack