Change in vdsm[master]: Introducing context manager to terminate async processes on ...

nsoffer at redhat.com nsoffer at redhat.com
Tue Jan 12 21:04:53 UTC 2016


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 <ybronhei at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list