Thanks Lucas! The only problem I see is that we still need something like the Private_Chassis approach that we discussed or otherwise bumping nb_cfg will still be generating N notifications (N == num_chassis) on every write. Still, this solves part of the problem.
I was wondering if we could leverage the Controller_Event table [0] for this purpose and generate an event to health check the chassis. Looks like ovn-controller monitors only its own chassis so we could be good here. What do you think? [0] https://mail.openvswitch.org/pipermail/ovs-dev/2019-June/359826.html On Thu, May 7, 2020 at 12:12 PM <[email protected]> wrote: > From: Lucas Alvares Gomes <[email protected]> > > 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]> > --- > 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 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
