From: Aaron Rosen <aaronorosen@gmail.com>
Date: Tue, 27 May 2014 20:39:39 +0000 (-0700)
Subject: Make linux.utils.execute log error on return codes
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=77046d0d889a8bff095083ae80a2accd0e25406b;p=openstack-build%2Fneutron-build.git

Make linux.utils.execute log error on return codes

Previously, the execute method in neutron logs everything as debug which hides
a lot of extremely fatal errors like unable to apply security group rules!
This patch changes this code so that we log all non 0 returns as error.

Change-Id: I7328e62269212ccd9c7950ff064a3e337de56918
Closes-bug: 1323832
Related-bug: 1322945
---

diff --git a/neutron/agent/linux/utils.py b/neutron/agent/linux/utils.py
index 72b17b425..d4ef237e5 100644
--- a/neutron/agent/linux/utils.py
+++ b/neutron/agent/linux/utils.py
@@ -71,9 +71,12 @@ def execute(cmd, root_helper=None, process_input=None, addl_env=None,
         m = _("\nCommand: %(cmd)s\nExit code: %(code)s\nStdout: %(stdout)r\n"
               "Stderr: %(stderr)r") % {'cmd': cmd, 'code': obj.returncode,
                                        'stdout': _stdout, 'stderr': _stderr}
-        LOG.debug(m)
-        if obj.returncode and check_exit_code:
-            raise RuntimeError(m)
+        if obj.returncode:
+            LOG.error(m)
+            if check_exit_code:
+                raise RuntimeError(m)
+        else:
+            LOG.debug(m)
     finally:
         # NOTE(termie): this appears to be necessary to let the subprocess
         #               call clean something up in between calls, without
diff --git a/neutron/tests/unit/test_agent_linux_utils.py b/neutron/tests/unit/test_agent_linux_utils.py
index fd085b883..d54d9f1eb 100644
--- a/neutron/tests/unit/test_agent_linux_utils.py
+++ b/neutron/tests/unit/test_agent_linux_utils.py
@@ -20,6 +20,19 @@ from neutron.agent.linux import utils
 from neutron.tests import base
 
 
+class FakeCreateProcess(object):
+    class FakeStdin(object):
+        def close(self):
+            pass
+
+    def __init__(self, returncode):
+        self.returncode = returncode
+        self.stdin = self.FakeStdin()
+
+    def communicate(self, process_input=None):
+        return '', ''
+
+
 class AgentUtilsExecuteTest(base.BaseTestCase):
     def setUp(self):
         super(AgentUtilsExecuteTest, self).setUp()
@@ -75,6 +88,28 @@ class AgentUtilsExecuteTest(base.BaseTestCase):
                                addl_env={'foo': 'bar'})
         self.assertEqual(result, expected)
 
+    def test_return_code_log_error_raise_runtime(self):
+        with mock.patch.object(utils, 'create_process') as create_process:
+            create_process.return_value = FakeCreateProcess(1), 'ls'
+            with mock.patch.object(utils, 'LOG') as log:
+                self.assertRaises(RuntimeError, utils.execute,
+                                  ['ls'])
+                self.assertTrue(log.error.called)
+
+    def test_return_code_log_error_no_raise_runtime(self):
+        with mock.patch.object(utils, 'create_process') as create_process:
+            create_process.return_value = FakeCreateProcess(1), 'ls'
+            with mock.patch.object(utils, 'LOG') as log:
+                utils.execute(['ls'], check_exit_code=False)
+                self.assertTrue(log.error.called)
+
+    def test_return_code_log_debug(self):
+        with mock.patch.object(utils, 'create_process') as create_process:
+            create_process.return_value = FakeCreateProcess(0), 'ls'
+            with mock.patch.object(utils, 'LOG') as log:
+                utils.execute(['ls'])
+                self.assertTrue(log.debug.called)
+
 
 class AgentUtilsGetInterfaceMAC(base.BaseTestCase):
     def test_get_interface_mac(self):