[ovirt-devel] execCmd() and storing stdout and stderr in log file

[Nir Soffer]

On Thu, Jul 21, 2016 at 1:51 PM, Tomáš Golembiovský <tgolembi at redhat.com> wrote:
> On Thu, 21 Jul 2016 03:26:44 -0400 (EDT)
> Francesco Romani <fromani at redhat.com> wrote:
>
>> ----- Original Message -----
>> > From: "Nir Soffer" <nsoffer at redhat.com>
>> > To: "Yaniv Bronheim" <ybronhei at redhat.com>
>> > Cc: "Shahar Havivi" <shaharh at redhat.com>, "Richard Jones" <rjones at redhat.com>, "devel" <devel at ovirt.org>
>> > Sent: Tuesday, July 19, 2016 4:18:31 PM
>> > Subject: Re: [ovirt-devel] execCmd() and storing stdout and stderr in log   file
>> >
>> > On Tue, Jul 19, 2016 at 5:07 PM, Yaniv Bronheim <ybronhei at redhat.com> wrote:
>> > >
>> > > On Tue, Jul 19, 2016 at 4:56 PM, Tomáš Golembiovský <tgolembi at redhat.com>
>> > > wrote:
>> > >>
>> > >> On Thu, 14 Jul 2016 17:25:28 +0300
>> > >> Nir Soffer <nsoffer at redhat.com> wrote:
>> > >>
>> > >> > After https://gerrit.ovirt.org/#/c/46733/ you should be able to create
>> > >> > the pipeline in python like this:
>> > >> >
>> > >> >     v2v = Popen(["virt-v2v", ...], stdout=PIPE, stderr=STDOUT)
>> > >> >     tee = Popen(["tee", "-a", logfile], stdin=v2v.stdout, stdout=PIPE,
>> > >> > stderr=PIPE)
>> > >> >
>> > >> > Now we can read output from tee.stdout, and when tee is finished, we can
>> > >> > wait
>> > >> > for v2v to get the exit code.
>> > >> >
>> > >> > Since all output would go to tee stdout and stderr may only contain tee
>> > >> > usage
>> > >> > errors, we don't need to use AsyncProc, making this code python 3
>> > >> > compatible.
>> > >>
>> > >>
>> > >> Yes, this may actualy work. And do we plan to adopt the cpopen 1.4.1,
>> > >> where
>> > >> this is fixed, in VDSM?
>> > >
>> > >
>> > > cpopen 1.5.1 - and yes but not in ovirt-4.0
>> >
>> > It is included in 1.4.1, released ages ago.
>> >
>> > $ git log --oneline --decorate
>> > 826b5ef (HEAD -> master, origin/master, origin/HEAD) Raise version for 1.5
>> > build
>> > a9d2a72 (inherit-fds) Add tests for disabling the close-on-exec flag
>> > 7163e79 Do not close inherited fds from parent
>> > 1b170fb (gerrit/master) Raising release number and tagging cpopen 1.4.1
>> > 69ae841 Sort imports to make it easier to add new imports
>> > 1116830 Simplify tests using CPopen.communicate()
>> > 58e1b43 Remove unneeded and wrong retry after fork
>> > fbf9ff3 Remove wrong and unneeded retry for execv
>> > 74ee4a0 Do not retry close() after interrupt
>> > 1abde2b Use _exit to terminate child after failure
>> > 8709c42 Fix incorrect fd closing
>> >
>> > $ git show 1b170fb --format=fuller
>> > commit 1b170fb8f2d204457ca66e53936fd3a059f4ed4b
>> > Author:     Yaniv Bronhaim <ybronhei at redhat.com>
>> > AuthorDate: Wed Oct 14 15:40:07 2015 +0300
>> > Commit:     Yaniv Bronhaim <ybronhei at redhat.com>
>> > CommitDate: Wed Oct 14 09:36:31 2015 -0400
>> >
>> >     Raising release number and tagging cpopen 1.4.1
>> >
>> >     Recent patches include only bug fixes. see commit messages
>> >
>> >     Change-Id: Ia1d32dd5bd7f60681b64251cef217c8d1e41b14b
>> >     Signed-off-by: Yaniv Bronhaim <ybronhei at redhat.com>
>>
>> Thanks to everyone for comments and insights.
>>
>> I think is time to wrap up so Tomas can finish his work.
>>
>> So, the options on the table are:
>> 1. Build pipelines in python (Nir's approach above)
>>   - looks like the the fix is in cpopen 1.4.1, available
>
> X. One more option that occured to me is: use Nir's approach but use
>    subprocess.Popen directly.

Until we integrate with subprocess32, you should use compat.CPopen.

When subprocess32 is ready, the correct way would be compat.Popen,
hiding the subprocess32 import on Python 2.7.

>      - since Yaniv said updated cpopen won't be available for 4.0

We don't need the update for 4.0, cpopen 1.4.1, available in 4.0 should be
good enough.

>      - and since we're aiming for subprocess.Popen in a log run anyway (if my
>        understanding is correct)

Correct, but we did not integrated with it yet

>      - and since the package for python-subprocess32 is available in our 4.0
>        dependencies repo already

Really?

I think subprocess32 is not available yet on rhel, so we cannot use it yet.

>> 2. add options to execCmd to redirect stderr
>>    - make execCmd even bigger, makes everyone grumpy
>>    - could require a couple of simple fixes (talked with
>>      Tomas privately, nothing groundbreaking - like one between stdout
>>      and stderr could be None and we must cope with that)

This is not an option, execCmd is too big and ugly today, we are not going
to add more features to it.

This helper is the way to use for the common case of running a process,
collecting the data from both stdout and stderr, and returning the results.

Code that have special needs should not use execCmd.

See qemuimg.QemuImgOperation, and storage.check.DirectioChecker for
examles.

You should use the helpers in cmdutils for consistent logging when starting
and finishing a process.

>> 3. DUP execCmd just for v2v and just to avoid making it more complex
>>    - see https://gerrit.ovirt.org/#/c/60660/
>>    - even worse than extending execCmd?
>>    - still needs the fixes above

No, one is enough

>> 4. use shell tricks and reuse existing execCmd
>>    - reviewers/future selves needs to wrap their head on that shell magic
>>    - PERHAPS fragile? I don't really know

I would not use the shell if I don't have to.

>> 5. something else I forgot?
>>
>>
>> I think the approaches here are ordered by likeness and feasibility,
>> so I'd actually try in this order (1->2->3->4)
>>
>> Please ACK/NACK the above, so Tomas can conclude this patch
>> for next monday (20160725)

There are other issues - where do you save the import log
(per-import log file?, v2v.log?) and how the logs are deleted.

Maybe keep per-import log files, in /var/log/vdsm/v2v, and use logrotate
configuration to rotate them?

Nir