From patchwork Tue Nov 24 13:51:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1405580 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=140.211.166.133; helo=hemlock.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: 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=OOgGoRMq; dkim-atps=neutral Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4CgQRd1cr7z9sSf for ; Wed, 25 Nov 2020 00:51:40 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id A8B63872BD; Tue, 24 Nov 2020 13:51:37 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xnVagPA8Stzh; Tue, 24 Nov 2020 13:51:36 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id B019B8700D; Tue, 24 Nov 2020 13:51:36 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 917DBC163C; Tue, 24 Nov 2020 13:51:36 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 09EE3C0052 for ; Tue, 24 Nov 2020 13:51:36 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 9F58F214EC for ; Tue, 24 Nov 2020 13:51:35 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id OH8M7CAPd3lg for ; Tue, 24 Nov 2020 13:51:33 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by silver.osuosl.org (Postfix) with ESMTPS id 271C5204CF for ; Tue, 24 Nov 2020 13:51:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1606225891; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:content-type:content-type; bh=0CtP7mimJukn/WBc953PD30zoLyGwYrFnBSmMH6ZgGY=; b=OOgGoRMqn/PiD7iM7xuLUyGOJphwxGFZ6UEdAlPv/rR4bu+JO4BaInSOH/987KRzxaWCni 3OaRQyT0RnPQ1Eh2s9uCcxrgOMeUg9Y445BBf75RkyE/q45ZoBDQvI3xydX7xGT6h5pThk 3y3GkCycU5Jk41hoxhNDaYkPjMLqE5w= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-11-I1TbeN8aPtuLzbrzzYKEaA-1; Tue, 24 Nov 2020 08:51:29 -0500 X-MC-Unique: I1TbeN8aPtuLzbrzzYKEaA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 86D0B8049CF; Tue, 24 Nov 2020 13:51:28 +0000 (UTC) Received: from dceara.remote.csb (ovpn-114-190.ams2.redhat.com [10.36.114.190]) by smtp.corp.redhat.com (Postfix) with ESMTP id 759B310016F7; Tue, 24 Nov 2020 13:51:27 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Tue, 24 Nov 2020 14:51:23 +0100 Message-Id: <1606225883-17192-1-git-send-email-dceara@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dceara@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [PATCH ovn v3] ovn-controller: Fix nb_cfg update with monitor_cond_change in flight. 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" It is not correct for ovn-controller to pass the current SB_Global.nb_cfg value to ofctrl_put() if there are pending changes to conditional monitoring clauses (local or in flight). It might be that after the monitor condition is acked by the SB, records that were added to the SB before SB_Global.nb_cfg was set are now sent as updates to ovn-controller. These should be first installed in OVS before ovn-controller reports that it caught up with the current SB_Global.nb_cfg value. Also: - ofctrl_put should not advance cur_cfg if there are flow updates in flight. - ofctrl_put should be called after SB monitor conditions are updated. Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet mode.") Acked-by: Ben Pfaff Signed-off-by: Dumitru Ceara Acked-by: Han Zhou --- V3: - move ofctrl_put() call after updating SB monitor conditions. - added Ben's ack. v2: - use new IDL *set_condition() return value. - fix ofctrl_put to not advance cur_cfg if there are flow updates in flight. --- controller/ofctrl.c | 2 +- controller/ovn-controller.c | 91 ++++++++++++++++++++++++++++++--------------- 2 files changed, 62 insertions(+), 31 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index c1bbc58..eac5305 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -2034,7 +2034,7 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, need_put = true; } else if (nb_cfg != old_nb_cfg) { /* nb_cfg changed since last ofctrl_put() call */ - if (cur_cfg == old_nb_cfg) { + if (cur_cfg == old_nb_cfg && ovs_list_is_empty(&flow_updates)) { /* we were up-to-date already, so just update with the * new nb_cfg */ cur_cfg = nb_cfg; diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index b0f1975..46589e4 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -133,7 +133,7 @@ get_bridge(const struct ovsrec_bridge_table *bridge_table, const char *br_name) return NULL; } -static void +static unsigned int update_sb_monitors(struct ovsdb_idl *ovnsb_idl, const struct sbrec_chassis *chassis, const struct sset *local_ifaces, @@ -239,16 +239,24 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl, } } -out: - sbrec_port_binding_set_condition(ovnsb_idl, &pb); - sbrec_logical_flow_set_condition(ovnsb_idl, &lf); - sbrec_mac_binding_set_condition(ovnsb_idl, &mb); - sbrec_multicast_group_set_condition(ovnsb_idl, &mg); - sbrec_dns_set_condition(ovnsb_idl, &dns); - sbrec_controller_event_set_condition(ovnsb_idl, &ce); - sbrec_ip_multicast_set_condition(ovnsb_idl, &ip_mcast); - sbrec_igmp_group_set_condition(ovnsb_idl, &igmp); - sbrec_chassis_private_set_condition(ovnsb_idl, &chprv); +out:; + unsigned int cond_seqnos[] = { + sbrec_port_binding_set_condition(ovnsb_idl, &pb), + sbrec_logical_flow_set_condition(ovnsb_idl, &lf), + sbrec_mac_binding_set_condition(ovnsb_idl, &mb), + sbrec_multicast_group_set_condition(ovnsb_idl, &mg), + sbrec_dns_set_condition(ovnsb_idl, &dns), + sbrec_controller_event_set_condition(ovnsb_idl, &ce), + sbrec_ip_multicast_set_condition(ovnsb_idl, &ip_mcast), + sbrec_igmp_group_set_condition(ovnsb_idl, &igmp), + sbrec_chassis_private_set_condition(ovnsb_idl, &chprv), + }; + + unsigned int expected_cond_seqno = 0; + for (size_t i = 0; i < ARRAY_SIZE(cond_seqnos); i++) { + expected_cond_seqno = MAX(expected_cond_seqno, cond_seqnos[i]); + } + ovsdb_idl_condition_destroy(&pb); ovsdb_idl_condition_destroy(&lf); ovsdb_idl_condition_destroy(&mb); @@ -258,6 +266,7 @@ out: ovsdb_idl_condition_destroy(&ip_mcast); ovsdb_idl_condition_destroy(&igmp); ovsdb_idl_condition_destroy(&chprv); + return expected_cond_seqno; } static const char * @@ -491,7 +500,7 @@ get_ofctrl_probe_interval(struct ovsdb_idl *ovs_idl) static void update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl, bool *monitor_all_p, bool *reset_ovnsb_idl_min_index, - bool *enable_lflow_cache) + bool *enable_lflow_cache, unsigned int *sb_cond_seqno) { const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl); if (!cfg) { @@ -516,7 +525,11 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl, * Otherwise, don't call it here, because there would be unnecessary * extra cost. Instead, it is called after the engine execution only * when it is necessary. */ - update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true); + unsigned int next_cond_seqno = + update_sb_monitors(ovnsb_idl, NULL, NULL, NULL, true); + if (sb_cond_seqno) { + *sb_cond_seqno = next_cond_seqno; + } } if (monitor_all_p) { *monitor_all_p = monitor_all; @@ -803,11 +816,23 @@ restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, } static int64_t -get_nb_cfg(const struct sbrec_sb_global_table *sb_global_table) +get_nb_cfg(const struct sbrec_sb_global_table *sb_global_table, + unsigned int cond_seqno, unsigned int expected_cond_seqno) { + static int64_t nb_cfg = 0; + + /* Delay getting nb_cfg if there are monitor condition changes + * in flight. It might be that those changes would instruct the + * server to send updates that happened before SB_Global.nb_cfg. + */ + if (cond_seqno != expected_cond_seqno) { + return nb_cfg; + } + const struct sbrec_sb_global *sb = sbrec_sb_global_table_first(sb_global_table); - return sb ? sb->nb_cfg : 0; + nb_cfg = sb ? sb->nb_cfg : 0; + return nb_cfg; } /* Propagates the local cfg seqno, 'cur_cfg', to the chassis_private record @@ -2615,6 +2640,7 @@ main(int argc, char *argv[]) unsigned int ovs_cond_seqno = UINT_MAX; unsigned int ovnsb_cond_seqno = UINT_MAX; + unsigned int ovnsb_expected_cond_seqno = UINT_MAX; struct controller_engine_ctx ctrl_engine_ctx = { .enable_lflow_cache = true @@ -2652,7 +2678,8 @@ main(int argc, char *argv[]) update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, &sb_monitor_all, &reset_ovnsb_idl_min_index, - &ctrl_engine_ctx.enable_lflow_cache); + &ctrl_engine_ctx.enable_lflow_cache, + &ovnsb_expected_cond_seqno); update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl)); @@ -2769,15 +2796,6 @@ main(int argc, char *argv[]) sbrec_sb_global_table_get(ovnsb_idl_loop.idl)); } - flow_output_data = engine_get_data(&en_flow_output); - if (flow_output_data && ct_zones_data) { - ofctrl_put(&flow_output_data->flow_table, - &ct_zones_data->pending, - sbrec_meter_table_get(ovnsb_idl_loop.idl), - get_nb_cfg(sbrec_sb_global_table_get( - ovnsb_idl_loop.idl)), - engine_node_changed(&en_flow_output)); - } runtime_data = engine_get_data(&en_runtime_data); if (runtime_data) { patch_run(ovs_idl_txn, @@ -2803,12 +2821,25 @@ main(int argc, char *argv[]) &runtime_data->local_datapaths, &runtime_data->active_tunnels); if (engine_node_changed(&en_runtime_data)) { - update_sb_monitors(ovnsb_idl_loop.idl, chassis, - &runtime_data->local_lports, - &runtime_data->local_datapaths, - sb_monitor_all); + ovnsb_expected_cond_seqno = + update_sb_monitors( + ovnsb_idl_loop.idl, chassis, + &runtime_data->local_lports, + &runtime_data->local_datapaths, + sb_monitor_all); } } + flow_output_data = engine_get_data(&en_flow_output); + if (flow_output_data && ct_zones_data) { + ofctrl_put(&flow_output_data->flow_table, + &ct_zones_data->pending, + sbrec_meter_table_get(ovnsb_idl_loop.idl), + get_nb_cfg(sbrec_sb_global_table_get( + ovnsb_idl_loop.idl), + ovnsb_cond_seqno, + ovnsb_expected_cond_seqno), + engine_node_changed(&en_flow_output)); + } } } @@ -2920,7 +2951,7 @@ loop_done: bool done = !ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl); while (!done) { update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, - NULL, NULL, NULL); + NULL, NULL, NULL, NULL); update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl)); struct ovsdb_idl_txn *ovs_idl_txn