Re: [MPTCP] [PATCH] mptcp: selftests: increase timeout and return error on ping failure

Re: [MPTCP] [PATCH] mptcp: selftests: increase timeout and return error on ping failure

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

Hi Florian, Paolo,

On 11/06/2019 10:49, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 11/06/2019 10:43, Paolo Abeni wrote:
>> Hi,
>>
>> Sorry for lagging behind ...
>> On Tue, 2019-06-11 at 10:32 +0200, Matthieu Baerts wrote:
>>> Hi Florian, Paolo,
>>>
>>> On 05/06/2019 11:36, Florian Westphal wrote:
>>>> Now that we add a qdisc that does random drop, there is a small chance
>>>> for the ping to be dropped, so this triggers:
>>>> ns2 -> 10.0.1.2 connectivity [ FAIL ]
>>>>
>>>> To fix this, split the ping test part to its own loop and only
>>>> add the qdiscs after the ping tests have completed.
>>>>
>>>> Also, the script would still exit with 0 status in this case, so update
>>>> run_tests to check the return value of do_transfer() rather than rely on
>>>> that function to set the global exit state in all cases.
>>>>
>>>> Last, increase the timeout: with qdiscs present, 10 seconds is too low:
>>>> in some rare cases sender or receiver exited too early.
>>>
>>> The patch looks good to me!
>>>
>>> @Paolo: as a major contributor to this shell script, is this patch also
>>> looks good to you?
>>
>> Just reviewed. Yes, the patch LGTM, too.
> 
> Thank you for the quick reply!

Thank you for the patch and the review!

- e9d3a8a4a460: "squashed" in "mptcp: selftests: switch to netns+veth
based tests"
- 9d00b70dbd94: conflict
- c088591c7178..7311dd424a7e: result

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: selftests: increase timeout and return error on ping failure

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

Hi Paolo,

On 11/06/2019 10:43, Paolo Abeni wrote:
> Hi,
> 
> Sorry for lagging behind ...
> On Tue, 2019-06-11 at 10:32 +0200, Matthieu Baerts wrote:
>> Hi Florian, Paolo,
>>
>> On 05/06/2019 11:36, Florian Westphal wrote:
>>> Now that we add a qdisc that does random drop, there is a small chance
>>> for the ping to be dropped, so this triggers:
>>> ns2 -> 10.0.1.2 connectivity [ FAIL ]
>>>
>>> To fix this, split the ping test part to its own loop and only
>>> add the qdiscs after the ping tests have completed.
>>>
>>> Also, the script would still exit with 0 status in this case, so update
>>> run_tests to check the return value of do_transfer() rather than rely on
>>> that function to set the global exit state in all cases.
>>>
>>> Last, increase the timeout: with qdiscs present, 10 seconds is too low:
>>> in some rare cases sender or receiver exited too early.
>>
>> The patch looks good to me!
>>
>> @Paolo: as a major contributor to this shell script, is this patch also
>> looks good to you?
> 
> Just reviewed. Yes, the patch LGTM, too.

Thank you for the quick reply!

>> Just one question for later: should we print something when we start
>> adding delays, losses and reordering?
> 
> I *think* it's not required. We could print the traversed artifacts in
> the test name/info line, but looks a bit overkill/overcomplicated.

Fine for me! I will apply this patch in our repo ASAP today!

Cheers,
Matt

> Cheers,
> 
> Paolo
> 

-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: selftests: increase timeout and return error on ping failure

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

Hi,

Sorry for lagging behind ...
On Tue, 2019-06-11 at 10:32 +0200, Matthieu Baerts wrote:
> Hi Florian, Paolo,
> 
> On 05/06/2019 11:36, Florian Westphal wrote:
> > Now that we add a qdisc that does random drop, there is a small chance
> > for the ping to be dropped, so this triggers:
> > ns2 -> 10.0.1.2 connectivity [ FAIL ]
> > 
> > To fix this, split the ping test part to its own loop and only
> > add the qdiscs after the ping tests have completed.
> > 
> > Also, the script would still exit with 0 status in this case, so update
> > run_tests to check the return value of do_transfer() rather than rely on
> > that function to set the global exit state in all cases.
> > 
> > Last, increase the timeout: with qdiscs present, 10 seconds is too low:
> > in some rare cases sender or receiver exited too early.
> 
> The patch looks good to me!
> 
> @Paolo: as a major contributor to this shell script, is this patch also
> looks good to you?

Just reviewed. Yes, the patch LGTM, too.

> Just one question for later: should we print something when we start
> adding delays, losses and reordering?

I *think* it's not required. We could print the traversed artifacts in
the test name/info line, but looks a bit overkill/overcomplicated.

Cheers,

Paolo

Re: [MPTCP] [PATCH] mptcp: selftests: increase timeout and return error on ping failure

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

Hi Florian, Paolo,

On 05/06/2019 11:36, Florian Westphal wrote:
> Now that we add a qdisc that does random drop, there is a small chance
> for the ping to be dropped, so this triggers:
> ns2 -> 10.0.1.2 connectivity [ FAIL ]
> 
> To fix this, split the ping test part to its own loop and only
> add the qdiscs after the ping tests have completed.
> 
> Also, the script would still exit with 0 status in this case, so update
> run_tests to check the return value of do_transfer() rather than rely on
> that function to set the global exit state in all cases.
> 
> Last, increase the timeout: with qdiscs present, 10 seconds is too low:
> in some rare cases sender or receiver exited too early.

The patch looks good to me!

@Paolo: as a major contributor to this shell script, is this patch also
looks good to you?

Thank you for having also cleaned the mix of spaces and tabs!

Just one question for later: should we print something when we start
adding delays, losses and reordering?

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] [PATCH] mptcp: selftests: increase timeout and return error on ping failure

[Florian Westphal]

[-- Attachment #1: Type: text/plain, Size: 4735 bytes --]

Now that we add a qdisc that does random drop, there is a small chance
for the ping to be dropped, so this triggers:
ns2 -> 10.0.1.2 connectivity [ FAIL ]

To fix this, split the ping test part to its own loop and only
add the qdiscs after the ping tests have completed.

Also, the script would still exit with 0 status in this case, so update
run_tests to check the return value of do_transfer() rather than rely on
that function to set the global exit state in all cases.

Last, increase the timeout: with qdiscs present, 10 seconds is too low:
in some rare cases sender or receiver exited too early.

Signed-off-by: Florian Westphal <fw(a)strlen.de>
---
 .../selftests/net/mptcp/mptcp_connect.sh      | 66 ++++++++++++++-----
 1 file changed, 49 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 6c1e19a39628..0a832c8e6b4f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -7,6 +7,7 @@ cin=""
 cout=""
 ksft_skip=4
 capture=0
+timeout=30
 
 TEST_COUNT=0
 
@@ -64,7 +65,6 @@ ip -net ns2 link set ns2eth1 up
 ip -net ns2 addr add 10.0.2.1/24 dev ns2eth3
 ip -net ns2 link set ns2eth3 up
 ip -net ns2 route add default via 10.0.2.2
-tc -net ns2 qdisc add dev ns2eth3 root netem loss random 1
 ip netns exec ns2 sysctl -q net.ipv4.ip_forward=1
 
 ip -net ns3 addr add 10.0.2.2/24 dev ns3eth2
@@ -73,8 +73,7 @@ ip -net ns3 link set ns3eth2 up
 ip -net ns3 addr add 10.0.3.2/24 dev ns3eth4
 ip -net ns3 link set ns3eth4 up
 ip -net ns3 route add default via 10.0.2.1
-tc -net ns3 qdisc add dev ns3eth4 root netem delay 10ms reorder 25% 50% gap 5
-ip netns exec ns3 ethtool -K ns3eth2 tso off
+ip netns exec ns3 ethtool -K ns3eth2 tso off 2>/dev/null
 ip netns exec ns3 sysctl -q net.ipv4.ip_forward=1
 
 ip -net ns4 addr add 10.0.3.1/24 dev ns4eth3
@@ -106,6 +105,19 @@ check_transfer()
 	return 0
 }
 
+do_ping()
+{
+	listener_ns="$1"
+	connector_ns="$2"
+	connect_addr="$3"
+
+	ip netns exec ${connector_ns} ping -q -c 1 $connect_addr >/dev/null
+	if [ $? -ne 0 ] ; then
+		echo "$listener_ns -> $connect_addr connectivity [ FAIL ]" 1>&2
+		ret=1
+	fi
+}
+
 do_transfer()
 {
 	listener_ns="$1"
@@ -120,11 +132,6 @@ do_transfer()
 	:> "$cout"
 	:> "$sout"
 	:> "$capout"
-	ip netns exec ${connector_ns} ping -q -c 1 $connect_addr >/dev/null
-	if [ $? -ne 0 ] ; then
-		echo "$listener_ns -> $connect_addr connectivity [ FAIL ]" 1>&2
-		return 1
-	fi
 
 	printf "%-4s %-5s -> %-4s (%s:%d) %-5s\t" ${connector_ns} ${cl_proto} ${listener_ns} ${connect_addr} ${port} ${srv_proto}
 
@@ -143,12 +150,12 @@ do_transfer()
 	    sleep 1
 	fi
 
-	ip netns exec ${listener_ns} ./mptcp_connect -t 10 -l -p $port -s ${srv_proto} 0.0.0.0 < "$sin" > "$sout" &
+	ip netns exec ${listener_ns} ./mptcp_connect -t $timeout -l -p $port -s ${srv_proto} 0.0.0.0 < "$sin" > "$sout" &
 	spid=$!
 
 	sleep 1
 
-	ip netns exec ${connector_ns} ./mptcp_connect -t 10 -p $port -s ${cl_proto} $connect_addr < "$cin" > "$cout" &
+	ip netns exec ${connector_ns} ./mptcp_connect -t $timeout -p $port -s ${cl_proto} $connect_addr < "$cin" > "$cout" &
 	cpid=$!
 
 	wait $cpid
@@ -169,7 +176,6 @@ do_transfer()
 		ip netns exec ${connector_ns} ss -nita 1>&2 -o "dport = :$port"
 
 		cat "$capout"
-		ret=$rets
 		return 1
 	fi
 
@@ -209,20 +215,46 @@ make_file()
 
 run_tests()
 {
-        listener_ns="$1"
-        connector_ns="$2"
+	listener_ns="$1"
+	connector_ns="$2"
 	connect_addr="$3"
+	lret=0
+
+	for proto in MPTCP TCP;do
+		do_transfer ${listener_ns} ${connector_ns} MPTCP "$proto" ${connect_addr}
+		lret=$?
+		if [ $lret -ne 0 ]; then
+			ret=$lret
+			return
+		fi
+	done
 
-	do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP ${connect_addr}
-	[ $? -ne 0 ] && return
-	do_transfer ${listener_ns} ${connector_ns} MPTCP TCP ${connect_addr}
-	[ $? -ne 0 ] && return
 	do_transfer ${listener_ns} ${connector_ns} TCP MPTCP ${connect_addr}
+	lret=$?
+	if [ $lret -ne 0 ]; then
+		ret=$lret
+		return
+	fi
 }
 
 make_file "$cin" "client"
 make_file "$sin" "server"
 
+for sender in 1 2 3 4;do
+	do_ping ns1 ns$sender 10.0.1.1
+
+	do_ping ns2 ns$sender 10.0.1.2
+	do_ping ns2 ns$sender 10.0.2.1
+
+	do_ping ns3 ns$sender 10.0.2.2
+	do_ping ns3 ns$sender 10.0.3.2
+
+	do_ping ns4 ns$sender 10.0.3.1
+done
+
+tc -net ns2 qdisc add dev ns2eth3 root netem loss random 1
+tc -net ns3 qdisc add dev ns3eth4 root netem delay 10ms reorder 25% 50% gap 5
+
 for sender in 1 2 3 4;do
 	run_tests ns1 ns$sender 10.0.1.1
 
-- 
2.21.0