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

----- Original Message ----- > From: "Nir Soffer" <nsoffer(a)redhat.com&gt; > To: "Yaniv Bronheim" <ybronhei(a)redhat.com&gt; > Cc: "Shahar Havivi" <shaharh(a)redhat.com&gt;, "Richard Jones" <rjones(a)redhat.com&gt;, "devel" <devel(a)ovirt.org&gt; > 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(a)redhat.com&gt; wrote: > > > > On Tue, Jul 19, 2016 at 4:56 PM, Tomáš Golembiovský <tgolembi(a)redhat.com&gt; > > wrote: > >> > >> On Thu, 14 Jul 2016 17:25:28 +0300 > >> Nir Soffer <nsoffer(a)redhat.com&gt; 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(a)redhat.com&gt; > AuthorDate: Wed Oct 14 15:40:07 2015 +0300 > Commit: Yaniv Bronhaim <ybronhei(a)redhat.com&gt; > 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(a)redhat.com&gt; 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