[ovs-dev] [PATCH ovn] ovn.at: Fix flaky test "controller I-P handling with monitoring disabled".

[Han Zhou]

On Fri, Oct 1, 2021 at 7:53 AM Mark Gray <mark.d.gray at redhat.com> wrote:
>
> 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?
>
We discussed the problem of delayed installation of the ARP responder
flows. It seems it is causing more trouble than benefit (if any at all). So
I proposed a change to make the default behavior not delaying installation
of those flows.
PTAL:
https://patchwork.ozlabs.org/project/ovn/patch/20211002071139.4068061-1-hzhou@ovn.org/

Thanks,
Han

> >
> > Thanks
> > Xavier
> >
> > On Thu, Sep 16, 2021 at 1:23 PM Xavier Simonart <xsimonar at 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.gray at 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 <xsimonar at 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev