From 03ac369854597f6c886906c1f71561894b10aa04 Mon Sep 17 00:00:00 2001 From: Yian Zong Date: Thu, 30 Nov 2023 09:30:19 +0000 Subject: [PATCH] Dell PowerMax: Fix SnapVx unlink failure Dell PowerMax SnapVx unlink fails as the linked device is not yet fully defined. This patch fixed the issue by checking the new configuration 'snapvx_unlink_symforce' and the linked device 'defined' status to determine the value of 'symforce' in the payload of SnapVx unlink restcall. Closes-Bug: #2045230 Change-Id: I614f6aef2d4da76c417b4a143ab80e4a5f716dcd --- .../powermax/test_powermax_provision.py | 20 ++++--- .../drivers/dell_emc/powermax/common.py | 18 ++++-- .../drivers/dell_emc/powermax/masking.py | 5 +- .../drivers/dell_emc/powermax/migrate.py | 6 +- .../drivers/dell_emc/powermax/provision.py | 55 +++++++++++++++---- .../volume/drivers/dell_emc/powermax/rest.py | 4 +- .../volume/drivers/dell_emc/powermax/utils.py | 1 + .../drivers/dell-emc-powermax-driver.rst | 3 + ...ax-fix-snapvx-unlink-e27d67d6b217d706.yaml | 32 +++++++++++ 9 files changed, 116 insertions(+), 28 deletions(-) create mode 100644 releasenotes/notes/bug-2045230-dell-powermax-fix-snapvx-unlink-e27d67d6b217d706.yaml diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_provision.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_provision.py index e4d8ae9207c..6e755d630af 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_provision.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_provision.py @@ -171,7 +171,7 @@ class PowerMaxProvisionTest(test.TestCase): mock_unlink.assert_called_once_with( array, source_device_id, target_device_id, snap_name, extra_specs, snap_id=self.data.snap_id, - list_volume_pairs=None, loop=True) + list_volume_pairs=None, loop=True, symforce=False) @mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall', new=test_utils.ZeroIntervalLoopingCall) @@ -184,7 +184,8 @@ class PowerMaxProvisionTest(test.TestCase): mock_mod.assert_called_once_with( self.data.array, self.data.device_id, self.data.device_id2, self.data.snap_location['snap_name'], self.data.extra_specs, - snap_id=self.data.snap_id, list_volume_pairs=None, unlink=True) + snap_id=self.data.snap_id, list_volume_pairs=None, unlink=True, + symforce=False) mock_mod.reset_mock() self.provision._unlink_volume( @@ -194,7 +195,8 @@ class PowerMaxProvisionTest(test.TestCase): mock_mod.assert_called_once_with( self.data.array, self.data.device_id, self.data.device_id2, self.data.snap_location['snap_name'], self.data.extra_specs, - snap_id=self.data.snap_id, list_volume_pairs=None, unlink=True) + snap_id=self.data.snap_id, list_volume_pairs=None, unlink=True, + symforce=False) @mock.patch('oslo_service.loopingcall.FixedIntervalLoopingCall', new=test_utils.ZeroIntervalLoopingCall) @@ -556,9 +558,12 @@ class PowerMaxProvisionTest(test.TestCase): @mock.patch.object( rest.PowerMaxRest, 'get_snap_linked_device_list', - side_effect=[[{'targetDevice': tpd.PowerMaxData.device_id2}], - [{'targetDevice': tpd.PowerMaxData.device_id2}, - {'targetDevice': tpd.PowerMaxData.device_id3}]]) + side_effect=[[{'targetDevice': tpd.PowerMaxData.device_id2, + 'defined': False}], + [{'targetDevice': tpd.PowerMaxData.device_id2, + 'defined': False}, + {'targetDevice': tpd.PowerMaxData.device_id3, + 'defined': False}]]) @mock.patch.object(provision.PowerMaxProvision, '_unlink_volume') def test_delete_volume_snap_check_for_links(self, mock_unlink, mock_tgts): self.provision.delete_volume_snap_check_for_links( @@ -568,7 +573,8 @@ class PowerMaxProvisionTest(test.TestCase): self.data.array, "", "", self.data.test_snapshot_snap_name, self.data.extra_specs, snap_id=self.data.snap_id, list_volume_pairs=[ - (self.data.device_id, tpd.PowerMaxData.device_id2)]) + (self.data.device_id, tpd.PowerMaxData.device_id2)], + symforce=False) mock_unlink.reset_mock() self.provision.delete_volume_snap_check_for_links( self.data.array, self.data.test_snapshot_snap_name, diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py index eae078f2737..b35fa54566e 100644 --- a/cinder/volume/drivers/dell_emc/powermax/common.py +++ b/cinder/volume/drivers/dell_emc/powermax/common.py @@ -154,7 +154,12 @@ powermax_opts = [ cfg.IntOpt(utils.REST_API_READ_TIMEOUT, default=30, min=1, help='Use this value to specify read ' - 'timeout value (in seconds) for rest call.')] + 'timeout value (in seconds) for rest call.'), + cfg.BoolOpt(utils.SNAPVX_UNLINK_SYMFORCE, + default=False, + help='Enable SnapVx unlink symforce, which forces ' + 'the operation to execute when normally it is rejected.'), +] CONF.register_opts(powermax_opts, group=configuration.SHARED_CONF_GROUP) @@ -195,11 +200,8 @@ class PowerMaxCommon(object): self.rest = rest.PowerMaxRest() self.utils = utils.PowerMaxUtils() - self.masking = masking.PowerMaxMasking(prtcl, self.rest) - self.provision = provision.PowerMaxProvision(self.rest) self.volume_metadata = volume_metadata.PowerMaxVolumeMetadata( self.rest, version, LOG.isEnabledFor(logging.DEBUG)) - self.migrate = migrate.PowerMaxMigrate(prtcl, self.rest) # Configuration/Attributes self.protocol = prtcl @@ -220,6 +222,14 @@ class PowerMaxCommon(object): if active_backend_id == utils.PMAX_FAILOVER_START_ARRAY_PROMOTION: self.promotion = True + # Init provision, masking and migrate instances + self.provision = provision.PowerMaxProvision( + self.rest, self.configuration) + self.masking = masking.PowerMaxMasking( + prtcl, self.rest, self.configuration) + self.migrate = migrate.PowerMaxMigrate( + prtcl, self.rest, self.configuration) + # Gather environment info self._get_replication_info() self._get_u4p_failover_info() diff --git a/cinder/volume/drivers/dell_emc/powermax/masking.py b/cinder/volume/drivers/dell_emc/powermax/masking.py index 54b86b96f08..159551f11ae 100644 --- a/cinder/volume/drivers/dell_emc/powermax/masking.py +++ b/cinder/volume/drivers/dell_emc/powermax/masking.py @@ -39,11 +39,12 @@ class PowerMaxMasking(object): Masking code to dynamically create a masking view. It supports VMAX 3, All Flash and PowerMax arrays. """ - def __init__(self, prtcl, rest): + def __init__(self, prtcl, rest, configuration): self.protocol = prtcl self.utils = utils.PowerMaxUtils() self.rest = rest - self.provision = provision.PowerMaxProvision(self.rest) + self.provision = provision.PowerMaxProvision(self.rest, + configuration) def setup_masking_view( self, serial_number, volume, masking_view_dict, extra_specs): diff --git a/cinder/volume/drivers/dell_emc/powermax/migrate.py b/cinder/volume/drivers/dell_emc/powermax/migrate.py index 73fda32a3b7..d1156ef2e28 100644 --- a/cinder/volume/drivers/dell_emc/powermax/migrate.py +++ b/cinder/volume/drivers/dell_emc/powermax/migrate.py @@ -34,11 +34,11 @@ class PowerMaxMigrate(object): It supports VMAX 3 and VMAX All Flash and PowerMax arrays. """ - def __init__(self, prtcl, rest): + def __init__(self, prtcl, rest, configuration): self.rest = rest self.utils = utils.PowerMaxUtils() - self.masking = masking.PowerMaxMasking(prtcl, self.rest) - self.provision = provision.PowerMaxProvision(self.rest) + self.masking = masking.PowerMaxMasking(prtcl, self.rest, configuration) + self.provision = provision.PowerMaxProvision(self.rest, configuration) def do_migrate_if_candidate( self, array, srp, device_id, volume, connector): diff --git a/cinder/volume/drivers/dell_emc/powermax/provision.py b/cinder/volume/drivers/dell_emc/powermax/provision.py index 24b7f39baf4..f9433fc1cf1 100644 --- a/cinder/volume/drivers/dell_emc/powermax/provision.py +++ b/cinder/volume/drivers/dell_emc/powermax/provision.py @@ -36,9 +36,11 @@ class PowerMaxProvision(object): It supports VMAX 3, All Flash and PowerMax arrays. """ - def __init__(self, rest): + def __init__(self, rest, configuration): self.utils = utils.PowerMaxUtils() self.rest = rest + self.snapvx_unlink_symforce = configuration.safe_get( + utils.SNAPVX_UNLINK_SYMFORCE) or False def create_storage_group( self, array, storagegroup_name, srp, slo, workload, @@ -183,6 +185,9 @@ class PowerMaxProvision(object): {'delta': self.utils.get_time_delta(start_time, time.time())}) + def _is_symforce_enabled(self, defined): + return self.snapvx_unlink_symforce and (not defined) + def unlink_snapvx_tgt_volume( self, array, target_device_id, source_device_id, snap_name, extra_specs, snap_id, loop=True): @@ -197,20 +202,33 @@ class PowerMaxProvision(object): :param loop: if looping call is required for handling retries """ @coordination.synchronized("emc-snapvx-{src_device_id}") - def do_unlink_volume(src_device_id): + def do_unlink_volume(src_device_id, symforce=False): LOG.debug("Break snap vx link relationship between: %(src)s " "and: %(tgt)s.", {'src': src_device_id, 'tgt': target_device_id}) self._unlink_volume(array, src_device_id, target_device_id, snap_name, extra_specs, snap_id=snap_id, - list_volume_pairs=None, loop=loop) + list_volume_pairs=None, loop=loop, + symforce=symforce) - do_unlink_volume(source_device_id) + # Get the link defined status + defined = True + linked_list = self.rest.get_snap_linked_device_list( + array, source_device_id, snap_name, snap_id) + for link in linked_list: + if target_device_id == link['targetDevice']: + if not link['defined']: + defined = False + break + LOG.debug("The link defined status: %s", defined) + do_unlink_volume( + source_device_id, symforce=self._is_symforce_enabled(defined)) def _unlink_volume( self, array, source_device_id, target_device_id, snap_name, - extra_specs, snap_id=None, list_volume_pairs=None, loop=True): + extra_specs, snap_id=None, list_volume_pairs=None, loop=True, + symforce=False): """Unlink a target volume from its source volume. :param array: the array serial number @@ -221,6 +239,7 @@ class PowerMaxProvision(object): :param snap_id: the unique snap id of the SnapVX :param list_volume_pairs: list of volume pairs, optional :param loop: if looping call is required for handling retries + :param symforce: if symforce is enabled :returns: return code """ def _unlink_vol(): @@ -234,7 +253,8 @@ class PowerMaxProvision(object): if not kwargs['modify_vol_success']: self.rest.modify_volume_snap( array, source_device_id, target_device_id, snap_name, - extra_specs, snap_id=snap_id, unlink=True, + extra_specs, snap_id=snap_id, + unlink=True, symforce=symforce, list_volume_pairs=list_volume_pairs) kwargs['modify_vol_success'] = True except exception.VolumeBackendAPIException: @@ -250,7 +270,8 @@ class PowerMaxProvision(object): if not loop: self.rest.modify_volume_snap( array, source_device_id, target_device_id, snap_name, - extra_specs, snap_id=snap_id, unlink=True, + extra_specs, snap_id=snap_id, + unlink=True, symforce=symforce, list_volume_pairs=list_volume_pairs) else: kwargs = {'retries': 0, @@ -387,6 +408,7 @@ class PowerMaxProvision(object): :param snap_id: the unique snap id of the SnapVX """ list_device_pairs = [] + list_device_pairs_defined = True if not isinstance(source_devices, list): source_devices = [source_devices] for source_device in source_devices: @@ -398,6 +420,8 @@ class PowerMaxProvision(object): if len(linked_list) == 1: target_device = linked_list[0]['targetDevice'] list_device_pairs.append((source_device, target_device)) + if not linked_list[0]['defined']: + list_device_pairs_defined = False else: for link in linked_list: # If a single source volume has multiple targets, @@ -405,11 +429,13 @@ class PowerMaxProvision(object): target_device = link['targetDevice'] self._unlink_volume( array, source_device, target_device, snap_name, - extra_specs, snap_id=snap_id) + extra_specs, snap_id=snap_id, + symforce=self._is_symforce_enabled(link['defined'])) if list_device_pairs: self._unlink_volume( array, "", "", snap_name, extra_specs, snap_id=snap_id, - list_volume_pairs=list_device_pairs) + list_volume_pairs=list_device_pairs, + symforce=self._is_symforce_enabled(list_device_pairs_defined)) if source_devices: self.delete_volume_snap( array, snap_name, source_devices, snap_id, restored=False) @@ -703,6 +729,14 @@ class PowerMaxProvision(object): self.rest.modify_volume_snap( array, None, None, snap_name, extra_specs, snap_id=snap_id, link=True, list_volume_pairs=list_volume_pairs) + # Get the link defined status + defined = True + for src, dst in list_volume_pairs: + linked_list = self.rest.get_snap_linked_device_list( + array, src, snap_name, snap_id) + if not linked_list[0]['defined']: + defined = False + break # Unlink the snapshot LOG.debug("Unlinking Snap Vx snapshot: source group: %(srcGroup)s " "targetGroup: %(tgtGroup)s.", @@ -710,7 +744,8 @@ class PowerMaxProvision(object): 'tgtGroup': target_group_name}) self._unlink_volume( array, None, None, snap_name, extra_specs, snap_id=snap_id, - list_volume_pairs=list_volume_pairs) + list_volume_pairs=list_volume_pairs, + symforce=self._is_symforce_enabled(defined)) # Delete the snapshot if necessary if delete_snapshot: LOG.debug("Deleting Snap Vx snapshot: source group: %(srcGroup)s " diff --git a/cinder/volume/drivers/dell_emc/powermax/rest.py b/cinder/volume/drivers/dell_emc/powermax/rest.py index 6a9e9724c31..96767063820 100644 --- a/cinder/volume/drivers/dell_emc/powermax/rest.py +++ b/cinder/volume/drivers/dell_emc/powermax/rest.py @@ -2197,7 +2197,7 @@ class PowerMaxRest(object): def modify_volume_snap(self, array, source_id, target_id, snap_name, extra_specs, snap_id=None, link=False, unlink=False, rename=False, new_snap_name=None, restore=False, - list_volume_pairs=None, copy=False): + list_volume_pairs=None, copy=False, symforce=False): """Modify a snapvx snapshot :param array: the array serial number @@ -2248,7 +2248,7 @@ class PowerMaxRest(object): "copy": copy, "action": action, "star": 'false', "force": force, "exact": 'false', "remote": 'false', - "symforce": 'false'} + "symforce": str(symforce).lower()} elif action == "Rename": operation = 'Rename snapVx snapshot' payload = {"deviceNameListSource": [{"name": source_id}], diff --git a/cinder/volume/drivers/dell_emc/powermax/utils.py b/cinder/volume/drivers/dell_emc/powermax/utils.py index eb3f1f95dc4..268a5b26581 100644 --- a/cinder/volume/drivers/dell_emc/powermax/utils.py +++ b/cinder/volume/drivers/dell_emc/powermax/utils.py @@ -179,6 +179,7 @@ LOAD_LOOKBACK = 'load_look_back' LOAD_LOOKBACK_RT = 'load_look_back_real_time' PORT_GROUP_LOAD_METRIC = 'port_group_load_metric' PORT_LOAD_METRIC = 'port_load_metric' +SNAPVX_UNLINK_SYMFORCE = 'snapvx_unlink_symforce' # One minute in milliseconds ONE_MINUTE = 60000 diff --git a/doc/source/configuration/block-storage/drivers/dell-emc-powermax-driver.rst b/doc/source/configuration/block-storage/drivers/dell-emc-powermax-driver.rst index 9620b17f899..a38c15a917d 100644 --- a/doc/source/configuration/block-storage/drivers/dell-emc-powermax-driver.rst +++ b/doc/source/configuration/block-storage/drivers/dell-emc-powermax-driver.rst @@ -3243,6 +3243,9 @@ options are detailed in the table below. | | | | | load calculation, minimum of 1 | | | | | | maximum of 60 (24 hours). | +-----------------------------+----------------+-----------------+----------------------------------------+ + | ``snapvx_unlink_symforce`` | ``True/False`` | ``False`` | | Enable/disable symforce | + | | | | | for SnapVx unlink. | + +-----------------------------+----------------+-----------------+----------------------------------------+ | ``port_group_load_metric`` | See below | ``PercentBusy`` | | Metric used for port group load | | | | | | calculation. | +-----------------------------+----------------+-----------------+----------------------------------------+ diff --git a/releasenotes/notes/bug-2045230-dell-powermax-fix-snapvx-unlink-e27d67d6b217d706.yaml b/releasenotes/notes/bug-2045230-dell-powermax-fix-snapvx-unlink-e27d67d6b217d706.yaml new file mode 100644 index 00000000000..74898f991e7 --- /dev/null +++ b/releasenotes/notes/bug-2045230-dell-powermax-fix-snapvx-unlink-e27d67d6b217d706.yaml @@ -0,0 +1,32 @@ +--- +fixes: + - | + Dell PowerMax Driver `Bug #2045230 + `_: Fixed + the issue that Dell PowerMax SnapVx link fails as the linked device is not + yet fully defined. + + Previously, the below operations could fail if the linked device was not + yet fully defined at the time of the call. + Now, when ``snapvx_unlink_symforce`` is enabled, those operations are not + interrupted by not fully defined devices. + + By default, ``snapvx_unlink_symforce`` is ``False``. Use extreme caution + with this option. If used when a link is copy in progress or when a restore + is restore in progress, this will cause an incomplete copy and data on the + copy target would not be usable. + + Impacted operations: + + * Clone a volume + * Create a volume from a snapshot + * Create volume snapshots + * Delete volume snapshots + * Revert volume to snapshot + * Create generic volume group from source + * Unmanage volumes +upgrade: + - | + Dell PowerMax Driver: introduced a new configuration option, + ``snapvx_unlink_symforce``, to address Bug #2045230. See the Bug Fixes + section for details.