commit a7f9eda389344b43276fe1e5a54d5b7d8ab6c5a8 Author: odonos12 Date: Fri Jul 17 14:43:13 2020 +0100 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 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 ac862bc..4c14b48 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 @@ -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 - snap_name = session['snap_name'] + 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() - mck_del.reset_mock() + def test_unlink_targets_and_delete_temp_snapvx_no_snap_name_found(self): + 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 - session['expired'] = True + @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_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', diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py index abd4528..3bcc1a8 100644 --- a/cinder/volume/drivers/dell_emc/powermax/common.py +++ b/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 - try: - 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 + LOG.debug("Locking on source device %(device_id)s.", + {'device_id': src_device_id}) 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 - # 3. If snapVX snapshot with copy mode disabled and not expired - if ('EMC_SMI' in snap_name or cm_enabled or ( - not cm_enabled and not expired)): + # 2. If snapVX snapshot is temporary + # If a target is present in the snap session it will be unlinked by + # the provision.unlink_snapvx_tgt_volume call above. This accounts + # 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.", { diff --git a/releasenotes/notes/bug-1887962-643379faf20f01cf.yaml b/releasenotes/notes/bug-1887962-643379faf20f01cf.yaml new file mode 100644 index 0000000..623041f --- /dev/null +++ b/releasenotes/notes/bug-1887962-643379faf20f01cf.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + `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.