| Message ID | 20200715175615.481433-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-controller: Fix the missing flows with monitor-all set to True | expand |
On 7/15/20 7:56 PM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > When the config ovn-monitor-all is set to True, then ovn-controller is not > programming the flows completely when it binds the logical port of > a logical switch until a full recompute is triggered. > > This issue was introduced by the commit in Fixes - which added incremental procesing > for runtime data changes. > > This patch fixes this issue. > > Fixes: 3d096f833c4a("ovn-controller: Handle runtime data changes in flow output engine") > > Reported-by: Dumitru Ceara <dceara@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/binding.c | 17 ++++++-- > tests/ovn.at | 101 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 115 insertions(+), 3 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index b2c388146d..8f6d68006e 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -103,9 +103,20 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > ld->localnet_port = NULL; > ld->has_local_l3gateway = has_local_l3gateway; > > - if (tracked_datapaths && > - !tracked_binding_datapath_find(tracked_datapaths, datapath)) { > - tracked_binding_datapath_create(datapath, true, tracked_datapaths); > + if (tracked_datapaths) { > + struct tracked_binding_datapath *tdp = > + tracked_binding_datapath_find(tracked_datapaths, datapath); > + if (!tdp) { > + tracked_binding_datapath_create(datapath, true, tracked_datapaths); > + } else { > + /* Its possible that there is already an entry in tracked datapaths Nit: s/Its/It's. > + * for this 'datapath'. tracked_binding_datapath_lport_add() may > + * have created it. Since the 'datapath' is added to the > + * local datapaths, set the tdp->is_new to true so that the flows Nit s/set the/set Otherwise: Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru > + * for this datapath are programmed properly. > + * */ > + tdp->is_new = true; > + } > } > > if (depth >= 100) { > diff --git a/tests/ovn.at b/tests/ovn.at > index 24d93bc245..ba1a534e92 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -20601,3 +20601,104 @@ OVS_WAIT_UNTIL([ > > OVN_CLEANUP([hv1], [hv2]) > AT_CLEANUP > + > +AT_SETUP([ovn -- controller I-P handling with monitoring disabled]) > +AT_KEYWORDS([lb]) > +ovn_start > + > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > + > +sim_add hv2 > +as hv2 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.2 > + > +ovn-nbctl ls-add sw0 > + > +ovn-nbctl lsp-add sw0 sw0-p1 > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" > + > +ovn-nbctl lsp-add sw0 sw0-p2 > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" > + > +as hv1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > + > +# 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" > +AT_CHECK([test $hv1_offlows -gt 0]) > + > +as hv2 > +ovs-vsctl -- add-port br-int hv2-vif1 -- \ > + set interface hv2-vif1 external-ids:iface-id=sw0-p2 \ > + options:tx_pcap=hv2/vif1-tx.pcap \ > + options:rxq_pcap=hv2/vif1-rx.pcap \ > + ofport-request=1 > + > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > + > +hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) > +echo "hv2 flows : $hv2_offlows" > +AT_CHECK([test $hv2_offlows -gt 0]) > + > +ovn-nbctl ls-del sw0 > +as hv1 ovs-vsctl del-port hv1-vif1 > +as hv2 ovs-vsctl del-port hv2-vif1 > + > +as hv1 ovs-vsctl set open . external_ids:ovn-monitor-all=true > +as hv2 ovs-vsctl set open . external_ids:ovn-monitor-all=true > + > +ovn-nbctl ls-add sw0 > + > +ovn-nbctl lsp-add sw0 sw0-p1 > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" > + > +ovn-nbctl lsp-add sw0 sw0-p2 > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" > + > +as hv1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > + > +# 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" > +AT_CHECK([test "$hv1_offlows" = "$hv1_offlows_mon"]) > + > +as hv2 > +ovs-vsctl -- add-port br-int hv2-vif1 -- \ > + set interface hv2-vif1 external-ids:iface-id=sw0-p2 \ > + options:tx_pcap=hv2/vif1-tx.pcap \ > + options:rxq_pcap=hv2/vif1-rx.pcap \ > + ofport-request=1 > + > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > + > +hv2_offlows_mon=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) > +echo "hv2 flows after monitor-all=true : $hv2_offlows" > +AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"]) > + > +OVN_CLEANUP([hv1], [hv2]) > +AT_CLEANUP >
On Thu, Jul 16, 2020 at 1:34 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 7/15/20 7:56 PM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > When the config ovn-monitor-all is set to True, then ovn-controller is not > > programming the flows completely when it binds the logical port of > > a logical switch until a full recompute is triggered. > > > > This issue was introduced by the commit in Fixes - which added incremental procesing > > for runtime data changes. > > > > This patch fixes this issue. > > > > Fixes: 3d096f833c4a("ovn-controller: Handle runtime data changes in flow output engine") > > > > Reported-by: Dumitru Ceara <dceara@redhat.com> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > controller/binding.c | 17 ++++++-- > > tests/ovn.at | 101 +++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 115 insertions(+), 3 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index b2c388146d..8f6d68006e 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -103,9 +103,20 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > > ld->localnet_port = NULL; > > ld->has_local_l3gateway = has_local_l3gateway; > > > > - if (tracked_datapaths && > > - !tracked_binding_datapath_find(tracked_datapaths, datapath)) { > > - tracked_binding_datapath_create(datapath, true, tracked_datapaths); > > + if (tracked_datapaths) { > > + struct tracked_binding_datapath *tdp = > > + tracked_binding_datapath_find(tracked_datapaths, datapath); > > + if (!tdp) { > > + tracked_binding_datapath_create(datapath, true, tracked_datapaths); > > + } else { > > + /* Its possible that there is already an entry in tracked datapaths > > Nit: s/Its/It's. > > > + * for this 'datapath'. tracked_binding_datapath_lport_add() may > > + * have created it. Since the 'datapath' is added to the > > + * local datapaths, set the tdp->is_new to true so that the flows > > Nit s/set the/set > > Otherwise: > Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks Dumitru. I corrected the typo and applied to master and branch-20.06 Thanks Numan > > Thanks, > Dumitru > > > + * for this datapath are programmed properly. > > + * */ > > + tdp->is_new = true; > > + } > > } > > > > if (depth >= 100) { > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 24d93bc245..ba1a534e92 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -20601,3 +20601,104 @@ OVS_WAIT_UNTIL([ > > > > OVN_CLEANUP([hv1], [hv2]) > > AT_CLEANUP > > + > > +AT_SETUP([ovn -- controller I-P handling with monitoring disabled]) > > +AT_KEYWORDS([lb]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > + > > + > > +sim_add hv2 > > +as hv2 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.2 > > + > > +ovn-nbctl ls-add sw0 > > + > > +ovn-nbctl lsp-add sw0 sw0-p1 > > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" > > + > > +ovn-nbctl lsp-add sw0 sw0-p2 > > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" > > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" > > + > > +as hv1 > > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > + > > +# 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" > > +AT_CHECK([test $hv1_offlows -gt 0]) > > + > > +as hv2 > > +ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > + set interface hv2-vif1 external-ids:iface-id=sw0-p2 \ > > + options:tx_pcap=hv2/vif1-tx.pcap \ > > + options:rxq_pcap=hv2/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > > + > > +hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) > > +echo "hv2 flows : $hv2_offlows" > > +AT_CHECK([test $hv2_offlows -gt 0]) > > + > > +ovn-nbctl ls-del sw0 > > +as hv1 ovs-vsctl del-port hv1-vif1 > > +as hv2 ovs-vsctl del-port hv2-vif1 > > + > > +as hv1 ovs-vsctl set open . external_ids:ovn-monitor-all=true > > +as hv2 ovs-vsctl set open . external_ids:ovn-monitor-all=true > > + > > +ovn-nbctl ls-add sw0 > > + > > +ovn-nbctl lsp-add sw0 sw0-p1 > > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" > > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" > > + > > +ovn-nbctl lsp-add sw0 sw0-p2 > > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" > > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" > > + > > +as hv1 > > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > + > > +# 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" > > +AT_CHECK([test "$hv1_offlows" = "$hv1_offlows_mon"]) > > + > > +as hv2 > > +ovs-vsctl -- add-port br-int hv2-vif1 -- \ > > + set interface hv2-vif1 external-ids:iface-id=sw0-p2 \ > > + options:tx_pcap=hv2/vif1-tx.pcap \ > > + options:rxq_pcap=hv2/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > > + > > +hv2_offlows_mon=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) > > +echo "hv2 flows after monitor-all=true : $hv2_offlows" > > +AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"]) > > + > > +OVN_CLEANUP([hv1], [hv2]) > > +AT_CLEANUP > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/binding.c b/controller/binding.c index b2c388146d..8f6d68006e 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -103,9 +103,20 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, ld->localnet_port = NULL; ld->has_local_l3gateway = has_local_l3gateway; - if (tracked_datapaths && - !tracked_binding_datapath_find(tracked_datapaths, datapath)) { - tracked_binding_datapath_create(datapath, true, tracked_datapaths); + if (tracked_datapaths) { + struct tracked_binding_datapath *tdp = + tracked_binding_datapath_find(tracked_datapaths, datapath); + if (!tdp) { + tracked_binding_datapath_create(datapath, true, tracked_datapaths); + } else { + /* Its possible that there is already an entry in tracked datapaths + * for this 'datapath'. tracked_binding_datapath_lport_add() may + * have created it. Since the 'datapath' is added to the + * local datapaths, set the tdp->is_new to true so that the flows + * for this datapath are programmed properly. + * */ + tdp->is_new = true; + } } if (depth >= 100) { diff --git a/tests/ovn.at b/tests/ovn.at index 24d93bc245..ba1a534e92 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -20601,3 +20601,104 @@ OVS_WAIT_UNTIL([ OVN_CLEANUP([hv1], [hv2]) AT_CLEANUP + +AT_SETUP([ovn -- controller I-P handling with monitoring disabled]) +AT_KEYWORDS([lb]) +ovn_start + +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + + +sim_add hv2 +as hv2 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.2 + +ovn-nbctl ls-add sw0 + +ovn-nbctl lsp-add sw0 sw0-p1 +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" + +ovn-nbctl lsp-add sw0 sw0-p2 +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" + +as hv1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) + +# 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" +AT_CHECK([test $hv1_offlows -gt 0]) + +as hv2 +ovs-vsctl -- add-port br-int hv2-vif1 -- \ + set interface hv2-vif1 external-ids:iface-id=sw0-p2 \ + options:tx_pcap=hv2/vif1-tx.pcap \ + options:rxq_pcap=hv2/vif1-rx.pcap \ + ofport-request=1 + +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) + +hv2_offlows=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) +echo "hv2 flows : $hv2_offlows" +AT_CHECK([test $hv2_offlows -gt 0]) + +ovn-nbctl ls-del sw0 +as hv1 ovs-vsctl del-port hv1-vif1 +as hv2 ovs-vsctl del-port hv2-vif1 + +as hv1 ovs-vsctl set open . external_ids:ovn-monitor-all=true +as hv2 ovs-vsctl set open . external_ids:ovn-monitor-all=true + +ovn-nbctl ls-add sw0 + +ovn-nbctl lsp-add sw0 sw0-p1 +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3" +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3" + +ovn-nbctl lsp-add sw0 sw0-p2 +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4" +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4" + +as hv1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) + +# 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" +AT_CHECK([test "$hv1_offlows" = "$hv1_offlows_mon"]) + +as hv2 +ovs-vsctl -- add-port br-int hv2-vif1 -- \ + set interface hv2-vif1 external-ids:iface-id=sw0-p2 \ + options:tx_pcap=hv2/vif1-tx.pcap \ + options:rxq_pcap=hv2/vif1-rx.pcap \ + ofport-request=1 + +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) + +hv2_offlows_mon=$(as hv2 ovs-ofctl dump-flows br-int | wc -l) +echo "hv2 flows after monitor-all=true : $hv2_offlows" +AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"]) + +OVN_CLEANUP([hv1], [hv2]) +AT_CLEANUP