Removes getfattr from Quobyte Nova driver

Replaces reading quobyte.info xarg with a range
of checks on a given mountpoint. Adds several
unit tests to verify the different check results.
Adds --disable-xattrs option to mount calls and
a release notes file.

Closes-Bug: 1659328

Change-Id: I1c07e513af14b769c257c72e51132f81e61673c7

nova/tests/unit/virt/libvirt/volume/test_quobyte.py

@@ -15,12 +15,15 @@
 """Unit tests for the Quobyte volume driver module."""
 
 import os
+import traceback
 
 import mock
 from oslo_concurrency import processutils
 from oslo_utils import fileutils
+import psutil
+import six
 
-from nova import exception
+from nova import exception as nova_exception
 from nova import test
 from nova.tests.unit.virt.libvirt.volume import test_volume
 from nova import utils
@@ -31,6 +34,31 @@ from nova.virt.libvirt.volume import quobyte
 class QuobyteTestCase(test.NoDBTestCase):
     """Tests the nova.virt.libvirt.volume.quobyte module utilities."""
 
+    TEST_MNT_POINT = mock.sentinel.TEST_MNT_POINT
+
+    def assertRaisesAndMessageMatches(
+            self, excClass, msg, callableObj, *args, **kwargs):
+        """Ensure that the specified exception was raised. """
+
+        caught = False
+        try:
+            callableObj(*args, **kwargs)
+        except Exception as exc:
+            caught = True
+            self.assertIsInstance(exc, excClass,
+                                  'Wrong exception caught: %s Stacktrace: %s' %
+                                  (exc, traceback.format_exc()))
+            self.assertIn(msg, six.text_type(exc))
+
+        if not caught:
+            self.fail('Expected raised exception but nothing caught.')
+
+    def get_mock_partitions(self):
+        mypart = mock.Mock()
+        mypart.device = "quobyte@"
+        mypart.mountpoint = self.TEST_MNT_POINT
+        return [mypart]
+
     @mock.patch.object(os.path, "exists", return_value=False)
     @mock.patch.object(fileutils, "ensure_tree")
     @mock.patch.object(utils, "execute")
@@ -46,6 +74,7 @@ class QuobyteTestCase(test.NoDBTestCase):
 
         mock_ensure_tree.assert_called_once_with(export_mnt_base)
         expected_commands = [mock.call('mount.quobyte',
+                                       '--disable-xattrs',
                                        quobyte_volume,
                                        export_mnt_base)
                              ]
@@ -70,6 +99,7 @@ class QuobyteTestCase(test.NoDBTestCase):
                                        '--scope',
                                        '--user',
                                        'mount.quobyte',
+                                       '--disable-xattrs',
                                        quobyte_volume,
                                        export_mnt_base)
                              ]
@@ -95,6 +125,7 @@ class QuobyteTestCase(test.NoDBTestCase):
 
         mock_ensure_tree.assert_called_once_with(export_mnt_base)
         expected_commands = [mock.call('mount.quobyte',
+                                       '--disable-xattrs',
                                        quobyte_volume,
                                        export_mnt_base,
                                        '-c',
@@ -170,49 +201,91 @@ class QuobyteTestCase(test.NoDBTestCase):
                                  "the Quobyte Volume at %s",
                                  export_mnt_base))
 
-    @mock.patch.object(os, "access", return_value=True)
+    @mock.patch.object(psutil, "disk_partitions")
-    @mock.patch.object(utils, "execute")
+    @mock.patch.object(os, "stat")
-    def test_quobyte_is_valid_volume(self, mock_execute, mock_access):
+    def test_validate_volume_all_good(self, stat_mock, part_mock):
-        mnt_base = '/mnt'
+        part_mock.return_value = self.get_mock_partitions()
-        quobyte_volume = '192.168.1.1/volume-00001'
+        drv = quobyte
-        export_mnt_base = os.path.join(mnt_base,
-                                       utils.get_hash_str(quobyte_volume))
 
-        quobyte.validate_volume(export_mnt_base)
+        def statMockCall(*args):
+            if args[0] == self.TEST_MNT_POINT:
+                stat_result = mock.Mock()
+                stat_result.st_size = 0
+                return stat_result
+            return os.stat(args)
+        stat_mock.side_effect = statMockCall
 
-        mock_execute.assert_called_once_with('getfattr',
+        drv.validate_volume(self.TEST_MNT_POINT)
-                                             '-n',
-                                             'quobyte.info',
-                                             export_mnt_base)
 
-    @mock.patch.object(utils, "execute",
+        stat_mock.assert_called_once_with(self.TEST_MNT_POINT)
-                       side_effect=(processutils.
+        part_mock.assert_called_once_with(all=True)
-                                    ProcessExecutionError))
-    def test_quobyte_is_valid_volume_vol_not_valid_volume(self, mock_execute):
-        mnt_base = '/mnt'
-        quobyte_volume = '192.168.1.1/volume-00001'
-        export_mnt_base = os.path.join(mnt_base,
-                                       utils.get_hash_str(quobyte_volume))
 
-        self.assertRaises(exception.NovaException,
+    @mock.patch.object(psutil, "disk_partitions")
+    @mock.patch.object(os, "stat")
+    def test_validate_volume_mount_not_working(self, stat_mock, part_mock):
+        part_mock.return_value = self.get_mock_partitions()
+        drv = quobyte
+
+        def statMockCall(*args):
+            print (args)
+            if args[0] == self.TEST_MNT_POINT:
+                raise nova_exception.InvalidVolume()
+        stat_mock.side_effect = [os.stat, statMockCall]
+
+        self.assertRaises(
+            excClass=nova_exception.InvalidVolume,
+            callableObj=drv.validate_volume,
+            mount_path=self.TEST_MNT_POINT)
+        stat_mock.assert_called_with(self.TEST_MNT_POINT)
+        part_mock.assert_called_once_with(all=True)
+
+    def test_validate_volume_no_mtab_entry(self):
+        msg = ("No matching Quobyte mount entry for %(mpt)s"
+               " could be found for validation in partition list."
+               % {'mpt': self.TEST_MNT_POINT})
+
+        self.assertRaisesAndMessageMatches(
+            nova_exception.InvalidVolume,
+            msg,
             quobyte.validate_volume,
-                          export_mnt_base)
+            self.TEST_MNT_POINT)
 
-    @mock.patch.object(os, "access", return_value=False)
+    @mock.patch.object(psutil, "disk_partitions")
-    @mock.patch.object(utils, "execute",
+    def test_validate_volume_wrong_mount_type(self, part_mock):
-                       side_effect=(processutils.
+        mypart = mock.Mock()
-                                    ProcessExecutionError))
+        mypart.device = "not-quobyte"
-    def test_quobyte_is_valid_volume_vol_no_valid_access(self,
+        mypart.mountpoint = self.TEST_MNT_POINT
-                                                         mock_execute,
+        part_mock.return_value = [mypart]
-                                                         mock_access):
+        msg = ("The mount %(mpt)s is not a valid"
-        mnt_base = '/mnt'
+               " Quobyte volume according to partition list."
-        quobyte_volume = '192.168.1.1/volume-00001'
+               % {'mpt': self.TEST_MNT_POINT})
-        export_mnt_base = os.path.join(mnt_base,
-                                       utils.get_hash_str(quobyte_volume))
 
-        self.assertRaises(exception.NovaException,
+        self.assertRaisesAndMessageMatches(
+            nova_exception.InvalidVolume,
+            msg,
             quobyte.validate_volume,
-                          export_mnt_base)
+            self.TEST_MNT_POINT)
+        part_mock.assert_called_once_with(all=True)
+
+    @mock.patch.object(os, "stat")
+    @mock.patch.object(psutil, "disk_partitions")
+    def test_validate_volume_stale_mount(self, part_mock, stat_mock):
+        part_mock.return_value = self.get_mock_partitions()
+
+        def statMockCall(*args):
+            if args[0] == self.TEST_MNT_POINT:
+                stat_result = mock.Mock()
+                stat_result.st_size = 1
+                return stat_result
+            return os.stat(args)
+        stat_mock.side_effect = statMockCall
+
+        # As this uses a dir size >0, it raises an exception
+        self.assertRaises(
+            nova_exception.InvalidVolume,
+            quobyte.validate_volume,
+            self.TEST_MNT_POINT)
+        part_mock.assert_called_once_with(all=True)
 
 
 class LibvirtQuobyteVolumeDriverTestCase(
@@ -368,12 +441,12 @@ class LibvirtQuobyteVolumeDriverTestCase(
 
         def exe_side_effect(*cmd, **kwargs):
             if cmd == mock.ANY:
-                raise exception.NovaException()
+                raise nova_exception.NovaException()
 
         with mock.patch.object(quobyte,
                                'validate_volume') as mock_execute:
             mock_execute.side_effect = exe_side_effect
-            self.assertRaises(exception.NovaException,
+            self.assertRaises(nova_exception.NovaException,
                               libvirt_driver.connect_volume,
                               connection_info,
                               self.disk_info,

nova/virt/libvirt/volume/quobyte.py

@@ -19,6 +19,7 @@ import os
 from oslo_concurrency import processutils
 from oslo_log import log as logging
 from oslo_utils import fileutils
+import psutil
 import six
 
 import nova.conf
@@ -42,12 +43,15 @@ def mount_volume(volume, mnt_base, configfile=None):
     """Wraps execute calls for mounting a Quobyte volume"""
     fileutils.ensure_tree(mnt_base)
 
-    command = ['mount.quobyte', volume, mnt_base]
+    # NOTE(kaisers): disable xattrs to speed up io as this omits
+    # additional metadata requests in the backend. xattrs can be
+    # enabled without issues but will reduce performance.
+    command = ['mount.quobyte', '--disable-xattrs', volume, mnt_base]
     if os.path.exists(" /run/systemd/system"):
         # Note(kaisers): with systemd this requires a separate CGROUP to
         # prevent Nova service stop/restarts from killing the mount.
-        command = ['systemd-run', '--scope', '--user', 'mount.quobyte', volume,
+        command = ['systemd-run', '--scope', '--user', 'mount.quobyte',
-                   mnt_base]
+                   '--disable-xattrs', volume, mnt_base]
     if configfile:
         command.extend(['-c', configfile])
 
@@ -70,21 +74,32 @@ def umount_volume(mnt_base):
                           mnt_base)
 
 
-def validate_volume(mnt_base):
+def validate_volume(mount_path):
-    """Wraps execute calls for checking validity of a Quobyte volume"""
+    """Runs a number of tests to be sure this is a (working) Quobyte mount"""
-    command = ['getfattr', "-n", "quobyte.info", mnt_base]
+    partitions = psutil.disk_partitions(all=True)
-    try:
+    for p in partitions:
-        utils.execute(*command)
+        if mount_path != p.mountpoint:
-    except processutils.ProcessExecutionError as exc:
+            continue
+        if p.device.startswith("quobyte@"):
+            statresult = os.stat(mount_path)
+            # Note(kaisers): Quobyte always shows mount points with size 0
+            if statresult.st_size == 0:
+                # client looks healthy
+                return  # we're happy here
+            else:
+                msg = (_("The mount %(mount_path)s is not a "
+                         "valid Quobyte volume. Stale mount?")
+                       % {'mount_path': mount_path})
+            raise nova_exception.InvalidVolume(msg)
+        else:
             msg = (_("The mount %(mount_path)s is not a valid"
-                 " Quobyte volume. Error: %(exc)s")
+                     " Quobyte volume according to partition list.")
-               % {'mount_path': mnt_base, 'exc': exc})
+                   % {'mount_path': mount_path})
-        raise nova_exception.InternalError(msg)
+            raise nova_exception.InvalidVolume(msg)
-
+    msg = (_("No matching Quobyte mount entry for %(mount_path)s"
-    if not os.access(mnt_base, os.W_OK | os.X_OK):
+             " could be found for validation in partition list.")
-        msg = _("Volume is not writable. Please broaden the file"
+           % {'mount_path': mount_path})
-                " permissions. Mount: %s") % mnt_base
+    raise nova_exception.InvalidVolume(msg)
-        raise nova_exception.InternalError(msg)
 
 
 class LibvirtQuobyteVolumeDriver(fs.LibvirtBaseFileSystemVolumeDriver):

releasenotes/notes/bug_1659328-73686be497f5f85a.yaml

@@ -0,0 +1,6 @@
+---
+
+fixes:
+  - |
+    The i/o performance for Quobyte volumes has been increased significantly
+    by disabling xattrs.