Change in vdsm[master]: Remove usage of deathSignal

nsoffer at redhat.com nsoffer at redhat.com
Sun Nov 15 17:57:41 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: Remove usage of deathSignal
......................................................................


Patch Set 3: Code-Review-1

(9 comments)

The current code is not ready for this change.

https://gerrit.ovirt.org/#/c/48121/3//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2015-11-12 14:32:50 +0200
Line 6: 
Line 7: Remove usage of deathSignal
Line 8: 
Line 9: deathSignal is an additional logic that python-cpopen addeds Popen
- logic -> argument
- adds Popen -> adds to Popen
Line 10: interface that will allow the initiator of subprocess to send automatic
Line 11: signal to the subprocess on crash. Using systemd does it for us and kill
Line 12: all subprocess in the same cgroup of the initiator, therefore we do not
Line 13: require to have this additional logic anymore.


Line 7: Remove usage of deathSignal
Line 8: 
Line 9: deathSignal is an additional logic that python-cpopen addeds Popen
Line 10: interface that will allow the initiator of subprocess to send automatic
Line 11: signal to the subprocess on crash. Using systemd does it for us and kill
This is not correct. Should be:  cause the child process to receive this signal when the thread creating it is terminated.
Line 12: all subprocess in the same cgroup of the initiator, therefore we do not
Line 13: require to have this additional logic anymore.
Line 14: 
Line 15: Change-Id: Ice7fc30ec90adf8055eb35a8c67c07f514030265


Line 9: deathSignal is an additional logic that python-cpopen addeds Popen
Line 10: interface that will allow the initiator of subprocess to send automatic
Line 11: signal to the subprocess on crash. Using systemd does it for us and kill
Line 12: all subprocess in the same cgroup of the initiator, therefore we do not
Line 13: require to have this additional logic anymore.
Systemd does not do exactly the same thing.

With deathSignal, the child process will receive the signal when the parent process/thread terminates.

With systemd cgroup-based termination, systemd terminates all the child processes when the main process is terminated.

So without deathSignal, we may have live child process after the thread starting them terminated. For example, if we start a thread for running a child process, and this thread terminates in an unclean way, the child process will not be killed, and will live until vdsm restarts or crashes, which may be lot of time.

To remove deathSignal, we should ensure using try finally that any child process started by a thread is terminated when the thread exist.
Line 14: 
Line 15: Change-Id: Ice7fc30ec90adf8055eb35a8c67c07f514030265


https://gerrit.ovirt.org/#/c/48121/3/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 209
Line 210
Line 211
Line 212
Line 213
The user of this class must use try finally to ensure that an instance is terminate before we can safely remove deathSignal.


https://gerrit.ovirt.org/#/c/48121/3/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 649
Line 650
Line 651
Line 652
Line 653
If writing to stdin fails, we are leaving zombie process. This must be protect with try expect.


Line 653
Line 654
Line 655
Line 656
Line 657
Please check that communicate cannot fail, leaving process running. If this may happen, we should protect it with try finally.


Line 681
Line 682
Line 683
Line 684
Line 685
Can this raise, leaving process running?


https://gerrit.ovirt.org/#/c/48121/3/vdsm/storage/imageSharing.py
File vdsm/storage/imageSharing.py:

Line 94
Line 95
Line 96
Line 97
Line 98
This is a good example of the code needed when using execCmd in async mode. On any failure, the process is killed.

However adding such code anywhere is not a good idea. We should add a context manger for terminating started processes. Something like:

    with execCmd(cmd, sync=False...) as p:
       ...

Or:
    p = execCmd(cmd, sync=False...)
    with terminating(p):
        ...


https://gerrit.ovirt.org/#/c/48121/3/vdsm/v2v.py
File vdsm/v2v.py:

Line 412
Line 413
Line 414
Line 415
Line 416
Another good example of code that ensuring that we don't leave stale process on errors.


-- 
To view, visit https://gerrit.ovirt.org/48121
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7fc30ec90adf8055eb35a8c67c07f514030265
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce 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