openstack
/
manila
Code Issues Proposed changes

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
This commit is contained in:
Valeriy Ponomaryov 2017-08-11 19:16:16 +03:00 committed by zhongjun
parent 9c37c596bc
commit 3c59630499
6 changed files with 83 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
manila/db/api.py
View File

@ -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):

42
manila/db/sqlalchemy/api.py
View File

@ -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):

34
manila/share/api.py
View File

@ -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')

4
manila/share/manager.py
View File

@ -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)

36
manila/tests/db/sqlalchemy/test_api.py
View File

@ -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()

32
manila/tests/share/test_api.py
View File

@ -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