Change in vdsm[master]: move exec_sync_bytes to common.cmdutils

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has uploaded a new change for review.

Change subject: move exec_sync_bytes to common.cmdutils
......................................................................

move exec_sync_bytes to common.cmdutils

exec_sync_bytes is useful not only in the network subpackage, so it
needs to reside under vdsm.common.

Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Signed-off-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M lib/vdsm/common/cmdutils.py
M lib/vdsm/network/cmd.py
2 files changed, 16 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/93/79693/1

diff --git a/lib/vdsm/common/cmdutils.py b/lib/vdsm/common/cmdutils.py
index 85a4ae0..26e08a7 100644
--- a/lib/vdsm/common/cmdutils.py
+++ b/lib/vdsm/common/cmdutils.py
@@ -22,6 +22,8 @@
 import os
 import re
 
+from vdsm.common.compat import CPopen as Popen
+
 
 SYSTEMD_RUN = "/usr/bin/systemd-run"
 
@@ -106,3 +108,16 @@
     CPU = 'CPU'
     Memory = 'Memory'
     BlockIO = 'BlockIO'
+
+
+def exec_sync_bytes(cmds):
+    logging.debug(cmdutils.command_log_line(cmds))
+
+    p = Popen(
+        cmds, close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
+    out, err = p.communicate()
+
+    logging.debug(cmdutils.retcode_log_line(p.returncode, err=err))
+
+    return p.returncode, out, err
diff --git a/lib/vdsm/network/cmd.py b/lib/vdsm/network/cmd.py
index f6d9b73..bf5d537 100644
--- a/lib/vdsm/network/cmd.py
+++ b/lib/vdsm/network/cmd.py
@@ -23,8 +23,7 @@
 import uuid
 
 from vdsm.common import cmdutils
-from vdsm.common.cmdutils import systemd_run
-from vdsm.common.compat import CPopen as Popen
+from vdsm.common.cmdutils import systemd_run, exec_sync_bytes
 from vdsm.network import py2to3
 
 
@@ -50,16 +49,3 @@
     cmds = systemd_run(cmds, scope=True, unit=unit, slice=slice_name)
 
     return exec_sync(cmds)
-
-
-def exec_sync_bytes(cmds):
-    logging.debug(cmdutils.command_log_line(cmds))
-
-    p = Popen(
-        cmds, close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-
-    out, err = p.communicate()
-
-    logging.debug(cmdutils.retcode_log_line(p.returncode, err=err))
-
-    return p.returncode, out, err


-- 
To view, visit https://gerrit.ovirt.org/79693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken(a)redhat.com&gt;

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: move exec_sync_bytes to common.cmdutils
......................................................................


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/79693/4/lib/vdsm/common/cmdutils.py
File lib/vdsm/common/cmdutils.py:

Line 111:     Memory = 'Memory'
Line 112:     BlockIO = 'BlockIO'
Line 113: 
Line 114: 
Line 115: def exec_sync_bytes(cmds):
>  Please rename it to run(). 
Are you asking me for a (limited)
backport of python3's subprocess.run? If so, why?

I am looking for a bare-bone version of execCmd, to be used in network/* and hooks.py;
Using run(check=True) to achieve  cleaner code all over Vdsm should not be part of this
task.

It is important for me to express that my out/err is kept in bytes. Each user should know
that it is responsible to decoding, if needed.
Line 116:     logging.debug(command_log_line(cmds))
Line 117: 
Line 118:     p = Popen(
Line 119:         cmds, close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)


-- 
To view, visit https://gerrit.ovirt.org/79693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: move exec_sync_bytes to common.cmdutils
......................................................................


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/79693/4/lib/vdsm/common/cmdutils.py
File lib/vdsm/common/cmdutils.py:

Line 111:     Memory = 'Memory'
Line 112:     BlockIO = 'BlockIO'
Line 113: 
Line 114: 
Line 115: def exec_sync_bytes(cmds):
>  I just suggested a nicer name. I don't want to have name like
> exec_sync_byt 
I don't know what you mean by default behavior. 

It is important to keep encoding mess local to function that communicate with the external
world. However, some external commands may return non-ut8 bytestring, so we cannot decode
at this low level.

I feel very bad about this, and want to warn the users of this function about this fact.

The problem with a nicer name like `run` is that may lead someone (e.g. me) to understand
it has a signature similar to that of subprocess.run.

How about a plain exec_cmd()?
Line 116:     logging.debug(command_log_line(cmds))
Line 117: 
Line 118:     p = Popen(
Line 119:         cmds, close_fds=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)


-- 
To view, visit https://gerrit.ovirt.org/79693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: move exec_sync_bytes to common.cmdutils.exec_cmd

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd
......................................................................


Patch Set 6: Verified+1

OST
http://jenkins.ovirt.org/view/oVirt%20system%20tests/job/ovirt-system-tes... is
happy

-- 
To view, visit https://gerrit.ovirt.org/79693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: move exec_sync_bytes to common.cmdutils.exec_cmd

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd
......................................................................


Patch Set 8: Verified+1

-- 
To view, visit https://gerrit.ovirt.org/79693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: move exec_sync_bytes to common.cmdutils.exec_cmd

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd
......................................................................


Patch Set 8:

(1 comment)

https://gerrit.ovirt.org/#/c/79693/8/lib/vdsm/network/cmd.py
File lib/vdsm/network/cmd.py:

Line 51
Line 52
Line 53
Line 54
Line 55
>  Dan, did you miss my comment about the docstring and the warnings
> needed he 
yep


-- 
To view, visit https://gerrit.ovirt.org/79693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes

Change in vdsm[master]: move exec_sync_bytes to common.cmdutils.exec_cmd

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has posted comments on this change.

Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd
......................................................................


Patch Set 10: Verified+1

OST passes
http://jenkins.ovirt.org/view/oVirt%20system%20tests/job/ovirt-system-tes...

-- 
To view, visit https://gerrit.ovirt.org/79693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Change in vdsm[master]: move exec_sync_bytes to common.cmdutils.exec_cmd

[Code Review]

From Dan Kenigsberg <danken(a)redhat.com&gt;:

Dan Kenigsberg has submitted this change and it was merged. (
https://gerrit.ovirt.org/79693 )

Change subject: move exec_sync_bytes to common.cmdutils.exec_cmd
......................................................................


move exec_sync_bytes to common.cmdutils.exec_cmd

exec_cmd(), a skinny version of our ancient over-grown execCmd, is
useful not only in the network subpackage, so it needs to reside under
vdsm.common.

Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Signed-off-by: Dan Kenigsberg <danken(a)redhat.com&gt;
---
M lib/vdsm/common/cmdutils.py
M lib/vdsm/network/cmd.py
M tests/common/cmdutils_test.py
3 files changed, 53 insertions(+), 17 deletions(-)

Approvals:
  Piotr Kliczewski: Looks good to me, approved
  Nir Soffer: Looks good to me, but someone else must approve
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Verified
  Francesco Romani: Looks good to me, but someone else must approve
  Edward Haas: Looks good to me, but someone else must approve



-- 
To view, visit https://gerrit.ovirt.org/79693
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Idca5b123a4ce6ef5ebaff8084d788e2acd47c2be
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;