From patchwork Wed Jan 4 08:32:08 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Xavier Simonart X-Patchwork-Id: 1721262 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.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=gYhD7n/D; dkim-atps=neutral Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.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 4Nn2sK4Tflz23fc for ; Wed, 4 Jan 2023 19:32:21 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id F1FFA81F44; Wed, 4 Jan 2023 08:32:18 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org F1FFA81F44 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=gYhD7n/D 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 8OVf7p5I_z0Q; Wed, 4 Jan 2023 08:32:17 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp1.osuosl.org (Postfix) with ESMTPS id 20F6D81F21; Wed, 4 Jan 2023 08:32:16 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 20F6D81F21 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id EC373C0032; Wed, 4 Jan 2023 08:32:15 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp3.osuosl.org (smtp3.osuosl.org [IPv6:2605:bc80:3010::136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 95195C002D for ; Wed, 4 Jan 2023 08:32:14 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 6FA9861014 for ; Wed, 4 Jan 2023 08:32:14 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 6FA9861014 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=gYhD7n/D 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 RMXRKDzNd9Vc for ; Wed, 4 Jan 2023 08:32:13 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org D5CB361012 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 D5CB361012 for ; Wed, 4 Jan 2023 08:32:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1672821131; 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=o6stOD3gnnPT5PigjXWDm6l1llktqbCFgMsasqcurHE=; b=gYhD7n/DXTbO9L11NxQTTYes9kJ9a41l3L9LOyHkxcjyQTWegaglJxsqGH55IkPI4jojKI 5dVQt3iRmYlVeBlI0vumT5JqTL8UAQtYPVR8uwsfvblfWnzv+faNhmPbEh0sj+qcuwdl4m e8DJEHq5cvmOlIpAS6hGQVIm71B0l3U= 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-531-wMAVux8TOMSJ6pjpafDtew-1; Wed, 04 Jan 2023 03:32:10 -0500 X-MC-Unique: wMAVux8TOMSJ6pjpafDtew-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0F96A38123B0 for ; Wed, 4 Jan 2023 08:32:10 +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 DEEC0140EBF5; Wed, 4 Jan 2023 08:32:09 +0000 (UTC) From: Xavier Simonart To: xsimonar@redhat.com, dev@openvswitch.org Date: Wed, 4 Jan 2023 03:32:08 -0500 Message-Id: <20230104083209.3880669-1-xsimonar@redhat.com> In-Reply-To: <20220627085202.1187277-1-xsimonar@redhat.com> References: <20220627085202.1187277-1-xsimonar@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: dceara@redhat.com Subject: [ovs-dev] [PATCH ovn v2 1/2] ovn-controller: fixed ovn-installed not always properly added or removed. 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" OVN checks whether ovn-installed is already present (in OVS) before updating it. This might cause ovn-installed related issues in the following case: - (1) ovn-installed is present - (2) we claim the interface - (3) we update ovs, removing ovn-installed and start installing flows - (4) (next loop), after flows installed, we check if ovn-installed is absent, and if yes, we update OVS with ovn-installed. However, in step4, if OVS is still busy from step 3, ovn-installed is read as present; hence OVN controller does not update it and move to INSTALLED state. ovn-installed was also not properly deleted on port or binding removal. Note that port->up is not properly removed on external_ids:iface-id removal when sb is read-only. This will be fixed in a further patch. Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB Port_Binding updates.") Signed-off-by: Xavier Simonart v2: - handled Dumitru's comments - rebased on main - updated state machine diagram as commented in v1 commit message - remove ovn-installed on port deletion or external_ids:iface-id removal. - added test case --- controller/binding.c | 59 +++++++++- controller/binding.h | 6 + controller/if-status.c | 113 +++++++++++++----- tests/ovn.at | 257 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 403 insertions(+), 32 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 5df62baef..d85a17730 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -746,6 +746,19 @@ local_binding_get_lport_ofport(const struct shash *local_bindings, u16_to_ofp(lbinding->iface->ofport[0]) : 0; } +bool +local_binding_is_ovn_installed(struct shash *local_bindings, + const char *pb_name) +{ + struct local_binding *lbinding = + local_binding_find(local_bindings, pb_name); + if (lbinding && lbinding->iface) { + return smap_get_bool(&lbinding->iface->external_ids, + OVN_INSTALLED_EXT_ID, false); + } + return false; +} + bool local_binding_is_up(struct shash *local_bindings, const char *pb_name, const struct sbrec_chassis *chassis_rec) @@ -783,6 +796,7 @@ local_binding_is_down(struct shash *local_bindings, const char *pb_name, } else if (b_lport->pb->chassis) { VLOG_DBG("lport %s already claimed by other chassis", b_lport->pb->logical_port); + return true; } } @@ -834,6 +848,33 @@ local_binding_set_up(struct shash *local_bindings, const char *pb_name, } } +static void +remove_ovn_installed(struct local_binding *lbinding, const char *pb_name) +{ + if (lbinding && lbinding->iface && + smap_get_bool(&lbinding->iface->external_ids, + OVN_INSTALLED_EXT_ID, false)) { + /* If iface has been deleted, do not try to delete a key from it */ + if (!ovsrec_interface_is_deleted(lbinding->iface)) { + VLOG_INFO("Removing lport %s ovn-installed in OVS", pb_name); + ovsrec_interface_update_external_ids_delkey(lbinding->iface, + OVN_INSTALLED_EXT_ID); + } + } +} + +void +local_binding_remove_ovn_installed(struct shash *local_bindings, + const char *pb_name, bool ovs_readonly) +{ + if (ovs_readonly) { + return; + } + struct local_binding *lbinding = + local_binding_find(local_bindings, pb_name); + remove_ovn_installed(lbinding, pb_name); +} + void local_binding_set_down(struct shash *local_bindings, const char *pb_name, const struct sbrec_chassis *chassis_rec, @@ -1239,7 +1280,9 @@ claim_lport(const struct sbrec_port_binding *pb, return false; } } else { - if (pb->n_up && !pb->up[0]) { + if ((pb->n_up && !pb->up[0]) || + !smap_get_bool(&iface_rec->external_ids, + OVN_INSTALLED_EXT_ID, false)) { if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, sb_readonly); } @@ -1464,9 +1507,11 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, const char *requested_chassis_option = smap_get( &pb->options, "requested-chassis"); VLOG_INFO_RL(&rl, - "Not claiming lport %s, chassis %s requested-chassis %s", + "Not claiming lport %s, chassis %s requested-chassis %s " + "pb->chassis %s", pb->logical_port, b_ctx_in->chassis_rec->name, - requested_chassis_option ? requested_chassis_option : "[]"); + requested_chassis_option ? requested_chassis_option : "[]", + pb->chassis ? pb->chassis->name: ""); } } @@ -2288,6 +2333,9 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, return false; } } + if (b_ctx_in->ovs_idl_txn) { + remove_ovn_installed(lbinding, iface_id); + } } else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT) { /* lbinding is associated with a localport. Remove it from the @@ -2558,6 +2606,7 @@ handle_deleted_lport(const struct sbrec_port_binding *pb, if (ld) { remove_pb_from_local_datapath(pb, b_ctx_out, ld); + if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port); return; } @@ -2581,6 +2630,7 @@ handle_deleted_lport(const struct sbrec_port_binding *pb, remove_pb_from_local_datapath(pb, b_ctx_out, ld); } + if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port); } } @@ -2627,6 +2677,9 @@ handle_deleted_vif_lport(const struct sbrec_port_binding *pb, } handle_deleted_lport(pb, b_ctx_in, b_ctx_out); + if (b_ctx_in->ovs_idl_txn) { + remove_ovn_installed(lbinding, pb->logical_port); + } return true; } diff --git a/controller/binding.h b/controller/binding.h index 6c3a98b02..72b4a35b6 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -159,6 +159,12 @@ bool local_binding_is_up(struct shash *local_bindings, const char *pb_name, bool local_binding_is_down(struct shash *local_bindings, const char *pb_name, const struct sbrec_chassis *); +bool local_binding_is_ovn_installed(struct shash *local_bindings, + const char *pb_name); +void local_binding_remove_ovn_installed(struct shash *local_bindings, + const char *pb_name, + bool ovs_readonly); + void local_binding_set_up(struct shash *local_bindings, const char *pb_name, const struct sbrec_chassis *chassis_rec, const char *ts_now_str, bool sb_readonly, diff --git a/controller/if-status.c b/controller/if-status.c index d1c14ac30..c008aa79e 100644 --- a/controller/if-status.c +++ b/controller/if-status.c @@ -54,32 +54,37 @@ VLOG_DEFINE_THIS_MODULE(if_status); */ enum if_state { - OIF_CLAIMED, /* Newly claimed interface. pb->chassis update not yet - initiated. */ - OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent to - * SB (but update notification not confirmed, so the - * update may be resent in any of the following states) - * and for which flows are still being installed. - */ - OIF_MARK_UP, /* Interface with flows successfully installed in OVS - * but not yet marked "up" in the binding module (in - * SB and OVS databases). - */ - OIF_MARK_DOWN, /* Released interface but not yet marked "down" in the - * binding module (in SB and/or OVS databases). - */ - OIF_INSTALLED, /* Interface flows programmed in OVS and binding marked - * "up" in the binding module. - */ + OIF_CLAIMED, /* Newly claimed interface. pb->chassis update not yet + initiated. */ + OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent to + * SB (but update notification not confirmed, so the + * update may be resent in any of the following + * states and for which flows are still being + * installed. + */ + OIF_REMOVE_OVN_INST, /* Interface with flows successfully installed in OVS, + * but with ovn-installed still in OVSDB. + */ + OIF_MARK_UP, /* Interface with flows successfully installed in OVS + * but not yet marked "up" in the binding module (in + * SB and OVS databases). + */ + OIF_MARK_DOWN, /* Released interface but not yet marked "down" in the + * binding module (in SB and/or OVS databases). + */ + OIF_INSTALLED, /* Interface flows programmed in OVS and binding + * marked "up" in the binding module. + */ OIF_MAX, }; static const char *if_state_names[] = { - [OIF_CLAIMED] = "CLAIMED", - [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS", - [OIF_MARK_UP] = "MARK_UP", - [OIF_MARK_DOWN] = "MARK_DOWN", - [OIF_INSTALLED] = "INSTALLED", + [OIF_CLAIMED] = "CLAIMED", + [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS", + [OIF_REMOVE_OVN_INST] = "REMOVE_OVN_INST", + [OIF_MARK_UP] = "MARK_UP", + [OIF_MARK_DOWN] = "MARK_DOWN", + [OIF_INSTALLED] = "INSTALLED", }; /* @@ -109,11 +114,26 @@ static const char *if_state_names[] = { * | | | - remove ovn-installed from ovsdb | | | * | | | mgr_update() | | | * | +----------------------+ - sbrec_update_chassis if needed | | | - * | | | | | - * | | mgr_run(seqno rcvd) | | | - * | | - set port up in sb | | | - * | release_iface | - set ovn-installed in ovs | | | - * | V | | | + * | | | | | | + * | | +----------------------------------------+ | | | + * | | | | | | + * | | mgr_run(seqno rcvd, ovn-installed present) | | | | + * | V | | | | + * | +--------------------+ | | | | + * | | | mgr_run() | | | | + * +--- | REMOVE_OVN_INST | - remove ovn-installed in ovs | | | | + * | +--------------------+ | | | | + * | | | | | | + * | | | | | | + * | | mgr_update( ovn_installed not present) | | | | + * | | | | | | + * | | +-------------------------------------------+ | | | + * | | | | | | + * | | | mgr_run(seqno rcvd, ovn-installed not present) | | | + * | | | - set port up in sb | | | + * | | | - set ovn-installed in ovs | | | + * |release_iface | | | | | + * | V V | | | * | +----------------------+ | | | * | | | mgr_run() | | | * +-- | MARK_UP | - set port up in sb | | | @@ -238,6 +258,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, switch (iface->state) { case OIF_CLAIMED: case OIF_INSTALL_FLOWS: + case OIF_REMOVE_OVN_INST: case OIF_MARK_UP: /* Nothing to do here. */ break; @@ -274,6 +295,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id) /* Not yet fully installed interfaces can be safely deleted. */ ovs_iface_destroy(mgr, iface); break; + case OIF_REMOVE_OVN_INST: case OIF_MARK_UP: case OIF_INSTALLED: /* Properly mark interfaces "down" if their flows were already @@ -305,6 +327,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) /* Not yet fully installed interfaces can be safely deleted. */ ovs_iface_destroy(mgr, iface); break; + case OIF_REMOVE_OVN_INST: case OIF_MARK_UP: case OIF_INSTALLED: /* Properly mark interfaces "down" if their flows were already @@ -359,6 +382,17 @@ if_status_mgr_update(struct if_status_mgr *mgr, struct shash *bindings = &binding_data->bindings; struct hmapx_node *node; + /* Move all interfaces that have been confirmed without ovn-installed, + * from OIF_REMOVE_OVN_INST to OIF_MARK_UP. + */ + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_REMOVE_OVN_INST]) { + struct ovs_iface *iface = node->data; + + if (!local_binding_is_ovn_installed(bindings, iface->id)) { + ovs_iface_set_state(mgr, iface, OIF_MARK_UP); + } + } + /* Interfaces in OIF_MARK_UP/INSTALL_FLOWS state have already set their * pb->chassis. However, the update might still be in fly (confirmation * not received yet) or pb->chassis was overwitten by another chassis. @@ -471,7 +505,19 @@ if_status_mgr_run(struct if_status_mgr *mgr, iface->install_seqno)) { continue; } - ovs_iface_set_state(mgr, iface, OIF_MARK_UP); + /* Wait for ovn-installed to be absent before moving to MARK_UP state. + * Most of the times ovn-installed is already absent and hence we will + * not have to wait. + * If there is no binding_data, we can't determine if ovn-installed is + * present or not; hence also go to the OIF_REMOVE_OVN_INST state. + */ + if (!binding_data || + local_binding_is_ovn_installed(&binding_data->bindings, + iface->id)) { + ovs_iface_set_state(mgr, iface, OIF_REMOVE_OVN_INST); + } else { + ovs_iface_set_state(mgr, iface, OIF_MARK_UP); + } } ofctrl_acked_seqnos_destroy(acked_seqnos); @@ -558,7 +604,16 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, sb_readonly, ovs_readonly); } - /* Notifiy the binding module to set "up" all bindings that have had + /* Notify the binding module to remove "ovn-installed" for all bindings + * in the OIF_REMOVE_OVN_INST state. + */ + HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_REMOVE_OVN_INST]) { + struct ovs_iface *iface = node->data; + + local_binding_remove_ovn_installed(bindings, iface->id, ovs_readonly); + } + + /* Notify the binding module to set "up" all bindings that have had * their flows installed but are not yet marked "up" in the binding * module. */ diff --git a/tests/ovn.at b/tests/ovn.at index d2163d87d..a0378a728 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -34248,3 +34248,260 @@ AT_CHECK([ovs-ofctl dump-flows br-int | grep table=68 | ofctl_strip_all], [0], [ OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +# This tests port->up/down and ovn-installed after adding and removing Ports and Interfaces. +# 3 Conditions x 3 tests: +# - 3 Conditions: +# - In normal conditions +# - Remove interface while starting and stopping SB and Controller +# - Remove and add back interface while starting and stopping SB and Controller +# - 3 tests: +# - Add/Remove Logical Port +# - Add/Remove iface-id +# - Add/Remove Interface +# Each tests/conditions checks for +# - Port_binding->chassis +# - Port up or down +# - ovn-installed +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-installed and ports up or down]) +ovn_start + +net_add n1 +sim_add hv1 +as hv1 +check ovs-vsctl add-br br-phys +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys +ovn_attach n1 br-phys 192.168.0.1 24 geneve + +check ovn-nbctl ls-add ls1 +check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16 + +check ovn-nbctl --wait=hv sync + +add_logical_ports() { + echo Adding logical ports + check ovn-nbctl lsp-add ls1 lsp1 + check ovn-nbctl lsp-add ls1 lsp2 +} + +remove_logical_ports() { + echo Removing logical ports + check ovn-nbctl lsp-del lsp1 + check ovn-nbctl lsp-del lsp2 +} + +add_ovs_interfaces() { + echo Adding interfaces + ovs-vsctl --no-wait -- add-port br-int vif1 \ + -- set Interface vif1 external_ids:iface-id=lsp1 + ovs-vsctl --no-wait -- add-port br-int vif2 \ + -- set Interface vif2 external_ids:iface-id=lsp2 +} +remove_ovs_interfaces() { + echo Removing interfaces + check ovs-vsctl --no-wait -- del-port vif1 + check ovs-vsctl --no-wait -- del-port vif2 +} +add_iface_ids() { + echo Adding back iface-id + ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1 + ovs-vsctl --no-wait -- set Interface vif2 external_ids:iface-id=lsp2 +} +remove_iface_ids() { + echo Removing iface-id + check ovs-vsctl remove Interface vif1 external_ids iface-id + check ovs-vsctl remove Interface vif2 external_ids iface-id +} +wait_for_local_bindings() { + OVS_WAIT_UNTIL( + [test `ovs-appctl -t ovn-controller debug/dump-local-bindings | grep interface | wc -l` -eq 2], + [kill -CONT $(cat ovn-sb/ovsdb-server.pid)] + ) +} +sleep_sb() { + echo SB going to sleep + AT_CHECK([kill -STOP $(cat ovn-sb/ovsdb-server.pid)]) +} +wake_up_sb() { + echo SB waking up + AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)]) +} +sleep_controller() { + echo Controller going to sleep + ovn-appctl debug/pause + OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) = "xpaused"]) +} +wake_up_controller() { + echo Controller waking up + ovn-appctl debug/resume +} +ensure_controller_run() { +# We want to make sure controller could run at least one full loop. +# We can't use wait=hv as sb might be sleeping. +# Use 2 ovn-appctl to guarentee that ovn-controller run the full loop, and not just the unixctl handling + OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) = "xrunning"]) + OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) = "xrunning"]) +} +sleep_ovsdb() { + echo OVSDB going to sleep + AT_CHECK([kill -STOP $(cat hv1/ovsdb-server.pid)]) +} +wake_up_ovsdb() { + echo OVSDB waking up + AT_CHECK([kill -CONT $(cat hv1/ovsdb-server.pid)]) +} +check_ovn_installed() { + OVS_WAIT_UNTIL([test `as hv1 ovs-vsctl get Interface vif1 external_ids:ovn-installed` = '"true"']) + OVS_WAIT_UNTIL([test `as hv1 ovs-vsctl get Interface vif2 external_ids:ovn-installed` = '"true"']) +} +check_ovn_uninstalled() { + OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface vif1 external_ids:ovn-installed` = x]) + OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface vif2 external_ids:ovn-installed` = x]) +} +check_ports_up() { + OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'true']) + OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'true']) +} +check_ports_down() { + OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'false']) + OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'false']) +} + +check_ports_bound() { + ch=$(fetch_column Chassis _uuid name=hv1) + wait_row_count Port_Binding 1 logical_port=lsp1 chassis=$ch + wait_row_count Port_Binding 1 logical_port=lsp2 chassis=$ch +} +check_ports_unbound() { + wait_column "" Port_Binding chassis logical_port=lsp1 + wait_column "" Port_Binding chassis logical_port=lsp2 +} +add_logical_ports +add_ovs_interfaces +wait_for_local_bindings +wait_for_ports_up +check ovn-nbctl --wait=hv sync +############################################################ +################### Add/Remove iface-id #################### +############################################################ +AS_BOX(["iface-id removal and added back (no sleeping sb or controller)"]) +remove_iface_ids +check_ovn_uninstalled +check_ports_down +check_ports_unbound +add_iface_ids +check_ovn_installed +check_ports_up +check_ports_bound + +AS_BOX(["iface-id removal"]) +sleep_sb +remove_iface_ids +ensure_controller_run +sleep_controller +wake_up_sb +wake_up_controller +check_ovn_uninstalled +# Port_down not always set on iface-id removal +# check_ports_down +# Port_Binding(chassis) not always removed on iface-id removal +# check_ports_unbound +add_iface_ids +check ovn-nbctl --wait=hv sync + +AS_BOX(["iface-id removal and added back"]) +sleep_sb +remove_iface_ids +ensure_controller_run +sleep_controller +add_iface_ids +wake_up_sb +wake_up_controller +check_ovn_installed +check_ports_up +check_ports_bound +############################################################ +###################### Add/Remove Interface ################ +############################################################ +AS_BOX(["Interface removal and added back (no sleeping sb or controller)"]) +remove_ovs_interfaces +check_ovn_uninstalled +check_ports_down +check_ports_unbound +add_ovs_interfaces +check_ovn_installed +check_ports_up +check_ports_bound +check ovn-nbctl --wait=hv sync + +AS_BOX(["Interface removal"]) +sleep_sb +remove_ovs_interfaces +ensure_controller_run +sleep_controller +wake_up_sb +wake_up_controller +check_ovn_uninstalled +# Port_down not always set on Interface removal +# check_ports_down +# Port_Binding(chassis) not always removed on Interface removal +# check_ports_unbound +add_ovs_interfaces +check ovn-nbctl --wait=hv sync + +AS_BOX(["Interface removal and added back"]) +sleep_sb +remove_ovs_interfaces +ensure_controller_run +sleep_controller +add_ovs_interfaces +wake_up_sb +wake_up_controller +check_ovn_installed +check_ports_up +check_ports_bound +check ovn-nbctl --wait=hv sync +############################################################ +###################### Add/Remove Logical Port ############# +############################################################ +AS_BOX(["Logical port removal and added back (no sleeping sb or controller)"]) +remove_logical_ports +check_ovn_uninstalled +check_ports_unbound +sleep_ovsdb +add_logical_ports +ensure_controller_run +wake_up_ovsdb +check_ovn_installed +check_ports_up +check_ports_bound +check ovn-nbctl --wait=hv sync + +AS_BOX(["Logical port removal"]) +sleep_sb +remove_logical_ports +ensure_controller_run +sleep_controller +wake_up_sb +wake_up_controller +check_ovn_uninstalled +check_ports_unbound +add_logical_ports +check ovn-nbctl --wait=hv sync + +AS_BOX(["Logical port removal and added back"]) +sleep_sb +remove_logical_ports +ensure_controller_run +sleep_controller +add_logical_ports +wake_up_sb +wake_up_controller +check_ovn_installed +check_ports_up +check_ports_bound +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) + From patchwork Wed Jan 4 08:32:09 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Xavier Simonart X-Patchwork-Id: 1721263 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=Ivbn4AmF; 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 4Nn2sP46Bsz23fc for ; Wed, 4 Jan 2023 19:32:25 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 18A0B61022; Wed, 4 Jan 2023 08:32:23 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 18A0B61022 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=Ivbn4AmF 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 hutfAe3eKNs6; Wed, 4 Jan 2023 08:32:20 +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 A307161012; Wed, 4 Jan 2023 08:32:18 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org A307161012 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8CB3CC007F; Wed, 4 Jan 2023 08:32:16 +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 DE7F9C002D for ; Wed, 4 Jan 2023 08:32:14 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id ACC9F40A95 for ; Wed, 4 Jan 2023 08:32:14 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org ACC9F40A95 Authentication-Results: smtp2.osuosl.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Ivbn4AmF 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 zo6NuP6HCRFS for ; Wed, 4 Jan 2023 08:32:13 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 80AAE400EC Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by smtp2.osuosl.org (Postfix) with ESMTPS id 80AAE400EC for ; Wed, 4 Jan 2023 08:32:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1672821132; 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=3GpDdXzSW4CzUSDQHQ+6VCcyY/sup9FrqBHZmNxP1BE=; b=Ivbn4AmFgXHgw5TPBHD0oX1VzxA3VUKKSvdXRHGIWsMJimudEr8qixbH5KZtmdfRVwU3t5 0UGDbi3m7LCLg3o5cr+PTpVNBFTKaoIe5znJ3KOOxWpx1nPToaQBOtvEk480Cu71iGhiQi UeGPqiBEguDjTSaLaSz9rpcEnxyQCzM= 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-641-tZLUL-uPOp-JZb0ShYi4qA-1; Wed, 04 Jan 2023 03:32:11 -0500 X-MC-Unique: tZLUL-uPOp-JZb0ShYi4qA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B3B332805582 for ; Wed, 4 Jan 2023 08:32:10 +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 97C10140EBF5; Wed, 4 Jan 2023 08:32:10 +0000 (UTC) From: Xavier Simonart To: xsimonar@redhat.com, dev@openvswitch.org Date: Wed, 4 Jan 2023 03:32:09 -0500 Message-Id: <20230104083209.3880669-2-xsimonar@redhat.com> In-Reply-To: <20230104083209.3880669-1-xsimonar@redhat.com> References: <20220627085202.1187277-1-xsimonar@redhat.com> <20230104083209.3880669-1-xsimonar@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: dceara@redhat.com Subject: [ovs-dev] [PATCH ovn v2 2/2] ovn-controller: fixed port not always set down when unbinding interface 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 interface was unbound, the port was not always set down and the port_binding->chassis not always removed. Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB Port_Binding updates.") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2150905 Signed-off-by: Xavier Simonart --- controller/binding.c | 2 +- controller/if-status.c | 44 +++++++++++++++++++++++++++++++++++++ controller/if-status.h | 4 ++++ controller/ovn-controller.c | 12 ++++++++++ tests/ovn.at | 12 ++++------ 5 files changed, 65 insertions(+), 9 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index d85a17730..eb8d580c8 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -894,7 +894,6 @@ local_binding_set_down(struct shash *local_bindings, const char *pb_name, if (!sb_readonly && b_lport && b_lport->pb->n_up && b_lport->pb->up[0] && (!b_lport->pb->chassis || b_lport->pb->chassis == chassis_rec)) { - VLOG_INFO("Setting lport %s down in Southbound", pb_name); binding_lport_set_down(b_lport, sb_readonly); LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { binding_lport_set_down(b_lport, sb_readonly); @@ -3384,6 +3383,7 @@ binding_lport_set_down(struct binding_lport *b_lport, bool sb_readonly) if (sb_readonly || !b_lport || !b_lport->pb->n_up || !b_lport->pb->up[0]) { return; } + VLOG_INFO("Setting lport %s down in Southbound", b_lport->name); bool up = false; sbrec_port_binding_set_up(b_lport->pb, &up, 1); diff --git a/controller/if-status.c b/controller/if-status.c index c008aa79e..26535c9e8 100644 --- a/controller/if-status.c +++ b/controller/if-status.c @@ -16,6 +16,7 @@ #include #include "binding.h" +#include "lport.h" #include "if-status.h" #include "ofctrl-seqno.h" #include "simap.h" @@ -75,6 +76,9 @@ enum if_state { OIF_INSTALLED, /* Interface flows programmed in OVS and binding * marked "up" in the binding module. */ + OIF_UPDATE_PORT, /* Logical ports need to be set down, and pb->chassis + * removed. + */ OIF_MAX, }; @@ -85,6 +89,7 @@ static const char *if_state_names[] = { [OIF_MARK_UP] = "MARK_UP", [OIF_MARK_DOWN] = "MARK_DOWN", [OIF_INSTALLED] = "INSTALLED", + [OIF_UPDATE_PORT] = "UPDATE_PORT", }; /* @@ -264,6 +269,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, break; case OIF_INSTALLED: case OIF_MARK_DOWN: + case OIF_UPDATE_PORT: ovs_iface_set_state(mgr, iface, OIF_CLAIMED); break; case OIF_MAX: @@ -304,6 +310,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id) ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); break; case OIF_MARK_DOWN: + case OIF_UPDATE_PORT: /* Nothing to do here. */ break; case OIF_MAX: @@ -336,6 +343,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); break; case OIF_MARK_DOWN: + case OIF_UPDATE_PORT: /* Nothing to do here. */ break; case OIF_MAX: @@ -344,6 +352,36 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) } } +void +if_status_handle_lports(struct if_status_mgr *mgr, + const struct sbrec_chassis *chassis_rec, + struct ovsdb_idl_index *sbrec_port_binding_by_name, + bool sb_readonly) +{ + if (sb_readonly) { + return; + } + + struct hmapx_node *node; + + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_UPDATE_PORT]) { + struct ovs_iface *iface = node->data; + const struct sbrec_port_binding *pb = lport_lookup_by_name( + sbrec_port_binding_by_name, iface->id); + + if ((pb == NULL) || sbrec_port_binding_is_deleted(pb)) { + VLOG_DBG("Removing lport %s from list of ports to set down", + iface->id); + } else { + bool up = false; + sbrec_port_binding_set_up(pb, &up, 1); + VLOG_INFO("Setting lport %s down in Southbound", iface->id); + set_pb_chassis_in_sbrec(pb, chassis_rec, false); + } + ovs_iface_destroy(mgr, node->data); + } +} + bool if_status_handle_claims(struct if_status_mgr *mgr, struct local_binding_data *binding_data, @@ -424,6 +462,12 @@ if_status_mgr_update(struct if_status_mgr *mgr, HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) { struct ovs_iface *iface = node->data; + if (!local_binding_find(bindings, iface->id) && sb_readonly) { + VLOG_DBG("%s not found in local_bindings and sb read only => " + "port down delayed", iface->id); + ovs_iface_set_state(mgr, iface, OIF_UPDATE_PORT); + continue; + } if (!sb_readonly) { local_binding_set_pb(bindings, iface->id, chassis_rec, NULL, false); diff --git a/controller/if-status.h b/controller/if-status.h index 5bd187a25..593597af7 100644 --- a/controller/if-status.h +++ b/controller/if-status.h @@ -48,5 +48,9 @@ bool if_status_handle_claims(struct if_status_mgr *mgr, const struct sbrec_chassis *chassis_rec, struct hmap *tracked_datapath, bool sb_readonly); +void if_status_handle_lports(struct if_status_mgr *mgr, + const struct sbrec_chassis *chassis_rec, + struct ovsdb_idl_index *sbrec_port_binding_by_name, + bool sb_readonly); # endif /* controller/if-status.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 814a88117..4094eb56e 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1603,6 +1603,11 @@ runtime_data_sb_ro_handler(struct engine_node *node, void *data) engine_get_input("SB_chassis", node), "name"); + struct ovsdb_idl_index *sbrec_port_binding_by_name = + engine_ovsdb_node_get_index( + engine_get_input("SB_port_binding", node), + "name"); + if (chassis_id) { chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); } @@ -1620,6 +1625,13 @@ runtime_data_sb_ro_handler(struct engine_node *node, void *data) engine_set_node_state(node, EN_UPDATED); rt_data->tracked = true; } + + if (sbrec_port_binding_by_name) { + if_status_handle_lports(ctrl_ctx->if_mgr, + chassis, + sbrec_port_binding_by_name, + sb_readonly); + } } return true; } diff --git a/tests/ovn.at b/tests/ovn.at index a0378a728..a849a483d 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -34403,10 +34403,8 @@ sleep_controller wake_up_sb wake_up_controller check_ovn_uninstalled -# Port_down not always set on iface-id removal -# check_ports_down -# Port_Binding(chassis) not always removed on iface-id removal -# check_ports_unbound +check_ports_down +check_ports_unbound add_iface_ids check ovn-nbctl --wait=hv sync @@ -34443,10 +34441,8 @@ sleep_controller wake_up_sb wake_up_controller check_ovn_uninstalled -# Port_down not always set on Interface removal -# check_ports_down -# Port_Binding(chassis) not always removed on Interface removal -# check_ports_unbound +check_ports_down +check_ports_unbound add_ovs_interfaces check ovn-nbctl --wait=hv sync