selftests: fix mptcp_join test

PATCH In review Andrea Claudi 3 participants 10 messages Jul 27, 2023 – Jul 28, 2023

Subsystems (3)

Networking Kselftest Networking MPTCP

lore

Share & tools

lore

Andrea Claudi

author

2023-07-27 11:09 UTC #0 lore raw

v2 cover selftests: fix mptcp_join test

This series fixes two mptcp_join testcases when using ip mptcp:
- 'implicit EP' fails because of:
- missing iproute support for mptcp 'implicit' flag, fixed with iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit flag")
- pm_nl_check_endpoint expecting two ip addresses, while only one is

    present in the iproute output;

- 'delete and re-add' fails because the endpoint delete command provide both id and ip address, while address should be provided only if id is 0.

Changelog:

v1 -> v2
- Clearly state test fails only when using ip mptcp, i.e. ./mptcp_join.sh -i
- Remove test numbers from commit messages
- On endpoint delete, continue to provide $addr when id is 0.

Andrea Claudi (2):
selftests: mptcp: join: fix 'delete and re-add' test
selftests: mptcp: join: fix 'implicit EP' test

 tools/testing/selftests/net/mptcp/mptcp_join.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Signature · 1 line

2.41.0

Andrea Claudi

author

2023-07-27 11:09 UTC #1 lore raw

PATCH v2 1/2 selftests: mptcp: join: fix 'delete and re-add' test

mptcp_join 'delete and re-add' test fails when using ip mptcp:

$ ./mptcp_join.sh -iI
<snip>
002 delete and re-add                    before delete[ ok ]
                                         mptcp_info subflows=1         [ ok ]
Error: argument "ADDRESS" is wrong: invalid for non-zero id address
                                         after delete[fail] got 2:2 subflows expected 1

This happens because endpoint delete includes an ip address while id is not 0, contrary to what is indicated in the ip mptcp man page:

"When used with the delete id operation, an IFADDR is only included when the ID is 0."

This fixes the issue using the $addr variable in pm_nl_del_endpoint() only when id is 0.

Fixes 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")

Signed-off-by Andrea Claudi <[email protected]>

---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
 1 file changed, 1 insertion(+)

tools/testing/selftests/net/mptcp/mptcp_join.sh +1 -0

index 0ae8cafde439..b972feb9a3a2 100755
@@ -678,6 +678,7 @@ pm_nl_del_endpoint()
 	local addr=$3
 
 	if [ $ip_mptcp -eq 1 ]; then
+		[ $id -ne 0 ] && addr=''
 		ip -n $ns mptcp endpoint delete id $id $addr
 	else
 		ip netns exec $ns ./pm_nl_ctl del $id $addr

Signature · 1 line

2.41.0

Andrea Claudi

author

2023-07-27 11:09 UTC #2 lore raw

PATCH v2 2/2 selftests: mptcp: join: fix 'implicit EP' test

In reply to Andrea Claudi

mptcp_join 'implicit EP' test currently fails when using ip mptcp:

$ ./mptcp_join.sh -iI
<snip>
001 implicit EP    creation[fail] expected '10.0.2.2 10.0.2.2 id 1 implicit' found '10.0.2.2 id 1 rawflags 10 '
Error: too many addresses or duplicate one: -22.
                   ID change is prevented[fail] expected '10.0.2.2 10.0.2.2 id 1 implicit' found '10.0.2.2 id 1 rawflags 10 '
                   modif is allowed[fail] expected '10.0.2.2 10.0.2.2 id 1 signal' found '10.0.2.2 id 1 signal '

This happens because of two reasons:
- iproute v6.3.0 does not support the implicit flag, fixed with iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit flag")
- pm_nl_check_endpoint wrongly expects the ip address to be repeated two times in iproute output, and does not account for a final whitespace in it.

This fixes the issue trimming the whitespace in the output string and removing the double address in the expected string.

Fixes 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")

Signed-off-by Andrea Claudi <[email protected]>

---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

tools/testing/selftests/net/mptcp/mptcp_join.sh +3 -2

index b972feb9a3a2..4938baa4c5fe 100755
@@ -769,10 +769,11 @@ pm_nl_check_endpoint()
 	fi
 
 	if [ $ip_mptcp -eq 1 ]; then
+		# get line and trim trailing whitespace
 		line=$(ip -n $ns mptcp endpoint show $id)
+		line="${line% }"
 		# the dump order is: address id flags port dev
-		expected_line="$addr"
-		[ -n "$addr" ] && expected_line="$expected_line $addr"
+		[ -n "$addr" ] && expected_line="$addr"
 		expected_line="$expected_line $id"
 		[ -n "$_flags" ] && expected_line="$expected_line ${_flags//","/" "}"
 		[ -n "$dev" ] && expected_line="$expected_line $dev"

Signature · 1 line

2.41.0

MPTCP CI

2023-07-27 12:29 UTC #3 lore raw

Re: selftests: mptcp: join: fix 'implicit EP' test: Tests Results

Hi Andrea,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6266739109920768
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6266739109920768/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5985264133210112
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5985264133210112/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/4859364226367488
- Summary: https://api.cirrus-ci.com/v1/artifact/task/4859364226367488/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5422314179788800
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5422314179788800/summary/summary.txt

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

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-debug

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 (Tessares)

Matthieu Baerts

2023-07-27 16:52 UTC #4 lore raw

Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: join: fix 'implicit EP' test

In reply to Andrea Claudi

Hi Andrea,

Quoted 17 lines · Andrea Claudi

mptcp_join 'implicit EP' test currently fails when using ip mptcp:
$ ./mptcp_join.sh -iI
<snip>
001 implicit EP    creation[fail] expected '10.0.2.2 10.0.2.2 id 1 implicit' found '10.0.2.2 id 1 rawflags 10 '
Error: too many addresses or duplicate one: -22.
                   ID change is prevented[fail] expected '10.0.2.2 10.0.2.2 id 1 implicit' found '10.0.2.2 id 1 rawflags 10 '
                   modif is allowed[fail] expected '10.0.2.2 10.0.2.2 id 1 signal' found '10.0.2.2 id 1 signal '
This happens because of two reasons:
- iproute v6.3.0 does not support the implicit flag, fixed with iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit flag")
- pm_nl_check_endpoint wrongly expects the ip address to be repeated two times in iproute output, and does not account for a final whitespace in it.

Quoted 4 lines

This fixes the issue trimming the whitespace in the output string and removing the double address in the expected string.
Fixes 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")

I guess it is not the right one to use, you might want to keep the one you had in v1:

Fixes 69c6ce7b6eca ("selftests: mptcp: add implicit endpoint test case")

No?

Cheers,
Matt

Signature · 2 lines

Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Andrea Claudi

author

2023-07-27 17:19 UTC #5 lore raw

Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: join: fix 'implicit EP' test

Quoted 23 lines · Matthieu Baerts

Hi Andrea,
Quoted 20 lines · Andrea Claudi
mptcp_join 'implicit EP' test currently fails when using ip mptcp:
$ ./mptcp_join.sh -iI
<snip>
001 implicit EP    creation[fail] expected '10.0.2.2 10.0.2.2 id 1 implicit' found '10.0.2.2 id 1 rawflags 10 '
Error: too many addresses or duplicate one: -22.
                   ID change is prevented[fail] expected '10.0.2.2 10.0.2.2 id 1 implicit' found '10.0.2.2 id 1 rawflags 10 '
                   modif is allowed[fail] expected '10.0.2.2 10.0.2.2 id 1 signal' found '10.0.2.2 id 1 signal '
This happens because of two reasons:
- iproute v6.3.0 does not support the implicit flag, fixed with iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit flag")
- pm_nl_check_endpoint wrongly expects the ip address to be repeated two times in iproute output, and does not account for a final whitespace in it.
This fixes the issue trimming the whitespace in the output string and removing the double address in the expected string.

Quoted 8 lines

Quoted 1 line
Fixes 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")
I guess it is not the right one to use, you might want to keep the one you had in v1:
Fixes 69c6ce7b6eca ("selftests: mptcp: add implicit endpoint test case")
No?

Yes, the one in v1 is the correct one. I somehow mixed it up amending the patch, sorry for that.

Quoted 7 lines

Cheers,
Matt
Signature · 2 lines
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

MPTCP CI

2023-07-27 18:39 UTC #6 lore raw

Re: selftests: mptcp: join: fix 'implicit EP' test: Tests Results

In reply to Andrea Claudi

Hi Andrea,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6040037498814464
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6040037498814464/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5477087545393152
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5477087545393152/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/4773400103616512
- Summary: https://api.cirrus-ci.com/v1/artifact/task/4773400103616512/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5708187203010560
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5708187203010560/summary/summary.txt

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

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-debug

For more details:

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

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

Matthieu Baerts

Reviewed-by on the v2 series

2023-07-27 16:52 UTC #7 lore raw

In reply to Andrea Claudi

Hi Andrea,

Quoted 11 lines · Andrea Claudi

This series fixes two mptcp_join testcases when using ip mptcp:
- 'implicit EP' fails because of:
- missing iproute support for mptcp 'implicit' flag, fixed with iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit flag")
- pm_nl_check_endpoint expecting two ip addresses, while only one is
    present in the iproute output;
- 'delete and re-add' fails because the endpoint delete command provide both id and ip address, while address should be provided only if id is 0.

Quoted 7 lines

Changelog:
v1 -> v2
- Clearly state test fails only when using ip mptcp, i.e. ./mptcp_join.sh -i
- Remove test numbers from commit messages
- On endpoint delete, continue to provide $addr when id is 0.

Thank you for the v2, it looks good to me!

Reviewed-by Matthieu Baerts <[email protected]>

Just one detail for patch 2/2 for the Fixes tag but I can fix that when applying the patches if needed and if that's OK for you.

These patches can be applied in mptcp-net (export-net branch).

Cheers,
Matt

Signature · 2 lines

Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Andrea Claudi

author

2023-07-27 17:20 UTC #8 lore raw

Quoted 22 lines · Matthieu Baerts

Hi Andrea,
Quoted 11 lines · Andrea Claudi
This series fixes two mptcp_join testcases when using ip mptcp:
- 'implicit EP' fails because of:
- missing iproute support for mptcp 'implicit' flag, fixed with iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit flag")
- pm_nl_check_endpoint expecting two ip addresses, while only one is
    present in the iproute output;
- 'delete and re-add' fails because the endpoint delete command provide both id and ip address, while address should be provided only if id is 0.
Quoted 7 lines
Changelog:
v1 -> v2
- Clearly state test fails only when using ip mptcp, i.e. ./mptcp_join.sh -i
- Remove test numbers from commit messages
- On endpoint delete, continue to provide $addr when id is 0.

Quoted 6 lines

Thank you for the v2, it looks good to me!
Reviewed-by Matthieu Baerts <[email protected]>
Just one detail for patch 2/2 for the Fixes tag but I can fix that when applying the patches if needed and if that's OK for you.

It's OK for me. Thanks for your review, Matthieu.

Regards,
Andrea

Quoted 9 lines

These patches can be applied in mptcp-net (export-net branch).
Cheers,
Matt
Signature · 2 lines
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Matthieu Baerts

2023-07-28 10:29 UTC #9 lore raw

In reply to Andrea Claudi

Hi Andrea,

Quoted 10 lines · Andrea Claudi

This series fixes two mptcp_join testcases when using ip mptcp:
- 'implicit EP' fails because of:
- missing iproute support for mptcp 'implicit' flag, fixed with iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit flag")
- pm_nl_check_endpoint expecting two ip addresses, while only one is
    present in the iproute output;
- 'delete and re-add' fails because the endpoint delete command provide both id and ip address, while address should be provided only if id is 0.

Thank you for the patches!

Now in our tree (fixes for -net) with my RvB tag and the use of the right Fixes tag:

New patches for t/upstream-net and t/upstream:
- 5013654dafcc: selftests: mptcp: join: fix 'delete and re-add' test
- b2b73765f521: selftests: mptcp: join: fix 'implicit EP' test
- Results: 401d3a99d2a9..be9e061da26c (export-net)
- Results: 570583e20711..c4e88c45e27d (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230728T102732
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230728T102732

Cheers,
Matt

Signature · 2 lines

Tessares | Belgium | Hybrid Access Solutions
www.tessares.net