Nir Soffer has posted comments on this change. Change subject: Introducing context manager to terminate async processes on internal fail ......................................................................
Patch Set 1: (2 comments) https://gerrit.ovirt.org/#/c/51407/1/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 704: Line 705: def terminator(proc, logger): Line 706: ''' Line 707: A context manager for taking care for killing process on failure. Line 708: The first exception will be remembered and re-raised. > but in most of the places you just want to kill on error, and let the proce No, when we run a process we always want to terminate it. Normally, we wait until it terminates, but if any unhandled exception happen, we want to kill the process before we raise the exception. Line 709: Line 710: Sample usage: Line 711: proc = ExecCmd('cmd', sync=False) Line 712: with terminator(proc, logger) as rollback: Line 733: try: Line 734: proc.kill() Line 735: except Exception: Line 736: self._log.error("Failed to kill subprocess") Line 737: six.reraise(*exc_info) > I don't think I understand what you suggest. where do you plan to use such In all the place where we use sync=False, for example imageSharing.py, iscsi.py, hba.py, and here (e.g. watchCmd). This is not useful to execCmd itself which is a special case. Line 738: Line 739: Line 740: def traceback(on="", msg="Unhandled exception"): Line 741: """ -- 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: 1 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
