From 6a5e4ae16005c2a0bf444a8e499921fd6b8d2789 Mon Sep 17 00:00:00 2001
From: raghavendrat <raghavendra-uddhav.tilay@hpe.com>
Date: Thu, 8 Jun 2023 14:31:50 +0000
Subject: [PATCH] HPE: Fix error during retype of volume without comment

On HPE 3par array, for volumes .. the comment section can be empty.
During retype of such volumes, below error is observed:

2023-05-16 16:46:20.338 93 ERROR cinder.volume.manager
    old_comment = old_volume_info['comment']
2023-05-16 16:46:20.338 93 ERROR cinder.volume.manager
    KeyError: 'comment'

This patch handles such scenario gracefully.

Closes-Bug: #2023253
Change-Id: Ia927145df855a50fb1c1ef67a9ce74e80bb7cc3a
---
 .../unit/volume/drivers/hpe/test_hpe3par.py   | 39 +++++++++++++++++++
 cinder/volume/drivers/hpe/hpe_3par_common.py  |  7 +++-
 ...-vol-without-comment-a44c9be1ed76e7bb.yaml |  6 +++
 3 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 releasenotes/notes/hpe-3par-retype-vol-without-comment-a44c9be1ed76e7bb.yaml

diff --git a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py
index be0195c2ebd..25e2aa02478 100644
--- a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py
+++ b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py
@@ -1979,6 +1979,45 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
             ]
             mock_client.assert_has_calls(expected + self.standard_logout)
 
+    @mock.patch.object(volume_types, 'get_volume_type')
+    def test_retype_volume_without_comment(self, _mock_volume_types):
+        _mock_volume_types.return_value = self.RETYPE_VOLUME_TYPE_1
+        mock_client = self.setup_driver(mock_conf=self.RETYPE_CONF)
+
+        volume = {'id': HPE3PARBaseDriver.CLONE_ID}
+
+        VOLUME_INFO_0 = self.RETYPE_VOLUME_INFO_0.copy()
+
+        # volume without comment
+        del(VOLUME_INFO_0['comment'])
+        mock_client.getVolume.return_value = VOLUME_INFO_0
+
+        with mock.patch.object(hpecommon.HPE3PARCommon,
+                               '_create_client') as mock_create_client:
+            mock_create_client.return_value = mock_client
+
+            retyped = self.driver.retype(
+                self.ctxt, volume, self.RETYPE_VOLUME_TYPE_1, None,
+                self.RETYPE_HOST)
+            self.assertTrue(retyped[0])
+
+            expected = [
+                mock.call.modifyVolume('osv-0DM4qZEVSKON-AAAAAAAAA',
+                                       {'comment': mock.ANY,
+                                        'snapCPG': 'OpenStackCPGSnap'}),
+                mock.call.deleteVolumeSet('vvs-0DM4qZEVSKON-AAAAAAAAA'),
+                mock.call.addVolumeToVolumeSet('myvvs',
+                                               'osv-0DM4qZEVSKON-AAAAAAAAA'),
+                mock.call.modifyVolume('osv-0DM4qZEVSKON-AAAAAAAAA',
+                                       {'action': 6,
+                                        'userCPG': 'OpenStackCPG',
+                                        'conversionOperation': 1,
+                                        'compression': False,
+                                        'tuneOperation': 1}),
+                mock.call.getTask(1),
+            ]
+            mock_client.assert_has_calls(expected + self.standard_logout)
+
     @mock.patch.object(volume_types, 'get_volume_type')
     def test_retype_non_rep_type_to_rep_type(self, _mock_volume_types):
 
diff --git a/cinder/volume/drivers/hpe/hpe_3par_common.py b/cinder/volume/drivers/hpe/hpe_3par_common.py
index c503aa1db0f..f81aa8c6ae0 100644
--- a/cinder/volume/drivers/hpe/hpe_3par_common.py
+++ b/cinder/volume/drivers/hpe/hpe_3par_common.py
@@ -3687,7 +3687,7 @@ class HPE3PARCommon(object):
         old_tpvv = old_volume_info['provisioningType'] == self.THIN
         old_tdvv = old_volume_info['provisioningType'] == self.DEDUP
         old_cpg = old_volume_info['userCPG']
-        old_comment = old_volume_info['comment']
+        old_comment = old_volume_info.get('comment')
         old_snap_cpg = None
         if 'snapCPG' in old_volume_info:
             old_snap_cpg = old_volume_info['snapCPG']
@@ -5314,7 +5314,10 @@ class ModifyVolumeTask(flow_utils.CinderTask):
                          new_type_name, new_type_id):
 
         # Modify the comment during ModifyVolume
-        comment_dict = dict(ast.literal_eval(old_comment))
+        if not old_comment:
+            comment_dict = {}
+        else:
+            comment_dict = dict(ast.literal_eval(old_comment))
         if 'vvs' in comment_dict:
             del comment_dict['vvs']
         if 'qos' in comment_dict:
diff --git a/releasenotes/notes/hpe-3par-retype-vol-without-comment-a44c9be1ed76e7bb.yaml b/releasenotes/notes/hpe-3par-retype-vol-without-comment-a44c9be1ed76e7bb.yaml
new file mode 100644
index 00000000000..e4587b651f4
--- /dev/null
+++ b/releasenotes/notes/hpe-3par-retype-vol-without-comment-a44c9be1ed76e7bb.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    HPE 3PAR driver `Bug #2023253 <https://bugs.launchpad.net/cinder/+bug/2023253>`_:
+    Fixed: Handle error during retype of volume without comment
+