From patchwork Wed Jun 12 10:38:39 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1114459 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 45P3Ls0kzlz9s9y for ; Wed, 12 Jun 2019 20:41:09 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id EEB1A19B4; Wed, 12 Jun 2019 10:40:34 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 531C1199F for ; Wed, 12 Jun 2019 10:38:50 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id AD302E6 for ; Wed, 12 Jun 2019 10:38:49 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 55B85C049598 for ; Wed, 12 Jun 2019 10:38:44 +0000 (UTC) Received: from dceara.remote.csb (ovpn-117-241.ams2.redhat.com [10.36.117.241]) by smtp.corp.redhat.com (Postfix) with ESMTP id ABFE517C21 for ; Wed, 12 Jun 2019 10:38:43 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Wed, 12 Jun 2019 12:38:39 +0200 Message-Id: <20190612103839.18479.48730.stgit@dceara.remote.csb> In-Reply-To: <20190612103757.18479.70964.stgit@dceara.remote.csb> References: <20190612103757.18479.70964.stgit@dceara.remote.csb> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Wed, 12 Jun 2019 10:38:44 +0000 (UTC) X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [PATCH 1/3] ovn-controller: Fix chassis ovn-sbdb record init X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org The chassis_run code didn't take into account the scenario when the system-id was changed in the Open_vSwitch table. Due to this the code was trying to insert a new Chassis record in the OVN_Southbound DB with the same Encaps as the previous Chassis record. The transaction used to insert the new records was aborting due to the ["type", "ip"] index constraint violation as we were creating new Encap entries with the same "type" and "ip" as the old ones. In order to fix this issue the flow is now: 1. the first time ovn-controller initializes the Chassis (shortly after start up) we store the chassis-id. 2. for subsequent chassis_run calls we use last configured chassis-id stored at the previous step to lookup the old Chassis record. 3. when ovn-controller shuts down gracefully we lookup the Chassis record based on the chassis-id stored in memory at steps 1 and 2 above. This is to avoid failing to cleanup the Chassis record in OVN_Southbound DB if the OVS system-id changes between the last call to chassis_run and chassis_cleanup. Reported-at: https://bugzilla.redhat.com/1708146 Reported-by: Haidong Li Signed-off-by: Dumitru Ceara --- ovn/controller/chassis.c | 82 +++++++++++++++++++++++++++++++++------ ovn/controller/chassis.h | 1 ovn/controller/ovn-controller.c | 2 - tests/ovn-controller.at | 9 ++++ 4 files changed, 81 insertions(+), 13 deletions(-) diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c index 0f537f1..85d418d 100644 --- a/ovn/controller/chassis.c +++ b/ovn/controller/chassis.c @@ -35,6 +35,44 @@ VLOG_DEFINE_THIS_MODULE(chassis); #define HOST_NAME_MAX 255 #endif /* HOST_NAME_MAX */ +/* + * Structure to hold chassis specific state (currently just chassis-id) + * to avoid database lookups when changes happen while the controller is + * running. + */ +struct chassis_info { + /* Last ID we initialized the Chassis SB record with. */ + struct ds id; + + /* True if Chassis SB record is initialized, false otherwise. */ + uint32_t id_inited : 1; +}; + +static struct chassis_info chassis_state = { + .id = DS_EMPTY_INITIALIZER, + .id_inited = false, +}; + +static void +chassis_info_set_id(struct chassis_info *info, const char *id) +{ + ds_clear(&info->id); + ds_put_cstr(&info->id, id); + info->id_inited = true; +} + +static bool +chassis_info_id_inited(const struct chassis_info *info) +{ + return info->id_inited; +} + +static const char * +chassis_info_id(const struct chassis_info *info) +{ + return ds_cstr_ro(&info->id); +} + void chassis_register_ovs_idl(struct ovsdb_idl *ovs_idl) { @@ -103,16 +141,20 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct ovsrec_bridge *br_int, const struct sset *transport_zones) { - const struct sbrec_chassis *chassis_rec - = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id); + const struct ovsrec_open_vswitch *cfg; + const char *encap_type, *encap_ip; + + const struct sbrec_chassis *chassis_rec = NULL; + + if (chassis_info_id_inited(&chassis_state)) { + chassis_rec = chassis_lookup_by_name(sbrec_chassis_by_name, + chassis_info_id(&chassis_state)); + } + if (!ovnsb_idl_txn) { return chassis_rec; } - const struct ovsrec_open_vswitch *cfg; - const char *encap_type, *encap_ip; - static bool inited = false; - cfg = ovsrec_open_vswitch_table_first(ovs_table); if (!cfg) { VLOG_INFO("No Open_vSwitch row defined."); @@ -251,10 +293,8 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn, if (same) { /* Nothing changed. */ - inited = true; - ds_destroy(&iface_types); - return chassis_rec; - } else if (!inited) { + goto inited; + } else if (!chassis_info_id_inited(&chassis_state)) { struct ds cur_encaps = DS_EMPTY_INITIALIZER; for (int i = 0; i < chassis_rec->n_encaps; i++) { ds_put_format(&cur_encaps, "%s,", @@ -281,7 +321,6 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn, smap_add(&ext_ids, "datapath-type", datapath_type); smap_add(&ext_ids, "iface-types", iface_types_str); chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn); - sbrec_chassis_set_name(chassis_rec, chassis_id); sbrec_chassis_set_hostname(chassis_rec, hostname); sbrec_chassis_set_external_ids(chassis_rec, &ext_ids); smap_destroy(&ext_ids); @@ -315,7 +354,13 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn, free(tokstr); free(encaps); - inited = true; +inited: + /* Store the name of the chassis for further lookups. */ + if (!chassis_rec->name || strcmp(chassis_id, chassis_rec->name)) { + sbrec_chassis_set_name(chassis_rec, chassis_id); + chassis_info_set_id(&chassis_state, chassis_id); + } + return chassis_rec; } @@ -336,3 +381,16 @@ chassis_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, } return false; } + +/* + * Returns the last initialized chassis-id. + */ +const char * +chassis_get_id(void) +{ + if (chassis_info_id_inited(&chassis_state)) { + return chassis_info_id(&chassis_state); + } + + return NULL; +} diff --git a/ovn/controller/chassis.h b/ovn/controller/chassis.h index 9847e19..8d57a09 100644 --- a/ovn/controller/chassis.h +++ b/ovn/controller/chassis.h @@ -36,5 +36,6 @@ const struct sbrec_chassis *chassis_run( const struct sset *transport_zones); bool chassis_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct sbrec_chassis *); +const char *chassis_get_id(void); #endif /* ovn/chassis.h */ diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c index 6019016..b5f30f8 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -2062,7 +2062,7 @@ main(int argc, char *argv[]) const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table); - const char *chassis_id = get_chassis_id(ovs_table); + const char *chassis_id = chassis_get_id(); const struct sbrec_chassis *chassis = (chassis_id ? chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id) diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index d4bb071..343c2ab 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -187,6 +187,15 @@ OVS_WAIT_UNTIL([ test "${expected_iface_types}" = "${chassis_iface_types}" ]) +# Change the value of external_ids:system-id and make sure it's mirrored +# in the Chassis record in the OVN_Southbound database. +sysid=${sysid}-foo +ovs-vsctl set Open_vSwitch . external-ids:system-id="${sysid}" +OVS_WAIT_UNTIL([ + chassis_id=$(ovn-sbctl get Chassis "${sysid}" name) + test "${sysid}" = "${chassis_id}" +]) + # Gracefully terminate daemons OVN_CLEANUP_SBOX([hv]) OVN_CLEANUP_VSWITCH([main])