ovn-controller to no longer monitor Chassis' external_ids

Prior to this patch, the external_ids column from the Chassis table were
being used for two purposes:

1. Holding configuration for the OVN (copied by ovn-controller from the
local OVS database)

2. Holding information from external systems (the main purpose of the
   external_ids column across other resources).

The problem with mixing these two use cases is that, ovn-controller
should be aware of changes to the configuration and it would trigger
flow re-computations upon those changes. It shouldn't care tho about the
information from external systems but, since these two things were put
together, CMSs writing things to the external_ids column of the Chassis
were waking up ovn-controller to recompute flows.

This patch is separating these two things by creating another column in
the Chassis table called "other_config". This new table holds the OVN
configuration that is copied over from the local OVS db leaving the
external_ids column unmonitored and free for other systems to make use
of it.

This change also keeps things backward compatible by continuing to
write the OVN configuration to the external_ids column for external
systems that may be reading them from there but, that column is no
longer monitored so it won't generate any events. This behavior should
be temporary and removed in the future. The note in the NEWS file and
comments in the code itself points to the future removal of this
behavior.

Reported-At: https://bugzilla.redhat.com/show_bug.cgi?id=1824220
Signed-off-by: Lucas Alvares Gomes <[email protected]>
Acked-by: Dumitru Ceara <[email protected]>
Signed-off-by: Han Zhou <[email protected]>

Author: umago

NEWS

@@ -1,7 +1,15 @@
 Post-v20.03.0
 --------------------------
    - Added support for external_port_range in NAT table.
-
+   - ovn-controller no longer monitor the external_ids column from
+     the Chassis table. This was done to avoid having to do a flow
+     recalculation every time external systems wrote to this column. The
+     chassis configuration has now being moved to a new column called
+     "other_config". As a note, the configurations are still be written to
+     the external_ids column (but no longer triggers an alert) to
+     keep the backward compatibility with current systems that may be
+     reading it from that column but, this behavior will be removed
+     in the future.
 
 OVN v20.03.0 - xx xxx xxxx
 --------------------------

TODO.rst

@@ -149,3 +149,7 @@ OVN To-do List
 * OVN Interconnection
 
   * Packaging for RHEL, Debian, etc.
+
+* ovn-controller: Stop copying the local OVS configuration into the
+  Chassis external_ids column (same for the "is-remote" configuration from
+  ovn-ic) a few releases after the 20.06 version (21.06 maybe ?).

controller/bfd.c

@@ -152,7 +152,7 @@ bfd_calculate_chassis(
             /* It's an HA chassis. So add the ref_chassis to the bfd set. */
             for (size_t i = 0; i < ha_chassis_grp->n_ref_chassis; i++) {
                 struct sbrec_chassis *ref_ch = ha_chassis_grp->ref_chassis[i];
-                if (smap_get_bool(&ref_ch->external_ids, "is-remote", false)) {
+                if (smap_get_bool(&ref_ch->other_config, "is-remote", false)) {
                     continue;
                 }
                 sset_add(&grp_chassis, ref_ch->name);

controller/chassis.c

@@ -299,24 +299,24 @@ chassis_parse_ovs_config(const struct ovsrec_open_vswitch_table *ovs_table,
 }
 
 static void
-chassis_build_external_ids(struct smap *ext_ids, const char *bridge_mappings,
+chassis_build_other_config(struct smap *config, const char *bridge_mappings,
                            const char *datapath_type, const char *cms_options,
                            const char *chassis_macs, const char *iface_types,
                            bool is_interconn)
 {
-    smap_replace(ext_ids, "ovn-bridge-mappings", bridge_mappings);
-    smap_replace(ext_ids, "datapath-type", datapath_type);
-    smap_replace(ext_ids, "ovn-cms-options", cms_options);
-    smap_replace(ext_ids, "iface-types", iface_types);
-    smap_replace(ext_ids, "ovn-chassis-mac-mappings", chassis_macs);
-    smap_replace(ext_ids, "is-interconn", is_interconn ? "true" : "false");
+    smap_replace(config, "ovn-bridge-mappings", bridge_mappings);
+    smap_replace(config, "datapath-type", datapath_type);
+    smap_replace(config, "ovn-cms-options", cms_options);
+    smap_replace(config, "iface-types", iface_types);
+    smap_replace(config, "ovn-chassis-mac-mappings", chassis_macs);
+    smap_replace(config, "is-interconn", is_interconn ? "true" : "false");
 }
 
 /*
  * Returns true if any external-id doesn't match the values in 'chassis-rec'.
  */
 static bool
-chassis_external_ids_changed(const char *bridge_mappings,
+chassis_other_config_changed(const char *bridge_mappings,
                              const char *datapath_type,
                              const char *cms_options,
                              const char *chassis_macs,
@@ -325,41 +325,41 @@ chassis_external_ids_changed(const char *bridge_mappings,
                              const struct sbrec_chassis *chassis_rec)
 {
     const char *chassis_bridge_mappings =
-        get_bridge_mappings(&chassis_rec->external_ids);
+        get_bridge_mappings(&chassis_rec->other_config);
 
     if (strcmp(bridge_mappings, chassis_bridge_mappings)) {
         return true;
     }
 
     const char *chassis_datapath_type =
-        smap_get_def(&chassis_rec->external_ids, "datapath-type", "");
+        smap_get_def(&chassis_rec->other_config, "datapath-type", "");
 
     if (strcmp(datapath_type, chassis_datapath_type)) {
         return true;
     }
 
     const char *chassis_cms_options =
-        get_cms_options(&chassis_rec->external_ids);
+        get_cms_options(&chassis_rec->other_config);
 
     if (strcmp(cms_options, chassis_cms_options)) {
         return true;
     }
 
     const char *chassis_mac_mappings =
-        get_chassis_mac_mappings(&chassis_rec->external_ids);
+        get_chassis_mac_mappings(&chassis_rec->other_config);
     if (strcmp(chassis_macs, chassis_mac_mappings)) {
         return true;
     }
 
     const char *chassis_iface_types =
-        smap_get_def(&chassis_rec->external_ids, "iface-types", "");
+        smap_get_def(&chassis_rec->other_config, "iface-types", "");
 
     if (strcmp(ds_cstr_ro(iface_types), chassis_iface_types)) {
         return true;
     }
 
     bool chassis_is_interconn =
-        smap_get_bool(&chassis_rec->external_ids, "is-interconn", false);
+        smap_get_bool(&chassis_rec->other_config, "is-interconn", false);
     if (chassis_is_interconn != is_interconn) {
         return true;
     }
@@ -538,25 +538,29 @@ chassis_update(const struct sbrec_chassis *chassis_rec,
         sbrec_chassis_set_hostname(chassis_rec, ovs_cfg->hostname);
     }
 
-    if (chassis_external_ids_changed(ovs_cfg->bridge_mappings,
+    if (chassis_other_config_changed(ovs_cfg->bridge_mappings,
                                      ovs_cfg->datapath_type,
                                      ovs_cfg->cms_options,
                                      ovs_cfg->chassis_macs,
                                      &ovs_cfg->iface_types,
                                      ovs_cfg->is_interconn,
                                      chassis_rec)) {
-        struct smap ext_ids;
+        struct smap other_config;
 
-        smap_clone(&ext_ids, &chassis_rec->external_ids);
-        chassis_build_external_ids(&ext_ids, ovs_cfg->bridge_mappings,
+        smap_clone(&other_config, &chassis_rec->other_config);
+        chassis_build_other_config(&other_config, ovs_cfg->bridge_mappings,
                                    ovs_cfg->datapath_type,
                                    ovs_cfg->cms_options,
                                    ovs_cfg->chassis_macs,
                                    ds_cstr_ro(&ovs_cfg->iface_types),
                                    ovs_cfg->is_interconn);
-        sbrec_chassis_verify_external_ids(chassis_rec);
-        sbrec_chassis_set_external_ids(chassis_rec, &ext_ids);
-        smap_destroy(&ext_ids);
+        sbrec_chassis_verify_other_config(chassis_rec);
+        sbrec_chassis_set_other_config(chassis_rec, &other_config);
+        /* TODO(lucasagomes): Continue writing the configuration to the
+         * external_ids column for backward compatibility with the current
+         * systems, this behavior should be removed in the future. */
+        sbrec_chassis_set_external_ids(chassis_rec, &other_config);
+        smap_destroy(&other_config);
     }
 
     update_chassis_transport_zones(transport_zones, chassis_rec);
@@ -626,7 +630,7 @@ chassis_get_mac(const struct sbrec_chassis *chassis_rec,
                 struct eth_addr *chassis_mac)
 {
     const char *tokens
-        = get_chassis_mac_mappings(&chassis_rec->external_ids);
+        = get_chassis_mac_mappings(&chassis_rec->other_config);
     if (!tokens[0]) {
        return false;
     }

controller/encaps.c

@@ -357,8 +357,8 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
                 continue;
             }
 
-            if (smap_get_bool(&chassis_rec->external_ids, "is-remote", false)
-                && !smap_get_bool(&this_chassis->external_ids, "is-interconn",
+            if (smap_get_bool(&chassis_rec->other_config, "is-remote", false)
+                && !smap_get_bool(&this_chassis->other_config, "is-interconn",
                                   false)) {
                 VLOG_DBG("Skipping encap creation for Chassis '%s' because "
                          "it is remote but this chassis is not interconn.",

controller/ovn-controller.8.xml

@@ -245,7 +245,7 @@
       <dd>
         This value is read from local OVS integration bridge row of
         <ref table="Bridge" db="Open_vSwitch"/> table and populated in
-        <ref key="datapath-type" table="Chassis" column="external_ids"
+        <ref key="datapath-type" table="Chassis" column="other_config"
         db="OVN_Southbound"/> of the <ref table="Chassis" db="OVN_Southbound"/>
         table in the OVN_Southbound database.
       </dd>

controller/ovn-controller.c

@@ -1811,10 +1811,6 @@ main(int argc, char *argv[])
      *  - DNS. pinctrl.c uses the external_ids column of DNS,
      *    which it shouldn't. This should be removed.
      *
-     *  - Chassis - chassis.c copies the chassis configuration from
-     *              local open_vswitch table to the external_ids of
-     *              chassis.
-     *
      *  - Datapath_binding - lflow.c is using this to check if the datapath
      *                       is switch or not. This should be removed.
      * */
@@ -1841,6 +1837,11 @@ main(int argc, char *argv[])
     ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_status);
     ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_connection_col_target);
 
+    /* Omit alerts to the Chassis external_ids column, the configuration
+     * from the local open_vswitch table has now being moved to the
+     * other_config column so we no longer need to monitor it */
+    ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_chassis_col_external_ids);
+
     update_sb_monitors(ovnsb_idl_loop.idl, NULL, NULL, NULL, false);
 
     stopwatch_create(CONTROLLER_LOOP_STOPWATCH_NAME, SW_MS);

controller/physical.c

@@ -424,7 +424,7 @@ populate_remote_chassis_macs(const struct sbrec_chassis *my_chassis,
         }
 
         const char *tokens
-            = get_chassis_mac_mappings(&chassis->external_ids);
+            = get_chassis_mac_mappings(&chassis->other_config);
 
         if (!strlen(tokens)) {
             continue;
@@ -1517,7 +1517,7 @@ physical_run(struct physical_ctx *p_ctx,
                 /*
                  * We split the tunnel_id to get the chassis-id
                  * and hash the tunnel list on the chassis-id. The
-                 * reason to use the chassis-id alone is because 
+                 * reason to use the chassis-id alone is because
                  * there might be cases (multicast, gateway chassis)
                  * where we need to tunnel to the chassis, but won't
                  * have the encap-ip specifically.

ic/ovn-ic.c

@@ -295,6 +295,10 @@ sync_isb_gw_to_sb(struct ic_context *ctx,
                   const struct sbrec_chassis *chassis)
 {
     sbrec_chassis_set_hostname(chassis, gw->hostname);
+    sbrec_chassis_update_other_config_setkey(chassis, "is-remote", "true");
+    /* TODO(lucasagomes): Continue writing the configuration to the
+     * external_ids column for backward compatibility with the current
+     * systems, this behavior should be removed in the future. */
     sbrec_chassis_update_external_ids_setkey(chassis, "is-remote", "true");
 
     /* Sync encaps used by this gateway. */
@@ -362,7 +366,7 @@ gateway_run(struct ic_context *ctx, const struct icsbrec_availability_zone *az)
 
     const struct sbrec_chassis *chassis;
     SBREC_CHASSIS_FOR_EACH (chassis, ctx->ovnsb_idl) {
-        if (smap_get_bool(&chassis->external_ids, "is-interconn", false)) {
+        if (smap_get_bool(&chassis->other_config, "is-interconn", false)) {
             gw = shash_find_and_delete(&local_gws, chassis->name);
             if (!gw) {
                 gw = icsbrec_gateway_insert(ctx->ovnisb_txn);
@@ -372,7 +376,7 @@ gateway_run(struct ic_context *ctx, const struct icsbrec_availability_zone *az)
             } else if (is_gateway_data_changed(gw, chassis)) {
                 sync_sb_gw_to_isb(ctx, chassis, gw);
             }
-        } else if (smap_get_bool(&chassis->external_ids, "is-remote", false)) {
+        } else if (smap_get_bool(&chassis->other_config, "is-remote", false)) {
             gw = shash_find_and_delete(&remote_gws, chassis->name);
             if (!gw) {
                 sbrec_chassis_delete(chassis);

northd/ovn-northd.c

@@ -11616,7 +11616,7 @@ update_northbound_cfg(struct northd_context *ctx,
         const struct sbrec_chassis *chassis;
         int64_t hv_cfg = nbg->nb_cfg;
         SBREC_CHASSIS_FOR_EACH (chassis, ctx->ovnsb_idl) {
-            if (!smap_get_bool(&chassis->external_ids, "is-remote", false) &&
+            if (!smap_get_bool(&chassis->other_config, "is-remote", false) &&
                 chassis->nb_cfg < hv_cfg) {
                 hv_cfg = chassis->nb_cfg;
             }
@@ -11901,7 +11901,7 @@ main(int argc, char *argv[])
     ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_chassis);
     ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_nb_cfg);
     ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_name);
-    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_external_ids);
+    ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_chassis_col_other_config);
 
     ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_ha_chassis);
     add_column_noalert(ovnsb_idl_loop.idl,