From patchwork Mon Oct 30 09:10:21 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Xavier Simonart X-Patchwork-Id: 1856890 X-Patchwork-Delegate: dceara@redhat.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org 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=OGTIPV8t; dkim-atps=neutral Authentication-Results: legolas.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=patchwork.ozlabs.org) 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 ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4SJnYZ2Ydzz1yQW for ; Mon, 30 Oct 2023 20:10:42 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 38011437C9; Mon, 30 Oct 2023 09:10:38 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 38011437C9 Authentication-Results: smtp2.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=OGTIPV8t 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 8cZvHwJIZHCU; Mon, 30 Oct 2023 09:10:35 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp2.osuosl.org (Postfix) with ESMTPS id 31822437AE; Mon, 30 Oct 2023 09:10:34 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 31822437AE Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id B9194C008C; Mon, 30 Oct 2023 09:10:31 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 4CF70C0DD8 for ; Mon, 30 Oct 2023 09:10:29 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 0736884DF3 for ; Mon, 30 Oct 2023 09:10:29 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 0736884DF3 Authentication-Results: smtp1.osuosl.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=OGTIPV8t 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 AD_aCJTWjubR for ; Mon, 30 Oct 2023 09:10:27 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp1.osuosl.org (Postfix) with ESMTPS id 319D184DCA for ; Mon, 30 Oct 2023 09:10:27 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 319D184DCA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1698657026; 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=Ug+PHAVHPb46qZ+HRnq9lege9g5S97phtfJOuQJFtIg=; b=OGTIPV8tP/x2cU1ksO205hByVKEsyyEEXLvH7BWBSzHs+RBHCAWlocMDgYKYg+c5U8WfyU oUMyNASLzMMhGsCvtOhypPZoh0yN2L6PviJWT1BDJJ0PtQ+VEPKbF0yNrFWbAlhNMD22TD jEw6F8yNbe9NXGsIQ8LKo5njmANNmoM= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-180-KI-PPMfQMW-ZPSjHfTXyqw-1; Mon, 30 Oct 2023 05:10:22 -0400 X-MC-Unique: KI-PPMfQMW-ZPSjHfTXyqw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id AD53429AA2D3 for ; Mon, 30 Oct 2023 09:10:22 +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 8B92E40C6EB9; Mon, 30 Oct 2023 09:10:22 +0000 (UTC) From: Xavier Simonart To: xsimonar@redhat.com, dev@openvswitch.org Date: Mon, 30 Oct 2023 10:10:21 +0100 Message-Id: <20231030091022.2397676-3-xsimonar@redhat.com> In-Reply-To: <20231030091022.2397676-1-xsimonar@redhat.com> References: <20230920154557.4010537-1-xsimonar@redhat.com> <20231030091022.2397676-1-xsimonar@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [PATCH ovn v3 2/3] binding: handle pb->chassis and pb->up from if-status module 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" Before this patch, if-status module handled pb->chassis and pb->up for vif ports, but not for non-VIF ports. This caused a few potential issues: - It was difficult to check that a no-vif port has been previously claimed as there was no state in if-status. Hence it was difficult to always release such a port when pb->chassis was set to a different chassis. This issue will be fixed in a future patch. - Releasing such ports caused ovn-controller to log warnings such as "Trying to delete unknown ports". - If sb is readonly at the time of the claim, it forced the module to recompute. This patch fixed issues two and three by moving the work of setting pb->chassis and pb->up to the if-status module. Signed-off-by: Xavier Simonart --- v2: Avoid clearing iface if already deleted v3: - handled Mark's feedback i.e. - clear comment in claimed_lport_set_up - changed notify_up to is_vif and clarify comments - change claimed_lport_set_up to void as was always called with !sb_readonly --- controller/binding.c | 143 ++++++++++++++++++++++++------------ controller/binding.h | 17 +++++ controller/if-status.c | 137 ++++++++++++++++++++++++++-------- controller/if-status.h | 9 ++- controller/ovn-controller.c | 6 +- tests/ovn.at | 94 ++++++++++++++++++++++++ 6 files changed, 325 insertions(+), 81 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 9a09b2074..bd48db621 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1193,33 +1193,22 @@ local_binding_set_pb(struct shash *local_bindings, const char *pb_name, * - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g., for * container and virtual ports). * - * Returns false if lport is not claimed due to 'sb_readonly'. - * Returns true otherwise. - * * Note: * Updates the 'pb->up' field only if it's explicitly set to 'false'. * This is to ensure compatibility with older versions of ovn-northd. */ -static bool +void claimed_lport_set_up(const struct sbrec_port_binding *pb, - const struct sbrec_port_binding *parent_pb, - bool sb_readonly) + const struct sbrec_port_binding *parent_pb) { - /* When notify_up is false in claim_port(), no state is created - * by if_status_mgr. In such cases, return false (i.e. trigger recompute) - * if we can't update sb (because it is readonly). - */ bool up = true; if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) { - if (!sb_readonly) { - if (pb->n_up) { - sbrec_port_binding_set_up(pb, &up, 1); - } - } else if (pb->n_up && !pb->up[0]) { - return false; + if (pb->n_up) { + VLOG_INFO("Setting lport %s up in Southbound", + pb->logical_port); + sbrec_port_binding_set_up(pb, &up, 1); } } - return true; } typedef void (*set_func)(const struct sbrec_port_binding *pb, @@ -1322,7 +1311,7 @@ claim_lport(const struct sbrec_port_binding *pb, const struct sbrec_port_binding *parent_pb, const struct sbrec_chassis *chassis_rec, const struct ovsrec_interface *iface_rec, - bool sb_readonly, bool notify_up, + bool sb_readonly, bool is_vif, struct hmap *tracked_datapaths, struct if_status_mgr *if_mgr, struct sset *postponed_ports) @@ -1347,40 +1336,27 @@ claim_lport(const struct sbrec_port_binding *pb, } update_tracked = true; - if (!notify_up) { - if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) { - return false; - } - if (sb_readonly) { - return false; - } - set_pb_chassis_in_sbrec(pb, chassis_rec, true); - } else { - if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec, - sb_readonly, can_bind); - } + if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec, + sb_readonly, can_bind, is_vif, + parent_pb); register_claim_timestamp(pb->logical_port, now); sset_find_and_delete(postponed_ports, pb->logical_port); } else { update_tracked = true; - if (!notify_up) { - if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) { - return false; - } - } else { - 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, - iface_rec, sb_readonly, - can_bind); - } + if ((pb->n_up && !pb->up[0]) || + (is_vif && !smap_get_bool(&iface_rec->external_ids, + OVN_INSTALLED_EXT_ID, false))) { + if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, + iface_rec, sb_readonly, + can_bind, is_vif, + parent_pb); } } } else if (can_bind == CAN_BIND_AS_ADDITIONAL) { if (!is_additional_chassis(pb, chassis_rec)) { if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec, - sb_readonly, can_bind); + sb_readonly, can_bind, is_vif, + parent_pb); update_tracked = true; } } @@ -2447,14 +2423,14 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, remove_related_lport(b_lport->pb, b_ctx_out); } - /* Clear the iface of the local binding. */ - lbinding->iface = NULL; - /* Check if the lbinding has children of type PB_CONTAINER. * If so, don't delete the local_binding. */ if (!is_lbinding_container_parent(lbinding)) { local_binding_delete(lbinding, local_bindings, binding_lports, b_ctx_out->if_mgr); + } else { + /* Clear the iface of the local binding. */ + lbinding->iface = NULL; } } @@ -3248,7 +3224,7 @@ local_binding_delete(struct local_binding *lbinding, struct if_status_mgr *if_mgr) { shash_find_and_delete(local_bindings, lbinding->name); - if_status_mgr_delete_iface(if_mgr, lbinding->name); + if_status_mgr_delete_iface(if_mgr, lbinding->name, lbinding->iface); local_binding_destroy(lbinding, binding_lports); } @@ -3465,6 +3441,79 @@ port_binding_set_down(const struct sbrec_chassis *chassis_rec, } } +void +port_binding_set_up(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const char *iface_id, + const struct uuid *pb_uuid) +{ + const struct sbrec_port_binding *pb = + sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid); + if (!pb) { + VLOG_DBG("port_binding already deleted for %s", iface_id); + return; + } + if (pb->n_up && !pb->up[0]) { + bool up = true; + sbrec_port_binding_set_up(pb, &up, 1); + VLOG_INFO("Setting lport %s up in Southbound", pb->logical_port); + set_pb_chassis_in_sbrec(pb, chassis_rec, true); + } +} + +void +port_binding_set_pb(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const char *iface_id, + const struct uuid *pb_uuid, + enum can_bind bind_type) +{ + const struct sbrec_port_binding *pb = + sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid); + if (!pb) { + VLOG_DBG("port_binding already deleted for %s", iface_id); + return; + } + if (bind_type == CAN_BIND_AS_MAIN) { + set_pb_chassis_in_sbrec(pb, chassis_rec, true); + } else if (bind_type == CAN_BIND_AS_ADDITIONAL) { + set_pb_additional_chassis_in_sbrec(pb, chassis_rec, true); + } +} + +bool +port_binding_pb_chassis_is_set(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const struct uuid *pb_uuid) +{ + const struct sbrec_port_binding *pb = + sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid); + + if (pb && ((pb->chassis == chassis_rec) || + is_additional_chassis(pb, chassis_rec))) { + return true; + } + return false; +} + +bool +port_binding_is_up(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const struct uuid *pb_uuid) +{ + const struct sbrec_port_binding *pb = + sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid); + + if (!pb || pb->chassis != chassis_rec) { + return false; + } + + if (pb->n_up && !pb->up[0]) { + return false; + } + return true; +} + static void binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly) { diff --git a/controller/binding.h b/controller/binding.h index d10eeec1f..9a1f9f923 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -217,6 +217,18 @@ void port_binding_set_down(const struct sbrec_chassis *chassis_rec, const struct sbrec_port_binding_table *pb_table, const char *iface_id, const struct uuid *pb_uuid); +void port_binding_set_up(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const char *iface_id, + const struct uuid *pb_uuid); +void port_binding_set_pb(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const char *iface_id, + const struct uuid *pb_uuid, + enum can_bind bind_type); +bool port_binding_pb_chassis_is_set(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const struct uuid *pb_uuid); /* This structure represents a logical port (or port binding) * which is associated with 'struct local_binding'. @@ -261,4 +273,9 @@ void update_qos(struct ovsdb_idl_index * sbrec_port_binding_by_name, bool lport_maybe_postpone(const char *port_name, long long int now, struct sset *postponed_ports); +void claimed_lport_set_up(const struct sbrec_port_binding *pb, + const struct sbrec_port_binding *parent_pb); +bool port_binding_is_up(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const struct uuid *pb_uuid); #endif /* controller/binding.h */ diff --git a/controller/if-status.c b/controller/if-status.c index 6f14549c8..a6bfdebe8 100644 --- a/controller/if-status.c +++ b/controller/if-status.c @@ -177,7 +177,9 @@ static const char *if_state_names[] = { struct ovs_iface { char *id; /* Extracted from OVS external_ids.iface_id. */ + char *name; /* OVS iface name. */ struct uuid pb_uuid; /* Port_binding uuid */ + struct uuid parent_pb_uuid; /* Port_binding uuid */ enum if_state state; /* State of the interface in the state machine. */ uint32_t install_seqno; /* Seqno at which this interface is expected to * be fully programmed in OVS. Only used in state @@ -185,6 +187,7 @@ struct ovs_iface { */ uint16_t mtu; /* Extracted from OVS interface.mtu field. */ enum can_bind bind_type;/* CAN_BIND_AS_MAIN or CAN_BIND_AS_ADDITIONAL */ + bool is_vif; /* Vifs, container or virtual ports */ }; static uint64_t ifaces_usage; @@ -224,6 +227,7 @@ static void if_status_mgr_update_bindings( struct if_status_mgr *mgr, struct local_binding_data *binding_data, const struct sbrec_chassis *, const struct ovsrec_interface_table *iface_table, + const struct sbrec_port_binding_table *pb_table, bool sb_readonly, bool ovs_readonly); static void ovn_uninstall_hash_account_mem(const char *name, bool erase); @@ -273,12 +277,23 @@ if_status_mgr_destroy(struct if_status_mgr *mgr) free(mgr); } +/* is_vif controls whether we wait for flows to be updated + * before setting the interface up. It is true for VIF, CONTAINER + * and VIRTUAL ports. + * Non-VIF ports are reported up as soon as they are claimed + * to maintain compatibility with older versions. + * See aae25e6 binding: Correctly set Port_Binding.up for container/virtual + * ports. + */ + void if_status_mgr_claim_iface(struct if_status_mgr *mgr, const struct sbrec_port_binding *pb, const struct sbrec_chassis *chassis_rec, const struct ovsrec_interface *iface_rec, - bool sb_readonly, enum can_bind bind_type) + bool sb_readonly, enum can_bind bind_type, + bool is_vif, + const struct sbrec_port_binding *parent_pb) { const char *iface_id = pb->logical_port; struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id); @@ -287,8 +302,13 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, iface = ovs_iface_create(mgr, iface_id, iface_rec, OIF_CLAIMED); } iface->bind_type = bind_type; + iface->is_vif = is_vif; memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid)); + if (parent_pb) { + memcpy(&iface->parent_pb_uuid, &parent_pb->header_.uuid, + sizeof(iface->pb_uuid)); + } if (!sb_readonly) { if (bind_type == CAN_BIND_AS_MAIN) { set_pb_chassis_in_sbrec(pb, chassis_rec, true); @@ -357,7 +377,8 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id) } void -if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) +if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id, + const struct ovsrec_interface *iface_rec) { struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id); @@ -365,6 +386,12 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) return; } + if (iface_rec && strcmp(iface->name, iface_rec->name)) { + VLOG_DBG("Interface %s not deleted as port %s bound to %s", + iface_rec->name, iface_id, iface->name); + return; + } + switch (iface->state) { case OIF_CLAIMED: case OIF_INSTALL_FLOWS: @@ -394,6 +421,7 @@ if_status_handle_claims(struct if_status_mgr *mgr, struct local_binding_data *binding_data, const struct sbrec_chassis *chassis_rec, struct hmap *tracked_datapath, + const struct sbrec_port_binding_table *pb_table, bool sb_readonly) { if (!binding_data || sb_readonly) { @@ -406,9 +434,15 @@ if_status_handle_claims(struct if_status_mgr *mgr, bool rc = false; HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_CLAIMED]) { struct ovs_iface *iface = node->data; - VLOG_INFO("if_status_handle_claims for %s", iface->id); - local_binding_set_pb(bindings, iface->id, chassis_rec, - tracked_datapath, true, iface->bind_type); + VLOG_DBG("if_status_handle_claims for %s, is_vif = %d", iface->id, + iface->is_vif); + if (iface->is_vif) { + local_binding_set_pb(bindings, iface->id, chassis_rec, + tracked_datapath, true, iface->bind_type); + } else { + port_binding_set_pb(chassis_rec, pb_table, iface->id, + &iface->pb_uuid, iface->bind_type); + } rc = true; } return rc; @@ -470,23 +504,37 @@ if_status_mgr_update(struct if_status_mgr *mgr, */ HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_UP]) { struct ovs_iface *iface = node->data; - - if (!local_bindings_pb_chassis_is_set(bindings, iface->id, - chassis_rec)) { - if (!sb_readonly) { - long long int now = time_msec(); - if (lport_maybe_postpone(iface->id, now, - get_postponed_ports())) { + if (iface->is_vif) { + if (!local_bindings_pb_chassis_is_set(bindings, iface->id, + chassis_rec)) { + if (!sb_readonly) { + long long int now = time_msec(); + if (lport_maybe_postpone(iface->id, now, + get_postponed_ports())) { + continue; + } + local_binding_set_pb(bindings, iface->id, chassis_rec, + NULL, true, iface->bind_type); + } else { continue; } - local_binding_set_pb(bindings, iface->id, chassis_rec, - NULL, true, iface->bind_type); - } else { - continue; } - } - if (local_binding_is_up(bindings, iface->id, chassis_rec)) { - ovs_iface_set_state(mgr, iface, OIF_INSTALLED); + if (local_binding_is_up(bindings, iface->id, chassis_rec)) { + ovs_iface_set_state(mgr, iface, OIF_INSTALLED); + } + } else { + if (!port_binding_pb_chassis_is_set(chassis_rec, pb_table, + &iface->pb_uuid)) { + if (!sb_readonly) { + port_binding_set_pb(chassis_rec, pb_table, iface->id, + &iface->pb_uuid, iface->bind_type); + } else { + continue; + } + } + if (port_binding_is_up(chassis_rec, pb_table, &iface->pb_uuid)) { + ovs_iface_set_state(mgr, iface, OIF_INSTALLED); + } } } @@ -537,9 +585,13 @@ if_status_mgr_update(struct if_status_mgr *mgr, /* No need to to update pb->chassis as already done * in if_status_handle_claims or if_status_mgr_claim_iface */ - ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS); - iface->install_seqno = mgr->iface_seqno + 1; - new_ifaces = true; + if (iface->is_vif) { + ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS); + iface->install_seqno = mgr->iface_seqno + 1; + new_ifaces = true; + } else { + ovs_iface_set_state(mgr, iface, OIF_MARK_UP); + } } } else { HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) { @@ -596,6 +648,7 @@ if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *binding_data, const struct sbrec_chassis *chassis_rec, const struct ovsrec_interface_table *iface_table, + const struct sbrec_port_binding_table *pb_table, bool sb_readonly, bool ovs_readonly) { struct ofctrl_acked_seqnos *acked_seqnos = @@ -631,15 +684,18 @@ if_status_mgr_run(struct if_status_mgr *mgr, /* Update binding states. */ if_status_mgr_update_bindings(mgr, binding_data, chassis_rec, - iface_table, + iface_table, pb_table, sb_readonly, ovs_readonly); } static void -ovs_iface_account_mem(const char *iface_id, bool erase) +ovs_iface_account_mem(const char *iface_id, char *iface_name, bool erase) { uint32_t size = (strlen(iface_id) + sizeof(struct ovs_iface) + sizeof(struct shash_node)); + if (iface_name) { + size += strlen(iface_name); + } if (erase) { ifaces_usage -= size; } else { @@ -691,12 +747,18 @@ ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id, { struct ovs_iface *iface = xzalloc(sizeof *iface); - VLOG_DBG("Interface %s create.", iface_id); + VLOG_DBG("Interface %s create for iface %s.", iface_id, + iface_rec ? iface_rec->name : ""); iface->id = xstrdup(iface_id); shash_add_nocopy(&mgr->ifaces, iface->id, iface); ovs_iface_set_state(mgr, iface, state); - ovs_iface_account_mem(iface_id, false); - if_status_mgr_iface_update(mgr, iface_rec); + if (iface_rec) { + ovs_iface_account_mem(iface_id, iface_rec->name, false); + if_status_mgr_iface_update(mgr, iface_rec); + iface->name = xstrdup(iface_rec->name); + } else { + ovs_iface_account_mem(iface_id, NULL, false); + } return iface; } @@ -720,7 +782,8 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface) if (node) { shash_steal(&mgr->ifaces, node); } - ovs_iface_account_mem(iface->id, true); + ovs_iface_account_mem(iface->id, iface->name, true); + free(iface->name); free(iface->id); free(iface); } @@ -761,6 +824,7 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, struct local_binding_data *binding_data, const struct sbrec_chassis *chassis_rec, const struct ovsrec_interface_table *iface_table, + const struct sbrec_port_binding_table *pb_table, bool sb_readonly, bool ovs_readonly) { if (!binding_data) { @@ -797,9 +861,20 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, char *ts_now_str = xasprintf("%lld", time_wall_msec()); HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_MARK_UP]) { struct ovs_iface *iface = node->data; - - local_binding_set_up(bindings, iface->id, chassis_rec, ts_now_str, - sb_readonly, ovs_readonly); + if (iface->is_vif) { + local_binding_set_up(bindings, iface->id, chassis_rec, ts_now_str, + sb_readonly, ovs_readonly); + } else if (!sb_readonly) { + const struct sbrec_port_binding *pb = + sbrec_port_binding_table_get_for_uuid(pb_table, + &iface->pb_uuid); + if (pb) { + const struct sbrec_port_binding *parent_pb = + sbrec_port_binding_table_get_for_uuid(pb_table, + &iface->parent_pb_uuid); + claimed_lport_set_up(pb, parent_pb); + } + } } free(ts_now_str); diff --git a/controller/if-status.h b/controller/if-status.h index 9714f6d8d..4ae5ad481 100644 --- a/controller/if-status.h +++ b/controller/if-status.h @@ -32,9 +32,12 @@ void if_status_mgr_claim_iface(struct if_status_mgr *, const struct sbrec_port_binding *pb, const struct sbrec_chassis *chassis_rec, const struct ovsrec_interface *iface_rec, - bool sb_readonly, enum can_bind bind_type); + bool sb_readonly, enum can_bind bind_type, + bool notify_up, + const struct sbrec_port_binding *parent_pb); void if_status_mgr_release_iface(struct if_status_mgr *, const char *iface_id); -void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id); +void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id, + const struct ovsrec_interface *iface_rec); void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *, const struct sbrec_chassis *chassis, @@ -45,6 +48,7 @@ void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *, void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *, const struct sbrec_chassis *, const struct ovsrec_interface_table *iface_table, + const struct sbrec_port_binding_table *pb_table, bool sb_readonly, bool ovs_readonly); void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, struct simap *usage); @@ -54,6 +58,7 @@ bool if_status_handle_claims(struct if_status_mgr *mgr, struct local_binding_data *binding_data, const struct sbrec_chassis *chassis_rec, struct hmap *tracked_datapath, + const struct sbrec_port_binding_table *pb_table, bool sb_readonly); void if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr, const char *name, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index da7d145ed..79754e753 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1820,6 +1820,8 @@ runtime_data_sb_ro_handler(struct engine_node *node, void *data) engine_ovsdb_node_get_index( engine_get_input("SB_chassis", node), "name"); + const struct sbrec_port_binding_table *pb_table = + EN_OVSDB_GET(engine_get_input("SB_port_binding", node)); if (chassis_id) { chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); @@ -1834,7 +1836,7 @@ runtime_data_sb_ro_handler(struct engine_node *node, void *data) &rt_data->lbinding_data, chassis, &rt_data->tracked_dp_bindings, - sb_readonly)) { + pb_table, sb_readonly)) { engine_set_node_state(node, EN_UPDATED); rt_data->tracked = true; } @@ -5880,6 +5882,8 @@ main(int argc, char *argv[]) if_status_mgr_run(if_mgr, binding_data, chassis, ovsrec_interface_table_get( ovs_idl_loop.idl), + sbrec_port_binding_table_get( + ovnsb_idl_loop.idl), !ovnsb_idl_txn, !ovs_idl_txn); stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME, time_msec()); diff --git a/tests/ovn.at b/tests/ovn.at index 637d92bed..970817c3f 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -36957,3 +36957,97 @@ AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([virtual port claim race condition]) +AT_KEYWORDS([virtual ports]) +ovn_start + +send_garp() { + local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6 + local request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa} + as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request +} + +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 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 +ovs-vsctl -- add-port br-int hv1-vif3 -- \ + set interface hv1-vif3 \ + options:tx_pcap=hv1/vif3-tx.pcap \ + options:rxq_pcap=hv1/vif3-rx.pcap \ + ofport-request=3 + +ovs-appctl -t ovn-controller vlog/set dbg + +ovn-nbctl ls-add sw0 + +check ovn-nbctl lsp-add sw0 sw0-vir +check ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10" +check ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10" +check ovn-nbctl lsp-set-type sw0-vir virtual +check ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10 +check ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1 + +check ovn-nbctl lsp-add sw0 sw0-p1 +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3" +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 10.0.0.10 1000::3" + +# Create a logical router and attach both logical switches +check ovn-nbctl lr-add lr0 +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 1000::a/64 +check ovn-nbctl lsp-add sw0 sw0-lr0 +check ovn-nbctl lsp-set-type sw0-lr0 router +check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 + +wait_for_ports_up +ovn-nbctl --wait=hv sync +hv1_ch_uuid=`ovn-sbctl --bare --columns _uuid find chassis name="hv1"` + +# Try to bind sw0-vir directly to an OVS port. This should be ignored by +# ovn-controller. +as hv1 ovs-vsctl set interface hv1-vif3 external-ids:iface-id=sw0-vir + +# Make sb to sleep, so that claim of sw0-vir (through pinctrl) and hv1-vif3 can be handled within same idl by controller +sleep_sb + +# From sw0-p0 send GARP for 10.0.0.10. hv1 should claim sw0-vir +# and sw0-p1 should be its virtual_parent. +eth_src=505400000003 +eth_dst=ffffffffffff +spa=$(ip_to_hex 10 0 0 10) +tpa=$(ip_to_hex 10 0 0 10) +send_garp 1 1 $eth_src $eth_dst $spa $tpa + +OVS_WAIT_UNTIL([test 1 = `cat hv1/ovn-controller.log | grep "pinctrl received packet-in" | \ +grep opcode=BIND_VPORT | grep OF_Table_ID=29 | wc -l`]) + +sleep_controller hv1 + +# Cleanup hv1-vif3. This should not interfere with sw0-vir claim +as hv1 ovs-vsctl del-port hv1-vif3 + +wake_up_sb +ovn-nbctl --wait=sb sync +wake_up_controller hv1 +check ovn-nbctl --wait=hv sync + +wait_row_count Port_Binding 1 logical_port=sw0-vir chassis=$hv1_ch_uuid +check_row_count Port_Binding 1 logical_port=sw0-vir virtual_parent=sw0-p1 +wait_for_ports_up sw0-vir +check ovn-nbctl --wait=hv sync + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) +