From patchwork Wed Jun 2 07:07:31 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1486443 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=2605:bc80:3010::133; helo=smtp2.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Fw0V350Qwz9sT6 for ; Wed, 2 Jun 2021 17:07:55 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 616A8402BE; Wed, 2 Jun 2021 07:07:53 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JXEvwkzyqkDz; Wed, 2 Jun 2021 07:07:51 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp2.osuosl.org (Postfix) with ESMTP id 7AEDD400BA; Wed, 2 Jun 2021 07:07:50 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 609EAC000D; Wed, 2 Jun 2021 07:07:50 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 95680C0001 for ; Wed, 2 Jun 2021 07:07:49 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 84929400F6 for ; Wed, 2 Jun 2021 07:07:49 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id NoEhe62-k49k for ; Wed, 2 Jun 2021 07:07:45 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by smtp2.osuosl.org (Postfix) with ESMTPS id 5F1FC400BA for ; Wed, 2 Jun 2021 07:07:45 +0000 (UTC) Received: (Authenticated sender: hzhou@ovn.org) by relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 00AE6240481; Wed, 2 Jun 2021 07:07:42 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Wed, 2 Jun 2021 00:07:31 -0700 Message-Id: <20210602070731.3736171-1-hzhou@ovn.org> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn] ovn-controller: Fix port-group incremental processing. 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" When SB port-group (per DP) is deleted and added back and the notification to ovn-controller include both operations in a reversed order, the current change handler in ovn-controller would end up deleting the SB port-group in the local cache, which in turn result in missing flows, and even worse it can result in crash if runtime data handler is triggered later because we asserts the SB PG should be equivalent to the local cache. This patch fixes it by always handling deletion for tracked PG changes, just like how we are handling other tracked changes in I-P (e.g. logical_flow). A test case is crafted to cover such scenario. Fixes: 0cfeba6b55 ("ovn-controller: Fix port group conjunction flow explosion problem.") Signed-off-by: Han Zhou Acked-by: Mark Michelson --- controller/ovn-controller.c | 6 +++++- tests/ovn.at | 28 +++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index d48ddc7a2..07c6fcfd1 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1556,7 +1556,11 @@ port_groups_update(const struct sbrec_port_group_table *port_group_table, expr_const_sets_remove(port_groups_cs_local, pg->name); port_group_ssets_delete(port_group_ssets, pg->name); sset_add(deleted, pg->name); - } else { + } + } + + SBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (pg, port_group_table) { + if (!sbrec_port_group_is_deleted(pg)) { port_group_ssets_add_or_update(port_group_ssets, pg); expr_const_sets_add_strings(port_groups_cs_local, pg->name, (const char *const *) pg->ports, diff --git a/tests/ovn.at b/tests/ovn.at index a21dbadac..7be494644 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -26562,7 +26562,7 @@ AT_CLEANUP ]) # Tests the efficiency of conjunction flow generation when port groups are used -# by ACLs. Make sure there is no open flow explosion in table 45 for an ACL +# by ACLs. Make sure there is no open flow explosion in table 44 for an ACL # with self-referencing match condition of a port group: # # "outport == @pg1 && ip4.src == $pg1_ip4" @@ -26648,6 +26648,32 @@ ovs-vsctl add-port br-int lsp1-1 -- set interface lsp1-1 external_ids:iface-id=l check ovn-nbctl --wait=hv sync AT_CHECK([test $(ovs-ofctl dump-flows br-int table=44 | grep conjunction | wc -l) == 24]) +# 6. Simulate a SB port-group "del and add" notification to ovn-controller in the +# same IDL iteration. ovn-controller should still program the same flows. In +# reality it can happen when PG memembership change triggers a SB port-group +# deletion and creation with the same SB port-group name, while the +# notification of the changes can come to ovn-controller in one shot and the +# order of the "del" and "add" in the notification is undefined. This test +# runs the scenario ten times to make sure the unpleasant order is tested. + +for i in $(seq 1 10); do + # Delete and recreate the SB PG with same name and content. + sb_pg_name=2_pg1 # dp key + NB pg name + sb_pg_uuid=$(fetch_column port_group _uuid name=$sb_pg_name) + ports_=$(fetch_column port_group ports name=2_pg1) + ports=${ports_/ /,} + AT_CHECK([ovn-sbctl destroy port_group $sb_pg_uuid -- create port_group name=$sb_pg_name ports=$ports], [0], [ignore]) + + # Unbind and bind lsp0-0 to trigger runtime data change as well. + ovs-vsctl del-port br-int lsp0-0 + check ovn-nbctl --wait=hv sync + ovs-vsctl add-port br-int lsp0-0 -- set interface lsp0-0 external_ids:iface-id=lsp0-0 + check ovn-nbctl --wait=hv sync + + # Finally check flow count is the same as before. + AT_CHECK([test $(ovs-ofctl dump-flows br-int table=44 | grep conjunction | wc -l) == 24]) +done + # Make sure all the above was performed with I-P (no recompute) AT_CHECK([test $(ovn-appctl -t ovn-controller coverage/read-counter lflow_run) == $lflow_run])