From 59103ff841797ad399e1679bfecfb6256bb6a0c4 Mon Sep 17 00:00:00 2001

From: Dumitru Ceara <dceara@redhat.com>

Date: Thu, 3 Sep 2020 17:04:01 +0200

Subject: [PATCH 2/2] chassis: Fix chassis_private record updates when the

 system-id changes.

Also:

- Change conditional monitoring for Chassis_Private to use the chassis uuid

  instead of chassis name. Using the chassis->name field does not work

  because this is the old value of the field and would cause ovsdb-server

  to inform ovn-controller that the updated Chassis_Private record was

  "deleted" because it doesn't match the monitor condition anymore.

- Allow ovn-sbctl to access Chassis_Private records by name.

Reported-at: https://bugzilla.redhat.com/1873032

Reported-by: Ying Xu <yinxu@redhat.com>

CC: Han Zhou <hzhou@ovn.org>

Fixes: 4adc10f58127 ("Avoid nb_cfg update notification flooding")

Signed-off-by: Dumitru Ceara <dceara@redhat.com>

Signed-off-by: Numan Siddique <numans@ovn.org>

(cherry picked from upstream commit dce1af31b550a9fb57b01cbe0b4139b6768f2521)

Change-Id: Ic5f7f0b820b43715e1f1cf68b215374daf237fd5

---

 controller/chassis.c        | 92 +++++++++++++++++++++++++++++++++++++++------

 controller/chassis.h        |  2 +

 controller/ovn-controller.c | 13 ++++---

 tests/ovn-controller.at     | 18 +++++++++

 utilities/ovn-sbctl.c       |  3 ++

 5 files changed, 112 insertions(+), 16 deletions(-)

diff --git a/controller/chassis.c b/controller/chassis.c

index 773d966..8231169 100644

--- a/controller/chassis.c

+++ b/controller/chassis.c

@@ -633,6 +633,77 @@ chassis_update(const struct sbrec_chassis *chassis_rec,

     return true;

 }

+/*

+ * Returns a pointer to a chassis_private record from 'chassis_pvt_table' that

+ * matches the chassis record.

+ */

+static const struct sbrec_chassis_private *

+chassis_private_get_stale_record(

+    const struct sbrec_chassis_private_table *chassis_pvt_table,

+    const struct sbrec_chassis *chassis)

+{

+    const struct sbrec_chassis_private *chassis_pvt_rec;

+

+    SBREC_CHASSIS_PRIVATE_TABLE_FOR_EACH (chassis_pvt_rec, chassis_pvt_table) {

+        if (chassis_pvt_rec->chassis == chassis) {

+            return chassis_pvt_rec;

+        }

+    }

+

+    return NULL;

+}

+

+/* If this is a chassis_private config update after we initialized the record

+ * once then we should always be able to find it with the ID we saved in

+ * chassis_state.

+ * Otherwise (i.e., first time we created the chassis record or if the

+ * system-id changed) then we check if there's a stale record from a previous

+ * controller run that didn't end gracefully and reuse it. If not then we

+ * create a new record.

+ *

+ * Returns the local chassis record.

+ */

+static const struct sbrec_chassis_private *

+chassis_private_get_record(

+    struct ovsdb_idl_txn *ovnsb_idl_txn,

+    struct ovsdb_idl_index *sbrec_chassis_pvt_by_name,

+    const struct sbrec_chassis_private_table *chassis_pvt_table,

+    const struct sbrec_chassis *chassis)

+{

+    const struct sbrec_chassis_private *chassis_p = NULL;

+

+    if (chassis_info_id_inited(&chassis_state)) {

+        chassis_p =

+            chassis_private_lookup_by_name(sbrec_chassis_pvt_by_name,

+                                           chassis_info_id(&chassis_state));

+    }

+

+    if (!chassis_p) {

+        chassis_p = chassis_private_get_stale_record(chassis_pvt_table,

+                                                     chassis);

+    }

+

+    if (!chassis_p && ovnsb_idl_txn) {

+        return sbrec_chassis_private_insert(ovnsb_idl_txn);

+    }

+

+    return chassis_p;

+}

+

+static void

+chassis_private_update(const struct sbrec_chassis_private *chassis_pvt,

+                       const struct sbrec_chassis *chassis,

+                       const char *chassis_id)

+{

+    if (!chassis_pvt->name || strcmp(chassis_pvt->name, chassis_id)) {

+        sbrec_chassis_private_set_name(chassis_pvt, chassis_id);

+    }

+

+    if (chassis_pvt->chassis != chassis) {

+        sbrec_chassis_private_set_chassis(chassis_pvt, chassis);

+    }

+}

+

 /* Returns this chassis's Chassis record, if it is available. */

 const struct sbrec_chassis *

 chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,

@@ -640,6 +711,7 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,

             struct ovsdb_idl_index *sbrec_chassis_private_by_name,

             const struct ovsrec_open_vswitch_table *ovs_table,

             const struct sbrec_chassis_table *chassis_table,

+            const struct sbrec_chassis_private_table *chassis_pvt_table,

             const char *chassis_id,

             const struct ovsrec_bridge *br_int,

             const struct sset *transport_zones,

@@ -668,7 +740,6 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,

         bool updated = chassis_update(chassis_rec, ovnsb_idl_txn, &ovs_cfg,

                                       chassis_id, transport_zones);

-        chassis_info_set_id(&chassis_state, chassis_id);

         if (!existed || updated) {

             ovsdb_idl_txn_add_comment(ovnsb_idl_txn,

                                       "ovn-controller: %s chassis '%s'",

@@ -676,17 +747,16 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,

                                       chassis_id);

         }

-        const struct sbrec_chassis_private *chassis_private_rec =

-            chassis_private_lookup_by_name(sbrec_chassis_private_by_name,

-                                           chassis_id);

-        if (!chassis_private_rec && ovnsb_idl_txn) {

-            chassis_private_rec = sbrec_chassis_private_insert(ovnsb_idl_txn);

-            sbrec_chassis_private_set_name(chassis_private_rec,

-                                           chassis_id);

-            sbrec_chassis_private_set_chassis(chassis_private_rec,

-                                              chassis_rec);

+        *chassis_private =

+            chassis_private_get_record(ovnsb_idl_txn,

+                                       sbrec_chassis_private_by_name,

+                                       chassis_pvt_table, chassis_rec);

+

+        if (*chassis_private) {

+            chassis_private_update(*chassis_private, chassis_rec, chassis_id);

         }

-        *chassis_private = chassis_private_rec;

+

+        chassis_info_set_id(&chassis_state, chassis_id);

     }

     ovs_chassis_cfg_destroy(&ovs_cfg);

diff --git a/controller/chassis.h b/controller/chassis.h

index 81055b4..220f726 100644

--- a/controller/chassis.h

+++ b/controller/chassis.h

@@ -26,6 +26,7 @@ struct ovsrec_bridge;

 struct ovsrec_open_vswitch_table;

 struct sbrec_chassis;

 struct sbrec_chassis_table;

+struct sbrec_chassis_private_table;

 struct sset;

 struct eth_addr;

 struct smap;

@@ -37,6 +38,7 @@ const struct sbrec_chassis *chassis_run(

     struct ovsdb_idl_index *sbrec_chassis_private_by_name,

     const struct ovsrec_open_vswitch_table *,

     const struct sbrec_chassis_table *,

+    const struct sbrec_chassis_private_table *,

     const char *chassis_id, const struct ovsrec_bridge *br_int,

     const struct sset *transport_zones,

     const struct sbrec_chassis_private **chassis_private);

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c

index 28ca7a8..6aeeb15 100644

--- a/controller/ovn-controller.c

+++ b/controller/ovn-controller.c

@@ -179,7 +179,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,

      * chassis */

     sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "chassisredirect");

     sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "external");

-    if (chassis) {

+    if (chassis && !sbrec_chassis_is_new(chassis)) {

         /* This should be mostly redundant with the other clauses for port

          * bindings, but it allows us to catch any ports that are assigned to

          * us but should not be.  That way, we can clear their chassis

@@ -202,9 +202,9 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,

         sbrec_igmp_group_add_clause_chassis(&igmp, OVSDB_F_EQ,

                                             &chassis->header_.uuid);

-        /* Monitors Chassis_Private record for current chassis only */

-        sbrec_chassis_private_add_clause_name(&chprv, OVSDB_F_EQ,

-                                              chassis->name);

+        /* Monitors Chassis_Private record for current chassis only. */

+        sbrec_chassis_private_add_clause_chassis(&chprv, OVSDB_F_EQ,

+                                                 &chassis->header_.uuid);

     } else {

         /* During initialization, we monitor all records in Chassis_Private so

          * that we don't try to recreate existing ones. */

@@ -2402,6 +2402,8 @@ main(int argc, char *argv[])

                 ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);

             const struct sbrec_chassis_table *chassis_table =

                 sbrec_chassis_table_get(ovnsb_idl_loop.idl);

+            const struct sbrec_chassis_private_table *chassis_pvt_table =

+                sbrec_chassis_private_table_get(ovnsb_idl_loop.idl);

             const struct ovsrec_bridge *br_int =

                 process_br_int(ovs_idl_txn, bridge_table, ovs_table);

             const char *chassis_id = get_ovs_chassis_id(ovs_table);

@@ -2410,7 +2412,8 @@ main(int argc, char *argv[])

             if (chassis_id) {

                 chassis = chassis_run(ovnsb_idl_txn, sbrec_chassis_by_name,

                                       sbrec_chassis_private_by_name,

-                                      ovs_table, chassis_table, chassis_id,

+                                      ovs_table, chassis_table,

+                                      chassis_pvt_table, chassis_id,

                                       br_int, &transport_zones,

                                       &chassis_private);

             }

diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at

index f2faf1f..812946b 100644

--- a/tests/ovn-controller.at

+++ b/tests/ovn-controller.at

@@ -195,6 +195,15 @@ OVS_WAIT_UNTIL([

     chassis_id=$(ovn-sbctl get Chassis "${sysid}" name)

     test "${sysid}" = "${chassis_id}"

 ])

+OVS_WAIT_UNTIL([

+    chassis_id=$(ovn-sbctl get Chassis_Private "${sysid}" name)

+    test "${sysid}" = "${chassis_id}"

+])

+

+# Only one Chassis_Private record should exist.

+OVS_WAIT_UNTIL([

+    test $(ovn-sbctl --columns _uuid list chassis_private | wc -l) -eq 1

+])

 # Simulate system-id changing while ovn-controller is disconnected from the

 # SB.

@@ -212,6 +221,15 @@ OVS_WAIT_UNTIL([

     chassis_id=$(ovn-sbctl get Chassis "${sysid}" name)

     test "${sysid}" = "${chassis_id}"

 ])

+OVS_WAIT_UNTIL([

+    chassis_id=$(ovn-sbctl get Chassis_Private "${sysid}" name)

+    test "${sysid}" = "${chassis_id}"

+])

+

+# Only one Chassis_Private record should exist.

+OVS_WAIT_UNTIL([

+    test $(ovn-sbctl --columns _uuid list chassis_private | wc -l) -eq 1

+])

 # Gracefully terminate daemons

 OVN_CLEANUP_SBOX([hv])

diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c

index 04e082c..d3eec91 100644

--- a/utilities/ovn-sbctl.c

+++ b/utilities/ovn-sbctl.c

@@ -1391,6 +1391,9 @@ cmd_set_ssl(struct ctl_context *ctx)

 static const struct ctl_table_class tables[SBREC_N_TABLES] = {

     [SBREC_TABLE_CHASSIS].row_ids[0] = {&sbrec_chassis_col_name, NULL, NULL},

+    [SBREC_TABLE_CHASSIS_PRIVATE].row_ids[0]

+    = {&sbrec_chassis_private_col_name, NULL, NULL},

+

     [SBREC_TABLE_DATAPATH_BINDING].row_ids

      = {{&sbrec_datapath_binding_col_external_ids, "name", NULL},

         {&sbrec_datapath_binding_col_external_ids, "name2", NULL},

-- 

1.8.3.1