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

[Dumitru Ceara]

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

>>
>> Thanks
>> Numan
>>
>>
>>
>>>>>   ovn-nbctl --wait=sb sync
>>>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>