From f3aee27a2298ec08bc2ea817207bef6afaa066eb Mon Sep 17 00:00:00 2001 From: Tom Swanson Date: Tue, 14 Jul 2015 12:21:19 -0500 Subject: [PATCH] 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 --- cinder/tests/unit/test_dellsc.py | 146 +++++++++++++++--- cinder/tests/unit/test_dellscapi.py | 50 ++++++ .../drivers/dell/dell_storagecenter_api.py | 19 +++ .../drivers/dell/dell_storagecenter_common.py | 51 ++++-- 4 files changed, 235 insertions(+), 31 deletions(-) diff --git a/cinder/tests/unit/test_dellsc.py b/cinder/tests/unit/test_dellsc.py index a99fdd60b..00d848dad 100644 --- 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 6fc071a45..1649f47aa 100644 --- 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 ddc38542b..f9296ad5a 100644 --- 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 f200cacf8..923558047 100644 --- 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'} -- 2.45.2