From b6ddbcda43a0a5b0c11be2192165d9235b522fe9 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 29 Jun 2020 19:43:15 +0200 Subject: [PATCH] Support references in driver configuration Cinder, through Oslo Config, supports references as values in its configuration options, such as the default value used in target_ip_address which is $my_ip. This patch adds the same functionality to cinderlib, which is convenient for library users that use the default values reported by the list_supported_drivers for the options to set the values. In other words, if for example we don't pass target_ip_address for the LVM driver configuration, then it correctly uses the value of my_ip field, but if we get the default value and pass it, the driver will fail, since the literal '$my_ip' is not usable as an IP address. Change-Id: I038da3637268c9e43c464e841f8c357b212fbc9f --- cinderlib/cinderlib.py | 47 ++++--- cinderlib/persistence/__init__.py | 18 --- cinderlib/tests/unit/test_cinderlib.py | 132 +++++++++++++++--- doc/source/topics/backends.rst | 18 +++ ...cept-template-values-e228d065800ac5b5.yaml | 5 + 5 files changed, 165 insertions(+), 55 deletions(-) create mode 100644 releasenotes/notes/accept-template-values-e228d065800ac5b5.yaml diff --git a/cinderlib/cinderlib.py b/cinderlib/cinderlib.py index 5b40ed2..5e94f28 100644 --- a/cinderlib/cinderlib.py +++ b/cinderlib/cinderlib.py @@ -204,15 +204,14 @@ class Backend(object): if host: cfg.CONF.host = host - cls._validate_options(cinder_config_params) - for k, v in cinder_config_params.items(): - setattr(cfg.CONF, k, v) + cls._validate_and_set_options(cinder_config_params) # Replace command line arg parser so we ignore caller's args cfg._CachedArgumentParser.parse_args = lambda *a, **kw: None @classmethod - def _validate_options(cls, kvs, group=None): + def _validate_and_set_options(cls, kvs, group=None): + """Validate options and substitute references.""" # Dynamically loading the driver triggers adding the specific # configuration options to the backend_defaults section if kvs.get('volume_driver'): @@ -221,22 +220,36 @@ class Backend(object): group = group or 'backend_defaults' for k, v in kvs.items(): - # The config may be removed from the Cinder RBD driver, but the - # functionality will remain for cinderlib usage only, so we do the - # validation manually. - if k == 'rbd_keyring_conf': - if v and not isinstance(v, str): - raise ValueError('%s must be a string' % k) - continue - try: # set_override does the validation cfg.CONF.set_override(k, v, group) - # for correctness, don't leave it there - cfg.CONF.clear_override(k, group) except cfg.NoSuchOptError: - # Don't fail on unknown variables, behave like cinder - LOG.warning('Unknown config option %s', k) + # RBD keyring may be removed from the Cinder RBD driver, but + # the functionality will remain for cinderlib usage only, so we + # do the validation manually in that case. + # NOTE: Templating won't work on the rbd_keyring_conf, but it's + # unlikely to be needed. + if k == 'rbd_keyring_conf': + if v and not isinstance(v, str): + raise ValueError('%s must be a string' % k) + else: + # Don't fail on unknown variables, behave like cinder + LOG.warning('Unknown config option %s', k) + + oslo_group = getattr(cfg.CONF, str(group), cfg.CONF) + # Now that we have validated/templated everything set updated values + for k, v in kvs.items(): + kvs[k] = getattr(oslo_group, k, v) + + # For global configuration we leave the overrides, but for drivers we + # don't to prevent cross-driver config polination. The cfg will be + # set as an attribute of the configuration that's passed to the driver. + if group: + for k in kvs.keys(): + try: + cfg.CONF.clear_override(k, group, clear_cache=True) + except cfg.NoSuchOptError: + pass def _get_backend_config(self, driver_cfg): # Create the group for the backend @@ -244,8 +257,8 @@ class Backend(object): cfg.CONF.register_group(cfg.OptGroup(backend_name)) # Validate and set config options + self._validate_and_set_options(driver_cfg) backend_group = getattr(cfg.CONF, backend_name) - self._validate_options(driver_cfg) for key, value in driver_cfg.items(): setattr(backend_group, key, value) diff --git a/cinderlib/persistence/__init__.py b/cinderlib/persistence/__init__.py index 26bfc7b..e77db37 100644 --- a/cinderlib/persistence/__init__.py +++ b/cinderlib/persistence/__init__.py @@ -15,7 +15,6 @@ from __future__ import absolute_import import inspect -from cinder.cmd import volume as volume_cmd import six from stevedore import driver @@ -26,20 +25,6 @@ from cinderlib.persistence import base DEFAULT_STORAGE = 'memory' -class MyDict(dict): - """Custom non clearable dictionary. - - Required to overcome the nature of oslo.config where configuration comes - from files and command line input. - - Using this dictionary we can load from memory everything and it won't clear - things when we dynamically load a driver and the driver has new - configuration options. - """ - def clear(self): - pass - - def setup(config): """Setup persistence to be used in cinderlib. @@ -61,9 +46,6 @@ def setup(config): else: config = config.copy() - # Prevent driver dynamic loading clearing configuration options - volume_cmd.CONF._ConfigOpts__cache = MyDict() - # Default configuration is using memory storage storage = config.pop('storage', None) or DEFAULT_STORAGE if isinstance(storage, base.PersistenceDriverBase): diff --git a/cinderlib/tests/unit/test_cinderlib.py b/cinderlib/tests/unit/test_cinderlib.py index c4d5da1..80d613f 100644 --- a/cinderlib/tests/unit/test_cinderlib.py +++ b/cinderlib/tests/unit/test_cinderlib.py @@ -101,12 +101,11 @@ class TestCinderlib(base.BaseTest): self.assertEqual(('default',), backend.pool_names) mock_workarounds.assert_called_once_with(mock_config.return_value) - @mock.patch('cinderlib.Backend._validate_options') + @mock.patch('cinderlib.Backend._validate_and_set_options') @mock.patch.object(cfg, 'CONF') def test__set_cinder_config(self, conf_mock, validate_mock): - cinder_cfg = {'debug': True} - - objects.Backend._set_cinder_config('host', 'locks_path', cinder_cfg) + objects.Backend._set_cinder_config('host', 'locks_path', + mock.sentinel.cfg) self.assertEqual(2, conf_mock.set_default.call_count) conf_mock.set_default.assert_has_calls( @@ -120,9 +119,7 @@ class TestCinderlib(base.BaseTest): cfg.CONF.coordination.backend_url) self.assertEqual('host', cfg.CONF.host) - validate_mock.assert_called_once_with(cinder_cfg) - - self.assertEqual(True, cfg.CONF.debug) + validate_mock.assert_called_once_with(mock.sentinel.cfg) self.assertIsNone(cfg._CachedArgumentParser().parse_args()) @@ -176,40 +173,135 @@ class TestCinderlib(base.BaseTest): cls.output_all_backend_info) @mock.patch('cinderlib.cinderlib.LOG.warning') - def test__validate_options(self, warning_mock): + def test__validate_and_set_options(self, warning_mock): + self.addCleanup(cfg.CONF.clear_override, 'osapi_volume_extension') + self.addCleanup(cfg.CONF.clear_override, 'debug') # Validate default group config with Boolean and MultiStrOpt - self.backend._validate_options( + self.backend._validate_and_set_options( {'debug': True, 'osapi_volume_extension': ['a', 'b', 'c'], }) - # Test driver options with String, ListOpt, PortOpt - self.backend._validate_options( - {'volume_driver': 'cinder.volume.drivers.lvm.LVMVolumeDriver', - 'volume_group': 'cinder-volumes', - 'iscsi_secondary_ip_addresses': ['w.x.y.z', 'a.b.c.d'], - 'target_port': 12345, - }) + # Global values overrides are left + self.assertIs(True, cfg.CONF.debug) + self.assertEqual(['a', 'b', 'c'], cfg.CONF.osapi_volume_extension) + cinder_cfg = { + 'volume_driver': 'cinder.volume.drivers.lvm.LVMVolumeDriver', + 'volume_group': 'lvm-volumes', + 'iscsi_secondary_ip_addresses': ['w.x.y.z', 'a.b.c.d'], + 'target_port': 12345, + } + expected_cfg = cinder_cfg.copy() + + # Test driver options with String, ListOpt, PortOpt + self.backend._validate_and_set_options(cinder_cfg) + # Non global value overrides have been cleaned up + self.assertEqual('cinder-volumes', + cfg.CONF.backend_defaults.volume_group) + self.assertEqual( + [], cfg.CONF.backend_defaults.iscsi_secondary_ip_addresses) + self.assertEqual(3260, cfg.CONF.backend_defaults.target_port) + self.assertEqual(expected_cfg, cinder_cfg) + + warning_mock.assert_not_called() + + @mock.patch('cinderlib.cinderlib.LOG.warning') + def test__validate_and_set_options_rbd(self, warning_mock): + original_override = cfg.CONF.set_override + original_getattr = cfg.ConfigOpts.GroupAttr.__getattr__ + + def my_override(option, value, *args): + original_override(option, value, *args) + # Simulate that the config option is missing if it's not + if option == 'rbd_keyring_conf': + raise cfg.NoSuchOptError('rbd_keyring_conf') + + def my_getattr(self, name): + res = original_getattr(self, name) + # Simulate that the config option is missing if it's not + if name == 'rbd_keyring_conf': + raise AttributeError() + return res + + self.patch('oslo_config.cfg.ConfigOpts.GroupAttr.__getattr__', + my_getattr) + self.patch('oslo_config.cfg.CONF.set_override', + side_effect=my_override) + + cinder_cfg = {'volume_driver': 'cinder.volume.drivers.rbd.RBDDriver', + 'rbd_keyring_conf': '/etc/ceph/ceph.client.adm.keyring', + 'rbd_user': 'adm', + 'rbd_pool': 'volumes'} + expected_cfg = cinder_cfg.copy() + # Test driver options with String, ListOpt, PortOpt + self.backend._validate_and_set_options(cinder_cfg) + self.assertEqual(expected_cfg, cinder_cfg) + # Non global value overrides have been cleaned up + self.assertEqual(None, cfg.CONF.backend_defaults.rbd_user) + self.assertEqual('rbd', cfg.CONF.backend_defaults.rbd_pool) warning_mock.assert_not_called() @ddt.data( ('debug', 'sure', None), ('target_port', 'abc', 'cinder.volume.drivers.lvm.LVMVolumeDriver')) @ddt.unpack - def test__validate_options_failures(self, option, value, driver): + def test__validate_and_set_options_failures(self, option, value, + driver): self.assertRaises( ValueError, - self.backend._validate_options, + self.backend._validate_and_set_options, {'volume_driver': driver, option: value}) @mock.patch('cinderlib.cinderlib.LOG.warning') - def test__validate_options_unkown(self, warning_mock): - self.backend._validate_options( + def test__validate_and_set_options_unknown(self, warning_mock): + self.backend._validate_and_set_options( {'volume_driver': 'cinder.volume.drivers.lvm.LVMVolumeDriver', 'vmware_cluster_name': 'name'}) self.assertEqual(1, warning_mock.call_count) + def test_validate_and_set_options_templates(self): + self.addCleanup(cfg.CONF.clear_override, 'my_ip') + cfg.CONF.set_override('my_ip', '127.0.0.1') + + config_options = dict( + volume_driver='cinder.volume.drivers.lvm.LVMVolumeDriver', + volume_backend_name='lvm_iscsi', + volume_group='my-${backend_defaults.volume_backend_name}-vg', + target_ip_address='$my_ip', + ) + expected = dict( + volume_driver='cinder.volume.drivers.lvm.LVMVolumeDriver', + volume_backend_name='lvm_iscsi', + volume_group='my-lvm_iscsi-vg', + target_ip_address='127.0.0.1', + ) + self.backend._validate_and_set_options(config_options) + + self.assertDictEqual(expected, config_options) + + # Non global value overrides have been cleaned up + self.assertEqual('cinder-volumes', + cfg.CONF.backend_defaults.volume_group) + + @mock.patch('cinderlib.cinderlib.Backend._validate_and_set_options') + def test__get_backend_config(self, mock_validate): + def my_validate(*args): + # Simulate the cache clear happening in _validate_and_set_options + cfg.CONF.clear_override('my_ip') + + mock_validate.side_effect = my_validate + config_options = dict( + volume_driver='cinder.volume.drivers.lvm.LVMVolumeDriver', + volume_backend_name='lvm_iscsi', + volume_group='volumes', + ) + res = self.backend._get_backend_config(config_options) + mock_validate.assert_called_once_with(config_options) + self.assertEqual('lvm_iscsi', res.config_group) + for opt in config_options.keys(): + self.assertEqual(config_options[opt], getattr(res, opt)) + def test_pool_names(self): pool_names = [mock.sentinel._pool_names] self.backend._pool_names = pool_names diff --git a/doc/source/topics/backends.rst b/doc/source/topics/backends.rst index 68e2764..be2abe0 100644 --- a/doc/source/topics/backends.rst +++ b/doc/source/topics/backends.rst @@ -50,6 +50,24 @@ To find what configuration options are available and which ones are compulsory the best is going to the Vendor's documentation or to the `OpenStack's Cinder volume driver configuration documentation`_. +*Cinderlib* supports references in the configuration values using the forms: + +- ``$[.]`` +- ``${[.]}`` + +Where ``config_group`` is ``backend_defaults`` for the driver configuration +options. + +.. attention:: + + The ``rbd_keyring_file`` configuration parameter does not accept + templating. + +Examples: + +- ``target_ip_address='$my_ip'`` +- ``volume_group='my-${backend_defaults.volume_backend_name}-vg'`` + .. attention:: Some drivers have external dependencies which we must satisfy before diff --git a/releasenotes/notes/accept-template-values-e228d065800ac5b5.yaml b/releasenotes/notes/accept-template-values-e228d065800ac5b5.yaml new file mode 100644 index 0000000..19803eb --- /dev/null +++ b/releasenotes/notes/accept-template-values-e228d065800ac5b5.yaml @@ -0,0 +1,5 @@ +--- +features: + - | + Support references in driver configuration values passed to + ``cinderlib.Backend``, such as ``target_ip_address='$my_ip'``.