Re: [kvm-unit-tests PATCH 3/3] add timeout support

[Andrew Jones]

On Tue, Dec 22, 2015 at 07:02:21PM +0100, Radim Krčmář wrote:
> 2015-12-21 13:45-0600, Andrew Jones:
> > On Mon, Dec 21, 2015 at 06:04:20PM +0100, Radim Krčmář wrote:
> >> 2015-12-17 14:10-0600, Andrew Jones:
> >> > diff --git a/run_tests.sh b/run_tests.sh
> >> > @@ -21,6 +21,7 @@ function run()
> >> > +local timeout="${9:-$TIMEOUT}"
> >> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> >> > @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
> >> > +if [ "$timeout" ]; then
> >> > +timeout_cmd='timeout --foreground $timeout'
> >> 
> >> Both would be nicer if they took the TIMEOUT variable as an override.
> > 
> > Everything already takes TIMEOUT as an override, i.e.
> > 
> > TIMEOUT=3 ./run_tests.sh
> > 
> > and
> > 
> > TIMEOUT=3 arm/run arm/test.flat
> > 
> > will both already set a timeout for any test that didn't have a timeout
> > set in unittests.cfg, or wasn't run with run()/unittests.cfg.
> 
> Tests made with mkstandalone.sh ignore the TIMEOUT variable ...
> 
> >   Or, did
> > you mean that you'd prefer TIMEOUT to override the timeout in
> > unittests.cfg?
> 
> ... and yes, I think that we could have a
> - global timeout for all tests.  Something far longer than any tests
>   should take (2 minutes?).  To automatically handle random hangs.
> 
> - per-test timeout in unittests.cfg.  When the test is known to timeout
>   often and the usual time to fail is much shorter than the global
>   default.  (Shouldn't be used much.)
> 
> - TIMEOUT variable.  It has to override the global timeout and I think
>   that if we are ever going to use it, it's because we want something
>   weird.  Like using `TIMEOUT=0 ./run_tests.sh` to disable all
>   timeouts, prolonging/shortening timeouts because of a special
>   configuration, ...
>   Because we should improve our defaults otherwise.

OK, I'll do something allowing us to easily enable a long default
timeout.

> 
>   (I'd probably allow something as evil as `eval`ing the TIMEOUT, for
>unlikely stuff like TIMEOUT='$(( ${timeout:-10} / 2 ))')

I'd prefer to avoid the evil^Weval stuff... And the timeout duration can
already be a floating point.

> 
> >That does make some sense, in the case the one in the
> > config is longer than desired, however it also makes sense the way I
> > have it now when the one in the config is shorter than TIMEOUT (the
> > fallback default). I think I like it this way better.
> 
> Ok, the difference was negligible to begin with.
> 
> >> We already don't do that for accel and the patch seems ok in other
> >> regards,
> > 
> > Hmm, for accel I see a need for a patch allowing us to do
> > 
> > ACCEL=?? ./run_tests.sh
> 
> Btw. why do we have ACCEL when the project is *kvm*_unit_tests?

arm tests are sometimes tcg only. Hey, we'll take what we get for
arm, as we're sadly missing everything...

> 
> > as I already have for TIMEOUT. Also, for both I should add a
> > mkstandalone patch allowing
> > 
> > TIMEOUT=? ACCEL=? make standalone
> 
> I'd also handle TIMEOUT/ACCEL in resulting standalone tests.

OK

Thanks,
drew

> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 3/3] add timeout support

[Radim Krčmář]

2015-12-21 13:45-0600, Andrew Jones:
> On Mon, Dec 21, 2015 at 06:04:20PM +0100, Radim Krčmář wrote:
>> 2015-12-17 14:10-0600, Andrew Jones:
>> > diff --git a/run_tests.sh b/run_tests.sh
>> > @@ -21,6 +21,7 @@ function run()
>> > +local timeout="${9:-$TIMEOUT}"
>> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>> > @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
>> > +if [ "$timeout" ]; then
>> > +  timeout_cmd='timeout --foreground $timeout'
>> 
>> Both would be nicer if they took the TIMEOUT variable as an override.
> 
> Everything already takes TIMEOUT as an override, i.e.
> 
> TIMEOUT=3 ./run_tests.sh
> 
> and
> 
> TIMEOUT=3 arm/run arm/test.flat
> 
> will both already set a timeout for any test that didn't have a timeout
> set in unittests.cfg, or wasn't run with run()/unittests.cfg.

Tests made with mkstandalone.sh ignore the TIMEOUT variable ...

>   Or, did
> you mean that you'd prefer TIMEOUT to override the timeout in
> unittests.cfg?

... and yes, I think that we could have a
- global timeout for all tests.  Something far longer than any tests
  should take (2 minutes?).  To automatically handle random hangs.

- per-test timeout in unittests.cfg.  When the test is known to timeout
  often and the usual time to fail is much shorter than the global
  default.  (Shouldn't be used much.)

- TIMEOUT variable.  It has to override the global timeout and I think
  that if we are ever going to use it, it's because we want something
  weird.  Like using `TIMEOUT=0 ./run_tests.sh` to disable all
  timeouts, prolonging/shortening timeouts because of a special
  configuration, ...
  Because we should improve our defaults otherwise.

  (I'd probably allow something as evil as `eval`ing the TIMEOUT, for
   unlikely stuff like TIMEOUT='$(( ${timeout:-10} / 2 ))')

>That does make some sense, in the case the one in the
> config is longer than desired, however it also makes sense the way I
> have it now when the one in the config is shorter than TIMEOUT (the
> fallback default). I think I like it this way better.

Ok, the difference was negligible to begin with.

>> We already don't do that for accel and the patch seems ok in other
>> regards,
> 
> Hmm, for accel I see a need for a patch allowing us to do
> 
> ACCEL=?? ./run_tests.sh

Btw. why do we have ACCEL when the project is *kvm*_unit_tests?

> as I already have for TIMEOUT. Also, for both I should add a
> mkstandalone patch allowing
> 
> TIMEOUT=? ACCEL=? make standalone

I'd also handle TIMEOUT/ACCEL in resulting standalone tests.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 3/3] add timeout support

[Andrew Jones]

On Mon, Dec 21, 2015 at 06:04:20PM +0100, Radim Krčmář wrote:
> 2015-12-17 14:10-0600, Andrew Jones:
> > Signed-off-by: Andrew Jones 
> > ---
> > diff --git a/arm/run b/arm/run
> > @@ -75,10 +75,14 @@ chr_testdev+=' -device virtconsole,chardev=ctd -chardev 
> > testdev,id=ctd'
> >  M+=",accel=$ACCEL"
> >  command="$qemu $M -cpu $processor $chr_testdev"
> >  command+=" -display none -serial stdio -kernel"
> > -echo $command "$@"
> > +
> > +if [ "$TIMEOUT" ]; then
> > +   timeout_cmd="timeout --foreground $TIMEOUT"
> 
> (command="timeout --foreground $TIMEOUT $command" # to keep the clutter
>  down.)
> 
> > +fi
> 
> (We paste it three times, so I'd rather see this abstracted in a
>  "scripts/" library.)

Sounds good

> 
> > diff --git a/run_tests.sh b/run_tests.sh
> > @@ -21,6 +21,7 @@ function run()
> > +local timeout="${9:-$TIMEOUT}"
> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> > @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
> > +if [ "$timeout" ]; then
> > +   timeout_cmd='timeout --foreground $timeout'
> 
> Both would be nicer if they took the TIMEOUT variable as an override.

Everything already takes TIMEOUT as an override, i.e.

TIMEOUT=3 ./run_tests.sh

and

TIMEOUT=3 arm/run arm/test.flat

will both already set a timeout for any test that didn't have a timeout
set in unittests.cfg, or wasn't run with run()/unittests.cfg. Or, did
you mean that you'd prefer TIMEOUT to override the timeout in
unittests.cfg? That does make some sense, in the case the one in the
config is longer than desired, however it also makes sense the way I
have it now when the one in the config is shorter than TIMEOUT (the
fallback default). I think I like it this way better.

> We already don't do that for accel and the patch seems ok in other
> regards,

Hmm, for accel I see a need for a patch allowing us to do

ACCEL=?? ./run_tests.sh

as I already have for TIMEOUT. Also, for both I should add a
mkstandalone patch allowing

TIMEOUT=? ACCEL=? make standalone

Thanks,
drew

> 
> Reviewed-by: Radim Krčmář 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH 3/3] add timeout support

[Radim Krčmář]

2015-12-17 14:10-0600, Andrew Jones:
> Signed-off-by: Andrew Jones 
> ---
> diff --git a/arm/run b/arm/run
> @@ -75,10 +75,14 @@ chr_testdev+=' -device virtconsole,chardev=ctd -chardev 
> testdev,id=ctd'
>  M+=",accel=$ACCEL"
>  command="$qemu $M -cpu $processor $chr_testdev"
>  command+=" -display none -serial stdio -kernel"
> -echo $command "$@"
> +
> +if [ "$TIMEOUT" ]; then
> + timeout_cmd="timeout --foreground $TIMEOUT"

(command="timeout --foreground $TIMEOUT $command" # to keep the clutter
 down.)

> +fi

(We paste it three times, so I'd rather see this abstracted in a
 "scripts/" library.)

> diff --git a/run_tests.sh b/run_tests.sh
> @@ -21,6 +21,7 @@ function run()
> +local timeout="${9:-$TIMEOUT}"
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
> +if [ "$timeout" ]; then
> + timeout_cmd='timeout --foreground $timeout'

Both would be nicer if they took the TIMEOUT variable as an override.
We already don't do that for accel and the patch seems ok in other
regards,

Reviewed-by: Radim Krčmář 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html