Message ID | 20200727165702.2769313-1-numans@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,ovn] ovn-controller: Clear flows not associated with db rows in physical flow change handler. | expand |
Acked-by: Mark Michelson <mmichels@redhat.com> There's one small comment in your test. You can just fix it when you merge, though. On 7/27/20 12:57 PM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > The commit in the Fixes tag while handling changes for OVS interface changes and ct zone > changes, called physical_run() without clearing the flow table. This works ok for existing > flows in the flow table which are associated with ovsdb rows. physical_run() ensures that > such flows are deleted by calling ofctrl_delete_flows() by adding them back again. > > But flows not associated with ovsdb rows (whose OF flow cookie is set to 0) are not cleared > at all until a full recompute is triggered. Particularly flows in table 33 which set > various conntrack zone registers still remain. Suppose a lport is claimed again and if > the ct-zone id for this lport changes, then the old flow for this lport still remains and > this causes the packet to enter the conntrack with a wrong zone id. > > Such flows are stored indexed with the uuid 'hc_uuid' in the flow table and it is easy > to clear them. This patch clears such flows before calling physical_run() to fix this issue. > > A more accurate fix would be to store the logical and physical flows in separate flow tables > and clear all the flows in the physical flow table before calling physical_run(). > This patch is good enough for now until we implement separate flow tables. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1861042 > Fixes: a3005f0dc777("ovn-controller: I-P for ct zone and OVS interface changes in flow output stage.") > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > controller/ovn-controller.c | 1 + > controller/physical.c | 8 +++ > controller/physical.h | 1 + > tests/ovn.at | 105 ++++++++++++++++++++++++++++++++++++ > tests/system-ovn.at | 89 ++++++++++++++++++++++++++++++ > 5 files changed, 204 insertions(+) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index e355007b88..5ca32ac433 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1994,6 +1994,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > > if (pfc_data->recompute_physical_flows) { > /* This indicates that we need to recompute the physical flows. */ > + physical_clear_unassoc_flows_with_db(&fo->flow_table); > physical_run(&p_ctx, &fo->flow_table); > return true; > } > diff --git a/controller/physical.c b/controller/physical.c > index 6d7d8e93bc..535c777307 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -1851,3 +1851,11 @@ get_tunnel_ofport(const char *chassis_name, char *encap_ip, ofp_port_t *ofport) > *ofport = tun->ofport; > return true; > } > + > +void > +physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *flow_table) > +{ > + if (hc_uuid) { > + ofctrl_remove_flows(flow_table, hc_uuid); > + } > +} > diff --git a/controller/physical.h b/controller/physical.h > index 9ca34436a5..feab41df4c 100644 > --- a/controller/physical.h > +++ b/controller/physical.h > @@ -61,6 +61,7 @@ struct physical_ctx { > void physical_register_ovs_idl(struct ovsdb_idl *); > void physical_run(struct physical_ctx *, > struct ovn_desired_flow_table *); > +void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *); > void physical_handle_port_binding_changes(struct physical_ctx *, > struct ovn_desired_flow_table *); > void physical_handle_mc_group_changes(struct physical_ctx *, > diff --git a/tests/ovn.at b/tests/ovn.at > index 7c9e14e2ea..48ab4e0655 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -20806,3 +20806,108 @@ AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"]) > > OVN_CLEANUP([hv1], [hv2]) > AT_CLEANUP > + > + > +# When a lport is released on a chassis, ovn-controller was > +# not clearing some of the flowss in the table 33. > +# Make sure that those flows are cleared properly. > +AT_SETUP([ovn -- Test clear flows in physical tables]) > +AT_KEYWORDS([lb]) Misleading keywords. > +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 > + > +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 > + > +as hv1 > +ovs-vsctl -- add-port br-int hv1-vif2 -- \ > + set interface hv1-vif2 external-ids:iface-id=sw0-p2 \ > + options:tx_pcap=hv1/vif2-tx.pcap \ > + options:rxq_pcap=hv1/vif2-rx.pcap \ > + ofport-request=2 > + > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > + > +sw0_dpkey=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding sw0) > +p1_dpkey=$(ovn-sbctl --bare --columns tunnel_key list port_binding sw0-p1) > +p2_dpkey=$(ovn-sbctl --bare --columns tunnel_key list port_binding sw0-p2) > + > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g') > +AT_CHECK([test ! -z $p1_zoneid]) > + > +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p2 | sed 's/"//g') > +AT_CHECK([test ! -z $p2_zoneid]) > + > +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1]) > + > +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ > +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) -eq 1]) > + > +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw1_dpkey},\ > +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 1]) > + > +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw1_dpkey},\ > +reg15=0x${p2_dpkey} | grep "load:0x${p2_zoneid}->NXM_NX_REG13" | wc -l) -eq 1]) > + > +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xdown]) > + > +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0]) > + > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g') > +AT_CHECK([test -z $p1_zoneid]) > + > +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=sw0-p1 > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > + > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g') > +AT_CHECK([test ! -z $p1_zoneid]) > + > +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1]) > + > +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ > +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) -eq 1]) > + > +ovs-vsctl del-port hv1-vif2 > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xdown]) > + > +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ > +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 0]) > + > +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p2 | sed 's/"//g') > +AT_CHECK([test -z $p2_zoneid]) > + > +ovn-nbctl lsp-del sw0-p1 > + > +OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0]) > + > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g') > +AT_CHECK([test ! -z $p1_zoneid]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index eddc530f97..bca2601267 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -4467,6 +4467,95 @@ NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 20.0.0.4 80], [0]) > # Send the packet to VIP. > NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 30.0.0.10 80], [0]) > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > + > +AT_CLEANUP > + > +# When a lport is released on a chassis, ovn-controller was > +# not clearing some of the flowss in the table 33 leading > +# to packet drops if ct() is hit. > +# Make sure that those flows are cleared properly. > +AT_SETUP([ovn --Test packet drops due to incorrect flows in physical table 33]) > +AT_KEYWORDS([lb]) > + > +ovn_start > + > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-int]) > + > +# Set external-ids in br-int needed for ovn-controller > +ovs-vsctl \ > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true > + > +# Start ovn-controller > +start_daemon ovn-controller > + > +ovn-nbctl ls-add sw0 > +ovn-nbctl lsp-add sw0 sw0-p1-f > +ovn-nbctl lsp-set-addresses sw0-p1-f "10:54:00:00:00:03 10.0.0.3" > + > +ovn-nbctl lsp-add sw0 sw0-p2-f > +ovn-nbctl lsp-set-addresses sw0-p2-f "10:54:00:00:00:04 10.0.0.4" > + > +ovn-nbctl lsp-add sw0 sw0-p3-f > +ovn-nbctl lsp-set-addresses sw0-p3-f "10:54:00:00:00:05 10.0.0.5" > + > +# Add ACL with allow-ralated so that conntrack is hit. > + > +ovn-nbctl acl-add sw0 from-lport 1002 "ip" allow-related > +ovn-nbctl acl-add sw0 to-lport 1002 "ip" allow-related > + > +ADD_NAMESPACES(sw0-p1-f) > +ADD_VETH(sw0-p1-f, sw0-p1-f, br-int, "10.0.0.3/24", "10:54:00:00:00:03", \ > + "10.0.0.1") > + > +ADD_NAMESPACES(sw0-p2-f) > +ADD_VETH(sw0-p2-f, sw0-p2-f, br-int, "10.0.0.4/24", "10:54:00:00:00:04", \ > + "10.0.0.1") > + > +ADD_NAMESPACES(sw0-p3-f) > +ADD_VETH(sw0-p3-f, sw0-p3-f, br-int, "10.0.0.5/24", "10:54:00:00:00:05", \ > + "10.0.0.1") > + > +# Send ping from sw0-p1-f to sw0-p3-f > +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | FORMAT_PING], \ > +[0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +ovs-vsctl remove interface ovs-sw0-p2-f external_ids iface-id > +ovs-vsctl remove interface ovs-sw0-p3-f external_ids iface-id > + > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2-f) = xdown]) > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xdown]) > + > +ovs-vsctl set interface ovs-sw0-p3-f external_ids:iface-id=sw0-p3-f > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xup]) > + > +# Send ping from sw0-p1-f to sw0-p3-f again and it should work. > +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | FORMAT_PING], \ > +[0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > + > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb >
On Mon, Jul 27, 2020 at 11:45 PM Mark Michelson <mmichels@redhat.com> wrote: > Acked-by: Mark Michelson <mmichels@redhat.com> > > There's one small comment in your test. You can just fix it when you > merge, though. > Thanks for the review. I addressed your comment and applied this patch to master and branch-20.06 Numan > > On 7/27/20 12:57 PM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > The commit in the Fixes tag while handling changes for OVS interface > changes and ct zone > > changes, called physical_run() without clearing the flow table. This > works ok for existing > > flows in the flow table which are associated with ovsdb rows. > physical_run() ensures that > > such flows are deleted by calling ofctrl_delete_flows() by adding them > back again. > > > > But flows not associated with ovsdb rows (whose OF flow cookie is set to > 0) are not cleared > > at all until a full recompute is triggered. Particularly flows in table > 33 which set > > various conntrack zone registers still remain. Suppose a lport is > claimed again and if > > the ct-zone id for this lport changes, then the old flow for this lport > still remains and > > this causes the packet to enter the conntrack with a wrong zone id. > > > > Such flows are stored indexed with the uuid 'hc_uuid' in the flow table > and it is easy > > to clear them. This patch clears such flows before calling > physical_run() to fix this issue. > > > > A more accurate fix would be to store the logical and physical flows in > separate flow tables > > and clear all the flows in the physical flow table before calling > physical_run(). > > This patch is good enough for now until we implement separate flow > tables. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1861042 > > Fixes: a3005f0dc777("ovn-controller: I-P for ct zone and OVS interface > changes in flow output stage.") > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > controller/ovn-controller.c | 1 + > > controller/physical.c | 8 +++ > > controller/physical.h | 1 + > > tests/ovn.at | 105 ++++++++++++++++++++++++++++++++++++ > > tests/system-ovn.at | 89 ++++++++++++++++++++++++++++++ > > 5 files changed, 204 insertions(+) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index e355007b88..5ca32ac433 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1994,6 +1994,7 @@ flow_output_physical_flow_changes_handler(struct > engine_node *node, void *data) > > > > if (pfc_data->recompute_physical_flows) { > > /* This indicates that we need to recompute the physical > flows. */ > > + physical_clear_unassoc_flows_with_db(&fo->flow_table); > > physical_run(&p_ctx, &fo->flow_table); > > return true; > > } > > diff --git a/controller/physical.c b/controller/physical.c > > index 6d7d8e93bc..535c777307 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -1851,3 +1851,11 @@ get_tunnel_ofport(const char *chassis_name, char > *encap_ip, ofp_port_t *ofport) > > *ofport = tun->ofport; > > return true; > > } > > + > > +void > > +physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table > *flow_table) > > +{ > > + if (hc_uuid) { > > + ofctrl_remove_flows(flow_table, hc_uuid); > > + } > > +} > > diff --git a/controller/physical.h b/controller/physical.h > > index 9ca34436a5..feab41df4c 100644 > > --- a/controller/physical.h > > +++ b/controller/physical.h > > @@ -61,6 +61,7 @@ struct physical_ctx { > > void physical_register_ovs_idl(struct ovsdb_idl *); > > void physical_run(struct physical_ctx *, > > struct ovn_desired_flow_table *); > > +void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table > *); > > void physical_handle_port_binding_changes(struct physical_ctx *, > > struct > ovn_desired_flow_table *); > > void physical_handle_mc_group_changes(struct physical_ctx *, > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 7c9e14e2ea..48ab4e0655 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -20806,3 +20806,108 @@ AT_CHECK([test "$hv2_offlows" = > "$hv2_offlows_mon"]) > > > > OVN_CLEANUP([hv1], [hv2]) > > AT_CLEANUP > > + > > + > > +# When a lport is released on a chassis, ovn-controller was > > +# not clearing some of the flowss in the table 33. > > +# Make sure that those flows are cleared properly. > > +AT_SETUP([ovn -- Test clear flows in physical tables]) > > +AT_KEYWORDS([lb]) > > Misleading keywords. > > > +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 > > + > > +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 > > + > > +as hv1 > > +ovs-vsctl -- add-port br-int hv1-vif2 -- \ > > + set interface hv1-vif2 external-ids:iface-id=sw0-p2 \ > > + options:tx_pcap=hv1/vif2-tx.pcap \ > > + options:rxq_pcap=hv1/vif2-rx.pcap \ > > + ofport-request=2 > > + > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) > > + > > +sw0_dpkey=$(ovn-sbctl --bare --columns tunnel_key list > datapath_binding sw0) > > +p1_dpkey=$(ovn-sbctl --bare --columns tunnel_key list port_binding > sw0-p1) > > +p2_dpkey=$(ovn-sbctl --bare --columns tunnel_key list port_binding > sw0-p2) > > + > > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int > external_ids:ct-zone-sw0-p1 | sed 's/"//g') > > +AT_CHECK([test ! -z $p1_zoneid]) > > + > > +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int > external_ids:ct-zone-sw0-p2 | sed 's/"//g') > > +AT_CHECK([test ! -z $p2_zoneid]) > > + > > +AT_CHECK([test $(ovs-ofctl dump-flows br-int > table=33,metadata=${sw0_dpkey},\ > > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1]) > > + > > +AT_CHECK([test $(ovs-ofctl dump-flows br-int > table=33,metadata=${sw0_dpkey},\ > > +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) > -eq 1]) > > + > > +AT_CHECK([test $(ovs-ofctl dump-flows br-int > table=33,metadata=${sw1_dpkey},\ > > +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 1]) > > + > > +AT_CHECK([test $(ovs-ofctl dump-flows br-int > table=33,metadata=${sw1_dpkey},\ > > +reg15=0x${p2_dpkey} | grep "load:0x${p2_zoneid}->NXM_NX_REG13" | wc -l) > -eq 1]) > > + > > +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xdown]) > > + > > +AT_CHECK([test $(ovs-ofctl dump-flows br-int > table=33,metadata=${sw0_dpkey},\ > > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0]) > > + > > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int > external_ids:ct-zone-sw0-p1 | sed 's/"//g') > > +AT_CHECK([test -z $p1_zoneid]) > > + > > +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=sw0-p1 > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) > > + > > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int > external_ids:ct-zone-sw0-p1 | sed 's/"//g') > > +AT_CHECK([test ! -z $p1_zoneid]) > > + > > +AT_CHECK([test $(ovs-ofctl dump-flows br-int > table=33,metadata=${sw0_dpkey},\ > > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1]) > > + > > +AT_CHECK([test $(ovs-ofctl dump-flows br-int > table=33,metadata=${sw0_dpkey},\ > > +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) > -eq 1]) > > + > > +ovs-vsctl del-port hv1-vif2 > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xdown]) > > + > > +AT_CHECK([test $(ovs-ofctl dump-flows br-int > table=33,metadata=${sw0_dpkey},\ > > +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 0]) > > + > > +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int > external_ids:ct-zone-sw0-p2 | sed 's/"//g') > > +AT_CHECK([test -z $p2_zoneid]) > > + > > +ovn-nbctl lsp-del sw0-p1 > > + > > +OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int > table=33,metadata=${sw0_dpkey},\ > > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0]) > > + > > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int > external_ids:ct-zone-sw0-p1 | sed 's/"//g') > > +AT_CHECK([test ! -z $p1_zoneid]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index eddc530f97..bca2601267 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -4467,6 +4467,95 @@ NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 20.0.0.4 80], > [0]) > > # Send the packet to VIP. > > NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 30.0.0.10 80], [0]) > > > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > + > > +as ovn-sb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > + > > +as ovn-nb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > + > > +as northd > > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > > + > > +as > > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > > +/connection dropped.*/d"]) > > + > > +AT_CLEANUP > > + > > +# When a lport is released on a chassis, ovn-controller was > > +# not clearing some of the flowss in the table 33 leading > > +# to packet drops if ct() is hit. > > +# Make sure that those flows are cleared properly. > > +AT_SETUP([ovn --Test packet drops due to incorrect flows in physical > table 33]) > > +AT_KEYWORDS([lb]) > > + > > +ovn_start > > + > > +OVS_TRAFFIC_VSWITCHD_START() > > +ADD_BR([br-int]) > > + > > +# Set external-ids in br-int needed for ovn-controller > > +ovs-vsctl \ > > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > > + -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > + -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > > + > > +# Start ovn-controller > > +start_daemon ovn-controller > > + > > +ovn-nbctl ls-add sw0 > > +ovn-nbctl lsp-add sw0 sw0-p1-f > > +ovn-nbctl lsp-set-addresses sw0-p1-f "10:54:00:00:00:03 10.0.0.3" > > + > > +ovn-nbctl lsp-add sw0 sw0-p2-f > > +ovn-nbctl lsp-set-addresses sw0-p2-f "10:54:00:00:00:04 10.0.0.4" > > + > > +ovn-nbctl lsp-add sw0 sw0-p3-f > > +ovn-nbctl lsp-set-addresses sw0-p3-f "10:54:00:00:00:05 10.0.0.5" > > + > > +# Add ACL with allow-ralated so that conntrack is hit. > > + > > +ovn-nbctl acl-add sw0 from-lport 1002 "ip" allow-related > > +ovn-nbctl acl-add sw0 to-lport 1002 "ip" allow-related > > + > > +ADD_NAMESPACES(sw0-p1-f) > > +ADD_VETH(sw0-p1-f, sw0-p1-f, br-int, "10.0.0.3/24", > "10:54:00:00:00:03", \ > > + "10.0.0.1") > > + > > +ADD_NAMESPACES(sw0-p2-f) > > +ADD_VETH(sw0-p2-f, sw0-p2-f, br-int, "10.0.0.4/24", > "10:54:00:00:00:04", \ > > + "10.0.0.1") > > + > > +ADD_NAMESPACES(sw0-p3-f) > > +ADD_VETH(sw0-p3-f, sw0-p3-f, br-int, "10.0.0.5/24", > "10:54:00:00:00:05", \ > > + "10.0.0.1") > > + > > +# Send ping from sw0-p1-f to sw0-p3-f > > +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | > FORMAT_PING], \ > > +[0], [dnl > > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > > +]) > > + > > +ovs-vsctl remove interface ovs-sw0-p2-f external_ids iface-id > > +ovs-vsctl remove interface ovs-sw0-p3-f external_ids iface-id > > + > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2-f) = xdown]) > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xdown]) > > + > > +ovs-vsctl set interface ovs-sw0-p3-f external_ids:iface-id=sw0-p3-f > > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xup]) > > + > > +# Send ping from sw0-p1-f to sw0-p3-f again and it should work. > > +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | > FORMAT_PING], \ > > +[0], [dnl > > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > > +]) > > + > > + > > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > > > as ovn-sb > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index e355007b88..5ca32ac433 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1994,6 +1994,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) if (pfc_data->recompute_physical_flows) { /* This indicates that we need to recompute the physical flows. */ + physical_clear_unassoc_flows_with_db(&fo->flow_table); physical_run(&p_ctx, &fo->flow_table); return true; } diff --git a/controller/physical.c b/controller/physical.c index 6d7d8e93bc..535c777307 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -1851,3 +1851,11 @@ get_tunnel_ofport(const char *chassis_name, char *encap_ip, ofp_port_t *ofport) *ofport = tun->ofport; return true; } + +void +physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *flow_table) +{ + if (hc_uuid) { + ofctrl_remove_flows(flow_table, hc_uuid); + } +} diff --git a/controller/physical.h b/controller/physical.h index 9ca34436a5..feab41df4c 100644 --- a/controller/physical.h +++ b/controller/physical.h @@ -61,6 +61,7 @@ struct physical_ctx { void physical_register_ovs_idl(struct ovsdb_idl *); void physical_run(struct physical_ctx *, struct ovn_desired_flow_table *); +void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *); void physical_handle_port_binding_changes(struct physical_ctx *, struct ovn_desired_flow_table *); void physical_handle_mc_group_changes(struct physical_ctx *, diff --git a/tests/ovn.at b/tests/ovn.at index 7c9e14e2ea..48ab4e0655 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -20806,3 +20806,108 @@ AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"]) OVN_CLEANUP([hv1], [hv2]) AT_CLEANUP + + +# When a lport is released on a chassis, ovn-controller was +# not clearing some of the flowss in the table 33. +# Make sure that those flows are cleared properly. +AT_SETUP([ovn -- Test clear flows in physical tables]) +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 + +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 + +as hv1 +ovs-vsctl -- add-port br-int hv1-vif2 -- \ + set interface hv1-vif2 external-ids:iface-id=sw0-p2 \ + options:tx_pcap=hv1/vif2-tx.pcap \ + options:rxq_pcap=hv1/vif2-rx.pcap \ + ofport-request=2 + +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup]) + +sw0_dpkey=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding sw0) +p1_dpkey=$(ovn-sbctl --bare --columns tunnel_key list port_binding sw0-p1) +p2_dpkey=$(ovn-sbctl --bare --columns tunnel_key list port_binding sw0-p2) + +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g') +AT_CHECK([test ! -z $p1_zoneid]) + +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p2 | sed 's/"//g') +AT_CHECK([test ! -z $p2_zoneid]) + +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1]) + +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) -eq 1]) + +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw1_dpkey},\ +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 1]) + +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw1_dpkey},\ +reg15=0x${p2_dpkey} | grep "load:0x${p2_zoneid}->NXM_NX_REG13" | wc -l) -eq 1]) + +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xdown]) + +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0]) + +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g') +AT_CHECK([test -z $p1_zoneid]) + +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=sw0-p1 +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) + +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g') +AT_CHECK([test ! -z $p1_zoneid]) + +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1]) + +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) -eq 1]) + +ovs-vsctl del-port hv1-vif2 +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xdown]) + +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 0]) + +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p2 | sed 's/"//g') +AT_CHECK([test -z $p2_zoneid]) + +ovn-nbctl lsp-del sw0-p1 + +OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\ +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0]) + +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g') +AT_CHECK([test ! -z $p1_zoneid]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP diff --git a/tests/system-ovn.at b/tests/system-ovn.at index eddc530f97..bca2601267 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -4467,6 +4467,95 @@ NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 20.0.0.4 80], [0]) # Send the packet to VIP. NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 30.0.0.10 80], [0]) +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([ovn-northd]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + +AT_CLEANUP + +# When a lport is released on a chassis, ovn-controller was +# not clearing some of the flowss in the table 33 leading +# to packet drops if ct() is hit. +# Make sure that those flows are cleared properly. +AT_SETUP([ovn --Test packet drops due to incorrect flows in physical table 33]) +AT_KEYWORDS([lb]) + +ovn_start + +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) + +# Set external-ids in br-int needed for ovn-controller +ovs-vsctl \ + -- set Open_vSwitch . external-ids:system-id=hv1 \ + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true + +# Start ovn-controller +start_daemon ovn-controller + +ovn-nbctl ls-add sw0 +ovn-nbctl lsp-add sw0 sw0-p1-f +ovn-nbctl lsp-set-addresses sw0-p1-f "10:54:00:00:00:03 10.0.0.3" + +ovn-nbctl lsp-add sw0 sw0-p2-f +ovn-nbctl lsp-set-addresses sw0-p2-f "10:54:00:00:00:04 10.0.0.4" + +ovn-nbctl lsp-add sw0 sw0-p3-f +ovn-nbctl lsp-set-addresses sw0-p3-f "10:54:00:00:00:05 10.0.0.5" + +# Add ACL with allow-ralated so that conntrack is hit. + +ovn-nbctl acl-add sw0 from-lport 1002 "ip" allow-related +ovn-nbctl acl-add sw0 to-lport 1002 "ip" allow-related + +ADD_NAMESPACES(sw0-p1-f) +ADD_VETH(sw0-p1-f, sw0-p1-f, br-int, "10.0.0.3/24", "10:54:00:00:00:03", \ + "10.0.0.1") + +ADD_NAMESPACES(sw0-p2-f) +ADD_VETH(sw0-p2-f, sw0-p2-f, br-int, "10.0.0.4/24", "10:54:00:00:00:04", \ + "10.0.0.1") + +ADD_NAMESPACES(sw0-p3-f) +ADD_VETH(sw0-p3-f, sw0-p3-f, br-int, "10.0.0.5/24", "10:54:00:00:00:05", \ + "10.0.0.1") + +# Send ping from sw0-p1-f to sw0-p3-f +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +ovs-vsctl remove interface ovs-sw0-p2-f external_ids iface-id +ovs-vsctl remove interface ovs-sw0-p3-f external_ids iface-id + +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2-f) = xdown]) +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xdown]) + +ovs-vsctl set interface ovs-sw0-p3-f external_ids:iface-id=sw0-p3-f +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xup]) + +# Send ping from sw0-p1-f to sw0-p3-f again and it should work. +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + + OVS_APP_EXIT_AND_WAIT([ovn-controller]) as ovn-sb