From patchwork Fri Jul 24 19:38:45 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1335934 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.133; helo=hemlock.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 hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BCzzW2HQxz9sRN for ; Sat, 25 Jul 2020 05:39:19 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 6AE898945E; Fri, 24 Jul 2020 19:39:17 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZW1wWkFEPRl4; Fri, 24 Jul 2020 19:39:15 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 6213489447; Fri, 24 Jul 2020 19:39:15 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 41CF3C004D; Fri, 24 Jul 2020 19:39:15 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 27696C004C for ; Fri, 24 Jul 2020 19:39:14 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 950EF88A69 for ; Fri, 24 Jul 2020 19:38:56 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id VauFTp5Noe1a for ; Fri, 24 Jul 2020 19:38:55 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by whitealder.osuosl.org (Postfix) with ESMTPS id 0C67D8862F for ; Fri, 24 Jul 2020 19:38:54 +0000 (UTC) X-Originating-IP: 27.7.184.31 Received: from nusiddiq.home.org.com (unknown [27.7.184.31]) (Authenticated sender: numans@ovn.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id E71301BF205; Fri, 24 Jul 2020 19:38:50 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Sat, 25 Jul 2020 01:08:45 +0530 Message-Id: <20200724193845.1956595-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn] ovn-controller: Fix the missing flows when logical router port is added after its peer. 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 When the logical router port is created by CMS after its peer port is created like below, ovn-controller doesn't add the logical router to the local_datapaths and hence misses programming the flows for the router datapath if the logical switch has logical ports which are already bound. This breaks routing for these logical ports connected to this router. ovn-nbctl lsp-add sw0 sw0-lr0 ovn-nbctl lsp-set-type sw0-lr0 router ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:00:01 ovn-nbctl lr-add lr0 ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:00:01 192.168.1.1/24 This patch fixes this issue. Fixes: 354bdba51abf("ovn-controller: I-P for SB port binding and OVS interface in runtime_data.") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1860053 Signed-off-by: Numan Siddique --- controller/binding.c | 51 ++++++++++++++++++++++----- tests/ovn.at | 82 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 8 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index b73abb982c..1936b93e7a 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1522,6 +1522,22 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, return !any_changes; } +static const struct sbrec_port_binding * +get_peer_lport(const struct sbrec_port_binding *pb, + struct binding_ctx_in *b_ctx_in) +{ + const char *peer_name = smap_get(&pb->options, "peer"); + if (strcmp(pb->type, "patch") || !peer_name) { + return NULL; + } + + const struct sbrec_port_binding *peer; + peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, + peer_name); + + return (peer && peer->datapath) ? peer : NULL; +} + /* This function adds the local datapath of the 'peer' of * lport 'pb' to the local datapaths if it is not yet added. */ @@ -1531,16 +1547,10 @@ add_local_datapath_peer_port(const struct sbrec_port_binding *pb, struct binding_ctx_out *b_ctx_out, struct local_datapath *ld) { - const char *peer_name = smap_get(&pb->options, "peer"); - if (strcmp(pb->type, "patch") || !peer_name) { - return; - } - const struct sbrec_port_binding *peer; - peer = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, - peer_name); + peer = get_peer_lport(pb, b_ctx_in); - if (!peer || !peer->datapath) { + if (!peer) { return; } @@ -2118,6 +2128,31 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, case LP_VTEP: update_local_lport_ids(pb, b_ctx_out); if (lport_type == LP_PATCH) { + if (!ld) { + /* If 'ld' for this lport is not present, then check if + * there is a peer for this lport. If peer is present + * and peer's datapath is already in the local datapaths, + * then add this lport's datapath to the local_datapaths. + * */ + const struct sbrec_port_binding *peer; + peer = get_peer_lport(pb, b_ctx_in); + struct local_datapath *peer_ld = + get_local_datapath(b_ctx_out->local_datapaths, + peer->datapath->tunnel_key); + if (peer_ld) { + add_local_datapath( + b_ctx_in->sbrec_datapath_binding_by_key, + b_ctx_in->sbrec_port_binding_by_datapath, + b_ctx_in->sbrec_port_binding_by_name, + pb->datapath, false, + b_ctx_out->local_datapaths, + b_ctx_out->tracked_dp_bindings); + } + + ld = get_local_datapath(b_ctx_out->local_datapaths, + pb->datapath->tunnel_key); + } + /* Add the peer datapath to the local datapaths if it's * not present yet. */ diff --git a/tests/ovn.at b/tests/ovn.at index 7c9e14e2ea..3be62584d2 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -20806,3 +20806,85 @@ AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"]) OVN_CLEANUP([hv1], [hv2]) AT_CLEANUP + + +AT_SETUP([ovn -- controller I-P handling when lrp added last]) +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 + +# Step 1. Add OVS interface with external_ids:iface-id set. +# Step 2. Create the logical switch and logical port. +# Step 3. Create logical switch port of type router and set the peer. +# Step 4. Create logical router and the logical router port (peer to logical switch) +# Step 5. Check that all the flows are added and logical port gets arp reply for +# router IP. + +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 + +ovn-nbctl ls-add sw0 +ovn-nbctl lsp-add sw0 sw0-p1 +ovn-nbctl lsp-set-addresses sw0-p1 "00:00:00:01:01:02 192.168.1.2" + +ovn-nbctl lsp-add sw0 sw0-lr0 +ovn-nbctl lsp-set-type sw0-lr0 router +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 +ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:00:01 + +ovn-nbctl lr-add lr0 +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:00:01 192.168.1.1/24 aef0:0:0:0:0:0:0:1/64 + +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup]) + +sw0_dpkey=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding sw0) +lr0_dpkey=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding lr0) + +AT_CHECK([test $(as hv1 ovs-ofctl dump-flows br-int metadata=0x${sw0_dpkey} | wc -l) -gt 80]) +AT_CHECK([test $(as hv1 ovs-ofctl dump-flows br-int metadata=0x${lr0_dpkey} | wc -l) -gt 80]) + +# test_arp INPORT SHA SPA TPA [REPLY_HA] +# +# Causes a packet to be received on INPORT. The packet is an ARP +# request with SHA, SPA, and TPA as specified. If REPLY_HA is provided, then +# it should be the hardware address of the target to expect to receive in an +# ARP reply; otherwise no reply is expected. +# +# INPORT is an logical switch port number, e.g. 11 for vif11. +# SHA and REPLY_HA are each 12 hex digits. +# SPA and TPA are each 8 hex digits. +test_arp() { + local hv=$1 inport=$2 sha=$3 spa=$4 tpa=$5 reply_ha=$6 + local request=ffffffffffff${sha}08060001080006040001${sha}${spa}ffffffffffff${tpa} + as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request + + if test X$reply_ha != X; then + # Expect to receive the reply, if any. + local reply=${sha}${reply_ha}08060001080006040002${reply_ha}${tpa}${sha}${spa} + echo $reply >> hv${hv}-vif$inport.expected + fi +} + +ip_to_hex() { + printf "%02x%02x%02x%02x" "$@" +} + +sw0p1_ip=$(ip_to_hex 192 168 1 2) +rtr_ip=$(ip_to_hex 192 168 1 1) +test_arp 1 1 000000010102 $sw0p1_ip $rtr_ip 000000000001 + +# Now check the packets actually received against the ones expected. +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [hv1-vif1.expected]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP