From patchwork Mon Jul 27 16:57:02 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1337087 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.137; helo=fraxinus.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BFmG83cC3z9sR4 for ; Tue, 28 Jul 2020 02:58:07 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 355BC86356; Mon, 27 Jul 2020 16:58:06 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id fH13YLcbj19e; Mon, 27 Jul 2020 16:58:04 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id DD82E85F19; Mon, 27 Jul 2020 16:58:04 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id C1C69C004F; Mon, 27 Jul 2020 16:58:04 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 088D4C004D for ; Mon, 27 Jul 2020 16:58:03 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id A0290220C4 for ; Mon, 27 Jul 2020 16:57:15 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UJYol5hPUMNy for ; Mon, 27 Jul 2020 16:57:14 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by silver.osuosl.org (Postfix) with ESMTPS id C2B661FE32 for ; Mon, 27 Jul 2020 16:57:13 +0000 (UTC) X-Originating-IP: 115.99.218.166 Received: from nusiddiq.home.org.com (unknown [115.99.218.166]) (Authenticated sender: numans@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 7941020007; Mon, 27 Jul 2020 16:57:09 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Mon, 27 Jul 2020 22:27:02 +0530 Message-Id: <20200727165702.2769313-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn] ovn-controller: Clear flows not associated with db rows in physical flow change handler. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" From: Numan Siddique 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 Acked-by: Mark Michelson --- 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