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 <ha...@redhat.com> Signed-off-by: Dumitru Ceara <dce...@redhat.com> --- 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 d0277ae..8099f30 100644 --- a/ovn/controller/chassis.c +++ b/ovn/controller/chassis.c @@ -36,6 +36,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) { @@ -110,16 +148,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."); @@ -263,10 +305,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,", @@ -293,7 +333,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); @@ -327,7 +366,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; } @@ -393,3 +438,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 e3fbc31..1366683 100644 --- a/ovn/controller/chassis.h +++ b/ovn/controller/chassis.h @@ -40,5 +40,6 @@ bool chassis_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, bool chassis_get_mac(const struct sbrec_chassis *chassis, const char *bridge_mapping, struct eth_addr *chassis_mac); +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 85dbcd2..d407c42 100644 --- a/ovn/controller/ovn-controller.c +++ b/ovn/controller/ovn-controller.c @@ -2098,7 +2098,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]) _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev