| Message ID | 20231102150342.3543-1-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [nft] tests: shell: Fix sets/reset_command_0 for current kernels | expand |
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
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
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).
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
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
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.
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
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.
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)
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(-)