[PATCH mptcp-next v2 0/2] selftests: fix mptcp_join test

[PATCH mptcp-next v2 0/2] selftests: fix mptcp_join test

[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.

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

-- 
2.41.0

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

[Andrea Claudi]

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 <aclaudi@redhat.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 0ae8cafde439..b972feb9a3a2 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -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
-- 
2.41.0

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

[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 <aclaudi@redhat.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index b972feb9a3a2..4938baa4c5fe 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -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"
-- 
2.41.0

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

[MPTCP CI]

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)

Re: [PATCH mptcp-next v2 0/2] selftests: fix mptcp_join test

[Matthieu Baerts]

Hi Andrea,

On 27/07/2023 13:08, Andrea Claudi wrote:
> 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.

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

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

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
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

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

[Matthieu Baerts]

Hi Andrea,

On 27/07/2023 13:08, Andrea Claudi wrote:
> 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")

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
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

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

[Andrea Claudi]

On Thu, Jul 27, 2023 at 06:52:50PM +0200, Matthieu Baerts wrote:
> Hi Andrea,
> 
> On 27/07/2023 13:08, Andrea Claudi wrote:
> > 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")
> 
> 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.

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

Re: [PATCH mptcp-next v2 0/2] selftests: fix mptcp_join test

[Andrea Claudi]

On Thu, Jul 27, 2023 at 06:52:45PM +0200, Matthieu Baerts wrote:
> Hi Andrea,
> 
> On 27/07/2023 13:08, Andrea Claudi wrote:
> > 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.
> 
> Thank you for the v2, it looks good to me!
> 
> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> 
> 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

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

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

[MPTCP CI]

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


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)

Re: [PATCH mptcp-next v2 0/2] selftests: fix mptcp_join test

[Matthieu Baerts]

Hi Andrea,

On 27/07/2023 13:08, Andrea Claudi wrote:
> 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
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net