From patchwork Mon Oct 17 12:20:46 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Xavier Simonart X-Patchwork-Id: 1690907 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::137; helo=smtp4.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=BsFgjwE3; dkim-atps=neutral Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) (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 4MrbgY0Jrdz23kK for ; Mon, 17 Oct 2022 23:20:57 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 26EA74092B; Mon, 17 Oct 2022 12:20:55 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 26EA74092B Authentication-Results: smtp4.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=BsFgjwE3 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 4-7IbCwPO3kN; Mon, 17 Oct 2022 12:20:53 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTPS id 835624085F; Mon, 17 Oct 2022 12:20:52 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 835624085F Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 24DEBC0033; Mon, 17 Oct 2022 12:20:52 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 7BA50C002D for ; Mon, 17 Oct 2022 12:20:51 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 5628660C22 for ; Mon, 17 Oct 2022 12:20:51 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 5628660C22 Authentication-Results: smtp3.osuosl.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=BsFgjwE3 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Wx5VQaHy9jQZ for ; Mon, 17 Oct 2022 12:20:50 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 04538600CA Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by smtp3.osuosl.org (Postfix) with ESMTPS id 04538600CA for ; Mon, 17 Oct 2022 12:20:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1666009248; 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; bh=nCm+kcAAAfqTEOWO6d3YUvdKRkjPVIaYIOTh16WtCsg=; b=BsFgjwE3FPyvcjZAw0twifB+B7MNHYydVoSxSuIw8RJIdjeej064hbmHVZlek66OstNRk8 o/GgCNRGpoMAhnZXYCW8+XhxVEk6Eo/AaC66Xls67adF5JnkyR37RuBxX234yNGWUhiSTD 0q+/y7egY18+L6k/Ip9EkNXQ5DPONmo= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-377-Oipxd7PDNtGXhWMhBsOiuA-1; Mon, 17 Oct 2022 08:20:47 -0400 X-MC-Unique: Oipxd7PDNtGXhWMhBsOiuA-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 58CB8185A79C for ; Mon, 17 Oct 2022 12:20:47 +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 3A0572166B29; Mon, 17 Oct 2022 12:20:47 +0000 (UTC) From: Xavier Simonart To: xsimonar@redhat.com, dev@openvswitch.org Date: Mon, 17 Oct 2022 08:20:46 -0400 Message-Id: <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] 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 --- controller/binding.c | 78 ++++++++++++++++++++++++++++--- controller/binding.h | 1 + tests/ovn.at | 106 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 7 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index c3d2b2e42..8ab0fe659 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2156,7 +2156,11 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, lbinding = local_binding_create(iface_id, iface_rec); local_binding_add(local_bindings, lbinding); } else { - lbinding->iface = iface_rec; + if (lbinding->iface != iface_rec) { + lbinding->iface = iface_rec; + lbinding->count++; + b_ctx_out->local_lports_changed = true; + } } struct binding_lport *b_lport = @@ -2219,13 +2223,21 @@ 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, + int *count) { 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->iface != iface_rec && !ofport) { + VLOG_DBG("Not releasing lport %s as %s was claimed " + "and %s was never bound)", + iface_id, 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,6 +2266,7 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, } if (lbinding) { + *count = lbinding->count - 1; /* Clear the iface of the local binding. */ lbinding->iface = NULL; } @@ -2375,6 +2388,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 +2452,57 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, } if (cleared_iface_id) { + int count = 0; handled = consider_iface_release(iface_rec, cleared_iface_id, - b_ctx_in, b_ctx_out); + b_ctx_in, b_ctx_out, &count); + if (!handled) { + break; + } + if (!count) { + continue; + } + /* There is another OVS interface bound to this port. + * First find its name + */ + VLOG_DBG("%d binding(s) remaining for %s", + count, 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; + } } if (!handled) { @@ -2448,13 +2514,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 +3097,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->count = 1; ovs_list_init(&lbinding->binding_lports); return lbinding; diff --git a/controller/binding.h b/controller/binding.h index ad959a9e6..a8591325e 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; + int count; }; struct local_binding_data { diff --git a/tests/ovn.at b/tests/ovn.at index f8b8db4df..c759423a8 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -32975,3 +32975,109 @@ 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 +ovn-appctl vlog/set dbg + +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 +])