From 8bdce306e761e3104372db54b073235fc8f1511c Mon Sep 17 00:00:00 2001
From: Helen Walsh <helen.walsh@emc.com>
Date: Mon, 14 Jun 2021 15:32:18 +0100
Subject: [PATCH 1/2] PowerMax Driver - Improve error handling around deletes

1. Bring storage group check within delete volume so that
it can be used in every place it is called and not just the
delete method.
2. Remove the reset identifier_name name as this makes the
device available for use straight away even if is not deleted.
3. Allow for None value in unlink_snapshot.
4. Add extra debugging information for device id.
5. Retry on getting connection info for volume.
6. Validate a masking view on an attach operation.
7. Reset identifier name on a rollback.
8. Retry a cleanup if snap id not available at the time.
9. Pylint fixes.
10. Retry on a failed delete with a cleanup snapvx in exception.

NOTE:
This commit is including a small change from Change-Id
I05edfb7db484103babf43c170c5834a000594fea that moves the location of a
call to rest.find_nsap_vx_sessions() in _cleanup_device_retry() so that
a call out to the REST API is only made when necessary and to pass the
unit tests.

Resolves: OSPRH-17708 (1/2)
Change-Id: I0c0f3d2246b840326579748027b9cd15bf174ff8
(cherry picked from commit e1f6de6f836e1d534d0196ba43e6642bc5e279c6)
Conflicts:
	cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py
	cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py
	cinder/volume/drivers/dell_emc/powermax/rest.py
---
 .../powermax/powermax_fake_objects.py         |   2 +-
 .../dell_emc/powermax/test_powermax_common.py |  99 +++++++++-
 .../powermax/test_powermax_masking.py         |  42 ++++-
 .../powermax/test_powermax_replication.py     |  15 +-
 .../dell_emc/powermax/test_powermax_rest.py   |  18 +-
 .../drivers/dell_emc/powermax/common.py       | 170 +++++++++++++-----
 .../drivers/dell_emc/powermax/masking.py      | 122 ++++++++++++-
 .../drivers/dell_emc/powermax/provision.py    |   5 +-
 .../volume/drivers/dell_emc/powermax/rest.py  |  55 ++++--
 9 files changed, 450 insertions(+), 78 deletions(-)

diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py
index 794f548b0d..aecfa40fa2 100644
--- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py
+++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py
@@ -146,7 +146,7 @@ class FakeRequestsSession(object):
         if '/private' in url:
             return_object = self.data.private_vol_details
         elif params:
-            if '1' in params.values():
+            if '1' in params.values() or 'volume_identifier' in params:
                 return_object = self.data.volume_list[0]
             elif '2' in params.values():
                 return_object = self.data.volume_list[1]
diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py
index 9c2e7d8762..ebc773585d 100644
--- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py
+++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py
@@ -399,7 +399,7 @@ class PowerMaxCommonTest(test.TestCase):
         extra_specs = self.common._initial_setup(test_volume)
         self.assertRaises(exception.VolumeBackendAPIException,
                           self.common._delete_volume, test_volume)
-        mck_cleanup.assert_called_once_with(array, device_id, extra_specs)
+        mck_cleanup.assert_called_with(array, device_id, extra_specs)
         mck_delete.assert_not_called()
 
     @mock.patch.object(common.PowerMaxCommon, '_delete_from_srp')
@@ -417,7 +417,7 @@ class PowerMaxCommonTest(test.TestCase):
         extra_specs = self.common._initial_setup(test_volume)
         self.assertRaises(exception.VolumeBackendAPIException,
                           self.common._delete_volume, test_volume)
-        mck_cleanup.assert_called_once_with(array, device_id, extra_specs)
+        mck_cleanup.assert_called_with(array, device_id, extra_specs)
         mck_delete.assert_not_called()
 
     @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx')
@@ -1943,12 +1943,15 @@ class PowerMaxCommonTest(test.TestCase):
                 clone_name, snap_name, self.data.extra_specs,
                 target_volume=clone_volume)
 
+    @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume',
+                       return_value=[])
     @mock.patch.object(rest.PowerMaxRest, 'get_snap_id',
                        return_value=tpd.PowerMaxData.snap_id)
     @mock.patch.object(
         masking.PowerMaxMasking,
         'remove_and_reset_members')
-    def test_cleanup_target_sync_present(self, mock_remove, mock_snaps):
+    def test_cleanup_target_sync_present(
+            self, mock_remove, mock_snaps, mock_sgs):
         array = self.data.array
         clone_volume = self.data.test_clone_volume
         source_device_id = self.data.device_id
@@ -2957,6 +2960,7 @@ class PowerMaxCommonTest(test.TestCase):
             model_update, __ = self.common._delete_group(group, volumes)
             self.assertEqual(ref_model_update, model_update)
 
+    @mock.patch.object(common.PowerMaxCommon, '_delete_from_srp')
     @mock.patch.object(masking.PowerMaxMasking, 'remove_and_reset_members')
     @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx')
     @mock.patch.object(common.PowerMaxCommon, '_get_members_of_volume_group',
@@ -2966,7 +2970,7 @@ class PowerMaxCommonTest(test.TestCase):
     @mock.patch.object(volume_utils, 'is_group_a_type', return_value=False)
     def test_delete_group_snapshot_and_volume_cleanup(
             self, mock_check, mck_get_snaps, mock_members, mock_cleanup,
-            mock_remove):
+            mock_remove, mock_del):
         group = self.data.test_group_1
         volumes = [fake_volume.fake_volume_obj(
             context='cxt', provider_location=None)]
@@ -3820,6 +3824,13 @@ class PowerMaxCommonTest(test.TestCase):
         act_metadata = self.common.get_volume_metadata(array, device_id)
         self.assertEqual(ref_metadata, act_metadata)
 
+    def test_get_volume_metadata_device_none(self):
+        ref_metadata = {}
+        array = self.data.array
+        device_id = None
+        act_metadata = self.common.get_volume_metadata(array, device_id)
+        self.assertEqual(ref_metadata, act_metadata)
+
     @mock.patch.object(rest.PowerMaxRest, 'get_volume_snap_info',
                        return_value=tpd.PowerMaxData.priv_snap_response)
     def test_get_snapshot_metadata(self, mck_snap):
@@ -3908,6 +3919,23 @@ class PowerMaxCommonTest(test.TestCase):
             model_update, existing_metadata, object_metadata)
         self.assertEqual(ref_model_update, model_update)
 
+    def test_update_metadata_no_object_metadata(self):
+        model_update = {'provider_location': six.text_type(
+            self.data.provider_location)}
+        ref_model_update = (
+            {'provider_location': six.text_type(self.data.provider_location),
+             'metadata': {'user-meta-key-1': 'user-meta-value-1',
+                          'user-meta-key-2': 'user-meta-value-2'}})
+
+        existing_metadata = {'user-meta-key-1': 'user-meta-value-1',
+                             'user-meta-key-2': 'user-meta-value-2'}
+
+        object_metadata = {}
+
+        model_update = self.common.update_metadata(
+            model_update, existing_metadata, object_metadata)
+        self.assertEqual(ref_model_update, model_update)
+
     def test_update_metadata_model_list_exception(self):
         model_update = [{'provider_location': six.text_type(
             self.data.provider_location)}]
@@ -4447,3 +4475,66 @@ class PowerMaxCommonTest(test.TestCase):
         array = self.data.array
         self.common._delete_temp_snapshot(session, array)
         mck_delete.assert_not_called()
+
+    @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx')
+    @mock.patch.object(rest.PowerMaxRest, 'get_volume_snapshot_list',
+                       return_value=[])
+    @mock.patch.object(rest.PowerMaxRest, 'find_snap_vx_sessions',
+                       side_effect=[(None, tpd.PowerMaxData.snap_tgt_session),
+                                    (None, None)])
+    def test_cleanup_device_retry(self, mock_snapvx, mock_ss_list, mock_clean):
+        self.common._cleanup_device_retry(
+            self.data.array, self.data.device_id, self.data.extra_specs)
+        self.assertEqual(2, mock_clean.call_count)
+
+    @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id',
+                       return_value=[tpd.PowerMaxData.device_id,
+                                     tpd.PowerMaxData.device_id2])
+    def test_get_device_id_from_identifier_list(self, mock_dev_id):
+        ret_dev = self.common._get_device_id_from_identifier(
+            self.data.array, 'vol', self.data.device_id)
+        self.assertEqual(self.data.device_id, ret_dev)
+
+    @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id',
+                       return_value=tpd.PowerMaxData.device_id2)
+    def test_get_device_id_from_identifier_wrong(self, mock_dev_id):
+        ret_dev = self.common._get_device_id_from_identifier(
+            self.data.array, 'vol', self.data.device_id)
+        self.assertEqual(self.data.device_id2, ret_dev)
+
+    @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id',
+                       return_value=tpd.PowerMaxData.device_id)
+    def test_get_device_id_from_identifier_same(self, mock_dev_id):
+        ret_dev = self.common._get_device_id_from_identifier(
+            self.data.array, 'vol', self.data.device_id)
+        self.assertIsNone(ret_dev)
+
+    @mock.patch.object(rest.PowerMaxRest, 'rename_volume')
+    @mock.patch.object(rest.PowerMaxRest, 'find_volume_identifier',
+                       return_value='vol')
+    @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id',
+                       return_value=tpd.PowerMaxData.device_id)
+    def test_reset_identifier_on_rollback_rename(
+            self, mock_dev, mock_ident, mock_rename):
+        self.common._reset_identifier_on_rollback(self.data.array, 'vol')
+        mock_rename.assert_called_once()
+
+    @mock.patch.object(rest.PowerMaxRest, 'rename_volume')
+    @mock.patch.object(rest.PowerMaxRest, 'find_volume_identifier',
+                       return_value='diff_vol_name')
+    @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id',
+                       return_value=tpd.PowerMaxData.device_id)
+    def test_reset_identifier_on_rollback_no_rename(
+            self, mock_dev, mock_ident, mock_rename):
+        self.common._reset_identifier_on_rollback(self.data.array, 'vol')
+        mock_rename.assert_not_called()
+
+    @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx')
+    @mock.patch.object(
+        provision.PowerMaxProvision, 'delete_volume_from_srp',
+        side_effect=[exception.VolumeBackendAPIException, None])
+    def test_test_delete_from_srp(self, mock_del, mock_clean):
+        self.common._delete_from_srp(
+            self.data.array, 'vol_name', self.data.device_id,
+            self.data.extra_specs)
+        mock_clean.assert_called_once()
diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_masking.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_masking.py
index aad266ac14..8e970b1945 100644
--- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_masking.py
+++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_masking.py
@@ -96,6 +96,8 @@ class PowerMaxMaskingTest(test.TestCase):
             self.maskingviewdict, self.extra_specs)
         mock_get_or_create_mv.assert_called_once()
 
+    @mock.patch.object(masking.PowerMaxMasking, '_validate_attach',
+                       return_value=True)
     @mock.patch.object(masking.PowerMaxMasking,
                        '_check_adding_volume_to_storage_group')
     @mock.patch.object(masking.PowerMaxMasking, '_move_vol_from_default_sg',
@@ -108,7 +110,7 @@ class PowerMaxMaskingTest(test.TestCase):
                                     Exception('Exception')])
     def test_get_or_create_masking_view_and_map_lun(
             self, mock_masking_view_element, mock_masking, mock_move,
-            mock_add_volume):
+            mock_add_volume, mock_validate):
         rollback_dict = (
             self.driver.masking.get_or_create_masking_view_and_map_lun(
                 self.data.array, self.data.test_volume,
@@ -838,6 +840,9 @@ class PowerMaxMaskingTest(test.TestCase):
             self.data.array, self.data.masking_view_name_i)
         mock_delete_mv.assert_called_once()
 
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_groups_from_volume',
+        return_value=list())
     @mock.patch.object(masking.PowerMaxMasking,
                        'return_volume_to_volume_group')
     @mock.patch.object(rest.PowerMaxRest, 'move_volume_between_storage_groups')
@@ -845,7 +850,7 @@ class PowerMaxMaskingTest(test.TestCase):
                        'get_or_create_default_storage_group')
     @mock.patch.object(masking.PowerMaxMasking, 'add_volume_to_storage_group')
     def test_add_volume_to_default_storage_group(
-            self, mock_add_sg, mock_get_sg, mock_move, mock_return):
+            self, mock_add_sg, mock_get_sg, mock_move, mock_return, mock_sgs):
         self.mask.add_volume_to_default_storage_group(
             self.data.array, self.device_id, self.volume_name,
             self.extra_specs)
@@ -1346,3 +1351,36 @@ class PowerMaxMaskingTest(test.TestCase):
                 exception_message):
             self.mask._check_director_and_port_status(
                 self.data.array, self.data.port_group_name_f)
+
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_groups_from_volume',
+        return_value=([tpd.PowerMaxData.storagegroup_name_f]))
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_masking_views_from_storage_group',
+        return_value=[tpd.PowerMaxData.masking_view_name_f])
+    def test_validate_attach(self, mock_sgs, mock_mvs):
+        self.assertTrue(self.mask._validate_attach(
+            self.data.array, self.data.device_id,
+            self.data.storagegroup_name_f, self.data.masking_view_name_f))
+
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_groups_from_volume',
+        return_value=([tpd.PowerMaxData.storagegroup_name_f]))
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_masking_views_from_storage_group',
+        return_value=[])
+    def test_validate_attach_no_mvs(self, mock_sgs, mock_mvs):
+        self.assertFalse(self.mask._validate_attach(
+            self.data.array, self.data.device_id,
+            self.data.storagegroup_name_f, self.data.masking_view_name_f))
+
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_groups_from_volume',
+        return_value=([tpd.PowerMaxData.defaultstoragegroup_name]))
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_masking_views_from_storage_group',
+        return_value=[])
+    def test_validate_attach_incorrect_sg(self, mock_sgs, mock_mvs):
+        self.assertFalse(self.mask._validate_attach(
+            self.data.array, self.data.device_id,
+            self.data.storagegroup_name_f, self.data.masking_view_name_f))
diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py
index 761d0639b3..488d5d1496 100644
--- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py
+++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py
@@ -218,12 +218,21 @@ class PowerMaxReplicationTest(test.TestCase):
                           self.iscsi_common.initialize_connection,
                           self.data.test_volume, self.data.connector)
 
+    @mock.patch.object(masking.PowerMaxMasking, '_validate_attach',
+                       return_value=True)
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_groups_from_volume',
+        side_effect=[
+            [], [], [], [], [tpd.PowerMaxData.storagegroup_name_f], [],
+            [tpd.PowerMaxData.storagegroup_name_f],
+            [tpd.PowerMaxData.storagegroup_name_f]], )
     @mock.patch.object(
         masking.PowerMaxMasking, '_check_director_and_port_status')
     @mock.patch.object(
         masking.PowerMaxMasking, 'pre_multiattach',
         return_value=tpd.PowerMaxData.masking_view_dict_multiattach)
-    def test_attach_metro_volume(self, mock_pre, mock_check):
+    def test_attach_metro_volume(
+            self, mock_pre, mock_check, mock_sgs, mock_validate):
         rep_extra_specs = deepcopy(tpd.PowerMaxData.rep_extra_specs)
         rep_extra_specs[utils.PORTGROUPNAME] = self.data.port_group_name_f
         hostlunid, remote_port_group = self.common._attach_metro_volume(
@@ -804,10 +813,12 @@ class PowerMaxReplicationTest(test.TestCase):
             self.data.test_rep_group, self.data.rep_extra_specs)
         mock_rm.assert_called_once()
 
+    @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume',
+                       return_value=[])
     @mock.patch.object(masking.PowerMaxMasking, 'remove_and_reset_members')
     @mock.patch.object(masking.PowerMaxMasking,
                        'remove_volumes_from_storage_group')
-    def test_cleanup_group_replication(self, mock_rm, mock_rm_reset):
+    def test_cleanup_group_replication(self, mock_rm, mock_rm_reset, mock_sgs):
         self.common._cleanup_group_replication(
             self.data.array, self.data.test_vol_grp_name,
             [self.data.device_id], self.extra_specs, self.data.rep_config_sync)
diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py
index ec294eba7a..6add0bf6c4 100644
--- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py
+++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py
@@ -742,7 +742,9 @@ class PowerMaxRestTest(test.TestCase):
             mck_modify.assert_called_once_with(array, device_id,
                                                extend_vol_payload)
 
-    def test_legacy_delete_volume(self):
+    @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume',
+                       return_value=[])
+    def test_legacy_delete_volume(self, mock_sgs):
         device_id = self.data.device_id
         vb_except = exception.VolumeBackendAPIException
         with mock.patch.object(self.rest, 'delete_resource') as mock_delete, (
@@ -756,7 +758,9 @@ class PowerMaxRestTest(test.TestCase):
             mock_delete.assert_called_once_with(
                 self.data.array, 'sloprovisioning', 'volume', device_id)
 
-    def test_delete_volume(self):
+    @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume',
+                       return_value=[])
+    def test_delete_volume(self, mock_sgs):
         device_id = self.data.device_id
         self.rest.ucode_major_level = utils.UCODE_5978
         self.rest.ucode_minor_level = utils.UCODE_5978_HICKORY
@@ -765,6 +769,16 @@ class PowerMaxRestTest(test.TestCase):
             mock_delete.assert_called_once_with(
                 self.data.array, 'sloprovisioning', 'volume', device_id)
 
+    @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume',
+                       return_value=['OS-SG'])
+    def test_delete_volume_in_sg(self, mock_sgs):
+        device_id = self.data.device_id
+        self.rest.ucode_major_level = utils.UCODE_5978
+        self.rest.ucode_minor_level = utils.UCODE_5978_HICKORY
+        self.assertRaises(
+            exception.VolumeBackendAPIException,
+            self.rest.delete_volume, self.data.array, device_id)
+
     def test_rename_volume(self):
         device_id = self.data.device_id
         payload = {'editVolumeActionParam': {
diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py
index 2b8fbbabdc..da2c288278 100644
--- a/cinder/volume/drivers/dell_emc/powermax/common.py
+++ b/cinder/volume/drivers/dell_emc/powermax/common.py
@@ -2054,22 +2054,37 @@ class PowerMaxCommon(object):
 
         device_id = self._find_device_on_array(volume, extra_specs)
         if device_id is None:
-            LOG.error("Volume %(name)s not found on the array. "
-                      "No volume to delete.",
-                      {'name': volume_name})
+            LOG.warning("Volume %(name)s not found on the array. "
+                        "No volume to delete.",
+                        {'name': volume_name})
             return volume_name
 
         array = extra_specs[utils.ARRAY]
         if self.utils.is_replication_enabled(extra_specs):
             self._validate_rdfg_status(array, extra_specs)
 
+        self._cleanup_device_retry(array, device_id, extra_specs)
+
+        # Remove from any storage groups and cleanup replication
+        self._remove_vol_and_cleanup_replication(
+            array, device_id, volume_name, extra_specs, volume)
+        self._delete_from_srp(
+            array, device_id, volume_name, extra_specs)
+        return volume_name
+
+    @retry(retry_exc_tuple, interval=1, retries=3)
+    def _cleanup_device_retry(self, array, device_id, extra_specs):
+        """Cleanup snapvx on the device
+
+        :param array: the serial number of the array -- str
+        :param device_id: the device id -- str
+        :param extra_specs: extra specs -- dict
+        """
         # Check if the volume being deleted is a
         # source or target for copy session
         self._cleanup_device_snapvx(array, device_id, extra_specs)
         # Confirm volume has no more snapshots associated and is not a target
         snapshots = self.rest.get_volume_snapshot_list(array, device_id)
-        __, snapvx_target_details = self.rest.find_snap_vx_sessions(
-            array, device_id, tgt_only=True)
         if snapshots:
             snapshot_names = ', '.join(
                 snap.get('snapshotName') for snap in snapshots)
@@ -2077,27 +2092,17 @@ class PowerMaxCommon(object):
                 'Cannot delete device %s as it currently has the following '
                 'active snapshots: %s. Please try again once these snapshots '
                 'are no longer active.') % (device_id, snapshot_names))
+        __, snapvx_target_details = self.rest.find_snap_vx_sessions(
+            array, device_id, tgt_only=True)
         if snapvx_target_details:
             source_device = snapvx_target_details.get('source_vol_id')
             snapshot_name = snapvx_target_details.get('snap_name')
-            raise exception.VolumeBackendAPIException(_(
-                'Cannot delete device %s as it is currently a linked target '
-                'of snapshot %s. The source device of this link is %s. '
-                'Please try again once this snapshots is no longer '
-                'active.') % (device_id, snapshot_name, source_device))
-
-        # Remove from any storage groups and cleanup replication
-        self._remove_vol_and_cleanup_replication(
-            array, device_id, volume_name, extra_specs, volume)
-        # Check if volume is in any storage group
-        sg_list = self.rest.get_storage_groups_from_volume(array, device_id)
-        if sg_list:
-            LOG.error("Device %(device_id)s is in storage group(s) "
-                      "%(sg_list)s prior to delete. Delete will fail.",
-                      {'device_id': device_id, 'sg_list': sg_list})
-        self._delete_from_srp(
-            array, device_id, volume_name, extra_specs)
-        return volume_name
+            if snapshot_name:
+                raise exception.VolumeBackendAPIException(_(
+                    'Cannot delete device %s as it is currently a linked '
+                    'target of snapshot %s. The source device of this link '
+                    'is %s. Please try again once this snapshot is no longer '
+                    'active.') % (device_id, snapshot_name, source_device))
 
     def _create_volume(self, volume, volume_name, volume_size, extra_specs):
         """Create a volume.
@@ -2159,15 +2164,40 @@ class PowerMaxCommon(object):
                     array, volume, volume_name, volume_size, extra_specs,
                     storagegroup_name, rep_mode))
 
-        # Compare volume ID against identifier on array. Update if needed.
-        # This can occur in cases where multiple edits are occurring at once.
-        found_device_id = self.rest.find_volume_device_id(array, volume_name)
-        returning_device_id = volume_dict['device_id']
-        if found_device_id != returning_device_id:
-            volume_dict['device_id'] = found_device_id
+        device_id = self._get_device_id_from_identifier(
+            array, volume_name, volume_dict['device_id'])
+        if device_id:
+            volume_dict['device_id'] = device_id
 
         return volume_dict, rep_update, rep_info_dict
 
+    def _get_device_id_from_identifier(
+            self, array, volume_name, orig_device_id):
+        """Get the device(s) using the identifier name
+
+        :param array: the serial number of the array -- str
+        :param volume_name: the user supplied volume name -- str
+        :param orig_device_id: the original device id -- str
+        :returns: device id -- str
+        """
+        # Compare volume ID against identifier on array. Update if needed.
+        # This can occur in cases where multiple edits are occurring at once.
+        dev_id_from_identifier = self.rest.find_volume_device_id(
+            array, volume_name)
+        if isinstance(dev_id_from_identifier, list):
+            if orig_device_id in dev_id_from_identifier:
+                return orig_device_id
+        else:
+            if dev_id_from_identifier != orig_device_id:
+                LOG.warning(
+                    "The device id %(dev_ident)s associated with %(vol_name)s "
+                    "is not the same as device %(dev_orig)s.",
+                    {'dev_ident': dev_id_from_identifier,
+                     'vol_name': volume_name,
+                     'dev_orig': orig_device_id})
+                return dev_id_from_identifier
+        return None
+
     @coordination.synchronized("emc-nonrdf-vol-{storagegroup_name}-{array}")
     def _create_non_replicated_volume(
             self, array, volume, volume_name, storagegroup_name, volume_size,
@@ -2192,6 +2222,7 @@ class PowerMaxCommon(object):
             return volume_dict
         except Exception as e:
             try:
+                self._reset_identifier_on_rollback(array, volume_name)
                 # Attempt cleanup of storage group post exception.
                 updated_devices = set(self.rest.get_volumes_in_storage_group(
                     array, storagegroup_name))
@@ -2511,6 +2542,7 @@ class PowerMaxCommon(object):
                 LOG.info("The tag list %(tag_list)s has been verified.",
                          {'tag_list': array_tag_list})
 
+    @retry(retry_exc_tuple, interval=3, retries=3)
     def _delete_from_srp(self, array, device_id, volume_name,
                          extra_specs):
         """Delete from srp.
@@ -2527,11 +2559,17 @@ class PowerMaxCommon(object):
             self.provision.delete_volume_from_srp(
                 array, device_id, volume_name)
         except Exception as e:
-            error_message = (_("Failed to delete volume %(volume_name)s. "
-                               "Exception received: %(e)s") %
-                             {'volume_name': volume_name,
-                              'e': six.text_type(e)})
+            error_message = (_(
+                "Failed to delete volume %(volume_name)s with device id "
+                "%(dev)s. Exception received: %(e)s.") %
+                {'volume_name': volume_name,
+                 'dev': device_id,
+                 'e': six.text_type(e)})
             LOG.error(error_message)
+            LOG.warning("Attempting device cleanup after a failed delete of: "
+                        "%(name)s. device_id: %(device_id)s.",
+                        {'name': volume_name, 'device_id': device_id})
+            self._cleanup_device_snapvx(array, device_id, extra_specs)
             raise exception.VolumeBackendAPIException(message=error_message)
 
     def _remove_vol_and_cleanup_replication(
@@ -2946,7 +2984,7 @@ class PowerMaxCommon(object):
 
         return source_device_id
 
-    @retry(retry_exc_tuple, interval=1, retries=3)
+    @retry(retry_exc_tuple, interval=10, retries=3)
     def _cleanup_device_snapvx(
             self, array, device_id, extra_specs):
         """Perform any snapvx cleanup before creating clones or snapshots
@@ -2992,17 +3030,15 @@ class PowerMaxCommon(object):
         :param array: the array serial number
         :param extra_specs: extra specifications
         """
-        try:
-            session_unlinked = self._unlink_snapshot(
-                session, array, extra_specs)
-            if session_unlinked:
-                self._delete_temp_snapshot(session, array)
-        except exception.VolumeBackendAPIException as e:
-            # Ignore and continue as snapshot has been unlinked
-            # successfully with incorrect status code returned
-            if ('404' and session['snap_name'] and
-                    'does not exist' in six.text_type(e)):
-                pass
+        session_unlinked = self._unlink_snapshot(
+            session, array, extra_specs)
+        if session_unlinked:
+            self._delete_temp_snapshot(session, array)
+        else:
+            LOG.warning(
+                "Snap name %(snap)s is still linked. The delete of "
+                "the temporary snapshot has not occurred.",
+                {'snap': session.get('snap_name')})
 
     def _unlink_snapshot(self, session, array, extra_specs):
         """Helper for unlinking temporary snapshot during cleanup.
@@ -3015,10 +3051,19 @@ class PowerMaxCommon(object):
         snap_name = session.get('snap_name')
         source = session.get('source_vol_id')
         snap_id = session.get('snapid')
+        if snap_id is None:
+            exception_message = (
+                _("Unable to get snapid from session %(session)s for source "
+                  "device %(dev)s.  Retrying...")
+                % {'dev': source, 'session': session})
+            # Warning only as there will be a retry
+            LOG.warning(exception_message)
+            raise exception.VolumeBackendAPIException(
+                message=exception_message)
 
         snap_info = self.rest.get_volume_snap(
             array, source, snap_name, snap_id)
-        is_linked = snap_info.get('linkedDevices')
+        is_linked = snap_info.get('linkedDevices') if snap_info else None
 
         target, cm_enabled = None, False
         if session.get('target_vol_id'):
@@ -3254,7 +3299,7 @@ class PowerMaxCommon(object):
         volume_identifier = None
         # Check if volume is already cinder managed
         if volume_details.get('volume_identifier'):
-            volume_identifier = volume_details['volume_identifier']
+            volume_identifier = volume_details.get('volume_identifier')
             if volume_identifier.startswith(utils.VOLUME_ELEMENT_NAME_PREFIX):
                 raise exception.ManageExistingAlreadyManaged(
                     volume_ref=volume_id)
@@ -3571,6 +3616,7 @@ class PowerMaxCommon(object):
                      "they can be managed into Cinder.")
             return manageable_vols
 
+        volumes = volumes or list()
         for device in volumes:
             # Determine if volume is valid for management
             if self.utils.is_volume_manageable(device):
@@ -3679,6 +3725,7 @@ class PowerMaxCommon(object):
                      "Cinder.")
             return manageable_snaps
 
+        volumes = volumes or list()
         for device in volumes:
             # Determine if volume is valid for management
             manageable_snaps = self._is_snapshot_valid_for_management(
@@ -4699,7 +4746,7 @@ class PowerMaxCommon(object):
             if self.rest.is_next_gen_array(target_array_serial):
                 target_workload = 'NONE'
         except IndexError:
-            LOG.error("Error parsing array, pool, SLO and workload.")
+            LOG.debug("Error parsing array, pool, SLO and workload.")
             return false_ret
 
         if self.promotion:
@@ -6023,7 +6070,7 @@ class PowerMaxCommon(object):
             # Get the volume group dict for getting the group name
             volume_group = (self._find_volume_group(array, source_group))
             if volume_group and volume_group.get('name'):
-                vol_grp_name = volume_group['name']
+                vol_grp_name = volume_group.get('name')
             if vol_grp_name is None:
                 exception_message = (
                     _("Cannot find generic volume group %(grp_id)s.") %
@@ -7039,6 +7086,8 @@ class PowerMaxCommon(object):
         :param device_id: the device ID
         :returns: dict -- volume metadata
         """
+        if device_id is None:
+            return dict()
         vol_info = self.rest._get_private_volume(array, device_id)
         vol_header = vol_info['volumeHeader']
         array_model, __ = self.rest.get_array_model_info(array)
@@ -7508,3 +7557,26 @@ class PowerMaxCommon(object):
                 management_sg_name, rdf_group_number, missing_volumes_str)
             is_valid = False
         return is_valid
+
+    def _reset_identifier_on_rollback(self, array, volume_name):
+        """Reset the user supplied name on a rollback
+
+        :param array: the serial number -- str
+        :param volume_name: the volume name assigned -- str
+        """
+        # Find volume based on identifier name
+        dev_id_from_identifier = self.rest.find_volume_device_id(
+            array, volume_name)
+        if dev_id_from_identifier and isinstance(
+                dev_id_from_identifier, str):
+            vol_identifier_name = self.rest.find_volume_identifier(
+                array, dev_id_from_identifier)
+            if vol_identifier_name and (
+                    vol_identifier_name == volume_name):
+                LOG.warning(
+                    "Attempting to reset name of %(vol_name)s on device "
+                    "%(dev_ident)s on a create volume rollback operation.",
+                    {'vol_name': volume_name,
+                     'dev_ident': dev_id_from_identifier})
+                self.rest.rename_volume(
+                    array, dev_id_from_identifier, None)
diff --git a/cinder/volume/drivers/dell_emc/powermax/masking.py b/cinder/volume/drivers/dell_emc/powermax/masking.py
index 746ca3e6e0..0dd5b01bd6 100644
--- a/cinder/volume/drivers/dell_emc/powermax/masking.py
+++ b/cinder/volume/drivers/dell_emc/powermax/masking.py
@@ -91,12 +91,14 @@ class PowerMaxMasking(object):
             error_message = self._get_or_create_masking_view(
                 serial_number, masking_view_dict,
                 default_sg_name, extra_specs)
-            LOG.debug(
-                "The masking view in the attach operation is "
-                "%(masking_name)s. The storage group "
-                "in the masking view is %(storage_name)s.",
-                {'masking_name': maskingview_name,
-                 'storage_name': storagegroup_name})
+            # Check that the device is in the correct storage group
+            if not self._validate_attach(
+                    serial_number, device_id, storagegroup_name,
+                    maskingview_name):
+                error_message = ("The attach validation for device "
+                                 "%(dev)s was unsuccessful.")
+                raise exception.VolumeBackendAPIException(
+                    message=error_message)
             rollback_dict['portgroup_name'] = (
                 self.rest.get_element_from_masking_view(
                     serial_number, maskingview_name, portgroup=True))
@@ -132,6 +134,71 @@ class PowerMaxMasking(object):
 
         return rollback_dict
 
+    def _validate_attach(
+            self, serial_number, device_id, dest_sg_name, dest_mv_name):
+        """Validate that the attach was successful.
+
+        :param serial_number: the array serial number
+        :param device_id: the device id
+        :param dest_sg_name: the correct storage group
+        :param dest_mv_name: the correct masking view
+        :returns: bool
+        """
+        sg_list = self.rest.get_storage_groups_from_volume(
+            serial_number, device_id)
+
+        def _validate_masking_view():
+            mv_list = self.rest.get_masking_views_from_storage_group(
+                serial_number, dest_sg_name)
+            if not mv_list:
+                LOG.error(
+                    "The masking view list of %(sg_name)s where device "
+                    "%(dev)s exists is empty.",
+                    {'sg_name': dest_sg_name,
+                     'dev': device_id})
+                return False
+
+            if dest_mv_name.lower() in (
+                    mv_name.lower() for mv_name in mv_list):
+                LOG.debug(
+                    "The masking view in the attach operation is "
+                    "%(masking_name)s. The storage group "
+                    "in the masking view is %(storage_name)s. "
+                    "The device id is %(dev)s.",
+                    {'masking_name': dest_mv_name,
+                     'storage_name': dest_sg_name,
+                     'dev': device_id})
+                return True
+            else:
+                LOG.error(
+                    "The masking view %(masking_name)s is not in "
+                    "the masking view list %(mv_list)s. "
+                    "%(sg_name)s is the storage group where device "
+                    "%(dev)s exists.",
+                    {'masking_name': dest_mv_name,
+                     'mv_list': mv_list,
+                     'sg_name': dest_sg_name,
+                     'dev': device_id})
+                return False
+
+        if not sg_list:
+            LOG.error(
+                "Device %(dev)s does not exist in any storage group.",
+                {'dev': device_id})
+            return False
+        if dest_sg_name.lower() in (
+                sg_name.lower() for sg_name in sg_list):
+            return _validate_masking_view()
+        else:
+            LOG.error(
+                "The storage group %(sg_name)s is not in "
+                "the storage group list %(sg_list)s "
+                "where device %(dev)s exists.",
+                {'sg_name': dest_sg_name,
+                 'sg_list': sg_list,
+                 'dev': device_id})
+            return False
+
     def _move_vol_from_default_sg(
             self, serial_number, device_id, volume_name,
             default_sg_name, dest_storagegroup, extra_specs,
@@ -727,6 +794,13 @@ class PowerMaxMasking(object):
                      {'volume_name': volume_name,
                       'sg_name': storagegroup_name})
         else:
+            storage_group_list = self.rest.get_storage_groups_from_volume(
+                serial_number, device_id)
+            if storage_group_list:
+                msg = self._check_add_volume_suitability(
+                    serial_number, device_id, volume_name, storagegroup_name)
+                if msg:
+                    return msg
             try:
                 force = True if extra_specs.get(utils.IS_RE) else False
                 self.add_volume_to_storage_group(
@@ -740,6 +814,42 @@ class PowerMaxMasking(object):
                 LOG.error(msg)
         return msg
 
+    def _check_add_volume_suitability(
+            self, serial_number, device_id, volume_name, add_sg_name):
+        """Check if possible to add a volume to a storage group
+
+        If a volume already belongs to a storage group that is associated
+        with FAST it is not possible to add that same volume to another
+        storage group which is also associated with FAST
+
+        :param serial_number: the array serial number
+        :param device_id: the device id
+        :param volume_name: the client supplied volume name
+        :param add_sg_name: storage group that the volume is to be added to
+        :returns: msg -- str or None
+        """
+        storage_group = self.rest.get_storage_group(
+            serial_number, add_sg_name)
+        if storage_group and storage_group.get('slo') and (
+                storage_group.get('slo').lower() == 'none'):
+            return None
+        storage_group_list = self.rest.get_storage_groups_from_volume(
+            serial_number, device_id)
+        if storage_group_list:
+            msg = ("Volume %(vol)s with device id %(dev)s belongs "
+                   "to storage group(s) %(sgs)s. Cannot add volume "
+                   "to another storage group associated with FAST."
+                   % {'vol': volume_name, 'dev': device_id,
+                      'sgs': storage_group_list})
+            for storage_group_name in storage_group_list:
+                storage_group = self.rest.get_storage_group(
+                    serial_number, storage_group_name)
+                if storage_group and storage_group.get('slo') and (
+                        storage_group.get('slo').lower() != 'none'):
+                    LOG.error(msg)
+                    return msg
+        return None
+
     def add_volume_to_storage_group(
             self, serial_number, device_id, storagegroup_name,
             volume_name, extra_specs, force=False):
diff --git a/cinder/volume/drivers/dell_emc/powermax/provision.py b/cinder/volume/drivers/dell_emc/powermax/provision.py
index 90c4112186..a2af02011f 100644
--- a/cinder/volume/drivers/dell_emc/powermax/provision.py
+++ b/cinder/volume/drivers/dell_emc/powermax/provision.py
@@ -121,8 +121,9 @@ class PowerMaxProvision(object):
         :param volume_name: the volume name
         """
         start_time = time.time()
-        LOG.debug("Delete volume %(volume_name)s from srp.",
-                  {'volume_name': volume_name})
+        LOG.debug("Delete volume %(volume_name)s with device id %(dev)s "
+                  "from srp.", {'volume_name': volume_name,
+                                'dev': device_id})
         self.rest.delete_volume(array, device_id)
         LOG.debug("Delete volume took: %(delta)s H:MM:SS.",
                   {'delta': self.utils.get_time_delta(
diff --git a/cinder/volume/drivers/dell_emc/powermax/rest.py b/cinder/volume/drivers/dell_emc/powermax/rest.py
index 7e57efbf4e..fc8bdc05ae 100644
--- a/cinder/volume/drivers/dell_emc/powermax/rest.py
+++ b/cinder/volume/drivers/dell_emc/powermax/rest.py
@@ -1242,6 +1242,11 @@ class PowerMaxRest(object):
                     element_name = self.utils.get_volume_element_name(name_id)
                     if vol_identifier == element_name:
                         found_device_id = device_id
+            else:
+                LOG.error("We cannot verify that device %(dev)s was "
+                          "created/managed by openstack by its "
+                          "identifier name.", {'dev': device_id})
+
         return found_device_id
 
     def add_vol_to_sg(self, array, storagegroup_name, device_id, extra_specs,
@@ -1546,6 +1551,9 @@ class PowerMaxRest(object):
         :param device_id: the volume device id
         :param new_name: the new name for the volume, can be None
         """
+        if not device_id:
+            LOG.warning('No device id supplied to rename operation.')
+            return
         if new_name is not None:
             vol_identifier_dict = {
                 "identifier_name": new_name,
@@ -1563,6 +1571,16 @@ class PowerMaxRest(object):
         :param array: the array serial number
         :param device_id: volume device id
         """
+        # Check if volume is in any storage group
+        sg_list = self.get_storage_groups_from_volume(
+            array, device_id)
+        if sg_list:
+            exception_message = (_(
+                "Device %(device_id)s is in storage group(s) "
+                "%(sg_list)s prior to delete.")
+                % {'device_id': device_id, 'sg_list': sg_list})
+            LOG.error(exception_message)
+            raise exception.VolumeBackendAPIException(exception_message)
 
         if ((self.ucode_major_level >= utils.UCODE_5978)
                 and (self.ucode_minor_level > utils.UCODE_5978_ELMSR) or (
@@ -1572,7 +1590,8 @@ class PowerMaxRest(object):
                 self.delete_resource(array, SLOPROVISIONING,
                                      "volume", device_id)
             except Exception as e:
-                LOG.warning('Delete volume failed with %(e)s.', {'e': e})
+                LOG.warning('Delete volume %(dev)s failed with %(e)s.',
+                            {'dev': device_id, 'e': e})
                 raise
         else:
             # Pre-Foxtail, deallocation and delete are separate calls
@@ -1584,8 +1603,9 @@ class PowerMaxRest(object):
                 self._modify_volume(array, device_id, payload)
                 pass
             except Exception as e:
-                LOG.warning('Deallocate volume failed with %(e)s.'
-                            'Attempting delete.', {'e': e})
+                LOG.warning('Deallocate volume %(dev)s failed with %(e)s.'
+                            'Attempting delete.',
+                            {'dev': device_id, 'e': e})
                 # Try to delete the volume if deallocate failed.
                 self.delete_resource(array, SLOPROVISIONING,
                                      "volume", device_id)
@@ -1664,7 +1684,16 @@ class PowerMaxRest(object):
         :returns: storagegroup_list
         """
         sg_list = []
+        if not device_id:
+            return sg_list
         vol = self.get_volume(array, device_id)
+        if vol and isinstance(vol, list):
+            LOG.warning(
+                "Device id %(dev_id)s has brought back "
+                "multiple volume objects.",
+                {'vol_name': device_id})
+            return sg_list
+
         if vol and vol.get('storageGroupId'):
             sg_list = vol['storageGroupId']
         num_storage_groups = len(sg_list)
@@ -1696,13 +1725,19 @@ class PowerMaxRest(object):
         """
         device_id = None
         params = {"volume_identifier": volume_name}
-
-        volume_list = self.get_volume_list(array, params)
-        if not volume_list:
-            LOG.debug("Cannot find record for volume %(volumeId)s.",
-                      {'volumeId': volume_name})
+        device_list = self.get_volume_list(array, params)
+        if not device_list:
+            LOG.debug("Cannot find record for volume %(vol_name)s.",
+                      {'vol_name': volume_name})
         else:
-            device_id = volume_list[0]
+            LOG.debug("The device id list is %(dev_list)s for %(vol_name)s.",
+                      {'dev_list': device_list,
+                       'vol_name': volume_name})
+            device_id = device_list[0] if len(device_list) == 1 else (
+                device_list)
+            if isinstance(device_id, list):
+                LOG.warning("More than one devices returned for %(vol_name)s.",
+                            {'vol_name': volume_name})
         return device_id
 
     def find_volume_identifier(self, array, device_id):
@@ -1713,7 +1748,7 @@ class PowerMaxRest(object):
         :returns: the volume identifier -- string
         """
         vol = self.get_volume(array, device_id)
-        return vol['volume_identifier']
+        return vol.get('volume_identifier') if vol else None
 
     def get_size_of_device_on_array(self, array, device_id):
         """Get the size of the volume from the array.
-- 
GitLab


From 21a1d1a6289e7077203e262b27c216b584565e02 Mon Sep 17 00:00:00 2001
From: Nilesh Thathagar <nilesh.thathagar@dell.com>
Date: Fri, 17 Jan 2025 06:25:50 +0000
Subject: [PATCH 2/2] Dell PowerMax: Enhanced the retry mechanism to verify the
 active snapshots.

Improved the retry mechanism of ``_cleanup_device_retry``
to account for active snapshots that take longer than
expected to be removed, thereby preventing deletion errors.

Updated the retry count based on trial and error.
The maximum observed retries reached the 6th attempt,
so we have set it to 6+1=7 retries.

Resolves: OSPRH-17708 (2/2)
Upstream-Closes-Bug: #2092259
Change-Id: I9a09eb8e5b6efee59688684211d893e1af055241
(cherry picked from commit 2155d681571337e3aa6f8ff02268dc8555213d05)
(cherry picked from commit e9315330fbc540948a1b700195a12a737f9a08c2)
(cherry picked from commit 86f282daeb1d76805c794853fd0b5f3f0158eda5)
Conflicts:
	cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py
---
 .../dell_emc/powermax/test_powermax_common.py | 31 +++++++++++++++++++
 .../drivers/dell_emc/powermax/common.py       |  2 +-
 ...-fix-active-snapshot-ccc3f9b6251d2634.yaml | 12 +++++++
 3 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 releasenotes/notes/bug-2092259-dell-powermax-volume-delete-failed-fix-active-snapshot-ccc3f9b6251d2634.yaml

diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py
index ebc773585d..2e3feae403 100644
--- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py
+++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py
@@ -4538,3 +4538,34 @@ class PowerMaxCommonTest(test.TestCase):
             self.data.array, 'vol_name', self.data.device_id,
             self.data.extra_specs)
         mock_clean.assert_called_once()
+
+    @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx')
+    @mock.patch.object(rest.PowerMaxRest, 'get_volume_snapshot_list',
+                       side_effect=([{'snapshotName': 'temp-clone-snapshot'}],
+                                    []))
+    @mock.patch.object(rest.PowerMaxRest, 'find_snap_vx_sessions',
+                       side_effect=[(None, None)])
+    def test_cleanup_device_retry_1(self, mock_snapvx,
+                                    mock_ss_list, mock_clean):
+        self.common._cleanup_device_retry(
+            self.data.array, self.data.device_id, self.data.extra_specs)
+        self.assertEqual(2, mock_ss_list.call_count)
+        self.assertEqual(1, mock_snapvx.call_count)
+        self.assertEqual(2, mock_clean.call_count)
+
+    @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx')
+    @mock.patch.object(rest.PowerMaxRest, 'get_volume_snapshot_list',
+                       return_value=[{'snapshotName': 'temp-clone-snapshot'}])
+    @mock.patch.object(rest.PowerMaxRest, 'find_snap_vx_sessions',
+                       side_effect=[(None, None)])
+    def test_cleanup_device_retry_2(self, mock_snapvx,
+                                    mock_ss_list, mock_clean):
+        self.assertRaises(
+            exception.VolumeBackendAPIException,
+            self.common._cleanup_device_retry,
+            self.data.array,
+            self.data.device_id,
+            self.data.extra_specs)
+        self.assertEqual(7, mock_ss_list.call_count)
+        self.assertEqual(0, mock_snapvx.call_count)
+        self.assertEqual(7, mock_clean.call_count)
diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py
index da2c288278..d9c4970a6f 100644
--- a/cinder/volume/drivers/dell_emc/powermax/common.py
+++ b/cinder/volume/drivers/dell_emc/powermax/common.py
@@ -2072,7 +2072,7 @@ class PowerMaxCommon(object):
             array, device_id, volume_name, extra_specs)
         return volume_name
 
-    @retry(retry_exc_tuple, interval=1, retries=3)
+    @retry(retry_exc_tuple, interval=2, retries=7)
     def _cleanup_device_retry(self, array, device_id, extra_specs):
         """Cleanup snapvx on the device
 
diff --git a/releasenotes/notes/bug-2092259-dell-powermax-volume-delete-failed-fix-active-snapshot-ccc3f9b6251d2634.yaml b/releasenotes/notes/bug-2092259-dell-powermax-volume-delete-failed-fix-active-snapshot-ccc3f9b6251d2634.yaml
new file mode 100644
index 0000000000..88c829b1e0
--- /dev/null
+++ b/releasenotes/notes/bug-2092259-dell-powermax-volume-delete-failed-fix-active-snapshot-ccc3f9b6251d2634.yaml
@@ -0,0 +1,12 @@
+---
+fixes:
+  - |
+    Dell PowerMax Driver `bug #2092259
+    <https://bugs.launchpad.net/cinder/+bug/2092259>`_: Before
+    a volume can be deleted, the driver issues a command to clean up
+    active snapshots in the backend and then polls the backend
+    to make sure the cleanup has occurred.
+    This fix enhances the polling mechanism to give the backend
+    more time to do the cleanup, thereby increasing the probability
+    that the driver will be able to make a successful volume deletion
+    request.
-- 
GitLab

