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

[Francesco Romani]

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