From 0c3c7b7bb501373b6d125c2810fbbdf69b7ee162 Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Mon, 4 Nov 2024 03:19:34 +0530 Subject: [PATCH] Add 'enforce_multipath' in connection_properties Previously, the check for multipath used to happen while fetching connector information. The downside of this was we didn't know what kind of connection (iSCSI, FC, NVMe) will be created and it was not possible to handle the multipath checks for different connectors. With change[1], the check was moved to the connect_volume method but that led to another problem that the connection information returned from cinder didn't contain the ``enforce_multipath`` parameter. The purpose of this patch is to include the ``enforce_multipath`` parameter in the connection information based on the value fetched from the connector. [1] https://review.opendev.org/c/openstack/os-brick/+/933250 Partial-Bug: #2085013 Change-Id: I4ea618a44b37cce5dd0b131ffac55bce51d08c0a --- cinder/api/contrib/volume_actions.py | 1 + .../unit/api/contrib/test_volume_actions.py | 8 +++- .../attachments/test_attachments_manager.py | 15 +++++-- cinder/tests/unit/volume/test_driver.py | 44 ++++++++++++++++++- cinder/volume/driver.py | 4 ++ cinder/volume/manager.py | 7 +++ 6 files changed, 72 insertions(+), 7 deletions(-) diff --git a/cinder/api/contrib/volume_actions.py b/cinder/api/contrib/volume_actions.py index 8b80a5edc02..0bb7ed1b875 100644 --- a/cinder/api/contrib/volume_actions.py +++ b/cinder/api/contrib/volume_actions.py @@ -172,6 +172,7 @@ class VolumeActionsController(wsgi.Controller): if error.exc_type == 'InvalidInput': raise exception.InvalidInput(reason=error.value) raise + info['enforce_multipath'] = connector.get('enforce_multipath', False) return {'connection_info': info} diff --git a/cinder/tests/unit/api/contrib/test_volume_actions.py b/cinder/tests/unit/api/contrib/test_volume_actions.py index 90a933eabda..acba239deed 100644 --- a/cinder/tests/unit/api/contrib/test_volume_actions.py +++ b/cinder/tests/unit/api/contrib/test_volume_actions.py @@ -97,12 +97,14 @@ class VolumeActionsTest(test.TestCase): res = req.get_response(app) self.assertEqual(HTTPStatus.ACCEPTED, res.status_int) - def test_initialize_connection(self): + @ddt.data(False, True) + def test_initialize_connection(self, enforce_mpath): with mock.patch.object(volume_api.API, 'initialize_connection') as init_conn: init_conn.return_value = {} body = {'os-initialize_connection': {'connector': { - 'fake': 'fake'}}} + 'fake': 'fake', 'enforce_multipath': enforce_mpath}}} + expected_conn_info = {'enforce_multipath': enforce_mpath} req = webob.Request.blank('/v3/%s/volumes/%s/action' % (fake.PROJECT_ID, fake.VOLUME_ID)) req.method = "POST" @@ -111,7 +113,9 @@ class VolumeActionsTest(test.TestCase): res = req.get_response(fakes.wsgi_app( fake_auth_context=self.context)) + actual_conn_info = jsonutils.loads(res.body).get('connection_info') self.assertEqual(HTTPStatus.OK, res.status_int) + self.assertEqual(expected_conn_info, actual_conn_info) def test_initialize_connection_without_connector(self): with mock.patch.object(volume_api.API, diff --git a/cinder/tests/unit/attachments/test_attachments_manager.py b/cinder/tests/unit/attachments/test_attachments_manager.py index 3fc1b132a42..7b4a4bd07ef 100644 --- a/cinder/tests/unit/attachments/test_attachments_manager.py +++ b/cinder/tests/unit/attachments/test_attachments_manager.py @@ -12,6 +12,7 @@ from unittest import mock +import ddt from oslo_config import cfg from oslo_utils import importutils @@ -28,6 +29,7 @@ from cinder.volume import configuration as conf CONF = cfg.CONF +@ddt.ddt class AttachmentManagerTestCase(test.TestCase): """Attachment related test for volume.manager.py.""" @@ -46,11 +48,13 @@ class AttachmentManagerTestCase(test.TestCase): self.manager.stats = {'allocated_capacity_gb': 100, 'pools': {}} + @ddt.data(False, True) @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') @mock.patch('cinder.volume.volume_types.get_volume_type_extra_specs') - def test_attachment_update(self, get_extra_specs, mock_type_get): + def test_attachment_update(self, enforce_mpath, get_extra_specs, + mock_type_get): """Test attachment_update.""" volume_params = {'status': 'available'} connector = { @@ -59,7 +63,8 @@ class AttachmentManagerTestCase(test.TestCase): "platform": "x86_64", "host": "tempest-1", "os_type": "linux2", - "multipath": False} + "multipath": False, + "enforce_multipath": enforce_mpath} vref = tests_utils.create_volume(self.context, **volume_params) self.manager.create_volume(self.context, vref) @@ -77,7 +82,8 @@ class AttachmentManagerTestCase(test.TestCase): 'cacheable': False, 'access_mode': 'rw', 'driver_volume_type': 'iscsi', - 'attachment_id': attachment_ref.id} + 'attachment_id': attachment_ref.id, + 'enforce_multipath': enforce_mpath} get_extra_specs.return_value = {} self.assertEqual(expected, @@ -92,7 +98,8 @@ class AttachmentManagerTestCase(test.TestCase): 'cacheable': True, 'access_mode': 'rw', 'driver_volume_type': 'iscsi', - 'attachment_id': attachment_ref.id} + 'attachment_id': attachment_ref.id, + 'enforce_multipath': enforce_mpath} get_extra_specs.return_value = {'cacheable': ' True'} self.assertEqual(expected, diff --git a/cinder/tests/unit/volume/test_driver.py b/cinder/tests/unit/volume/test_driver.py index 0285bdf31e0..815a78cb160 100644 --- a/cinder/tests/unit/volume/test_driver.py +++ b/cinder/tests/unit/volume/test_driver.py @@ -383,6 +383,47 @@ class GenericVolumeDriverTestCase(BaseDriverTestCase): db.volume_destroy(self.context, src_vol['id']) db.volume_destroy(self.context, dest_vol['id']) + @ddt.data(False, True) + @mock.patch(driver_name + '.initialize_connection') + @mock.patch(driver_name + '.create_export', return_value=None) + @mock.patch(driver_name + '._connect_device') + def test_attach_volume_enforce_mpath(self, enforce_mpath, connect_mock, + export_mock, initialize_mock): + properties = {'host': 'myhost', 'ip': '192.168.1.43', + 'initiator': u'iqn.1994-05.com.redhat:d9be887375', + 'multipath': False, 'os_type': 'linux2', + 'platform': 'x86_64', + 'enforce_multipath': enforce_mpath} + + data = {'target_discovered': True, + 'target_iqn': 'iqn.2010-10.org.openstack:volume-00000001', + 'target_portal': '127.0.0.0.1:3260', + 'volume_id': 1, + 'discard': False, + 'encrypted': False, + 'enforce_multipath': enforce_mpath} + + passed_conn = {'driver_volume_type': 'iscsi', 'data': data.copy()} + initialize_mock.return_value = passed_conn + + expected_conn = {'driver_volume_type': 'iscsi', 'data': data.copy()} + + volume = tests_utils.create_volume( + self.context, status='available', + size=2) + + attach_info, vol = self.volume.driver._attach_volume(self.context, + volume, + properties) + + export_mock.assert_called_once_with(self.context, volume, properties) + initialize_mock.assert_called_once_with(volume, properties) + + connect_mock.assert_called_once_with(expected_conn) + + self.assertEqual(connect_mock.return_value, attach_info) + self.assertEqual(volume, vol) + @mock.patch(driver_name + '.initialize_connection') @mock.patch(driver_name + '.create_export', return_value=None) @mock.patch(driver_name + '._connect_device') @@ -397,7 +438,8 @@ class GenericVolumeDriverTestCase(BaseDriverTestCase): 'target_iqn': 'iqn.2010-10.org.openstack:volume-00000001', 'target_portal': '127.0.0.0.1:3260', 'volume_id': 1, - 'discard': False} + 'discard': False, + 'enforce_multipath': False} passed_conn = {'driver_volume_type': 'iscsi', 'data': data.copy()} initialize_mock.return_value = passed_conn diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 23b8a8a59fe..2bbe9e8ca5a 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -1087,6 +1087,10 @@ class BaseVD(object, metaclass=abc.ABCMeta): encrypted = bool(volume.encryption_key_id) conn['data']['encrypted'] = encrypted + # Append the enforce_multipath value if the connector has it + conn['data']['enforce_multipath'] = properties.get( + 'enforce_multipath', False) + try: attach_info = self._connect_device(conn) except Exception as exc: diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 30de5d8c771..d691710ddd1 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -1980,6 +1980,8 @@ class VolumeManager(manager.CleanableManager, raise exception.VolumeBackendAPIException(data=err_msg) conn_info = self._parse_connection_options(context, volume, conn_info) + conn_info['data']['enforce_multipath'] = connector.get( + 'enforce_multipath', False) LOG.info("Initialize volume connection completed successfully.", resource=volume) return conn_info @@ -2040,6 +2042,8 @@ class VolumeManager(manager.CleanableManager, LOG.error(ex_msg) raise exception.VolumeBackendAPIException(data=ex_msg) raise exception.VolumeBackendAPIException(data=err_msg) + conn['data']['enforce_multipath'] = connector.get( + 'enforce_multipath', False) LOG.info("Initialize snapshot connection completed successfully.", resource=snapshot) @@ -4851,6 +4855,9 @@ class VolumeManager(manager.CleanableManager, self.db.volume_attachment_update(ctxt, attachment.id, values) connection_info['attachment_id'] = attachment.id + # Append the enforce_multipath value if the connector has it + connection_info['enforce_multipath'] = connector.get( + 'enforce_multipath', False) LOG.debug("Connection info returned from driver %(connection_info)s", {'connection_info': strutils.mask_dict_password(connection_info)})