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

[Phil Sutter <phil@nwl.cc>]

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