[PATCH mptcp-net] selftests: mptcp: don't exit when a symbol not found

[PATCH mptcp-net] selftests: mptcp: don't exit when a symbol not found

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

mptcp_lib_kallsyms_has() will always exit when a symbol has not found, it
breaks the test itself. Unexpected errors occur:

 007 userspace pm add & remove address
       syn                                 [ OK ]
       synack                              [ OK ]
       ack                                 [ OK ]
       add                                 [ OK ]
       echo                                [ OK ]
       mptcp_info subflows=2:2             [ OK ]
       mptcp_info subflows_total=3:3       [ OK ]
       mptcp_info add_addr_signal=2:2      [ OK ]
       mptcp_info last_data_sent=191:18    [ OK ]
       mptcp_info last_data_recv=40:68     [ OK ]
       mptcp_info last_ack_recv=93:74      [ OK ]
       dump addrs signal                   ERROR: missing feature: \
			mptcp_userspace_pm_dump_addr$ symbol not found
 Cannot open network namespace "ns1-65d6fb5a-5FviVK": \
			No such file or directory
 Cannot open network namespace "ns2-65d6fb5a-5FviVK": \
			No such file or directory
 cmp: /tmp/tmp.a7osJs7Nj0: No such file or directory
 cmp: /tmp/tmp.f02z6brCQu: No such file or directory
 cat: /tmp/tmp.27TzxD2efV: No such file or directory
not ok 1 test: selftest_mptcp_join # FAIL

To fix this, this patch adds a new argument 'continue' for the helper
mptcp_lib_fail_if_expected_feature() to control whether exit or not.

Always set this argument to 1 in mptcp_lib_kallsyms_has().

Fixes: 83013bdf90a ("selftests: mptcp: connect: skip if MPTCP is not supported")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_lib.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 556a7d9784d7..e23fd85902d8 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -55,11 +55,13 @@ mptcp_lib_expect_all_features() {
 	[ "${SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES:-}" = "1" ]
 }
 
-# $1: msg
+# $1: msg $2: continue or not
 mptcp_lib_fail_if_expected_feature() {
+	local continue="${2:-0}"
+
 	if mptcp_lib_expect_all_features; then
-		echo "ERROR: missing feature: ${*}"
-		exit ${KSFT_FAIL}
+		echo "ERROR: missing feature: ${1}"
+		[ "${continue}" -eq 0 ] && exit ${KSFT_FAIL}
 	fi
 
 	return 1
@@ -107,7 +109,7 @@ mptcp_lib_kallsyms_has() {
 		return 0
 	fi
 
-	mptcp_lib_fail_if_expected_feature "${sym} symbol not found"
+	mptcp_lib_fail_if_expected_feature "${sym} symbol not found" 1
 }
 
 # $1: part of a symbol to look at, add '$' at the end for full name
-- 
2.40.1

Re: selftests: mptcp: don't exit when a symbol not found: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI (GitHub Action) did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8001358412

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/13a7910a697f


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-net] selftests: mptcp: don't exit when a symbol not found

[Matthieu Baerts]

Hi Geliang,

On 22/02/2024 8:56 am, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> mptcp_lib_kallsyms_has() will always exit when a symbol has not found, it
> breaks the test itself. Unexpected errors occur:
> 
>  007 userspace pm add & remove address
>        syn                                 [ OK ]
>        synack                              [ OK ]
>        ack                                 [ OK ]
>        add                                 [ OK ]
>        echo                                [ OK ]
>        mptcp_info subflows=2:2             [ OK ]
>        mptcp_info subflows_total=3:3       [ OK ]
>        mptcp_info add_addr_signal=2:2      [ OK ]
>        mptcp_info last_data_sent=191:18    [ OK ]
>        mptcp_info last_data_recv=40:68     [ OK ]
>        mptcp_info last_ack_recv=93:74      [ OK ]
>        dump addrs signal                   ERROR: missing feature: \
> 			mptcp_userspace_pm_dump_addr$ symbol not found
>  Cannot open network namespace "ns1-65d6fb5a-5FviVK": \
> 			No such file or directory
>  Cannot open network namespace "ns2-65d6fb5a-5FviVK": \
> 			No such file or directory
>  cmp: /tmp/tmp.a7osJs7Nj0: No such file or directory
>  cmp: /tmp/tmp.f02z6brCQu: No such file or directory
>  cat: /tmp/tmp.27TzxD2efV: No such file or directory
> not ok 1 test: selftest_mptcp_join # FAIL
> 
> To fix this, this patch adds a new argument 'continue' for the helper
> mptcp_lib_fail_if_expected_feature() to control whether exit or not.
> 
> Always set this argument to 1 in mptcp_lib_kallsyms_has().
> 
> Fixes: 83013bdf90a ("selftests: mptcp: connect: skip if MPTCP is not supported")

I'm not sure to understand why you sent this :)

The fixes commit ('d' is missing at the beginning) here mentions:

> Note that this check can also
> mark the test as failed if 'SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES' env
> var is set to 1: by doing that, we can make sure a test is not being
> skipped by mistake.

So the goal is to fail when the env var is set. Otherwise, we would miss
tests that are skipped by accident.

If you use 'mptcp-upstream-virtme-docker' and you want to run recent
selftests on an older kernel, you can add this to the docker command not
to exit if a feature is missing:

  -e INPUT_SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES=0

But clearly here, we don't want to continue if we expect all features to
work!

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net] selftests: mptcp: don't exit when a symbol not found

[Geliang Tang]

On Thu, Feb 22, 2024 at 09:58:39AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/02/2024 8:56 am, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > mptcp_lib_kallsyms_has() will always exit when a symbol has not found, it
> > breaks the test itself. Unexpected errors occur:
> > 
> >  007 userspace pm add & remove address
> >        syn                                 [ OK ]
> >        synack                              [ OK ]
> >        ack                                 [ OK ]
> >        add                                 [ OK ]
> >        echo                                [ OK ]
> >        mptcp_info subflows=2:2             [ OK ]
> >        mptcp_info subflows_total=3:3       [ OK ]
> >        mptcp_info add_addr_signal=2:2      [ OK ]
> >        mptcp_info last_data_sent=191:18    [ OK ]
> >        mptcp_info last_data_recv=40:68     [ OK ]
> >        mptcp_info last_ack_recv=93:74      [ OK ]
> >        dump addrs signal                   ERROR: missing feature: \
> > 			mptcp_userspace_pm_dump_addr$ symbol not found
> >  Cannot open network namespace "ns1-65d6fb5a-5FviVK": \
> > 			No such file or directory
> >  Cannot open network namespace "ns2-65d6fb5a-5FviVK": \
> > 			No such file or directory
> >  cmp: /tmp/tmp.a7osJs7Nj0: No such file or directory
> >  cmp: /tmp/tmp.f02z6brCQu: No such file or directory
> >  cat: /tmp/tmp.27TzxD2efV: No such file or directory
> > not ok 1 test: selftest_mptcp_join # FAIL
> > 
> > To fix this, this patch adds a new argument 'continue' for the helper
> > mptcp_lib_fail_if_expected_feature() to control whether exit or not.
> > 
> > Always set this argument to 1 in mptcp_lib_kallsyms_has().
> > 
> > Fixes: 83013bdf90a ("selftests: mptcp: connect: skip if MPTCP is not supported")
> 
> I'm not sure to understand why you sent this :)

Say in the test "signal addresses race test":

                if ! mptcp_lib_kallsyms_has "mptcp_pm_subflow_check_next$"; then
                        chk_join_nr 3 3 2
                        chk_add_nr 4 4
                else
                        chk_join_nr 3 3 3
                        # the server will not signal the address terminating
                        # the MPC subflow
                        chk_add_nr 3 3
                fi

This code will never run to this block:

                        chk_join_nr 3 3 2
                        chk_add_nr 4 4

Since if no symbal of mptcp_pm_subflow_check_next, mptcp_lib_kallsyms_has exit.

You can trigger this by renaming "mptcp_pm_subflow_check_next$" to
"no_mptcp_pm_subflow_check_next$", an non-exist function name, for test.

Thanks,
-Geliang

> 
> The fixes commit ('d' is missing at the beginning) here mentions:
> 
> > Note that this check can also
> > mark the test as failed if 'SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES' env
> > var is set to 1: by doing that, we can make sure a test is not being
> > skipped by mistake.
> 
> So the goal is to fail when the env var is set. Otherwise, we would miss
> tests that are skipped by accident.
> 
> If you use 'mptcp-upstream-virtme-docker' and you want to run recent
> selftests on an older kernel, you can add this to the docker command not
> to exit if a feature is missing:
> 
>   -e INPUT_SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES=0
> 
> But clearly here, we don't want to continue if we expect all features to
> work!
> 
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net] selftests: mptcp: don't exit when a symbol not found

[Matthieu Baerts]

Hi Geliang,

On 22/02/2024 11:31 am, Geliang Tang wrote:
> On Thu, Feb 22, 2024 at 09:58:39AM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 22/02/2024 8:56 am, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> mptcp_lib_kallsyms_has() will always exit when a symbol has not found, it
>>> breaks the test itself. Unexpected errors occur:
>>>
>>>  007 userspace pm add & remove address
>>>        syn                                 [ OK ]
>>>        synack                              [ OK ]
>>>        ack                                 [ OK ]
>>>        add                                 [ OK ]
>>>        echo                                [ OK ]
>>>        mptcp_info subflows=2:2             [ OK ]
>>>        mptcp_info subflows_total=3:3       [ OK ]
>>>        mptcp_info add_addr_signal=2:2      [ OK ]
>>>        mptcp_info last_data_sent=191:18    [ OK ]
>>>        mptcp_info last_data_recv=40:68     [ OK ]
>>>        mptcp_info last_ack_recv=93:74      [ OK ]
>>>        dump addrs signal                   ERROR: missing feature: \
>>> 			mptcp_userspace_pm_dump_addr$ symbol not found
>>>  Cannot open network namespace "ns1-65d6fb5a-5FviVK": \
>>> 			No such file or directory
>>>  Cannot open network namespace "ns2-65d6fb5a-5FviVK": \
>>> 			No such file or directory
>>>  cmp: /tmp/tmp.a7osJs7Nj0: No such file or directory
>>>  cmp: /tmp/tmp.f02z6brCQu: No such file or directory
>>>  cat: /tmp/tmp.27TzxD2efV: No such file or directory
>>> not ok 1 test: selftest_mptcp_join # FAIL
>>>
>>> To fix this, this patch adds a new argument 'continue' for the helper
>>> mptcp_lib_fail_if_expected_feature() to control whether exit or not.
>>>
>>> Always set this argument to 1 in mptcp_lib_kallsyms_has().
>>>
>>> Fixes: 83013bdf90a ("selftests: mptcp: connect: skip if MPTCP is not supported")
>>
>> I'm not sure to understand why you sent this :)
> 
> Say in the test "signal addresses race test":
> 
>                 if ! mptcp_lib_kallsyms_has "mptcp_pm_subflow_check_next$"; then
>                         chk_join_nr 3 3 2
>                         chk_add_nr 4 4
>                 else
>                         chk_join_nr 3 3 3
>                         # the server will not signal the address terminating
>                         # the MPC subflow
>                         chk_add_nr 3 3
>                 fi
> 
> This code will never run to this block:
> 
>                         chk_join_nr 3 3 2
>                         chk_add_nr 4 4
> 
> Since if no symbal of mptcp_pm_subflow_check_next, mptcp_lib_kallsyms_has exit.

This block will never be executed in our CI, because our CI always run
the selftests linked to the same version of the kernel. In this case,
that would not be normal to execute this block in our CI, it has to
complain because we expect to have this symbol in the latest version.

Other CIs, like LKFT, which are validating stable kernels (e.g. 5.15.x)
using the selftests from the last stable kernel (e.g. 6.7.x) will not
have SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES env var being set to 1 as
we did in our CI. Then it is fine, tests and checks will be skipped if
some features are missing:

https://tuxapi.tuxsuite.com/v1/groups/linaro/projects/lkft/tests/2cJxC6Sua3VVzRZmpQYKK2jzzNN/logs?format=html

> You can trigger this by renaming "mptcp_pm_subflow_check_next$" to
> "no_mptcp_pm_subflow_check_next$", an non-exist function name, for test.

I confirm that if you use 'mptcp-upstream-virtme-docker', this env var
will be set to 1 by default:

  SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES=1

You can unset it on your side, but it only makes sense to do that if you
work on stable kernels (and you use selftests from a more recent
version, not the ones attached to the kernel you are validating).

So yes, in this environment (where we force setting the env var to 1,
not the "default" behaviour), picking a non-existing function name will
cause the test to fail: but that's what we want → we want to be notified
if we picked a non-existing function, like it happened recently:

  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7988706773

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net] selftests: mptcp: don't exit when a symbol not found

[Geliang Tang]

Hi Matt,

On Thu, Feb 22, 2024 at 11:49:18AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/02/2024 11:31 am, Geliang Tang wrote:
> > On Thu, Feb 22, 2024 at 09:58:39AM +0100, Matthieu Baerts wrote:
> >> Hi Geliang,
> >>
> >> On 22/02/2024 8:56 am, Geliang Tang wrote:
> >>> From: Geliang Tang <tanggeliang@kylinos.cn>
> >>>
> >>> mptcp_lib_kallsyms_has() will always exit when a symbol has not found, it
> >>> breaks the test itself. Unexpected errors occur:
> >>>
> >>>  007 userspace pm add & remove address
> >>>        syn                                 [ OK ]
> >>>        synack                              [ OK ]
> >>>        ack                                 [ OK ]
> >>>        add                                 [ OK ]
> >>>        echo                                [ OK ]
> >>>        mptcp_info subflows=2:2             [ OK ]
> >>>        mptcp_info subflows_total=3:3       [ OK ]
> >>>        mptcp_info add_addr_signal=2:2      [ OK ]
> >>>        mptcp_info last_data_sent=191:18    [ OK ]
> >>>        mptcp_info last_data_recv=40:68     [ OK ]
> >>>        mptcp_info last_ack_recv=93:74      [ OK ]
> >>>        dump addrs signal                   ERROR: missing feature: \
> >>> 			mptcp_userspace_pm_dump_addr$ symbol not found
> >>>  Cannot open network namespace "ns1-65d6fb5a-5FviVK": \
> >>> 			No such file or directory
> >>>  Cannot open network namespace "ns2-65d6fb5a-5FviVK": \
> >>> 			No such file or directory
> >>>  cmp: /tmp/tmp.a7osJs7Nj0: No such file or directory
> >>>  cmp: /tmp/tmp.f02z6brCQu: No such file or directory
> >>>  cat: /tmp/tmp.27TzxD2efV: No such file or directory
> >>> not ok 1 test: selftest_mptcp_join # FAIL
> >>>
> >>> To fix this, this patch adds a new argument 'continue' for the helper
> >>> mptcp_lib_fail_if_expected_feature() to control whether exit or not.
> >>>
> >>> Always set this argument to 1 in mptcp_lib_kallsyms_has().
> >>>
> >>> Fixes: 83013bdf90a ("selftests: mptcp: connect: skip if MPTCP is not supported")
> >>
> >> I'm not sure to understand why you sent this :)
> > 
> > Say in the test "signal addresses race test":
> > 
> >                 if ! mptcp_lib_kallsyms_has "mptcp_pm_subflow_check_next$"; then
> >                         chk_join_nr 3 3 2
> >                         chk_add_nr 4 4
> >                 else
> >                         chk_join_nr 3 3 3
> >                         # the server will not signal the address terminating
> >                         # the MPC subflow
> >                         chk_add_nr 3 3
> >                 fi
> > 
> > This code will never run to this block:
> > 
> >                         chk_join_nr 3 3 2
> >                         chk_add_nr 4 4
> > 
> > Since if no symbal of mptcp_pm_subflow_check_next, mptcp_lib_kallsyms_has exit.
> 
> This block will never be executed in our CI, because our CI always run
> the selftests linked to the same version of the kernel. In this case,
> that would not be normal to execute this block in our CI, it has to
> complain because we expect to have this symbol in the latest version.
> 
> Other CIs, like LKFT, which are validating stable kernels (e.g. 5.15.x)
> using the selftests from the last stable kernel (e.g. 6.7.x) will not
> have SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES env var being set to 1 as
> we did in our CI. Then it is fine, tests and checks will be skipped if
> some features are missing:
> 
> https://tuxapi.tuxsuite.com/v1/groups/linaro/projects/lkft/tests/2cJxC6Sua3VVzRZmpQYKK2jzzNN/logs?format=html
> 
> > You can trigger this by renaming "mptcp_pm_subflow_check_next$" to
> > "no_mptcp_pm_subflow_check_next$", an non-exist function name, for test.
> 
> I confirm that if you use 'mptcp-upstream-virtme-docker', this env var
> will be set to 1 by default:
> 
>   SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES=1
> 
> You can unset it on your side, but it only makes sense to do that if you
> work on stable kernels (and you use selftests from a more recent
> version, not the ones attached to the kernel you are validating).
> 
> So yes, in this environment (where we force setting the env var to 1,
> not the "default" behaviour), picking a non-existing function name will
> cause the test to fail: but that's what we want → we want to be notified
> if we picked a non-existing function, like it happened recently:
> 
>   https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7988706773

Thanks for your explanation, it took me some time to understand it. Yes, we
can use SELFTESTS_MPTCP_LIB_EXPECT_ALL_FEATURES to control whether exit or
not. Let's drop this patch.

-Geliang

> 
> Cheers,
> Matt
> -- 
> Sponsored by the NGI0 Core fund.