From patchwork Mon Nov 14 09:24: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: 1703491 X-Patchwork-Delegate: zhouhan@gmail.com 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=140.211.166.136; helo=smtp3.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=e7HSn9uW; dkim-atps=neutral Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (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 4N9kRS6dKWz20KC for ; Mon, 14 Nov 2022 20:24:50 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 507BE60B95; Mon, 14 Nov 2022 09:24:47 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 507BE60B95 Authentication-Results: smtp3.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=e7HSn9uW 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 bGqmg9I1Pdgx; Mon, 14 Nov 2022 09:24:46 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp3.osuosl.org (Postfix) with ESMTPS id 0B12660A8A; Mon, 14 Nov 2022 09:24:44 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 0B12660A8A Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id D4B10C0032; Mon, 14 Nov 2022 09:24:44 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 882B5C002D for ; Mon, 14 Nov 2022 09:24:43 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 5C1B740363 for ; Mon, 14 Nov 2022 09:24:43 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 5C1B740363 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=e7HSn9uW 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 yqYTGXq7YIvX for ; Mon, 14 Nov 2022 09:24:41 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 9002040330 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 9002040330 for ; Mon, 14 Nov 2022 09:24:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1668417880; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Q00IUkso5JFFkOV7cff56q750QFffmpKRQurbjgeczA=; b=e7HSn9uWbHcsF2fUpmbhXlhV4w5k6kRvfD2W8d0sTFt0KahS+cplo+aSmiUldqAVl4cUlY tgmMChQxAL0zv1e7XpcfnYjnHlYhhah7OzXGQOJT41wVSGjxJPHYj5gVFPJcabQy5yizwM D4ra2Fx1k7W3XWxDodlEx/A5rsjIbnk= 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-28-rYL7ud9hOu2SpmQA9SmnkQ-1; Mon, 14 Nov 2022 04:24:38 -0500 X-MC-Unique: rYL7ud9hOu2SpmQA9SmnkQ-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9D14D3C025A1; Mon, 14 Nov 2022 09:24: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 59746492B12; Mon, 14 Nov 2022 09:24:38 +0000 (UTC) From: Xavier Simonart To: xsimonar@redhat.com, dev@openvswitch.org Date: Mon, 14 Nov 2022 04:24:37 -0500 Message-Id: <20221114092437.2807815-1-xsimonar@redhat.com> In-Reply-To: <20221024152144.1787203-1-xsimonar@redhat.com> References: <20221024152144.1787203-1-xsimonar@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: dceara@redhat.com Subject: [ovs-dev] [PATCH ovn v5] 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. The state where old and new interfaces have both external_ids:iface-id set at the same time is "invalid", and all flows are not installed for lpold. 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 Reviewed-by: Ales Musil Acked-by: Han Zhou Acked-by: Ihar Hrachyshka --- 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 v3: - Based on Dumitru's and Mark's feedback: - Support any number of interfaces bound to the same port - Use recomputes to make code simpler (this is corner case) - Added test case using three interfaces bound to te same port v4: - Updated based on Ales' feedback - Also support scenario for port-types different than localport - Added test case for VIF port - Rebased on latest main v5: - Updated test case based on Numan/Dumitru's feedback (hit ofport = 0) and added comments in code. - Rebased on latest main. --- controller/binding.c | 36 ++++++++++ controller/binding.h | 1 + tests/ovn.at | 168 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 205 insertions(+) diff --git a/controller/binding.c b/controller/binding.c index c3d2b2e42..5f04b9a74 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1866,6 +1866,7 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in, lbinding = local_binding_create(iface_id, iface_rec); local_binding_add(local_bindings, lbinding); } else { + lbinding->multiple_bindings = true; static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); VLOG_WARN_RL( @@ -2156,6 +2157,10 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, lbinding = local_binding_create(iface_id, iface_rec); local_binding_add(local_bindings, lbinding); } else { + if (lbinding->iface && lbinding->iface != iface_rec) { + lbinding->multiple_bindings = true; + b_ctx_out->local_lports_changed = true; + } lbinding->iface = iface_rec; } @@ -2174,6 +2179,13 @@ consider_iface_claim(const struct ovsrec_interface *iface_rec, return true; } + /* If multiple bindings to the same port, remove the "old" binding. + * This ensures that change tracking is correct. + */ + if (lbinding->multiple_bindings) { + remove_related_lport(pb, b_ctx_out); + } + enum en_lport_type lport_type = get_lport_type(pb); if (lport_type == LP_LOCALPORT) { return consider_localport(pb, b_ctx_in, b_ctx_out); @@ -2226,6 +2238,29 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, struct shash *binding_lports = &b_ctx_out->lbinding_data->lports; lbinding = local_binding_find(local_bindings, iface_id); + + if (lbinding) { + if (lbinding->multiple_bindings) { + VLOG_INFO("Multiple bindings for %s: force recompute to clean up", + iface_id); + return false; + } else { + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0; + if (lbinding->iface != iface_rec && !ofport) { + /* If external_ids:iface-id is set within the same transaction + * as adding an interface to a bridge, ovn-controller is + * usually initially notified of ovs interface changes with + * ofport == 0. If the lport was bound to a different interface + * we do not want to release it. + */ + 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)) { @@ -3034,6 +3069,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 6552681bd..7a918c639 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -33053,3 +33053,171 @@ check ovn-nbctl --wait=hv sync OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +m4_define([MULTIPLE_OVS_INT], + [OVN_FOR_EACH_NORTHD([ + AT_SETUP([ovn-controller: Multiple OVS interfaces bound to same logical port ($1)]) + 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 + if test X$1 != X; then + check ovn-nbctl lsp-set-type lp $1 + fi + 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 ====================================================== + # Set external_ids:iface-id within same transaction as adding the port. + # This will generally cause ovn-controller to get initially notified of ovs interface changes with ofport == 0. + check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal -- 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 for controller to get time to process any update + check ovn-nbctl --wait=hv sync + 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 ====================================================== + # Set external_ids:iface-id in a different transaction as adding the port. + # This will generally cause ovn-controller to get notified of ovs interface changes with a proper ofport. + 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) + echo $ofport + ovs-ofctl dump-flows br-int | grep $COOKIE + 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 + ]) + + echo ====================================================== + echo ======= Three interfaces bound to the same port ====== + echo ====================================================== + check ovs-vsctl add-port br-int lpold -- set interface lpold type=internal + check ovs-vsctl set interface lpold external_ids:iface-id=lp + check ovs-vsctl add-port br-int lpnew -- set interface lpnew type=internal + check ovs-vsctl set interface lpnew external_ids:iface-id=lp + + # Wait for lpnew flows to be installed + 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" + ]) + flows_lpnew=$(get_flows $COOKIE) + nb_flows=`ovs-ofctl dump-flows br-int | grep $COOKIE | wc -l` + + check ovs-vsctl add-port br-int lptemp -- set Interface lptemp type=internal + check ovs-vsctl set Interface lptemp external_ids:iface-id=lp + + # Wait for lptemp flows to be installed + 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" + ]) + + # Delete both lpold and lptemp to go to a stable situation + check ovs-vsctl del-port br-int lptemp + check ovs-vsctl del-port br-int lpold + + OVS_WAIT_UNTIL([ + test 0 = $(ovs-vsctl show | grep "Port lpold" | wc -l) + ]) + + # Wait for correct/lpnew flows to be installed + 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" + + # Check that recompute still works + check ovn-appctl -t ovn-controller recompute + 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" + + OVN_CLEANUP([hv1]) + AT_CLEANUP + ])]) + +MULTIPLE_OVS_INT([localport]) +MULTIPLE_OVS_INT([])