diff --git a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py index a310647cbae..1d5cff514c9 100644 --- a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py +++ b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py @@ -199,15 +199,18 @@ class HPE3PARBaseDriver(test.TestCase): 'volume_type': None, 'volume_type_id': None} - volume_replicated = {'name': VOLUME_NAME, - 'id': VOLUME_ID, - 'display_name': 'Foo Volume', - 'replication_status': 'disabled', - 'provider_location': CLIENT_ID, - 'size': 2, - 'host': FAKE_CINDER_HOST, - 'volume_type': 'replicated', - 'volume_type_id': VOLUME_TYPE_ID_REPLICATED} + volume_replicated = fake_volume.fake_volume_obj( + context.get_admin_context(), + name=VOLUME_NAME, + id=VOLUME_ID, + display_name='Foo Volume', + replication_status='disabled', + provider_location = CLIENT_ID, + size=2, + host=FAKE_CINDER_HOST, + volume_type = 'replicated', + volume_type_id = VOLUME_TYPE_ID_REPLICATED, + migration_status = None) volume_tiramisu = {'name': VOLUME_NAME, 'id': VOLUME_ID, @@ -1257,7 +1260,8 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): '_create_client') as mock_create_client: mock_create_client.return_value = mock_client - volume = self.volume_replicated.copy() + volume = copy.deepcopy(self.volume_replicated) + volume['status'] = 'available' volume['replication_status'] = 'failed-over' self.driver.delete_volume(volume) @@ -2092,8 +2096,15 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): mock.call.startRemoteCopy(self.RCG_3PAR_NAME)] mock_client.assert_has_calls(expected + self.standard_logout) + # volume's status and migration_status + # (i) default scenario + # (ii) deleting temporary volume + @ddt.data({'status': 'available', 'migration_status': 'migrating'}, + {'status': 'deleting', 'migration_status': 'deleting'}) + @ddt.unpack @mock.patch.object(volume_types, 'get_volume_type') - def test_retype_rep_type_to_non_rep_type(self, _mock_volume_types): + def test_retype_rep_type_to_non_rep_type(self, _mock_volume_types, + status, migration_status): conf = self.setup_configuration() self.replication_targets[0]['replication_mode'] = 'periodic' @@ -2120,7 +2131,9 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): 'size': 2, 'host': self.FAKE_CINDER_HOST, 'volume_type': 'replicated', - 'volume_type_id': 'gold'} + 'volume_type_id': 'gold', + 'status': status, + 'migration_status': migration_status} volume_type = {'name': 'replicated', 'deleted': False, @@ -2215,7 +2228,9 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): 'size': 2, 'host': self.FAKE_CINDER_HOST, 'volume_type': 'replicated', - 'volume_type_id': 'gold'} + 'volume_type_id': 'gold', + 'status': 'available', + 'migration_status': 'migrating'} volume_type = {'name': 'replicated', 'deleted': False, @@ -2427,6 +2442,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): mock_client.assert_has_calls(expected) + # Default scenario: vvset name is similar to volume name @mock.patch.object(volume_types, 'get_volume_type') def test_delete_volume_replicated(self, _mock_volume_types): # setup_mock_client drive with default configuration @@ -2436,6 +2452,55 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): ex = hpeexceptions.HTTPConflict("In use") ex._error_code = 34 mock_client.deleteVolume = mock.Mock(side_effect=[ex, 200]) + mock_client.getVolumeSet.return_value = 'vvs-0DM4qZEVSKON-DXN-NwVpw' + _mock_volume_types.return_value = { + 'name': 'replicated', + 'extra_specs': { + 'cpg': HPE3PAR_CPG_QOS, + 'snap_cpg': HPE3PAR_CPG_SNAP, + 'vvs_name': self.VVS_NAME, + 'qos': self.QOS, + 'replication_enabled': ' True', + 'replication:mode': 'periodic', + 'replication:sync_period': '900', + 'volume_type': self.volume_type_replicated}} + VVS_NAME = self.VOLUME_3PAR_NAME.replace('osv-', 'vvs-') + with mock.patch.object(hpecommon.HPE3PARCommon, + '_create_client') as mock_create_client: + mock_create_client.return_value = mock_client + volume = copy.deepcopy(self.volume_replicated) + volume['status'] = 'available' + self.driver.delete_volume(volume) + + expected = [ + mock.call.stopRemoteCopy(self.RCG_3PAR_NAME), + mock.call.removeVolumeFromRemoteCopyGroup( + self.RCG_3PAR_NAME, + self.VOLUME_3PAR_NAME, + removeFromTarget=True), + mock.call.removeRemoteCopyGroup(self.RCG_3PAR_NAME), + mock.call.deleteVolume(self.VOLUME_3PAR_NAME), + mock.call.getVolumeSet(VVS_NAME), + mock.call.deleteVolumeSet(VVS_NAME), + mock.call.deleteVolume(self.VOLUME_3PAR_NAME)] + + mock_client.assert_has_calls(expected) + + # Second scenario: vvset name is altogether different from volume name + @mock.patch.object(volume_types, 'get_volume_type') + def test_delete_volume_repl_different_vvset(self, _mock_volume_types): + # setup_mock_client drive with default configuration + # and return the mock HTTP 3PAR client + mock_client = self.setup_driver() + mock_client.getStorageSystemInfo.return_value = {'id': self.CLIENT_ID} + ex = hpeexceptions.HTTPConflict("In use") + ex._error_code = 34 + mock_client.deleteVolume = mock.Mock(side_effect=[ex, 200]) + + ex_not_found = hpeexceptions.HTTPNotFound("Set does not exist") + ex_not_found._error_code = 102 + mock_client.getVolumeSet = mock.Mock(side_effect=[ex_not_found, 404]) + mock_client.findVolumeSet.return_value = self.VVS_NAME _mock_volume_types.return_value = { 'name': 'replicated', @@ -2448,10 +2513,13 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): 'replication:mode': 'periodic', 'replication:sync_period': '900', 'volume_type': self.volume_type_replicated}} + vvs_name_similar = self.VOLUME_3PAR_NAME.replace('osv-', 'vvs-') with mock.patch.object(hpecommon.HPE3PARCommon, '_create_client') as mock_create_client: mock_create_client.return_value = mock_client - self.driver.delete_volume(self.volume_replicated) + volume = copy.deepcopy(self.volume_replicated) + volume['status'] = 'available' + self.driver.delete_volume(volume) expected = [ mock.call.stopRemoteCopy(self.RCG_3PAR_NAME), @@ -2461,6 +2529,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): removeFromTarget=True), mock.call.removeRemoteCopyGroup(self.RCG_3PAR_NAME), mock.call.deleteVolume(self.VOLUME_3PAR_NAME), + mock.call.getVolumeSet(vvs_name_similar), mock.call.findVolumeSet(self.VOLUME_3PAR_NAME), mock.call.removeVolumeFromVolumeSet(self.VVS_NAME, self.VOLUME_3PAR_NAME), @@ -2468,6 +2537,45 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): mock_client.assert_has_calls(expected) + @mock.patch.object(volume_types, 'get_volume_type') + def test_delete_volume_replicated_migrated(self, _mock_volume_types): + # setup_mock_client drive with default configuration + # and return the mock HTTP 3PAR client + mock_client = self.setup_driver() + mock_client.getStorageSystemInfo.return_value = {'id': self.CLIENT_ID} + mock_client.getVolume.return_value = {'rcopyGroup': + 'rcg-CArwlBBhRqq3K-eLUh'} + _mock_volume_types.return_value = { + 'name': 'replicated', + 'extra_specs': { + 'cpg': HPE3PAR_CPG, + 'snap_cpg': HPE3PAR_CPG_SNAP, + 'replication_enabled': ' True', + 'replication:mode': 'periodic', + 'replication:sync_period': '900', + 'volume_type': self.volume_type_replicated}} + with mock.patch.object(hpecommon.HPE3PARCommon, + '_create_client') as mock_create_client: + mock_create_client.return_value = mock_client + volume = copy.deepcopy(self.volume_replicated) + volume['status'] = 'available' + volume['migration_status'] = 'success' + self.driver.delete_volume(volume) + + rcg_name_updated = 'rcg-CArwlBBhRqq3K-eLUh' + + expected = [ + mock.call.getVolume(self.VOLUME_3PAR_NAME), + mock.call.stopRemoteCopy(rcg_name_updated), + mock.call.removeVolumeFromRemoteCopyGroup( + rcg_name_updated, + self.VOLUME_3PAR_NAME, + removeFromTarget=True), + mock.call.removeRemoteCopyGroup(rcg_name_updated), + mock.call.deleteVolume(self.VOLUME_3PAR_NAME)] + + mock_client.assert_has_calls(expected) + def test_get_cpg_with_volume_return_usercpg(self): # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client @@ -3378,6 +3486,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): @ddt.unpack def test_revert_to_snapshot(self, snapshot_attr, vol_name): snapshot = getattr(self, snapshot_attr) + snapshot['volume']['migration_status'] = None # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client mock_client = self.setup_driver() @@ -5895,6 +6004,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): group.replication_status = fields.ReplicationStatus.ENABLED group.volume_types = [self.volume_type_tiramisu] volume['group'] = group + volume['migration_status'] = None self.driver.revert_to_snapshot( self.ctxt, @@ -6746,7 +6856,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): mock_replication_client.return_value = mock_replicated_client # Test a successful fail-back. - volume = self.volume_replicated.copy() + volume = copy.deepcopy(self.volume_replicated) volume['replication_status'] = 'failed-over' return_model = self.driver.failover_host( context.get_admin_context(), @@ -6797,7 +6907,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): mock_replication_client.return_value = mock_replicated_client # Test an unsuccessful fail-back. - volume = self.volume_replicated.copy() + volume = copy.deepcopy(self.volume_replicated) volume['replication_status'] = 'failed-over' self.assertRaises( @@ -6905,6 +7015,51 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): if 'HP:3PAR' in new_key: self.assertIn(new_key, properties) + @mock.patch.object(volume_types, 'get_volume_type') + def test_rename_migrated_vvset(self, _mock_volume_types): + # Setup_mock_client drive with default configuration + # and return the mock HTTP 3PAR client + mock_client = self.setup_driver() + mock_client.getStorageSystemInfo.return_value = {'id': self.CLIENT_ID} + + with mock.patch.object(hpecommon.HPE3PARCommon, + '_create_client') as mock_create_client: + mock_create_client.return_value = mock_client + dest_volume = {'name': self.VOLUME_NAME, + 'id': self.VOLUME_ID, + 'display_name': 'Foo Volume', + 'size': 2, + 'host': self.FAKE_CINDER_HOST, + 'volume_type': None, + 'volume_type_id': None} + + src_volume = {'name': self.SRC_CG_VOLUME_NAME, + 'id': self.SRC_CG_VOLUME_ID, + 'display_name': 'Foo Volume', + 'size': 2, + 'host': self.FAKE_CINDER_HOST, + 'volume_type': None, + 'volume_type_id': None} + + vvs_name_dest = 'vvs-0DM4qZEVSKON-DXN-NwVpw' + vvs_name_src = 'vvs-vSHRG8dlTGiJbGsH9jz8tg' + vvs_name_temp = 'tos-vSHRG8dlTGiJbGsH9jz8tg' + + common = self.driver._login() + return_model = common._rename_migrated_vvset(src_volume, + dest_volume) + + expected = [ + mock.call.modifyVolumeSet( + vvs_name_dest, newName=vvs_name_temp), + mock.call.modifyVolumeSet( + vvs_name_src, newName=vvs_name_dest), + mock.call.modifyVolumeSet( + vvs_name_temp, newName=vvs_name_src)] + + mock_client.assert_has_calls(expected) + self.assertIsNone(return_model) + @ddt.ddt class TestHPE3PARFCDriver(HPE3PARBaseDriver): diff --git a/cinder/volume/drivers/hpe/hpe_3par_common.py b/cinder/volume/drivers/hpe/hpe_3par_common.py index f81aa8c6ae0..2718a3cf01a 100644 --- a/cinder/volume/drivers/hpe/hpe_3par_common.py +++ b/cinder/volume/drivers/hpe/hpe_3par_common.py @@ -305,11 +305,12 @@ class HPE3PARCommon(object): error out if it has child snapshot(s). Bug #1994521 4.0.19 - Update code to work with new WSAPI (of 2023). Bug #2015746 4.0.20 - Use small QoS Latency value. Bug #2018994 + 4.0.21 - Fix issue seen during retype/migrate. Bug #2026718 """ - VERSION = "4.0.20" + VERSION = "4.0.21" stats = {} @@ -1463,9 +1464,22 @@ class HPE3PARCommon(object): # v2 replication conversion def _get_3par_rcg_name(self, volume): - rcg_name = self._encode_name(volume.get('_name_id') or volume['id']) - rcg = "rcg-%s" % rcg_name - return rcg[:22] + # if non-replicated volume is retyped or migrated to replicated vol, + # then rcg_name is different. Try to get that new rcg_name. + if volume['migration_status'] == 'success': + vol_name = self._get_3par_vol_name(volume) + vol_details = self.client.getVolume(vol_name) + rcg_name = vol_details.get('rcopyGroup') + + LOG.debug("new rcg_name: %(name)s", + {'name': rcg_name}) + return rcg_name + else: + # by default, rcg_name is similar to volume name + rcg_name = self._encode_name(volume.get('_name_id') + or volume['id']) + rcg = "rcg-%s" % rcg_name + return rcg[:22] def _get_3par_remote_rcg_name(self, volume, provider_location): return self._get_3par_rcg_name(volume) + ".r" + ( @@ -2691,6 +2705,10 @@ class HPE3PARCommon(object): raise exception.CinderException(ex) def delete_volume(self, volume): + vol_id = volume.id + name_id = volume.get('_name_id') + LOG.debug("DELETE volume vol_id: %(vol_id)s, name_id: %(name_id)s", + {'vol_id': vol_id, 'name_id': name_id}) @utils.retry(exception.VolumeIsBusy, interval=2, retries=10) def _try_remove_volume(volume_name): @@ -2706,15 +2724,30 @@ class HPE3PARCommon(object): # If the volume type is replication enabled, we want to call our own # method of deconstructing the volume and its dependencies if self._volume_of_replicated_type(volume, hpe_tiramisu_check=True): + LOG.debug("volume is of replicated_type") replication_status = volume.get('replication_status', None) - if replication_status and replication_status == "failed-over": - self._delete_replicated_failed_over_volume(volume) - else: - self._do_volume_replication_destroy(volume) - return + LOG.debug("replication_status: %(status)s", + {'status': replication_status}) + if replication_status: + if replication_status == "failed-over": + self._delete_replicated_failed_over_volume(volume) + else: + self._do_volume_replication_destroy(volume) + return + volume_name = self._get_3par_vol_name(volume) + + # during retype/migrate + if (self._volume_of_replicated_type(volume, hpe_tiramisu_check=True) + and volume['migration_status'] == 'deleting'): + # don't use current osv_name (which was from name_id) + # get new osv_name from id + LOG.debug("get osv_name from volume id") + volume_name = self._encode_name(volume.id) + volume_name = "osv-" + volume_name + + LOG.debug("volume_name: %(name)s", {'name': volume_name}) try: - volume_name = self._get_3par_vol_name(volume) # Try and delete the volume, it might fail here because # the volume is part of a volume set which will have the # volume set name in the error. @@ -3073,6 +3106,39 @@ class HPE3PARCommon(object): log_error('original', e, temp_name, current_name, temp_name) return True + def _rename_migrated_vvset(self, src_volume, dest_volume): + """Rename the vvsets after a migration. + + """ + vvs_name_src = self._get_3par_vvs_name(src_volume['id']) + vvs_name_dest = self._get_3par_vvs_name(dest_volume['id']) + + # There can be parallel execution. Ensure that temp_vvs_name is unique + # eg. if vvs_name_src is: vvs-DK3sEwkPTCqVHdHKHtwZBA + # then temp_vvs_name is : tos-DK3sEwkPTCqVHdHKHtwZBA + temp_vvs_name = 'tos-' + vvs_name_src[4:] + + try: + self.client.modifyVolumeSet(vvs_name_dest, newName=temp_vvs_name) + LOG.debug("Renamed vvset %(old)s to %(new)s", + {'old': vvs_name_dest, 'new': temp_vvs_name}) + except Exception as ex: + LOG.error("exception: %(details)s", {'details': str(ex)}) + + try: + self.client.modifyVolumeSet(vvs_name_src, newName=vvs_name_dest) + LOG.debug("Renamed vvset %(old)s to %(new)s", + {'old': vvs_name_src, 'new': vvs_name_dest}) + except Exception as ex: + LOG.error("exception: %(details)s", {'details': str(ex)}) + + try: + self.client.modifyVolumeSet(temp_vvs_name, newName=vvs_name_src) + LOG.debug("Renamed vvset %(old)s to %(new)s", + {'old': temp_vvs_name, 'new': vvs_name_src}) + except Exception as ex: + LOG.error("exception: %(details)s", {'details': str(ex)}) + def update_migrated_volume(self, context, volume, new_volume, original_volume_status): """Rename the new (temp) volume to it's original name. @@ -3105,6 +3171,13 @@ class HPE3PARCommon(object): self._update_comment(current_name, volume_id=volume['id'], _name_id=name_id) + if new_volume_renamed: + type_info = self.get_volume_settings_from_type(volume) + qos = type_info['qos'] + if qos: + # rename the vvsets as per volume names + self._rename_migrated_vvset(volume, new_volume) + return {'_name_id': name_id, 'provider_location': provider_location} @staticmethod @@ -4491,7 +4564,7 @@ class HPE3PARCommon(object): return True except Exception as ex: - self._do_volume_replication_destroy(volume) + self._do_volume_replication_destroy(volume, retype=retype) msg = (_("There was an error setting up a remote copy group " "on the 3PAR arrays: ('%s'). The volume will not be " "recognized as replication type.") % @@ -4576,20 +4649,29 @@ class HPE3PARCommon(object): def _delete_vvset(self, volume): # volume is part of a volume set. + LOG.debug("_delete_vvset. vol_id: %(id)s", {'id': volume['id']}) volume_name = self._get_3par_vol_name(volume) - vvset_name = self.client.findVolumeSet(volume_name) - LOG.debug("Returned vvset_name = %s", vvset_name) - if vvset_name is not None: - if vvset_name.startswith('vvs-'): - # We have a single volume per volume set, so - # remove the volume set. - self.client.deleteVolumeSet( - self._get_3par_vvs_name(volume['id'])) - else: - # We have a pre-defined volume set just remove the - # volume and leave the volume set. - self.client.removeVolumeFromVolumeSet(vvset_name, - volume_name) + vvset_name = self._get_3par_vvs_name(volume['id']) + + try: + # find vvset + self.client.getVolumeSet(vvset_name) + + # (a) vvset is found: + # We have a single volume per volume set, so + # remove the volume set. + LOG.debug("Deleting vvset: %(name)s", {'name': vvset_name}) + self.client.deleteVolumeSet(vvset_name) + + except hpeexceptions.HTTPNotFound: + # (b) vvset not found: + # - find the vvset name from volume name + # - remove the volume and leave the vvset + vvset_name = self.client.findVolumeSet(volume_name) + + LOG.debug("Removing vol %(volume_name)s from vvset %(vvset_name)s", + {'volume_name': volume_name, 'vvset_name': vvset_name}) + self.client.removeVolumeFromVolumeSet(vvset_name, volume_name) def _get_3par_rcg_name_of_group(self, group_id): rcg_name = self._encode_name(group_id) diff --git a/releasenotes/notes/hpe-3par-retype-migrate-2383ddaf92c87f9e.yaml b/releasenotes/notes/hpe-3par-retype-migrate-2383ddaf92c87f9e.yaml new file mode 100644 index 00000000000..168f937db11 --- /dev/null +++ b/releasenotes/notes/hpe-3par-retype-migrate-2383ddaf92c87f9e.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + HPE 3PAR driver `Bug #2026718 `_: + Fixed: With this patch, added logic to fetch correct volume name on 3par + (osv_name), rcg_name and vvset_name (for particular scenarios); + so that volumes can be identified and deleted from 3par.