From 0d241780eec2571a310593e15f0f001d5fad8637 Mon Sep 17 00:00:00 2001 From: Helen Walsh Date: Wed, 31 Aug 2016 10:54:46 +0100 Subject: [PATCH] VMAX driver - Retrieve volume from snapshot The VMAX driver retrieves volume id from snapshot['volume_name'] in create_snapshot and delete_snapshot and then gets volume from db using the volume id. If the volume_name is changed, volume id may not be in it any more. This is not a reliable way of getting volume id. Also a driver should not access db directly. In this patch, the source volume is retrieved from snapshot without parsing the volume name for volume id and accessing db. Change-Id: I99ebecb27d82d5a967369a04bf2c34158e6670ec Closes-Bug: #1616133 --- .../unit/volume/drivers/emc/test_emc_vmax.py | 60 +++++++------------ cinder/volume/drivers/emc/emc_vmax_fc.py | 18 ++---- cinder/volume/drivers/emc/emc_vmax_iscsi.py | 18 ++---- 3 files changed, 31 insertions(+), 65 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py b/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py index 3f0009c3904..a23a87ef632 100644 --- a/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py +++ b/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py @@ -490,8 +490,17 @@ class EMCVMAXCommonData(object): test_snapshot = {'name': 'myCG1', 'id': '12345abcde', 'status': 'available', - 'host': fake_host + 'host': fake_host, + 'volume': test_source_volume, + 'provider_location': six.text_type(provider_location) } + test_snapshot_v3 = {'name': 'myCG1', + 'id': '12345abcde', + 'status': 'available', + 'host': fake_host_v3, + 'volume': test_source_volume_v3, + 'provider_location': six.text_type(provider_location) + } test_CG_snapshot = {'name': 'testSnap', 'id': '12345abcde', 'consistencygroup_id': '123456789', @@ -3316,18 +3325,13 @@ class EMCVMAXISCSIDriverNoFastTestCase(test.TestCase): emc_vmax_utils.EMCVMAXUtils, 'get_volume_meta_head', return_value=[EMCVMAXCommonData.test_volume]) - @mock.patch.object( - FakeDB, - 'volume_get', - return_value=EMCVMAXCommonData.test_source_volume) @mock.patch.object( volume_types, 'get_volume_type_extra_specs', return_value={'volume_backend_name': 'ISCSINoFAST'}) def test_create_snapshot_different_sizes_meta_no_fast_success( - self, mock_volume_type, mock_volume, + self, mock_volume_type, mock_meta, mock_size, mock_pool): - self.data.test_volume['volume_name'] = "vmax-1234567" common = self.driver.common volumeDict = {'classname': u'Symm_StorageVolume', 'keybindings': EMCVMAXCommonData.keybindings} @@ -3335,13 +3339,13 @@ class EMCVMAXISCSIDriverNoFastTestCase(test.TestCase): mock.Mock(return_value=(volumeDict, 0))) common.provision.get_volume_dict_from_job = ( mock.Mock(return_value=volumeDict)) - self.driver.create_snapshot(self.data.test_volume) + self.driver.create_snapshot(self.data.test_snapshot) def test_create_snapshot_no_fast_failed(self): self.data.test_volume['volume_name'] = "vmax-1234567" self.assertRaises(exception.VolumeBackendAPIException, self.driver.create_snapshot, - self.data.test_volume) + self.data.test_snapshot) @unittest.skip("Skip until bug #1578986 is fixed") @mock.patch.object( @@ -4231,16 +4235,12 @@ class EMCVMAXISCSIDriverFastTestCase(test.TestCase): emc_vmax_utils.EMCVMAXUtils, 'get_volume_meta_head', return_value=[EMCVMAXCommonData.test_volume]) - @mock.patch.object( - FakeDB, - 'volume_get', - return_value=EMCVMAXCommonData.test_source_volume) @mock.patch.object( volume_types, 'get_volume_type_extra_specs', return_value={'volume_backend_name': 'ISCSIFAST'}) def test_create_snapshot_different_sizes_meta_fast_success( - self, mock_volume_type, mock_volume, + self, mock_volume_type, mock_meta, mock_size, mock_pool, mock_policy): self.data.test_volume['volume_name'] = "vmax-1234567" common = self.driver.common @@ -4253,13 +4253,13 @@ class EMCVMAXISCSIDriverFastTestCase(test.TestCase): mock.Mock(return_value=volumeDict)) common.fast.is_volume_in_default_SG = ( mock.Mock(return_value=True)) - self.driver.create_snapshot(self.data.test_volume) + self.driver.create_snapshot(self.data.test_snapshot) def test_create_snapshot_fast_failed(self): self.data.test_volume['volume_name'] = "vmax-1234567" self.assertRaises(exception.VolumeBackendAPIException, self.driver.create_snapshot, - self.data.test_volume) + self.data.test_snapshot) @unittest.skip("Skip until bug #1578986 is fixed") @mock.patch.object( @@ -5457,18 +5457,13 @@ class EMCVMAXFCDriverFastTestCase(test.TestCase): emc_vmax_utils.EMCVMAXUtils, 'get_volume_meta_head', return_value=[EMCVMAXCommonData.test_volume]) - @mock.patch.object( - FakeDB, - 'volume_get', - return_value=EMCVMAXCommonData.test_source_volume) @mock.patch.object( volume_types, 'get_volume_type_extra_specs', return_value={'volume_backend_name': 'FCFAST'}) def test_create_snapshot_different_sizes_meta_fast_success( - self, mock_volume_type, mock_volume, + self, mock_volume_type, mock_meta, mock_size, mock_pool, mock_policy): - self.data.test_volume['volume_name'] = "vmax-1234567" common = self.driver.common volumeDict = {'classname': u'Symm_StorageVolume', @@ -5479,7 +5474,7 @@ class EMCVMAXFCDriverFastTestCase(test.TestCase): mock.Mock(return_value=volumeDict)) common.fast.is_volume_in_default_SG = ( mock.Mock(return_value=True)) - self.driver.create_snapshot(self.data.test_volume) + self.driver.create_snapshot(self.data.test_snapshot) @mock.patch.object( emc_vmax_common.EMCVMAXCommon, @@ -5489,7 +5484,7 @@ class EMCVMAXFCDriverFastTestCase(test.TestCase): self.data.test_volume['volume_name'] = "vmax-1234567" self.assertRaises(exception.VolumeBackendAPIException, self.driver.create_snapshot, - self.data.test_volume) + self.data.test_snapshot) @unittest.skip("Skip until bug #1578986 is fixed") @mock.patch.object( @@ -6065,13 +6060,8 @@ class EMCV3DriverTestCase(test.TestCase): volume_types, 'get_volume_type_extra_specs', return_value={'volume_backend_name': 'V3_BE'}) - @mock.patch.object( - FakeDB, - 'volume_get', - return_value=EMCVMAXCommonData.test_source_volume_v3) def test_create_snapshot_v3_success( - self, mock_volume_db, mock_type, moke_pool): - self.data.test_volume_v3['volume_name'] = "vmax-1234567" + self, mock_type, mock_pool): common = self.driver.common common.provisionv3.utils.get_v3_default_sg_instance_name = mock.Mock( return_value=(None, None, self.data.default_sg_instance_name)) @@ -6079,21 +6069,17 @@ class EMCV3DriverTestCase(test.TestCase): mock.Mock(return_value=True)) common._initial_setup = mock.Mock( return_value=self.default_extraspec()) - self.driver.create_snapshot(self.data.test_volume_v3) + self.driver.create_snapshot(self.data.test_snapshot_v3) - @mock.patch.object( - FakeDB, - 'volume_get', - return_value=EMCVMAXCommonData.test_source_volume_v3) @mock.patch.object( volume_types, 'get_volume_type_extra_specs', return_value={'volume_backend_name': 'V3_BE'}) - def test_delete_snapshot_v3_success(self, mock_volume_type, mock_db): + def test_delete_snapshot_v3_success(self, mock_volume_type): self.data.test_volume_v3['volume_name'] = "vmax-1234567" self.driver.common._initial_setup = mock.Mock( return_value=self.default_extraspec()) - self.driver.delete_snapshot(self.data.test_volume_v3) + self.driver.delete_snapshot(self.data.test_snapshot_v3) @unittest.skip("Skip until bug #1578986 is fixed") @mock.patch.object( diff --git a/cinder/volume/drivers/emc/emc_vmax_fc.py b/cinder/volume/drivers/emc/emc_vmax_fc.py index cefcfe9ed4c..766c59b85bc 100644 --- a/cinder/volume/drivers/emc/emc_vmax_fc.py +++ b/cinder/volume/drivers/emc/emc_vmax_fc.py @@ -18,7 +18,6 @@ import ast from oslo_log import log as logging import six -from cinder import context from cinder.i18n import _LW from cinder import interface from cinder.volume import driver @@ -123,13 +122,8 @@ class EMCVMAXFCDriver(driver.FibreChannelDriver): def create_snapshot(self, snapshot): """Creates a snapshot.""" - ctxt = context.get_admin_context() - volumename = snapshot['volume_name'] - index = volumename.index('-') - volumeid = volumename[index + 1:] - volume = self.db.volume_get(ctxt, volumeid) - - volpath = self.common.create_snapshot(snapshot, volume) + src_volume = snapshot['volume'] + volpath = self.common.create_snapshot(snapshot, src_volume) model_update = {} snapshot['provider_location'] = six.text_type(volpath) @@ -138,13 +132,9 @@ class EMCVMAXFCDriver(driver.FibreChannelDriver): def delete_snapshot(self, snapshot): """Deletes a snapshot.""" - ctxt = context.get_admin_context() - volumename = snapshot['volume_name'] - index = volumename.index('-') - volumeid = volumename[index + 1:] - volume = self.db.volume_get(ctxt, volumeid) + src_volume = snapshot['volume'] - self.common.delete_snapshot(snapshot, volume) + self.common.delete_snapshot(snapshot, src_volume) def ensure_export(self, context, volume): """Driver entry point to get the export info for an existing volume.""" diff --git a/cinder/volume/drivers/emc/emc_vmax_iscsi.py b/cinder/volume/drivers/emc/emc_vmax_iscsi.py index a5df907dbfe..f5ccb0c36ee 100644 --- a/cinder/volume/drivers/emc/emc_vmax_iscsi.py +++ b/cinder/volume/drivers/emc/emc_vmax_iscsi.py @@ -21,7 +21,6 @@ import os from oslo_log import log as logging import six -from cinder import context from cinder import exception from cinder.i18n import _, _LE, _LI from cinder import interface @@ -132,13 +131,8 @@ class EMCVMAXISCSIDriver(driver.ISCSIDriver): def create_snapshot(self, snapshot): """Creates a snapshot.""" - ctxt = context.get_admin_context() - volumename = snapshot['volume_name'] - index = volumename.index('-') - volumeid = volumename[index + 1:] - volume = self.db.volume_get(ctxt, volumeid) - - volpath = self.common.create_snapshot(snapshot, volume) + src_volume = snapshot['volume'] + volpath = self.common.create_snapshot(snapshot, src_volume) model_update = {} snapshot['provider_location'] = six.text_type(volpath) @@ -147,13 +141,9 @@ class EMCVMAXISCSIDriver(driver.ISCSIDriver): def delete_snapshot(self, snapshot): """Deletes a snapshot.""" - ctxt = context.get_admin_context() - volumename = snapshot['volume_name'] - index = volumename.index('-') - volumeid = volumename[index + 1:] - volume = self.db.volume_get(ctxt, volumeid) + src_volume = snapshot['volume'] - self.common.delete_snapshot(snapshot, volume) + self.common.delete_snapshot(snapshot, src_volume) def ensure_export(self, context, volume): """Driver entry point to get the export info for an existing volume."""