| Message ID | 20201014175828.GH86739@greg-smith |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] tests: Use ovn-nbctl --wait=hv sync for I-P test. | expand |
Gregory Smith wrote: > This patch makes the "controller I-P handling with monitoring disabled" > testcase more determinstic, by waiting for flows to be synchronized to > the hypervisor before counting them. > > Signed-off-by: Gregory Smith <gasmith@nutanix.com> > --- It turns out this patch is insufficient. It seems that ovn-controller ACKs the nb_cfg sequence number before waiting for an OFPT_BARRIER_REPLY from ovs-vswitchd. Is there a better way to synchronize? Regards, Greg > tests/ovn.at | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/tests/ovn.at b/tests/ovn.at > index 488fd119b..ba83dc1bf 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -21083,6 +21083,7 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ > ofport-request=1 > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > +ovn-nbctl --wait=hv sync > > # Get the number of OF flows in hv1 and hv2 > hv1_offlows=$(as hv1 ovs-ofctl dump-flows br-int | wc -l) > @@ -21097,6 +21098,7 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > ofport-request=1 > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > +ovn-nbctl --wait=hv sync > > hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) > echo "hv2 flows : $hv2_offlows" > @@ -21135,10 +21137,11 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ > ofport-request=1 > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > +ovn-nbctl --wait=hv sync > > # 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 > @@ -21149,9 +21152,10 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > ofport-request=1 > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > +ovn-nbctl --wait=hv sync > > 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.28.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=0HZBgQW39TLbzbCyTLQNu1VE33o6KzimT1AMqSUuNEk&m=J-m0jQ4DaLzjS_gzKLFTe8sCyojKk5qTRxVcgc5_2Yw&s=R5k-jJh8wMVT0j9_M8ZuxGrJXgfmM4fzahFmboB0BYw&e=
On Thu, Oct 15, 2020 at 4:45 AM Gregory Smith <gasmith@nutanix.com> wrote: > > Gregory Smith wrote: > > This patch makes the "controller I-P handling with monitoring disabled" > > testcase more determinstic, by waiting for flows to be synchronized to > > the hypervisor before counting them. > > > > Signed-off-by: Gregory Smith <gasmith@nutanix.com> > > --- > > It turns out this patch is insufficient. It seems that ovn-controller > ACKs the nb_cfg sequence number before waiting for an OFPT_BARRIER_REPLY > from ovs-vswitchd. Is there a better way to synchronize? What is the issue you're seeing ? Is the test case failing in your setup ? Maybe you can replace the below AT_CHECK with OVS_WAIT_UNTIL(). ***** hv1_offlows=$(as hv1 ovs-ofctl dump-flows br-int | wc -l) echo "hv1 flows : $hv1_offlows" AT_CHECK([test $hv1_offlows -gt 0]) **** Thanks Numan > > Regards, > Greg > > > tests/ovn.at | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 488fd119b..ba83dc1bf 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -21083,6 +21083,7 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > ofport-request=1 > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > +ovn-nbctl --wait=hv sync > > > > # Get the number of OF flows in hv1 and hv2 > > hv1_offlows=$(as hv1 ovs-ofctl dump-flows br-int | wc -l) > > @@ -21097,6 +21098,7 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > ofport-request=1 > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > > +ovn-nbctl --wait=hv sync > > > > hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) > > echo "hv2 flows : $hv2_offlows" > > @@ -21135,10 +21137,11 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > ofport-request=1 > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > +ovn-nbctl --wait=hv sync > > > > # 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 > > @@ -21149,9 +21152,10 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > ofport-request=1 > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > > +ovn-nbctl --wait=hv sync > > > > 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.28.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=0HZBgQW39TLbzbCyTLQNu1VE33o6KzimT1AMqSUuNEk&m=J-m0jQ4DaLzjS_gzKLFTe8sCyojKk5qTRxVcgc5_2Yw&s=R5k-jJh8wMVT0j9_M8ZuxGrJXgfmM4fzahFmboB0BYw&e= > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Oct 14, 2020 at 4:15 PM Gregory Smith <gasmith@nutanix.com> wrote: > > Gregory Smith wrote: > > This patch makes the "controller I-P handling with monitoring disabled" > > testcase more determinstic, by waiting for flows to be synchronized to > > the hypervisor before counting them. > > > > Signed-off-by: Gregory Smith <gasmith@nutanix.com> > > --- > > It turns out this patch is insufficient. It seems that ovn-controller > ACKs the nb_cfg sequence number before waiting for an OFPT_BARRIER_REPLY > from ovs-vswitchd. Is there a better way to synchronize? > It sounds like a bug. I think the right way to fix this is to make sure it waits for the OFPT_BARRIER_REPLY before acking the nb_cfg. I didn't check the details though. Thanks, Han > Regards, > Greg > > > tests/ovn.at | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 488fd119b..ba83dc1bf 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -21083,6 +21083,7 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > ofport-request=1 > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > +ovn-nbctl --wait=hv sync > > > > # Get the number of OF flows in hv1 and hv2 > > hv1_offlows=$(as hv1 ovs-ofctl dump-flows br-int | wc -l) > > @@ -21097,6 +21098,7 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > ofport-request=1 > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > > +ovn-nbctl --wait=hv sync > > > > hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) > > echo "hv2 flows : $hv2_offlows" > > @@ -21135,10 +21137,11 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > ofport-request=1 > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > +ovn-nbctl --wait=hv sync > > > > # 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 > > @@ -21149,9 +21152,10 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > ofport-request=1 > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > > +ovn-nbctl --wait=hv sync > > > > 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.28.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=0HZBgQW39TLbzbCyTLQNu1VE33o6KzimT1AMqSUuNEk&m=J-m0jQ4DaLzjS_gzKLFTe8sCyojKk5qTRxVcgc5_2Yw&s=R5k-jJh8wMVT0j9_M8ZuxGrJXgfmM4fzahFmboB0BYw&e= > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Numan Siddique wrote: > On Thu, Oct 15, 2020 at 4:45 AM Gregory Smith <gasmith@nutanix.com> wrote: > > > > Gregory Smith wrote: > > > This patch makes the "controller I-P handling with monitoring disabled" > > > testcase more determinstic, by waiting for flows to be synchronized to > > > the hypervisor before counting them. > > > > > > Signed-off-by: Gregory Smith <gasmith@nutanix.com> > > > --- > > > > It turns out this patch is insufficient. It seems that ovn-controller > > ACKs the nb_cfg sequence number before waiting for an OFPT_BARRIER_REPLY > > from ovs-vswitchd. Is there a better way to synchronize? > > What is the issue you're seeing ? Is the test case failing in your setup ? Yes, the test case fails intermittently in my environment, even with my patch. I turned on debug logging in ovs-vswitchd, and I noticed the following interleaving: 2020-10-21T15:58:46.270Z|00089|vconn|DBG|unix#2: received: OFPT_FLOW_MOD (OF1.5) (xid=0x1e): ADD table:33 ... ... 2020-10-21T15:58:46.282Z|00195|vconn|DBG|unix#4: received: NXST_FLOW request (xid=0x4): 2020-10-21T15:58:46.284Z|00196|vconn|DBG|unix#4: sent (Success): NXST_FLOW reply (xid=0x4): ... 2020-10-21T15:58:46.285Z|00202|vconn|DBG|unix#2: received: OFPT_FLOW_MOD (OF1.5) (xid=0x87): ADD table:50 ... 2020-10-21T15:58:46.285Z|00203|vconn|DBG|unix#2: received: OFPT_BARRIER_REQUEST (OF1.5) (xid=0x88): 2020-10-21T15:58:46.285Z|00204|vconn|DBG|unix#2: sent (Success): OFPT_BARRIER_REPLY (OF1.5) (xid=0x88): And indeed, from ovn-controller's perspective: 2020-10-21T15:58:46.270Z|01009|vconn|DBG|unix:/src/ovn/tests/testsuite.dir/145/hv2/br-int.mgmt: sent (Success): OFPT_FLOW_MOD (OF1.5) (xid=0x1e): ADD table:33 ... ... 2020-10-21T15:58:46.273Z|01115|vconn|DBG|unix:/src/ovn/tests/testsuite.dir/145/hv2/br-int.mgmt: sent (Success): OFPT_BARRIER_REQUEST (OF1.5) (xid=0x88): ... 2020-10-21T15:58:46.274Z|01170|jsonrpc|DBG|unix:/src/ovn/tests/testsuite.dir/145/ovn-sb/ovn-sb.sock: send request, method="transact", params=["OVN_Southbound",{"where":[["_uuid","==",["uuid","5a529d18-e8a4-418c-a906-b37e6c6ed6b9"]]],"row":{"nb_cfg":2,"nb_cfg_timestamp":1603295926273},"op":"update","table":"Chassis_Pr ivate"},{"where":[["_uuid","==",["uuid","a228fa82-67e0-4c94-965f-7b23844e87b4"]]],"row":{"chassis_name":"hv2"},"op":"update","table":"Encap"},{"where":[["_uuid","==",["uuid","ad6484bf-0d8d-4830-8c42-37b99c10b6fe"]]],"row":{"chassis_name":"hv2"},"op":"update","table":"Encap"}], id=18 ... 2020-10-21T15:58:46.285Z|01253|vconn|DBG|unix:/src/ovn/tests/testsuite.dir/145/hv2/br-int.mgmt: received: OFPT_BARRIER_REPLY (OF1.5) (xid=0x88): > Maybe you can replace the below AT_CHECK with OVS_WAIT_UNTIL(). > > ***** > hv1_offlows=$(as hv1 ovs-ofctl dump-flows br-int | wc -l) > echo "hv1 flows : $hv1_offlows" > AT_CHECK([test $hv1_offlows -gt 0]) > **** Yes, I think that would fix the test, but I wonder whether it would be more appropriate to fix ovn-controller's behavior: it doesn't seem to match the intended semantics for ovn-nbctl --wait=hv sync. Thanks, Greg > > Thanks > Numan > > > > > > > > Regards, > > Greg > > > > > tests/ovn.at | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > > index 488fd119b..ba83dc1bf 100644 > > > --- a/tests/ovn.at > > > +++ b/tests/ovn.at > > > @@ -21083,6 +21083,7 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > > ofport-request=1 > > > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > > +ovn-nbctl --wait=hv sync > > > > > > # Get the number of OF flows in hv1 and hv2 > > > hv1_offlows=$(as hv1 ovs-ofctl dump-flows br-int | wc -l) > > > @@ -21097,6 +21098,7 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > > ofport-request=1 > > > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > > > +ovn-nbctl --wait=hv sync > > > > > > hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) > > > echo "hv2 flows : $hv2_offlows" > > > @@ -21135,10 +21137,11 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > > ofport-request=1 > > > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > > +ovn-nbctl --wait=hv sync > > > > > > # 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 > > > @@ -21149,9 +21152,10 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > > ofport-request=1 > > > > > > OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > > > +ovn-nbctl --wait=hv sync > > > > > > 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.28.0 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=s883GpUCOChKOHiocYtGcg&r=0HZBgQW39TLbzbCyTLQNu1VE33o6KzimT1AMqSUuNEk&m=J-m0jQ4DaLzjS_gzKLFTe8sCyojKk5qTRxVcgc5_2Yw&s=R5k-jJh8wMVT0j9_M8ZuxGrJXgfmM4fzahFmboB0BYw&e= > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=0HZBgQW39TLbzbCyTLQNu1VE33o6KzimT1AMqSUuNEk&m=ncNsLdGCMlL6N-8HjyxbEgARjlwrZOf53UU2v18t2s4&s=Dzfzw8kFY9WygIKg8OU89ayXx69bqgTE-lJCysEn6mU&e= > >
diff --git a/tests/ovn.at b/tests/ovn.at index 488fd119b..ba83dc1bf 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -21083,6 +21083,7 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ ofport-request=1 OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) +ovn-nbctl --wait=hv sync # Get the number of OF flows in hv1 and hv2 hv1_offlows=$(as hv1 ovs-ofctl dump-flows br-int | wc -l) @@ -21097,6 +21098,7 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ ofport-request=1 OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) +ovn-nbctl --wait=hv sync hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) echo "hv2 flows : $hv2_offlows" @@ -21135,10 +21137,11 @@ ovs-vsctl -- add-port br-int hv1-vif1 -- \ ofport-request=1 OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) +ovn-nbctl --wait=hv sync # 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 @@ -21149,9 +21152,10 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \ ofport-request=1 OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) +ovn-nbctl --wait=hv sync 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])
This patch makes the "controller I-P handling with monitoring disabled" testcase more determinstic, by waiting for flows to be synchronized to the hypervisor before counting them. Signed-off-by: Gregory Smith <gasmith@nutanix.com> --- tests/ovn.at | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)