diff --git a/cinder/api/contrib/types_extra_specs.py b/cinder/api/contrib/types_extra_specs.py index a7790ee10c8..657c05abf7b 100644 --- a/cinder/api/contrib/types_extra_specs.py +++ b/cinder/api/contrib/types_extra_specs.py @@ -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 = ' True' if (specs.get('multiattach') == isTrue and cacheable == isTrue) or ( specs.get('cacheable') == isTrue and multiattach == diff --git a/cinder/tests/unit/api/contrib/test_types_extra_specs.py b/cinder/tests/unit/api/contrib/test_types_extra_specs.py index df3eed23c76..058d40d3797 100644 --- a/cinder/tests/unit/api/contrib/test_types_extra_specs.py +++ b/cinder/tests/unit/api/contrib/test_types_extra_specs.py @@ -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': ' True', 'cacheable': ' True'} self.assertRaises(webob.exc.HTTPBadRequest, self.controller._check_cacheable, specs, 'typeid') - ret_multiattach = ' True' - ret_cacheable = '' + get_extra_specs.return_value = {'multiattach': ' True'} specs = {'cacheable': ' True'} self.assertRaises(webob.exc.HTTPBadRequest, self.controller._check_cacheable, specs, 'typeid') - ret_multiattach = '' - ret_cacheable = ' True' + get_extra_specs.return_value = {'cacheable': ' True'} specs = {'multiattach': ' True'} self.assertRaises(webob.exc.HTTPBadRequest, self.controller._check_cacheable, specs, 'typeid') - ret_multiattach = ' False' - ret_cacheable = '' + get_extra_specs.return_value = {'multiattach': ' False'} specs = {'multiattach': ' True'} # Should NOT has exception when calling below line self.controller._check_cacheable(specs, 'typeid') - ret_multiattach = ' True' - ret_cacheable = '' + get_extra_specs.return_value = {'multiattach': ' True'} specs = {'multiattach': ' False', 'cacheable': ' True'} # Should NOT setting both at the same time self.assertRaises(webob.exc.HTTPBadRequest, self.controller._check_cacheable, specs, 'typeid') - ret_multiattach = ' False' - ret_cacheable = '' + get_extra_specs.return_value = {'multiattach': ' False'} specs = {'multiattach': ' False', 'cacheable': ' True'} # Should NOT has exception when calling below line self.controller._check_cacheable(specs, 'typeid') diff --git a/cinder/tests/unit/attachments/test_attachments_manager.py b/cinder/tests/unit/attachments/test_attachments_manager.py index ab722e1bf33..3fc1b132a42 100644 --- a/cinder/tests/unit/attachments/test_attachments_manager.py +++ b/cinder/tests/unit/attachments/test_attachments_manager.py @@ -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 = ' True' + get_extra_specs.return_value = {'cacheable': ' True'} self.assertEqual(expected, self.manager.attachment_update( self.context, diff --git a/cinder/tests/unit/volume/test_connection.py b/cinder/tests/unit/volume/test_connection.py index 6bd9b89b6f7..8fd96901306 100644 --- a/cinder/tests/unit/volume/test_connection.py +++ b/cinder/tests/unit/volume/test_connection.py @@ -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, diff --git a/cinder/tests/unit/volume/test_volume_manager.py b/cinder/tests/unit/volume/test_volume_manager.py index 9a7a50ff61f..b54bf4a802d 100644 --- a/cinder/tests/unit/volume/test_volume_manager.py +++ b/cinder/tests/unit/volume/test_volume_manager.py @@ -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 = ' True' + mock_get_extra_specs.return_value = {'cacheable': ' 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 = ' True' + mock_get_extra_specs.return_value = {'cacheable': ' 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 = ' False' + mock_get_extra_specs.return_value = {'cacheable': ' False'} manager._parse_connection_options(ctxt, vol, conn_info) self.assertIn('cacheable', conn_info['data']) self.assertIs(conn_info['data']['cacheable'], False) diff --git a/cinder/volume/drivers/vmware/vmdk.py b/cinder/volume/drivers/vmware/vmdk.py index 154410e7e10..44876023192 100644 --- a/cinder/volume/drivers/vmware/vmdk.py +++ b/cinder/volume/drivers/vmware/vmdk.py @@ -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 diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index e03767b946b..0f3ce10c56b 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -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 diff --git a/cinder/volume/volume_types.py b/cinder/volume/volume_types.py index a5da0ef05e4..16451230d34 100644 --- a/cinder/volume/volume_types.py +++ b/cinder/volume/volume_types.py @@ -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,