Skip to contentSkip to conversation

selftests: fix mptcp_join test

PATCH Superseded Andrea Claudi 3 participants 10 messages Jun 23, 2023 – Jun 27, 2023

Subsystems (4)
Networking Network drivers Kselftest Networking MPTCP

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Andrea Claudi

author
#0 lore raw

cover selftests: fix mptcp_join test

This series fixes two mptcp_join testcases.
- '001 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;

- '002 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.

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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Signature · 1 line
2.41.0
author
#1 lore raw

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

mptcp_join '002 delete and re-add' test currently fails in the 'after delete' testcase.

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 simply not using the $addr variable in pm_nl_del_endpoint().

Fixes 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")
Signed-off-by Andrea Claudi <[email protected]>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/net/mptcp/mptcp_join.sh +1 -1
index 0ae8cafde439..5424dcacfffa 100755
@@ -678,7 +678,7 @@ pm_nl_del_endpoint()
 	local addr=$3
 
 	if [ $ip_mptcp -eq 1 ]; then
-		ip -n $ns mptcp endpoint delete id $id $addr
+		ip -n $ns mptcp endpoint delete id $id
 	else
 		ip netns exec $ns ./pm_nl_ctl del $id $addr
 	fi
Signature · 1 line
2.41.0
author
#4 lore raw

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

In reply to Andrea Claudi

mptcp_join '001 implicit EP' test currently fails 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 69c6ce7b6eca ("selftests: mptcp: add implicit endpoint test case")
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 5424dcacfffa..6c3525e42273 100755
@@ -768,10 +768,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
#5 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/5461583669755904
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5461583669755904/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
- Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
- Task: https://cirrus-ci.com/task/6587483576598528
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6587483576598528/summary/summary.txt

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

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

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

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)

#9 lore raw
In reply to Andrea Claudi

Hi Andrea,

Quoted 6 lines · Andrea Claudi

This series fixes two mptcp_join testcases.
- '001 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

Quoted 8 lines
    present in the iproute output;

- '002 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.

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

Thank you for these patches!

I have some comments, please see my replies on the individual patches.

Do you mind sending a v2 only to MPTCP ML -- not to Netdev and its maintainers -- if you don't mind? When these patches will be ready, we will apply them in MPTCP tree and send them later to netdev.

(Maybe we should modify the MAINTAINERS file to exclude Netdev maintainers and list when someone sends a patch.)

Cheers,
Matt

Signature · 2 lines
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
#2 lore raw

Re: [PATCH net 1/2] selftests: mptcp: join: fix 'delete and re-add' test

In reply to Andrea Claudi

Hi Andrea,

Quoted 2 lines · Andrea Claudi

mptcp_join '002 delete and re-add' test currently fails in the 'after delete' testcase.

I guess it only fails if you use "-i" option to use "ip mptcp" instead of "pm_nl_ctl", right?

MPTCP CI doesn't launch the tests with the "-i" option.

Can you mention that it fails only when using "ip mptcp" which is not the default mode please? It might be good to include that in the title too not to think the test is broken and the CI didn't complain about that.

BTW, how did you launch mptcp_join.sh selftest to have this test launched as second position ("002")? With "-Ii"?

(you can remove this "002": it is specific to the way you launched the test, not using the default mode)

Quoted 8 lines

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 simply not using the $addr variable in pm_nl_del_endpoint().

If you do that, are you not going to break other tests? e.g.
- "remove id 0 subflow"
- "remove id 0 address"

(I didn't check all possibilities, maybe not or maybe there are others)

Because if you specify the ID 0, you do need to specify the address, no?

Quoted 11 lines
Fixes 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")
Signed-off-by Andrea Claudi <[email protected]>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/net/mptcp/mptcp_join.sh +0 -0
index 0ae8cafde439..5424dcacfffa 100755

tools/testing/selftests/net/mptcp/mptcp_join.sh pm_nl_del_endpoint() · line 681

Quoted 5 lines
tools/testing/selftests/net/mptcp/mptcp_join.sh +1 -1
@@ -678,7 +678,7 @@ pm_nl_del_endpoint()
 	local addr=$3
 
 	if [ $ip_mptcp -eq 1 ]; then
-		ip -n $ns mptcp endpoint delete id $id $addr
+		ip -n $ns mptcp endpoint delete id $id

Should you not add "${addr}" only if ${id} == 1?

tools/testing/selftests/net/mptcp/mptcp_join.sh pm_nl_del_endpoint() · line 684

Quoted 3 lines
tools/testing/selftests/net/mptcp/mptcp_join.sh +0 -0
 	else
 		ip netns exec $ns ./pm_nl_ctl del $id $addr
 	fi

Cheers,
Matt

Signature · 2 lines
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
#6 lore raw

Re: [PATCH net 2/2] selftests: mptcp: join: fix 'implicit EP' test

In reply to Andrea Claudi

Hi Andrea,

Quoted 2 lines · Andrea Claudi

mptcp_join '001 implicit EP' test currently fails because of two reasons:

Same as on patch 1/2: can you remove "001" and mention it is only failing when using "ip mptcp" ("-i" option) please?

Quoted 3 lines

- iproute v6.3.0 does not support the implicit flag, fixed with iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit flag")

Thank you for that!

Out of curiosity: why is it in iproute2-next (following net-next tree, for v6.5) and not in iproute2 tree (following -net / Linus tree: for v6.4)?

Quoted 21 lines

- 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 69c6ce7b6eca ("selftests: mptcp: add implicit endpoint test case")
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 +0 -0
index 5424dcacfffa..6c3525e42273 100755
@@ -768,10 +768,11 @@ pm_nl_check_endpoint()
 	fi
 
 	if [ $ip_mptcp -eq 1 ]; then

tools/testing/selftests/net/mptcp/mptcp_join.sh pm_nl_check_endpoint() · line 778

Quoted 10 lines
tools/testing/selftests/net/mptcp/mptcp_join.sh +3 -2
+		# 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"

It looks good, no need to change anything here but later (not for -net anyway), we should probably parse the JSON output of "ip -j mptcp" instead.

Cheers,
Matt

Signature · 2 lines
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
author
#3 lore raw

Re: [PATCH net 1/2] selftests: mptcp: join: fix 'delete and re-add' test

In reply to Matthieu Baerts
Quoted 1 line · Matthieu Baerts

Hi Andrea,

Hi Matthieu,
Thanks for your review.

Quoted 8 lines · Andrea Claudi
Quoted 2 lines

mptcp_join '002 delete and re-add' test currently fails in the 'after delete' testcase.

I guess it only fails if you use "-i" option to use "ip mptcp" instead of "pm_nl_ctl", right?

Yes, exactly.

Quoted 6 lines

MPTCP CI doesn't launch the tests with the "-i" option.

Can you mention that it fails only when using "ip mptcp" which is not the default mode please? It might be good to include that in the title too not to think the test is broken and the CI didn't complain about that.

Sure, will do that.

Quoted 3 lines

BTW, how did you launch mptcp_join.sh selftest to have this test launched as second position ("002")? With "-Ii"?

Yes, that's exactly the case, I use "./mptcp_join.sh -I -i"

Quoted 2 lines

(you can remove this "002": it is specific to the way you launched the test, not using the default mode)

Will do.

Quoted 10 lines
Quoted 9 lines

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 simply not using the $addr variable in pm_nl_del_endpoint().

Quoted 8 lines

If you do that, are you not going to break other tests? e.g.
- "remove id 0 subflow"
- "remove id 0 address"

(I didn't check all possibilities, maybe not or maybe there are others)

Because if you specify the ID 0, you do need to specify the address, no?

That's right, of course. I'll fix that in v2 and make sure no other tests are impacted with a "mptcp_join.sh -i" run.

Quoted 29 lines
Quoted 11 lines
Fixes 34aa6e3bccd8 ("selftests: mptcp: add ip mptcp wrappers")
Signed-off-by Andrea Claudi <[email protected]>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/testing/selftests/net/mptcp/mptcp_join.sh +0 -0
index 0ae8cafde439..5424dcacfffa 100755

tools/testing/selftests/net/mptcp/mptcp_join.sh pm_nl_del_endpoint() · line 681

Quoted 5 lines
tools/testing/selftests/net/mptcp/mptcp_join.sh +1 -1
@@ -678,7 +678,7 @@ pm_nl_del_endpoint()
 	local addr=$3
 
 	if [ $ip_mptcp -eq 1 ]; then
-		ip -n $ns mptcp endpoint delete id $id $addr
+		ip -n $ns mptcp endpoint delete id $id

Should you not add "${addr}" only if ${id} == 1?

tools/testing/selftests/net/mptcp/mptcp_join.sh pm_nl_del_endpoint() · line 684

Quoted 3 lines
tools/testing/selftests/net/mptcp/mptcp_join.sh +0 -0
 	else
 		ip netns exec $ns ./pm_nl_ctl del $id $addr
 	fi

Cheers,
Matt

Signature · 2 lines
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
author
#7 lore raw

Re: [PATCH net 2/2] selftests: mptcp: join: fix 'implicit EP' test

In reply to Matthieu Baerts
Quoted 9 lines · Matthieu Baerts

Hi Andrea,

Quoted 2 lines · Andrea Claudi

mptcp_join '001 implicit EP' test currently fails because of two reasons:

Same as on patch 1/2: can you remove "001" and mention it is only failing when using "ip mptcp" ("-i" option) please?

Sure, no problem.

Quoted 9 lines
Quoted 3 lines

- iproute v6.3.0 does not support the implicit flag, fixed with iproute2-next commit 3a2535a41854 ("mptcp: add support for implicit flag")

Thank you for that!

Out of curiosity: why is it in iproute2-next (following net-next tree, for v6.5) and not in iproute2 tree (following -net / Linus tree: for v6.4)?

I usually target fixes to iproute2 and new stuff to iproute2-next, no other reason than that. But I see your point here, having this on -net may end up in the commit not landing in the same release cycle.

Should I send v2 for this series to mptcp-next, then?

Andrea

Quoted 41 lines
Quoted 21 lines

- 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 69c6ce7b6eca ("selftests: mptcp: add implicit endpoint test case")
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 +0 -0
index 5424dcacfffa..6c3525e42273 100755
@@ -768,10 +768,11 @@ pm_nl_check_endpoint()
 	fi
 
 	if [ $ip_mptcp -eq 1 ]; then

tools/testing/selftests/net/mptcp/mptcp_join.sh pm_nl_check_endpoint() · line 778

Quoted 10 lines
tools/testing/selftests/net/mptcp/mptcp_join.sh +3 -2
+		# 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"

It looks good, no need to change anything here but later (not for -net anyway), we should probably parse the JSON output of "ip -j mptcp" instead.

Cheers,
Matt

Signature · 2 lines
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
#8 lore raw

Re: [PATCH net 2/2] selftests: mptcp: join: fix 'implicit EP' test

Hi Andrea,

Thank you for your replies!

Quoted 2 lines · Andrea Claudi
Quoted 1 line · Matthieu Baerts

On 23/06/2023 14:19, Andrea Claudi wrote:

(...)

Quoted 7 lines
Quoted 3 lines

Out of curiosity: why is it in iproute2-next (following net-next tree, for v6.5) and not in iproute2 tree (following -net / Linus tree: for v6.4)?

I usually target fixes to iproute2 and new stuff to iproute2-next, no other reason than that. But I see your point here, having this on -net may end up in the commit not landing in the same release cycle.

I see, thank you for the explanation.

If I'm not mistaken, a big difference with how the 'net' tree is handled -- i.e. only bug fixes -- 'iproute2' tree accepts new features as long as the kernel using the 'net' tree supports these new features. If it depends on features only in 'net-next', then the patches should target 'iproute2-next'.

Quoted 1 line

Should I send v2 for this series to mptcp-next, then?

Yes please, only to the MPTCP list without netdev list and maintainers if you don't mind, just to avoid bothering too many people with MPTCP specific stuff :)

Cheers,
Matt

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