PowerMax Driver - Volume group delete failure

1. Volume group deletion failed after a group snapshot had been created
on and deleted from it. This fix eliminates the "cannot use the device for
the function because it is a Copy Session source" on cleanup.
2. Fix to update the metadata correctly for a generic group.

Change-Id: I558e9e67e427b40708220046eef953cb09ea4917
Closes-Bug: #1853589

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

@@ -3101,3 +3101,17 @@ class PowerMaxCommonTest(test.TestCase):
         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)}]
+
+        existing_metadata = None
+
+        object_metadata = {'device-meta-key-1': 'device-meta-value-1',
+                           'device-meta-key-2': 'device-meta-value-2'}
+
+        self.assertRaises(
+            exception.VolumeBackendAPIException,
+            self.common.update_metadata, model_update, existing_metadata,
+            object_metadata)

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

@@ -454,15 +454,39 @@ class PowerMaxProvisionTest(test.TestCase):
         array = self.data.array
         snap_name = self.data.group_snapshot_name
         source_group_name = self.data.storagegroup_name_source
-        extra_specs = self.data.extra_specs
-        src_dev_ids = [self.data.device_id]
         with mock.patch.object(
                 self.provision,
                 'delete_group_replica') as mock_delete_replica:
             self.provision.delete_group_replica(
-                array, snap_name, source_group_name, src_dev_ids, extra_specs)
+                array, snap_name, source_group_name)
             mock_delete_replica.assert_called_once_with(
-                array, snap_name, source_group_name, src_dev_ids, extra_specs)
+                array, snap_name, source_group_name)
+
+    @mock.patch.object(rest.PowerMaxRest,
+                       'get_storagegroup_snap_generation_list',
+                       side_effect=[['0', '3', '1', '2'],
+                                    ['0', '1'], ['0'], list()])
+    def test_delete_group_replica_side_effect(self, mock_list):
+        array = self.data.array
+        snap_name = self.data.group_snapshot_name
+        source_group_name = self.data.storagegroup_name_source
+        with mock.patch.object(
+                self.rest, 'delete_storagegroup_snap') as mock_del:
+            self.provision.delete_group_replica(
+                array, snap_name, source_group_name)
+            self.assertEqual(4, mock_del.call_count)
+            mock_del.reset_mock()
+            self.provision.delete_group_replica(
+                array, snap_name, source_group_name)
+            self.assertEqual(2, mock_del.call_count)
+            mock_del.reset_mock()
+            self.provision.delete_group_replica(
+                array, snap_name, source_group_name)
+            self.assertEqual(1, mock_del.call_count)
+            mock_del.reset_mock()
+            self.provision.delete_group_replica(
+                array, snap_name, source_group_name)
+            mock_del.assert_not_called()
 
     def test_link_and_break_replica(self):
         array = self.data.array

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

@@ -1545,6 +1545,27 @@ class PowerMaxRestTest(test.TestCase):
             mock_create.assert_called_once_with(
                 array, source_group, snap_name, extra_specs)
 
+    def test_delete_storagegroup_snap(self):
+        array = self.data.array
+        source_group = self.data.storagegroup_name_source
+        snap_name = self.data.group_snapshot_name
+        with mock.patch.object(
+                self.rest, 'delete_storagegroup_snap') as mock_delete:
+            self.rest.delete_storagegroup_snap(
+                array, source_group, snap_name, '0')
+            mock_delete.assert_called_once_with(
+                array, source_group, snap_name, '0')
+
+    @mock.patch.object(rest.PowerMaxRest, 'get_resource',
+                       return_value={'generations': ['0', '1']})
+    def test_get_storagegroup_snap_generation_list(self, mock_list):
+        array = self.data.array
+        source_group = self.data.storagegroup_name_source
+        snap_name = self.data.group_snapshot_name
+        ret_list = self.rest.get_storagegroup_snap_generation_list(
+            array, source_group, snap_name)
+        self.assertEqual(['0', '1'], ret_list)
+
     def test_get_storagegroup_rdf_details(self):
         details = self.rest.get_storagegroup_rdf_details(
             self.data.array, self.data.test_vol_grp_name,

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

@@ -4458,16 +4458,17 @@ class PowerMaxCommon(object):
             src_dev_id = self._get_src_device_id_for_group_snap(snapshot)
             extra_specs = self._initial_setup(snapshot.volume)
             array = extra_specs['array']
+            snapshot_model_dict = {
+                'id': snapshot.id,
+                'provider_location': six.text_type(
+                    {'source_id': src_dev_id, 'snap_name': snap_name}),
+                'status': fields.SnapshotStatus.AVAILABLE}
 
-            snapshots_model_update.append(
+            snapshot_model_dict = self.update_metadata(
-                {'id': snapshot.id,
+                snapshot_model_dict, snapshot.metadata,
-                 'provider_location': six.text_type(
-                     {'source_id': src_dev_id, 'snap_name': snap_name}),
-                 'status': fields.SnapshotStatus.AVAILABLE})
-            snapshots_model_update = self.update_metadata(
-                snapshots_model_update, snapshot.metadata,
                 self.get_snapshot_metadata(
                     array, src_dev_id, snap_name))
+            snapshots_model_update.append(snapshot_model_dict)
         model_update = {'status': fields.GroupStatus.AVAILABLE}
 
         return model_update, snapshots_model_update
@@ -4555,16 +4556,10 @@ class PowerMaxCommon(object):
                     {'group_id': source_group.id})
                 raise exception.VolumeBackendAPIException(
                     message=exception_message)
-            # Check if the snapshot exists
+
-            if 'snapVXSnapshots' in volume_group:
+            self.provision.delete_group_replica(
-                if snap_name in volume_group['snapVXSnapshots']:
+                array, snap_name, vol_grp_name)
-                    src_devs = self._get_snap_src_dev_list(array, snapshots)
+
-                    self.provision.delete_group_replica(
-                        array, snap_name, vol_grp_name, src_devs, extra_specs)
-            else:
-                # Snapshot has been already deleted, return successfully
-                LOG.error("Cannot find group snapshot %(snapId)s.",
-                          {'snapId': group_snapshot.id})
             model_update = {'status': fields.GroupSnapshotStatus.DELETED}
             for snapshot in snapshots:
                 snapshots_model_update.append(
@@ -4945,12 +4940,9 @@ class PowerMaxCommon(object):
             # Delete the snapshot if required
             if rollback_dict.get("snap_name"):
                 try:
-                    src_dev_ids = [
-                        a for a, b in rollback_dict['list_volume_pairs']]
                     self.provision.delete_group_replica(
                         array, rollback_dict["snap_name"],
-                        rollback_dict["source_group_name"],
+                        rollback_dict["source_group_name"])
-                        src_dev_ids, rollback_dict['interval_retries_dict'])
                 except Exception as e:
                     LOG.debug("Failed to delete group snapshot. Attempting "
                               "further rollback. Exception received: %(e)s.",
@@ -5341,28 +5333,46 @@ class PowerMaxCommon(object):
                 message=exception_message)
 
     def update_metadata(
-            self, model_update, existing_metadata, object_metadata):
+            self, model_update, existing_metadata, new_metadata):
         """Update volume metadata in model_update.
 
         :param model_update: existing model
         :param existing_metadata: existing metadata
-        :param object_metadata: object metadata
+        :param new_metadata: new object metadata
         :returns: dict -- updated model
         """
+        if new_metadata:
+            self._is_dict(new_metadata, 'new object metadata')
         if model_update:
+            self._is_dict(model_update, 'existing model')
             if 'metadata' in model_update:
-                model_update['metadata'].update(object_metadata)
+                model_update['metadata'].update(new_metadata)
             else:
-                model_update.update({'metadata': object_metadata})
+                model_update.update({'metadata': new_metadata})
         else:
             model_update = {}
-            model_update.update({'metadata': object_metadata})
+            model_update.update({'metadata': new_metadata})
 
         if existing_metadata:
+            self._is_dict(existing_metadata, 'existing metadata')
             model_update['metadata'].update(existing_metadata)
 
         return model_update
 
+    def _is_dict(self, input, description):
+        """Check that the input is a dict
+
+        :param input: object for checking
+        :raises: VolumeBackendAPIException
+        """
+        if not isinstance(input, dict):
+            exception_message = (_(
+                "Input %(desc)s is not a dict.") % {'desc': description})
+
+            LOG.error(exception_message)
+            raise exception.VolumeBackendAPIException(
+                message=exception_message)
+
     def get_volume_metadata(self, array, device_id):
         """Get volume metadata for model_update.
 

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

@@ -392,8 +392,9 @@ class PowerMaxProvision(object):
             self._unlink_volume(array, "", "", snap_name, extra_specs,
                                 list_volume_pairs=list_device_pairs,
                                 generation=generation)
-        self.delete_volume_snap(array, snap_name, source_devices,
+        if source_devices:
-                                restored=False, generation=generation)
+            self.delete_volume_snap(array, snap_name, source_devices,
+                                    restored=False, generation=generation)
 
     def extend_volume(self, array, device_id, new_size, extra_specs,
                       rdf_group=None):
@@ -701,8 +702,7 @@ class PowerMaxProvision(object):
         self.rest.create_storagegroup_snap(
             array, source_group, snap_name, extra_specs)
 
-    def delete_group_replica(self, array, snap_name, source_group_name,
+    def delete_group_replica(self, array, snap_name, source_group_name):
-                             src_dev_ids, extra_specs):
         """Delete the snapshot.
 
         :param array: the array serial number
@@ -711,12 +711,19 @@ class PowerMaxProvision(object):
         :param src_dev_ids: the list of source device ids
         :param extra_specs: extra specifications
         """
-        # Delete snapvx snapshot
         LOG.debug("Deleting Snap Vx snapshot: source group: %(srcGroup)s "
                   "snapshot: %(snap_name)s.",
                   {'srcGroup': source_group_name, 'snap_name': snap_name})
-        self.delete_volume_snap_check_for_links(
+        gen_list = self.rest.get_storagegroup_snap_generation_list(
-            array, snap_name, src_dev_ids, extra_specs)
+            array, source_group_name, snap_name)
+        if gen_list:
+            gen_list.sort(reverse=True)
+            for gen in gen_list:
+                self.rest.delete_storagegroup_snap(
+                    array, source_group_name, snap_name, gen)
+        else:
+            LOG.debug("Unable to get generation number(s) for: %(srcGroup)s.",
+                      {'srcGroup': source_group_name})
 
     def link_and_break_replica(self, array, source_group_name,
                                target_group_name, snap_name, extra_specs,

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

@@ -2461,6 +2461,43 @@ class PowerMaxRest(object):
         self.wait_for_job('Create storage group snapVx', status_code,
                           job, extra_specs)
 
+    def delete_storagegroup_snap(self, array, source_group,
+                                 snap_name, generation='0'):
+        """Delete a snapVx snapshot of a storage group.
+
+        :param array: the array serial number
+        :param source_group: the source group name
+        :param snap_name: the name of the snapshot
+        :param generation: the generation number of the SnapVX
+        """
+        resource_name = ("%(sg_name)s/snapshot/%(snap_name)s"
+                         "/generation/%(generation)s"
+                         % {'sg_name': source_group, 'snap_name': snap_name,
+                            'generation': generation})
+
+        self.delete_resource(
+            array, REPLICATION, 'storagegroup', resource_name=resource_name)
+
+    def get_storagegroup_snap_generation_list(
+            self, array, source_group, snap_name):
+        """Get a snapshot and its generation count information for an sg.
+
+        The most recent snapshot will have a gen number of 0. The oldest
+        snapshot will have a gen number = genCount - 1 (i.e. if there are 4
+        generations of particular snapshot, the oldest will have a gen num of
+        3).
+
+        :param array: name of the array -- str
+        :param source_group: name of the storage group -- str
+        :param snap_name: the name of the snapshot -- str
+        :returns: generation numbers -- list
+        """
+        resource_name = ("%(sg_name)s/snapshot/%(snap_name)s/generation"
+                         % {'sg_name': source_group, 'snap_name': snap_name})
+        response = self.get_resource(array, REPLICATION, 'storagegroup',
+                                     resource_name=resource_name)
+        return response.get('generations', list()) if response else list()
+
     def get_storagegroup_rdf_details(self, array, storagegroup_name,
                                      rdf_group_num):
         """Get the remote replication details of a storage group.

releasenotes/notes/powermax-bug-1853589-f6c7164177da0496.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    PowerMax driver - fix to eliminate 'cannot use the device for the function
+    because it is in a Copy Session' when attempting to delete a volume group
+    that previously had a group snapshot created on and deleted from it.