From bf23678fe0f8583f80dc216ecbe645debb044220 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 23 Nov 2021 17:27:24 +0100 Subject: [PATCH] Honor multipath config everywhere There are multiple places in Cinder where the os-brick call to get the connector properties is not passing the "multipath" and "enforce_multipath" configuration options. This means that some driver won't return a multipathed connection information on the initialize_connection RPC call, resulting in a single pathed attachment even when the os-brick connector is instantiated with the right configuration option. This patch fixes the different calls where this is an issue: - Backup create - Backup restore - Kaminario create volume from snapshot - Kaminario create volume from volume - Volume rekeying And also in places where this is not affecting operations because drivers are always returning multipathed information: - Unity driver - IBM FlashSystem Closes-Bug: #1951982 Closes-Bug: #1951977 Closes-Bug: #1951981 Change-Id: I73ab87b5aaa4835a814389bc1cdd8016d75f52ef --- cinder/backup/manager.py | 6 ++++-- cinder/tests/unit/backup/test_backup.py | 6 +++--- .../volume/drivers/dell_emc/unity/test_adapter.py | 4 +++- cinder/tests/unit/volume/drivers/test_kaminario.py | 2 ++ cinder/volume/drivers/dell_emc/unity/adapter.py | 4 +++- cinder/volume/drivers/ibm/flashsystem_common.py | 4 +++- .../volume/drivers/kaminario/kaminario_common.py | 8 ++++++-- cinder/volume/flows/manager/create_volume.py | 9 ++++++++- .../use-multipath-everywhere-3707593eebdaf9eb.yaml | 14 ++++++++++++++ 9 files changed, 46 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/use-multipath-everywhere-3707593eebdaf9eb.yaml diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py index 3401c5f26a1..92d482434f3 100644 --- a/cinder/backup/manager.py +++ b/cinder/backup/manager.py @@ -456,7 +456,8 @@ class BackupManager(manager.SchedulerDependentManager): previous_status = volume.get('previous_status', None) backup_service = self.service(context) - properties = volume_utils.brick_get_connector_properties() + properties = volume_utils.brick_get_connector_properties( + CONF.use_multipath_for_image_xfer, enforce_multipath=False) updates = {} try: @@ -724,7 +725,8 @@ class BackupManager(manager.SchedulerDependentManager): orig_key_id = volume.encryption_key_id backup_service = self.service(context) - properties = volume_utils.brick_get_connector_properties() + properties = volume_utils.brick_get_connector_properties( + CONF.use_multipath_for_image_xfer, enforce_multipath=False) secure_enabled = ( self.volume_rpcapi.secure_file_operations_enabled(context, volume)) diff --git a/cinder/tests/unit/backup/test_backup.py b/cinder/tests/unit/backup/test_backup.py index 2cc786ac41c..40a5df826a0 100644 --- a/cinder/tests/unit/backup/test_backup.py +++ b/cinder/tests/unit/backup/test_backup.py @@ -729,7 +729,7 @@ class BackupTestCase(BaseBackupTest): mock_attach_device.assert_called_once_with(self.ctxt, vol, properties, False) mock_get_backup_device.assert_called_once_with(self.ctxt, backup, vol) - mock_get_conn.assert_called_once_with() + mock_get_conn.assert_called_once_with(False, enforce_multipath=False) mock_detach_device.assert_called_once_with(self.ctxt, attach_info, vol, properties, False, force=True, @@ -977,7 +977,7 @@ class BackupTestCase(BaseBackupTest): mock_initialize_connection_snapshot.assert_called_once_with( self.ctxt, snap, properties) mock_get_backup_device.assert_called_once_with(self.ctxt, backup, vol) - mock_get_conn.assert_called_once_with() + mock_get_conn.assert_called_once_with(False, enforce_multipath=False) mock_terminate_connection_snapshot.assert_called_once_with( self.ctxt, snap, properties, force=True) mock_remove_export_snapshot.assert_called_once_with( @@ -1282,7 +1282,7 @@ class BackupTestCase(BaseBackupTest): mock_open.assert_called_once_with('/dev/null', exp_open_mode) mock_temporary_chown.assert_called_once_with('/dev/null') - mock_get_conn.assert_called_once_with() + mock_get_conn.assert_called_once_with(False, enforce_multipath=False) vol.status = 'available' vol.obj_reset_changes() mock_secure_enabled.assert_called_once_with(self.ctxt, vol) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/unity/test_adapter.py b/cinder/tests/unit/volume/drivers/dell_emc/unity/test_adapter.py index 0edb645b06b..31535a85483 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/unity/test_adapter.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/unity/test_adapter.py @@ -51,6 +51,8 @@ class MockConfig(object): self.driver_ssl_cert_verify = True self.driver_ssl_cert_path = None self.remove_empty_host = False + self.use_multipath_for_image_xfer = False + self.enforce_multipath_for_image_xfer = False def safe_get(self, name): return getattr(self, name) @@ -391,7 +393,7 @@ def get_backend_qos_specs(volume): return None -def get_connector_properties(): +def get_connector_properties(use_multipath, enforce_multipath): return {'host': 'host1', 'wwpns': 'abcdefg'} diff --git a/cinder/tests/unit/volume/drivers/test_kaminario.py b/cinder/tests/unit/volume/drivers/test_kaminario.py index dd1833755e7..b228ece1af3 100644 --- a/cinder/tests/unit/volume/drivers/test_kaminario.py +++ b/cinder/tests/unit/volume/drivers/test_kaminario.py @@ -143,6 +143,8 @@ class TestKaminarioCommon(test.TestCase): self.conf.volume_dd_blocksize = 2 self.conf.disable_discovery = False self.conf.unique_fqdn_network = True + self.conf.use_multipath_for_image_xfer = False + self.conf.enforce_multipath_for_image_xfer = False def _setup_driver(self): self.driver = (kaminario_iscsi. diff --git a/cinder/volume/drivers/dell_emc/unity/adapter.py b/cinder/volume/drivers/dell_emc/unity/adapter.py index 93a8eb7d95f..a77d604aece 100644 --- a/cinder/volume/drivers/dell_emc/unity/adapter.py +++ b/cinder/volume/drivers/dell_emc/unity/adapter.py @@ -813,7 +813,9 @@ class CommonAdapter(object): is_compressed=vol_params.is_compressed) src_id = src_snap.get_id() try: - conn_props = volume_utils.brick_get_connector_properties() + conn_props = volume_utils.brick_get_connector_properties( + self.config.use_multipath_for_image_xfer, + self.config.enforce_multipath_for_image_xfer) with self._connect_resource(dest_lun, conn_props, vol_params.volume_id) as dest_info, \ diff --git a/cinder/volume/drivers/ibm/flashsystem_common.py b/cinder/volume/drivers/ibm/flashsystem_common.py index e27ea7034e4..bfb16b9e3f3 100644 --- a/cinder/volume/drivers/ibm/flashsystem_common.py +++ b/cinder/volume/drivers/ibm/flashsystem_common.py @@ -219,7 +219,9 @@ class FlashSystemDriver(san.SanDriver, LOG.debug('enter: _copy_vdisk_data: %(src)s -> %(dest)s.', {'src': src_vdisk_name, 'dest': dest_vdisk_name}) - connector = volume_utils.brick_get_connector_properties() + connector = volume_utils.brick_get_connector_properties( + self.configuration.use_multipath_for_image_xfer, + self.configuration.enforce_multipath_for_image_xfer) (src_map, src_lun_id) = self._is_vdisk_map( src_vdisk_name, connector) (dest_map, dest_lun_id) = self._is_vdisk_map( diff --git a/cinder/volume/drivers/kaminario/kaminario_common.py b/cinder/volume/drivers/kaminario/kaminario_common.py index 498e9efc9d4..d86c1f6bfd9 100644 --- a/cinder/volume/drivers/kaminario/kaminario_common.py +++ b/cinder/volume/drivers/kaminario/kaminario_common.py @@ -553,7 +553,9 @@ class KaminarioCinderDriver(cinder.volume.driver.ISCSIDriver): vol_name = self.get_volume_name(volume.id) cview = src_attach_info = dest_attach_info = None rpolicy = self.get_policy() - properties = volume_utils.brick_get_connector_properties() + properties = volume_utils.brick_get_connector_properties( + self.configuration.use_multipath_for_image_xfer, + self.configuration.enforce_multipath_for_image_xfer) LOG.debug("Searching for snapshot: %s in K2.", snap_name) snap_rs = self.client.search("snapshots", short_name=snap_name) if hasattr(snap_rs, 'hits') and snap_rs.total != 0: @@ -624,7 +626,9 @@ class KaminarioCinderDriver(cinder.volume.driver.ISCSIDriver): LOG.error(msg) raise KaminarioCinderDriverException(reason=msg) try: - properties = volume_utils.brick_get_connector_properties() + properties = volume_utils.brick_get_connector_properties( + self.configuration.use_multipath_for_image_xfer, + self.configuration.enforce_multipath_for_image_xfer) conn = self.initialize_connection(src_vref, properties) src_attach_info = self._connect_device(conn) self.create_volume(volume) diff --git a/cinder/volume/flows/manager/create_volume.py b/cinder/volume/flows/manager/create_volume.py index 0c3a53d7fc8..0cd71786077 100644 --- a/cinder/volume/flows/manager/create_volume.py +++ b/cinder/volume/flows/manager/create_volume.py @@ -523,7 +523,14 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): LOG.debug('rekey volume %s', volume.name) - properties = volume_utils.brick_get_connector_properties(False, False) + # Rekeying writes very little data (KB), so doing a single pathed + # connection is more efficient, but it is less robust, because the + # driver could be returning a single path, and it happens to be down + # then the attach would fail where the multipathed connection would + # have succeeded. + properties = volume_utils.brick_get_connector_properties( + self.driver.configuration.use_multipath_for_image_xfer, + self.driver.configuration.enforce_multipath_for_image_xfer) LOG.debug("properties: %s", properties) attach_info = None model_update = {} diff --git a/releasenotes/notes/use-multipath-everywhere-3707593eebdaf9eb.yaml b/releasenotes/notes/use-multipath-everywhere-3707593eebdaf9eb.yaml new file mode 100644 index 00000000000..13a981ace12 --- /dev/null +++ b/releasenotes/notes/use-multipath-everywhere-3707593eebdaf9eb.yaml @@ -0,0 +1,14 @@ +--- +fixes: + - | + `Bug #1951982 `_: Fixed + cloning of encrypted volumes not using multipathing to change the + encryption key used on the new volume. + - | + `Bug #1951977 `_: Fixed + backup create and restore not using multipath configuration when attaching + the volume. + - | + Kaminario driver `bug #1951981 + `_: Fixed + create volume from volume or snapshot not using multipath configuration.