openstack/cinder
Code Issues Proposed changes

Handle quota in 2 phases when managing resource

Browse Source 
Cinder would commit quota twice when managing resource
in a clean environment that the corresponding
'quota_usage' record is empty. This is all because of
we create db entity before reservation, hence the SYNC
mechanism would refresh new record into quota_usage
before actually reserved.

This patch fix this issue by introducing 2 phases
reserve&commit, the latter only intends to update
the actual size.

Closes-Bug: #1587376
Change-Id: I79940e534ec03f2d327e8a7e14e45bc93ae41b0c
This commit is contained in:
TommyLike 2017-11-17 15:41:23 +08:00
parent 503e246b3c
commit e72f0fdf26
9 changed files with 83 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
cinder/tests/unit/api/contrib/test_snapshot_manage.py
View File

@ -135,8 +135,7 @@ class SnapshotManageTest(test.TestCase):
args = mock_create_snapshot.call_args[0]
named_args = mock_create_snapshot.call_args[1]
self.assertEqual(fake.VOLUME_ID, args[1].get('id'))
# We should commit quota in cinder-volume layer for this operation.
self.assertFalse(named_args['commit_quota'])
self.assertTrue(named_args['commit_quota'])
# Check the volume_rpcapi.manage_existing_snapshot was called with
# correct arguments.

55
cinder/tests/unit/volume/flows/test_manage_snapshot_flow.py
View File

@ -16,6 +16,7 @@
# TODO(mdovgal): add tests for other TaskFlow cases
import ddt
import mock
from cinder import context
@ -26,6 +27,7 @@ from cinder.tests.unit import fake_volume
from cinder.volume.flows.manager import manage_existing_snapshot as manager
@ddt.ddt
class ManageSnapshotFlowTestCase(test.TestCase):
def setUp(self):
super(ManageSnapshotFlowTestCase, self).setUp()
@ -58,9 +60,10 @@ class ManageSnapshotFlowTestCase(test.TestCase):
@mock.patch('cinder.objects.volume.Volume.get_by_id')
def test_quota_reservation_task(self, mock_get_vol_by_id, mock_type_get,
mock_quota_reserve):
fake_size = 1
volume_size = 1
fake_size = '2'
fake_snap = fake_snapshot.fake_snapshot_obj(self.ctxt,
volume_size=fake_size)
volume_size=volume_size)
fake_snap.save = mock.MagicMock()
fake_vol = fake_volume.fake_volume_obj(
self.ctxt, id=fake.VOLUME_ID, volume_type_id=fake.VOLUME_TYPE_ID)
@ -74,3 +77,51 @@ class ManageSnapshotFlowTestCase(test.TestCase):
'gigabytes_fake_type_name': 1,
'snapshots_fake_type_name': 1}
mock_quota_reserve.assert_called_once_with(self.ctxt, **reserve_opts)
@ddt.data(True, False)
@mock.patch('cinder.quota.QuotaEngine.reserve')
@mock.patch('cinder.db.sqlalchemy.api.volume_type_get')
@mock.patch('cinder.objects.volume.Volume.get_by_id')
def test_quota_reservation_task_with_update_flag(
self, need_update, mock_get_vol_by_id,
mock_type_get, mock_quota_reserve):
volume_size = 1
fake_size = '2'
fake_snap = fake_snapshot.fake_snapshot_obj(self.ctxt,
volume_size=volume_size)
fake_snap.save = mock.MagicMock()
fake_vol = fake_volume.fake_volume_obj(
self.ctxt, id=fake.VOLUME_ID, volume_type_id=fake.VOLUME_TYPE_ID)
mock_get_vol_by_id.return_value = fake_vol
mock_type_get.return_value = {'name': 'fake_type_name'}
task = manager.QuotaReserveTask()
task.execute(self.ctxt, fake_size, fake_snap,
{'update_size': need_update})
reserve_opts = {'gigabytes': 1, 'gigabytes_fake_type_name': 1}
if not need_update:
reserve_opts.update({'snapshots': 1,
'snapshots_fake_type_name': 1})
mock_quota_reserve.assert_called_once_with(self.ctxt, **reserve_opts)
def test_prepare_for_quota_reserveration_task_execute(self):
mock_db = mock.MagicMock()
mock_driver = mock.MagicMock()
mock_manage_existing_ref = mock.MagicMock()
mock_get_snapshot_size = self.mock_object(
mock_driver, 'manage_existing_snapshot_get_size')
mock_get_snapshot_size.return_value = '5'
fake_snap = fake_snapshot.fake_snapshot_obj(self.ctxt,
volume_size=1)
task = manager.PrepareForQuotaReservationTask(mock_db, mock_driver)
result = task.execute(self.ctxt, fake_snap, mock_manage_existing_ref)
self.assertEqual(fake_snap, result['snapshot_properties'])
self.assertEqual('5', result['size'])
mock_get_snapshot_size.assert_called_once_with(
snapshot=fake_snap,
existing_ref=mock_manage_existing_ref)

3
cinder/tests/unit/volume/flows/test_manage_volume_flow.py
View File

@ -110,7 +110,8 @@ class ManageVolumeFlowTestCase(test.TestCase):
'volume': mock.sentinel.volume,
'manage_existing_ref': mock.sentinel.ref,
'group_snapshot': None,
'optional_args': {'is_quota_committed': False},
'optional_args': {'is_quota_committed': False,
'update_size': True}
}
manager.get_flow(

6
cinder/volume/api.py
View File

@ -1709,6 +1709,10 @@ class API(base.Base):
'metadata': metadata,
'availability_zone': availability_zone,
'bootable': bootable,
'size': 0,
'group_snapshot': None,
'optional_args': {'is_quota_committed': False},
'volume_type_id': None if not volume_type else volume_type['id'],
}
try:
@ -1749,7 +1753,7 @@ class API(base.Base):
snapshot_object = self.create_snapshot_in_db(context, volume, name,
description, True,
metadata, None,
commit_quota=False)
commit_quota=True)
self.volume_rpcapi.manage_existing_snapshot(
context, snapshot_object, ref, service.service_topic_queue)
return snapshot_object

3
cinder/volume/flows/api/create_volume.py
View File

@ -631,6 +631,9 @@ class QuotaReserveTask(flow_utils.CinderTask):
reserve_opts = {'volumes': 1}
else:
reserve_opts = {'volumes': 1, 'gigabytes': size}
if ('update_size' in optional_args
and optional_args['update_size']):
reserve_opts.pop('volumes', None)
QUOTAS.add_volume_type_opts(context, reserve_opts, volume_type_id)
reservations = QUOTAS.reserve(context, **reserve_opts)
return {

5
cinder/volume/flows/api/manage_existing.py
View File

@ -20,6 +20,7 @@ from cinder import exception
from cinder import flow_utils
from cinder import objects
from cinder.objects import fields
from cinder.volume.flows.api import create_volume as create_api
from cinder.volume.flows import common
LOG = logging.getLogger(__name__)
@ -138,7 +139,9 @@ def get_flow(scheduler_rpcapi, db_api, create_what):
# This will cast it out to either the scheduler or volume manager via
# the rpc apis provided.
api_flow.add(EntryCreateTask(db_api),
api_flow.add(create_api.QuotaReserveTask(),
EntryCreateTask(db_api),
create_api.QuotaCommitTask(),
ManageCastTask(scheduler_rpcapi, db_api))
# Now load (but do not run) the flow using the provided initial data.

2
cinder/volume/flows/manager/manage_existing.py
View File

@ -118,7 +118,7 @@ def get_flow(context, db, driver, host, volume, ref):
'volume': volume,
'manage_existing_ref': ref,
'group_snapshot': None,
'optional_args': {'is_quota_committed': False},
'optional_args': {'is_quota_committed': False, 'update_size': True}
}
volume_flow.add(create_mgr.NotifyVolumeActionTask(db,

12
cinder/volume/flows/manager/manage_existing_snapshot.py
View File

@ -147,7 +147,15 @@ class QuotaReserveTask(flow_utils.CinderTask):
if CONF.no_snapshot_gb_quota:
reserve_opts = {'snapshots': 1}
else:
reserve_opts = {'snapshots': 1, 'gigabytes': size}
# NOTE(tommylikehu): We only use the difference of size here
# as we already committed the original size at the API
# service before and this reservation task is only used for
# managing snapshots now.
reserve_opts = {'snapshots': 1,
'gigabytes':
int(size) - snapshot_ref.volume_size}
if 'update_size' in optional_args and optional_args['update_size']:
reserve_opts.pop('snapshots', None)
volume = objects.Volume.get_by_id(context, snapshot_ref.volume_id)
QUOTAS.add_volume_type_opts(context,
reserve_opts,
@ -322,7 +330,7 @@ def get_flow(context, db, driver, host, snapshot_id, ref):
'context': context,
'snapshot_id': snapshot_id,
'manage_existing_ref': ref,
'optional_args': {'is_quota_committed': False}
'optional_args': {'is_quota_committed': False, 'update_size': True}
}
notify_start_msg = "manage_existing_snapshot.start"

4
releasenotes/notes/bug-1587376-fix-manage-resource-quota-issue-78f59f39b9fa4762.yaml Normal file
View File

@ -0,0 +1,4 @@
---
fixes:
- Fix the bug that Cinder would commit quota twice in a clean environment
when managing volume and snapshot resource (Bug #1587376).