[ovs-dev] [PATCH v2 1/3] ovn-controller: Fix chassis ovn-sbdb record init

Dumitru Ceara dceara at redhat.com
Mon Jul 8 10:07:00 UTC 2019


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 <haili at redhat.com>
Signed-off-by: Dumitru Ceara <dceara at 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])



More information about the dev mailing list