Re: [PATCH mptcp-next v8 8/8] selftests: mptcp: add mp_fail testcases

[Matthieu Baerts <matthieu.baerts@tessares.net>]

Hi Geliang,

On 04/11/2021 14:13, Geliang Tang wrote:
> On Thu, Nov 04, 2021 at 12:48:38PM +0100, Matthieu Baerts wrote:
>> On 04/11/2021 11:30, Geliang Tang wrote:
>>> Is this a bug in act_pedit, or we use pedit incorrectly? How can I fix
>>> this?
>>
>> Do you mean you have this error?
>>
>>   parse error: Invalid numeric literal at line 1, column 47
>>
>> To me, it looks like a bug on printing data, so on 'tc' side.
>> Which version of 'tc' are you using? May you try to upgrade to a more
>> recent one? e.g. IPRoute2 5.15.0? Maybe it has already been fixed.
> 
> You're right, it's a tc bug. And fixed already in IPRoute2 5.15.0.

Which version were you using before?
I guess we need at least the 5.8.0 to have this commit:

https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/tc/m_pedit.c?id=081d6c310d3a6e0412431a9652f641dff3f72aee

If yes, we should probably skip/abort the test if we cannot parse the json.

(...)

>>>>>> +    # 1 subflow
>>>>>> +    reset
>>>>>> +    ip netns exec $ns1 ./pm_nl_ctl limits 0 2
>>>>>> +    ip netns exec $ns2 ./pm_nl_ctl limits 0 2
>>>>>> +    run_tests $ns1 $ns2 10.0.1.1 2 0 "fail_1" fast
>>>
>>> Here I passed 2 as the 4th argument of run_tests like the link_failure
>>> tests to create the bigger data files. I don't know why it dosen't work
>>> when passing 0 as the argument?
>>
>> Should we first insert the TC rule, then start the test, then look at
>> counters?
>> It would be "safer" because we would not need need to make sure we have
>> enough data to transfer. WDYT?
> 
> Good point, I'll try it.

Thanks!

May you also make sure that for this test, we transfer data only in one
direction? I don't know if for the other direction, we could not give
/dev/null or generate a 1 byte file.

>>>>>> +    chk_join_nr "MP_FAIL test, 1 subflow" 0 0 0 1 1
>>>>>> +
>>>>>> +    # multiple subflows
>>>>>> +    reset
>>>>>> +    ip netns exec $ns1 ./pm_nl_ctl limits 0 2
>>>>>> +    ip netns exec $ns2 ./pm_nl_ctl limits 0 2
>>>>>> +    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.2.2 dev ns2eth2 flags
>>>>>> subflow
>>>>>> +    run_tests $ns1 $ns2 10.0.1.1 2 0 "fail_2" fast
>>>>>> +    chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
>>>>>> +}
>>>>>> +
>>>>>> all_tests()
>>>>>> {
>>>>>>     subflows_tests
>>>>>> @@ -1853,6 +1907,7 @@ all_tests()
>>>>>>     checksum_tests
>>>>>>     deny_join_id0_tests
>>>>>>     fullmesh_tests
>>>>>> +    fail_tests
>>>>>> }
>>>>>>
>>>>>> usage()
>>>>>> @@ -1872,6 +1927,7 @@ usage()
>>>>>>     echo "  -S checksum_tests"
>>>>>>     echo "  -d deny_join_id0_tests"
>>>>>>     echo "  -m fullmesh_tests"
>>>>>> +    echo "  -F fail_tests"
>>>>>>     echo "  -c capture pcap files"
>>>>>>     echo "  -C enable data checksum"
>>>>>>     echo "  -h help"
>>>>>> @@ -1907,7 +1963,7 @@ if [ $do_all_tests -eq 1 ]; then
>>>>>>     exit $ret
>>>>>> fi
>>>>>>
>>>>>> -while getopts 'fsltra64bpkdmchCS' opt; do
>>>>>> +while getopts 'fsltra64bpkdmchCSF' opt; do
>>>>>>     case $opt in
>>>>>>         f)
>>>>>>             subflows_tests
>>>>>> @@ -1951,6 +2007,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
>>>>>>         m)
>>>>>>             fullmesh_tests
>>>>>>             ;;
>>>>>> +        F)
>>>>>> +            fail_tests
>>>>>> +            ;;
>>>>>>         c)
>>>>>>             ;;
>>>>>>         C)
>>>>>> -- 
>>>>>> 2.26.2
>>>>>
>>>>> When I run:
>>>>>
>>>>> $ sudo ./mptcp_join.sh -FC
>>>>>
>>>>> I get:
>>>>>
>>>>> """
>>>>> Created /tmp/tmp.UTIlkuhfRW (size 1 KB) containing data sent by client
>>>>> Created /tmp/tmp.pMPhJddNfQ (size 1 KB) containing data sent by server
>>>>> Created /tmp/tmp.OEx5cmVqbp (size 10075 KB) containing data sent by client
>>>>> Created /tmp/tmp.RAtBQXOXtR (size 6144 KB) containing data sent by server
>>>>> [ FAIL ] file received by server does not match (in, out):
>>>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
>>>>> Trailing bytes are:
>>>>> MPTCP_TEST_FILE_END_MARKER
>>>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
>>>>> Trailing bytes are:
>>>>> MPTCP_TEST_FILE_END_MARKER
>>>>> 01 MP_FAIL test, 1 subflow              syn[ ok ] - synack[ ok ] - ack[
>>>>> ok ]
>>>>>                                         sum[ ok ] - csum  [ ok ]
>>>>>                                         ftx[ ok ] - frx   [ ok ]
>>>>>                                         itx[ ok ] - irx   [ ok ]
>>>>> [ FAIL ] file received by server does not match (in, out):
>>>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
>>>>> Trailing bytes are:
>>>>> MPTCP_TEST_FILE_END_MARKER
>>>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
>>>>> Trailing bytes are:
>>>>> MPTCP_TEST_FILE_END_MARKER
>>>>> 02 MP_FAIL test, multiple subflows      syn[ ok ] - synack[ ok ] - ack[
>>>>> ok ]
>>>>>                                         sum[ ok ] - csum  [ ok ]
>>>>>                                         ftx[ ok ] - frx   [ ok ]
>>>>>                                         itx[ ok ] - irx   [ ok ]
>>>>> """
>>>>>
>>>>> The chk_join_nr output shows all [ ok ], which is good. But the file
>>>>> mismatches (I assume due to the inverted data) are failures because the
>>>>> checksums are intended to reject the bad data. Am I understanding the
>>>>> file mismatches correctly?
>>>>>
>>>>> If MP_FAIL is working, the files should match.
>>>>
>>>> I also have the same error on my side but not all the time.
>>>
>>> I got these file mismatches errors all the time. Could you please share
>>> your test script and the logs when you get the right output?
>>
>> I don't think I saw them for both tests all the time but it is possible
>> it was always there for the 1st one and not for the second one. I will
>> look at that next time.
>>
>>> The first test "01 MP_FAIL test, 1 subflow" will get the file mismatches
>>> all the time, since the checksum error data can't be retransmitted in
>>> the current codes.
>>
>> Should we disable this test then?
> 
> We should keep this test. It's useful for testing sending and receiving
> of the infinite mapping. The infinite mapping is only sent in this single
> subflow case.

We should then not check the output files if we expect not to receive
the full file. Or we should compare files up to the size of the
smallest. Because what has been given to the app should not be "corrupted".

>>> But the second test "02 MP_FAIL test, multiple subflows" shouldn't get
>>> the file mismatches. RST is sent on the checksum error subflow, so this
>>> subflow is closed. The data will be retransmitted on the other subflow.
>>> It works by using the old test case "[PATCH mptcp-next v7 8/8]
>>> DO-NOT-MERGE: mptcp: mp_fail test". Data will be retransmitted, and no
>>> the file mismatches. I don't know why it dosen't work by this new test
>>> case. In this test case, I saw the error data has been dropped, and the
>>> new data has been retransmitted in the log. I'll try to fix this.
>>
> 
> Could we merge patches 1-7 of this patchset into the export branch? Then
> I'll just iterate a new version of this test case patch. And if there's
> something I need to fix next week, I'll send the squash-to patch. Please
> discuss this with Mat in today's weekly meeting.

Sounds like a good point to me. I will wait for Mat's ACK about that!
I can raise the point at today's weekly meeting.

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