PowerMax Driver - Fix non-temporary snapshot delete

Fix bug where non-temporary snapshots were being
deleted from volume when do_sync_check is called
for that volume, caused by missing check for
temporary snapshot naming convention. Fixed by
adding this check back into the conditional
that determined if a snapshot should be deleted.

Change-Id: I20194f57437e9ece5d95259bd91c95a4f19d9ab5
Closes-Bug: 1887962

cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py

@@ -3165,19 +3165,6 @@ class PowerMaxCommonTest(test.TestCase):
         port = self.common._get_unisphere_port()
         self.assertEqual(ref_port, port)
 
-    @mock.patch.object(common.PowerMaxCommon,
-                       '_do_sync_check')
-    def test_sync_check_no_source_device_on_array(self, mock_check):
-        with mock.patch.object(self.rest, 'get_volume',
-                               side_effect=exception.VolumeBackendAPIException(
-                                   "404 00123 does not exist")):
-            array = self.data.array
-            device_id = self.data.device_id
-            extra_specs = self.data.extra_specs
-            self.common._sync_check(array, device_id, extra_specs,
-                                    source_device_id='00123')
-            mock_check.assert_not_called()
-
     def test_sync_check(self):
         array = self.data.array
         device_id = self.data.device_id
@@ -3186,13 +3173,13 @@ class PowerMaxCommonTest(test.TestCase):
         with mock.patch.object(self.common, '_do_sync_check') as mck_sync:
             self.common._sync_check(array, device_id, extra_specs, False,
                                     self.data.device_id2)
-            mck_sync.assert_called_with(array, self.data.device_id2,
+            mck_sync.assert_called_with(array, self.data.device_id,
                                         extra_specs, False)
             mck_sync.reset_mock()
             with mock.patch.object(self.common, '_get_target_source_device',
                                    return_value=self.data.device_id3):
                 self.common._sync_check(array, device_id, extra_specs, True)
-                mck_sync.assert_called_with(array, self.data.device_id3,
+                mck_sync.assert_called_with(array, self.data.device_id,
                                             extra_specs, True)
             mck_sync.reset_mock()
             self.common._sync_check(array, device_id, extra_specs)
@@ -3215,30 +3202,64 @@ class PowerMaxCommonTest(test.TestCase):
 
     @mock.patch.object(provision.PowerMaxProvision, 'delete_temp_volume_snap')
     @mock.patch.object(provision.PowerMaxProvision, 'unlink_snapvx_tgt_volume')
-    def test_unlink_targets_and_delete_temp_snapvx(self, mck_break, mck_del):
+    def test_unlink_targets_and_delete_temp_snapvx_legacy(
+            self, mck_break, mck_del):
         array = self.data.array
         extra_specs = self.data.extra_specs
-        session = self.data.snap_tgt_session_cm_enabled
+        session = deepcopy(self.data.snap_tgt_session_cm_enabled)
+        legacy_snap_name = 'EMC_SMI'
+        session['snap_name'] = legacy_snap_name
+        source = session['source_vol_id']
+        snap_id = session['snapid']
+        target = session['target_vol_id']
+        self.common._unlink_targets_and_delete_temp_snapvx(
+            session, array, extra_specs)
+        mck_break.assert_called_with(array, target, source, legacy_snap_name,
+                                     extra_specs, snap_id, True)
+        mck_del.assert_called_once_with(
+            array, legacy_snap_name, source, snap_id)
+
+    @mock.patch.object(provision.PowerMaxProvision, 'delete_temp_volume_snap')
+    @mock.patch.object(provision.PowerMaxProvision, 'unlink_snapvx_tgt_volume')
+    def test_unlink_targets_and_delete_temp_snapvx_is_temp(
+            self, mck_break, mck_del):
+        array = self.data.array
+        extra_specs = self.data.extra_specs
+        session = deepcopy(self.data.snap_tgt_session_cm_enabled)
         snap_name = session['snap_name']
         source = session['source_vol_id']
         snap_id = session['snapid']
         target = session['target_vol_id']
-
         self.common._unlink_targets_and_delete_temp_snapvx(
             session, array, extra_specs)
         mck_break.assert_called_with(array, target, source, snap_name,
                                      extra_specs, snap_id, True)
         mck_del.assert_called_once_with(array, snap_name, source, snap_id)
 
-        mck_break.reset_mock()
+    def test_unlink_targets_and_delete_temp_snapvx_no_snap_name_found(self):
-        mck_del.reset_mock()
+        array = self.data.array
+        extra_specs = self.data.extra_specs
+        session = dict()
+        self.assertRaises(exception.VolumeBackendAPIException,
+                          self.common._unlink_targets_and_delete_temp_snapvx,
+                          session, array, extra_specs)
 
-        session['copy_mode'] = False
+    @mock.patch.object(provision.PowerMaxProvision, 'delete_temp_volume_snap')
-        session['expired'] = True
+    @mock.patch.object(provision.PowerMaxProvision, 'unlink_snapvx_tgt_volume')
+    def test_unlink_targets_and_delete_temp_snapvx_not_legacy_not_temp(
+            self, mck_break, mck_del):
+        array = self.data.array
+        extra_specs = self.data.extra_specs
+        session = deepcopy(self.data.snap_tgt_session_cm_enabled)
+        session['snap_name'] = 'not_leg_or_tmp'
+        snap_name = session['snap_name']
+        source = session['source_vol_id']
+        snap_id = session['snapid']
+        target = session['target_vol_id']
         self.common._unlink_targets_and_delete_temp_snapvx(
             session, array, extra_specs)
         mck_break.assert_called_with(array, target, source, snap_name,
-                                     extra_specs, snap_id, False)
+                                     extra_specs, snap_id, True)
         mck_del.assert_not_called()
 
     @mock.patch.object(rest.PowerMaxRest, 'find_snap_vx_sessions',

cinder/volume/drivers/dell_emc/powermax/common.py

@@ -2909,16 +2909,10 @@ class PowerMaxCommon(object):
         if source_device_id:
             @coordination.synchronized("emc-source-{src_device_id}")
             def do_unlink_and_delete_snap(src_device_id):
-                # Check if source device exists on the array
+                LOG.debug("Locking on source device %(device_id)s.",
-                try:
+                          {'device_id': src_device_id})
-                    self.rest.get_volume(array, src_device_id)
-                except exception.VolumeBackendAPIException:
-                    LOG.debug("Device %(device_id)s not found on array, no "
-                              "sync check required.",
-                              {'device_id': src_device_id})
-                    return
                 self._do_sync_check(
-                    array, src_device_id, extra_specs, tgt_only)
+                    array, device_id, extra_specs, tgt_only)
 
             do_unlink_and_delete_snap(source_device_id)
         else:
@@ -2961,9 +2955,15 @@ class PowerMaxCommon(object):
         :param extra_specs: extra specifications
         """
         snap_name = session.get('snap_name')
+        if not snap_name:
+            msg = _("Snap_name not present in volume's snapvx session. "
+                    "Unable to perform unlink and cleanup.")
+            LOG.error(msg)
+            raise exception.VolumeBackendAPIException(msg)
         source = session.get('source_vol_id')
         snap_id = session.get('snapid')
-        expired = session.get('expired')
+        is_legacy = 'EMC_SMI' in snap_name
+        is_temp = utils.CLONE_SNAPSHOT_NAME in snap_name
 
         target, cm_enabled = None, False
         if session.get('target_vol_id'):
@@ -2982,10 +2982,13 @@ class PowerMaxCommon(object):
 
         # Candidates for deletion:
         # 1. If legacy snapshot with 'EMC_SMI' in snapshot name
-        # 2. If snapVX snapshot with copy mode enabled
+        # 2. If snapVX snapshot is temporary
-        # 3. If snapVX snapshot with copy mode disabled and not expired
+        # If a target is present in the snap session it will be unlinked by
-        if ('EMC_SMI' in snap_name or cm_enabled or (
+        # the provision.unlink_snapvx_tgt_volume call above. This accounts
-                not cm_enabled and not expired)):
+        # for both CM enabled and disabled, as such by this point there will
+        # either be no target or it would have been unlinked, therefore
+        # delete if it is legacy or temp.
+        if is_legacy or is_temp:
             LOG.debug(
                 "Deleting temporary snapshot. Source: %(vol)s, snap name: "
                 "%(name)s, snap id: %(snapid)s.", {

releasenotes/notes/bug-1887962-643379faf20f01cf.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    `Bug #1887962 <https://bugs.launchpad.net/cinder/+bug/1887962>`_:
+    PowerMax driver fix to rectify incorrectly deleted non-temporary
+    snapshots when calling do_sync_check used in multiple operations due
+    to missing check for temporary snapshot name.