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

[Tomáš Golembiovský]

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.
     - since Yaniv said updated cpopen won't be available for 4.0
     - and since we're aiming for subprocess.Popen in a log run anyway (if my
       understanding is correct)
     - and since the package for python-subprocess32 is available in our 4.0
       dependencies repo already

> 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)
> 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
> 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
> 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)
> 
> -- 
> Francesco Romani
> RedHat Engineering Virtualization R & D
> Phone: 8261328
> IRC: fromani


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