From 1e4d070a5b638de58c87363b18a76171dcd0ce38 Mon Sep 17 00:00:00 2001 From: Vipin Balachandran Date: Mon, 17 Mar 2014 23:46:22 +0530 Subject: [PATCH] vmware: Remove pbm_default_policy config option The pbm_default_policy config option is used to associate a storage policy with the volume backing if there is no associated volume_type or the volume_type doesn't have the storage_profile option. A better approach is to set the storage_profile option in the default_volume_type. This change removes the pbm_default_policy and queries the policy using the storage_profile option in the given volume_type. Closes-Bug: #1291181 Closes-Bug: #1291804 DocImpact Change-Id: I2519ec1145712ce6927dacde91db468d066af021 --- cinder/tests/test_vmware_vmdk.py | 57 +++++++++++----------- cinder/volume/drivers/vmware/error_util.py | 5 -- cinder/volume/drivers/vmware/vmdk.py | 42 +++------------- etc/cinder/cinder.conf.sample | 6 --- 4 files changed, 36 insertions(+), 74 deletions(-) diff --git a/cinder/tests/test_vmware_vmdk.py b/cinder/tests/test_vmware_vmdk.py index 7031a2bbf..dd7d399aa 100644 --- a/cinder/tests/test_vmware_vmdk.py +++ b/cinder/tests/test_vmware_vmdk.py @@ -1213,12 +1213,10 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase): """Test class for VMwareVcVmdkDriver.""" VMDK_DRIVER = vmdk.VMwareVcVmdkDriver - DEFAULT_PROFILE = 'fakeProfile' DEFAULT_VC_VERSION = '5.5' def setUp(self): super(VMwareVcVmdkDriverTestCase, self).setUp() - self._config.pbm_default_policy = self.DEFAULT_PROFILE self._config.vmware_host_version = self.DEFAULT_VC_VERSION self._driver = vmdk.VMwareVcVmdkDriver(configuration=self._config) @@ -1266,35 +1264,38 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase): '_get_vc_version') @mock.patch('cinder.volume.drivers.vmware.vmdk.VMwareVcVmdkDriver.' 'session', new_callable=mock.PropertyMock) - @mock.patch('cinder.volume.drivers.vmware.vmdk.VMwareVcVmdkDriver.' - 'volumeops', new_callable=mock.PropertyMock) - def test_do_setup(self, vol_ops, session, _get_vc_version, - _get_pbm_wsdl_location): - vol_ops = vol_ops.return_value + def test_do_setup(self, session, _get_vc_version, _get_pbm_wsdl_location): session = session.return_value - # pbm is enabled and pbm_default_policy is used - _get_vc_version.return_value = LooseVersion('5.5') - _get_pbm_wsdl_location.return_value = 'fake_pbm_location' - self._driver.do_setup(mock.ANY) - default = self.DEFAULT_PROFILE - vol_ops.retrieve_profile_id.assert_called_once_with(default) - self.assertTrue(self._driver._storage_policy_enabled) - - # pbm is enabled and pbm_default_policy is wrong - self._driver._storage_policy_enabled = False - vol_ops.retrieve_profile_id.reset_mock() - vol_ops.retrieve_profile_id.return_value = None - self.assertRaises(error_util.PbmDefaultPolicyDoesNotExist, - self._driver.do_setup, mock.ANY) - vol_ops.retrieve_profile_id.assert_called_once_with(default) # pbm is disabled - self._driver._storage_policy_enabled = False - vol_ops.retrieve_profile_id.reset_mock() - _get_vc_version.return_value = LooseVersion('5.0') + vc_version = LooseVersion('5.0') + _get_vc_version.return_value = vc_version self._driver.do_setup(mock.ANY) self.assertFalse(self._driver._storage_policy_enabled) - self.assertFalse(vol_ops.retrieve_profile_id.called) + _get_vc_version.assert_called_once_with() + + # pbm is enabled and invalid pbm wsdl location + vc_version = LooseVersion('5.5') + _get_vc_version.reset_mock() + _get_vc_version.return_value = vc_version + _get_pbm_wsdl_location.return_value = None + self.assertRaises(error_util.VMwareDriverException, + self._driver.do_setup, + mock.ANY) + self.assertFalse(self._driver._storage_policy_enabled) + _get_vc_version.assert_called_once_with() + _get_pbm_wsdl_location.assert_called_once_with(vc_version) + + # pbm is enabled and valid pbm wsdl location + vc_version = LooseVersion('5.5') + _get_vc_version.reset_mock() + _get_vc_version.return_value = vc_version + _get_pbm_wsdl_location.reset_mock() + _get_pbm_wsdl_location.return_value = 'fake_pbm_location' + self._driver.do_setup(mock.ANY) + self.assertTrue(self._driver._storage_policy_enabled) + _get_vc_version.assert_called_once_with() + _get_pbm_wsdl_location.assert_called_once_with(vc_version) @mock.patch.object(VMDK_DRIVER, '_extend_volumeops_virtual_disk') @mock.patch.object(VMDK_DRIVER, '_create_backing_in_inventory') @@ -1672,11 +1673,11 @@ class VMwareVcVmdkDriverTestCase(VMwareEsxVmdkDriverTestCase): spec_key = 'vmware:storage_profile' get_volume_type_extra_specs.assert_called_once_with(fake_id, spec_key) - # default profile should be returned when no storage profile is + # None should be returned when no storage profile is # associated with the volume type get_volume_type_extra_specs.return_value = False profile = self._driver._get_storage_profile(volume) - self.assertEqual(self.DEFAULT_PROFILE, profile) + self.assertIsNone(profile) @mock.patch('cinder.volume.drivers.vmware.vim_util.' 'convert_datastores_to_hubs') diff --git a/cinder/volume/drivers/vmware/error_util.py b/cinder/volume/drivers/vmware/error_util.py index a9c02bd86..fdfc36210 100644 --- a/cinder/volume/drivers/vmware/error_util.py +++ b/cinder/volume/drivers/vmware/error_util.py @@ -62,8 +62,3 @@ class VMwaredriverConfigurationException(VMwareDriverException): """Base class for all configuration exceptions. """ message = _("VMware VMDK driver configuration error.") - - -class PbmDefaultPolicyDoesNotExist(VMwaredriverConfigurationException): - message = _("The configured default PBM policy is not defined on " - "vCenter Server.") diff --git a/cinder/volume/drivers/vmware/vmdk.py b/cinder/volume/drivers/vmware/vmdk.py index dbd31f497..291f8c2d3 100644 --- a/cinder/volume/drivers/vmware/vmdk.py +++ b/cinder/volume/drivers/vmware/vmdk.py @@ -91,16 +91,8 @@ vmdk_opts = [ 'override the VC server version.'), ] -spbm_opts = [ - cfg.StrOpt('pbm_default_policy', - help='The PBM default policy. If using VC server version 5.5 ' - 'or above and there is no defined storage policy for the ' - 'specific request then this policy will be used.'), -] - CONF = cfg.CONF CONF.register_opts(vmdk_opts) -CONF.register_opts(spbm_opts) def _get_volume_type_extra_spec(type_id, spec_key, possible_values=None, @@ -356,19 +348,16 @@ class VMwareEsxVmdkDriver(driver.VolumeDriver): return best_summary def _get_storage_profile(self, volume): - """Get storage profile associated with this volume's volume_type. + """Get storage profile associated with the given volume's volume_type. - :param volume: volume whose storage profile should be queried - :return: string value of storage profile if volume type is associated, - default global profile if configured in pbm_default_profile, - None otherwise + :param volume: Volume whose storage profile should be queried + :return: String value of storage profile if volume type is associated + and contains storage_profile extra_spec option; None otherwise """ type_id = volume['volume_type_id'] - if not type_id: - return - default_policy = self.configuration.pbm_default_policy - return _get_volume_type_extra_spec(type_id, 'storage_profile', - default_value=default_policy) + if type_id is None: + return None + return _get_volume_type_extra_spec(type_id, 'storage_profile') def _filter_ds_by_profile(self, datastores, storage_profile): """Filter out datastores that do not match given storage profile. @@ -1062,7 +1051,6 @@ class VMwareVcVmdkDriver(VMwareEsxVmdkDriver): def __init__(self, *args, **kwargs): super(VMwareVcVmdkDriver, self).__init__(*args, **kwargs) - self.configuration.append_config_values(spbm_opts) self._session = None @property @@ -1144,22 +1132,6 @@ class VMwareVcVmdkDriver(VMwareEsxVmdkDriver): max_objects = self.configuration.vmware_max_objects_retrieval self._volumeops = volumeops.VMwareVolumeOps(self.session, max_objects) - # if default policy is configured verify it exists in VC - default_policy = self.configuration.pbm_default_policy - if default_policy: - if not self._storage_policy_enabled: - LOG.warn(_("Ignoring default policy '%(policy)s' since " - "Storage Policy Based Management is not " - "enabled on VC version %(ver)s") % - {'policy': default_policy, 'ver': vc_version}) - else: - if not self.volumeops.retrieve_profile_id(default_policy): - msg = _("The configured default PBM policy '%s' is not " - "defined on vCenter Server.") % default_policy - raise error_util.PbmDefaultPolicyDoesNotExist(message=msg) - LOG.info(_("Successfully verified existence of " - "pbm_default_policy: %s."), default_policy) - LOG.info(_("Successfully setup driver: %(driver)s for server: " "%(ip)s.") % {'driver': self.__class__.__name__, 'ip': self.configuration.vmware_host_ip}) diff --git a/etc/cinder/cinder.conf.sample b/etc/cinder/cinder.conf.sample index 88b802822..01f4de9d8 100644 --- a/etc/cinder/cinder.conf.sample +++ b/etc/cinder/cinder.conf.sample @@ -1699,12 +1699,6 @@ # Options defined in cinder.volume.drivers.vmware.vmdk # -# The PBM default policy. If using VC server version 5.5 or -# above and there is no defined storage policy for the -# specific request then this policy will be used. (string -# value) -#pbm_default_policy= - # IP address for connecting to VMware ESX/VC server. (string # value) #vmware_host_ip= -- 2.45.2