From 1d10e63bdf38fd174a2e367abc12e98bb415fae5 Mon Sep 17 00:00:00 2001 From: GirishChilukuri Date: Tue, 6 Oct 2020 19:09:34 +0000 Subject: [PATCH] [SVF]:Fix clone fcmap not being deleted in cleanup [Spectrum Virtualize Family] check_vdisk_fc_mappings is not deleting clone fc maps, if the flashcopy status is in copying autodelete is set to on and copy progress is 100%. when multiple clones and snapshots are created using common source volume, deleting the source volume is not removing all the fcmaps involved in mapping with source volume. This patch fixes the issue by removing the clone fcmap and then removing the snapshot fcmap when the source has more than one fcmap. Closes-Bug: #1890254 Change-Id: I84629cde6d27d6889a30d7c13d99f57c660adde7 (cherry picked from commit 3570a79a16127fd42216aa40b4f492e40533f0ed) (cherry picked from commit 6e592a8dd49a85f8658ffeeb8c95a9d39f1d8507) (cherry picked from commit c469c1294d5a09f926905414f505d6e84ebaf506) --- .../volume/drivers/ibm/test_storwize_svc.py | 164 +++++++++++++++++- .../ibm/storwize_svc/storwize_svc_common.py | 107 +++++++++--- ...-deleting-in-cleanup-f5bbb467be1b889d.yaml | 8 + 3 files changed, 257 insertions(+), 22 deletions(-) create mode 100644 releasenotes/notes/bug-1890254-clone-fcmap-is-not-deleting-in-cleanup-f5bbb467be1b889d.yaml diff --git a/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py b/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py index 249641472a6..c5c90845f0b 100644 --- a/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py +++ b/cinder/tests/unit/volume/drivers/ibm/test_storwize_svc.py @@ -28,6 +28,7 @@ import time from oslo_concurrency import processutils from oslo_config import cfg +from oslo_service import loopingcall from oslo_utils import importutils from oslo_utils import units import six @@ -8309,7 +8310,168 @@ class StorwizeHelpersTestCase(test.TestCase): 'status': 'copying', 'target_vdisk_name': 'testvol'} self.storwize_svc_common.pretreatment_before_revert(vol) - stopfcmap.assert_called_once_with('4', split=True) + stopfcmap.assert_called_once_with('4') + + @ddt.data({'copy_rate': '50', 'progress': '3', 'status': 'copying'}, + {'copy_rate': '50', 'progress': '100', 'status': 'copying'}, + {'copy_rate': '0', 'progress': '0', 'status': 'copying'}, + {'copy_rate': '50', 'progress': '0', 'status': 'copying'}, + {'copy_rate': '0', 'progress': '0', 'status': 'idle_or_copied'}) + @mock.patch.object(storwize_svc_common.StorwizeSSH, 'chfcmap') + @mock.patch.object(storwize_svc_common.StorwizeSSH, 'stopfcmap') + @mock.patch.object(storwize_svc_common.StorwizeSSH, 'rmfcmap') + @mock.patch.object(storwize_svc_common.StorwizeHelpers, + '_get_flashcopy_mapping_attributes') + @mock.patch.object(storwize_svc_common.StorwizeHelpers, + '_get_vdisk_fc_mappings') + def test_check_vdisk_fc_mappings(self, + fc_data, + get_vdisk_fc_mappings, + get_fc_mapping_attributes, + rmfcmap, stopfcmap, chfcmap): + vol = 'testvol' + get_vdisk_fc_mappings.return_value = ['4'] + get_fc_mapping_attributes.return_value = { + 'copy_rate': fc_data['copy_rate'], + 'progress': fc_data['progress'], + 'status': fc_data['status'], + 'target_vdisk_name': 'tar-testvol', + 'rc_controlled': 'no', + 'source_vdisk_name': 'testvol'} + + if(fc_data['copy_rate'] != '0' and fc_data['progress'] == '100' + and fc_data['status'] == 'copying'): + (self.assertRaises(loopingcall.LoopingCallDone, + self.storwize_svc_common._check_vdisk_fc_mappings, vol, True, + False)) + stopfcmap.assert_called_with('4') + self.assertEqual(1, stopfcmap.call_count) + else: + self.storwize_svc_common._check_vdisk_fc_mappings(vol, True, + False) + stopfcmap.assert_not_called() + self.assertEqual(0, stopfcmap.call_count) + + get_vdisk_fc_mappings.assert_called() + get_fc_mapping_attributes.assert_called_with('4') + rmfcmap.assert_not_called() + self.assertEqual(1, get_fc_mapping_attributes.call_count) + self.assertEqual(0, rmfcmap.call_count) + + if(fc_data['copy_rate'] == '0' and fc_data['progress'] == '0' + and fc_data['status'] in ['copying', 'idle_or_copied']): + chfcmap.assert_called_with('4', copyrate='50', autodel='on') + self.assertEqual(1, chfcmap.call_count) + else: + chfcmap.assert_not_called() + self.assertEqual(0, chfcmap.call_count) + + @mock.patch.object(storwize_svc_common.StorwizeSSH, 'chfcmap') + @mock.patch.object(storwize_svc_common.StorwizeSSH, 'stopfcmap') + @mock.patch.object(storwize_svc_common.StorwizeSSH, 'rmfcmap') + @mock.patch.object(storwize_svc_common.StorwizeHelpers, + '_get_flashcopy_mapping_attributes') + @mock.patch.object(storwize_svc_common.StorwizeHelpers, + '_get_vdisk_fc_mappings') + def test_check_vdisk_fc_mappings_tarisvol(self, + get_vdisk_fc_mappings, + get_fc_mapping_attributes, + rmfcmap, stopfcmap, chfcmap): + vol = 'tar-testvol' + get_vdisk_fc_mappings.return_value = ['4'] + get_fc_mapping_attributes.return_value = { + 'copy_rate': '0', + 'progress': '0', + 'status': 'idle_or_copied', + 'target_vdisk_name': 'tar-testvol', + 'rc_controlled': 'no', + 'source_vdisk_name': 'testvol'} + + self.assertRaises(loopingcall.LoopingCallDone, + self.storwize_svc_common._check_vdisk_fc_mappings, + vol, True, False) + + get_vdisk_fc_mappings.assert_called() + get_fc_mapping_attributes.assert_called_with('4') + stopfcmap.assert_not_called() + rmfcmap.assert_called_with('4') + chfcmap.assert_not_called() + self.assertEqual(1, get_fc_mapping_attributes.call_count) + self.assertEqual(0, stopfcmap.call_count) + self.assertEqual(1, rmfcmap.call_count) + self.assertEqual(0, chfcmap.call_count) + + @ddt.data(([{'cp_rate': '0', 'prgs': '0', 'status': 'idle_or_copied', + 'trg_vdisk': 'testvol', 'src_vdisk': 'tar_testvol'}, + {'cp_rate': '50', 'prgs': '100', 'status': 'copying', + 'trg_vdisk': 'tar_testvol', 'src_vdisk': 'testvol'}, + {'cp_rate': '50', 'prgs': '3', 'status': 'copying', + 'trg_vdisk': 'tar_testvol', 'src_vdisk': 'testvol'}], 1), + ([{'cp_rate': '50', 'prgs': '100', 'status': 'idle_or_copied', + 'trg_vdisk': 'testvol', 'src_vdisk': 'tar_testvol'}, + {'cp_rate': '50', 'prgs': '100', 'status': 'copying', + 'trg_vdisk': 'tar_testvol', 'src_vdisk': 'testvol'}, + {'cp_rate': '50', 'prgs': '100', 'status': 'copying', + 'trg_vdisk': 'testvol', 'src_vdisk': 'tar_testvol'}], 1), + ([{'cp_rate': '50', 'prgs': '100', 'status': 'idle_or_copied', + 'trg_vdisk': 'testvol', 'src_vdisk': 'tar_testvol'}, + {'cp_rate': '50', 'prgs': '100', 'status': 'copying', + 'trg_vdisk': 'tar_testvol', 'src_vdisk': 'testvol'}, + {'cp_rate': '50', 'prgs': '100', 'status': 'copying', + 'trg_vdisk': 'tar_testvol_1', 'src_vdisk': 'testvol'}], 2), + ([{'cp_rate': '0', 'prgs': '0', 'status': 'copying', + 'trg_vdisk': 'testvol', 'src_vdisk': 'snap_testvol'}, + {'cp_rate': '50', 'prgs': '0', 'status': 'copying', + 'trg_vdisk': 'tar_testvol', 'src_vdisk': 'testvol'}, + {'cp_rate': '50', 'prgs': '0', 'status': 'copying', + 'trg_vdisk': 'tar_testvol_1', 'src_vdisk': 'testvol'}], 0)) + @mock.patch.object(storwize_svc_common.StorwizeSSH, 'chfcmap') + @mock.patch.object(storwize_svc_common.StorwizeSSH, 'stopfcmap') + @mock.patch.object(storwize_svc_common.StorwizeSSH, 'rmfcmap') + @mock.patch.object(storwize_svc_common.StorwizeHelpers, + '_get_flashcopy_mapping_attributes') + @mock.patch.object(storwize_svc_common.StorwizeHelpers, + '_get_vdisk_fc_mappings') + @ddt.unpack + def test_check_vdisk_fc_mappings_mul_fcs(self, + fc_data, stopfc_count, + get_vdisk_fc_mappings, + get_fc_mapping_attributes, + rmfcmap, stopfcmap, chfcmap): + vol = 'testvol' + get_vdisk_fc_mappings.return_value = ['4', '5', '7'] + get_fc_mapping_attributes.side_effect = [ + { + 'copy_rate': fc_data[0]['cp_rate'], + 'progress': fc_data[0]['prgs'], + 'status': fc_data[0]['status'], + 'target_vdisk_name': fc_data[0]['trg_vdisk'], + 'rc_controlled': 'no', + 'source_vdisk_name': fc_data[0]['src_vdisk']}, + { + 'copy_rate': fc_data[1]['cp_rate'], + 'progress': fc_data[1]['prgs'], + 'status': fc_data[1]['status'], + 'target_vdisk_name': fc_data[1]['trg_vdisk'], + 'rc_controlled': 'no', + 'source_vdisk_name': fc_data[1]['src_vdisk']}, + { + 'copy_rate': fc_data[2]['cp_rate'], + 'progress': fc_data[2]['prgs'], + 'status': fc_data[2]['status'], + 'target_vdisk_name': fc_data[2]['trg_vdisk'], + 'rc_controlled': 'no', + 'source_vdisk_name': fc_data[2]['src_vdisk']}] + + self.storwize_svc_common._check_vdisk_fc_mappings(vol, True, True) + get_vdisk_fc_mappings.assert_called() + get_fc_mapping_attributes.assert_called() + rmfcmap.assert_not_called() + chfcmap.assert_not_called() + self.assertEqual(3, get_fc_mapping_attributes.call_count) + self.assertEqual(stopfc_count, stopfcmap.call_count) + self.assertEqual(0, rmfcmap.call_count) + self.assertEqual(0, chfcmap.call_count) def test_storwize_check_flashcopy_rate_invalid1(self): with mock.patch.object(storwize_svc_common.StorwizeHelpers, diff --git a/cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py b/cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py index 11e6071674b..66f02753622 100644 --- a/cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py +++ b/cinder/volume/drivers/ibm/storwize_svc/storwize_svc_common.py @@ -2042,10 +2042,10 @@ class StorwizeHelpers(object): return None return resp[0] - def _check_vdisk_fc_mappings(self, name, - allow_snaps=True, allow_fctgt=False): + @cinder_utils.trace + def _check_delete_vdisk_fc_mappings(self, name, allow_snaps=True, + allow_fctgt=False): """FlashCopy mapping check helper.""" - LOG.debug('Loopcall: _check_vdisk_fc_mappings(), vdisk %s.', name) mapping_ids = self._get_vdisk_fc_mappings(name) wait_for_copy = False for map_id in mapping_ids: @@ -2058,10 +2058,23 @@ class StorwizeHelpers(object): target = attrs['target_vdisk_name'] copy_rate = attrs['copy_rate'] status = attrs['status'] + progress = attrs['progress'] + LOG.debug('Loopcall: source: %s, target: %s, copy_rate: %s, ' + 'status: %s, progress: %s, mapid: %s', source, target, + copy_rate, status, progress, map_id) if allow_fctgt and target == name and status == 'copying': - self.ssh.stopfcmap(map_id) - attrs = self._get_flashcopy_mapping_attributes(map_id) + try: + self.ssh.stopfcmap(map_id) + except exception.VolumeBackendAPIException as ex: + LOG.warning(ex) + wait_for_copy = True + try: + attrs = self._get_flashcopy_mapping_attributes(map_id) + except exception.VolumeBackendAPIException as ex: + LOG.warning(ex) + wait_for_copy = True + continue if attrs: status = attrs['status'] else: @@ -2083,28 +2096,80 @@ class StorwizeHelpers(object): {'name': name, 'src': source, 'tgt': target}) LOG.error(msg) raise exception.VolumeDriverException(message=msg) - if status in ['copying', 'prepared']: - self.ssh.stopfcmap(map_id) - # Need to wait for the fcmap to change to - # stopped state before remove fcmap + try: + if status in ['copying', 'prepared']: + self.ssh.stopfcmap(map_id) + # Need to wait for the fcmap to change to + # stopped state before remove fcmap + wait_for_copy = True + elif status in ['stopping', 'preparing']: + wait_for_copy = True + else: + self.ssh.rmfcmap(map_id) + except exception.VolumeBackendAPIException as ex: + LOG.warning(ex) wait_for_copy = True - elif status in ['stopping', 'preparing']: - wait_for_copy = True - else: - self.ssh.rmfcmap(map_id) # Case 4: Copy in progress - wait and will autodelete else: - if status == 'prepared': - self.ssh.stopfcmap(map_id) - self.ssh.rmfcmap(map_id) - elif status in ['idle_or_copied', 'stopped']: - # Prepare failed or stopped - self.ssh.rmfcmap(map_id) - else: + try: + if status == 'prepared': + self.ssh.stopfcmap(map_id) + self.ssh.rmfcmap(map_id) + elif status in ['idle_or_copied', 'stopped']: + # Prepare failed or stopped + self.ssh.rmfcmap(map_id) + elif (status in ['copying', 'prepared'] and + progress == '100'): + self.ssh.stopfcmap(map_id) + else: + wait_for_copy = True + except exception.VolumeBackendAPIException as ex: + LOG.warning(ex) wait_for_copy = True + if not wait_for_copy or not len(mapping_ids): raise loopingcall.LoopingCallDone(retvalue=True) + @cinder_utils.trace + def _check_vdisk_fc_mappings(self, name, allow_snaps=True, + allow_fctgt=False): + """FlashCopy mapping check helper.""" + # if this is a remove disk we need to be down to one fc clone + mapping_ids = self._get_vdisk_fc_mappings(name) + if len(mapping_ids) > 1 and allow_fctgt: + LOG.debug('Loopcall: vdisk %s has ' + 'more than one fc map. Waiting.', name) + for map_id in mapping_ids: + attrs = self._get_flashcopy_mapping_attributes(map_id) + if not attrs: + continue + source = attrs['source_vdisk_name'] + target = attrs['target_vdisk_name'] + copy_rate = attrs['copy_rate'] + status = attrs['status'] + progress = attrs['progress'] + LOG.debug('Loopcall: source: %s, target: %s, copy_rate: %s, ' + 'status: %s, progress: %s, mapid: %s', + source, target, copy_rate, status, progress, map_id) + + if copy_rate != '0' and source == name: + try: + if status in ['copying'] and progress == '100': + self.ssh.stopfcmap(map_id) + elif status == 'idle_or_copied' and progress == '100': + # wait for auto-delete of fcmap. + continue + elif status in ['idle_or_copied', 'stopped']: + # Prepare failed or stopped + self.ssh.rmfcmap(map_id) + # handle VolumeBackendAPIException to let it go through + # next attempts in case of any cli exception. + except exception.VolumeBackendAPIException as ex: + LOG.warning(ex) + return + return self._check_delete_vdisk_fc_mappings( + name, allow_snaps=allow_snaps, allow_fctgt=allow_fctgt) + def ensure_vdisk_no_fc_mappings(self, name, allow_snaps=True, allow_fctgt=False): """Ensure vdisk has no flashcopy mappings.""" @@ -2543,7 +2608,7 @@ class StorwizeHelpers(object): elif copy_rate != '0' and progress == '100': LOG.debug('Split completed clone map_id=%(map_id)s fcmap', {'map_id': map_id}) - self.ssh.stopfcmap(map_id, split=True) + self.ssh.stopfcmap(map_id) class CLIResponse(object): diff --git a/releasenotes/notes/bug-1890254-clone-fcmap-is-not-deleting-in-cleanup-f5bbb467be1b889d.yaml b/releasenotes/notes/bug-1890254-clone-fcmap-is-not-deleting-in-cleanup-f5bbb467be1b889d.yaml new file mode 100644 index 00000000000..53e0be8a4a4 --- /dev/null +++ b/releasenotes/notes/bug-1890254-clone-fcmap-is-not-deleting-in-cleanup-f5bbb467be1b889d.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + IBM Spectrum Virtualize driver `Bug #1890254 + `_: + Fix check_vdisk_fc_mappings is not deleting all flashcopy + mappings while deleting source volume, when multiple clones + and snapshots are created using common source volume.