| Message ID | 20210915085251.2609426-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ovn.at: Fix flaky test "controller I-P handling with monitoring disabled". | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
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@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]) >
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@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@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]) > > > >
Hi Mark 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. Thanks Xavier On Thu, Sep 16, 2021 at 1:23 PM Xavier Simonart <xsimonar@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@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@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]) >> > >> >>
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 <xsimonar@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@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@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]) >>>> >>> >>> >
On Fri, Oct 1, 2021 at 7:53 AM Mark Gray <mark.d.gray@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@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@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Sep 15, 2021 at 4:53 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > Test was waiting for port to be up in SBDB before checking number of flows > 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@redhat.com> Thanks for fixing this issue. I applied this patch to the main branch. I did one change. Please see below Thanks Numan > --- > 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 With the commit 2cc42bdef("northd: Change the default value of ignore_lsp_down to true.") merged, this arp flow may not be the last flow. So I replaced "last flow" with "arp flow". Numan > +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]) > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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])
Test was waiting for port to be up in SBDB before checking number of flows 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@redhat.com> --- tests/ovn.at | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)