Provide cfg to ignore lvm descriptor leak warnings

For some reason the leaded descriptor warning message coming
from LVM is causing Cinder to fail startup and it appears to be
masking out the vg response in vgs calls.

We typically don't hit this, but due to the nature of Kolla and
I guess going through the different processes via the containers
this gets logged every time vgs is called.  Eric Harney rightly
pointed out that rather than use exception handling and such
that we should use the LVM env variable mechanism we already have
in place in Cinder.

For now this patch added a new config option to the LVM driver:
    lvm_suppress_fd_warnings=True|False

This is useful for K8 deployments that have an indirect call to the
LVM cmds which results in failures.

For those that are interested, this can also be done outside of
cinder by setting the silence_logs variable in lvm.conf

This is made optional as a config flag to avoid any breakage for
existing deployments during upgrade.

Change-Id: I85612fa49475beea58d30330c8fe8352a2f91123
Closes-Bug: #1619701

cinder/brick/local_dev/lvm.py

@@ -41,7 +41,8 @@ class LVM(executor.Executor):
 
     def __init__(self, vg_name, root_helper, create_vg=False,
                  physical_volumes=None, lvm_type='default',
-                 executor=putils.execute, lvm_conf=None):
+                 executor=putils.execute, lvm_conf=None,
+                 suppress_fd_warn=False):
 
         """Initialize the LVM object.
 
@@ -55,6 +56,7 @@ class LVM(executor.Executor):
         :param physical_volumes: List of PVs to build VG on
         :param lvm_type: VG and Volume type (default, or thin)
         :param executor: Execute method to use, None uses common/processutils
+        :param suppress_fd_warn: Add suppress FD Warn to LVM env
 
         """
         super(LVM, self).__init__(execute=executor, root_helper=root_helper)
@@ -74,11 +76,19 @@ class LVM(executor.Executor):
         # Ensure LVM_SYSTEM_DIR has been added to LVM.LVM_CMD_PREFIX
         # before the first LVM command is executed, and use the directory
         # where the specified lvm_conf file is located as the value.
+
+        # NOTE(jdg): We use the temp var here becuase LVM_CMD_PREFIX is a
+        # class global and if you use append here, you'll literally just keep
+        # appending values to the global.
+        _lvm_cmd_prefix = ['env', 'LC_ALL=C']
+
         if lvm_conf and os.path.isfile(lvm_conf):
             lvm_sys_dir = os.path.dirname(lvm_conf)
-            LVM.LVM_CMD_PREFIX = ['env',
+            _lvm_cmd_prefix.append('LVM_SYSTEM_DIR=' + lvm_sys_dir)
-                                  'LC_ALL=C',
+
-                                  'LVM_SYSTEM_DIR=' + lvm_sys_dir]
+        if suppress_fd_warn:
+            _lvm_cmd_prefix.append('LVM_SUPPRESS_FD_WARNINGS=1')
+        LVM.LVM_CMD_PREFIX = _lvm_cmd_prefix
 
         if create_vg and physical_volumes is not None:
             self.pv_list = physical_volumes

cinder/tests/unit/brick/test_brick_lvm.py

@@ -23,16 +23,20 @@ from cinder.volume import configuration as conf
 
 class BrickLvmTestCase(test.TestCase):
     def setUp(self):
-        self.configuration = mock.Mock(conf.Configuration)
+        if not hasattr(self, 'configuration'):
+            self.configuration = mock.Mock(conf.Configuration)
+            self.configuration.lvm_suppress_fd_warnings = False
         self.configuration.volume_group_name = 'fake-vg'
         super(BrickLvmTestCase, self).setUp()
 
         self.mock_object(processutils, 'execute', self.fake_execute)
-        self.vg = brick.LVM(self.configuration.volume_group_name,
+        self.vg = brick.LVM(
-                            'sudo',
+            self.configuration.volume_group_name,
-                            False, None,
+            'sudo',
-                            'default',
+            False, None,
-                            self.fake_execute)
+            'default',
+            self.fake_execute,
+            suppress_fd_warn=self.configuration.lvm_suppress_fd_warnings)
 
     def failed_fake_execute(obj, *cmd, **kwargs):
         return ("\n", "fake-error")
@@ -48,24 +52,28 @@ class BrickLvmTestCase(test.TestCase):
         return ("  LVM version:     2.02.100(2)-RHEL6 (2013-09-12)\n", "")
 
     def fake_execute(obj, *cmd, **kwargs):  # noqa
+        if obj.configuration.lvm_suppress_fd_warnings:
+            _lvm_prefix = 'env, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=1, '
+        else:
+            _lvm_prefix = 'env, LC_ALL=C, '
+
         cmd_string = ', '.join(cmd)
         data = "\n"
-
+        if (_lvm_prefix + 'vgs, --noheadings, --unit=g, -o, name' ==
-        if ('env, LC_ALL=C, vgs, --noheadings, --unit=g, -o, name' ==
                 cmd_string):
             data = "  fake-vg\n"
             data += "  some-other-vg\n"
-        elif ('env, LC_ALL=C, vgs, --noheadings, -o, name, fake-vg' ==
+        elif (_lvm_prefix + 'vgs, --noheadings, -o, name, fake-vg' ==
                 cmd_string):
             data = "  fake-vg\n"
-        elif 'env, LC_ALL=C, vgs, --version' in cmd_string:
+        elif _lvm_prefix + '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(_lvm_prefix + 'vgs, --noheadings, -o, uuid, fake-vg' in
-              cmd_string):
+             cmd_string):
             data = "  kVxztV-dKpG-Rz7E-xtKY-jeju-QsYU-SLG6Z1\n"
-        elif 'env, LC_ALL=C, vgs, --noheadings, --unit=g, ' \
+        elif(_lvm_prefix + 'vgs, --noheadings, --unit=g, '
-             '-o, name,size,free,lv_count,uuid, ' \
+             '-o, name,size,free,lv_count,uuid, '
-             '--separator, :, --nosuffix' in cmd_string:
+             '--separator, :, --nosuffix' in cmd_string):
             data = ("  test-prov-cap-vg-unit:10.00:10.00:0:"
                     "mXzbuX-dKpG-Rz7E-xtKY-jeju-QsYU-SLG8Z4\n")
             if 'test-prov-cap-vg-unit' in cmd_string:
@@ -82,17 +90,17 @@ class BrickLvmTestCase(test.TestCase):
                     "lWyauW-dKpG-Rz7E-xtKY-jeju-QsYU-SLG7Z2\n"
             data += "  fake-vg-3:10.00:10.00:0:"\
                     "mXzbuX-dKpG-Rz7E-xtKY-jeju-QsYU-SLG8Z3\n"
-        elif ('env, LC_ALL=C, lvs, --noheadings, '
+        elif (_lvm_prefix + 'lvs, --noheadings, '
               '--unit=g, -o, vg_name,name,size, --nosuffix, '
               'fake-vg/lv-nothere' in cmd_string):
             raise processutils.ProcessExecutionError(
                 stderr="One or more specified logical volume(s) not found.")
-        elif ('env, LC_ALL=C, lvs, --noheadings, '
+        elif (_lvm_prefix + 'lvs, --noheadings, '
               '--unit=g, -o, vg_name,name,size, --nosuffix, '
               'fake-vg/lv-newerror' in cmd_string):
             raise processutils.ProcessExecutionError(
                 stderr="Failed to find logical volume \"fake-vg/lv-newerror\"")
-        elif ('env, LC_ALL=C, lvs, --noheadings, '
+        elif (_lvm_prefix + 'lvs, --noheadings, '
               '--unit=g, -o, vg_name,name,size' in cmd_string):
             if 'fake-unknown' in cmd_string:
                 raise processutils.ProcessExecutionError(
@@ -111,7 +119,7 @@ class BrickLvmTestCase(test.TestCase):
             else:
                 data = "  fake-vg fake-1 1.00g\n"
                 data += "  fake-vg fake-2 1.00g\n"
-        elif ('env, LC_ALL=C, lvdisplay, --noheading, -C, -o, Attr' in
+        elif (_lvm_prefix + 'lvdisplay, --noheading, -C, -o, Attr' in
               cmd_string):
             if 'test-volumes' in cmd_string:
                 data = '  wi-a-'
@@ -121,19 +129,19 @@ class BrickLvmTestCase(test.TestCase):
                 data = '  -wi-ao---'
             else:
                 data = '  owi-a-'
-        elif ('env, LC_ALL=C, lvdisplay, --noheading, -C, -o, Origin' in
+        elif (_lvm_prefix + 'lvdisplay, --noheading, -C, -o, Origin' in
               cmd_string):
             if 'snapshot' in cmd_string:
                 data = '  fake-volume-1'
             else:
                 data = '       '
-        elif 'env, LC_ALL=C, pvs, --noheadings' in cmd_string:
+        elif _lvm_prefix + 'pvs, --noheadings' in cmd_string:
             data = "  fake-vg|/dev/sda|10.00|1.00\n"
             data += "  fake-vg|/dev/sdb|10.00|1.00\n"
             data += "  fake-vg|/dev/sdc|10.00|8.99\n"
             data += "  fake-vg-2|/dev/sdd|10.00|9.99\n"
-        elif 'env, LC_ALL=C, lvs, --noheadings, --unit=g' \
+        elif _lvm_prefix + 'lvs, --noheadings, --unit=g' \
-             ', -o, size,data_percent, --separator, :' in cmd_string:
+                ', -o, size,data_percent, --separator, :' in cmd_string:
             if 'test-prov-cap-pool' in cmd_string:
                 data = "  9.5:20\n"
             else:
@@ -389,3 +397,10 @@ class BrickLvmTestCase(test.TestCase):
             self.vg.create_volume('test', '1G')
             self.assertRaises(exception.VolumeNotDeactivated,
                               self.vg.deactivate_lv, 'test')
+
+
+class BrickLvmTestCaseIgnoreFDWarnings(BrickLvmTestCase):
+    def setUp(self):
+        self.configuration = mock.Mock(conf.Configuration)
+        self.configuration.lvm_suppress_fd_warnings = True
+        super(BrickLvmTestCaseIgnoreFDWarnings, self).setUp()

cinder/volume/drivers/lvm.py

@@ -69,7 +69,11 @@ volume_opts = [
                  help='max_over_subscription_ratio setting for the LVM '
                       'driver.  If set, this takes precedence over the '
                       'general max_over_subscription_ratio option.  If '
-                      'None, the general option is used.')
+                      'None, the general option is used.'),
+    cfg.BoolOpt('lvm_suppress_fd_warnings',
+                default=False,
+                help='Suppress leaked file descriptor warnings in LVM '
+                     'commands.')
 ]
 
 CONF = cfg.CONF
@@ -287,11 +291,14 @@ class LVMVolumeDriver(driver.VolumeDriver):
                 lvm_conf_file = None
 
             try:
-                self.vg = lvm.LVM(self.configuration.volume_group,
+                self.vg = lvm.LVM(
-                                  root_helper,
+                    self.configuration.volume_group,
-                                  lvm_type=self.configuration.lvm_type,
+                    root_helper,
-                                  executor=self._execute,
+                    lvm_type=self.configuration.lvm_type,
-                                  lvm_conf=lvm_conf_file)
+                    executor=self._execute,
+                    lvm_conf=lvm_conf_file,
+                    suppress_fd_warn=(
+                        self.configuration.lvm_suppress_fd_warnings))
 
             except exception.VolumeGroupNotFound:
                 message = (_("Volume Group %s does not exist") %

etc/cinder/rootwrap.d/volume.filters

@@ -16,12 +16,25 @@ 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
+# -LVM related show commands with suppress fd warnings
+pvs_fdwarn: EnvFilter, env, root, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, pvs
+vgs_fdwarn: EnvFilter, env, root, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, vgs
+lvs_fdwarn: EnvFilter, env, root, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, lvs
+lvdisplay_fdwarn: EnvFilter, env, root, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, lvdisplay
+
+
+# -LVM related show commands 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
 
+# -LVM conf var with suppress fd_warnings
+pvs_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, pvs
+vgs_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, vgs
+lvs_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, lvs
+lvdisplay_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, lvdisplay
+
 # os-brick library commands
 # os_brick.privileged.run_as_root oslo.privsep context
 # This line ties the superuser privs with the config files, context name,
@@ -40,6 +53,8 @@ vgcreate: CommandFilter, vgcreate, root
 # cinder/brick/local_dev/lvm.py: 'lvcreate', '-L', ...
 lvcreate: EnvFilter, env, root, LC_ALL=C, lvcreate
 lvcreate_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, lvcreate
+lvcreate_fdwarn: EnvFilter, env, root, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, lvcreate
+lvcreate_lvmconf_fdwarn: EnvFilter, env, root, LVM_SYSTEM_DIR=, LVM_SUPPRESS_FD_WARNINGS=, LC_ALL=C, lvcreate
 
 # cinder/volume/driver.py: 'dd', 'if=%s' % srcstr, 'of=%s' % deststr,...
 dd: CommandFilter, dd, root
@@ -54,6 +69,8 @@ lvrename: CommandFilter, lvrename, root
 # cinder/brick/local_dev/lvm.py: 'lvextend', '-L' '%(new_size)s', '%(thin_pool)s' ...
 lvextend: EnvFilter, env, root, LC_ALL=C, lvextend
 lvextend_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, lvextend
+lvextend_fdwarn: EnvFilter, env, root, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, lvextend
+lvextend_lvmconf_fdwarn: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, LVM_SUPPRESS_FD_WARNINGS=, lvextend
 
 # cinder/brick/local_dev/lvm.py: 'lvchange -a y -K <lv>'
 lvchange: CommandFilter, lvchange, root

releasenotes/notes/add-suppress-lvm-fd-warnings-option.402bebc03b0a9f00.yaml

@@ -0,0 +1,17 @@
+---
+upgrade:
+    - |
+      In certain environments (Kubernetes for example) indirect calls to the LVM
+      commands result in file descriptor leak warning messages which in turn cause
+      the process_execution method to raise and exception.
+
+      To accommodate these environments, and to maintain backward compatibility
+      in Newton we add a ``lvm_suppress_fd_warnings`` bool config to the LVM driver.
+      Setting this to True will append the LVM env vars to include the variable
+      ``LVM_SUPPRESS_FD_WARNINGS=1``.
+
+      This is made an optional configuration because it only applies to very specific
+      environments.  If we were to make this global that would require a rootwrap/privsep
+      update that could break compatibility when trying to do rolling upgrades of the
+      volume service.
+