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=<Locations.user: (4, True)>,
  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

@@ -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']

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)

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.