Change in vdsm[master]: make misc.pgrep for general usage

Royce Lv has uploaded a new change for review.

Change subject: make misc.pgrep for general usage
......................................................................

make misc.pgrep for general usage

pgrep now just grep specific named process,
  Make it accept other params(e.g.ppid, state, etc)
for general usage.

Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Signed-off-by: Royce Lv
---
M tests/miscTests.py
M vdsm/storage/misc.py
2 files changed, 21 insertions(+), 4 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/33/11033/1

diff --git a/tests/miscTests.py b/tests/miscTests.py
index a949a09..885b913 100644
--- a/tests/miscTests.py
+++ b/tests/miscTests.py
@@ -63,7 +63,7 @@

class PgrepTests(TestCaseBase):
-    def test(self):
+    def testGrepName(self):
         sleepProcs = []
         for i in range(3):
             sleepProcs.append(misc.execCmd([EXT_SLEEP, "3"], sync=False,
@@ -78,6 +78,21 @@
             proc.kill()
             proc.wait()

+    def testGrepPpid(self):
+        sleepProcs = []
+        for i in range(3):
+            sleepProcs.append(misc.execCmd([EXT_SLEEP, "3"], sync=False,
+                              sudo=False))
+
+        pids = misc.pgrep(os.getpid(), 'ppid')
+        for proc in sleepProcs:
+            self.assertTrue(proc.pid in pids, "pid %d was not located by pgrep"
+                            % proc.pid)
+
+        for proc in sleepProcs:
+            proc.kill()
+            proc.wait()
+

class GetCmdArgsTests(TestCaseBase):
     def test(self):
diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py
index 5ec13e4..2438335 100644
--- a/vdsm/storage/misc.py
+++ b/vdsm/storage/misc.py
@@ -1109,8 +1109,10 @@
         yield int(pid)

-def pgrep(name):
+def pgrep(statValue, statKey='name'):
     res = []
+    statMap = {'name': 1, 'state': 2, 'ppid': 3}
+    statIndex = statMap.get(statKey, 1)
     for pid in iteratePids():
         try:
             pid = int(pid)
@@ -1118,8 +1120,8 @@
             continue

try:
-            procName = utils.pidStat(pid)[1]
-            if procName == name:
+            watchedStat = utils.pidStat(pid)[statIndex]
+            if watchedStat == statValue:
                 res.append(pid)
         except (OSError, IOError):
             continue

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/715/ (1/2)

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 1:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/750/ (2/2)

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 1:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/750/ : SUCCESS

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/storage/misc.py
Line 1108:         pid = os.path.basename(path)
Line 1109:         yield int(pid)
Line 1110: 
Line 1111: 
Line 1112: def pgrep(statValue, statKey='name'):
I prefer (value, field)
and a little doctring explaining that the fields are from /proc/[pid]/stats/
Line 1113:     res = []
Line 1114:     statMap = {'name': 1, 'state': 2, 'ppid': 3}
Line 1115:     statIndex = statMap.get(statKey, 1)
Line 1116:     for pid in iteratePids():

Line 1111: 
Line 1112: def pgrep(statValue, statKey='name'):
Line 1113:     res = []
Line 1114:     statMap = {'name': 1, 'state': 2, 'ppid': 3}
Line 1115:     statIndex = statMap.get(statKey, 1)
if someone gave an illegal field name, we should die, not assume that he meant "name".
Line 1116:     for pid in iteratePids():
Line 1117:         try:
Line 1118:             pid = int(pid)
Line 1119:         except ValueError:

Line 1119:         except ValueError:
Line 1120:             continue
Line 1121: 
Line 1122:         try:
Line 1123:             watchedStat = utils.pidStat(pid)[statIndex]
I think it would be much nicer to have pidStat return a namedtuple, with fieldnames taken from proc(5). It would make other usages of pidStat nicer, too.

pid comm state ppid pgrp session tty_nr tpgid flags minflt cminflt majflt cmajflt utime stime cutime cstime priority nice num_threads itrealvalue starttime vsize rss rsslim startcode endcode startstack kstkesp kstkeip signal blocked sigignore sigcatch wchan nswap cnswap exit_signal processor rt_priority policy delayacct_blkio_ticks guest_time cguest_time
Line 1124:             if watchedStat == statValue:
Line 1125:                 res.append(pid)
Line 1126:         except (OSError, IOError):
Line 1127:             continue

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/792/ (1/2)

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 2:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/757/ (2/2)

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 2:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/792/ : SUCCESS

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File tests/miscTests.py
Line 68:         for i in range(3):
Line 69:             sleepProcs.append(misc.execCmd([EXT_SLEEP, "3"], sync=False,
Line 70:                               sudo=False))
Line 71: 
Line 72:         pids = misc.pgrep("sleep", 'comm')
I hope you won't hate me for this, but using two types of quotes for the same thing on the same line throws me off balance.
Line 73:         for proc in sleepProcs:
Line 74:             self.assertTrue(proc.pid in pids, "pid %d was not located by pgrep"
Line 75:                             % proc.pid)
Line 76:

....................................................
File vdsm/storage/misc.py
Line 1109:         yield int(pid)
Line 1110: 
Line 1111: 
Line 1112: def pgrep(statValue, statKey):
Line 1113:     # see man proc(5) /proc/[pid]/stat for statKey value range
please use a proper docstring instead of a comment.

please rename the args to (value, field) to better match the man page language.
Line 1114:     res = []
Line 1115:     for pid in iteratePids():
Line 1116:         try:
Line 1117:             pid = int(pid)

....................................................
File vdsm/utils.py
Line 214:         res.append(statline[procNameStart + 1:procNameEnd])
Line 215:         args = statline[procNameEnd + 2:].split()
Line 216:         res.append(args[0])
Line 217:         res.extend([int(item) for item in args[1:]])
Line 218:         return dict(zip(statName, tuple(res)))
thanks, this change is cool.

but why did you choose not to use namedtuple? imo, namedtuple fits to this use case like a glove to a hand: stats.nice is nicer than stats['nice'].

btw, why do you convert res to a tuple before zipping it?
Line 219: 
Line 220: 
Line 221: class TimedSample:
Line 222:     def __init__(self):

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Shu Ming has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 2: (2 inline comments)

....................................................
File vdsm/storage/blockSD.py
Line 1087:             raise
Line 1088: 
Line 1089:         for umountPid in umountPids:
Line 1090:             try:
Line 1091:                 state = utils.pidStat(umountPid)['state']
I think you need another patch for this change. So as the other similar fixes.
Line 1092:                 mountPoint = misc.getCmdArgs(umountPid)[-1]
Line 1093:             except:
Line 1094:                 # Process probably exited
Line 1095:                 continue

....................................................
File vdsm/utils.py
Line 212:         procNameEnd = statline.rfind(")")
Line 213:         res.append(int(statline[:procNameStart]))
Line 214:         res.append(statline[procNameStart + 1:procNameEnd])
Line 215:         args = statline[procNameEnd + 2:].split()
Line 216:         res.append(args[0])
In case the evolving of the format of the proc file syste, would you please add some comments about how statName tuple is mapped to the res list?
Line 217:         res.extend([int(item) for item in args[1:]])
Line 218:         return dict(zip(statName, tuple(res)))
Line 219: 
Line 220:

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 3:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/888/ (1/2)

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 3:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/853/ (2/2)

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 3:

Build Successful

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

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

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Mark Wu has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 3: Looks good to me, but someone else must approve

(1 inline comment)

....................................................
File vdsm/utils.py
Line 213:         res.append(statline[procNameStart + 1:procNameEnd])
Line 214:         args = statline[procNameEnd + 2:].split()
Line 215:         res.append(args[0])
Line 216:         res.extend([int(item) for item in args[1:]])
Line 217:         # only 44 feilds are documented in man page while /proc/pid/stat has 52
according to kernel code, the other 8 fields are start_data,end_data,start_brk,arg_start,arg_end,env_start,env_end and exit_code. It's the process memory layout and exit_code.  Currently I don't think we need go that far. So the first 44 fields are enough. You could remove the comment.
Line 218:         return stat._make(res[:44])
Line 219: 
Line 220: 
Line 221: class TimedSample:

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Yaniv Bronhaim has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File tests/utilsTests.py
Line 52:         sproc = misc.execCmd(args, sync=False, sudo=False)
Line 53:         stats = utils.pidStat(sproc.pid)
Line 54:         pid = int(stats.pid)
Line 55:         # procName comes in the format of (procname)
Line 56:         name = stats.comm
you should add tests for more fields that we use to be sure pidStat works as we want, and i would add the change of pidStat to the patch comment.
Line 57:         self.assertEquals(pid, sproc.pid)
Line 58:         self.assertEquals(name, args[0])
Line 59:         sproc.kill()

....................................................
File vdsm/storage/misc.py
Line 1108:         pid = os.path.basename(path)
Line 1109:         yield int(pid)
Line 1110: 
Line 1111: 
Line 1112: def pgrep(value, field):
As I understand pgrep just returns pid of processes with specific name.. you can add more restrictions as owners but you don't use pgrep to match other fields except the name of process..

You can add another misc\util function for this purpose
Line 1113:     """
Line 1114:     grep processes whose given field's value matches specified value
Line 1115:     see man 5 proc '/proc/[pid]/stat' for fields description
Line 1116:     """

Line 1287: def killall(name, signum, group=False):
Line 1288:     exception = None
Line 1289:     knownPgs = set()
Line 1290:     pidList = pgrep(name)
Line 1291:     if len(pidList) == 0:
what about this call?
Line 1292:         raise OSError(errno.ESRCH,
Line 1293:                       "Could not find processes named `%s`" % name)
Line 1294: 
Line 1295:     for pid in pidList:

--
To view, visit http://gerrit.ovirt.org/11033
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2034b04a4787d2f222cb98dfe3ffcdbb2a94ebb8
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: Yaniv Bronhaim 
Gerrit-Reviewer: oVirt Jenkins CI Server

Saggi Mizrahi has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 3: Code-Review-1

(1 comment)

....................................................
File vdsm/storage/misc.py
Line 1108:         pid = os.path.basename(path)
Line 1109:         yield int(pid)
Line 1110: 
Line 1111: 
Line 1112: def pgrep(value, field):
I think using the existing method in conjunction with http://docs.python.org/2/library/itertools.html#itertools.ifilter would be a lot simpler to use and would solve more a diverse set of problems.
Line 1113:     """
Line 1114:     grep processes whose given field's value matches specified value
Line 1115:     see man 5 proc '/proc/[pid]/stat' for fields description
Line 1116:     """

Itamar Heim has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 3:

ping?

Yaniv Bronhaim has posted comments on this change.

Change subject: make misc.pgrep for general usage
......................................................................

Patch Set 3:

the part of pidStat was added as part of http://gerrit.ovirt.org/15024 , still this patch includes improvement for pgrep tool, but I don't see who needs that yet so I left it.

the patch can be abandoned imo

Itamar Heim has abandoned this change.

Change subject: make misc.pgrep for general usage
......................................................................

Abandoned