[ovs-dev] [PATCH] Fix race condition in test 121

Anton Ivanov anton.ivanov at cambridgegreys.com
Thu Apr 16 13:55:48 UTC 2020


On 16/04/2020 11:05, Dumitru Ceara wrote:
> On 4/16/20 9:21 AM, Anton Ivanov wrote:
>> On 15/04/2020 16:28, Numan Siddique wrote:
>>> On Wed, Apr 15, 2020 at 5:05 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>>>> On 4/15/20 1:22 PM, Ilya Maximets wrote:
>>>>> On 4/15/20 11:10 AM, anton.ivanov at cambridgegreys.com wrote:
>>>>>> From: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>>>>>>
>>>>>> Test 121 was configuring the interface with test traffic
>>>>>> before the LSP was configured. As a result, test traffic
>>>>>> could be processed before the LSP was set resulting in a
>>>>>> test failure
>>>>>>
>>>>>> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com>
>>>>>> ---
>>>>> Hi.  Thanks for improving the tests!
>>>>> I didn't check the main logic here, just a few general notes:
>>>>>
>>>>> 1. Please, add "ovn" to the subject prefix while sending ovn patches,
>>>>>      i.e. --subject-prefix="PATCH ovn", so the robot (and people) will
>>>>>      know where to apply this patch.
>>>>>
>>>>> 2. See inline.
>>>> 3. It also might make sense to have more meaningful patch name as
>>>>      test numbers change over time.  Something like:
>>>>        "ovn.at: Fix race condition in RARP test."
>>>>
>>>>> Best regards, Ilya Maximets.
>>>>>
>>>>>>    tests/ovn.at | 12 +++++++-----
>>>>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>>>>> index 9a44f0a6f..cf6956cdd 100644
>>>>>> --- a/tests/ovn.at
>>>>>> +++ b/tests/ovn.at
>>>>>> @@ -9318,6 +9318,7 @@ OVN_CHECK_PACKETS([hv3/vif1-tx.pcap],
>>>>>> [hv3-vif1.expected])
>>>>>>
>>>>>>    # Now add bridge-mappings on hv2, which should make everything work
>>>>>>    as hv2 ovs-vsctl set open .
>>>>>> external-ids:ovn-bridge-mappings=phys:br-phys
>>>>>> +sleep 1
>>>>> Do we need this sleep here?  Looks like unrelated change.
>>>>>
>>>>>>    # Wait until the patch ports are created in hv2 to connect br-int
>>>>>> to br-phys
>>>>>>    OVS_WAIT_UNTIL([test 1 = `as hv2 ovs-vsctl show | \
>>>>>> @@ -17356,11 +17357,6 @@ ovs-vsctl add-br br-phys
>>>>>>    ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
>>>>>>    ovn_attach n1 br-phys 192.168.0.1
>>>>>>
>>>>>> -ovs-vsctl add-port br-int vif11 -- \
>>>>>> -    set Interface vif11 external-ids:iface-id=lp11 \
>>>>>> -                          options:tx_pcap=hv1/vif11-tx.pcap \
>>>>>> -                          options:rxq_pcap=hv1/vif11-rx.pcap \
>>>>>> -                          ofport-request=11
>>>>>>
>>>>>>    lsp_name=lp11
>>>>>>
>>>>>> @@ -17368,6 +17364,12 @@ ovn-nbctl lsp-add ls1 lp11
>>>>>>    ovn-nbctl lsp-set-addresses lp11 "f0:00:00:00:00:11"
>>>>>>    ovn-nbctl lsp-set-port-security lp11 f0:00:00:00:00:11
>>>>>>
>>>>>> +ovs-vsctl add-port br-int vif11 -- \
>>>>>> +    set Interface vif11 external-ids:iface-id=lp11 \
>>>>>> +                          options:tx_pcap=hv1/vif11-tx.pcap \
>>>>>> +                          options:rxq_pcap=hv1/vif11-rx.pcap \
>>>>>> +                          ofport-request=11
>>>>>> +
>>>>>>    OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up lp11` = xup])
>>> Hi Anton,
>>>
>>> Thanks for the patch.
>>>
>>> I don't think this can be the reason for the failure.
>>>
>>> A user can create an OVS interface first (with external_ids:iface-id
>>> set) or
>>> can create the logical port first. Both are valid use cases.
>>> ovn-controller
>>> will bind the logical port (and maps the ovs interface to the logical
>>> port)
>>> and program the flows when an ovs interface has
>>> external_ids:iface-id configured and the corresponding logical port
>>> present.
>>>
>>> The test traffic can not be processed until the logical port is mapped to
>>> the ovs interface and flows programmed.
>>>
>>> I think the issue is somewhere else.
>> Statement of the fact - the as is test rarely (like once in a 50+
>> times), flakes out on my machine. It is not easy to trigger and I can
>> trigger it only on the fastest test system I have at present and only
>> once in a blue moon.
>>
>> After reordering the statements as per the suggested patch, I ran this
>> test in a loop for 3 hours - no flakes.
>>
>> A.
>>
> Hi Anton,
>
> It might be related to the fact that the RARP packet is sent out before
> the patch port to br-phys is up. Could you please try with the following
> patch to confirm if that's the case?
>
> Thanks,
> Dumitru
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f83d3f5..07689e3 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -17439,6 +17439,9 @@ ovs-vsctl add-port br-int vif11 -- \
>   lsp_name=lp11
>
>   ovn-nbctl lsp-add ls1 lp11
> +OVS_WAIT_UNTIL([test $(as hv1 ovs-vsctl --bare --columns _uuid list \
> +    interface patch-br-int-to-ln1 | wc -l)] = 1)
> +
>   ovn-nbctl lsp-set-addresses lp11 "f0:00:00:00:00:11"
>   ovn-nbctl lsp-set-port-security lp11 f0:00:00:00:00:11

This works.

I ran it in a loop for ~ 1h, no flakes.

A.


>
>>> Thanks
>>> Numan
>>>
>>>
>>>
>>>>>>    ovn-nbctl --wait=sb sync
>>>>>>
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev at openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
>
-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/



More information about the dev mailing list