| Message ID | 20230406-mptcp-issue-368-selftests-old-kernels-v2-9-50313e4f83ab@tessares.net (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | selftests: mptcp: skip tests when features are not supported | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| matttbe/KVM_Validation__normal__except_selftest_mptcp_join_ | warning | Unstable: 1 failed test(s): packetdrill_fastopen |
| matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ | warning | Unstable: 2 failed test(s): packetdrill_fastopen selftest_diag |
| matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__normal__only_selftest_mptcp_join_ | success | Success! ✅ |
On Mon, 2023-05-22 at 18:37 +0200, Matthieu Baerts wrote: > Selftests are supposed to run on any kernels, including the old ones not > supporting all MPTCP features. > > A new function is now available to easily detect if a feature is > missing by looking at the kernel version. That's clearly not ideal and > this kind of check should be avoided as soon as possible. But sometimes, > there are no external sign that a "feature" is available or not: > internal behaviours can change without modifying the uAPI and these > selftests are verifying the internal behaviours. Sometimes, the only > (easy) way to verify if the feature is present is to run the test but > then the validation cannot determine if there is a failure with the > feature or if the feature is missing. Then it looks better to check the > kernel version instead of having tests that can never fail. > > This new helper is going to be used in the following commits. In order > to ease the backport of such future patches, it would be good if this > patch is backported up to the introduction of MPTCP selftests, hence the > Fixes tag below: this type of check was supposed to be done from the > beginning. > > Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368 > Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp") > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> > --- > tools/testing/selftests/net/mptcp/mptcp_lib.sh | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh > index 29b65f4b73b2..dcfe5eb6f7c0 100644 > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh > @@ -76,3 +76,24 @@ mptcp_lib_kallsyms_doesnt_have() { > > mptcp_lib_fail_if_expected_feature "${sym} symbol has been found" > } > + > +# !!!AVOID USING THIS!!! > +# Features might not land in the expected version and features can be backported IMHO the above wording is a symptom we should not introduce this infra, nor the patches using it. Yep, self test will not be perfect on all older kernel :) /P
Hi Paolo, On 22/05/2023 19:48, Paolo Abeni wrote: > On Mon, 2023-05-22 at 18:37 +0200, Matthieu Baerts wrote: >> Selftests are supposed to run on any kernels, including the old ones not >> supporting all MPTCP features. >> >> A new function is now available to easily detect if a feature is >> missing by looking at the kernel version. That's clearly not ideal and >> this kind of check should be avoided as soon as possible. But sometimes, >> there are no external sign that a "feature" is available or not: >> internal behaviours can change without modifying the uAPI and these >> selftests are verifying the internal behaviours. Sometimes, the only >> (easy) way to verify if the feature is present is to run the test but >> then the validation cannot determine if there is a failure with the >> feature or if the feature is missing. Then it looks better to check the >> kernel version instead of having tests that can never fail. >> >> This new helper is going to be used in the following commits. In order >> to ease the backport of such future patches, it would be good if this >> patch is backported up to the introduction of MPTCP selftests, hence the >> Fixes tag below: this type of check was supposed to be done from the >> beginning. >> >> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368 >> Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp") >> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> >> --- >> tools/testing/selftests/net/mptcp/mptcp_lib.sh | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh >> index 29b65f4b73b2..dcfe5eb6f7c0 100644 >> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh >> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh >> @@ -76,3 +76,24 @@ mptcp_lib_kallsyms_doesnt_have() { >> >> mptcp_lib_fail_if_expected_feature "${sym} symbol has been found" >> } >> + >> +# !!!AVOID USING THIS!!! >> +# Features might not land in the expected version and features can be backported > > IMHO the above wording is a symptom we should not introduce this infra, > nor the patches using it. > > Yep, self test will not be perfect on all older kernel :) I understand, that's something we should avoid. But the thing is that even if selftests cannot be perfect on older kernels, when one sub-test fails, the whole selftest is marked as failed, making all the other sub-test useless because we only track the result of the whole selftest. In other words, if MPTCP join subtest 42/100 is always failing on kernel 5.15 because a feature is not supported, the result of the 99 other subtests will be ignored most of the time because we will only track the result of the selftest which will be "failed" all the time. To support kernels backporting new features, we can also add a new env var to skip this check (always returning that the version is recent enough). WDYT? Anyway I guess on these kernels, only the selftests from the same kernel version is used (not the one from the last dev branch). But if we see other (easy) way to check if a feature is present, always better to use alternatives. Cheers, Matt
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh index 29b65f4b73b2..dcfe5eb6f7c0 100644 --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh @@ -76,3 +76,24 @@ mptcp_lib_kallsyms_doesnt_have() { mptcp_lib_fail_if_expected_feature "${sym} symbol has been found" } + +# !!!AVOID USING THIS!!! +# Features might not land in the expected version and features can be backported +# +# $1: kernel version, e.g. 6.3 +mptcp_lib_kversion_ge() { + local exp_maj="${1%.*}" + local exp_min="${1#*.}" + local v maj min + + v=$(uname -r | cut -d'.' -f1,2) + maj=${v%.*} + min=${v#*.} + + if [ "${maj}" -gt "${exp_maj}" ] || + { [ "${maj}" -eq "${exp_maj}" ] && [ "${min}" -ge "${exp_min}" ]; }; then + return 0 + fi + + mptcp_lib_fail_if_expected_feature "kernel version ${1} lower than ${v}" +}
Selftests are supposed to run on any kernels, including the old ones not supporting all MPTCP features. A new function is now available to easily detect if a feature is missing by looking at the kernel version. That's clearly not ideal and this kind of check should be avoided as soon as possible. But sometimes, there are no external sign that a "feature" is available or not: internal behaviours can change without modifying the uAPI and these selftests are verifying the internal behaviours. Sometimes, the only (easy) way to verify if the feature is present is to run the test but then the validation cannot determine if there is a failure with the feature or if the feature is missing. Then it looks better to check the kernel version instead of having tests that can never fail. This new helper is going to be used in the following commits. In order to ease the backport of such future patches, it would be good if this patch is backported up to the introduction of MPTCP selftests, hence the Fixes tag below: this type of check was supposed to be done from the beginning. Link: https://github.com/multipath-tcp/mptcp_net-next/issues/368 Fixes: 048d19d444be ("mptcp: add basic kselftest for mptcp") Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> --- tools/testing/selftests/net/mptcp/mptcp_lib.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)