HPE 3par: Fix issue seen during retype/migrate

1] If migration_status is deleting,
incorrect volume name on 3par (osv_name) is fetched.

2] For retyped/migrated volume, Remote Copy Group name
(rcg_name) is different.

3] For retyped/migrated volume having QOS setting,
vvset_name is different.

This patch adds necessary logic to get appropriate osv_name,
rcg_name and vvset_name (for above scenarios).

Closes-Bug: #2026718
Change-Id: Ifa78eb42b0fa7a4446e5e300fcb13567eac04403

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,
+    volume_replicated = fake_volume.fake_volume_obj(
-                         'id': VOLUME_ID,
+        context.get_admin_context(),
-                         'display_name': 'Foo Volume',
+        name=VOLUME_NAME,
-                         'replication_status': 'disabled',
+        id=VOLUME_ID,
-                         'provider_location': CLIENT_ID,
+        display_name='Foo Volume',
-                         'size': 2,
+        replication_status='disabled',
-                         'host': FAKE_CINDER_HOST,
+        provider_location = CLIENT_ID,
-                         'volume_type': 'replicated',
+        size=2,
-                         'volume_type_id': VOLUME_TYPE_ID_REPLICATED}
+        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': '<is> 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': '<is> 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):

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'])
+        # if non-replicated volume is retyped or migrated to replicated vol,
-        rcg = "rcg-%s" % rcg_name
+        # then rcg_name is different. Try to get that new rcg_name.
-        return rcg[:22]
+        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":
+            LOG.debug("replication_status: %(status)s",
-                self._delete_replicated_failed_over_volume(volume)
+                      {'status': replication_status})
-            else:
+            if replication_status:
-                self._do_volume_replication_destroy(volume)
+                if replication_status == "failed-over":
-            return
+                    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)
+        vvset_name = self._get_3par_vvs_name(volume['id'])
-        LOG.debug("Returned vvset_name = %s", vvset_name)
+
-        if vvset_name is not None:
+        try:
-            if vvset_name.startswith('vvs-'):
+            # find vvset
-                # We have a single volume per volume set, so
+            self.client.getVolumeSet(vvset_name)
-                # remove the volume set.
+
-                self.client.deleteVolumeSet(
+            # (a) vvset is found:
-                    self._get_3par_vvs_name(volume['id']))
+            # We have a single volume per volume set, so
-            else:
+            # remove the volume set.
-                # We have a pre-defined volume set just remove the
+            LOG.debug("Deleting vvset: %(name)s", {'name': vvset_name})
-                # volume and leave the volume set.
+            self.client.deleteVolumeSet(vvset_name)
-                self.client.removeVolumeFromVolumeSet(vvset_name,
+
-                                                      volume_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)

releasenotes/notes/hpe-3par-retype-migrate-2383ddaf92c87f9e.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    HPE 3PAR driver `Bug #2026718 <https://bugs.launchpad.net/cinder/+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.