[nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[Phil Sutter]

Kernel behaviour changed regarding element reset in sets with timeouts,
disable the offending pieces.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/shell/testcases/sets/reset_command_0 | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/tests/shell/testcases/sets/reset_command_0 b/tests/shell/testcases/sets/reset_command_0
index e663dac831f8c..02d88d291bcf0 100755
--- a/tests/shell/testcases/sets/reset_command_0
+++ b/tests/shell/testcases/sets/reset_command_0
@@ -2,6 +2,10 @@
 
 # NFT_TEST_REQUIRES(NFT_TEST_HAVE_reset_set)
 
+# Note: Element expiry is no longer reset since kernel commit 4c90bba60c26
+# ("netfilter: nf_tables: do not refresh timeout when resetting element"),
+# the respective parts of the test have therefore been commented out.
+
 set -e
 
 trap '[[ $? -eq 0 ]] || echo FAIL' EXIT
@@ -44,10 +48,11 @@ elem='element t s { 1.0.0.1 . udp . 53 }'
 	grep 'elements = ' | drop_seconds | uniq | wc -l) == 1 ]]
 echo OK
 
-echo -n "counters and expiry are reset: "
+#echo -n "counters and expiry are reset: "
+echo -n "counters are reset: "
 NEW=$($NFT "get $elem")
 grep -q 'counter packets 0 bytes 0' <<< "$NEW"
-[[ $(expires_minutes <<< "$NEW") -gt 20 ]]
+#[[ $(expires_minutes <<< "$NEW") -gt 20 ]]
 echo OK
 
 echo -n "get map elem matches reset map elem: "
@@ -80,11 +85,11 @@ OUT=$($NFT reset map t m)
 $DIFF -u <(echo "$EXP") <(echo "$OUT")
 echo OK
 
-echo -n "reset command respects per-element timeout: "
-VAL=$($NFT get element t s '{ 2.0.0.2 . tcp . 22 }' | expires_minutes)
-[[ $VAL -lt 15 ]]	# custom timeout applies
-[[ $VAL -gt 10 ]]	# expires was reset
-echo OK
+#echo -n "reset command respects per-element timeout: "
+#VAL=$($NFT get element t s '{ 2.0.0.2 . tcp . 22 }' | expires_minutes)
+#[[ $VAL -lt 15 ]]	# custom timeout applies
+#[[ $VAL -gt 10 ]]	# expires was reset
+#echo OK
 
 echo -n "remaining elements are reset: "
 OUT=$($NFT list ruleset)
-- 
2.41.0

Re: [nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[Thomas Haller]

On Thu, 2023-11-02 at 16:03 +0100, Phil Sutter wrote:
>  
> +# Note: Element expiry is no longer reset since kernel commit
> 4c90bba60c26
> +# ("netfilter: nf_tables: do not refresh timeout when resetting
> element"),
> +# the respective parts of the test have therefore been commented
> out.

Hi Phil,

do you expect that the old behavior ever comes back?

Why keep the old checks (commented out)? Maybe drop them? We can get it
from git history.

If you think something is still unclear without them, then instead
elaborate further in the code comment (thanks for adding a code comment
in the first place, very useful).


Thomas

Re: [nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[Phil Sutter]

On Thu, Nov 02, 2023 at 04:29:34PM +0100, Thomas Haller wrote:
> On Thu, 2023-11-02 at 16:03 +0100, Phil Sutter wrote:
> >  
> > +# Note: Element expiry is no longer reset since kernel commit
> > 4c90bba60c26
> > +# ("netfilter: nf_tables: do not refresh timeout when resetting
> > element"),
> > +# the respective parts of the test have therefore been commented
> > out.
> 
> Hi Phil,
> 
> do you expect that the old behavior ever comes back?

A recent nfbz comment[1] from Pablo made me doubt the decision is final,
though I may have misread it.

> Why keep the old checks (commented out)? Maybe drop them? We can get it
> from git history.

Should the change be permanent, one should change the tests to assert
the opposite, namely that expires values are unaffected by the reset.

> If you think something is still unclear without them, then instead
> elaborate further in the code comment (thanks for adding a code comment
> in the first place, very useful).

Cheers, Phil

Re: [nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[Pablo Neira Ayuso]

On Thu, Nov 02, 2023 at 06:06:34PM +0100, Phil Sutter wrote:
> On Thu, Nov 02, 2023 at 04:29:34PM +0100, Thomas Haller wrote:
> > On Thu, 2023-11-02 at 16:03 +0100, Phil Sutter wrote:
> > >  
> > > +# Note: Element expiry is no longer reset since kernel commit
> > > 4c90bba60c26
> > > +# ("netfilter: nf_tables: do not refresh timeout when resetting
> > > element"),
> > > +# the respective parts of the test have therefore been commented
> > > out.
> > 
> > Hi Phil,
> > 
> > do you expect that the old behavior ever comes back?
> 
> A recent nfbz comment[1] from Pablo made me doubt the decision is final,
> though I may have misread it.

I hesitate on changing --stateless behaviour, but I don't find a
usecase for this option all alone unless it is combined with --terse,
to store an initial ruleset skeleton with no elements and no states.
Sets with timeout likely contain elements that get dynamically added
either via control plane or packet path based on some heuristics.

> > Why keep the old checks (commented out)? Maybe drop them? We can get it
> > from git history.
> 
> Should the change be permanent, one should change the tests to assert
> the opposite, namely that expires values are unaffected by the reset.

I think it is fine as it is now in the kernel. I have posted patches
to allow to update element timeouts via transaction, which looks more
flexible and run through the transaction path. As for counter and
quota, users likely only want to either: 1) restore a previous state
(after reboot) or 2) dump-and-reset counters for stats collection
(e.g. fetch counters at the end of the day).

Re: [nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[Pablo Neira Ayuso]

For the record, patch is in -stable 6.5 since 6.5.9:

https://cdn.kernel.org/pub/linux/kernel/v6.x/ChangeLog-6.5.9

Re: [nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[Phil Sutter]

On Thu, Nov 02, 2023 at 09:32:30PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 02, 2023 at 06:06:34PM +0100, Phil Sutter wrote:
> > On Thu, Nov 02, 2023 at 04:29:34PM +0100, Thomas Haller wrote:
> > > On Thu, 2023-11-02 at 16:03 +0100, Phil Sutter wrote:
> > > >  
> > > > +# Note: Element expiry is no longer reset since kernel commit
> > > > 4c90bba60c26
> > > > +# ("netfilter: nf_tables: do not refresh timeout when resetting
> > > > element"),
> > > > +# the respective parts of the test have therefore been commented
> > > > out.
> > > 
> > > Hi Phil,
> > > 
> > > do you expect that the old behavior ever comes back?
> > 
> > A recent nfbz comment[1] from Pablo made me doubt the decision is final,
> > though I may have misread it.
> 
> I hesitate on changing --stateless behaviour, but I don't find a
> usecase for this option all alone unless it is combined with --terse,
> to store an initial ruleset skeleton with no elements and no states.
> Sets with timeout likely contain elements that get dynamically added
> either via control plane or packet path based on some heuristics.

Unrelated to the expires vs. reset question, I wonder if one should
treat set elements with timeout as state themselves. If one leaves the
ruleset alone for long enough, they all will eventually vanish. So one
may argue the ruleset in its stateless form does not have elements in a
set with defined timeout.

> > > Why keep the old checks (commented out)? Maybe drop them? We can get it
> > > from git history.
> > 
> > Should the change be permanent, one should change the tests to assert
> > the opposite, namely that expires values are unaffected by the reset.
> 
> I think it is fine as it is now in the kernel. I have posted patches
> to allow to update element timeouts via transaction, which looks more
> flexible and run through the transaction path. As for counter and
> quota, users likely only want to either: 1) restore a previous state
> (after reboot) or 2) dump-and-reset counters for stats collection
> (e.g. fetch counters at the end of the day).

I still doubt there's a use-case to do (1) or (2) in sets with
temporary elements.

Cheers, Phil

Re: [nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[Pablo Neira Ayuso]

Hi Phil,

Picking up on this because I still see:

W: [FAILED]     331/389 testcases/sets/reset_command_0

here, maybe you can merge this change now? 6.5.x -stable will also
enter EoL in one more.

More comments below regarding your open questions.

On Tue, Nov 07, 2023 at 11:38:18AM +0100, Phil Sutter wrote:
> On Thu, Nov 02, 2023 at 09:32:30PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Nov 02, 2023 at 06:06:34PM +0100, Phil Sutter wrote:
> > > On Thu, Nov 02, 2023 at 04:29:34PM +0100, Thomas Haller wrote:
> > > > On Thu, 2023-11-02 at 16:03 +0100, Phil Sutter wrote:
> > > > >  
> > > > > +# Note: Element expiry is no longer reset since kernel commit
> > > > > 4c90bba60c26
> > > > > +# ("netfilter: nf_tables: do not refresh timeout when resetting
> > > > > element"),
> > > > > +# the respective parts of the test have therefore been commented
> > > > > out.
> > > > 
> > > > Hi Phil,
> > > > 
> > > > do you expect that the old behavior ever comes back?
> > > 
> > > A recent nfbz comment[1] from Pablo made me doubt the decision is final,
> > > though I may have misread it.
> > 
> > I hesitate on changing --stateless behaviour, but I don't find a
> > usecase for this option all alone unless it is combined with --terse,
> > to store an initial ruleset skeleton with no elements and no states.
> > Sets with timeout likely contain elements that get dynamically added
> > either via control plane or packet path based on some heuristics.
> 
> Unrelated to the expires vs. reset question, I wonder if one should
> treat set elements with timeout as state themselves. If one leaves the
> ruleset alone for long enough, they all will eventually vanish. So one
> may argue the ruleset in its stateless form does not have elements in a
> set with defined timeout.

The only usecase I can find for --stateless is diff'ing outputs
between two delta in time, to see what new elements are added and what
are gone. So I inclined to leave --stateless as is now.

> > > > Why keep the old checks (commented out)? Maybe drop them? We can get it
> > > > from git history.
> > > 
> > > Should the change be permanent, one should change the tests to assert
> > > the opposite, namely that expires values are unaffected by the reset.
> > 
> > I think it is fine as it is now in the kernel. I have posted patches
> > to allow to update element timeouts via transaction, which looks more
> > flexible and run through the transaction path. As for counter and
> > quota, users likely only want to either: 1) restore a previous state
> > (after reboot) or 2) dump-and-reset counters for stats collection
> > (e.g. fetch counters at the end of the day).
> 
> I still doubt there's a use-case to do (1) or (2) in sets with
> temporary elements.

For the reboot case, restoring temporary elements (which were added
via datapath) might make sense to me.

But there are limitations: connlimit is one of them because the
internal state of this datastructure gets losts between reboots.

Re: [nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[Phil Sutter]

Hi Pablo,

On Wed, Nov 22, 2023 at 12:30:57PM +0100, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> Picking up on this because I still see:
> 
> W: [FAILED]     331/389 testcases/sets/reset_command_0
> 
> here, maybe you can merge this change now? 6.5.x -stable will also
> enter EoL in one more.

There is a v2 of this patch adding an explicit check for expiry to not
change upon element reset. Are you fine with that? For reference, its
message ID is 20231102175754.15020-1-phil@nwl.cc.

> More comments below regarding your open questions.
> 
> On Tue, Nov 07, 2023 at 11:38:18AM +0100, Phil Sutter wrote:
> > On Thu, Nov 02, 2023 at 09:32:30PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Nov 02, 2023 at 06:06:34PM +0100, Phil Sutter wrote:
> > > > On Thu, Nov 02, 2023 at 04:29:34PM +0100, Thomas Haller wrote:
> > > > > On Thu, 2023-11-02 at 16:03 +0100, Phil Sutter wrote:
> > > > > >  
> > > > > > +# Note: Element expiry is no longer reset since kernel commit
> > > > > > 4c90bba60c26
> > > > > > +# ("netfilter: nf_tables: do not refresh timeout when resetting
> > > > > > element"),
> > > > > > +# the respective parts of the test have therefore been commented
> > > > > > out.
> > > > > 
> > > > > Hi Phil,
> > > > > 
> > > > > do you expect that the old behavior ever comes back?
> > > > 
> > > > A recent nfbz comment[1] from Pablo made me doubt the decision is final,
> > > > though I may have misread it.
> > > 
> > > I hesitate on changing --stateless behaviour, but I don't find a
> > > usecase for this option all alone unless it is combined with --terse,
> > > to store an initial ruleset skeleton with no elements and no states.
> > > Sets with timeout likely contain elements that get dynamically added
> > > either via control plane or packet path based on some heuristics.
> > 
> > Unrelated to the expires vs. reset question, I wonder if one should
> > treat set elements with timeout as state themselves. If one leaves the
> > ruleset alone for long enough, they all will eventually vanish. So one
> > may argue the ruleset in its stateless form does not have elements in a
> > set with defined timeout.
> 
> The only usecase I can find for --stateless is diff'ing outputs
> between two delta in time, to see what new elements are added and what
> are gone. So I inclined to leave --stateless as is now.

I see --stateless as a way to dump the ruleset in its basic form for a
fresh start with zeroed counters, etc. Hence why I wondered if it should
omit expiring elements as those are usually added by packet path or at
least explicitly after loading the ruleset itself.

> > > > > Why keep the old checks (commented out)? Maybe drop them? We can get it
> > > > > from git history.
> > > > 
> > > > Should the change be permanent, one should change the tests to assert
> > > > the opposite, namely that expires values are unaffected by the reset.
> > > 
> > > I think it is fine as it is now in the kernel. I have posted patches
> > > to allow to update element timeouts via transaction, which looks more
> > > flexible and run through the transaction path. As for counter and
> > > quota, users likely only want to either: 1) restore a previous state
> > > (after reboot) or 2) dump-and-reset counters for stats collection
> > > (e.g. fetch counters at the end of the day).
> > 
> > I still doubt there's a use-case to do (1) or (2) in sets with
> > temporary elements.
> 
> For the reboot case, restoring temporary elements (which were added
> via datapath) might make sense to me.
> 
> But there are limitations: connlimit is one of them because the
> internal state of this datastructure gets losts between reboots.

Cheers, Phil

Re: [nft PATCH] tests: shell: Fix sets/reset_command_0 for current kernels

[Pablo Neira Ayuso]

On Wed, Nov 22, 2023 at 02:35:22PM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> On Wed, Nov 22, 2023 at 12:30:57PM +0100, Pablo Neira Ayuso wrote:
> > Hi Phil,
> > 
> > Picking up on this because I still see:
> > 
> > W: [FAILED]     331/389 testcases/sets/reset_command_0
> > 
> > here, maybe you can merge this change now? 6.5.x -stable will also
> > enter EoL in one more.
> 
> There is a v2 of this patch adding an explicit check for expiry to not
> change upon element reset. Are you fine with that? For reference, its
> message ID is 20231102175754.15020-1-phil@nwl.cc.

If it works reliable and you are happy with it, all fine with me.