Change in vdsm[master]: execCmd: Make loggign when successful optional

Antoni Segura Puimedon has uploaded a new change for review.

Change subject: execCmd: Make loggign when successful optional
......................................................................

execCmd: Make loggign when successful optional

Make execCmd have an option that logs to debug only when the
proc returns an error code.

Make getLinks use it so the log is not spammed anymore by
nic info getting (sorry everyone about the spam).

Change-Id: I0c550aded704dd944f4f2af88b7925f9473f1a4a
Signed-off-by: Antoni S. Puimedon 
---
M lib/vdsm/ipwrapper.py
M lib/vdsm/utils.py
2 files changed, 7 insertions(+), 5 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/15/21515/1

diff --git a/lib/vdsm/ipwrapper.py b/lib/vdsm/ipwrapper.py
index afd7f6c..9f55d82 100644
--- a/lib/vdsm/ipwrapper.py
+++ b/lib/vdsm/ipwrapper.py
@@ -475,7 +475,7 @@
     command = [_IP_BINARY.cmd, '-d', '-o', 'link']
     if dev:
         command += ['show', 'dev', dev]
-    return _execCmd(command)
+    return _execCmd(command, logWhenSuccessful=False)

def addrAdd(dev, ipaddr, netmask):
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py
index 3166a92..b6edf64 100644
--- a/lib/vdsm/utils.py
+++ b/lib/vdsm/utils.py
@@ -490,7 +490,7 @@
 def execCmd(command, sudo=False, cwd=None, data=None, raw=False, logErr=True,
             printable=None, env=None, sync=True, nice=None, ioclass=None,
             ioclassdata=None, setsid=False, execCmdLogger=logging.root,
-            deathSignal=0):
+            deathSignal=0, logWhenSuccessful=True):
     """
     Executes an external command, optionally via sudo.

@@ -535,9 +535,11 @@
         # Prevent splitlines() from barfing later on
         out = ""

-    execCmdLogger.debug("%s: <err> = %s; <rc> = %d",
-                        {True: "SUCCESS", False: "FAILED"}[p.returncode == 0],
-                        repr(err), p.returncode)
+    if logWhenSuccessful or p.returncode != 0:
+        execCmdLogger.debug(
+            "%s: <err> = %s; <rc> = %d",
+            {True: "SUCCESS", False: "FAILED"}[p.returncode == 0], repr(err),
+            p.returncode)

if not raw:
         out = out.splitlines(False)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: execCmd: Make loggign when successful optional
......................................................................

Patch Set 1: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4796/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5596/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/834/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5679/ : FAILURE

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: execCmd: Make loggign when successful optional
......................................................................

Patch Set 2:

This could be useful for other places as well, but I know Saggi would like to get rid of all these options by creating new sane execCmd function that does not log anything by default.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: execCmd: Make loggign when successful optional
......................................................................

Patch Set 2: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4798/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5598/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_network_functional_tests/835/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5681/ : SUCCESS

Assaf Muller has posted comments on this change.

Change subject: execCmd: Make loggign when successful optional
......................................................................

Patch Set 2: Code-Review+1

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: execCmd: Make loggign when successful optional
......................................................................

Patch Set 2:

(3 comments)

I think this patch can be more useful if it allows to turn off all logging in execCmd. This should be under complete control of the caller.

....................................................
File lib/vdsm/utils.py
Line 489: 
Line 490: def execCmd(command, sudo=False, cwd=None, data=None, raw=False, logErr=True,
Line 491:             printable=None, env=None, sync=True, nice=None, ioclass=None,
Line 492:             ioclassdata=None, setsid=False, execCmdLogger=logging.root,
Line 493:             deathSignal=0, logWhenSuccessful=True):
I would call this "verbose"
Line 494:     """
Line 495:     Executes an external command, optionally via sudo.
Line 496: 
Line 497:     IMPORTANT NOTE: don't define a deathSignal when sync=False

Line 516:     if not printable:
Line 517:         printable = command
Line 518: 
Line 519:     cmdline = repr(subprocess.list2cmdline(printable))
Line 520:     execCmdLogger.debug("%s (cwd %s)", cmdline, cwd)
You should disable also this log:

if verbose:
        # log command line here
Line 521: 
Line 522:     p = BetterPopen(command, close_fds=True, cwd=cwd, env=env,
Line 523:                     deathSignal=deathSignal)
Line 524:     p = AsyncProc(p)

Line 534:     if out is None:
Line 535:         # Prevent splitlines() from barfing later on
Line 536:         out = ""
Line 537: 
Line 538:     if logWhenSuccessful or p.returncode != 0:
You don't need to check error code here. The caller gets all the information needed to log the proper error if needed.

if verbose:
        # log debug here

If you want to turn off only return value debug logs, you don't have to add a new argument, there is already a logErr argument. Note that Saggi rejected the last patch that tried to do that though.
Line 539:         execCmdLogger.debug(
Line 540:             "%s: <err> = %s; <rc> = %d",
Line 541:             {True: "SUCCESS", False: "FAILED"}[p.returncode == 0], repr(err),
Line 542:             p.returncode)

Antoni Segura Puimedon has abandoned this change.

Change subject: execCmd: Make loggign when successful optional
......................................................................

Abandoned