openstack/cinder
Code Issues Proposed changes

Refactor get_volume_type_extra_specs

Browse Source 
The current interface is confusing because the function returns
"a spec", not "specs", when the key argument is used. Return specs
consistently and leave the logic to pick up a specific spec to the code
using the utility method.

Change-Id: I256c080c8f472340cffe733423698a5fd1f487fd
This commit is contained in:
Takashi Kajinami
2024-05-18 15:26:48 +09:00
parent 0fbd736183
commit 169388b86b
8 changed files with 25 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
cinder/api/contrib/types_extra_specs.py
View File

@ -70,10 +70,9 @@ class VolumeTypeExtraSpecsController(wsgi.Controller):
return
def _check_cacheable(self, specs, type_id):
multiattach = volume_types.get_volume_type_extra_specs(
type_id, key='multiattach')
cacheable = volume_types.get_volume_type_extra_specs(
type_id, key='cacheable')
extra_specs = volume_types.get_volume_type_extra_specs(type_id)
multiattach = extra_specs.get('multiattach')
cacheable = extra_specs.get('cacheable')
isTrue = '<is> True'
if (specs.get('multiattach') == isTrue and cacheable == isTrue) or (
specs.get('cacheable') == isTrue and multiattach ==

26
cinder/tests/unit/api/contrib/test_types_extra_specs.py
View File

@ -491,53 +491,39 @@ class VolumeTypesExtraSpecsTest(test.TestCase):
@mock.patch('cinder.volume.volume_types.get_volume_type_extra_specs')
def test_check_cacheable(self, get_extra_specs):
ret_multiattach = ''
ret_cacheable = ''
def side_get_specs(type_id, key):
if key == 'multiattach':
return ret_multiattach
if key == 'cacheable':
return ret_cacheable
get_extra_specs.return_value = ''
get_extra_specs.side_effect = side_get_specs
get_extra_specs.return_value = {}
specs = {'multiattach': '<is> True',
'cacheable': '<is> True'}
self.assertRaises(webob.exc.HTTPBadRequest,
self.controller._check_cacheable,
specs, 'typeid')
ret_multiattach = '<is> True'
ret_cacheable = ''
get_extra_specs.return_value = {'multiattach': '<is> True'}
specs = {'cacheable': '<is> True'}
self.assertRaises(webob.exc.HTTPBadRequest,
self.controller._check_cacheable,
specs, 'typeid')
ret_multiattach = ''
ret_cacheable = '<is> True'
get_extra_specs.return_value = {'cacheable': '<is> True'}
specs = {'multiattach': '<is> True'}
self.assertRaises(webob.exc.HTTPBadRequest,
self.controller._check_cacheable,
specs, 'typeid')
ret_multiattach = '<is> False'
ret_cacheable = ''
get_extra_specs.return_value = {'multiattach': '<is> False'}
specs = {'multiattach': '<is> True'}
# Should NOT has exception when calling below line
self.controller._check_cacheable(specs, 'typeid')
ret_multiattach = '<is> True'
ret_cacheable = ''
get_extra_specs.return_value = {'multiattach': '<is> True'}
specs = {'multiattach': '<is> False', 'cacheable': '<is> True'}
# Should NOT setting both at the same time
self.assertRaises(webob.exc.HTTPBadRequest,
self.controller._check_cacheable,
specs, 'typeid')
ret_multiattach = '<is> False'
ret_cacheable = ''
get_extra_specs.return_value = {'multiattach': '<is> False'}
specs = {'multiattach': '<is> False', 'cacheable': '<is> True'}
# Should NOT has exception when calling below line
self.controller._check_cacheable(specs, 'typeid')

4
cinder/tests/unit/attachments/test_attachments_manager.py
View File

@ -79,7 +79,7 @@ class AttachmentManagerTestCase(test.TestCase):
'driver_volume_type': 'iscsi',
'attachment_id': attachment_ref.id}
get_extra_specs.return_value = ''
get_extra_specs.return_value = {}
self.assertEqual(expected,
self.manager.attachment_update(
self.context,
@ -94,7 +94,7 @@ class AttachmentManagerTestCase(test.TestCase):
'driver_volume_type': 'iscsi',
'attachment_id': attachment_ref.id}
get_extra_specs.return_value = '<is> True'
get_extra_specs.return_value = {'cacheable': '<is> True'}
self.assertEqual(expected,
self.manager.attachment_update(
self.context,

8
cinder/tests/unit/volume/test_connection.py
View File

@ -158,7 +158,7 @@ class VolumeConnectionTestCase(base.BaseVolumeTestCase):
mock.patch.object(cinder.tests.fake_driver.FakeLoggingVolumeDriver,
'initialize_connection') as driver_init:
type_qos.return_value = dict(qos_specs=qos_values)
type_extra_specs.return_value = 'True'
type_extra_specs.return_value = {}
driver_init.return_value = {'data': {}}
mock_get_target.return_value = None
qos_specs_expected = {'key1': 'value1',
@ -228,7 +228,7 @@ class VolumeConnectionTestCase(base.BaseVolumeTestCase):
mock.patch.object(cinder.tests.fake_driver.FakeLoggingVolumeDriver,
'initialize_connection') as driver_init:
type_qos.return_value = dict(qos_specs=qos_values)
type_extra_specs.return_value = 'True'
type_extra_specs.return_value = {}
driver_init.return_value = {'data': {}}
mock_get_target.return_value = None
qos_specs_expected = {'write_iops_sec': 90,
@ -300,7 +300,7 @@ class VolumeConnectionTestCase(base.BaseVolumeTestCase):
mock.patch.object(cinder.tests.fake_driver.FakeLoggingVolumeDriver,
'initialize_connection') as driver_init:
type_qos.return_value = dict(qos_specs=qos_values)
type_extra_specs.return_value = 'True'
type_extra_specs.return_value = {}
driver_init.return_value = {'data': {}}
mock_get_target.return_value = None
qos_specs_expected = {'write_iops_sec': 800,
@ -372,7 +372,7 @@ class VolumeConnectionTestCase(base.BaseVolumeTestCase):
mock.patch.object(cinder.tests.fake_driver.FakeLoggingVolumeDriver,
'initialize_connection') as driver_init:
type_qos.return_value = dict(qos_specs=qos_values)
type_extra_specs.return_value = 'True'
type_extra_specs.return_value = {}
driver_init.return_value = {'data': {}}
mock_get_target.return_value = None
qos_specs_expected = {'write_iops_sec': 3000,

8
cinder/tests/unit/volume/test_volume_manager.py
View File

@ -264,14 +264,14 @@ class VolumeManagerTestCase(base.BaseVolumeTestCase):
# no 'cacheable' set by driver, should be extra spec
conn_info = {"data": {}}
mock_get_extra_specs.return_value = '<is> True'
mock_get_extra_specs.return_value = {'cacheable': '<is> True'}
manager._parse_connection_options(ctxt, vol, conn_info)
self.assertIn('cacheable', conn_info['data'])
self.assertIs(conn_info['data']['cacheable'], True)
# driver sets 'cacheable' False, should override extra spec
conn_info = {"data": {"cacheable": False}}
mock_get_extra_specs.return_value = '<is> True'
mock_get_extra_specs.return_value = {'cacheable': '<is> True'}
manager._parse_connection_options(ctxt, vol, conn_info)
self.assertIn('cacheable', conn_info['data'])
self.assertIs(conn_info['data']['cacheable'], False)
@ -279,7 +279,7 @@ class VolumeManagerTestCase(base.BaseVolumeTestCase):
# driver sets 'cacheable' True, nothing in extra spec,
# extra spec should override driver
conn_info = {"data": {"cacheable": True}}
mock_get_extra_specs.return_value = None
mock_get_extra_specs.return_value = {}
manager._parse_connection_options(ctxt, vol, conn_info)
self.assertIn('cacheable', conn_info['data'])
self.assertIs(conn_info['data']['cacheable'], False)
@ -287,7 +287,7 @@ class VolumeManagerTestCase(base.BaseVolumeTestCase):
# driver sets 'cacheable' True, extra spec has False,
# extra spec should override driver
conn_info = {"data": {"cacheable": True}}
mock_get_extra_specs.return_value = '<is> False'
mock_get_extra_specs.return_value = {'cacheable': '<is> False'}
manager._parse_connection_options(ctxt, vol, conn_info)
self.assertIn('cacheable', conn_info['data'])
self.assertIs(conn_info['data']['cacheable'], False)

5
cinder/volume/drivers/vmware/vmdk.py
View File

@ -194,8 +194,9 @@ def _get_volume_type_extra_spec(type_id, spec_key, possible_values=None,
return default_value
spec_key = ('vmware:%s') % spec_key
spec_value = volume_types.get_volume_type_extra_specs(type_id,
spec_key)
spec_value = volume_types.get_volume_type_extra_specs(type_id).get(
spec_key, False)
if not spec_value:
LOG.debug("Returning default spec value: %s.", default_value)
return default_value

2
cinder/volume/manager.py
View File

@ -1872,7 +1872,7 @@ class VolumeManager(manager.CleanableManager,
# Add cacheable flag to connection_info if not set in the driver.
if typeid:
cacheable = volume_types.get_volume_type_extra_specs(
typeid, key='cacheable')
typeid).get('cacheable')
if conn_info['data'].get('cacheable') is not None:
driver_setting = bool(conn_info['data']['cacheable'])
# override a True driver_setting but respect False

11
cinder/volume/volume_types.py
View File

@ -244,17 +244,10 @@ def get_default_volume_type(
def get_volume_type_extra_specs(
volume_type_id: str,
key: Union[str, bool] = False) -> Union[dict, bool]:
) -> dict:
volume_type = get_volume_type(context.get_admin_context(),
volume_type_id)
extra_specs = volume_type['extra_specs']
if key:
if extra_specs.get(key):
return extra_specs.get(key)
else:
return False
else:
return extra_specs
return volume_type['extra_specs']
def is_public_volume_type(context: context.RequestContext,