From patchwork Fri Jul 24 21:02:52 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1335978 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 4BD1rG00BJz9sRR for ; Sat, 25 Jul 2020 07:03:09 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 1B1DE89455; Fri, 24 Jul 2020 21:03:07 +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 3PrYA5wIFgTp; Fri, 24 Jul 2020 21:03:06 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id E9E56894D6; Fri, 24 Jul 2020 21:03:05 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id BCD75C004E; Fri, 24 Jul 2020 21:03:05 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 0C319C004C for ; Fri, 24 Jul 2020 21:03:04 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id E94198735E for ; Fri, 24 Jul 2020 21:03:03 +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 99BmhYwEemEw for ; Fri, 24 Jul 2020 21:03:02 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 5E1E687358 for ; Fri, 24 Jul 2020 21:03:02 +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 relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 23B36240003; Fri, 24 Jul 2020 21:02:57 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Sat, 25 Jul 2020 02:32:52 +0530 Message-Id: <20200724210252.1976634-1-numans@ovn.org> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn v2] 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 Acked-by: Mark Michelson v2 ---- * Addressed the review comment from Mark. controller/binding.c | 54 ++++++++++++++++++++++++----- tests/ovn.at | 82 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 8 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index b73abb982c..520bff080f 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,34 @@ 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; + struct local_datapath *peer_ld = NULL; + peer = get_peer_lport(pb, b_ctx_in); + if (peer) { + 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