[MPTCP][PATCH mptcp-next] Squash to "selftests: mptcp: add testcase for active-back"

[MPTCP][PATCH mptcp-next] Squash to "selftests: mptcp: add testcase for active-back"

[Geliang Tang]

Drop the debug logs and make the output aligned:

link ns1eth3 down
01 multiple flows, signal, link failure syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        stale[ ok ]
Created /tmp/tmp.dltHx8oBcT (size 2048 KB) containing data sent by server
link ns1eth3 down
02 multiple flows, signal, bidirectional, link failure syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        stale[ ok ]
link ns1eth1 down
03 backup subflow unused with link failure syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        link usage [ ok ]
link ns1eth1 down
link ns1eth2 down
04 backup subflow used due to multiple links failure syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        stale[ ok ]
                                        link usage [ ok ]
link ns1eth1 down
link ns1eth2 down
05 backup subflow in use, bidirectional, link failure syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        stale[ ok ]
                                        link usage [ ok ]

->

01 multiple flows, signal, link failure syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        stale             [ ok ]
Created /tmp/tmp.JYR5OkgZ9C (size 6144 KB) containing data sent by server
02 multi flows, signal, bidi, link fail syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        stale             [ ok ]
03 backup subflow unused, link failure  syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        link usage        [ ok ]
04 backup flow used, multi links fail   syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        stale             [ ok ]
                                        link usage        [ ok ]
05 backup flow used, bidi, link failure syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [ ok ]
                                        stale             [ ok ]
                                        link usage        [ ok ]

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 220154cb92a7..d0518487b874 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -228,7 +228,6 @@ link_failure()
 
 	for l in $FAILING_LINKS; do
 		veth="ns1eth$l"
-		echo "link $veth down" 1>&2
 		ip -net "$ns" link set "$veth" down
 	done
 }
@@ -634,7 +633,7 @@ chk_stale_nr()
 	local stale_nr
 	local recover_nr
 
-	printf "%-39s %s" " " "stale"
+	printf "%-39s %s" " " "stale             "
 	stale_nr=`ip netns exec $ns nstat -as | grep MPTcpExtSubflowStale | awk '{print $2}'`
 	[ -z "$stale_nr" ] && stale_nr=0
 	recover_nr=`ip netns exec $ns nstat -as | grep MPTcpExtSubflowRecover | awk '{print $2}'`
@@ -878,7 +877,7 @@ chk_link_usage()
 	local tx_rate=$((tx_link * 100 / $tx_total))
 	local tolerance=5
 
-	printf "%-39s %s" " " "link usage "
+	printf "%-39s %s" " " "link usage        "
 	if [ $tx_rate -lt $((expected_rate - $tolerance)) -o \
 	     $tx_rate -gt $((expected_rate + $tolerance)) ]; then
 		echo "[fail] got $tx_rate% usage, expected $expected_rate%"
@@ -1039,7 +1038,7 @@ link_failure_tests()
 	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow
 	ip netns exec $ns2 ./pm_nl_ctl add 10.0.4.2 dev ns2eth4 flags subflow
 	run_tests $ns1 $ns2 10.0.1.1 2
-	chk_join_nr "multiple flows, signal, bidirectional, link failure" 3 3 3
+	chk_join_nr "multi flows, signal, bidi, link fail" 3 3 3
 	chk_add_nr 1 1
 	chk_stale_nr $ns2 1 -1 1
 
@@ -1053,7 +1052,7 @@ link_failure_tests()
 	export FAILING_LINKS="1"
 	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow,backup
 	run_tests $ns1 $ns2 10.0.1.1 1
-	chk_join_nr "backup subflow unused with link failure" 2 2 2
+	chk_join_nr "backup subflow unused, link failure" 2 2 2
 	chk_add_nr 1 1
 	chk_link_usage $ns2 ns2eth3 $cinsent 0
 
@@ -1067,7 +1066,7 @@ link_failure_tests()
 	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow,backup
 	export FAILING_LINKS="1 2"
 	run_tests $ns1 $ns2 10.0.1.1 1
-	chk_join_nr "backup subflow used due to multiple links failure" 2 2 2
+	chk_join_nr "backup flow used, multi links fail" 2 2 2
 	chk_add_nr 1 1
 	chk_stale_nr $ns2 2 4 2
 	chk_link_usage $ns2 ns2eth3 $cinsent 50
@@ -1081,7 +1080,7 @@ link_failure_tests()
 	ip netns exec $ns2 ./pm_nl_ctl limits 1 3
 	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow,backup
 	run_tests $ns1 $ns2 10.0.1.1 2
-	chk_join_nr "backup subflow in use, bidirectional, link failure" 2 2 2
+	chk_join_nr "backup flow used, bidi, link failure" 2 2 2
 	chk_add_nr 1 1
 	chk_stale_nr $ns2 1 -1 2
 	chk_link_usage $ns2 ns2eth3 $cinsent 50
-- 
2.31.1

Re: [MPTCP][PATCH mptcp-next] Squash to "selftests: mptcp: add testcase for active-back"

[Matthieu Baerts]

Hi Geliang,

On 21/07/2021 10:56, Geliang Tang wrote:
> Drop the debug logs and make the output aligned:

Good idea!

> 05 backup flow used, bidi, link failure syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                         add[ ok ] - echo  [ ok ]
>                                         stale             [ ok ]
>                                         link usage        [ ok ]

I wonder if it would not be even clearer to display one item per line, e.g.:


05 backup flow used, bidi, link failure:
     syn        [ ok ]
     synack     [ ok ]
     ack        [ ok ]
     add        [ ok ]
     echo       [ ok ]
     stale      [ ok ]
     link usage [ ok ]
06 (...)

But that is for another patch anyway. I can look at that later when
trying to make this test more reliable by removing fixed values for
"sleep" and others if nobody is already looking at that.

> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 220154cb92a7..d0518487b874 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -228,7 +228,6 @@ link_failure()
>  
>  	for l in $FAILING_LINKS; do
>  		veth="ns1eth$l"
> -		echo "link $veth down" 1>&2
>  		ip -net "$ns" link set "$veth" down
>  	done
>  }
> @@ -634,7 +633,7 @@ chk_stale_nr()
>  	local stale_nr
>  	local recover_nr
>  
> -	printf "%-39s %s" " " "stale"
> +	printf "%-39s %s" " " "stale             "
>  	stale_nr=`ip netns exec $ns nstat -as | grep MPTcpExtSubflowStale | awk '{print $2}'`
>  	[ -z "$stale_nr" ] && stale_nr=0
>  	recover_nr=`ip netns exec $ns nstat -as | grep MPTcpExtSubflowRecover | awk '{print $2}'`
> @@ -878,7 +877,7 @@ chk_link_usage()
>  	local tx_rate=$((tx_link * 100 / $tx_total))
>  	local tolerance=5
>  
> -	printf "%-39s %s" " " "link usage "
> +	printf "%-39s %s" " " "link usage        "

Do you mind if I change this when applying the patch to avoid plenty of
spaces?

  printf "%-39s %-18s" " " "stale"
  printf "%-39s %-18s" " " "link usage"

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

Re: [MPTCP][PATCH mptcp-next] Squash to "selftests: mptcp: add testcase for active-back"

[Geliang Tang]

Hi Matt,

Matthieu Baerts <matthieu.baerts@tessares.net> 于2021年7月22日周四 下午4:55写道：
>
> Hi Geliang,
>
> On 21/07/2021 10:56, Geliang Tang wrote:
> > Drop the debug logs and make the output aligned:
>
> Good idea!
>
> > 05 backup flow used, bidi, link failure syn[ ok ] - synack[ ok ] - ack[ ok ]
> >                                         add[ ok ] - echo  [ ok ]
> >                                         stale             [ ok ]
> >                                         link usage        [ ok ]
>
> I wonder if it would not be even clearer to display one item per line, e.g.:
>
>
> 05 backup flow used, bidi, link failure:
>      syn        [ ok ]
>      synack     [ ok ]
>      ack        [ ok ]
>      add        [ ok ]
>      echo       [ ok ]
>      stale      [ ok ]
>      link usage [ ok ]
> 06 (...)
>

I think the original display is much better than one item per line. Since
it put the related items in one line, and do the different tests in each
line. I think that's more clearer than one item per line. Do you agree?

> But that is for another patch anyway. I can look at that later when
> trying to make this test more reliable by removing fixed values for
> "sleep" and others if nobody is already looking at that.
>
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> >  tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index 220154cb92a7..d0518487b874 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -228,7 +228,6 @@ link_failure()
> >
> >       for l in $FAILING_LINKS; do
> >               veth="ns1eth$l"
> > -             echo "link $veth down" 1>&2
> >               ip -net "$ns" link set "$veth" down
> >       done
> >  }
> > @@ -634,7 +633,7 @@ chk_stale_nr()
> >       local stale_nr
> >       local recover_nr
> >
> > -     printf "%-39s %s" " " "stale"
> > +     printf "%-39s %s" " " "stale             "
> >       stale_nr=`ip netns exec $ns nstat -as | grep MPTcpExtSubflowStale | awk '{print $2}'`
> >       [ -z "$stale_nr" ] && stale_nr=0
> >       recover_nr=`ip netns exec $ns nstat -as | grep MPTcpExtSubflowRecover | awk '{print $2}'`
> > @@ -878,7 +877,7 @@ chk_link_usage()
> >       local tx_rate=$((tx_link * 100 / $tx_total))
> >       local tolerance=5
> >
> > -     printf "%-39s %s" " " "link usage "
> > +     printf "%-39s %s" " " "link usage        "
>
> Do you mind if I change this when applying the patch to avoid plenty of
> spaces?
>
>   printf "%-39s %-18s" " " "stale"
>   printf "%-39s %-18s" " " "link usage"

It's much better to avoid spaces like this, thanks Matt.

-Geliang

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

Re: [MPTCP][PATCH mptcp-next] Squash to "selftests: mptcp: add testcase for active-back"

[Matthieu Baerts]

Hi Geliang,

On 22/07/2021 11:28, Geliang Tang wrote:
> Matthieu Baerts <matthieu.baerts@tessares.net> 于2021年7月22日周四 下午4:55写道：
>> 05 backup flow used, bidi, link failure:
>>      syn        [ ok ]
>>      synack     [ ok ]
>>      ack        [ ok ]
>>      add        [ ok ]
>>      echo       [ ok ]
>>      stale      [ ok ]
>>      link usage [ ok ]
>> 06 (...)
>>
> 
> I think the original display is much better than one item per line. Since
> it put the related items in one line, and do the different tests in each
> line. I think that's more clearer than one item per line. Do you agree?

If everything was on one line, ideally with one global result at the
end, that would be fine for me. But here, when you have a long list of
tests with multiple '[ ok ]' on the same line but in different columns,
it is "hard" to spot the errors I think.

Here you can see the output when it fails:

https://api.cirrus-ci.com/v1/artifact/task/5542024628666368/tap_result/selftest_mptcp_join.tap

There are three columns and I think it is not "direct" to spot where is
the error.

If you have: plenty of "[ ok ]" with nothing after, it is easy to spot a
"[FAIL]", especially if something more is written on the right of it I
think.

An alternative would be to have one global "[ OK ]", e.g.

  03 backup subflow unused, link failure:  [ OK ]
  04 backup flow used, multi links fail:   [FAIL]
       syn        [ ok ]
       synack     [ ok ]
       ack        [ ok ]
       add        [ ok ]
       echo       [FAIL] # extra info
       stale      [ ok ]
       link usage [ ok ]
  05 backup flow used, bidi, link failure: [ OK ]

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

Re: [MPTCP][PATCH mptcp-next] Squash to "selftests: mptcp: add testcase for active-back"

[Matthieu Baerts]

Hi Geliang,

On 22/07/2021 10:55, Matthieu Baerts wrote:
> On 21/07/2021 10:56, Geliang Tang wrote:
>> Drop the debug logs and make the output aligned:

(...)

>> @@ -878,7 +877,7 @@ chk_link_usage()
>>  	local tx_rate=$((tx_link * 100 / $tx_total))
>>  	local tolerance=5
>>  
>> -	printf "%-39s %s" " " "link usage "
>> +	printf "%-39s %s" " " "link usage        "
> 
> Do you mind if I change this when applying the patch to avoid plenty of
> spaces?

Thank you for the patch! Just applied with this additional small
modification.

- bef6bd8eb873: "squashed" in "selftests: mptcp: add testcase for
active-back"
- 8135e0128562: "Signed-off-by" + "Co-developed-by"
- 85c4567fba08: selftests: mptcp join: avoid multiple spaces
- Results: 3a1215522891..1180d21b724d

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210722T092910
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210722T092910

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