From patchwork Tue Oct 18 09:19:37 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Xavier Simonart X-Patchwork-Id: 1691430 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::138; helo=smtp1.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Q65Z9dmb; dkim-atps=neutral Received: from smtp1.osuosl.org (smtp1.osuosl.org [IPv6:2605:bc80:3010::138]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4Ms7c52q1Hz23jp for ; Tue, 18 Oct 2022 20:19:49 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 2E58883131; Tue, 18 Oct 2022 09:19:47 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 2E58883131 Authentication-Results: smtp1.osuosl.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Q65Z9dmb X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GHTyaYKXvqyv; Tue, 18 Oct 2022 09:19:46 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp1.osuosl.org (Postfix) with ESMTPS id 168A482868; Tue, 18 Oct 2022 09:19:45 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 168A482868 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id E11BFC0033; Tue, 18 Oct 2022 09:19:44 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 349CEC002D for ; Tue, 18 Oct 2022 09:19:43 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 032E941774 for ; Tue, 18 Oct 2022 09:19:43 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 032E941774 Authentication-Results: smtp4.osuosl.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Q65Z9dmb X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id q7Km31eamlGM for ; Tue, 18 Oct 2022 09:19:41 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org D3D3440354 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp4.osuosl.org (Postfix) with ESMTPS id D3D3440354 for ; Tue, 18 Oct 2022 09:19:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1666084779; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=EDGMHDKcSpCWKwi+fMvC+jp8Ih7tq+9/ffYXcb17p9Q=; b=Q65Z9dmbc8iQ58f9cgxSAscHK3VNr6e4Jy87CJ32PiJutSGbLXTMPHktJRlic68IU1g11C PC7pT66qMHc9UT4gaMZdo6sem5Y6LWcQaLxn0nKQ4F5kRUCtEZjZViQbaWiDesl2eoHLFG 0vtlPkkEm7ngIMVolRotFZYtyWqN+t0= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-634-uhUM5sdGNW6MxfHsg8ZlIw-1; Tue, 18 Oct 2022 05:19:38 -0400 X-MC-Unique: uhUM5sdGNW6MxfHsg8ZlIw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 65F91380670D for ; Tue, 18 Oct 2022 09:19:38 +0000 (UTC) Received: from wsfd-netdev90.ntdv.lab.eng.bos.redhat.com (wsfd-netdev90.ntdv.lab.eng.bos.redhat.com [10.19.188.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 45A82215CDCA; Tue, 18 Oct 2022 09:19:38 +0000 (UTC) From: Xavier Simonart To: xsimonar@redhat.com, dev@openvswitch.org Date: Tue, 18 Oct 2022 05:19:37 -0400 Message-Id: <20221018091937.955476-1-xsimonar@redhat.com> In-Reply-To: <20221017122046.1381341-1-xsimonar@redhat.com> References: <20221017122046.1381341-1-xsimonar@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [PATCH ovn v2] ovn-controller: Fixed missing flows after interface deletion 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" In the following scenario: - interface "old" is created and external_ids:iface-id is set (to lp) - interface "new" is created and external_ids:iface-id is set (to same lp) - interface "old" is deleted flows related to lp were deleted. Note that after "new" interface is created, flows use "new" ofport. This patch only supports up to two ovs interfaces bound to the same iface-id. Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for pflow_output engine.") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866 Signed-off-by: Xavier Simonart --- v2: - Use bool instead of int for binding count to better reflect only one additional binding is supported. - Fix use after free. - Remove debug logging from test case --- controller/binding.c | 85 +++++++++++++++++++++++++++++++---- controller/binding.h | 1 + tests/ovn.at | 105 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 9 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index c3d2b2e42..b1dfb86b3 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2155,8 +2155,12 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, if (!lbinding) { lbinding = local_binding_create(iface_id, iface_rec); local_binding_add(local_bindings, lbinding); - } else { + } else if (!lbinding->iface) { + lbinding->iface = iface_rec; + } else if (lbinding->iface != iface_rec) { lbinding->iface = iface_rec; + lbinding->multiple_bindings = true; + b_ctx_out->local_lports_changed = true; } struct binding_lport *b_lport = @@ -2219,13 +2223,23 @@ static bool consider_iface_release(const struct ovsrec_interface *iface_rec, const char *iface_id, struct binding_ctx_in *b_ctx_in, - struct binding_ctx_out *b_ctx_out) + struct binding_ctx_out *b_ctx_out, + bool *multiple_bindings) { struct local_binding *lbinding; struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; lbinding = local_binding_find(local_bindings, iface_id); + if (lbinding && !lbinding->multiple_bindings && + lbinding->iface != iface_rec && !ofport) { + VLOG_DBG("Not releasing lport %s as %s was claimed " + "and %s was never bound)", + iface_id, lbinding->iface ? lbinding->iface->name:"", + iface_rec->name); + return true; + } struct binding_lport *b_lport = local_binding_get_primary_or_localport_lport(lbinding); if (is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec)) { @@ -2254,8 +2268,10 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, } if (lbinding) { + *multiple_bindings = lbinding->multiple_bindings; /* Clear the iface of the local binding. */ lbinding->iface = NULL; + lbinding->multiple_bindings = false; } /* Check if the lbinding has children of type PB_CONTAINER. @@ -2266,8 +2282,6 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, } remove_local_lports(iface_id, b_ctx_out); - smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); - return true; } @@ -2375,6 +2389,10 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, bool handled = true; + struct hmap qos_map = HMAP_INITIALIZER(&qos_map); + struct hmap *qos_map_ptr = + sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; + /* Run the tracked interfaces loop twice. One to handle deleted * changes. And another to handle add/update changes. * This will ensure correctness. @@ -2435,8 +2453,59 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, } if (cleared_iface_id) { + bool multiple_bindings = false; handled = consider_iface_release(iface_rec, cleared_iface_id, - b_ctx_in, b_ctx_out); + b_ctx_in, b_ctx_out, + &multiple_bindings); + if (!handled) { + break; + } + if (!multiple_bindings) { + smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); + continue; + } + /* There is another OVS interface bound to this port. + * First find its name + */ + VLOG_DBG("binding remaining for %s", cleared_iface_id); + struct smap_node *node; + bool found = false; + SMAP_FOR_EACH (node, b_ctx_out->local_iface_ids) { + if (strcmp(node->value, cleared_iface_id) || + !strcmp(node->key, iface_rec->name)) { + continue; + } + /* Then find its iface_rec */ + char *other_iface_rec_name = node->key; + const struct ovsrec_interface *other_iface_rec; + OVSREC_INTERFACE_TABLE_FOR_EACH (other_iface_rec, + b_ctx_in->iface_table) { + if (!strcmp(other_iface_rec->name, other_iface_rec_name)) { + found = true; + break; + } + } + if (found) { + int64_t ofport = other_iface_rec->n_ofport ? + *other_iface_rec->ofport : 0; + if (cleared_iface_id && ofport > 0 && + is_iface_in_int_bridge(other_iface_rec, + b_ctx_in->br_int)) { + handled = consider_iface_claim(other_iface_rec, + cleared_iface_id, + b_ctx_in, + b_ctx_out, qos_map_ptr); + } + } + break; + } + if (!found) { + /* This is unexpected. Recompute to clean up */ + VLOG_WARN("Did not find which OVS interface still bound to %s", + cleared_iface_id); + handled = false; + } + smap_remove(b_ctx_out->local_iface_ids, iface_rec->name); } if (!handled) { @@ -2448,13 +2517,10 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, /* This can happen if any non vif OVS interface is in the tracked * list or if consider_iface_release() returned false. * There is no need to process further. */ + destroy_qos_map(&qos_map); return false; } - struct hmap qos_map = HMAP_INITIALIZER(&qos_map); - struct hmap *qos_map_ptr = - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map; - /* * We consider an OVS interface for claiming if the following * 2 conditions are met: @@ -3034,6 +3100,7 @@ local_binding_create(const char *name, const struct ovsrec_interface *iface) struct local_binding *lbinding = xzalloc(sizeof *lbinding); lbinding->name = xstrdup(name); lbinding->iface = iface; + lbinding->multiple_bindings = false; ovs_list_init(&lbinding->binding_lports); return lbinding; diff --git a/controller/binding.h b/controller/binding.h index ad959a9e6..6c3a98b02 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -135,6 +135,7 @@ struct local_binding { char *name; const struct ovsrec_interface *iface; struct ovs_list binding_lports; + bool multiple_bindings; }; struct local_binding_data { diff --git a/tests/ovn.at b/tests/ovn.at index f8b8db4df..d9b720a98 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -32975,3 +32975,108 @@ check ovn-nbctl --wait=hv sync OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical port]) +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 + +get_flows() +{ + cookie=$1 + ovs-ofctl dump-flows br-int | grep $cookie | + sed -e 's/duration=[[0-9.]]*s, //g' | + sed -e 's/idle_age=[[0-9]]*, //g' | + sed -e 's/n_packets=[[0-9]]*, //g' | + sed -e 's/n_bytes=[[0-9]]*, //g' +} + +check ovn-nbctl ls-add ls +check ovn-nbctl lsp-add ls lp +check ovn-nbctl lsp-set-type lp localport +check ovn-nbctl lsp-set-addresses lp "00:00:00:01:01:02 192.168.1.2" + +check ovn-nbctl lsp-add ls vm1 +check ovn-nbctl lsp-set-addresses vm1 "00:00:00:01:01:11 192.168.1.11" +check ovs-vsctl add-port br-int vm1 -- set interface vm1 type=internal external_ids:iface-id=vm1 + +check ovn-nbctl --wait=hv sync + +check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal +check ovs-vsctl set interface lpold external_ids:iface-id=lp + +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns _uuid find port_binding logical_port=lp) != x]) +echo ====================================================== +echo === Flows after iface-id set for the old interface === +echo ====================================================== +COOKIE=$(ovn-sbctl find port_binding logical_port=lp|grep uuid|cut -d: -f2| cut -c1-8 | sed 's/^\s*0\{0,8\}//') + +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpold) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` +echo $nb_flows "flows after iface-id set for old interface" + +echo ====================================================== +echo === Flows after iface-id set for the new interface === +echo ====================================================== +check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal +check ovs-vsctl set interface lpnew external_ids:iface-id=lp +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_lpnew=$(get_flows $COOKIE) + +echo ====================================================== +echo ======= Flows after old interface is deleted ========= +echo ====================================================== +check ovs-vsctl del-port br-int lpold +# We do not expect changes, so let's wait a few seconds to check nothing changed +sleep 2 +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_after_deletion=$(get_flows $COOKIE) +check test "$flows_lpnew" = "$flows_after_deletion" + +echo ====================================================== +echo ======= Flows after lptemp interface is created ==== +echo ====================================================== +check ovs-vsctl add-port br-int lptemp -- set Interface lptemp type=internal +check ovs-vsctl set Interface lptemp external_ids:iface-id=lp +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lptemp) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) + +echo ====================================================== +echo ======= Flows after lptemp interface is deleted ====== +echo ====================================================== +check ovs-vsctl del-port br-int lptemp +OVS_WAIT_UNTIL([ + ofport=$(ovs-vsctl --bare --columns ofport find Interface name=lpnew) + ovs-ofctl dump-flows br-int | grep $COOKIE | grep "actions=output:$ofport" +]) +check test "$nb_flows" = $(ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l) +flows_after_deletion=$(get_flows $COOKIE) +check test "$flows_lpnew" = "$flows_after_deletion" + +echo ====================================================== +echo ======= Flows after new interface is deleted ========= +echo ====================================================== +check ovs-vsctl del-port br-int lpnew +OVS_WAIT_UNTIL([ + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` + test "${nb_flows}" = 0 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +])