Ensure processes death by terminating decorator - https://gerrit.ovirt.org/51407
Hi guys, Following the work to omit deathSignal attribute from our cpopen implementation we posted https://gerrit.ovirt.org/51407 which is ready for use. Currently locations that should use it are: (I wrote above who I expect to check the area and post a patch for that - we'll discuss it during next vdsm-sync to follow the work) shavivi: vdsm/v2v.py - in _start_virt_v2v you return aysnProc that should call kill() on fail fromani: vdsm_hooks/checkimages/before_vm_start.py - in checkImage - the code looks ok, but check if not better to use the terminating decorator.. I think it will be nicer nsoffer: vdsm/storage/mount.py - good looks ok, I prefer to use terminator there vdsm/storage/iscsiadm.py vdsm/storage/imageSharing.py vdsm/storage/hba.py -good handling, use terminator vdsm/storage/blockSD.py please check your usage with the returned process and see that you're not depending on deathSignal for it to die properly on crush some places define deathSignal for no reason, the call is sync - please remove those places: nsoffer: lib/vdsm/qemuimg.py vdsm/storage/curlImgWrap.py vdsm/storage/storage_mailbox.py vdsm/storage/misc.py fromani: lib/vdsm/virtsparsify.py ybronhei: vdsm/API.py If you can't get to it in a reasonable time, add the task to the list [1] and someone else will be it up. Please try to go over before the sync call. [1] - https://docs.google.com/spreadsheets/d/180F-C1jU54ajUn7TuR-NwrKRZY1IiZI1Z8U5... -- *Yaniv Bronhaim.*
----- Original Message -----
From: "Yaniv Bronheim" <ybronhei@redhat.com> To: "devel" <devel@ovirt.org>, "Shahar Havivi" <shavivi@redhat.com>, "Francesco Romani" <fromani@redhat.com>, "Nir Soffer" <nsoffer@redhat.com> Sent: Monday, January 18, 2016 11:01:10 AM Subject: Ensure processes death by terminating decorator - https://gerrit.ovirt.org/51407
Hi guys,
Following the work to omit deathSignal attribute from our cpopen implementation we posted https://gerrit.ovirt.org/51407 which is ready for use. Currently locations that should use it are: (I wrote above who I expect to check the area and post a patch for that - we'll discuss it during next vdsm-sync to follow the work)
fromani: vdsm_hooks/checkimages/before_vm_start.py - in checkImage - the code looks ok, but check if not better to use the terminating decorator.. I think it will be nicer
Fair enough, posted https://gerrit.ovirt.org/52349
some places define deathSignal for no reason, the call is sync - please remove those places: [...] fromani: lib/vdsm/virtsparsify.py
Done in https://gerrit.ovirt.org/52357 -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
any updates about that? https://gerrit.ovirt.org/#/c/52357/ - this can be verified and get it https://gerrit.ovirt.org/#/c/52349/ - got tiny comment about unneeded kill call https://gerrit.ovirt.org/51407 - Nir can merge Nir - please review https://gerrit.ovirt.org/52646 or take over and update soon what the plans regarding the async usages in vdsm/storage/mount.py vdsm/storage/iscsiadm.py vdsm/storage/imageSharing.py vdsm/storage/hba.py vdsm/storage/blockSD.py and v2v.py I prefer not to wait for that too long - we can remove the deathSignal usages there, and continue with https://gerrit.ovirt.org/#/c/48384 Please also check if you can take over the re-implementation of async proc ( https://gerrit.ovirt.org/49441) as you (storage operations) are the main and only user of it, and it should fit Popen proc. On Mon, Jan 18, 2016 at 3:12 PM, Francesco Romani <fromani@redhat.com> wrote:
----- Original Message -----
From: "Yaniv Bronheim" <ybronhei@redhat.com> To: "devel" <devel@ovirt.org>, "Shahar Havivi" <shavivi@redhat.com>, "Francesco Romani" <fromani@redhat.com>, "Nir Soffer" <nsoffer@redhat.com> Sent: Monday, January 18, 2016 11:01:10 AM Subject: Ensure processes death by terminating decorator - https://gerrit.ovirt.org/51407
Hi guys,
Following the work to omit deathSignal attribute from our cpopen implementation we posted https://gerrit.ovirt.org/51407 which is ready for use. Currently locations that should use it are: (I wrote above who I expect to check the area and post a patch for that - we'll discuss it during next vdsm-sync to follow the work)
fromani: vdsm_hooks/checkimages/before_vm_start.py - in checkImage - the code looks ok, but check if not better to use the terminating decorator.. I think it will be nicer
Fair enough, posted https://gerrit.ovirt.org/52349
some places define deathSignal for no reason, the call is sync - please remove those places: [...] fromani: lib/vdsm/virtsparsify.py
Done in https://gerrit.ovirt.org/52357
-- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
-- *Yaniv Bronhaim.*
On Sat, Jan 23, 2016 at 7:11 PM, Yaniv Bronheim <ybronhei@redhat.com> wrote:
any updates about that?
https://gerrit.ovirt.org/#/c/52357/ - this can be verified and get it
Should wait for execCmd (it should use the new terminating() context manager to ensure process is killed on errors).
https://gerrit.ovirt.org/#/c/52349/ - got tiny comment about unneeded kill call
Not ready yet
https://gerrit.ovirt.org/51407 - Nir can merge
Waiting for Piotr to ack the tests (https://gerrit.ovirt.org/52362)
Nir - please review https://gerrit.ovirt.org/52646 or take over
Should wait for execCmd
and update soon what the plans regarding the async usages in vdsm/storage/mount.py vdsm/storage/iscsiadm.py vdsm/storage/imageSharing.py vdsm/storage/hba.py vdsm/storage/blockSD.py
I will not have time for this at least after devconf, and even after devconf we are busy with spm removal (probably not for 4.0) and ovirt-image (for 4.0). I will add this to the storage todo list for now.
and v2v.py
v2v need to read only from stdout. If v2v is not expected to write more then 64k errors to stderr (highly unlikely), we don't need to use AsyncProc. Can create the process like we do in qemuimg.py, and read from the process stdout. If we want to handle the unlikely case of huge error output, we can use CommandStream to collect the output from both streams, but we will have to the change the output parser to be driven by output callback, instead of reading lines from stdout.
I prefer not to wait for that too long - we can remove the deathSignal usages there, and continue with https://gerrit.ovirt.org/#/c/48384
I think we should continue with other Python 3 porting efforts until we can eliminate cpopen non-standard apis.
Please also check if you can take over the re-implementation of async proc (https://gerrit.ovirt.org/49441) as you (storage operations) are the main and only user of it, and it should fit Popen proc.
I think the current patch does not need too much work - only implement wait with a timeout. What about the subproces32 project you found? it looks like the right direction: https://pypi.python.org/pypi/subprocess32/ The author is Python core developer: https://github.com/python/cpython/commits?author=gpshead We can drop cpopen, AsyncProc, AsyncProcOperation (used only in iscsi), and use this module to get Python 3 features and reliability, and be compatible with both Python 3 (fedora 24?) and 2 (el 7.x).
On Mon, Jan 18, 2016 at 3:12 PM, Francesco Romani <fromani@redhat.com> wrote:
----- Original Message -----
From: "Yaniv Bronheim" <ybronhei@redhat.com> To: "devel" <devel@ovirt.org>, "Shahar Havivi" <shavivi@redhat.com>, "Francesco Romani" <fromani@redhat.com>, "Nir Soffer" <nsoffer@redhat.com> Sent: Monday, January 18, 2016 11:01:10 AM Subject: Ensure processes death by terminating decorator - https://gerrit.ovirt.org/51407
Hi guys,
Following the work to omit deathSignal attribute from our cpopen implementation we posted https://gerrit.ovirt.org/51407 which is ready for use. Currently locations that should use it are: (I wrote above who I expect to check the area and post a patch for that - we'll discuss it during next vdsm-sync to follow the work)
fromani: vdsm_hooks/checkimages/before_vm_start.py - in checkImage - the code looks ok, but check if not better to use the terminating decorator.. I think it will be nicer
Fair enough, posted https://gerrit.ovirt.org/52349
some places define deathSignal for no reason, the call is sync - please remove those places: [...] fromani: lib/vdsm/virtsparsify.py
Done in https://gerrit.ovirt.org/52357
-- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
-- Yaniv Bronhaim.
On Sat, Jan 23, 2016 at 8:05 PM, Nir Soffer <nsoffer@redhat.com> wrote:
On Sat, Jan 23, 2016 at 7:11 PM, Yaniv Bronheim <ybronhei@redhat.com> wrote:
any updates about that?
https://gerrit.ovirt.org/#/c/52357/ - this can be verified and get it
Should wait for execCmd (it should use the new terminating() context manager to ensure process is killed on errors).
https://gerrit.ovirt.org/#/c/52349/ - got tiny comment about unneeded kill
call
Not ready yet
https://gerrit.ovirt.org/51407 - Nir can merge
Waiting for Piotr to ack the tests (https://gerrit.ovirt.org/52362)
Nir - please review https://gerrit.ovirt.org/52646 or take over
Should wait for execCmd
and update soon what the plans regarding the async usages in vdsm/storage/mount.py vdsm/storage/iscsiadm.py vdsm/storage/imageSharing.py vdsm/storage/hba.py vdsm/storage/blockSD.py
I will not have time for this at least after devconf, and even after devconf we are busy with spm removal (probably not for 4.0) and ovirt-image (for 4.0).
I will add this to the storage todo list for now.
and v2v.py
v2v need to read only from stdout. If v2v is not expected to write more then 64k errors to stderr (highly unlikely), we don't need to use AsyncProc. Can create the process like we do in qemuimg.py, and read from the process stdout.
If we want to handle the unlikely case of huge error output, we can use CommandStream to collect the output from both streams, but we will have to the change the output parser to be driven by output callback, instead of reading lines from stdout.
I prefer not to wait for that too long - we can remove the deathSignal usages there, and continue with https://gerrit.ovirt.org/#/c/48384
I think we should continue with other Python 3 porting efforts until we can eliminate cpopen non-standard apis.
Please also check if you can take over the re-implementation of async proc (https://gerrit.ovirt.org/49441) as you (storage operations) are the main and only user of it, and it should fit Popen proc.
I think the current patch does not need too much work - only implement wait with a timeout.
What about the subproces32 project you found? it looks like the right direction: https://pypi.python.org/pypi/subprocess32/
The author is Python core developer:
https://github.com/python/cpython/commits?author=gpshead
We can drop cpopen, AsyncProc, AsyncProcOperation (used only in iscsi), and use this module to get Python 3 features and reliability, and be compatible with both Python 3 (fedora 24?) and 2 (el 7.x).
Maybe, we first need to remove our non standard usages, then we might be able to change implementation easily and see the results.. first thing first. Try to estimate when your team will be able to proceed with storage parts, I already started with https://gerrit.ovirt.org/#/c/52646/ , if I'll get to more parts it will be great.. but we need to have more force here
On Mon, Jan 18, 2016 at 3:12 PM, Francesco Romani <fromani@redhat.com> wrote:
----- Original Message -----
From: "Yaniv Bronheim" <ybronhei@redhat.com> To: "devel" <devel@ovirt.org>, "Shahar Havivi" <shavivi@redhat.com>, "Francesco Romani" <fromani@redhat.com>, "Nir Soffer" <nsoffer@redhat.com> Sent: Monday, January 18, 2016 11:01:10 AM Subject: Ensure processes death by terminating decorator - https://gerrit.ovirt.org/51407
Hi guys,
Following the work to omit deathSignal attribute from our cpopen implementation we posted https://gerrit.ovirt.org/51407 which is
for use. Currently locations that should use it are: (I wrote above who I expect to check the area and post a patch for
- we'll discuss it during next vdsm-sync to follow the work)
fromani: vdsm_hooks/checkimages/before_vm_start.py - in checkImage - the code looks ok, but check if not better to use the terminating decorator.. I think it will be nicer
Fair enough, posted https://gerrit.ovirt.org/52349
some places define deathSignal for no reason, the call is sync -
ready that please
remove those places: [...] fromani: lib/vdsm/virtsparsify.py
Done in https://gerrit.ovirt.org/52357
-- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani
-- Yaniv Bronhaim.
-- *Yaniv Bronhaim.*
participants (3)
-
Francesco Romani -
Nir Soffer -
Yaniv Bronheim