diff mbox series

[ovs-dev] Fix race condition in test 121

Message ID 20200415091025.1134-1-anton.ivanov@cambridgegreys.com
State Not Applicable
Headers show
Series [ovs-dev] Fix race condition in test 121 | expand

Commit Message

Anton Ivanov April 15, 2020, 9:10 a.m. UTC 
From: Anton Ivanov <anton.ivanov@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@cambridgegreys.com>
---
 tests/ovn.at | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

0-day Robot April 15, 2020, 9:56 a.m. UTC | #1 
Bleep bloop.  Greetings Anton Ivanov, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
error: sha1 information is lacking or useless (tests/ovn.at).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 Fix race condition in test 121
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Ilya Maximets April 15, 2020, 11:22 a.m. UTC | #2 
On 4/15/20 11:10 AM, anton.ivanov@cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov@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@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.

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])
>  
>  ovn-nbctl --wait=sb sync
>
Ilya Maximets April 15, 2020, 11:35 a.m. UTC | #3 
On 4/15/20 1:22 PM, Ilya Maximets wrote:
> On 4/15/20 11:10 AM, anton.ivanov@cambridgegreys.com wrote:
>> From: Anton Ivanov <anton.ivanov@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@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])
>>  
>>  ovn-nbctl --wait=sb sync
>>
>
Numan Siddique April 15, 2020, 3:28 p.m. UTC | #4 
On Wed, Apr 15, 2020 at 5:05 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 4/15/20 1:22 PM, Ilya Maximets wrote:
> > On 4/15/20 11:10 AM, anton.ivanov@cambridgegreys.com wrote:
> >> From: Anton Ivanov <anton.ivanov@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@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.

Thanks
Numan



> >>
> >>  ovn-nbctl --wait=sb sync
> >>
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Anton Ivanov April 16, 2020, 7:21 a.m. UTC | #5 
On 15/04/2020 16:28, Numan Siddique wrote:
> On Wed, Apr 15, 2020 at 5:05 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>> On 4/15/20 1:22 PM, Ilya Maximets wrote:
>>> On 4/15/20 11:10 AM, anton.ivanov@cambridgegreys.com wrote:
>>>> From: Anton Ivanov <anton.ivanov@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@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.

>
> Thanks
> Numan
>
>
>
>>>>   ovn-nbctl --wait=sb sync
>>>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
Dumitru Ceara April 16, 2020, 10:05 a.m. UTC | #6 
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@ovn.org> wrote:
>>> On 4/15/20 1:22 PM, Ilya Maximets wrote:
>>>> On 4/15/20 11:10 AM, anton.ivanov@cambridgegreys.com wrote:
>>>>> From: Anton Ivanov <anton.ivanov@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@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@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
Anton Ivanov April 16, 2020, 1:55 p.m. UTC | #7 
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@ovn.org> wrote:
>>>> On 4/15/20 1:22 PM, Ilya Maximets wrote:
>>>>> On 4/15/20 11:10 AM, anton.ivanov@cambridgegreys.com wrote:
>>>>>> From: Anton Ivanov <anton.ivanov@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@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@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>
>
diff mbox series

Patch

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
 
 # 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])
 
 ovn-nbctl --wait=sb sync