Nir Soffer has posted comments on this change. Change subject: Introducing context manager to terminate async processes on internal fail ......................................................................
Patch Set 8: (2 comments) https://gerrit.ovirt.org/#/c/51407/8/lib/vdsm/commands.py File lib/vdsm/commands.py: Line 41: BUFFSIZE = 1024 Line 42: Line 43: Line 44: @contextmanager Line 45: def terminating(proc): Lets move this to utils, so we can test this code with Python 3 *now*. It does not depend on exeCmd, it should work with CPopen, Popen and AsyncProc. Line 46: try: Line 47: yield proc Line 48: finally: Line 49: try: Line 47: yield proc Line 48: finally: Line 49: try: Line 50: if proc.poll() is None: Line 51: proc.kill() After you call kill, you should check if the process is still running. Successful kill (using python2 and python2 sematics) is sending signal to the process. If the process is in D state, it will not get the signal, but kill() will succeed. So we need: finally: try: if proc.poll() is None: proc.kill() if proc.poll() is None: zombiereaper.autoReapPID(proc.pid) except Exception: logging.exception("Failed to kill process %d" % proc.pid) zombiereaper.autoReapPID(proc.pid) And it woujld be nice to log debug message before killing a process, something like - Terminating process pid=1234 Line 52: except Exception: Line 53: logging.exception("Failed to kill process %n" % proc.pid) Line 54: zombiereaper.autoReapPID(proc.pid) Line 55: -- To view, visit https://gerrit.ovirt.org/51407 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I003cce39d62dba5644937fafaf2c6e24c526c075 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
