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

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}],
+        side_effect=[[{'targetDevice': tpd.PowerMaxData.device_id2,
-                     [{'targetDevice': tpd.PowerMaxData.device_id2},
+                       'defined': False}],
-                      {'targetDevice': tpd.PowerMaxData.device_id3}]])
+                     [{'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,

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()

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):

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.masking = masking.PowerMaxMasking(prtcl, self.rest, configuration)
-        self.provision = provision.PowerMaxProvision(self.rest)
+        self.provision = provision.PowerMaxProvision(self.rest, configuration)
 
     def do_migrate_if_candidate(
             self, array, srp, device_id, volume, connector):

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 "

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}],

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

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.                         |
     +-----------------------------+----------------+-----------------+----------------------------------------+

releasenotes/notes/bug-2045230-dell-powermax-fix-snapvx-unlink-e27d67d6b217d706.yaml

@@ -0,0 +1,32 @@
+---
+fixes:
+  - |
+    Dell PowerMax Driver `Bug #2045230
+    <https://bugs.launchpad.net/cinder/+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.