On 27/09/2021 12:13, Xavier Simonart wrote:
> Hi Mark
Sorry for the late reply. If you cc me, I will usually reply earlier.
>
> After further investigation, checking that port is up does not fix the
> issue, as port up is reported too early by the ovn controller (before all
> flows are installed), when using conditional monitoring.
>
> I proposed different potential solutions on the mailing list and consensus
> seems to be to leave OVN as is (i.e. port is still reported up too early)
> and only fix the test cases. Fully fixing the port up issue is perceived as
> too risky for the benefit.
>
> For this test case, there is one additional issue: when port is reported up
> (both conditional monitoring and monitor-all), ovn-controller adds some
> more flows related for ARP handling. Hence, counting the number of flows
> when port is up might/might not take into account those flows and the test
> might fail from time to time...
> As a consequence, I propose to modify the patch in the following way: still
> use an OVS_WAIT_UNTIL, but check whether ARP responder has been installed
> by counting the number of flows related to it.
What would happen if a developer changed how the ARP responder flows
were generated (e.g. the number of them, or whether or not they were
installed at all)? Would this cause this test to fail consistently so it
could be caught?
>
> Thanks
> Xavier
>
> On Thu, Sep 16, 2021 at 1:23 PM Xavier Simonart <xsimo...@redhat.com> wrote:
>
>> Hi Mark
>>
>> Thanks for looking into this.
>> Yes and no... I do not think that the patch itself will help, but it
>> pointed me to the existence of Logical_Switch_Port.up, which in this case
>> should do the trick.
>>
>> I will update the patch: instead of looking at the flows, I will use
>> something like
>> *wait_column "true" nb:Logical_Switch_Port up name=sw0-p1*
>>
>> Thanks
>> Xavier
>>
>> On Thu, Sep 16, 2021 at 11:40 AM Mark Gray <mark.d.g...@redhat.com> wrote:
>>
>>> On 15/09/2021 09:52, Xavier Simonart wrote:
>>>> Test was waiting for port to be up in SBDB before checking number of
>>> flows
>>>
>>> Would this feature prevent this?
>>>
>>> 5c3371922994 ("if-status: Add OVS interface status management module.")
>>>
>>>> in OVS. However, there is no guarantee that all flows are installed
>>>> in OVS when port is up. Test was randomly failing as some flows were
>>>> installed, but not all.
>>>> To fix this, we wait until the last flow (with actions=output) is
>>>> installed.
>>>> Also fixed small typo in logging (for the same test).
>>>>
>>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004390
>>>> Fixes: f8a81693b0 ("ovn-controller: Fix the missing flows with
>>> monitor-all set to True")
>>>> Signed-off-by: Xavier Simonart <xsimo...@redhat.com>
>>>> ---
>>>> tests/ovn.at | 28 ++++++++++++++++++++++++++--
>>>> 1 file changed, 26 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>>> index 30625ec37..18aeacd02 100644
>>>> --- a/tests/ovn.at
>>>> +++ b/tests/ovn.at
>>>> @@ -23448,6 +23448,12 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \
>>>>
>>>> wait_for_ports_up sw0-p1
>>>>
>>>> +# Wait for last flow to be installed
>>>> +OVS_WAIT_UNTIL([
>>>> + test $(as hv1 ovs-ofctl dump-flows br-int | \
>>>> + grep "actions=output" -c) -eq 1
>>>> +])
>>>> +
>>>> # Get the number of OF flows in hv1 and hv2
>>>> hv1_offlows=$(as hv1 ovs-ofctl dump-flows br-int | wc -l)
>>>> echo "hv1 flows : $hv1_offlows"
>>>> @@ -23462,6 +23468,12 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \
>>>>
>>>> wait_for_ports_up sw0-p2
>>>>
>>>> +# Wait for last flow to be installed
>>>> +OVS_WAIT_UNTIL([
>>>> + test $(as hv2 ovs-ofctl dump-flows br-int | \
>>>> + grep "actions=output" -c) -eq 1
>>>> +])
>>>> +
>>>> hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l)
>>>> echo "hv2 flows : $hv2_offlows"
>>>> AT_CHECK([test $hv2_offlows -gt 0])
>>>> @@ -23500,9 +23512,15 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \
>>>>
>>>> wait_for_ports_up sw0-p1
>>>>
>>>> +# Wait for last flow to be installed
>>>> +OVS_WAIT_UNTIL([
>>>> + test $(as hv1 ovs-ofctl dump-flows br-int | \
>>>> + grep "actions=output" -c) -eq 1
>>>> +])
>>>> +
>>>> # Get the number of OF flows in hv1 and hv2
>>>> hv1_offlows_mon=$(as hv1 ovs-ofctl dump-flows br-int | wc -l)
>>>> -echo "hv1 flows after monitor-all=true : $hv1_offlows"
>>>> +echo "hv1 flows after monitor-all=true : $hv1_offlows_mon"
>>>> AT_CHECK([test "$hv1_offlows" = "$hv1_offlows_mon"])
>>>>
>>>> as hv2
>>>> @@ -23514,8 +23532,14 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \
>>>>
>>>> wait_for_ports_up sw0-p2
>>>>
>>>> +# Wait for last flow to be installed
>>>> +OVS_WAIT_UNTIL([
>>>> + test $(as hv2 ovs-ofctl dump-flows br-int | \
>>>> + grep "actions=output" -c) -eq 1
>>>> +])
>>>> +
>>>> hv2_offlows_mon=$(as hv2 ovs-ofctl dump-flows br-int | wc -l)
>>>> -echo "hv2 flows after monitor-all=true : $hv2_offlows"
>>>> +echo "hv2 flows after monitor-all=true : $hv2_offlows_mon"
>>>> AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"])
>>>>
>>>> OVN_CLEANUP([hv1], [hv2])
>>>>
>>>
>>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
