gluster*: clean up volume option querying

GlusterFS has two kind of options:
- regular ones, which are a hardcoded set, and option names
  are verified in "gluster volume get"
- user ones, whose name matches user.* -- these are
  arbitrarily named, are ignored by "gluster volume get" and
  are listed in "gluster volume info" output

So far we used "gluster volume info" universally, but that,
apart from being cumbersome for regular options, is also
incorrect, as it can't distinguish an unset option name from
an undefined one (querying the former should be treated OK,
querying the second should be treated as error).

- implement querying of regular options with "gluster volume
  get" (accepting empty response)
- implement querying of user options with searching "gluster vol
  info" data
- verify operations on the XML tree, make tacit XML layout
  assumptions explicit
- implement optional Boolean coercion of values

Partially implements bp gluster-code-cleanup

Change-Id: I9e0843b88cd1a1668fe48c6979029c012dcbaa13

manila/share/drivers/glusterfs/__init__.py

@@ -96,10 +96,10 @@ class GlusterfsShareDriver(driver.ExecuteMixin, driver.GaneshaMixin,
         gluster_manager = share_manager['manager']
         # TODO(csaba): This should be refactored into proper dispatch to helper
         if self.nfs_helper == GlusterNFSHelper and not gluster_manager.path:
-            # default is 'on'
+            # default behavior of NFS_EXPORT_VOL is as if it were 'on'
             export_vol = gluster_manager.get_vol_option(
-                NFS_EXPORT_VOL) or 'on'
-            if export_vol.lower() not in ('on', '1', 'true', 'yes', 'enable'):
+                NFS_EXPORT_VOL, boolean=True)
+            if export_vol is False:
                 raise exception.GlusterfsException(
                     _("Gluster-NFS with volume layout should be used "
                       "with `nfs.export-volumes = on`"))

manila/share/drivers/glusterfs/common.py

@@ -61,6 +61,17 @@ def _check_volume_presence(f):
     return wrapper
 
 
+def volxml_get(xmlout, path, *default):
+    """Extract a value by a path from XML."""
+    value = xmlout.find(path)
+    if value is None:
+        if default:
+            return default[0]
+        raise exception.InvalidShare(
+            _('Xpath %s not found in volume query response XML') % path)
+    return value.text
+
+
 class GlusterManager(object):
     """Interface with a GlusterFS volume."""
 
@@ -68,6 +79,13 @@ class GlusterManager(object):
                         '(?P<host>[^:@/]+)'
                         '(?::/(?P<volume>[^/]+)(?P<path>/.*)?)?\Z')
 
+    # See this about GlusterFS' convention for Boolean interpretation
+    # of strings:
+    # https://github.com/gluster/glusterfs/blob/v3.7.8/
+    #         libglusterfs/src/common-utils.c#L1680-L1708
+    GLUSTERFS_TRUE_VALUES = ('ON', 'YES', 'TRUE', 'ENABLE', '1')
+    GLUSTERFS_FALSE_VALUES = ('OFF', 'NO', 'FALSE', 'DISABLE', '0')
+
     @classmethod
     def parse(cls, address):
         """Parse address string into component dict."""
@@ -174,9 +192,38 @@ class GlusterManager(object):
 
         return _gluster_call
 
-    @_check_volume_presence
-    def get_vol_option(self, option):
-        """Get the value of an option set on a GlusterFS volume."""
+    def xml_response_check(self, xmlout, command, countpath=None):
+        """Sanity check for GlusterFS XML response."""
+        commandstr = ' '.join(command)
+        ret = {}
+        for e in 'opRet', 'opErrno':
+            ret[e] = int(volxml_get(xmlout, e))
+        if ret == {'opRet': -1, 'opErrno': 0}:
+            raise exception.GlusterfsException(_(
+                'GlusterFS command %(command)s on volume %(volume)s failed'
+            ) % {'volume': self.volume, 'command': command})
+        if list(six.itervalues(ret)) != [0, 0]:
+            errdct = {'volume': self.volume, 'command': commandstr,
+                      'opErrstr': volxml_get(xmlout, 'opErrstr', None)}
+            errdct.update(ret)
+            raise exception.InvalidShare(_(
+                'GlusterFS command %(command)s on volume %(volume)s got '
+                'unexpected response: '
+                'opRet=%(opRet)s, opErrno=%(opErrno)s, opErrstr=%(opErrstr)s'
+            ) % errdct)
+        if not countpath:
+            return
+        count = volxml_get(xmlout, countpath)
+        if count != '1':
+            raise exception.InvalidShare(
+                _('GlusterFS command %(command)s on volume %(volume)s got '
+                  'ambiguous response: '
+                  '%(count)s records') % {
+                    'volume': self.volume, 'command': commandstr,
+                    'count': count})
+
+    def _get_vol_option_via_info(self, option):
+        """Get the value of an option set on a GlusterFS volume via volinfo."""
         args = ('--xml', 'volume', 'info', self.volume)
         out, err = self.gluster_call(*args, log=_LE("retrieving volume info"))
 
@@ -186,19 +233,65 @@ class GlusterManager(object):
                 self.volume
             )
 
-        vix = etree.fromstring(out)
-        if int(vix.find('./volInfo/volumes/count').text) != 1:
-            raise exception.InvalidShare('Volume name ambiguity')
-        for e in vix.findall(".//option"):
-            o, v = (e.find(a).text for a in ('name', 'value'))
+        volxml = etree.fromstring(out)
+        self.xml_response_check(volxml, args[1:], './volInfo/volumes/count')
+        for e in volxml.findall(".//option"):
+            o, v = (volxml_get(e, a) for a in ('name', 'value'))
             if o == option:
                 return v
 
+    @_check_volume_presence
+    def _get_vol_user_option(self, useropt):
+        """Get the value of an user option set on a GlusterFS volume."""
+        option = '.'.join(('user', useropt))
+        return self._get_vol_option_via_info(option)
+
+    @_check_volume_presence
+    def _get_vol_regular_option(self, option):
+        """Get the value of a regular option set on a GlusterFS volume."""
+        args = ('--xml', 'volume', 'get', self.volume, option)
+
+        out, err = self.gluster_call(*args, check_exit_code=False)
+
+        if not out:
+            # all input is valid, but the option has not been set
+            # (nb. some options do come by a null value, but some
+            # don't even have that, see eg. cluster.nufa)
+            return
+
+        try:
+            optxml = etree.fromstring(out)
+        except Exception:
+            # non-xml output indicates that GlusterFS backend does not support
+            # 'vol get', we fall back to 'vol info' based retrieval (glusterfs
+            # < 3.7).
+            return self._get_vol_option_via_info(option)
+
+        self.xml_response_check(optxml, args[1:], './volGetopts/count')
+        return volxml_get(optxml, './volGetopts/Value')
+
+    def get_vol_option(self, option, boolean=False):
+        """Get the value of an option set on a GlusterFS volume."""
+        useropt = re.sub('\Auser\.', '', option)
+        if option == useropt:
+            value = self._get_vol_regular_option(option)
+        else:
+            value = self._get_vol_user_option(useropt)
+        if not boolean or value is None:
+            return value
+        if value.upper() in self.GLUSTERFS_TRUE_VALUES:
+            return True
+        if value.upper() in self.GLUSTERFS_FALSE_VALUES:
+            return False
+        raise exception.GlusterfsException(_(
+            "GlusterFS volume option on volume %(volume)s: "
+            "%(option)s=%(value)s cannot be interpreted as Boolean") % {
+                'volume': self.volume, 'option': option, 'value': value})
+
     @_check_volume_presence
     def set_vol_option(self, option, value, ignore_failure=False):
-        if value is True:
-            value
-        value = {True: 'on', False: 'off'}.get(value, value)
+        value = {True: self.GLUSTERFS_TRUE_VALUES[0],
+                 False: self.GLUSTERFS_FALSE_VALUES[0]}.get(value, value)
         if value is None:
             args = ('reset', (option,))
         else:

manila/share/drivers/glusterfs/layout_volume.py

@@ -531,9 +531,9 @@ class GlusterfsVolumeMappedLayout(layout.GlusterfsShareLayoutBase):
                 )
 
             outxml = etree.fromstring(out)
-            opret = int(outxml.find('opRet').text)
-            operrno = int(outxml.find('opErrno').text)
-            operrstr = outxml.find('opErrstr').text
+            opret = int(common.volxml_get(outxml, 'opRet'))
+            operrno = int(common.volxml_get(outxml, 'opErrno'))
+            operrstr = common.volxml_get(outxml, 'opErrstr', None)
 
         if opret == -1:
             vers = self.glusterfs_versions[gluster_mgr.host_access]
@@ -573,18 +573,7 @@ class GlusterfsVolumeMappedLayout(layout.GlusterfsShareLayoutBase):
             )
 
         outxml = etree.fromstring(out)
-        opret = int(outxml.find('opRet').text)
-        operrno = int(outxml.find('opErrno').text)
-        operrstr = outxml.find('opErrstr').text
-
-        if opret:
-            raise exception.GlusterfsException(
-                _("Deleting snapshot %(snap_id)s of share %(share_id)s failed "
-                  "with %(errno)d: %(errstr)s") % {
-                      'snap_id': snapshot['id'],
-                      'share_id': snapshot['share_id'],
-                      'errno': operrno,
-                      'errstr': operrstr})
+        gluster_mgr.xml_response_check(outxml, args[1:])
 
     def ensure_share(self, context, share, share_server=None):
         """Invoked to ensure that share is exported."""

manila/share/drivers/glusterfs_native.py

@@ -203,9 +203,8 @@ class GlusterfsNativeShareDriver(driver.ExecuteMixin,
         ssl_allow_opt = ','.join(ssl_allow)
         gluster_mgr.set_vol_option(AUTH_SSL_ALLOW, ssl_allow_opt)
 
-        dynauth = gluster_mgr.get_vol_option(DYNAMIC_AUTH) or 'off'
-        # TODO(csaba): boolean option processing shoud be done in common
-        if dynauth.lower() not in ('on', '1', 'true', 'yes', 'enable'):
+        dynauth = gluster_mgr.get_vol_option(DYNAMIC_AUTH, boolean=True)
+        if not dynauth:
             common._restart_gluster_vol(gluster_mgr)
 
     def _update_share_stats(self):

manila/tests/share/drivers/glusterfs/test_common.py

@@ -73,6 +73,32 @@ class GlusterManagerTestCase(test.TestCase):
             exception.GlusterfsException,
             common._check_volume_presence(mock.Mock()), gmgr)
 
+    def test_volxml_get(self):
+        xmlout = mock.Mock()
+        value = mock.Mock()
+        value.text = 'foobar'
+        xmlout.find = mock.Mock(return_value=value)
+
+        ret = common.volxml_get(xmlout, 'some/path')
+
+        self.assertEqual('foobar', ret)
+
+    @ddt.data(None, 'some-value')
+    def test_volxml_get_notfound_fallback(self, default):
+        xmlout = mock.Mock()
+        xmlout.find = mock.Mock(return_value=None)
+
+        ret = common.volxml_get(xmlout, 'some/path', default)
+
+        self.assertEqual(default, ret)
+
+    def test_volxml_get_notfound(self):
+        xmlout = mock.Mock()
+        xmlout.find = mock.Mock(return_value=None)
+
+        self.assertRaises(exception.InvalidShare, common.volxml_get,
+                          xmlout, 'some/path')
+
     def test_gluster_manager_init(self):
         self.assertEqual(fake_gluster_manager_attrs['user'],
                          self._gluster_manager.user)
@@ -222,21 +248,104 @@ class GlusterManagerTestCase(test.TestCase):
                               gluster_manager.make_gluster_call(fake_execute),
                               *fake_args, error_policy='foobar')
 
-    def test_get_vol_option_empty_volinfo(self):
+    @ddt.data({}, {'opErrstr': None}, {'opErrstr': 'error'})
+    def test_xml_response_check(self, xdict):
+        fdict = {'opRet': '0', 'opErrno': '0', 'some/count': '1'}
+        fdict.update(xdict)
+
+        def vxget(x, e, *a):
+            if a:
+                return fdict.get(e, a[0])
+            else:
+                return fdict[e]
+
+        xtree = mock.Mock()
+        command = ['volume', 'command', 'fake']
+
+        with mock.patch.object(common, 'volxml_get', side_effect=vxget):
+            self._gluster_manager.xml_response_check(xtree, command,
+                                                     'some/count')
+
+            self.assertTrue(common.volxml_get.called)
+
+    @ddt.data('1', '2')
+    def test_xml_response_check_failure(self, count):
+        fdict = {'opRet': '-1', 'opErrno': '0', 'some/count': count}
+
+        def vxget(x, e, *a):
+            if a:
+                return fdict.get(e, a[0])
+            else:
+                return fdict[e]
+
+        xtree = mock.Mock()
+        command = ['volume', 'command', 'fake']
+
+        with mock.patch.object(common, 'volxml_get', side_effect=vxget):
+            self.assertRaises(exception.GlusterfsException,
+                              self._gluster_manager.xml_response_check,
+                              xtree, command, 'some/count')
+
+            self.assertTrue(common.volxml_get.called)
+
+    @ddt.data({'opRet': '-2', 'opErrno': '0', 'some/count': '1'},
+              {'opRet': '0', 'opErrno': '1', 'some/count': '1'},
+              {'opRet': '0', 'opErrno': '0', 'some/count': '0'},
+              {'opRet': '0', 'opErrno': '0', 'some/count': '2'})
+    def test_xml_response_check_invalid(self, fdict):
+
+        def vxget(x, e, *a):
+            if a:
+                return fdict.get(e, a[0])
+            else:
+                return fdict[e]
+
+        xtree = mock.Mock()
+        command = ['volume', 'command', 'fake']
+
+        with mock.patch.object(common, 'volxml_get', side_effect=vxget):
+            self.assertRaises(exception.InvalidShare,
+                              self._gluster_manager.xml_response_check,
+                              xtree, command, 'some/count')
+
+            self.assertTrue(common.volxml_get.called)
+
+    @ddt.data({'opRet': '0', 'opErrno': '0'},
+              {'opRet': '0', 'opErrno': '0', 'some/count': '2'})
+    def test_xml_response_check_count_ignored(self, fdict):
+
+        def vxget(x, e, *a):
+            if a:
+                return fdict.get(e, a[0])
+            else:
+                return fdict[e]
+
+        xtree = mock.Mock()
+        command = ['volume', 'command', 'fake']
+
+        with mock.patch.object(common, 'volxml_get', side_effect=vxget):
+            self._gluster_manager.xml_response_check(xtree, command)
+
+            self.assertTrue(common.volxml_get.called)
+
+    def test_get_vol_option_via_info_empty_volinfo(self):
         args = ('--xml', 'volume', 'info', self._gluster_manager.volume)
         self.mock_object(self._gluster_manager, 'gluster_call',
                          mock.Mock(return_value=('', {})))
         self.assertRaises(exception.GlusterfsException,
-                          self._gluster_manager.get_vol_option,
-                          NFS_EXPORT_DIR)
+                          self._gluster_manager._get_vol_option_via_info,
+                          'foobar')
         self._gluster_manager.gluster_call.assert_called_once_with(
             *args, log=mock.ANY)
 
-    def test_get_vol_option_ambiguous_volinfo(self):
+    def test_get_vol_option_via_info_ambiguous_volinfo(self):
 
         def xml_output(*ignore_args, **ignore_kwargs):
             return """<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
 <cliOutput>
+  <opRet>0</opRet>
+  <opErrno>0</opErrno>
+  <opErrstr/>
   <volInfo>
     <volumes>
       <count>0</count>
@@ -248,16 +357,19 @@ class GlusterManagerTestCase(test.TestCase):
         self.mock_object(self._gluster_manager, 'gluster_call',
                          mock.Mock(side_effect=xml_output))
         self.assertRaises(exception.InvalidShare,
-                          self._gluster_manager.get_vol_option,
-                          NFS_EXPORT_DIR)
+                          self._gluster_manager._get_vol_option_via_info,
+                          'foobar')
         self._gluster_manager.gluster_call.assert_called_once_with(
             *args, log=mock.ANY)
 
-    def test_get_vol_option_trivial_volinfo(self):
+    def test_get_vol_option_via_info_trivial_volinfo(self):
 
         def xml_output(*ignore_args, **ignore_kwargs):
             return """<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
 <cliOutput>
+  <opRet>0</opRet>
+  <opErrno>0</opErrno>
+  <opErrstr/>
   <volInfo>
     <volumes>
       <volume>
@@ -270,23 +382,26 @@ class GlusterManagerTestCase(test.TestCase):
         args = ('--xml', 'volume', 'info', self._gluster_manager.volume)
         self.mock_object(self._gluster_manager, 'gluster_call',
                          mock.Mock(side_effect=xml_output))
-        ret = self._gluster_manager.get_vol_option(NFS_EXPORT_DIR)
+        ret = self._gluster_manager._get_vol_option_via_info('foobar')
         self.assertIsNone(ret)
         self._gluster_manager.gluster_call.assert_called_once_with(
             *args, log=mock.ANY)
 
-    def test_get_vol_option(self):
+    def test_get_vol_option_via_info(self):
 
         def xml_output(*ignore_args, **ignore_kwargs):
             return """<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
 <cliOutput>
+  <opRet>0</opRet>
+  <opErrno>0</opErrno>
+  <opErrstr/>
   <volInfo>
     <volumes>
       <volume>
         <options>
            <option>
-              <name>nfs.export-dir</name>
-              <value>/foo(10.0.0.1|10.0.0.2),/bar(10.0.0.1)</value>
+              <name>foobar</name>
+              <value>FIRE MONKEY!</value>
            </option>
         </options>
       </volume>
@@ -298,15 +413,158 @@ class GlusterManagerTestCase(test.TestCase):
         args = ('--xml', 'volume', 'info', self._gluster_manager.volume)
         self.mock_object(self._gluster_manager, 'gluster_call',
                          mock.Mock(side_effect=xml_output))
-        ret = self._gluster_manager.get_vol_option(NFS_EXPORT_DIR)
-        self.assertEqual('/foo(10.0.0.1|10.0.0.2),/bar(10.0.0.1)', ret)
+        ret = self._gluster_manager._get_vol_option_via_info('foobar')
+        self.assertEqual('FIRE MONKEY!', ret)
         self._gluster_manager.gluster_call.assert_called_once_with(
             *args, log=mock.ANY)
 
+    def test_get_vol_user_option(self):
+        self.mock_object(self._gluster_manager, '_get_vol_option_via_info',
+                         mock.Mock(return_value='VALUE'))
+
+        ret = self._gluster_manager._get_vol_user_option('OPT')
+
+        self.assertEqual(ret, 'VALUE')
+        (self._gluster_manager._get_vol_option_via_info.
+         assert_called_once_with('user.OPT'))
+
+    def test_get_vol_regular_option_empty_reponse(self):
+        args = ('--xml', 'volume', 'get', self._gluster_manager.volume,
+                NFS_EXPORT_DIR)
+        self.mock_object(self._gluster_manager, 'gluster_call',
+                         mock.Mock(return_value=('', {})))
+
+        ret = self._gluster_manager._get_vol_regular_option(NFS_EXPORT_DIR)
+
+        self.assertIsNone(ret)
+        self._gluster_manager.gluster_call.assert_called_once_with(
+            *args, check_exit_code=False)
+
+    @ddt.data(0, 2)
+    def test_get_vol_regular_option_ambiguous_volinfo(self, count):
+
+        def xml_output(*ignore_args, **ignore_kwargs):
+            return """<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<cliOutput>
+  <opRet>0</opRet>
+  <opErrno>0</opErrno>
+  <opErrstr/>
+  <volGetopts>
+    <count>%d</count>
+  </volGetopts>
+</cliOutput>""" % count, ''
+
+        args = ('--xml', 'volume', 'get', self._gluster_manager.volume,
+                NFS_EXPORT_DIR)
+        self.mock_object(self._gluster_manager, 'gluster_call',
+                         mock.Mock(side_effect=xml_output))
+
+        self.assertRaises(exception.InvalidShare,
+                          self._gluster_manager._get_vol_regular_option,
+                          NFS_EXPORT_DIR)
+
+        self._gluster_manager.gluster_call.assert_called_once_with(
+            *args, check_exit_code=False)
+
+    def test_get_vol_regular_option(self):
+
+        def xml_output(*ignore_args, **ignore_kwargs):
+            return """<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<cliOutput>
+  <opRet>0</opRet>
+  <opErrno>0</opErrno>
+  <opErrstr/>
+  <volGetopts>
+    <count>1</count>
+    <Option>nfs.export-dir</Option>
+    <Value>/foo(10.0.0.1|10.0.0.2),/bar(10.0.0.1)</Value>
+  </volGetopts>
+</cliOutput>""", ''
+
+        args = ('--xml', 'volume', 'get', self._gluster_manager.volume,
+                NFS_EXPORT_DIR)
+        self.mock_object(self._gluster_manager, 'gluster_call',
+                         mock.Mock(side_effect=xml_output))
+
+        ret = self._gluster_manager._get_vol_regular_option(NFS_EXPORT_DIR)
+
+        self.assertEqual('/foo(10.0.0.1|10.0.0.2),/bar(10.0.0.1)', ret)
+        self._gluster_manager.gluster_call.assert_called_once_with(
+            *args, check_exit_code=False)
+
+    def test_get_vol_regular_option_not_suppored(self):
+        args = ('--xml', 'volume', 'get', self._gluster_manager.volume,
+                NFS_EXPORT_DIR)
+        self.mock_object(self._gluster_manager, 'gluster_call',
+                         mock.Mock(return_value=(
+                                   """Ceci n'est pas un XML.""", '')))
+        self.mock_object(self._gluster_manager, '_get_vol_option_via_info',
+                         mock.Mock(return_value="VALUE"))
+
+        ret = self._gluster_manager._get_vol_regular_option(NFS_EXPORT_DIR)
+
+        self.assertEqual("VALUE", ret)
+        self._gluster_manager.gluster_call.assert_called_once_with(
+            *args, check_exit_code=False)
+        (self._gluster_manager._get_vol_option_via_info.
+         assert_called_once_with(NFS_EXPORT_DIR))
+
+    @ddt.data({'opt': 'some.option', 'opttype': 'regular',
+               'lowopt': 'some.option'},
+              {'opt': 'user.param', 'opttype': 'user', 'lowopt': 'param'})
+    @ddt.unpack
+    def test_get_vol_option(self, opt, opttype, lowopt):
+        for t in ('user', 'regular'):
+            self.mock_object(self._gluster_manager, '_get_vol_%s_option' % t,
+                             mock.Mock(return_value='value-%s' % t))
+
+        ret = self._gluster_manager.get_vol_option(opt)
+
+        self.assertEqual('value-%s' % opttype, ret)
+        for t in ('user', 'regular'):
+            func = getattr(self._gluster_manager, '_get_vol_%s_option' % t)
+            if opttype == t:
+                func.assert_called_once_with(lowopt)
+            else:
+                self.assertFalse(func.called)
+
+    def test_get_vol_option_unset(self):
+        self.mock_object(self._gluster_manager, '_get_vol_regular_option',
+                         mock.Mock(return_value=None))
+
+        ret = self._gluster_manager.get_vol_option('some.option')
+
+        self.assertIsNone(ret)
+
+    @ddt.data({'value': '0', 'boolval': False},
+              {'value': 'Off', 'boolval': False},
+              {'value': 'no', 'boolval': False},
+              {'value': '1', 'boolval': True},
+              {'value': 'true', 'boolval': True},
+              {'value': 'enAble', 'boolval': True},
+              {'value': None, 'boolval': None})
+    @ddt.unpack
+    def test_get_vol_option_boolean(self, value, boolval):
+        self.mock_object(self._gluster_manager, '_get_vol_regular_option',
+                         mock.Mock(return_value=value))
+
+        ret = self._gluster_manager.get_vol_option('some.option',
+                                                   boolean=True)
+
+        self.assertEqual(boolval, ret)
+
+    def test_get_vol_option_boolean_bad(self):
+        self.mock_object(self._gluster_manager, '_get_vol_regular_option',
+                         mock.Mock(return_value='jabberwocky'))
+
+        self.assertRaises(exception.GlusterfsException,
+                          self._gluster_manager.get_vol_option,
+                          'some.option', boolean=True)
+
     @ddt.data({'setting': 'some_value', 'args': ('set', 'some_value')},
               {'setting': None, 'args': ('reset',)},
-              {'setting': True, 'args': ('set', 'on')},
-              {'setting': False, 'args': ('set', 'off')})
+              {'setting': True, 'args': ('set', 'ON')},
+              {'setting': False, 'args': ('set', 'OFF')})
     @ddt.unpack
     def test_set_vol_option(self, setting, args):
         self.mock_object(self._gluster_manager, 'gluster_call', mock.Mock())

manila/tests/share/drivers/glusterfs/test_layout_volume.py

@@ -913,7 +913,7 @@ class GlusterfsVolumeMappedLayoutTestCase(test.TestCase):
         (self._layout._find_actual_backend_snapshot_name.
             assert_called_once_with(gmgr1, snapshot))
 
-    @ddt.data({'side_effect': (glusterXMLOut(ret=-1, errno=2),),
+    @ddt.data({'side_effect': (glusterXMLOut(ret=-1, errno=0),),
                '_exception': exception.GlusterfsException},
               {'side_effect': (('', ''),),
                '_exception': exception.GlusterfsException})

manila/tests/share/drivers/test_glusterfs.py

@@ -116,31 +116,31 @@ class GlusterfsShareDriverTestCase(test.TestCase):
             helpercls = mock.Mock(return_value=helper)
         self._driver.nfs_helper = helpercls
         if helpercls == glusterfs.GlusterNFSHelper and path is None:
-            gmgr.get_vol_option = mock.Mock(return_value='on')
+            gmgr.get_vol_option = mock.Mock(return_value=True)
 
         self._driver._setup_via_manager(
             {'manager': gmgr, 'share': self.share})
 
         if helpercls == glusterfs.GlusterNFSHelper and path is None:
             gmgr.get_vol_option.assert_called_once_with(
-                NFS_EXPORT_VOL)
+                NFS_EXPORT_VOL, boolean=True)
             args = (NFS_RPC_AUTH_REJECT, '*')
         else:
             args = (NFS_EXPORT_VOL, False)
         gmgr.set_vol_option.assert_called_once_with(*args)
 
-    @ddt.data('off', 'no', '0', 'false', 'disable', 'foobarbaz')
-    def test_setup_via_manager_export_volumes_on(self, export_vol):
+    def test_setup_via_manager_export_volumes_off(self):
         gmgr = mock.Mock()
         gmgr.path = None
-        gmgr.get_vol_option = mock.Mock(return_value=export_vol)
+        gmgr.get_vol_option = mock.Mock(return_value=False)
         self._driver.nfs_helper = glusterfs.GlusterNFSHelper
 
         self.assertRaises(exception.GlusterfsException,
                           self._driver._setup_via_manager,
                           {'manager': gmgr, 'share': self.share})
 
-        gmgr.get_vol_option.assert_called_once_with(NFS_EXPORT_VOL)
+        gmgr.get_vol_option.assert_called_once_with(NFS_EXPORT_VOL,
+                                                    boolean=True)
 
     def test_check_for_setup_error(self):
         self._driver.check_for_setup_error()

manila/tests/share/drivers/test_glusterfs_native.py

@@ -227,18 +227,17 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase):
 
         self.assertEqual(expected_exec, fake_utils.fake_execute_get_log())
 
-    @ddt.data('on', '1', 'Yes', 'TRUE', 'enable')
-    def test_deny_access_via_manager(self, trueish):
+    def test_deny_access_via_manager(self):
         self.mock_object(common, '_restart_gluster_vol', mock.Mock())
         access = {'access_type': 'cert', 'access_to': 'client.example.com'}
         gmgr1 = common.GlusterManager(self.glusterfs_target1, self._execute,
                                       None, None)
 
-        def _get_vol_option(opt):
+        def _get_vol_option(opt, **kw):
             if opt == 'auth.ssl-allow':
                 return('some.common.name,' + access['access_to'])
             elif opt == 'server.dynamic-auth':
-                return trueish
+                return True
 
         self.mock_object(
             gmgr1, 'get_vol_option',
@@ -250,23 +249,24 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase):
                                               self.share1, access)
 
         gmgr1.get_vol_option.assert_has_calls(
-            [mock.call(a) for a in ('auth.ssl-allow', 'server.dynamic-auth')])
+            [mock.call(a, **kw) for a, kw in (
+                ('auth.ssl-allow', {}),
+                ('server.dynamic-auth', {'boolean': True}))])
         test_args = ('auth.ssl-allow', 'some.common.name')
         gmgr1.set_vol_option.assert_called_once_with(*test_args)
         self.assertFalse(common._restart_gluster_vol.called)
 
-    @ddt.data('off', None, 'strangelove')
-    def test_deny_access_via_manager_no_dyn_auth(self, falseish):
+    def test_deny_access_via_manager_no_dyn_auth(self):
         self.mock_object(common, '_restart_gluster_vol', mock.Mock())
         access = {'access_type': 'cert', 'access_to': 'client.example.com'}
         gmgr1 = common.GlusterManager(self.glusterfs_target1, self._execute,
                                       None, None)
 
-        def _get_vol_option(opt):
+        def _get_vol_option(opt, **kw):
             if opt == 'auth.ssl-allow':
                 return('some.common.name,' + access['access_to'])
             elif opt == 'server.dynamic-auth':
-                return falseish
+                return False
 
         self.mock_object(
             gmgr1, 'get_vol_option',
@@ -278,7 +278,9 @@ class GlusterfsNativeShareDriverTestCase(test.TestCase):
                                               self.share1, access)
 
         gmgr1.get_vol_option.assert_has_calls(
-            [mock.call(a) for a in ('auth.ssl-allow', 'server.dynamic-auth')])
+            [mock.call(a, **kw) for a, kw in (
+                ('auth.ssl-allow', {}),
+                ('server.dynamic-auth', {'boolean': True}))])
         test_args = ('auth.ssl-allow', 'some.common.name')
         gmgr1.set_vol_option.assert_called_once_with(*test_args)
         common._restart_gluster_vol.assert_called_once_with(gmgr1)