[ovs-dev] [PATCH ovn] ovn-controller: Clear flows not associated with db rows in physical flow change handler.

numans at ovn.org numans at ovn.org
Mon Jul 27 16:57:02 UTC 2020


From: Numan Siddique <numans at 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 at 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])
+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
-- 
2.26.2



More information about the dev mailing list