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 7fb327c56a1..7a22ad92406 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 @@ -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) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_provision.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_provision.py index 95e8d0c4a2c..3d83342aa69 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_provision.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_provision.py @@ -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 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 dc1efc0dd80..6abbb6b2a28 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 @@ -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, diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py index 2db1204ee49..d44a96d0080 100644 --- a/cinder/volume/drivers/dell_emc/powermax/common.py +++ b/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( - {'id': snapshot.id, - '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, + snapshot_model_dict = self.update_metadata( + snapshot_model_dict, 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: - if snap_name in volume_group['snapVXSnapshots']: - 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}) + + self.provision.delete_group_replica( + array, snap_name, vol_grp_name) + 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"], - src_dev_ids, rollback_dict['interval_retries_dict']) + rollback_dict["source_group_name"]) 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. diff --git a/cinder/volume/drivers/dell_emc/powermax/provision.py b/cinder/volume/drivers/dell_emc/powermax/provision.py index 02c4b8c07a5..adba0bbd1b1 100644 --- a/cinder/volume/drivers/dell_emc/powermax/provision.py +++ b/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, - restored=False, generation=generation) + if source_devices: + 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, - src_dev_ids, extra_specs): + def delete_group_replica(self, array, snap_name, source_group_name): """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( - array, snap_name, src_dev_ids, extra_specs) + gen_list = self.rest.get_storagegroup_snap_generation_list( + 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, diff --git a/cinder/volume/drivers/dell_emc/powermax/rest.py b/cinder/volume/drivers/dell_emc/powermax/rest.py index 21d9b4c34d3..37c5d2e4e1e 100644 --- a/cinder/volume/drivers/dell_emc/powermax/rest.py +++ b/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. diff --git a/releasenotes/notes/powermax-bug-1853589-f6c7164177da0496.yaml b/releasenotes/notes/powermax-bug-1853589-f6c7164177da0496.yaml new file mode 100644 index 00000000000..8902d33c4b5 --- /dev/null +++ b/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.