diff mbox series

[U] UBUNTU: SAUCE: selftests: seccomp: bump up timeout to 5min

Message ID YMnARV0OCwokgo9Z@xps-13-7390
State New
Headers show
Series [U] UBUNTU: SAUCE: selftests: seccomp: bump up timeout to 5min | expand

Commit Message

Andrea Righi June 16, 2021, 9:11 a.m. UTC
DEBUG| [stdout] # selftests: seccomp: seccomp_benchmark
 DEBUG| [stdout] # net.core.bpf_jit_enable = 1
 DEBUG| [stdout] # net.core.bpf_jit_harden = 0
 DEBUG| [stdout] #
 DEBUG| [stdout] not ok 2 selftests: seccomp: seccomp_benchmark # TIMEOUT 120 seconds

This test can easily fail if the testing environment is a bit
overloaded, so bump up the timeout to 5min to prevent false positive
failures.

Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---
 tools/testing/selftests/seccomp/settings | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Colin Ian King June 16, 2021, 9:17 a.m. UTC | #1
On 16/06/2021 10:11, Andrea Righi wrote:
>  DEBUG| [stdout] # selftests: seccomp: seccomp_benchmark
>  DEBUG| [stdout] # net.core.bpf_jit_enable = 1
>  DEBUG| [stdout] # net.core.bpf_jit_harden = 0
>  DEBUG| [stdout] #
>  DEBUG| [stdout] not ok 2 selftests: seccomp: seccomp_benchmark # TIMEOUT 120 seconds
> 
> This test can easily fail if the testing environment is a bit
> overloaded, so bump up the timeout to 5min to prevent false positive
> failures.
> 
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> ---
>  tools/testing/selftests/seccomp/settings | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/seccomp/settings b/tools/testing/selftests/seccomp/settings
> index 6091b45d226b..694d70710ff0 100644
> --- a/tools/testing/selftests/seccomp/settings
> +++ b/tools/testing/selftests/seccomp/settings
> @@ -1 +1 @@
> -timeout=120
> +timeout=300
> 
Makes sense.

Acked-by: Colin Ian King <colin.king@canonical.com>
Colin Ian King June 16, 2021, 9:19 a.m. UTC | #2
On 16/06/2021 10:17, Colin Ian King wrote:
> On 16/06/2021 10:11, Andrea Righi wrote:
>>  DEBUG| [stdout] # selftests: seccomp: seccomp_benchmark
>>  DEBUG| [stdout] # net.core.bpf_jit_enable = 1
>>  DEBUG| [stdout] # net.core.bpf_jit_harden = 0
>>  DEBUG| [stdout] #
>>  DEBUG| [stdout] not ok 2 selftests: seccomp: seccomp_benchmark # TIMEOUT 120 seconds
>>
>> This test can easily fail if the testing environment is a bit
>> overloaded, so bump up the timeout to 5min to prevent false positive
>> failures.
>>
>> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
>> ---
>>  tools/testing/selftests/seccomp/settings | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/seccomp/settings b/tools/testing/selftests/seccomp/settings
>> index 6091b45d226b..694d70710ff0 100644
>> --- a/tools/testing/selftests/seccomp/settings
>> +++ b/tools/testing/selftests/seccomp/settings
>> @@ -1 +1 @@
>> -timeout=120
>> +timeout=300
>>
> Makes sense.

BTW, should this be sent upstream too?

> 
> Acked-by: Colin Ian King <colin.king@canonical.com>
>
Andrea Righi June 16, 2021, 9:27 a.m. UTC | #3
On Wed, Jun 16, 2021 at 10:19:54AM +0100, Colin Ian King wrote:
> On 16/06/2021 10:17, Colin Ian King wrote:
> > On 16/06/2021 10:11, Andrea Righi wrote:
> >>  DEBUG| [stdout] # selftests: seccomp: seccomp_benchmark
> >>  DEBUG| [stdout] # net.core.bpf_jit_enable = 1
> >>  DEBUG| [stdout] # net.core.bpf_jit_harden = 0
> >>  DEBUG| [stdout] #
> >>  DEBUG| [stdout] not ok 2 selftests: seccomp: seccomp_benchmark # TIMEOUT 120 seconds
> >>
> >> This test can easily fail if the testing environment is a bit
> >> overloaded, so bump up the timeout to 5min to prevent false positive
> >> failures.
> >>
> >> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> >> ---
> >>  tools/testing/selftests/seccomp/settings | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/tools/testing/selftests/seccomp/settings b/tools/testing/selftests/seccomp/settings
> >> index 6091b45d226b..694d70710ff0 100644
> >> --- a/tools/testing/selftests/seccomp/settings
> >> +++ b/tools/testing/selftests/seccomp/settings
> >> @@ -1 +1 @@
> >> -timeout=120
> >> +timeout=300
> >>
> > Makes sense.
> 
> BTW, should this be sent upstream too?
> 

Good idea. I'll also send this upstream.

-Andrea

> > 
> > Acked-by: Colin Ian King <colin.king@canonical.com>
> >
Thadeu Lima de Souza Cascardo June 16, 2021, 10:31 a.m. UTC | #4
On Wed, Jun 16, 2021 at 11:11:33AM +0200, Andrea Righi wrote:
>  DEBUG| [stdout] # selftests: seccomp: seccomp_benchmark
>  DEBUG| [stdout] # net.core.bpf_jit_enable = 1
>  DEBUG| [stdout] # net.core.bpf_jit_harden = 0
>  DEBUG| [stdout] #
>  DEBUG| [stdout] not ok 2 selftests: seccomp: seccomp_benchmark # TIMEOUT 120 seconds
> 
> This test can easily fail if the testing environment is a bit
> overloaded, so bump up the timeout to 5min to prevent false positive
> failures.
> 
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>

There are two tests running here, seccomp_bpf and seccomp_benchmark.
seccomp_benchmark tries to time its own run. Have you managed to identify if it
is seccomp_benchmark that is running late, and why (and by how much), or if it
is seccomp_bpf that is taking too long on these instances/workloads?

And do we really run it concurrently with any other workload to justify it
being late? Or is it the fact that it's running on a virtualized environment
that is overcommitted?

Just curious.

Cascardo.

> ---
>  tools/testing/selftests/seccomp/settings | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/seccomp/settings b/tools/testing/selftests/seccomp/settings
> index 6091b45d226b..694d70710ff0 100644
> --- a/tools/testing/selftests/seccomp/settings
> +++ b/tools/testing/selftests/seccomp/settings
> @@ -1 +1 @@
> -timeout=120
> +timeout=300
> -- 
> 2.31.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Andrea Righi June 16, 2021, 12:23 p.m. UTC | #5
On Wed, Jun 16, 2021 at 07:31:19AM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Wed, Jun 16, 2021 at 11:11:33AM +0200, Andrea Righi wrote:
> >  DEBUG| [stdout] # selftests: seccomp: seccomp_benchmark
> >  DEBUG| [stdout] # net.core.bpf_jit_enable = 1
> >  DEBUG| [stdout] # net.core.bpf_jit_harden = 0
> >  DEBUG| [stdout] #
> >  DEBUG| [stdout] not ok 2 selftests: seccomp: seccomp_benchmark # TIMEOUT 120 seconds
> > 
> > This test can easily fail if the testing environment is a bit
> > overloaded, so bump up the timeout to 5min to prevent false positive
> > failures.
> > 
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> 
> There are two tests running here, seccomp_bpf and seccomp_benchmark.
> seccomp_benchmark tries to time its own run. Have you managed to identify if it
> is seccomp_benchmark that is running late, and why (and by how much), or if it
> is seccomp_bpf that is taking too long on these instances/workloads?

seccomp_benchmark is the one that requires more time, because it's
testing a lot of syscalls:

 # time ./seccomp_benchmark
 Current BPF sysctl settings:
 net.core.bpf_jit_enable = 1
 net.core.bpf_jit_harden = 0
 Calibrating sample size for 15 seconds worth of syscalls ...
 Benchmarking 29470215 syscalls...
 ...
 real	1m15.558s
 user	0m33.147s
 sys	0m42.405s

seccomp_bpf instead is quite fast, usually it takes between 2-3 sec in
my test VM, for example:

 # time ./seccomp_bpf
 ...
 real	0m2.819s
 user	0m0.039s
 sys	0m0.045s

> 
> And do we really run it concurrently with any other workload to justify it
> being late? Or is it the fact that it's running on a virtualized environment
> that is overcommitted?

I think the reason is that tests are running in a virtualized
environment and it's not so rare to have overcommitted/overloaded
conditions.

In my local VM, where the environment is not overcommitted, the elapsed
time to run both tests is very consistent, always around 75 sec.

-Andrea

> 
> Just curious.
> 
> Cascardo.
> 
> > ---
> >  tools/testing/selftests/seccomp/settings | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/seccomp/settings b/tools/testing/selftests/seccomp/settings
> > index 6091b45d226b..694d70710ff0 100644
> > --- a/tools/testing/selftests/seccomp/settings
> > +++ b/tools/testing/selftests/seccomp/settings
> > @@ -1 +1 @@
> > -timeout=120
> > +timeout=300
> > -- 
> > 2.31.1
> > 
> > 
> > -- 
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Guilherme G. Piccoli June 16, 2021, 9:19 p.m. UTC | #6
On Wed, Jun 16, 2021 at 6:12 AM Andrea Righi <andrea.righi@canonical.com> wrote:
>
>  DEBUG| [stdout] # selftests: seccomp: seccomp_benchmark
>  DEBUG| [stdout] # net.core.bpf_jit_enable = 1
>  DEBUG| [stdout] # net.core.bpf_jit_harden = 0
>  DEBUG| [stdout] #
>  DEBUG| [stdout] not ok 2 selftests: seccomp: seccomp_benchmark # TIMEOUT 120 seconds
>
> This test can easily fail if the testing environment is a bit
> overloaded, so bump up the timeout to 5min to prevent false positive
> failures.
>
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> ---
>  tools/testing/selftests/seccomp/settings | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/seccomp/settings b/tools/testing/selftests/seccomp/settings
> index 6091b45d226b..694d70710ff0 100644
> --- a/tools/testing/selftests/seccomp/settings
> +++ b/tools/testing/selftests/seccomp/settings
> @@ -1 +1 @@
> -timeout=120
> +timeout=300

Thank you Andrea! I've seen similar timeouts, I think on Hirsute (or
maybe Groovy?) testing. Is there a reason to not have it in previous
releases?
The change itself looks good to me:

Acked-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
Andrea Righi June 17, 2021, 7:09 a.m. UTC | #7
On Wed, Jun 16, 2021 at 06:19:29PM -0300, Guilherme Piccoli wrote:
> On Wed, Jun 16, 2021 at 6:12 AM Andrea Righi <andrea.righi@canonical.com> wrote:
> >
> >  DEBUG| [stdout] # selftests: seccomp: seccomp_benchmark
> >  DEBUG| [stdout] # net.core.bpf_jit_enable = 1
> >  DEBUG| [stdout] # net.core.bpf_jit_harden = 0
> >  DEBUG| [stdout] #
> >  DEBUG| [stdout] not ok 2 selftests: seccomp: seccomp_benchmark # TIMEOUT 120 seconds
> >
> > This test can easily fail if the testing environment is a bit
> > overloaded, so bump up the timeout to 5min to prevent false positive
> > failures.
> >
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > ---
> >  tools/testing/selftests/seccomp/settings | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/seccomp/settings b/tools/testing/selftests/seccomp/settings
> > index 6091b45d226b..694d70710ff0 100644
> > --- a/tools/testing/selftests/seccomp/settings
> > +++ b/tools/testing/selftests/seccomp/settings
> > @@ -1 +1 @@
> > -timeout=120
> > +timeout=300
> 
> Thank you Andrea! I've seen similar timeouts, I think on Hirsute (or
> maybe Groovy?) testing. Is there a reason to not have it in previous
> releases?

I guess we can hit the same timeout issue also with the other kernels,
the failures are mostly caused by overcommitting/overloading the testing
environment (VMs running concurrently on the same physical hosts).

Ideally it'd be nice to have this fix applied upstream and then
cherry-pick the commit, let me see if we can get this applied upstream.

-Andrea

> The change itself looks good to me:
> 
> Acked-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
diff mbox series

Patch

diff --git a/tools/testing/selftests/seccomp/settings b/tools/testing/selftests/seccomp/settings
index 6091b45d226b..694d70710ff0 100644
--- a/tools/testing/selftests/seccomp/settings
+++ b/tools/testing/selftests/seccomp/settings
@@ -1 +1 @@ 
-timeout=120
+timeout=300