[PATCH mptcp-next] selftests: mptcp: fix diag instability

[PATCH mptcp-next] selftests: mptcp: fix diag instability

[Paolo Abeni]

Increase the time waiting for mptcp sockets to get established,
to cope with very slow running host, or high jitter caused by
VMs.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
note: just to trigger the public CI, probably a better commit
message needed
---
 tools/testing/selftests/net/mptcp/diag.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index 2674ba20d524..baafd36c3e0e 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -87,9 +87,9 @@ chk_msk_nr 0 "no msk on netns creation"
 echo "b" | \
 	timeout ${timeout_test} \
 		ip netns exec $ns \
-			./mptcp_connect -p 10000 -j -t ${timeout_poll} \
+			./mptcp_connect -p 10000 -r 0 -t ${timeout_poll} \
 				127.0.0.1 >/dev/null &
-sleep 0.1
+sleep 0.4
 chk_msk_nr 2 "after MPC handshake "
 chk_msk_remote_key_nr 2 "....chk remote_key"
 chk_msk_fallback_nr 0 "....chk no fallback"
@@ -105,9 +105,9 @@ sleep 0.1
 echo "b" | \
 	timeout ${timeout_test} \
 		ip netns exec $ns \
-			./mptcp_connect -p 10001 -j -t ${timeout_poll} \
+			./mptcp_connect -p 10001 -r 0 -t ${timeout_poll} \
 				127.0.0.1 >/dev/null &
-sleep 0.1
+sleep 0.4
 chk_msk_fallback_nr 1 "check fallback"
 flush_pids
 
-- 
2.34.1

Re: [PATCH mptcp-next] selftests: mptcp: fix diag instability

[Matthieu Baerts]

Hi Paolo,

Thank you for looking at that!

On 02/02/2022 19:15, Paolo Abeni wrote:
> Increase the time waiting for mptcp sockets to get established,
> to cope with very slow running host, or high jitter caused by
> VMs.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> note: just to trigger the public CI, probably a better commit
> message needed

I was not able to reproduce the issue #248 on my side after 650+
attempts with a non debug kernel. I tried with that mode because the
public CI mostly have issues with this test without the debug.

But at the end, I managed to reproduce it with the debug kernel without
your patch. Trying your patch  in a loop on my side.

> ---
>  tools/testing/selftests/net/mptcp/diag.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index 2674ba20d524..baafd36c3e0e 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -87,9 +87,9 @@ chk_msk_nr 0 "no msk on netns creation"
>  echo "b" | \
>  	timeout ${timeout_test} \
>  		ip netns exec $ns \
> -			./mptcp_connect -p 10000 -j -t ${timeout_poll} \
> +			./mptcp_connect -p 10000 -r 0 -t ${timeout_poll} \
>  				127.0.0.1 >/dev/null &
> -sleep 0.1
> +sleep 0.4

I guess the ideal solution is to wait for the connection to be
established -- e.g. by monitoring /proc/net/tcp* like we do in
mptcp_connect.sh -- before checking stats, no?
But probably enough like that?

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

Re: [PATCH mptcp-next] selftests: mptcp: fix diag instability

[Mat Martineau]

On Wed, 2 Feb 2022, Paolo Abeni wrote:

> Increase the time waiting for mptcp sockets to get established,
> to cope with very slow running host, or high jitter caused by
> VMs.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> note: just to trigger the public CI, probably a better commit
> message needed

Ok, noted. I also haven't seen instability in diag.sh, for what it's 
worth.

Suggestion for the commit message, explain why the change from "-j" to "-r 
0" when invoking mptcp_connect.

-Mat

> ---
> tools/testing/selftests/net/mptcp/diag.sh | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
> index 2674ba20d524..baafd36c3e0e 100755
> --- a/tools/testing/selftests/net/mptcp/diag.sh
> +++ b/tools/testing/selftests/net/mptcp/diag.sh
> @@ -87,9 +87,9 @@ chk_msk_nr 0 "no msk on netns creation"
> echo "b" | \
> 	timeout ${timeout_test} \
> 		ip netns exec $ns \
> -			./mptcp_connect -p 10000 -j -t ${timeout_poll} \
> +			./mptcp_connect -p 10000 -r 0 -t ${timeout_poll} \
> 				127.0.0.1 >/dev/null &
> -sleep 0.1
> +sleep 0.4
> chk_msk_nr 2 "after MPC handshake "
> chk_msk_remote_key_nr 2 "....chk remote_key"
> chk_msk_fallback_nr 0 "....chk no fallback"
> @@ -105,9 +105,9 @@ sleep 0.1
> echo "b" | \
> 	timeout ${timeout_test} \
> 		ip netns exec $ns \
> -			./mptcp_connect -p 10001 -j -t ${timeout_poll} \
> +			./mptcp_connect -p 10001 -r 0 -t ${timeout_poll} \
> 				127.0.0.1 >/dev/null &
> -sleep 0.1
> +sleep 0.4
> chk_msk_fallback_nr 1 "check fallback"
> flush_pids
>
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next] selftests: mptcp: fix diag instability

[Matthieu Baerts]

Hi Paolo,

On 02/02/2022 20:35, Matthieu Baerts wrote:
> Hi Paolo,
> 
> Thank you for looking at that!
> 
> On 02/02/2022 19:15, Paolo Abeni wrote:
>> Increase the time waiting for mptcp sockets to get established,
>> to cope with very slow running host, or high jitter caused by
>> VMs.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>> note: just to trigger the public CI, probably a better commit
>> message needed
> 
> I was not able to reproduce the issue #248 on my side after 650+
> attempts with a non debug kernel. I tried with that mode because the
> public CI mostly have issues with this test without the debug.
> 
> But at the end, I managed to reproduce it with the debug kernel without
> your patch. Trying your patch  in a loop on my side.

Thanks to your patch, I was not able to reproduce the issue on my side
after more than 2500 attempts with a debug kernel.

I don't know if it is important because we often miss them but just in case:

Reported-and-tested-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/258

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