Re: [ptest-runner] Run ptests via stdbuf configured to line-buffering

[<Mikko.Rapeli@bmw.de>]

On Fri, Apr 05, 2019 at 07:59:41AM +0100, richard.purdie@linuxfoundation.org wrote:
> On Fri, 2019-04-05 at 06:54 +0000, Mikko.Rapeli@bmw.de wrote:
> > On Fri, Apr 05, 2019 at 07:46:00AM +0100, 
> > richard.purdie@linuxfoundation.org wrote:
> > > On Fri, 2019-04-05 at 06:16 +0000, Mikko.Rapeli@bmw.de wrote:
> > > > On Thu, Apr 04, 2019 at 10:48:17PM +0100, Richard Purdie wrote:
> > > > > On Thu, 2019-04-04 at 18:00 +0200, Alexander Kanavin wrote:
> > > > > > As ptest-runner communicates with child processes via
> > > > > > pipe2(),
> > > > > > the corresponding channels are not attached to a pty. In that
> > > > > > situation stdio facilities like printf() or fwrite() are
> > > > > > fully
> > > > > > buffered. If a ptest would use them, without bothering
> > > > > > to fflush() the output, ptest-runner will only receive what
> > > > > > was written by the child ptest process after a buffer gets
> > > > > > filled.
> > > > > > If the unit tests are proceeding slowly, this may mean that
> > > > > > ptest-runner will erroneously timeout due to an apparent lack
> > > > > > of
> > > > > > 'signs of life' from the child process.
> > > > > > 
> > > > > > stdbuf utility from coreutils adjusts the buffering to a
> > > > > > line-
> > > > > > buffered
> > > > > > one, and so ptest-runner will get the lines as soon as they
> > > > > > are
> > > > > > written.
> > > > > > 
> > > > > > Signed-off-by: Alexander Kanavin <alex.kanavin@gmail.com>
> > > > > > ---
> > > > > >  utils.c | 7 ++-----
> > > > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > > > 
> > > > > I'm a little torn on this. I noticed some of the run-ptest
> > > > > scripts
> > > > > use
> > > > > "| sed -u" whilst the one you were seeing problems with uses "|
> > > > > sed"
> > > > > without -u.
> > > > > 
> > > > > We may want to consider strongly recommending -u. I'm testing a
> > > > > patch
> > > > > with some tweaks like that in it...
> > > > 
> > > > Please no. I'm running images without sed and using busybox sed
> > > > instead, and that
> > > > doesn't support -u. I'd rather be compatible with sed from
> > > > busybox to
> > > > keep changes
> > > > to images minimal (e.g. install of additional packages) before
> > > > executing ptests.
> > > 
> > > The other alternative option being proposed is for ptest-runner to
> > > depend on coreutils which is worse?
> > 
> > GNU sed does not come from coreutils but from sed recipe.
> > 
> > Your call in the end. I just provided my point of view.
> 
> The original patch in this thread from Alex needs stdbuf which would
> add a coreutils depends to ptest-runner for everything as I understand
> it.

Ok, that too then. Then all hope is lost so lets include sed there as well :)

> > > I did test the -u option to sed in the openssh ptest runner and it
> > > did
> > > fix the problems we've been seeing.
> > > 
> > > I'm open to other alternatives but the -u option to sed is looking
> > > like
> > > the best one we have right now. These bugs are making our testing
> > > of
> > > ptests effectively useless and unpredictable so this is a serious
> > > issue...
> > 
> > Understood. I hope you could also add 'set -eux' to all ptest shell
> > scripts. Many of them seem to be missing shell script error handling
> > and failures like providing -u to busybox sed may go unnoticed.
> 
> Patches would be welcome to help clean up some of these scripts. I feel
> like I'm fighting a lonely battle trying to get any of this to work at
> times :(

We're ramping up with ptests and will provide fixes back to upstream.

> The good news is the automated monitoring of the ptest results should
> catch problems like that as if a chunk of ptests "disappear", the new
> testing should quickly highlight that.
> 
> (We should improve the scripts, we just also have another way to spot
> problems)

All this work is very much appreciated here in downstream! Thanks you very much!

-Mikko