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

[Francesco Romani]

----- Original Message -----
> From: "Nir Soffer" <nsoffer at redhat.com>
> To: "Tomáš Golembiovský" <tgolembi at redhat.com>
> Cc: "Francesco Romani" <fromani at redhat.com>, "devel" <devel at ovirt.org>, "Yaniv Bronheim" <ybronhei at redhat.com>,
> "Shahar Havivi" <shaharh at redhat.com>
> Sent: Thursday, July 21, 2016 2:17:19 PM
> Subject: Re: [ovirt-devel] execCmd() and storing stdout and stderr in log file
> 
> 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:
[...]
> >> 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.

Tomas, could you please check about the availability in RHEL?
So, turns out we have only one option (and half? :)) which is good, it simplifies
the things.

> >> 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.

Fine, and thanks for the examples.

> 
> >> 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

Indeed it is, going to abandon 60660 and bury it very deep as soon as we reach consensus.

> 
> >> 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.

ok, let's keep this as very last resort.

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

Indeed they are, but I'm less worried about those since are been already solved
in the past and we already have tools to deal with them

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

I think we though about this (me and Tomas) in the past, and that was the plan;
Anyway I agree with this, let's make this our plan if it wasn't already :)

Thanks and bests,

-- 
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani