Enable use of an /etc/cinder/lvm.conf file

During tempest and Rally runs we've noticed occasional
LVM command hangs (lvs, vgs and pvs), we've also gathered
enough data to show that we see very slow response times from
these commands almost all of the time.

It turns out that this seems to be an issue with us scanning
all devices during LVM operations, including devices that may
be Cinder Volumes that are attaching and detaching from the system.

Inspecting a run instrumented with strace shows a number of LVM
commands timing out due to the device being scanned being removed
during scan, and the LVM command in turn waiting until it times out
on the scan that's in process.

This patch just adds the ability to setup a lvm.conf file in
/etc/cinder.  The Cinder LVM code will now specifically set
the LVM_SYSTEM_DIR environment variable to that directory for
each of the LVM scan commands in brick/local_dev/lvm.py.
If the system doesn't have the file, we use the empty string
which tells LVM to use it's defaults.  This only affects LVM
commands in Cinder, the idea is to ensure we don't impact any
other LVM operations on the node outside of Cinder and that we
behave as we always have in the case of no lvm.conf file being
setup in /etc/cinder.  The presence of the file is auto-detected
on brick/localdev/lvm init.

We'll update the OpenStack Devstack deployment scripts to put this
together and fix things up there first. Until that's done and until
we auto-generate the conf (or document it well), this will be a
*partial* bugfix.

I considered adding a default lvm.conf file to cinder/etc/<sample>
that would be copied in on install, but decided against this to
avoid any possible issues with compatability issues between
platforms or versions.

To use, just copy the /etc/lvm/lvm.conf file to /etc/cinder and
modify the filter as appropriate, for example:
  To use loopback device only:
    filter = [ "a/loop/", "r/.*/" ]
  If you have a physical drive like /dev/sdb1
    filter = [ "a/dev/sdb1/", "r/.*/" ]

Finally, this patch also goes through and cleans up our cmd
variables in brick/localdev/lvm.  We had a mix of using a
cmd array, and strings; this causes inconsistencies and makes
it difficult to extend or modify commands.  Switch everything to
using an array and use extend to provide the correct prefix.

Need to update docs to include a recommendation to create an
/etc/cinder/lvm.conf file and set device filters appropriately.
Doc-Impact
Partial-Bug: #1373513

Change-Id: Ia2289197a6d7fcfc910ee3de48e0a2173881a1e6

diff --git a/cinder/brick/local_dev/lvm.py b/cinder/brick/local_dev/lvm.py
index 315c820687396fd53c9ca806a768336a1777c1d9..b0ceff71d0089237de394bdc268537fa60945c55 100644 (file)
--- a/cinder/brick/local_dev/lvm.py
+++ b/cinder/brick/local_dev/lvm.py
@@ -19,6 +19,7 @@ LVM class for performing LVM operations.
 
 import itertools
 import math
+import os
 import re
 import time
 
@@ -37,10 +38,11 @@ LOG = logging.getLogger(__name__)
 
 class LVM(executor.Executor):
     """LVM object to enable various LVM related operations."""
+    LVM_CMD_PREFIX = ['env', 'LC_ALL=C']
 
     def __init__(self, vg_name, root_helper, create_vg=False,
                  physical_volumes=None, lvm_type='default',
-                 executor=putils.execute):
+                 executor=putils.execute, lvm_conf=None):
 
         """Initialize the LVM object.
 
@@ -97,6 +99,10 @@ class LVM(executor.Executor):
 
             self.activate_lv(self.vg_thin_pool)
         self.pv_list = self.get_all_physical_volumes(root_helper, vg_name)
+        if lvm_conf and os.path.isfile(lvm_conf):
+            LVM.LVM_CMD_PREFIX = ['env',
+                                  'LC_ALL=C',
+                                  'LVM_SYSTEM_DIR=/etc/cinder']
 
     def _vg_exists(self):
         """Simple check to see if VG exists.
@@ -105,9 +111,11 @@ class LVM(executor.Executor):
 
         """
         exists = False
-        (out, _err) = self._execute(
-            'env', 'LC_ALL=C', 'vgs', '--noheadings', '-o', 'name',
-            self.vg_name, root_helper=self._root_helper, run_as_root=True)
+        cmd = LVM.LVM_CMD_PREFIX + ['vgs', '--noheadings',
+                                    '-o', 'name', self.vg_name]
+        (out, _err) = self._execute(*cmd,
+                                    root_helper=self._root_helper,
+                                    run_as_root=True)
 
         if out is not None:
             volume_groups = out.split()
@@ -121,8 +129,11 @@ class LVM(executor.Executor):
         self._execute(*cmd, root_helper=self._root_helper, run_as_root=True)
 
     def _get_vg_uuid(self):
-        (out, _err) = self._execute('env', 'LC_ALL=C', 'vgs', '--noheadings',
-                                    '-o uuid', self.vg_name)
+        cmd = LVM.LVM_CMD_PREFIX + ['vgs', '--noheadings',
+                                    '-o', 'uuid', self.vg_name]
+        (out, _err) = self._execute(*cmd,
+                                    root_helper=self._root_helper,
+                                    run_as_root=True)
         if out is not None:
             return out.split()
         else:
@@ -136,9 +147,10 @@ class LVM(executor.Executor):
         :returns: Free space in GB (float), calculated using data_percent
 
         """
-        cmd = ['env', 'LC_ALL=C', 'lvs', '--noheadings', '--unit=g',
-               '-o', 'size,data_percent', '--separator', ':', '--nosuffix']
-
+        cmd = LVM.LVM_CMD_PREFIX +\
+            ['lvs', '--noheadings', '--unit=g',
+             '-o', 'size,data_percent', '--separator',
+             ':', '--nosuffix']
         # NOTE(gfidente): data_percent only applies to some types of LV so we
         # make sure to append the actual thin pool name
         cmd.append("/dev/%s/%s" % (vg_name, thin_pool_name))
@@ -174,7 +186,7 @@ class LVM(executor.Executor):
 
         """
 
-        cmd = ['env', 'LC_ALL=C', 'vgs', '--version']
+        cmd = LVM.LVM_CMD_PREFIX + ['vgs', '--version']
         (out, _err) = putils.execute(*cmd,
                                      root_helper=root_helper,
                                      run_as_root=True)
@@ -247,9 +259,8 @@ class LVM(executor.Executor):
 
         """
 
-        cmd = ['env', 'LC_ALL=C', 'lvs', '--noheadings', '--unit=g',
-               '-o', 'vg_name,name,size', '--nosuffix']
-
+        cmd = LVM.LVM_CMD_PREFIX + ['lvs', '--noheadings', '--unit=g',
+                                    '-o', 'vg_name,name,size', '--nosuffix']
         if lv_name is not None and vg_name is not None:
             cmd.append("%s/%s" % (vg_name, lv_name))
         elif vg_name is not None:
@@ -314,13 +325,11 @@ class LVM(executor.Executor):
 
         """
         field_sep = '|'
-
-        cmd = ['env', 'LC_ALL=C', 'pvs', '--noheadings',
-               '--unit=g',
-               '-o', 'vg_name,name,size,free',
-               '--separator', field_sep,
-               '--nosuffix']
-
+        cmd = LVM.LVM_CMD_PREFIX + ['pvs', '--noheadings',
+                                    '--unit=g',
+                                    '-o', 'vg_name,name,size,free',
+                                    '--separator', field_sep,
+                                    '--nosuffix']
         (out, _err) = putils.execute(*cmd,
                                      root_helper=root_helper,
                                      run_as_root=True)
@@ -357,10 +366,11 @@ class LVM(executor.Executor):
         :returns: List of Dictionaries with VG info
 
         """
-        cmd = ['env', 'LC_ALL=C', 'vgs', '--noheadings', '--unit=g',
-               '-o', 'name,size,free,lv_count,uuid', '--separator', ':',
-               '--nosuffix']
-
+        cmd = LVM.LVM_CMD_PREFIX + ['vgs', '--noheadings',
+                                    '--unit=g', '-o',
+                                    'name,size,free,lv_count,uuid',
+                                    '--separator', ':',
+                                    '--nosuffix']
         if vg_name is not None:
             cmd.append(vg_name)
 
@@ -678,10 +688,11 @@ class LVM(executor.Executor):
                       run_as_root=True)
 
     def lv_has_snapshot(self, name):
-        out, _err = self._execute(
-            'env', 'LC_ALL=C', 'lvdisplay', '--noheading',
-            '-C', '-o', 'Attr', '%s/%s' % (self.vg_name, name),
-            root_helper=self._root_helper, run_as_root=True)
+        cmd = LVM.LVM_CMD_PREFIX + ['lvdisplay', '--noheading', '-C', '-o',
+                                    'Attr', '%s/%s' % (self.vg_name, name)]
+        out, _err = self._execute(*cmd,
+                                  root_helper=self._root_helper,
+                                  run_as_root=True)
         if out:
             out = out.strip()
             if (out[0] == 'o') or (out[0] == 'O'):

diff --git a/cinder/tests/brick/test_brick_lvm.py b/cinder/tests/brick/test_brick_lvm.py
index cd8ad48cb3fc5736b74ae551ac1004b29e0348b0..1e84032f768bc64e1f153d1c376ecc09581cd208 100644 (file)
--- a/cinder/tests/brick/test_brick_lvm.py
+++ b/cinder/tests/brick/test_brick_lvm.py
@@ -73,7 +73,7 @@ class BrickLvmTestCase(test.TestCase):
             data = "  fake-vg\n"
         elif 'env, LC_ALL=C, vgs, --version' in cmd_string:
             data = "  LVM version:     2.02.95(2) (2012-03-06)\n"
-        elif ('env, LC_ALL=C, vgs, --noheadings, -o uuid, fake-vg' in
+        elif ('env, LC_ALL=C, vgs, --noheadings, -o, uuid, fake-vg' in
               cmd_string):
             data = "  kVxztV-dKpG-Rz7E-xtKY-jeju-QsYU-SLG6Z1\n"
         elif 'env, LC_ALL=C, vgs, --noheadings, --unit=g, ' \

diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py
index 4954e34b15ea386f91bb955bbef7a614a1187270..f5a4b1e2d88145f67eaa6e336fa8d95c04641b1e 100644 (file)
--- a/cinder/volume/drivers/lvm.py
+++ b/cinder/volume/drivers/lvm.py
@@ -52,6 +52,12 @@ volume_opts = [
     cfg.StrOpt('lvm_type',
                default='default',
                help='Type of LVM volumes to deploy; (default or thin)'),
+    cfg.StrOpt('lvm_conf_file',
+               default='/etc/cinder/lvm.conf',
+               help='LVM conf file to use for the LVM driver in Cinder; '
+                    'this setting is ignored if the specified file does '
+                    'not exist (You can also specify \'None\' to not use '
+                    'a conf file even if one exists).')
 ]
 
 CONF = cfg.CONF
@@ -229,11 +235,18 @@ class LVMVolumeDriver(driver.VolumeDriver):
         """Verify that requirements are in place to use LVM driver."""
         if self.vg is None:
             root_helper = utils.get_root_helper()
+
+            lvm_conf_file = self.configuration.lvm_conf_file
+            if lvm_conf_file.lower() == 'none':
+                lvm_conf_file = None
+
             try:
                 self.vg = lvm.LVM(self.configuration.volume_group,
                                   root_helper,
                                   lvm_type=self.configuration.lvm_type,
-                                  executor=self._execute)
+                                  executor=self._execute,
+                                  lvm_conf=lvm_conf_file)
+
             except brick_exception.VolumeGroupNotFound:
                 message = (_("Volume Group %s does not exist") %
                            self.configuration.volume_group)
@@ -519,9 +532,16 @@ class LVMVolumeDriver(driver.VolumeDriver):
                 return false_ret
 
             helper = utils.get_root_helper()
+
+            lvm_conf_file = self.configuration.lvm_conf_file
+            if lvm_conf_file.lower() == 'none':
+                lvm_conf_file = None
+
             dest_vg_ref = lvm.LVM(dest_vg, helper,
                                   lvm_type=lvm_type,
-                                  executor=self._execute)
+                                  executor=self._execute,
+                                  lvm_conf=lvm_conf_file)
+
             self.remove_export(ctxt, volume)
             self._create_volume(volume['name'],
                                 self._sizestr(volume['size']),

diff --git a/etc/cinder/rootwrap.d/volume.filters b/etc/cinder/rootwrap.d/volume.filters
index a90f8ded16b7ec5637f0e735d19369b3075e8fae..5fc7d7daa87feac11c78d79d4df7591d6fc3e7e5 100644 (file)
--- a/etc/cinder/rootwrap.d/volume.filters
+++ b/etc/cinder/rootwrap.d/volume.filters
@@ -14,6 +14,12 @@ vgs: EnvFilter, env, root, LC_ALL=C, vgs
 lvs: EnvFilter, env, root, LC_ALL=C, lvs
 lvdisplay: EnvFilter, env, root, LC_ALL=C, lvdisplay
 
+# LVM conf var
+pvs_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, pvs
+vgs_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, vgs
+lvs_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, lvs
+lvdisplay_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, lvdisplay
+
 # cinder/volumes/drivers/srb.py: 'pvresize', '--setphysicalvolumesize', sizestr, pvname
 pvresize: CommandFilter, pvresize, root