From afcaf0b9dd7ca7b8eb8d85f4ae44dceca4af5240 Mon Sep 17 00:00:00 2001 From: Liang Fang Date: Sat, 11 Apr 2020 09:42:49 +0000 Subject: [PATCH] Add support volume local cache Use volume type extra spec 'cacheable' to identify if a volume can be cached or not. If it is set to ' True', then it means the volume can be cached in compute node locally via caching software (e.g. open-cas) open-cas: https://open-cas.github.io/ Nova Spec: https://review.opendev.org/#/c/689070/ Cinder Spec: https://review.opendev.org/#/c/684556/ Change-Id: I61a795f4ab2c4208094245bd577c39de1b35d6ef Signed-off-by: Liang Fang --- cinder/api/contrib/types_extra_specs.py | 22 ++++++++ .../api/contrib/test_types_extra_specs.py | 53 +++++++++++++++++++ .../attachments/test_attachments_manager.py | 20 ++++++- cinder/tests/unit/volume/test_connection.py | 16 ++++++ cinder/tests/unit/volume/test_volume.py | 6 ++- .../tests/unit/volume/test_volume_manager.py | 42 +++++++++++++++ cinder/volume/driver.py | 7 +++ cinder/volume/manager.py | 24 +++++++++ 8 files changed, 188 insertions(+), 2 deletions(-) diff --git a/cinder/api/contrib/types_extra_specs.py b/cinder/api/contrib/types_extra_specs.py index f313bd00acf..b9d5a4981d7 100644 --- a/cinder/api/contrib/types_extra_specs.py +++ b/cinder/api/contrib/types_extra_specs.py @@ -63,6 +63,20 @@ class VolumeTypeExtraSpecsController(wsgi.Controller): raise webob.exc.HTTPBadRequest(explanation=expl) 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') + isTrue = ' True' + if (specs.get('multiattach') == isTrue and cacheable == isTrue) or ( + specs.get('cacheable') == isTrue and multiattach == + isTrue) or (specs.get('cacheable') == isTrue and + specs.get('multiattach') == isTrue): + expl = _('cacheable cannot be set with multiattach.') + raise webob.exc.HTTPBadRequest(explanation=expl) + return + @validation.schema(types_extra_specs.create) def create(self, req, type_id, body): context = req.environ['cinder.context'] @@ -76,6 +90,9 @@ class VolumeTypeExtraSpecsController(wsgi.Controller): image_service_store_id = specs['image_service:store_id'] image_utils.validate_stores_id(context, image_service_store_id) + # Check if multiattach be set with cacheable + self._check_cacheable(specs, type_id) + db.volume_type_extra_specs_update_or_create(context, type_id, specs) @@ -104,6 +121,11 @@ class VolumeTypeExtraSpecsController(wsgi.Controller): image_service_store_id = body['image_service:store_id'] image_utils.validate_stores_id(context, image_service_store_id) + if 'extra_specs' in body: + specs = body['extra_specs'] + # Check if multiattach be set with cacheable + self._check_cacheable(specs, type_id) + db.volume_type_extra_specs_update_or_create(context, type_id, body) 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 0df01bdb8a3..f36045e743c 100644 --- a/cinder/tests/unit/api/contrib/test_types_extra_specs.py +++ b/cinder/tests/unit/api/contrib/test_types_extra_specs.py @@ -454,3 +454,56 @@ class VolumeTypesExtraSpecsTest(test.TestCase): self.assertRaises(exception.ValidationError, self.controller.create, req, fake.VOLUME_ID, body=body) + + @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 + + specs = {'multiattach': ' True', + 'cacheable': ' True'} + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._check_cacheable, + specs, 'typeid') + + ret_multiattach = ' True' + ret_cacheable = '' + specs = {'cacheable': ' True'} + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._check_cacheable, + specs, 'typeid') + + ret_multiattach = '' + ret_cacheable = ' True' + specs = {'multiattach': ' True'} + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._check_cacheable, + specs, 'typeid') + + ret_multiattach = ' False' + ret_cacheable = '' + specs = {'multiattach': ' True'} + # Should NOT has exception when calling below line + self.controller._check_cacheable(specs, 'typeid') + + ret_multiattach = ' True' + ret_cacheable = '' + 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 = '' + 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 a09dfc99bae..fa501da7671 100644 --- a/cinder/tests/unit/attachments/test_attachments_manager.py +++ b/cinder/tests/unit/attachments/test_attachments_manager.py @@ -48,7 +48,8 @@ class AttachmentManagerTestCase(test.TestCase): @mock.patch.object(db.sqlalchemy.api, '_volume_type_get', v2_fakes.fake_volume_type_get) @mock.patch('cinder.db.sqlalchemy.api.volume_type_qos_specs_get') - def test_attachment_update(self, mock_type_get): + @mock.patch('cinder.volume.volume_types.get_volume_type_extra_specs') + def test_attachment_update(self, get_extra_specs, mock_type_get): """Test attachment_update.""" volume_params = {'status': 'available'} connector = { @@ -74,10 +75,12 @@ class AttachmentManagerTestCase(test.TestCase): expected = { 'encrypted': False, 'qos_specs': None, + 'cacheable': False, 'access_mode': 'rw', 'driver_volume_type': 'iscsi', 'attachment_id': attachment_ref.id} + get_extra_specs.return_value = '' self.assertEqual(expected, self.manager.attachment_update( self.context, @@ -89,6 +92,21 @@ class AttachmentManagerTestCase(test.TestCase): attachment_ref.instance_uuid, connector['host'], "na") + expected = { + 'encrypted': False, + 'qos_specs': None, + 'cacheable': True, + 'access_mode': 'rw', + 'driver_volume_type': 'iscsi', + 'attachment_id': attachment_ref.id} + + get_extra_specs.return_value = ' True' + self.assertEqual(expected, + self.manager.attachment_update( + self.context, + vref, + connector, + attachment_ref.id)) new_attachment_ref = db.volume_attachment_get(self.context, attachment_ref.id) diff --git a/cinder/tests/unit/volume/test_connection.py b/cinder/tests/unit/volume/test_connection.py index 420057cf980..0d472d6228b 100644 --- a/cinder/tests/unit/volume/test_connection.py +++ b/cinder/tests/unit/volume/test_connection.py @@ -152,9 +152,13 @@ class VolumeConnectionTestCase(base.BaseVolumeTestCase): with mock.patch.object(cinder.volume.volume_types, 'get_volume_type_qos_specs') as type_qos, \ + mock.patch.object(cinder.volume.volume_types, + 'get_volume_type_extra_specs' + ) as type_extra_specs, \ 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' driver_init.return_value = {'data': {}} mock_get_target.return_value = None qos_specs_expected = {'key1': 'value1', @@ -218,9 +222,13 @@ class VolumeConnectionTestCase(base.BaseVolumeTestCase): with mock.patch.object(cinder.volume.volume_types, 'get_volume_type_qos_specs') as type_qos, \ + mock.patch.object(cinder.volume.volume_types, + 'get_volume_type_extra_specs' + ) as type_extra_specs, \ 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' driver_init.return_value = {'data': {}} mock_get_target.return_value = None qos_specs_expected = {'write_iops_sec': 90, @@ -286,9 +294,13 @@ class VolumeConnectionTestCase(base.BaseVolumeTestCase): with mock.patch.object(cinder.volume.volume_types, 'get_volume_type_qos_specs') as type_qos, \ + mock.patch.object(cinder.volume.volume_types, + 'get_volume_type_extra_specs' + ) as type_extra_specs, \ 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' driver_init.return_value = {'data': {}} mock_get_target.return_value = None qos_specs_expected = {'write_iops_sec': 800, @@ -354,9 +366,13 @@ class VolumeConnectionTestCase(base.BaseVolumeTestCase): with mock.patch.object(cinder.volume.volume_types, 'get_volume_type_qos_specs') as type_qos, \ + mock.patch.object(cinder.volume.volume_types, + 'get_volume_type_extra_specs' + ) as type_extra_specs, \ 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' 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.py b/cinder/tests/unit/volume/test_volume.py index 227cf04c4d4..4468822d2d5 100644 --- a/cinder/tests/unit/volume/test_volume.py +++ b/cinder/tests/unit/volume/test_volume.py @@ -172,6 +172,8 @@ class VolumeTestCase(base.BaseVolumeTestCase): myfilterfunction = "myFilterFunction" mygoodnessfunction = "myGoodnessFunction" expected = {'name': 'cinder-volumes', + 'storage_protocol': 'iSCSI', + 'cacheable': True, 'filter_function': myfilterfunction, 'goodness_function': mygoodnessfunction, } @@ -181,7 +183,9 @@ class VolumeTestCase(base.BaseVolumeTestCase): 'get_goodness_function') as m_get_goodness: with mock.patch.object(manager.driver, 'get_filter_function') as m_get_filter: - m_get_stats.return_value = {'name': 'cinder-volumes'} + m_get_stats.return_value = {'name': 'cinder-volumes', + 'storage_protocol': 'iSCSI', + } m_get_filter.return_value = myfilterfunction m_get_goodness.return_value = mygoodnessfunction manager._report_driver_status(context.get_admin_context()) diff --git a/cinder/tests/unit/volume/test_volume_manager.py b/cinder/tests/unit/volume/test_volume_manager.py index 0a715311588..ef1ce48c8a9 100644 --- a/cinder/tests/unit/volume/test_volume_manager.py +++ b/cinder/tests/unit/volume/test_volume_manager.py @@ -18,6 +18,7 @@ from unittest import mock from cinder import exception from cinder.message import message_field +from cinder.tests.unit import fake_constants as fake from cinder.tests.unit import fake_volume from cinder.tests.unit import volume as base from cinder.volume import manager as vol_manager @@ -245,3 +246,44 @@ class VolumeManagerTestCase(base.BaseVolumeTestCase): mock_detach.assert_called_once_with( ctxt, mock_connect.return_value, vol, mock.sentinel.properties, force=True, remote=mock.sentinel.remote) + + @mock.patch('cinder.volume.volume_types.get_volume_type_extra_specs') + @mock.patch('cinder.volume.volume_types.get_volume_type_qos_specs', + return_value={'qos_specs': None}) + def test_parse_connection_options_cacheable(self, + mock_get_qos, + mock_get_extra_specs): + ctxt = mock.Mock() + manager = vol_manager.VolumeManager() + vol = fake_volume.fake_volume_obj(ctxt) + vol.volume_type_id = fake.VOLUME_TYPE_ID + + # no 'cacheable' set by driver, should be extra spec + conn_info = {"data": {}} + mock_get_extra_specs.return_value = ' 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' + manager._parse_connection_options(ctxt, vol, conn_info) + self.assertIn('cacheable', conn_info['data']) + self.assertIs(conn_info['data']['cacheable'], False) + + # 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 + manager._parse_connection_options(ctxt, vol, conn_info) + self.assertIn('cacheable', conn_info['data']) + self.assertIs(conn_info['data']['cacheable'], False) + + # 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' + 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/driver.py b/cinder/volume/driver.py index c081bcf6f65..d21473430c3 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -1464,6 +1464,13 @@ class BaseVD(object): def initialize_connection(self, volume, connector): """Allow connection to connector and return connection info. + ..note:: + Whether or not a volume is 'cacheable' for volume local cache on + the hypervisor is normally configured in the volume-type + extra-specs. Support may be disabled at the driver level, however, + by returning "cacheable": False in the conn_info. This will + override any setting in the volume-type extra-specs. + :param volume: The volume to be attached :param connector: Dictionary containing information about what is being connected to. diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 0fd4d4e566b..e7ec93f33b4 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -1760,6 +1760,18 @@ class VolumeManager(manager.CleanableManager, encrypted = bool(volume.encryption_key_id) conn_info['data']['encrypted'] = encrypted + # 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') + if conn_info['data'].get('cacheable') is not None: + driver_setting = bool(conn_info['data']['cacheable']) + # override a True driver_setting but respect False + conn_info['data']['cacheable'] = (driver_setting and + (cacheable == ' True')) + else: + conn_info['data']['cacheable'] = (cacheable == ' True') + # Add discard flag to connection_info if not set in the driver and # configured to be reported. if conn_info['data'].get('discard') is None: @@ -2613,6 +2625,18 @@ class VolumeManager(manager.CleanableManager, # Append volume stats with 'allocated_capacity_gb' self._append_volume_stats(volume_stats) + # Append cacheable flag for iSCSI/FC/NVMe-oF and only when + # cacheable is not set in driver level + if volume_stats['storage_protocol'] in [ + 'iSCSI', 'FC', 'NVMe-oF']: + if volume_stats.get('pools'): + for pool in volume_stats.get('pools'): + if pool.get('cacheable') is None: + pool['cacheable'] = True + else: + if volume_stats.get('cacheable') is None: + volume_stats['cacheable'] = True + # Append filter and goodness function if needed volume_stats = ( self._append_filter_goodness_functions(volume_stats))