From 085cb4e4327624dd76f00f2d34aa2bf28a470d8c Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 27 Nov 2019 18:32:17 +0100 Subject: [PATCH] Fix complex configuration options Cinderlib cannot properly load configuration options of the type ListOpt, or MultiOpt where each item is a dictionary, or DictOpt... For example, when we try to load a ListOpt option we will see: oslo_config.cfg.ConfigFileValueError: Value for option XYZ from LocationInfo(location=, detail='/home/me/in_memory_file') is not valid: Value should start with "[" This is because cinderlib is treating this option as a MultiOpt. Cinderlib used to just set the values of the options in the instance and pass it to the driver, and back then we had no problems with complex types. The problem with that approach is that there are some drivers (like NetApp's) that dynamically add configuration options and then force a reload of the file, and they couldn't work with our old approach. So we changed to used oslo.config's normal parsing of files, but to do that we had to convert the driver parameters passed to cinderlib to a config file and make the parser use a StringIO instance instead of a real file. We also had to prevent it from looking at the CLI options (since those belong to the program that imports the library). This new approach is more complex, because cinderlib receives the parameters as Python primitives (list, dicts, integers, etc.), but the Oslo config parser expects an INI file conforming to the non standard types it provides. Some of these non standar types are: - DictOpt is in the form: option = key1:value1,key2:value2 - MultiOpt is in the form: option = value1 option = value2 - ListOpt is in the form: option = [value1,value2] So cinderlib would receive a list [value1, value2] or a tuple (value1, value2) and needs to generate a string with the right configuration option which can be in the form of a MultiOpt or a ListOpt, depending on how the configuration option was defined. This patch adds more logic to the conversion of cinder driver configuration parameters to oslo config file and uses the configuration options definitions to decide what conversion needs to be done. With this change we now do the right converstion for ListOpt, DictOpt, and we can even support MultiOpt options that have dictionaries as items. Closes-Bug: #1854188 Change-Id: I62e992804a3ae6aa0b4aa4f883807783197d4b33 --- cinderlib/cinderlib.py | 100 +++++++++++-- cinderlib/tests/unit/test_cinderlib.py | 140 +++++++++++++++++- ...omple-config-options-39ba50c6c9165cc3.yaml | 5 + 3 files changed, 229 insertions(+), 16 deletions(-) create mode 100644 releasenotes/notes/support-comple-config-options-39ba50c6c9165cc3.yaml diff --git a/cinderlib/cinderlib.py b/cinderlib/cinderlib.py index 9a4766e..a0322a7 100644 --- a/cinderlib/cinderlib.py +++ b/cinderlib/cinderlib.py @@ -250,24 +250,94 @@ class Backend(object): 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.""" + # Dynamically loading the driver triggers adding the specific + # configuration options to the backend_defaults section + if 'volume_driver' in kvs: + driver_ns = kvs['volume_driver'].rsplit('.', 1)[0] + __import__(driver_ns) + opts = configuration.CONF._groups['backend_defaults']._opts + return opts + + @classmethod + def __val_to_str(cls, val): + """Convert an oslo config value to its string representation. + + 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): - for key, val in kvs.items(): - # We receive list or tuple for multiopt and ConfigParser doesn't - # support repeating the same entry multiple times, so we hack our - # way around it - if isinstance(val, (list, tuple)): - 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:])) + """Set Oslo configuration options in our parser. - if not isinstance(val, six.string_types): - val = six.text_type(val) - cls._parser.set(section, key, val) + 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): backend_name = driver_cfg['volume_backend_name'] diff --git a/cinderlib/tests/unit/test_cinderlib.py b/cinderlib/tests/unit/test_cinderlib.py index 8ba4d28..9434a8d 100644 --- a/cinderlib/tests/unit/test_cinderlib.py +++ b/cinderlib/tests/unit/test_cinderlib.py @@ -13,8 +13,11 @@ # License for the specific language governing permissions and limitations # under the License. +import collections + import mock from oslo_config import cfg +from oslo_config import types import cinderlib from cinderlib import objects @@ -72,6 +75,7 @@ class TestCinderlib(base.BaseTest): driver.get_volume_stats.assert_not_called() self.assertEqual(('default',), backend.pool_names) + @mock.patch('cinderlib.Backend._Backend__convert_option_to_string') @mock.patch('urllib3.disable_warnings') @mock.patch('cinder.coordination.COORDINATOR') @mock.patch('cinderlib.Backend._set_priv_helper') @@ -79,7 +83,9 @@ class TestCinderlib(base.BaseTest): @mock.patch('cinderlib.cinderlib.serialization') @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_sudo, mock_coord, mock_disable_warn, + mock_convert_to_str): + mock_convert_to_str.side_effect = lambda *args: args[1] cls = objects.Backend cls.global_initialization = False cinder_cfg = {'k': 'v', 'k2': 'v2'} @@ -247,3 +253,135 @@ class TestCinderlib(base.BaseTest): self.backend._volumes = None 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() + for i in range(0, len(args), 2): + 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) + + 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) diff --git a/releasenotes/notes/support-comple-config-options-39ba50c6c9165cc3.yaml b/releasenotes/notes/support-comple-config-options-39ba50c6c9165cc3.yaml new file mode 100644 index 0000000..95a8c0f --- /dev/null +++ b/releasenotes/notes/support-comple-config-options-39ba50c6c9165cc3.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Bug #1854188: Work with complex configuration options like ListOpt, + DictOpt, and MultiOpt with dict items.