From fbd7a72c1bbfe4696eada41064c476ba3ecd6a8f Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 18 Dec 2019 17:48:16 +0100 Subject: [PATCH] Fix configuration options When fixing bug #1854188 (Change-ID: I62e992804a3ae6aa0b4aa4f883807783197d4b33) we broke configuration options, because now we cannot pass configuration options from the DEFAULT section, and some ListOpt don't work because cinderlib was incorrectly expecting all these options to have bounds=True, but some don't define it and default to False. Most of the "crazy" code we currently have around oslo config is because we wanted to support drivers that add configuration options on runtime and reload the configuration. This made sense when cinderlib was not an OpenStack official project, but now that it is it makes more sense to force Cinder drivers to do the right thing. Aligned with this, this patch removes all the faking of the cinder.conf file as a StringIO object, and simply sets things in the configuration object that is passed to the drivers, which is the only thing they should be using. This will make cinderlib code more robust, even if some drivers will now need to be modified to work with cinderlib. We are validating known configuration options and ignoring unknown ones, like Cinder does. The NetApp driver was one of the drivers that would get broken, but since I have access to it and we can make a very small and targeted workaround to avoid breaking it, we do it as well. Closes-Bug: #1856556 Change-Id: I67fbf8e9d7ee79f3d6617b4d0ae755dae0e1987a --- cinderlib/cinderlib.py | 185 +++++--------------- cinderlib/tests/unit/test_cinderlib.py | 225 ++++++++++--------------- doc/source/validated.rst | 4 +- 3 files changed, 134 insertions(+), 280 deletions(-) diff --git a/cinderlib/cinderlib.py b/cinderlib/cinderlib.py index a0322a7..26222b9 100644 --- a/cinderlib/cinderlib.py +++ b/cinderlib/cinderlib.py @@ -18,7 +18,6 @@ import json as json_lib import logging import multiprocessing import os -import six from cinder import coordination from cinder.db import api as db_api @@ -31,7 +30,7 @@ cinder_objects.register_all() # noqa from cinder.interface import util as cinder_interface_util from cinder import utils from cinder.volume import configuration -from cinder.volume import manager +from cinder.volume import manager # noqa We need to import config options from oslo_config import cfg from oslo_log import log as oslo_logging from oslo_utils import importutils @@ -76,7 +75,8 @@ class Backend(object): driver_cfg['volume_backend_name'] = volume_backend_name Backend.backends[volume_backend_name] = self - conf = self._set_backend_config(driver_cfg) + conf = self._get_backend_config(driver_cfg) + self._apply_backend_workarounds(conf) self.driver = importutils.import_object( conf.volume_driver, configuration=conf, @@ -190,164 +190,59 @@ class Backend(object): # the persistence plugin. db_api.IMPL = cls.persistence.db - # NOTE(geguileo): Staticmethod used instead of classmethod to make it work - # on Python3 when assigning the unbound method. - @staticmethod - def _config_parse(self): - """Replacer oslo_config.cfg.ConfigParser.parse for in-memory cfg.""" - res = super(cfg.ConfigParser, self).parse(Backend._config_string_io) - return res - - @classmethod - def _update_cinder_config(cls): - """Parse in-memory file to update OSLO configuration used by Cinder.""" - cls._config_string_io.seek(0) - cls._parser.write(cls._config_string_io) - - # Check if we have any multiopt - cls._config_string_io.seek(0) - current_cfg = cls._config_string_io.read() - if '\n\t' in current_cfg: - cls._config_string_io.seek(0) - cls._config_string_io.write(current_cfg.replace('\n\t', '\n')) - - cls._config_string_io.seek(0) - cfg.CONF.reload_config_files() - @classmethod def _set_cinder_config(cls, host, locks_path, cinder_config_params): """Setup the parser with all the known Cinder configuration.""" cfg.CONF.set_default('state_path', os.getcwd()) cfg.CONF.set_default('lock_path', '$state_path', 'oslo_concurrency') - - cls._parser = six.moves.configparser.SafeConfigParser() - cls._parser.set('DEFAULT', 'enabled_backends', '') + cfg.CONF.version = cinderlib.__version__ if locks_path: - cls._parser.add_section('oslo_concurrency') - cls._parser.set('oslo_concurrency', 'lock_path', locks_path) - cls._parser.add_section('coordination') - cls._parser.set('coordination', - 'backend_url', - 'file://' + locks_path) + cfg.CONF.oslo_concurrency.lock_path = locks_path + cfg.CONF.coordination.backend_url = 'file://' + locks_path + if host: - cls._parser.set('DEFAULT', 'host', host) + cfg.CONF.host = host - # All other configuration options go into the DEFAULT section - cls.__set_parser_kv(cinder_config_params, 'DEFAULT') - - # We replace the OSLO's default parser to read from a StringIO instead - # of reading from a file. - cls._config_string_io = six.moves.StringIO() - cfg.ConfigParser.parse = six.create_unbound_method(cls._config_parse, - cfg.ConfigParser) + cls._validate_options(cinder_config_params) + for k, v in cinder_config_params.items(): + setattr(cfg.CONF, k, v) # Replace command line arg parser so we ignore caller's args cfg._CachedArgumentParser.parse_args = lambda *a, **kw: None - # Update the configuration with the options we have configured - cfg.CONF(project='cinder', version=cinderlib.__version__, - default_config_files=['in_memory_file']) - cls._update_cinder_config() - - @staticmethod - def __get_options_types(kvs): - """Get loaded oslo options and load driver if we know it.""" + @classmethod + def _validate_options(cls, kvs, group=None): # Dynamically loading the driver triggers adding the specific # configuration options to the backend_defaults section - if 'volume_driver' in kvs: + if kvs.get('volume_driver'): driver_ns = kvs['volume_driver'].rsplit('.', 1)[0] __import__(driver_ns) - opts = configuration.CONF._groups['backend_defaults']._opts - return opts + group = group or 'backend_defaults' - @classmethod - def __val_to_str(cls, val): - """Convert an oslo config value to its string representation. + for k, v in kvs.items(): + 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) - Lists and tuples are treated as ListOpt classes and converted to - "[value1,value2]" instead of the standard string representation of - "['value1','value2']". - - Dictionaries are treated as DictOpt and converted to 'k1:v1,k2:v2" - instead of the standard representation of "{'k1':'v1','k2':'v2'}". - - Anything else is converted to a string. - """ - if isinstance(val, six.string_types): - return val - - # Recursion is used to to handle options like u4p_failover_target that - # is a MultiOpt where each entry is a dictionary. - if isinstance(val, (list, tuple)): - return '[' + ','.join((cls.__val_to_str(v) for v in val)) + ']' - - if isinstance(val, dict): - return ','.join('%s:%s' % (k, cls.__val_to_str(v)) - for k, v in val.items()) - - return six.text_type(val) - - @classmethod - def __convert_option_to_string(cls, key, val, opts): - """Convert a Cinder driver cfg option into oslo config file format. - - A single Python object represents multiple Oslo config types. For - example a list can be a ListOpt or a MultOpt, and their string - representations on a file are different. - - This method uses the configuration option types to return the right - string representation. - """ - opt = opts[key]['opt'] - - # Convert to a list for ListOpt opts were the caller didn't pass a list - if (isinstance(opt, cfg.ListOpt) and - not isinstance(val, (list, tuple))): - val = [val] - - # For MultiOpt we need multiple entries in the file but ConfigParser - # doesn't support repeating the same entry multiple times, so we hack - # our way around it - elif isinstance(opt, cfg.MultiOpt): - if not isinstance(val, (list, tuple)): - val = [val] if val else [] - val = [cls.__val_to_str(v) for v in val] - if not val: - val = '' - elif len(val) == 1: - val = val[0] - else: - val = (('%s\n' % val[0]) + - '\n'.join('%s = %s' % (key, v) for v in val[1:])) - - # This will handle DictOpt and ListOpt - if not isinstance(val, six.string_types): - val = cls.__val_to_str(val) - return val - - @classmethod - def __set_parser_kv(cls, kvs, section): - """Set Oslo configuration options in our parser. - - The Oslo parser needs to have the configuration options like they are - in a file, but we have them as Python objects, so we need to set them - for the parser in the format it is expecting them, strings. - """ - opts = cls.__get_options_types(kvs) - for key, val in kvs.items(): - string_val = cls.__convert_option_to_string(key, val, opts) - cls._parser.set(section, key, string_val) - - def _set_backend_config(self, driver_cfg): + def _get_backend_config(self, driver_cfg): + # Create the group for the backend backend_name = driver_cfg['volume_backend_name'] - self._parser.add_section(backend_name) - self.__set_parser_kv(driver_cfg, backend_name) - self._parser.set('DEFAULT', 'enabled_backends', - ','.join(self.backends.keys())) - self._update_cinder_config() - config = configuration.Configuration(manager.volume_backend_opts, - config_group=backend_name) + cfg.CONF.register_group(cfg.OptGroup(backend_name)) + + # Validate and set config options + backend_group = getattr(cfg.CONF, backend_name) + self._validate_options(driver_cfg) + for key, value in driver_cfg.items(): + setattr(backend_group, key, value) + + # Return the Configuration that will be passed to the driver + config = configuration.Configuration([], config_group=backend_name) return config @classmethod @@ -384,6 +279,14 @@ class Backend(object): cls.global_initialization = True cls.output_all_backend_info = output_all_backend_info + def _apply_backend_workarounds(self, config): + """Apply workarounds for drivers that do bad stuff.""" + if 'netapp' in config.volume_driver: + # Workaround NetApp's weird replication stuff that makes it reload + # config sections in get_backend_configuration. OK since we don't + # support replication. + cfg.CONF.list_all_sections = lambda: config.volume_backend_name + @classmethod def _set_logging(cls, disable_logs): if disable_logs: diff --git a/cinderlib/tests/unit/test_cinderlib.py b/cinderlib/tests/unit/test_cinderlib.py index 9434a8d..03c47e6 100644 --- a/cinderlib/tests/unit/test_cinderlib.py +++ b/cinderlib/tests/unit/test_cinderlib.py @@ -14,16 +14,18 @@ # under the License. import collections +import os +import ddt import mock from oslo_config import cfg -from oslo_config import types import cinderlib from cinderlib import objects from cinderlib.tests.unit import base +@ddt.ddt class TestCinderlib(base.BaseTest): def test_list_supported_drivers(self): expected_keys = {'version', 'class_name', 'supported', 'ci_wiki_name', @@ -40,10 +42,12 @@ class TestCinderlib(base.BaseTest): self.assertEqual(cinderlib.Backend, cinderlib.objects.Object.backend_class) + @mock.patch('cinderlib.Backend._apply_backend_workarounds') @mock.patch('oslo_utils.importutils.import_object') - @mock.patch('cinderlib.Backend._set_backend_config') + @mock.patch('cinderlib.Backend._get_backend_config') @mock.patch('cinderlib.Backend.global_setup') - def test_init(self, mock_global_setup, mock_config, mock_import): + def test_init(self, mock_global_setup, mock_config, mock_import, + mock_workarounds): cfg.CONF.set_override('host', 'host') driver_cfg = {'k': 'v', 'k2': 'v2', 'volume_backend_name': 'Test'} cinderlib.Backend.global_initialization = False @@ -74,8 +78,34 @@ class TestCinderlib(base.BaseTest): self.assertIsNone(backend._volumes) driver.get_volume_stats.assert_not_called() self.assertEqual(('default',), backend.pool_names) + mock_workarounds.assert_called_once_with(mock_config.return_value) - @mock.patch('cinderlib.Backend._Backend__convert_option_to_string') + @mock.patch('cinderlib.Backend._validate_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) + + self.assertEqual(2, conf_mock.set_default.call_count) + conf_mock.set_default.assert_has_calls( + [mock.call('state_path', os.getcwd()), + mock.call('lock_path', '$state_path', 'oslo_concurrency')]) + + self.assertEqual(cinderlib.__version__, cfg.CONF.version) + + self.assertEqual('locks_path', cfg.CONF.oslo_concurrency.lock_path) + self.assertEqual('file://locks_path', + 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) + + self.assertIsNone(cfg._CachedArgumentParser().parse_args()) + + @mock.patch('cinderlib.Backend._set_cinder_config') @mock.patch('urllib3.disable_warnings') @mock.patch('cinder.coordination.COORDINATOR') @mock.patch('cinderlib.Backend._set_priv_helper') @@ -84,13 +114,12 @@ class TestCinderlib(base.BaseTest): @mock.patch('cinderlib.Backend.set_persistence') def test_global_setup(self, mock_set_pers, mock_serial, mock_log, mock_sudo, mock_coord, mock_disable_warn, - mock_convert_to_str): - mock_convert_to_str.side_effect = lambda *args: args[1] + mock_set_config): cls = objects.Backend cls.global_initialization = False cinder_cfg = {'k': 'v', 'k2': 'v2'} - cls.global_setup('file_locks', + cls.global_setup(mock.sentinel.locks_path, mock.sentinel.root_helper, mock.sentinel.ssl_warnings, mock.sentinel.disable_logs, @@ -100,23 +129,21 @@ class TestCinderlib(base.BaseTest): mock.sentinel.user_id, mock.sentinel.pers_cfg, mock.sentinel.fail_missing_backend, - 'mock.sentinel.host', + mock.sentinel.host, **cinder_cfg) - self.assertEqual('file_locks', cfg.CONF.oslo_concurrency.lock_path) - self.assertEqual('file://file_locks', - cfg.CONF.coordination.backend_url) + mock_set_config.assert_called_once_with(mock.sentinel.host, + mock.sentinel.locks_path, + cinder_cfg) + self.assertEqual(mock.sentinel.fail_missing_backend, cls.fail_on_missing_backend) self.assertEqual(mock.sentinel.root_helper, cls.root_helper) self.assertEqual(mock.sentinel.project_id, cls.project_id) self.assertEqual(mock.sentinel.user_id, cls.user_id) self.assertEqual(mock.sentinel.non_uuid_ids, cls.non_uuid_ids) - self.assertEqual('mock.sentinel.host', cfg.CONF.host) mock_set_pers.assert_called_once_with(mock.sentinel.pers_cfg) - self.assertEqual(cinderlib.__version__, cfg.CONF.version) - mock_serial.setup.assert_called_once_with(cls) mock_log.assert_called_once_with(mock.sentinel.disable_logs) mock_sudo.assert_called_once_with(mock.sentinel.root_helper) @@ -127,6 +154,41 @@ class TestCinderlib(base.BaseTest): self.assertEqual(mock.sentinel.backend_info, cls.output_all_backend_info) + @mock.patch('cinderlib.cinderlib.LOG.warning') + def test__validate_options(self, warning_mock): + # Validate default group config with Boolean and MultiStrOpt + self.backend._validate_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, + }) + + 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): + self.assertRaises( + ValueError, + self.backend._validate_options, + {'volume_driver': driver, + option: value}) + + @mock.patch('cinderlib.cinderlib.LOG.warning') + def test__validate_options_unkown(self, warning_mock): + self.backend._validate_options( + {'volume_driver': 'cinder.volume.drivers.lvm.LVMVolumeDriver', + 'vmware_cluster_name': 'name'}) + self.assertEqual(1, warning_mock.call_count) + def test_pool_names(self): pool_names = [mock.sentinel._pool_names] self.backend._pool_names = pool_names @@ -254,32 +316,6 @@ class TestCinderlib(base.BaseTest): self.backend.refresh() self.persistence.get_volumes.assert_not_called() - def test___get_options_types(self): - # Before knowing the driver we don't have driver specific options. - opts = self.backend._Backend__get_options_types({}) - self.assertNotIn('volume_group', opts) - # But we do have the basic options - self.assertIn('volume_driver', opts) - - # When we know the driver the method can load it to retrieve its - # specific options. - cfg = {'volume_driver': 'cinder.volume.drivers.lvm.LVMVolumeDriver'} - opts = self.backend._Backend__get_options_types(cfg) - self.assertIn('volume_group', opts) - - def test___val_to_str_integer(self): - res = self.backend._Backend__val_to_str(1) - self.assertEqual('1', res) - - def test___val_to_str_list_tuple(self): - val = ['hola', 'hello'] - expected = '[hola,hello]' - res = self.backend._Backend__val_to_str(val) - self.assertEqual(expected, res) - # Same with a tuple - res = self.backend._Backend__val_to_str(tuple(val)) - self.assertEqual(expected, res) - @staticmethod def odict(*args): res = collections.OrderedDict() @@ -287,101 +323,16 @@ class TestCinderlib(base.BaseTest): res[args[i]] = args[i + 1] return res - def test___val_to_str_dict(self): - val = self.odict('k1', 'v1', 'k2', 2) - res = self.backend._Backend__val_to_str(val) - self.assertEqual('k1:v1,k2:2', res) + @mock.patch('cinderlib.cinderlib.cfg.CONF') + def test__apply_backend_workarounds(self, mock_conf): + cfg = mock.Mock(volume_driver='cinder.volume.drivers.netapp...') + self.backend._apply_backend_workarounds(cfg) + self.assertEqual(cfg.volume_backend_name, + mock_conf.list_all_sections()) - def test___val_to_str_recursive(self): - val = self.odict('k1', ['hola', 'hello'], 'k2', [1, 2]) - expected = 'k1:[hola,hello],k2:[1,2]' - res = self.backend._Backend__val_to_str(val) - self.assertEqual(expected, res) - - @mock.patch('cinderlib.Backend._Backend__val_to_str') - def test___convert_option_to_string_int(self, convert_mock): - PortType = types.Integer(1, 65535) - opt = cfg.Opt('port', type=PortType, default=9292, help='Port number') - opts = {'port': {'opt': opt, 'cli': False}} - res = self.backend._Backend__convert_option_to_string( - 'port', 12345, opts) - self.assertEqual(convert_mock.return_value, res) - convert_mock.assert_called_once_with(12345) - - @mock.patch('cinderlib.Backend._Backend__val_to_str') - def test___convert_option_to_string_listopt(self, convert_mock): - opt = cfg.ListOpt('my_opt', help='my cool option') - opts = {'my_opt': {'opt': opt, 'cli': False}} - res = self.backend._Backend__convert_option_to_string( - 'my_opt', [mock.sentinel.value], opts) - self.assertEqual(convert_mock.return_value, res) - convert_mock.assert_called_once_with([mock.sentinel.value]) - - # Same result if we don't pass a list, since method converts it - convert_mock.reset_mock() - res = self.backend._Backend__convert_option_to_string( - 'my_opt', mock.sentinel.value, opts) - self.assertEqual(convert_mock.return_value, res) - convert_mock.assert_called_once_with([mock.sentinel.value]) - - @mock.patch('cinderlib.Backend._Backend__val_to_str') - def test___convert_option_to_string_multistr_one(self, convert_mock): - convert_mock.side_effect = lambda x: x - opt = cfg.MultiStrOpt('my_opt', default=['default'], help='help') - opts = {'my_opt': {'opt': opt, 'cli': False}} - res = self.backend._Backend__convert_option_to_string( - 'my_opt', ['value1'], opts) - self.assertEqual('value1', res) - convert_mock.assert_called_once_with('value1') - - # Same result if we don't pass a list, since method converts it - convert_mock.reset_mock() - res = self.backend._Backend__convert_option_to_string( - 'my_opt', 'value1', opts) - self.assertEqual('value1', res) - convert_mock.assert_called_once_with('value1') - - @mock.patch('cinderlib.Backend._Backend__val_to_str') - def test___convert_option_to_string_multistr(self, convert_mock): - convert_mock.side_effect = lambda x: x - opt = cfg.MultiStrOpt('my_opt', default=['default'], help='help') - opts = {'my_opt': {'opt': opt, 'cli': False}} - res = self.backend._Backend__convert_option_to_string( - 'my_opt', ['value1', 'value2'], opts) - self.assertEqual('value1\nmy_opt = value2', res) - self.assertEqual(2, convert_mock.call_count) - convert_mock.assert_has_calls([mock.call('value1'), - mock.call('value2')]) - - def test___convert_option_to_string_multiopt(self): - opt = cfg.MultiOpt('my_opt', item_type=types.Dict()) - opts = {'my_opt': {'opt': opt, 'cli': False}} - elem1 = self.odict('v1_k1', 'v1_v1', 'v1_k2', 'v1_v2') - elem2 = self.odict('v2_k1', 'v2_v1', 'v2_k2', 'v2_v2') - res = self.backend._Backend__convert_option_to_string( - 'my_opt', [elem1, elem2], opts) - expect = 'v1_k1:v1_v1,v1_k2:v1_v2\nmy_opt = v2_k1:v2_v1,v2_k2:v2_v2' - self.assertEqual(expect, res) - - @mock.patch('cinderlib.Backend._parser') - @mock.patch('cinderlib.Backend._Backend__convert_option_to_string') - @mock.patch('cinderlib.Backend._Backend__get_options_types') - def test___set_parser_kv(self, types_mock, convert_mock, parser_mock): - convert_mock.side_effect = [mock.sentinel.res1, mock.sentinel.res2] - section = mock.sentinel.section - kvs = self.odict('k1', 1, 'k2', 2) - - self.backend._Backend__set_parser_kv(kvs, section) - - types_mock.assert_called_once_with(kvs) - self.assertEqual(2, convert_mock.call_count) - convert_mock.assert_has_calls( - [mock.call('k1', 1, types_mock.return_value), - mock.call('k2', 2, types_mock.return_value)], - any_order=True) - - self.assertEqual(2, parser_mock.set.call_count) - parser_mock.set.assert_has_calls( - [mock.call(section, 'k1', mock.sentinel.res1), - mock.call(section, 'k2', mock.sentinel.res2)], - any_order=True) + @mock.patch('cinderlib.cinderlib.cfg.CONF') + def test__apply_backend_workarounds_do_nothing(self, mock_conf): + cfg = mock.Mock(volume_driver='cinder.volume.drivers.lvm...') + self.backend._apply_backend_workarounds(cfg) + self.assertEqual(mock_conf.list_all_sections.return_value, + mock_conf.list_all_sections()) diff --git a/doc/source/validated.rst b/doc/source/validated.rst index 5a69606..1f7da4e 100644 --- a/doc/source/validated.rst +++ b/doc/source/validated.rst @@ -223,12 +223,12 @@ VMAX hpe3par_api_url: https://w.x.y.z:8080/api/v1 hpe3par_username: user hpe3par_password: toomanysecrets - hpe3par_cpg: CPG_name + hpe3par_cpg: [CPG_name] san_ip: w.x.y.z san_login: user san_password: toomanysecrets volume_driver: cinder.volume.drivers.hpe.hpe_3par_iscsi.HPE3PARISCSIDriver - hpe3par_iscsi_ips: w.x.y2.z2,w.x.y2.z3,w.x.y2.z4,w.x.y2.z4 + hpe3par_iscsi_ips: [w.x.y2.z2,w.x.y2.z3,w.x.y2.z4,w.x.y2.z4] hpe3par_debug: false hpe3par_iscsi_chap_enabled: false hpe3par_snapshot_retention: 0