From: Eric Harney <eharney@redhat.com>
Date: Fri, 21 Nov 2014 16:50:48 +0000 (-0500)
Subject: Brick LVM: LV not found logging and error handling
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=8a08a6cdd3688e799ae2db4bd94b8b14dc50f2a9;p=openstack-build%2Fcinder-build.git

Brick LVM: LV not found logging and error handling

Fix up two things in get_lv_info/get_volume:
1. ProcessExecutionError should be handled where we call the
   command.  We can just return an empty list for this case
   which makes things simple for callers and consistent with
   querying a VG.
2. Not found errors should be logged as info and not warning
   since this is generally not actionable by the admin (and not
   a problem).

Fix typo in lvm command output for not found test.

Add test for get_lv_info not found error.

Change-Id: Iebccf7b8f252303f586b36aad33b85945ea5c927
Related-Bug: #1390081
---

diff --git a/cinder/brick/local_dev/lvm.py b/cinder/brick/local_dev/lvm.py
index 2a94892cc..8b9cc861d 100644
--- a/cinder/brick/local_dev/lvm.py
+++ b/cinder/brick/local_dev/lvm.py
@@ -27,7 +27,7 @@ from oslo.utils import excutils
 
 from cinder.brick import exception
 from cinder.brick import executor
-from cinder.i18n import _, _LE, _LW
+from cinder.i18n import _, _LE, _LI, _LW
 from cinder.openstack.common import log as logging
 
 
@@ -241,6 +241,7 @@ class LVM(executor.Executor):
 
         :param root_helper: root_helper to use for execute
         :param vg_name: optional, gathers info for only the specified VG
+        :param lv_name: optional, gathers info for only the specified LV
         :returns: List of Dictionaries with LV info
 
         """
@@ -254,12 +255,22 @@ class LVM(executor.Executor):
             cmd.append(vg_name)
 
         lvs_start = time.time()
-        (out, _err) = putils.execute(*cmd,
-                                     root_helper=root_helper,
-                                     run_as_root=True)
+        try:
+            (out, _err) = putils.execute(*cmd,
+                                         root_helper=root_helper,
+                                         run_as_root=True)
+        except putils.ProcessExecutionError as err:
+            with excutils.save_and_reraise_exception(reraise=True) as ctx:
+                if "not found" in err.stderr:
+                    ctx.reraise = False
+                    msg = _LI("'Not found' when querying LVM info. "
+                              "(vg_name=%(vg)s, lv_name=%(lv)s")
+                    LOG.info(msg, {'vg': vg_name, 'lv': lv_name})
+                    out = None
+
         total_time = time.time() - lvs_start
         if total_time > 60:
-            LOG.warning(_LW('Took %s seconds to get logical volumes.'),
+            LOG.warning(_LW('Took %s seconds to get logical volume info.'),
                         total_time)
 
         lv_list = []
@@ -287,21 +298,10 @@ class LVM(executor.Executor):
         :returns: dict representation of Logical Volume if exists
 
         """
-        try:
-            ref_list = self.get_volumes(name)
-            for r in ref_list:
-                if r['name'] == name:
-                    return r
-        except putils.ProcessExecutionError as err:
-            # NOTE(joachim): Catch the "notfound" case from the stderr
-            # in order to let the other errors through.
-            with excutils.save_and_reraise_exception(reraise=True) as ctx:
-                if "not found" in err.stderr:
-                    ctx.reraise = False
-                    LOG.warning(
-                        _LW("Caught exception for lvs 'LV not found': %s")
-                        % (name)
-                    )
+        ref_list = self.get_volumes(name)
+        for r in ref_list:
+            if r['name'] == name:
+                return r
         return None
 
     @staticmethod
diff --git a/cinder/tests/brick/test_brick_lvm.py b/cinder/tests/brick/test_brick_lvm.py
index 156c9a5ac..3cfacbab8 100644
--- a/cinder/tests/brick/test_brick_lvm.py
+++ b/cinder/tests/brick/test_brick_lvm.py
@@ -87,11 +87,16 @@ 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, '
+              '--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, '
               '--unit=g, -o, vg_name,name,size' in cmd_string):
             if 'fake-unknown' in cmd_string:
                 raise processutils.ProcessExecutionError(
-                    stderr="One of more volume(s) not found."
+                    stderr="One or more volume(s) not found."
                 )
             data = "  fake-vg fake-1 1.00g\n"
             data += "  fake-vg fake-2 1.00g\n"
@@ -154,6 +159,13 @@ class BrickLvmTestCase(test.TestCase):
     def test_get_volume_none(self):
         self.assertEqual(self.vg.get_volume('fake-unknown'), None)
 
+    def test_get_lv_info_notfound(self):
+        self.assertEqual(
+            self.vg.get_lv_info(
+                'sudo', vg_name='fake-vg', lv_name='lv-nothere'),
+            []
+        )
+
     def test_get_all_physical_volumes(self):
         # Filtered VG version
         pvs = self.vg.get_all_physical_volumes('sudo', 'fake-vg')