Merge "3PAR: Error out if vol cannot be converted to base"

3 files changed, 59 insertions, 5 deletions

diff --git a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py
index 1d1e440cb..7916792fa 100644
--- a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py
+++ b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py
@@ -3398,6 +3398,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
         }
 
         mock_client = self.setup_driver(mock_conf=conf)
+        mock_client.getVolumeSnapshots.return_value = []
         with mock.patch.object(hpecommon.HPE3PARCommon,
                                '_create_client') as mock_create_client:
             mock_create_client.return_value = mock_client
@@ -3426,6 +3427,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
                     {
                         'comment': comment,
                         'readOnly': False}),
+                mock.call.getVolumeSnapshots(self.VOLUME_3PAR_NAME),
                 mock.call.copyVolume(
                     osv_matcher, omv_matcher, HPE3PAR_CPG, mock.ANY),
                 mock.call.getTask(mock.ANY),
@@ -3449,6 +3451,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
         }
 
         mock_client = self.setup_driver(mock_conf=conf)
+        mock_client.getVolumeSnapshots.return_value = []
         _mock_volume_types.return_value = {
             'name': 'gold',
             'extra_specs': {
@@ -3490,6 +3493,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
                         'comment': comment,
                         'readOnly': False}),
                 mock.call.getCPG(HPE3PAR_CPG),
+                mock.call.getVolumeSnapshots(self.VOLUME_3PAR_NAME),
                 mock.call.copyVolume(
                     osv_matcher, omv_matcher, HPE3PAR_CPG, mock.ANY),
                 mock.call.getTask(mock.ANY),
@@ -3606,6 +3610,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
             'getVolume.return_value': {}
         }
         mock_client = self.setup_driver(mock_conf=conf)
+        mock_client.getVolumeSnapshots.return_value = []
         volume_type_hos = copy.deepcopy(self.volume_type_hos)
         volume_type_hos['extra_specs']['convert_to_base'] = True
         _mock_volume_types.return_value = volume_type_hos
@@ -3635,6 +3640,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
                     {
                         'comment': comment,
                         'readOnly': False}),
+                mock.call.getVolumeSnapshots(self.VOLUME_3PAR_NAME),
                 mock.call.copyVolume(
                     osv_matcher, omv_matcher, HPE3PAR_CPG, mock.ANY),
                 mock.call.getTask(mock.ANY),
@@ -3656,6 +3662,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
             'getVolume.return_value': {}
         }
         mock_client = self.setup_driver(mock_conf=conf)
+        mock_client.getVolumeSnapshots.return_value = []
         _mock_volume_types.return_value = self.volume_type_hos
         with mock.patch.object(hpecommon.HPE3PARCommon,
                                '_create_client') as mock_create_client:
@@ -3684,6 +3691,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
                     {
                         'comment': comment,
                         'readOnly': False}),
+                mock.call.getVolumeSnapshots(self.VOLUME_3PAR_NAME),
                 mock.call.copyVolume(
                     osv_matcher, omv_matcher, HPE3PAR_CPG, mock.ANY),
                 mock.call.getTask(mock.ANY),
@@ -3706,6 +3714,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
             'getVolume.return_value': {}
         }
         mock_client = self.setup_driver(mock_conf=conf)
+        mock_client.getVolumeSnapshots.return_value = []
         volume_type_hos = copy.deepcopy(self.volume_type_hos)
         volume_type_hos['extra_specs']['convert_to_base'] = True
         _mock_volume_types.return_value = volume_type_hos
@@ -3736,6 +3745,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
                     {
                         'comment': comment,
                         'readOnly': False}),
+                mock.call.getVolumeSnapshots(self.VOLUME_3PAR_NAME),
                 mock.call.copyVolume(
                     osv_matcher, omv_matcher, HPE3PAR_CPG, mock.ANY),
                 mock.call.getTask(mock.ANY),
@@ -3834,6 +3844,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
         }
 
         mock_client = self.setup_driver(mock_conf=conf)
+        mock_client.getVolumeSnapshots.return_value = []
         with mock.patch.object(hpecommon.HPE3PARCommon,
                                '_create_client') as mock_create_client:
             mock_create_client.return_value = mock_client
@@ -3857,6 +3868,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
         }
 
         mock_client = self.setup_driver(mock_conf=conf)
+        mock_client.getVolumeSnapshots.return_value = []
         with mock.patch.object(hpecommon.HPE3PARCommon,
                                '_create_client') as mock_create_client:
             mock_create_client.return_value = mock_client
@@ -3868,6 +3880,18 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver):
                               self.volume,
                               str(new_size))
 
+    def test__convert_to_base_volume_failure(self):
+        mock_client = self.setup_driver()
+        mock_client.getVolumeSnapshots.return_value = (
+            ['oss-nwJVbXaEQMi0w.xPutFRQw'])
+        with mock.patch.object(hpecommon.HPE3PARCommon,
+                               '_create_client') as mock_create_client:
+            mock_create_client.return_value = mock_client
+            common = self.driver._login()
+            self.assertRaises(exception.VolumeIsBusy,
+                              common._convert_to_base_volume,
+                              self.volume)
+
     @mock.patch.object(volume_types, 'get_volume_type')
     def test_extend_volume_replicated(self, _mock_volume_types):
         # Managed vs. unmanaged and periodic vs. sync are not relevant when
diff --git a/cinder/volume/drivers/hpe/hpe_3par_common.py b/cinder/volume/drivers/hpe/hpe_3par_common.py
index 971fac3e8..94c346b37 100644
--- a/cinder/volume/drivers/hpe/hpe_3par_common.py
+++ b/cinder/volume/drivers/hpe/hpe_3par_common.py
@@ -300,11 +300,13 @@ class HPE3PARCommon(object):
         4.0.16 - In multi host env, fix multi-detach operation. Bug #1958122
         4.0.17 - Added get_manageable_volumes and get_manageable_snapshots.
                  Bug #1819903
+        4.0.18 - During conversion of volume to base volume,
+                 error out if it has child snapshot(s). Bug #1994521
 
 
     """
 
-    VERSION = "4.0.17"
+    VERSION = "4.0.18"
 
     stats = {}
 
@@ -3139,6 +3141,21 @@ class HPE3PARCommon(object):
 
             compression = self.get_compression_policy(
                 type_info['hpe3par_keys'])
+
+            # If volume (osv-) has snapshot, while converting the volume
+            # to base volume (omv-), snapshot cannot be transferred to
+            # new base volume (omv-) i.e it remain with volume (osv-).
+            # So error out for such volume.
+            snap_list = self.client.getVolumeSnapshots(volume_name)
+            if snap_list:
+                snap_str = ",".join(snap_list)
+                msg = (_("Volume %(name)s has dependent snapshots: %(snap)s."
+                         " Either flatten or remove the dependent snapshots:"
+                         " %(snap)s for the conversion of volume %(name)s to"
+                         " succeed." % {'name': volume_name,
+                                        'snap': snap_str}))
+                raise exception.VolumeIsBusy(message=msg)
+
             # Create a physical copy of the volume
             task_id = self._copy_volume(volume_name, temp_vol_name,
                                         cpg, cpg, type_info['tpvv'],
@@ -3162,16 +3179,18 @@ class HPE3PARCommon(object):
             comment = self._get_3par_vol_comment(volume_name)
             if comment:
                 self.client.modifyVolume(temp_vol_name, {'comment': comment})
-            LOG.debug('Volume rename completed: convert_to_base_volume: '
-                      'id=%s.', volume['id'])
+                LOG.debug('Assigned the comment: convert_to_base_volume: '
+                          'id=%s.', volume['id'])
 
-            # Delete source volume after the copy is complete
+            # Delete source volume (osv-) after the copy is complete
             self.client.deleteVolume(volume_name)
             LOG.debug('Delete src volume completed: convert_to_base_volume: '
                       'id=%s.', volume['id'])
 
-            # Rename the new volume to the original name
+            # Rename the new volume (omv-) to the original name (osv-)
             self.client.modifyVolume(temp_vol_name, {'newName': volume_name})
+            LOG.debug('Volume rename completed: convert_to_base_volume: '
+                      'id=%s.', volume['id'])
 
             LOG.info('Completed: convert_to_base_volume: '
                      'id=%s.', volume['id'])
diff --git a/releasenotes/notes/hpe-3par-convert-to-base-vol-delete-snap-a460a4b1c419804a.yaml b/releasenotes/notes/hpe-3par-convert-to-base-vol-delete-snap-a460a4b1c419804a.yaml
new file mode 100644
index 000000000..e087e3353
--- /dev/null
+++ b/releasenotes/notes/hpe-3par-convert-to-base-vol-delete-snap-a460a4b1c419804a.yaml
@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    HPE 3PAR driver `Bug #1994521 <https://bugs.launchpad.net/cinder/+bug/1994521>`_:
+    Fixed: While performing a delete snapshot (s1) operation, the volumes (v2)
+    dependent on the snapshot (s1) are converted to base volumes. This
+    operation fails if these dependent volumes (v2) have their own dependent
+    snapshots (s2). The errors during the failure were vague and not helpful.
+    With this release, we added conditions to fail this operation early and
+    also added useful error message.
+