[ovs-dev] [PATCH ovn] ovn-controller to no longer monitor Chassis' external_ids

lmartins at redhat.com lmartins at redhat.com
Thu May 7 10:12:16 UTC 2020


From: Lucas Alvares Gomes <lucasagomes at gmail.com>

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 <lucasagomes at gmail.com>
---
 NEWS                            | 10 ++++++-
 controller/bfd.c                |  2 +-
 controller/chassis.c            | 48 ++++++++++++++++++---------------
 controller/encaps.c             |  4 +--
 controller/ovn-controller.8.xml |  2 +-
 controller/ovn-controller.c     |  9 ++++---
 controller/physical.c           |  4 +--
 ic/ovn-ic.c                     |  6 ++---
 northd/ovn-northd.c             |  4 +--
 ovn-sb.ovsschema                |  7 +++--
 ovn-sb.xml                      | 14 +++++-----
 tests/ovn-controller.at         | 16 +++++------
 12 files changed, 71 insertions(+), 55 deletions(-)

diff --git a/NEWS b/NEWS
index c2b7945df..1b33be249 100644
--- a/NEWS
+++ b/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
 --------------------------
diff --git a/controller/bfd.c b/controller/bfd.c
index 2b1e87f6d..b305eb158 100644
--- a/controller/bfd.c
+++ b/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);
diff --git a/controller/chassis.c b/controller/chassis.c
index 522893ead..c1c609028 100644
--- a/controller/chassis.c
+++ b/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;
     }
diff --git a/controller/encaps.c b/controller/encaps.c
index 846628ac1..7eac4bb06 100644
--- a/controller/encaps.c
+++ b/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.",
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index 76bbbdc5f..92e0a6e43 100644
--- a/controller/ovn-controller.8.xml
+++ b/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>
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index a2d92429c..130081daa 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1790,10 +1790,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.
      * */
@@ -1820,6 +1816,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);
diff --git a/controller/physical.c b/controller/physical.c
index 144aeb7bd..f06313b9d 100644
--- a/controller/physical.c
+++ b/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.
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index a1ed25623..a0cf325bc 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -295,7 +295,7 @@ sync_isb_gw_to_sb(struct ic_context *ctx,
                   const struct sbrec_chassis *chassis)
 {
     sbrec_chassis_set_hostname(chassis, gw->hostname);
-    sbrec_chassis_update_external_ids_setkey(chassis, "is-remote", "true");
+    sbrec_chassis_update_other_config_setkey(chassis, "is-remote", "true");
 
     /* Sync encaps used by this gateway. */
     ovs_assert(gw->n_encaps);
@@ -362,7 +362,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 +372,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);
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3324d20f4..257c54caf 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -11645,7 +11645,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;
             }
@@ -11930,7 +11930,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,
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index d89f8dbbb..c196ddaf3 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Southbound",
-    "version": "2.7.0",
-    "cksum": "4286723485 21693",
+    "version": "2.8.0",
+    "cksum": "1643994484 21853",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -38,6 +38,9 @@
                 "external_ids": {
                     "type": {"key": "string", "value": "string",
                              "min": 0, "max": "unlimited"}},
+                "other_config": {
+                    "type": {"key": "string", "value": "string",
+                             "min": 0, "max": "unlimited"}},
                 "transport_zones" : {"type": {"key": "string",
                                               "min": 0,
                                               "max": "unlimited"}}},
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 3aa7cd4da..417424fb8 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -262,14 +262,14 @@
       from the <ref table="SB_Global"/> table into this column.
     </column>
 
-    <column name="external_ids" key="ovn-bridge-mappings">
+    <column name="other_config" key="ovn-bridge-mappings">
       <code>ovn-controller</code> populates this key with the set of bridge
       mappings it has been configured to use.  Other applications should treat
       this key as read-only.  See <code>ovn-controller</code>(8) for more
       information.
     </column>
 
-    <column name="external_ids" key="datapath-type">
+    <column name="other_config" key="datapath-type">
       <code>ovn-controller</code> populates this key with the datapath type
       configured in the <ref table="Bridge" column="datapath_type"/> column of
       the Open_vSwitch database's <ref table="Bridge" db="Open_vSwitch"/>
@@ -277,7 +277,7 @@
       <code>ovn-controller</code>(8) for more information.
     </column>
 
-    <column name="external_ids" key="iface-types">
+    <column name="other_config" key="iface-types">
       <code>ovn-controller</code> populates this key with the interface types
       configured in the <ref table="Open_vSwitch" column="iface_types"/> column
       of the Open_vSwitch database's <ref table="Open_vSwitch"
@@ -285,7 +285,7 @@
       read-only. See <code>ovn-controller</code>(8) for more information.
     </column>
 
-    <column name="external_ids" key="ovn-cms-options">
+    <column name="other_config" key="ovn-cms-options">
       <code>ovn-controller</code> populates this key with the set of options
       configured in the <ref table="Open_vSwitch"
       column="external_ids:ovn-cms-options"/> column of the Open_vSwitch
@@ -293,7 +293,7 @@
       See <code>ovn-controller</code>(8) for more information.
     </column>
 
-    <column name="external_ids" key="is-interconn">
+    <column name="other_config" key="is-interconn">
       <code>ovn-controller</code> populates this key with the setting
       configured in the <ref table="Open_vSwitch"
       column="external_ids:ovn-is-interconn"/> column of the Open_vSwitch
@@ -302,7 +302,7 @@
       See <code>ovn-controller</code>(8) for more information.
     </column>
 
-    <column name="external_ids" key="is-remote">
+    <column name="other_config" key="is-remote">
       <code>ovn-ic</code> set this key to true for remote interconnection
       gateway chassises learned from the interconnection southbound database.
       See <code>ovn-ic</code>(8) for more information.
@@ -316,7 +316,7 @@
       See <code>ovn-controller</code>(8) for more information.
     </column>
 
-    <column name="external_ids" key="ovn-chassis-mac-mappings">
+    <column name="other_config" key="ovn-chassis-mac-mappings">
       <code>ovn-controller</code> populates this key with the set of options
       configured in the <ref table="Open_vSwitch"
       column="external_ids:ovn-chassis-mac-mappings"/> column of the
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 63b2581c0..77936c776 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -51,7 +51,7 @@ patch
 check_bridge_mappings () {
     local_mappings=$1
     sysid=$(ovs-vsctl get Open_vSwitch . external_ids:system-id)
-    OVS_WAIT_UNTIL([test x"${local_mappings}" = x$(ovn-sbctl get Chassis ${sysid} external_ids:ovn-bridge-mappings | sed -e 's/\"//g')])
+    OVS_WAIT_UNTIL([test x"${local_mappings}" = x$(ovn-sbctl get Chassis ${sysid} other_config:ovn-bridge-mappings | sed -e 's/\"//g')])
 }
 
 # Initially there should be no patch ports.
@@ -118,8 +118,8 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 AT_CLEANUP
 
 # Checks that ovn-controller populates datapath-type and iface-types
-# correctly in the Chassis external-ids column.
-AT_SETUP([ovn-controller - Chassis external_ids])
+# correctly in the Chassis other_config column.
+AT_SETUP([ovn-controller - Chassis other_config])
 AT_KEYWORDS([ovn])
 ovn_init_db ovn-sb
 
@@ -139,7 +139,7 @@ sysid=$(ovs-vsctl get Open_vSwitch . external_ids:system-id)
 # is mirrored into the Chassis record in the OVN_Southbound db.
 check_datapath_type () {
     datapath_type=$1
-    chassis_datapath_type=$(ovn-sbctl get Chassis ${sysid} external_ids:datapath-type | sed -e 's/"//g') #"
+    chassis_datapath_type=$(ovn-sbctl get Chassis ${sysid} other_config:datapath-type | sed -e 's/"//g') #"
     test "${datapath_type}" = "${chassis_datapath_type}"
 }
 
@@ -174,15 +174,15 @@ OVS_WAIT_UNTIL([check_datapath_type foobar])
 
 expected_iface_types=$(ovs-vsctl get Open_vSwitch . iface_types | tr -d '[[]] ""')
 echo "expected_iface_types = ${expected_iface_types}"
-chassis_iface_types=$(ovn-sbctl get Chassis ${sysid} external_ids:iface-types | sed -e 's/\"//g')
+chassis_iface_types=$(ovn-sbctl get Chassis ${sysid} other_config:iface-types | sed -e 's/\"//g')
 echo "chassis_iface_types = ${chassis_iface_types}"
 AT_CHECK([test "${expected_iface_types}" = "${chassis_iface_types}"])
 
-# Change the value of external_ids:iface-types using ovn-sbctl.
+# Change the value of other_config:iface-types using ovn-sbctl.
 # ovn-controller should again set it back to proper one.
-ovn-sbctl set Chassis ${sysid} external_ids:iface-types="foo"
+ovn-sbctl set Chassis ${sysid} other_config:iface-types="foo"
 OVS_WAIT_UNTIL([
-    chassis_iface_types=$(ovn-sbctl get Chassis ${sysid} external_ids:iface-types | sed -e 's/\"//g')
+    chassis_iface_types=$(ovn-sbctl get Chassis ${sysid} other_config:iface-types | sed -e 's/\"//g')
     echo "chassis_iface_types = ${chassis_iface_types}"
     test "${expected_iface_types}" = "${chassis_iface_types}"
 ])
-- 
2.26.2



More information about the dev mailing list