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

[Tomáš Golembiovský]

On Thu, 21 Jul 2016 08:24:57 -0400 (EDT)
Francesco Romani <fromani at redhat.com> wrote:

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

I still see only 1.4 in repos. But if there will be 1.4.1 for 4.0 it is
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.

It turned out the python-subprocess32 is available from our
centos-ovirt40-candidate[1] repo. Not in RHEL as such. 


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

I didn't know that.

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

Logs will be stored in /var/log/vdsm/imports.

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

I didn't bother with setup for logrotate yet. The import process is not
something happening automatically or regularly. It's unlikely the user
will end up with full /var partition because of that.

If you believe it's necessary it can be done.


[1] http://cbs.centos.org/repos/virt7-ovirt-40-candidate/x86_64/os/

-- 
Tomáš Golembiovský <tgolembi at redhat.com>