[PATCH nft 1/3] tests/shell: skip "table_onoff" test if kernel patch is missing

[PATCH nft 1/3] tests/shell: skip "table_onoff" test if kernel patch is missing

[Thomas Haller]

Passing the test suite must not require latest kernel patches.  If test
"table_onoff" appears to not work due to a missing kernel patch, skip
it.

If you run a special kernel and expect that all test pass, set
NFT_TEST_FAIL_ON_SKIP=y to catch unexpected skips.

Fixes: bcca2d67656f ('tests: add test for dormant on/off/on bug')
Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/testcases/transactions/table_onoff | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/shell/testcases/transactions/table_onoff b/tests/shell/testcases/transactions/table_onoff
index 831d4614c1f2..d5ad09ef334c 100755
--- a/tests/shell/testcases/transactions/table_onoff
+++ b/tests/shell/testcases/transactions/table_onoff
@@ -11,7 +11,8 @@ delete table ip t
 EOF
 
 if [ $? -eq 0 ]; then
-	exit 1
+	echo "Command to re-awaken a dormant table did not fail. Assume https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c9bd26513b3a11b3adb3c2ed8a31a01a87173ff1 is missing"
+	exit 77
 fi
 
 set -e
-- 
2.41.0

[PATCH nft 2/3] tests/shell: skip "vlan_8021ad_tag" test instead of failing

[Thomas Haller]

The test "vlan_8021ad_tag" requires recent kernel patches to pass. This
makes the test suite unusable to contributors, who don't also run the
required kernel.

Instead of failing, just skip the test.

If you run with a kernel that is supposed to pass all tests, consider
setting NFT_TEST_FAIL_ON_SKIP=y.

Fixes: 74cf3d16d8e9 ('tests: shell: add vlan match test case')
Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/testcases/packetpath/vlan_8021ad_tag | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/shell/testcases/packetpath/vlan_8021ad_tag b/tests/shell/testcases/packetpath/vlan_8021ad_tag
index 379a5710c1cb..246427062323 100755
--- a/tests/shell/testcases/packetpath/vlan_8021ad_tag
+++ b/tests/shell/testcases/packetpath/vlan_8021ad_tag
@@ -47,4 +47,9 @@ EOF
 ip netns exec "$ns1" ping -c 1 10.1.1.2
 
 ip netns exec "$ns2" $NFT list ruleset
-ip netns exec "$ns2" $NFT list chain netdev t c | grep 'counter packets 1 bytes 84'
+OUT="$(ip netns exec "$ns2" $NFT list chain netdev t c)"
+
+if ! printf "%s" "$OUT" | grep -q 'counter packets 1 bytes 84' ; then
+	echo "Filter did not match. Assume kernel lacks fix https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=af84f9e447a65b4b9f79e7e5d69e19039b431c56"
+	exit 77
+fi
-- 
2.41.0

[PATCH nft 3/3] tests/shell: add missing "vlan_8021ad_tag.nodump" file

[Thomas Haller]

This is an inconsistency. The test should have either a .nft or a
.nodump file. "./tools/check-tree.sh" enforces that and will in the
future run by `make check`.

Fixes: 74cf3d16d8e9 ('tests: shell: add vlan match test case')
Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 tests/shell/testcases/packetpath/dumps/vlan_8021ad_tag.nodump | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 tests/shell/testcases/packetpath/dumps/vlan_8021ad_tag.nodump

diff --git a/tests/shell/testcases/packetpath/dumps/vlan_8021ad_tag.nodump b/tests/shell/testcases/packetpath/dumps/vlan_8021ad_tag.nodump
new file mode 100644
index 000000000000..e69de29bb2d1
-- 
2.41.0

Re: [PATCH nft 1/3] tests/shell: skip "table_onoff" test if kernel patch is missing

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> Passing the test suite must not require latest kernel patches.  If test
> "table_onoff" appears to not work due to a missing kernel patch, skip
> it.
> 
> If you run a special kernel and expect that all test pass, set
> NFT_TEST_FAIL_ON_SKIP=y to catch unexpected skips.

This makes the test suite and all feature probing moot for my use cases.
If I see SKIP, I assume that the feature is missing.

This is a bug, and it tells me that I might have to do something
about it.

If you absolutely cannot have a failure because of this, then
please add another error state for this, so that I can see that
something is wrong.

This is NOT the same as a skip because some distro kernel lacks
anonymous chain support.

That said, I would STRONLGY perfer failure here.
Distros will ship updates that eventually also include this bug fix.

This fix is included in 6.5.6 for example.

Re: [PATCH nft 1/3] tests/shell: skip "table_onoff" test if kernel patch is missing

[Thomas Haller]

Hi,


On Mon, 2023-10-16 at 22:20 +0200, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > Passing the test suite must not require latest kernel patches.  If
> > test
> > "table_onoff" appears to not work due to a missing kernel patch,
> > skip
> > it.
> > 
> > If you run a special kernel and expect that all test pass, set
> > NFT_TEST_FAIL_ON_SKIP=y to catch unexpected skips.
> 
> This makes the test suite and all feature probing moot for my use
> cases.
> If I see SKIP, I assume that the feature is missing.

As you probably run a self-built kernel, wouldn't you just `export
NFT_TEST_FAIL_ON_SKIP=y` and reject all skips as failures? What's the
problem with that? That exists exactly for your use case.

The test suite should be usable on some recent + popular distro kernels
(like Fedora 38). That will also be rather important, once the tests
are invoked by `make check` (I have patches for that, it's very simple
and rather nice!).


> 
> This is a bug, and it tells me that I might have to do something
> about it.

OK, do you intend to fix this bug in a very timely manner on Fedora 38
(and other popular kernels)? Then maybe hold back the test until that
happend? (or let it skip for now, and in a few weeks, upgrade to hard
failure -- the only problem is not to forget about that).

> 
> If you absolutely cannot have a failure because of this, then
> please add another error state for this, so that I can see that
> something is wrong.
> 
> This is NOT the same as a skip because some distro kernel lacks
> anonymous chain support.
> 
> That said, I would STRONLGY perfer failure here.
> Distros will ship updates that eventually also include this bug fix.

> 
> This fix is included in 6.5.6 for example.
> 

Ah right. "tests/shell/testcases/transactions/table_onoff" is fixed on
6.5.6-200.fc38.x86_64. There still is a general problem. For example
what about tests/shell/testcases/packetpath/vlan_8021ad_tag ?



Maybe there could be two levels of NFT_TEST_FAIL_ON_SKIP. Maybe
"always" or "bugs-only". Then either:

1) the test would exit 78 instead of 77. And run-test.sh would treat 78
either as failure or as skip, based on NFT_TEST_FAIL_ON_SKIP

2) the test itself could look at NFT_TEST_FAIL_ON_SKIP and decide
whether to exit with 77 or 1.


Or how about adding a mechanism, that compares the kernel version and
decides whether to skip? For example


if ! printf "%s" "$OUT" | grep -q 'counter packets 1 bytes 84' ; then
    echo "Filter did not match. Assume kernel lacks fix https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=af84f9e447a65b4b9f79e7e5d69e19039b431c56"
    "$NFT_TEST_BASEDIR/helpers/eval-exit-code" kernel 6.5.6 || exit $?
fi


where "eval-exit-code" will either exit with 1 or 77 and log a message
(about the result of the comparison).

The "kernel" arguments indicates to compare the `uname` output in some
useful >= way.



How about "eval-exit-code"?


Thomas

Re: [PATCH nft 1/3] tests/shell: skip "table_onoff" test if kernel patch is missing

[Thomas Haller]

On Tue, 2023-10-17 at 08:22 +0200, Thomas Haller wrote:
> How about "eval-exit-code"?

Hi Florian,


shown in

  [PATCH nft v2 0/3] add "eval-exit-code" and skip tests based on kernel version

How do you like that?


Thomas

Re: [PATCH nft 1/3] tests/shell: skip "table_onoff" test if kernel patch is missing

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> As you probably run a self-built kernel, wouldn't you just `export
> NFT_TEST_FAIL_ON_SKIP=y` and reject all skips as failures? What's the
> problem with that? That exists exactly for your use case.

No, its not my use case.

The use case is to make sure that the frankenkernels that I am in charge
of do not miss any important bug fixes.

This is the reason for the feature probing, "skip" should tell me that
I can safely ignore it because the feature is not present.

I could built a list of "expected failures", but that will mask real
actual regressions.

> > This is a bug, and it tells me that I might have to do something
> > about it.
> 
> OK, do you intend to fix this bug in a very timely manner on Fedora 38
> (and other popular kernels)? Then maybe hold back the test until that
> happend? (or let it skip for now, and in a few weeks, upgrade to hard
> failure -- the only problem is not to forget about that).

I did keep the test back until I saw that -stable had picked it up.

I can wait longer, sure.

> Ah right. "tests/shell/testcases/transactions/table_onoff" is fixed on
> 6.5.6-200.fc38.x86_64. There still is a general problem. For example
> what about tests/shell/testcases/packetpath/vlan_8021ad_tag ?

Its also a bug that needs to be fixed in the kernel.
I applied it after stable had picked it up for 6.5.7.

> 1) the test would exit 78 instead of 77. And run-test.sh would treat 78
> either as failure or as skip, based on NFT_TEST_FAIL_ON_SKIP
> 
> 2) the test itself could look at NFT_TEST_FAIL_ON_SKIP and decide
> whether to exit with 77 or 1.
> 
> 
> Or how about adding a mechanism, that compares the kernel version and
> decides whether to skip? For example

I don't think that kernel versions work or are something that we can
realistically handle.  Even just RHEL would be a nightmare if one
considers all the different release streams.

I think even just handling upstream -stable is too much work.

That said, I hope that these kinds of tests will happen less frequently
over time.

Re: [PATCH nft 1/3] tests/shell: skip "table_onoff" test if kernel patch is missing

[Thomas Haller]

On Tue, 2023-10-17 at 11:32 +0200, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > As you probably run a self-built kernel, wouldn't you just `export
> > NFT_TEST_FAIL_ON_SKIP=y` and reject all skips as failures? What's
> > the
> > problem with that? That exists exactly for your use case.
> 
> No, its not my use case.
> 
> The use case is to make sure that the frankenkernels that I am in
> charge
> of do not miss any important bug fixes.
> 
> This is the reason for the feature probing, "skip" should tell me
> that
> I can safely ignore it because the feature is not present.
> 
> I could built a list of "expected failures", but that will mask real
> actual regressions.

How did you handle that, before the recent addition of the skip
functionality? Did you just have a list of known failures, and manually
ignored them?


Anyway, the "eval-exit-code" in v2 can easily honor an environment
variable, to always fail hard. The only question is how exactly it
should work.

I propse that NFT_TEST_FAIL_ON_SKIP=y should honor a variable
"NFT_TEST_FAIL_ON_SKIP_EXCEPT=", which takes a regex of test names, for
which a skip is *not* fatal (you opt-in the tests that are allowed to
fail). If you maintain c9s, the list of known skipped tests is small
and relatively static. You can maintain a per-kernel-variant regex in
that case.

If we want, we can even parse /etc/os-release and uname and code a
default list of regexes.



> > > This is a bug, and it tells me that I might have to do something
> > > about it.
> > 
> > OK, do you intend to fix this bug in a very timely manner on Fedora
> > 38
> > (and other popular kernels)? Then maybe hold back the test until
> > that
> > happend? (or let it skip for now, and in a few weeks, upgrade to
> > hard
> > failure -- the only problem is not to forget about that).
> 
> I did keep the test back until I saw that -stable had picked it up.
> 
> I can wait longer, sure.

I think it is good to merge tests soon. There just needs to be a
reasonable+convenient way to handle the problem.

Having a policy that requires you to wait is broken. Especially, since
it's unclear how long to wait. You are not waiting for yourself, but
for any unknown user who is affected.


> > Ah right. "tests/shell/testcases/transactions/table_onoff" is fixed
> > on
> > 6.5.6-200.fc38.x86_64. There still is a general problem. For
> > example
> > what about tests/shell/testcases/packetpath/vlan_8021ad_tag ?
> 
> Its also a bug that needs to be fixed in the kernel.
> I applied it after stable had picked it up for 6.5.7.
> 
> > 1) the test would exit 78 instead of 77. And run-test.sh would
> > treat 78
> > either as failure or as skip, based on NFT_TEST_FAIL_ON_SKIP
> > 
> > 2) the test itself could look at NFT_TEST_FAIL_ON_SKIP and decide
> > whether to exit with 77 or 1.
> > 
> > 
> > Or how about adding a mechanism, that compares the kernel version
> > and
> > decides whether to skip? For example
> 
> I don't think that kernel versions work or are something that we can
> realistically handle.  Even just RHEL would be a nightmare if one
> considers all the different release streams.
> 
> I think even just handling upstream -stable is too much work.

I think the kernel versions work reasonably well for upstream and
Fedora kernels (which is something already!).

I guess, there could be a smarter

  "$NFT_TEST_BASEDIR/helpers/eval-exit-code" kernel  upstream-6.6  upstream-6.5.6  c9s-5.14.0-373

that also can cover different "streams" (e.g. the uname from a centos).
But I like a NFT_TEST_FAIL_ON_SKIP_EXCEPT= better.

Also, at worst on the Frankenkernel you get a SKIP, when it should have
been a FAIL. For the non-expert user who writes a patch to fix a type
the SKIP is better during `make check`.

On upstream/Fedora kernels, you also don't need anything, and "eval-
exit-code" will end up doing the right thing automatically.

And if you maintain CentOS9Stream, then set NFT_TEST_FAIL_ON_SKIP=y and
NFT_TEST_FAIL_ON_SKIP_EXCEPT=<REGEX> and keep track of the tests that
are known to fail. You know your kernel, and the tests that are known
to be skipped.

How about that?

> 
> That said, I hope that these kinds of tests will happen less
> frequently
> over time.
> 

I like the optimism :)


Thomas