[PATCH nft] tests: shell: fix spurious errors in sets/0036add_set_element_expiration_0

[PATCH nft] tests: shell: fix spurious errors in sets/0036add_set_element_expiration_0

[Pablo Neira Ayuso]

Add seconds as expiration, otherwise 14m59 reports 14m in minute
granularity, this ensures suficient time in a very slow environment with
debugging instrumentation.

Fixes: adf38fd84257 ("tests: shell: use minutes granularity in sets/0036add_set_element_expiration_0")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
I still see this failing occasionally due to timing issues, fix it.

 tests/shell/testcases/sets/0036add_set_element_expiration_0 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/shell/testcases/sets/0036add_set_element_expiration_0 b/tests/shell/testcases/sets/0036add_set_element_expiration_0
index 12f10074409f..a50ac91d43a6 100755
--- a/tests/shell/testcases/sets/0036add_set_element_expiration_0
+++ b/tests/shell/testcases/sets/0036add_set_element_expiration_0
@@ -8,7 +8,7 @@ drop_seconds() {
 
 RULESET="add table ip x
 add set ip x y { type ipv4_addr; flags dynamic,timeout; } 
-add element ip x y { 1.1.1.1 timeout 30m expires 15m }"
+add element ip x y { 1.1.1.1 timeout 30m expires 15m59s }"
 
 test_output=$($NFT -e -f - <<< "$RULESET" 2>&1 | grep -v '# new generation' | drop_seconds)
 
-- 
2.30.2

Re: [PATCH nft] tests: shell: fix spurious errors in sets/0036add_set_element_expiration_0

[Phil Sutter]

On Wed, Sep 27, 2023 at 04:48:03PM +0200, Pablo Neira Ayuso wrote:
> Add seconds as expiration, otherwise 14m59 reports 14m in minute
> granularity, this ensures suficient time in a very slow environment with
> debugging instrumentation.
> 
> Fixes: adf38fd84257 ("tests: shell: use minutes granularity in sets/0036add_set_element_expiration_0")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> I still see this failing occasionally due to timing issues, fix it.
> 
>  tests/shell/testcases/sets/0036add_set_element_expiration_0 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/shell/testcases/sets/0036add_set_element_expiration_0 b/tests/shell/testcases/sets/0036add_set_element_expiration_0
> index 12f10074409f..a50ac91d43a6 100755
> --- a/tests/shell/testcases/sets/0036add_set_element_expiration_0
> +++ b/tests/shell/testcases/sets/0036add_set_element_expiration_0
> @@ -8,7 +8,7 @@ drop_seconds() {
>  
>  RULESET="add table ip x
>  add set ip x y { type ipv4_addr; flags dynamic,timeout; } 
> -add element ip x y { 1.1.1.1 timeout 30m expires 15m }"
> +add element ip x y { 1.1.1.1 timeout 30m expires 15m59s }"
>  
>  test_output=$($NFT -e -f - <<< "$RULESET" 2>&1 | grep -v '# new generation' | drop_seconds)

The next line in that file is:

| if [ "$test_output" != "$RULESET" ] ; then

You add "59s" to $RULESET and drop it from $test_output. I guess, to
make it work, you also need to pipe $RULESET through drop_seconds before
the comparison.

Cheers, Phil

[PATCH nft] tests: shell: fix spurious errors in sets/0036add_set_element_expiration_0

[Pablo Neira Ayuso]

Add seconds as expiration, otherwise 14m59 reports 14m in minute
granularity, this ensures suficient time in a very slow environment with
debugging instrumentation.

Moreover, drop seconds from original input ruleset when comparing input
and output rulesets.

Fixes: adf38fd84257 ("tests: shell: use minutes granularity in sets/0036add_set_element_expiration_0")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
My second attempt to fix this test.

 tests/shell/testcases/sets/0036add_set_element_expiration_0 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/shell/testcases/sets/0036add_set_element_expiration_0 b/tests/shell/testcases/sets/0036add_set_element_expiration_0
index 12f10074409f..54e259a93f23 100755
--- a/tests/shell/testcases/sets/0036add_set_element_expiration_0
+++ b/tests/shell/testcases/sets/0036add_set_element_expiration_0
@@ -8,12 +8,14 @@ drop_seconds() {
 
 RULESET="add table ip x
 add set ip x y { type ipv4_addr; flags dynamic,timeout; } 
-add element ip x y { 1.1.1.1 timeout 30m expires 15m }"
+add element ip x y { 1.1.1.1 timeout 30m expires 15m59s }"
+
+test_input=$(echo $RULESET | drop_seconds)
 
 test_output=$($NFT -e -f - <<< "$RULESET" 2>&1 | grep -v '# new generation' | drop_seconds)
 
 if [ "$test_output" != "$RULESET" ] ; then
-	$DIFF -u <(echo "$test_output") <(echo "$RULESET")
+	$DIFF -u <(echo "$test_output") <(echo "$test_input")
 	exit 1
 fi
 
-- 
2.30.2

Re: [PATCH nft] tests: shell: fix spurious errors in sets/0036add_set_element_expiration_0

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 05:02:04PM +0200, Phil Sutter wrote:
> On Wed, Sep 27, 2023 at 04:48:03PM +0200, Pablo Neira Ayuso wrote:
> > Add seconds as expiration, otherwise 14m59 reports 14m in minute
> > granularity, this ensures suficient time in a very slow environment with
> > debugging instrumentation.
> > 
> > Fixes: adf38fd84257 ("tests: shell: use minutes granularity in sets/0036add_set_element_expiration_0")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > I still see this failing occasionally due to timing issues, fix it.
> > 
> >  tests/shell/testcases/sets/0036add_set_element_expiration_0 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/shell/testcases/sets/0036add_set_element_expiration_0 b/tests/shell/testcases/sets/0036add_set_element_expiration_0
> > index 12f10074409f..a50ac91d43a6 100755
> > --- a/tests/shell/testcases/sets/0036add_set_element_expiration_0
> > +++ b/tests/shell/testcases/sets/0036add_set_element_expiration_0
> > @@ -8,7 +8,7 @@ drop_seconds() {
> >  
> >  RULESET="add table ip x
> >  add set ip x y { type ipv4_addr; flags dynamic,timeout; } 
> > -add element ip x y { 1.1.1.1 timeout 30m expires 15m }"
> > +add element ip x y { 1.1.1.1 timeout 30m expires 15m59s }"
> >  
> >  test_output=$($NFT -e -f - <<< "$RULESET" 2>&1 | grep -v '# new generation' | drop_seconds)
> 
> The next line in that file is:
> 
> | if [ "$test_output" != "$RULESET" ] ; then
> 
> You add "59s" to $RULESET and drop it from $test_output. I guess, to
> make it work, you also need to pipe $RULESET through drop_seconds before
> the comparison.

Indeed, apologies: See latest patch.

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230927152514.473765-1-pablo@netfilter.org/

Re: [PATCH nft] tests: shell: fix spurious errors in sets/0036add_set_element_expiration_0

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 05:32:31PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 27, 2023 at 05:02:04PM +0200, Phil Sutter wrote:
> > On Wed, Sep 27, 2023 at 04:48:03PM +0200, Pablo Neira Ayuso wrote:
> > > Add seconds as expiration, otherwise 14m59 reports 14m in minute
> > > granularity, this ensures suficient time in a very slow environment with
> > > debugging instrumentation.
> > > 
> > > Fixes: adf38fd84257 ("tests: shell: use minutes granularity in sets/0036add_set_element_expiration_0")
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > ---
> > > I still see this failing occasionally due to timing issues, fix it.
> > > 
> > >  tests/shell/testcases/sets/0036add_set_element_expiration_0 | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/shell/testcases/sets/0036add_set_element_expiration_0 b/tests/shell/testcases/sets/0036add_set_element_expiration_0
> > > index 12f10074409f..a50ac91d43a6 100755
> > > --- a/tests/shell/testcases/sets/0036add_set_element_expiration_0
> > > +++ b/tests/shell/testcases/sets/0036add_set_element_expiration_0
> > > @@ -8,7 +8,7 @@ drop_seconds() {
> > >  
> > >  RULESET="add table ip x
> > >  add set ip x y { type ipv4_addr; flags dynamic,timeout; } 
> > > -add element ip x y { 1.1.1.1 timeout 30m expires 15m }"
> > > +add element ip x y { 1.1.1.1 timeout 30m expires 15m59s }"
> > >  
> > >  test_output=$($NFT -e -f - <<< "$RULESET" 2>&1 | grep -v '# new generation' | drop_seconds)
> > 
> > The next line in that file is:
> > 
> > | if [ "$test_output" != "$RULESET" ] ; then
> > 
> > You add "59s" to $RULESET and drop it from $test_output. I guess, to
> > make it work, you also need to pipe $RULESET through drop_seconds before
> > the comparison.
> 
> Indeed, apologies: See latest patch.
> 
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230927152514.473765-1-pablo@netfilter.org/

Ouch, still fails. Damn, I don't get a proper fix for this script.

Re: [PATCH nft] tests: shell: fix spurious errors in sets/0036add_set_element_expiration_0

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230927152514.473765-1-pablo@netfilter.org/
> 
> Ouch, still fails. Damn, I don't get a proper fix for this script.

What about restoring an expire value of 3s, then check there is *one*
element, wait 4s, check element is gone?

Tests are parallelized, so this won't cause noticeable slowdown.

Re: [PATCH nft] tests: shell: fix spurious errors in sets/0036add_set_element_expiration_0

[Pablo Neira Ayuso]

On Wed, Sep 27, 2023 at 05:53:17PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230927152514.473765-1-pablo@netfilter.org/
> > 
> > Ouch, still fails. Damn, I don't get a proper fix for this script.
> 
> What about restoring an expire value of 3s, then check there is *one*
> element, wait 4s, check element is gone?
> 
> Tests are parallelized, so this won't cause noticeable slowdown.

:) I have been running tests for a while and it seems no more spurious
errors show with:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230927163937.757167-1-pablo@netfilter.org/

Not related, next one to look at is:

W: [DUMP FAIL]   31/378 testcases/sets/0062set_connlimit

which occasionally fails. This is because this adds an element:

table ip x {
        set est-connlimit {
                type ipv4_addr
                size 65535
                flags dynamic
                elements = { 84.245.120.167 ct count over 20 }
        }
}

the conncount list is empty, then GC might win race to delete this
element with no entries (GC removes empty conncount lists).

I added this test to make sure this restores fine, even if GC removes
all elements that has been restored as soon as it gets a chance to
run, because the conncount list is empty.

For this one, I can set a larger gc-interval, so GC takes longer time
to release it, and still parser coverage for this restoration remains
in place.