Dell SC: Fix Consistency Group issues

The Dell Storage Center driver was not honoring the consisgroup_id
in the volume object in volume creation. Updated create_volume,
create_cloned_volume and create_volume_from_snapshot to honor this
value.

The driver was also not checking that volumes were initialized prior
to snapshotting a consistency group. The Dell backend does not
actually create the volumes until there is data or the volumes are
mapped to a server.

On create_volume_from_snapshot the driver was not checking for
cgsnapshot_id before snapshot_id.

Changed from calling the db to update snapshots to retrieving
the SnapshotList() object.

In addition some tests were not checking results. Fixed that.

Change-Id: I896f7438c0b98003aa345ca1c3841aba8835cc2a
Closes-Bug: #1474062
Closes-Bug: #1474105
Closes-Bug: #1475016

diff --git a/cinder/tests/unit/test_dellsc.py b/cinder/tests/unit/test_dellsc.py
index a99fdd60b5b29de20906055d2e26fd3f23e55a0f..00d848dad6bf75082c8a811a1c5b22b30a8fd554 100644 (file)
--- a/cinder/tests/unit/test_dellsc.py
+++ b/cinder/tests/unit/test_dellsc.py
@@ -292,6 +292,34 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
                                                    1,
                                                    None)
 
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_replay_profile',
+                       return_value='fake')
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'update_cg_volumes')
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'create_volume',
+                       return_value=VOLUME)
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_sc',
+                       return_value=12345)
+    def test_create_volume_consistency_group(self,
+                                             mock_find_sc,
+                                             mock_create_volume,
+                                             mock_update_cg_volumes,
+                                             mock_find_replay_profile,
+                                             mock_close_connection,
+                                             mock_open_connection,
+                                             mock_init):
+        volume = {'id': self.volume_name, 'size': 1,
+                  'consistencygroup_id': 'guid'}
+        self.driver.create_volume(volume)
+        mock_create_volume.assert_called_once_with(self.volume_name,
+                                                   1,
+                                                   None)
+        self.assertTrue(mock_find_replay_profile.called)
+        self.assertTrue(mock_update_cg_volumes.called)
+
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'create_volume',
                        return_value=VOLUME)
@@ -751,6 +779,8 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
                           self.driver.create_snapshot,
                           snapshot)
 
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_replay_profile')
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_sc',
                        return_value=12345)
@@ -768,12 +798,55 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
                                          mock_find_replay,
                                          mock_find_volume,
                                          mock_find_sc,
+                                         mock_find_replay_profile,
                                          mock_close_connection,
                                          mock_open_connection,
                                          mock_init):
         volume = {'id': 'fake'}
         snapshot = {'id': 'fake', 'volume_id': 'fake'}
         self.driver.create_volume_from_snapshot(volume, snapshot)
+        mock_create_view_volume.assert_called_once_with('fake',
+                                                        'fake')
+        self.assertTrue(mock_find_replay.called)
+        self.assertTrue(mock_find_volume.called)
+        self.assertFalse(mock_find_replay_profile.called)
+
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_replay_profile',
+                       return_value='fake')
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'update_cg_volumes')
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_sc',
+                       return_value=12345)
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_volume',
+                       return_value=VOLUME)
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_replay',
+                       return_value='fake')
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'create_view_volume',
+                       return_value=VOLUME)
+    def test_create_volume_from_snapshot_cg(self,
+                                            mock_create_view_volume,
+                                            mock_find_replay,
+                                            mock_find_volume,
+                                            mock_find_sc,
+                                            mock_update_cg_volumes,
+                                            mock_find_replay_profile,
+                                            mock_close_connection,
+                                            mock_open_connection,
+                                            mock_init):
+        volume = {'id': 'fake', 'consistencygroup_id': 'guid'}
+        snapshot = {'id': 'fake', 'volume_id': 'fake'}
+        self.driver.create_volume_from_snapshot(volume, snapshot)
+        mock_create_view_volume.assert_called_once_with('fake',
+                                                        'fake')
+        self.assertTrue(mock_find_replay.called)
+        self.assertTrue(mock_find_volume.called)
+        self.assertTrue(mock_find_replay_profile.called)
+        self.assertTrue(mock_update_cg_volumes.called)
 
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_sc',
@@ -826,6 +899,9 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
         self.assertRaises(exception.VolumeBackendAPIException,
                           self.driver.create_volume_from_snapshot,
                           volume, snapshot)
+        self.assertTrue(mock_find_volume.called)
+        self.assertTrue(mock_find_replay.called)
+        self.assertFalse(mock_create_view_volume.called)
 
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_sc',
@@ -833,16 +909,12 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_volume',
                        return_value=VOLUME)
-    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
-                       'find_replay',
-                       return_value='fake')
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'create_cloned_volume',
                        return_value=VOLUME)
     def test_create_cloned_volume(self,
                                   mock_create_cloned_volume,
                                   mock_find_volume,
-                                  mock_find_replay,
                                   mock_find_sc,
                                   mock_close_connection,
                                   mock_open_connection,
@@ -850,26 +922,57 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
         volume = {'id': self.volume_name + '_clone'}
         src_vref = {'id': self.volume_name}
         self.driver.create_cloned_volume(volume, src_vref)
-        mock_create_cloned_volume. \
-            assert_called_once_with(self.volume_name + '_clone',
-                                    self.VOLUME)
+        mock_create_cloned_volume.assert_called_once_with(
+            self.volume_name + '_clone',
+            self.VOLUME)
+        self.assertTrue(mock_find_volume.called)
 
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_replay_profile',
+                       return_value='fake')
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'update_cg_volumes')
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_sc',
                        return_value=12345)
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_volume',
-                       return_value=None)
+                       return_value=VOLUME)
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
-                       'find_replay',
-                       return_value='fake')
+                       'create_cloned_volume',
+                       return_value=VOLUME)
+    def test_create_cloned_volume_consistency_group(self,
+                                                    mock_create_cloned_volume,
+                                                    mock_find_volume,
+                                                    mock_find_sc,
+                                                    mock_update_cg_volumes,
+                                                    mock_find_replay_profile,
+                                                    mock_close_connection,
+                                                    mock_open_connection,
+                                                    mock_init):
+        volume = {'id': self.volume_name + '_clone',
+                  'consistencygroup_id': 'guid'}
+        src_vref = {'id': self.volume_name}
+        self.driver.create_cloned_volume(volume, src_vref)
+        mock_create_cloned_volume.assert_called_once_with(
+            self.volume_name + '_clone',
+            self.VOLUME)
+        self.assertTrue(mock_find_volume.called)
+        self.assertTrue(mock_find_replay_profile.called)
+        self.assertTrue(mock_update_cg_volumes.called)
+
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_sc',
+                       return_value=12345)
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       'find_volume',
+                       return_value=None)
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'create_cloned_volume',
                        return_value=VOLUME)
     def test_create_cloned_volume_no_volume(self,
                                             mock_create_cloned_volume,
                                             mock_find_volume,
-                                            mock_find_replay,
                                             mock_find_sc,
                                             mock_close_connection,
                                             mock_open_connection,
@@ -879,6 +982,8 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
         self.assertRaises(exception.VolumeBackendAPIException,
                           self.driver.create_cloned_volume,
                           volume, src_vref)
+        self.assertTrue(mock_find_volume.called)
+        self.assertFalse(mock_create_cloned_volume.called)
 
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_sc',
@@ -1299,17 +1404,18 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_replay_profile',
                        return_value=SCRPLAYPROFILE)
+    @mock.patch('cinder.objects.snapshot.SnapshotList.get_all_for_cgsnapshot')
     def test_create_cgsnapshot(self,
+                               mock_get_all_for_cgsnapshot,
                                mock_find_replay_profile,
                                mock_snap_cg_replay,
                                mock_close_connection,
                                mock_open_connection,
                                mock_init):
-        self.driver.db = mock.Mock()
         mock_snapshot = mock.MagicMock()
         expected_snapshots = [mock_snapshot]
-        self.driver.db.snapshot_get_all_for_cgsnapshot.return_value = (
-            expected_snapshots)
+        mock_get_all_for_cgsnapshot.return_value = (expected_snapshots)
+
         context = {}
         cggrp = {'consistencygroup_id': 'fc8f2fec-fab2-4e34-9148-c094c913b9a3',
                  'id': '100'}
@@ -1371,17 +1477,17 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_replay_profile',
                        return_value=SCRPLAYPROFILE)
+    @mock.patch('cinder.objects.snapshot.SnapshotList.get_all_for_cgsnapshot')
     def test_delete_cgsnapshot(self,
+                               mock_get_all_for_cgsnapshot,
                                mock_find_replay_profile,
                                mock_delete_cg_replay,
                                mock_close_connection,
                                mock_open_connection,
                                mock_init):
-        self.driver.db = mock.Mock()
         mock_snapshot = mock.MagicMock()
         expected_snapshots = [mock_snapshot]
-        self.driver.db.snapshot_get_all_for_cgsnapshot.return_value = (
-            expected_snapshots)
+        mock_get_all_for_cgsnapshot.return_value = (expected_snapshots)
         context = {}
         cgsnap = {'consistencygroup_id':
                   'fc8f2fec-fab2-4e34-9148-c094c913b9a3',
@@ -1401,17 +1507,17 @@ class DellSCSanISCSIDriverTestCase(test.TestCase):
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        'find_replay_profile',
                        return_value=None)
+    @mock.patch('cinder.objects.snapshot.SnapshotList.get_all_for_cgsnapshot')
     def test_delete_cgsnapshot_profile_not_found(self,
+                                                 mock_get_all_for_cgsnapshot,
                                                  mock_find_replay_profile,
                                                  mock_delete_cg_replay,
                                                  mock_close_connection,
                                                  mock_open_connection,
                                                  mock_init):
-        self.driver.db = mock.Mock()
         mock_snapshot = mock.MagicMock()
         expected_snapshots = [mock_snapshot]
-        self.driver.db.snapshot_get_all_for_cgsnapshot.return_value = (
-            expected_snapshots)
+        mock_get_all_for_cgsnapshot.return_value = (expected_snapshots)
         context = {}
         cgsnap = {'consistencygroup_id':
                   'fc8f2fec-fab2-4e34-9148-c094c913b9a3',

diff --git a/cinder/tests/unit/test_dellscapi.py b/cinder/tests/unit/test_dellscapi.py
index 6fc071a4507825e3a1b32f1bbbc46e0ff83a21d5..1649f47aa433695849109bd8f9933e3732a0255e 100644 (file)
--- a/cinder/tests/unit/test_dellscapi.py
+++ b/cinder/tests/unit/test_dellscapi.py
@@ -4528,13 +4528,58 @@ class DellSCSanAPITestCase(test.TestCase):
                                                        remove_volumes)
         self.assertFalse(res)
 
+    @mock.patch.object(dell_storagecenter_api.HttpClient,
+                       'get',
+                       return_value=RESPONSE_200)
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       '_get_json',
+                       return_value=[INACTIVE_VOLUME])
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       '_init_volume')
+    def test_init_cg_volumes_inactive(self,
+                                      mock_init_volume,
+                                      mock_get_json,
+                                      mock_get,
+                                      mock_close_connection,
+                                      mock_open_connection,
+                                      mock_init):
+        profileid = 100
+        self.scapi._init_cg_volumes(profileid)
+        self.assertTrue(mock_get.called)
+        self.assertTrue(mock_get_json.called)
+        mock_init_volume.assert_called_once_with(self.INACTIVE_VOLUME)
+
+    @mock.patch.object(dell_storagecenter_api.HttpClient,
+                       'get',
+                       return_value=RESPONSE_200)
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       '_get_json',
+                       return_value=[VOLUME])
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       '_init_volume')
+    def test_init_cg_volumes_active(self,
+                                    mock_init_volume,
+                                    mock_get_json,
+                                    mock_get,
+                                    mock_close_connection,
+                                    mock_open_connection,
+                                    mock_init):
+        profileid = 100
+        self.scapi._init_cg_volumes(profileid)
+        self.assertTrue(mock_get.called)
+        self.assertTrue(mock_get_json.called)
+        self.assertFalse(mock_init_volume.called)
+
     @mock.patch.object(dell_storagecenter_api.HttpClient,
                        'post',
                        return_value=RESPONSE_204)
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        '_get_id',
                        return_value='100')
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       '_init_cg_volumes')
     def test_snap_cg_replay(self,
+                            mock_init_cg_volumes,
                             mock_get_id,
                             mock_post,
                             mock_close_connection,
@@ -4549,6 +4594,7 @@ class DellSCSanAPITestCase(test.TestCase):
         res = self.scapi.snap_cg_replay(profile, replayid, expire)
         mock_post.assert_called_once_with(expected_url, expected_payload)
         self.assertTrue(mock_get_id.called)
+        self.assertTrue(mock_init_cg_volumes.called)
         self.assertTrue(res)
 
     @mock.patch.object(dell_storagecenter_api.HttpClient,
@@ -4557,7 +4603,10 @@ class DellSCSanAPITestCase(test.TestCase):
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
                        '_get_id',
                        return_value='100')
+    @mock.patch.object(dell_storagecenter_api.StorageCenterApi,
+                       '_init_cg_volumes')
     def test_snap_cg_replay_bad_return(self,
+                                       mock_init_cg_volumes,
                                        mock_get_id,
                                        mock_post,
                                        mock_close_connection,
@@ -4572,6 +4621,7 @@ class DellSCSanAPITestCase(test.TestCase):
         res = self.scapi.snap_cg_replay(profile, replayid, expire)
         mock_post.assert_called_once_with(expected_url, expected_payload)
         self.assertTrue(mock_get_id.called)
+        self.assertTrue(mock_init_cg_volumes.called)
         self.assertFalse(res)
 
     @mock.patch.object(dell_storagecenter_api.StorageCenterApi,

diff --git a/cinder/volume/drivers/dell/dell_storagecenter_api.py b/cinder/volume/drivers/dell/dell_storagecenter_api.py
index ddc38542ba160850a16434c57b2cdfcf21235c54..f9296ad5af6dcd5047f8de6495bbe57d11e9cb09 100644 (file)
--- a/cinder/volume/drivers/dell/dell_storagecenter_api.py
+++ b/cinder/volume/drivers/dell/dell_storagecenter_api.py
@@ -1737,6 +1737,21 @@ class StorageCenterApi(object):
             ret = self._remove_cg_volumes(profileid, remove_volumes)
         return ret
 
+    def _init_cg_volumes(self, profileid):
+        '''Gets the cg volume list and maps/unmaps the non active volumes.
+
+        :param profileid: Replay profile identifier.
+        :return: Nothing
+        '''
+        r = self.client.get('StorageCenter/ScReplayProfile/%s/VolumeList' %
+                            profileid)
+        if r.status_code == 200:
+            vols = self._get_json(r)
+            for vol in vols:
+                if (vol.get('active') is not True or
+                   vol.get('replayAllowed') is not True):
+                    self._init_volume(vol)
+
     def snap_cg_replay(self, profile, replayid, expire):
         '''Snaps a replay of a consistency group.
 
@@ -1747,6 +1762,10 @@ class StorageCenterApi(object):
         :returns: Dell SC replay object.
         '''
         if profile:
+            # We have to make sure these are snappable.
+            self._init_cg_volumes(self._get_id(profile))
+
+            # Succeed or fail we soldier on.
             payload = {}
             payload['description'] = replayid
             payload['expireTime'] = expire

diff --git a/cinder/volume/drivers/dell/dell_storagecenter_common.py b/cinder/volume/drivers/dell/dell_storagecenter_common.py
index f200cacf8fa4823b9b6f2249271ed540b040c9ae..92355804777c2c5a2764b91009cb1dae1f7ac11f 100644 (file)
--- a/cinder/volume/drivers/dell/dell_storagecenter_common.py
+++ b/cinder/volume/drivers/dell/dell_storagecenter_common.py
@@ -17,6 +17,7 @@ from oslo_log import log as logging
 from oslo_utils import excutils
 
 from cinder import exception
+from cinder import objects
 from cinder.i18n import _, _LE, _LI, _LW
 from cinder.volume import driver
 from cinder.volume.drivers.dell import dell_storagecenter_api
@@ -97,6 +98,20 @@ class DellCommonDriver(driver.VolumeDriver):
 
         return {}
 
+    def _add_volume_to_consistency_group(self, api, scvolume, volume):
+        '''Just a helper to add a volume to a consistency group.
+
+        :param api: Dell SC API opbject.
+        :param scvolume: Dell SC Volume object.
+        :param volume: Cinder Volume object.
+        :return: Nothing.
+        '''
+        if scvolume and volume.get('consistencygroup_id'):
+            profile = api.find_replay_profile(
+                volume.get('consistencygroup_id'))
+            if profile:
+                api.update_cg_volumes(profile, [volume])
+
     def create_volume(self, volume):
         '''Create a volume.'''
 
@@ -118,6 +133,8 @@ class DellCommonDriver(driver.VolumeDriver):
                     scvolume = api.create_volume(volume_name,
                                                  volume_size,
                                                  storage_profile)
+                # Update Consistency Group
+                self._add_volume_to_consistency_group(api, scvolume, volume)
             except Exception:
                 with excutils.save_and_reraise_exception():
                     LOG.error(_LE('Failed to create volume %s'),
@@ -176,7 +193,15 @@ class DellCommonDriver(driver.VolumeDriver):
         '''Create new volume from other volume's snapshot on appliance.'''
         scvolume = None
         src_volume_name = snapshot.get('volume_id')
-        snapshot_id = snapshot.get('id')
+        # This snapshot could have been created on its own or as part of a
+        # cgsnapshot.  If it was a cgsnapshot it will be identified on the Dell
+        # backend under cgsnapshot_id.  Given the volume ID and the
+        # cgsnapshot_id we can find the appropriate snapshot.
+        # So first we look for cgsnapshot_id.  If that is blank then it must
+        # have been a normal snapshot which will be found under snapshot_id.
+        snapshot_id = snapshot.get('cgsnapshot_id')
+        if not snapshot_id:
+            snapshot_id = snapshot.get('id')
         volume_name = volume.get('id')
         LOG.debug(
             'Creating new volume %(vol)s from snapshot %(snap)s '
@@ -195,6 +220,10 @@ class DellCommonDriver(driver.VolumeDriver):
                             volume_name = volume.get('id')
                             scvolume = api.create_view_volume(volume_name,
                                                               replay)
+                            # Update Consistency Group
+                            self._add_volume_to_consistency_group(api,
+                                                                  scvolume,
+                                                                  volume)
             except Exception:
                 with excutils.save_and_reraise_exception():
                     LOG.error(_LE('Failed to create volume %s'),
@@ -222,6 +251,10 @@ class DellCommonDriver(driver.VolumeDriver):
                     if srcvol is not None:
                         scvolume = api.create_cloned_volume(volume_name,
                                                             srcvol)
+                        # Update Consistency Group
+                        self._add_volume_to_consistency_group(api,
+                                                              scvolume,
+                                                              volume)
             except Exception:
                 with excutils.save_and_reraise_exception():
                     LOG.error(_LE('Failed to create volume %s'),
@@ -476,13 +509,10 @@ class DellCommonDriver(driver.VolumeDriver):
             if profile:
                 LOG.debug('profile %s replayid %s', profile, snapshotid)
                 if api.snap_cg_replay(profile, snapshotid, 0):
-                    snapshots = self.db.snapshot_get_all_for_cgsnapshot(
-                        context,
-                        snapshotid)
-                    LOG.debug(snapshots)
+                    snapshots = objects.SnapshotList().get_all_for_cgsnapshot(
+                        context, snapshotid)
                     for snapshot in snapshots:
-                        LOG.debug(snapshot)
-                        snapshot['status'] = 'available'
+                        snapshot.status = 'available'
 
                     model_update = {'status': 'available'}
 
@@ -521,11 +551,10 @@ class DellCommonDriver(driver.VolumeDriver):
                         _('Unable to delete Consistency Group snapshot %s') %
                         snapshotid)
 
-            snapshots = self.db.snapshot_get_all_for_cgsnapshot(context,
-                                                                snapshotid)
-
+            snapshots = objects.SnapshotList().get_all_for_cgsnapshot(
+                context, snapshotid)
             for snapshot in snapshots:
-                snapshot['status'] = 'deleted'
+                snapshot.status = 'deleted'
 
             model_update = {'status': 'deleted'}