Nir Soffer has posted comments on this change. Change subject: Introducing context manager to terminate async processes on internal fail ......................................................................
Patch Set 5: (7 comments) https://gerrit.ovirt.org/#/c/51407/5//COMMIT_MSG Commit Message: Line 15: p.wait(5) Line 16: ... Line 17: p.communicate() Line 18: Line 19: print p.stderr The example code is wrong, you don't call both wait() and communicate(), and you don't print proc.stderr since this is a StringIO instance, not a string. Lets show something like this: with terminating(proc): if proc.wait(30) is None: raise Timeout() And explain that proc will be killed on timeout, and if we cannot kill it, zombiereaper will wait for it. Line 20: ... Line 21: Line 22: return Line 23: https://gerrit.ovirt.org/#/c/51407/5/lib/vdsm/commands.py File lib/vdsm/commands.py: Line 43: BUFFSIZE = 1024 Line 44: Line 45: Line 46: @contextmanager Line 47: def terminating(proc): And lets add tests checking that - process is killed in normal run - process is killed when exception is raised in the with block - the original error is raised - if process did not exit after kill, zombiereaper should get the process pid (maybe mock it) Line 48: try: Line 49: yield proc Line 50: finally: Line 51: if proc.returncode is None: Line 47: def terminating(proc): Line 48: try: Line 49: yield proc Line 50: finally: Line 51: if proc.returncode is None: We must use proc.poll() - returncode is updated only when you call communicate(), wait() or poll(). Line 52: try: Line 53: proc.kill() Line 54: except Exception: Line 55: logging.exception("Cannot kill process %n" % proc.pid) Line 48: try: Line 49: yield proc Line 50: finally: Line 51: if proc.returncode is None: Line 52: try: Lets put everything in the finally block in the try except, so we raise the original error after any failure in the finally block: try: yield finally: try: check and kill process, pass to zombiereaper, etc. except Exception: logging.exception(...) Line 53: proc.kill() Line 54: except Exception: Line 55: logging.exception("Cannot kill process %n" % proc.pid) Line 56: if proc.returncode is None: Line 52: try: Line 53: proc.kill() Line 54: except Exception: Line 55: logging.exception("Cannot kill process %n" % proc.pid) Line 56: if proc.returncode is None: Should use poll() Line 57: zombiereaper.autoReapPID(proc.pid) Line 58: Line 59: Line 60: def execCmd(command, sudo=False, cwd=None, data=None, raw=False, Line 97: try: Line 98: p.kill() Line 99: except Exception: Line 100: execCmdLogger.error("Process %n failed to die" % p.pid) Line 101: six.reraise(*exc_info) This should be separate change, lets keep patch small as possible. Line 102: Line 103: return p Line 104: Line 105: (out, err) = p.communicate(data) Line 363: deathSignal=deathSignal) Line 364: Line 365: with terminating(proc): Line 366: if not proc.wait(cond=stop): Line 367: raise ActionStopped() Can remain in the terminating patch, but I'll need more time to understand if this is correct, not sure what cond does, and what stop is doing. It may be possible that the caller is responsible for killing this command. You may like to separate this to another patch so we can take the context manager patch now. Line 368: Line 369: out = stripNewLines(proc.stdout) Line 370: err = stripNewLines(proc.stderr) Line 371: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
