On Mon, Nov 21, 2022 at 3:00 PM Mark Michelson <[email protected]> wrote:
>
> On 10/18/22 14:33, Ihar Hrachyshka wrote:
> > Before the patch, all controller instances were reading configuration
> > from the same external-ids:ovn-* options. This patch adds support for
> > distinct config otions for different chassis names stored in the same
> > ovsdb global config object.
> >
> > To configure an option for a distinct chassis name, an admin may add a
> > suffix with the desired chassis name to a config option. For example, if
> > the following is configured in ovsdb, only a controller with the
> > corresponding chassis name (either 'hv1' or 'hv2') would read just one
> > of the following options:
> >
> > ovs-vsctl set open . external-ids:ovn-bridge-mappings-hv1=phys:br-phys-1
> > ovs-vsctl set open . external-ids:ovn-bridge-mappings-hv2=phys:br-phys-2
> >
> > Chassis specific config options override any global settings, so for
> > example if the following configuration is used, then controller 'hv1'
> > will use the first setting but not the latter. Any other controllers
> > will use the global setting, which is the second setting..
> >
> > ovs-vsctl set open . external-ids:ovn-bridge-mappings-hv1=phys:br-phys-1
> > ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys-2
> >
> > This is supported for other options too.
> >
> > This is in preparation to support running multiple controller instances
> > using the same vswitchd instance.
> >
> > Signed-off-by: Ihar Hrachyshka <[email protected]>
> > ---
> > controller/chassis.c | 153 +++++++++++++++++++++-----------
> > controller/chassis.h | 6 +-
> > controller/encaps.c | 19 ++--
> > controller/ovn-controller.8.xml | 12 +++
> > controller/ovn-controller.c | 123 ++++++++++++++-----------
> > controller/patch.c | 6 +-
> > controller/physical.c | 2 +-
> > lib/ovn-util.c | 87 ++++++++++++++++++
> > lib/ovn-util.h | 26 ++++++
> > tests/automake.mk | 1 +
> > tests/ovn-macros.at | 4 +-
> > tests/ovn.at | 41 +++++++++
> > 12 files changed, 361 insertions(+), 119 deletions(-)
> >
> > diff --git a/controller/chassis.c b/controller/chassis.c
> > index 685d9b2ae..c8a332786 100644
> > --- a/controller/chassis.c
> > +++ b/controller/chassis.c
> > @@ -93,9 +93,10 @@ chassis_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> > }
> >
> > static const char *
> > -get_hostname(const struct smap *ext_ids)
> > +get_hostname(const struct smap *ext_ids, const char *chassis_id)
> > {
> > - const char *hostname = smap_get_def(ext_ids, "hostname", "");
> > + const char *hostname = get_chassis_external_id_value(ext_ids,
> > chassis_id,
> > + "hostname", "");
>
> hostname is an interesting field on the chassis. The "requested-chassis"
> option for logical switch ports may refer to a chassis name or chassis
> hostname. The "hostname" column is not indexed, so there currently isn't
> anything stopping multiple chassis from having the same hostname.
> However, I suspect this tends to just work because people don't set the
> hostname in the OVSDB and instead rely on the fallback behavior of
> get_hostname(), which calls the system's gethostname() function instead.
> Since there is one chassis record per host, this results in each chassis
> record having a unique hostname.
>
> So now we introduce the situation where there can be multiple chassis
> records on the same host. This means that if the hostname is not
> configured in the OVSDB, then all chassis records on this host will have
> the same hostname value.
>
> This can be OK. If each chassis is running as part of a distinct OVN
> deployment (i.e. each chassis record is in a different southbound
> database), then there is no issue of ambiguity. Even though each chassis
> will have the same hostname, each ovn northd will only be able to see
> the chassis that is in its southbound DB. This is what I had assumed the
> target use case for multiple chassis on the same host is.
>
> However, if each chassis is part of the same OVN deployment (which is
> what the tests introduced in patches 5 and 6 do), then having the same
> hostname could be a problem, since "requested-chassis" can be ambiguous.
>
> I'm not sure what the best way to handle this is. One way is to just
> allow for the same hostname to be repeated across chassis records and
> just document that chassis names *must* be used for "requested-chassis".
> Another would be to append the chassis index to the hostname and store
> that in the chassis record's hostname field. Both ideas have drawbacks,
> and I'm not sure what is best.
>
I hate adding indices everywhere. A more reasonable approach is CMS /
deployment based: consistently use "virtual chassis" names, not
hostnames. I'll document it for requested-chassis option of LSP.
> >
> > if (strlen(hostname) == 0) {
> > static char hostname_[HOST_NAME_MAX + 1];
> > @@ -111,69 +112,81 @@ get_hostname(const struct smap *ext_ids)
> > }
> >
> > static const char *
> > -get_bridge_mappings(const struct smap *ext_ids)
> > +get_bridge_mappings(const struct smap *ext_ids, const char *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-bridge-mappings", "");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-bridge-mappings", "");
> > }
> >
> > const char *
> > -get_chassis_mac_mappings(const struct smap *ext_ids)
> > +get_chassis_mac_mappings(const struct smap *ext_ids, const char
> > *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-chassis-mac-mappings", "");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-chassis-mac-mappings", "");
> > }
> >
> > static const char *
> > -get_cms_options(const struct smap *ext_ids)
> > +get_cms_options(const struct smap *ext_ids, const char *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-cms-options", "");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-cms-options", "");
> > }
> >
> > static const char *
> > -get_monitor_all(const struct smap *ext_ids)
> > +get_monitor_all(const struct smap *ext_ids, const char *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-monitor-all", "false");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-monitor-all", "false");
> > }
> >
> > static const char *
> > -get_enable_lflow_cache(const struct smap *ext_ids)
> > +get_enable_lflow_cache(const struct smap *ext_ids, const char *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-enable-lflow-cache", "true");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-enable-lflow-cache", "true");
> > }
> >
> > static const char *
> > -get_limit_lflow_cache(const struct smap *ext_ids)
> > +get_limit_lflow_cache(const struct smap *ext_ids, const char *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-limit-lflow-cache", "");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-limit-lflow-cache", "");
> > }
> >
> > static const char *
> > -get_memlimit_lflow_cache(const struct smap *ext_ids)
> > +get_memlimit_lflow_cache(const struct smap *ext_ids, const char
> > *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-memlimit-lflow-cache-kb", "");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-memlimit-lflow-cache-kb",
> > "");
> > }
> >
> > static const char *
> > -get_trim_limit_lflow_cache(const struct smap *ext_ids)
> > +get_trim_limit_lflow_cache(const struct smap *ext_ids, const char
> > *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-trim-limit-lflow-cache", "");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-trim-limit-lflow-cache", "");
> > }
> >
> > static const char *
> > -get_trim_wmark_perc_lflow_cache(const struct smap *ext_ids)
> > +get_trim_wmark_perc_lflow_cache(const struct smap *ext_ids,
> > + const char *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-trim-wmark-perc-lflow-cache", "");
> > + return get_chassis_external_id_value(
> > + ext_ids, chassis_id, "ovn-trim-wmark-perc-lflow-cache", "");
> > }
> >
> > static const char *
> > -get_trim_timeout(const struct smap *ext_ids)
> > +get_trim_timeout(const struct smap *ext_ids, const char *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-trim-timeout-ms", "");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-trim-timeout-ms", "");
> > }
> >
> > static const char *
> > -get_encap_csum(const struct smap *ext_ids)
> > +get_encap_csum(const struct smap *ext_ids, const char *chassis_id)
> > {
> > - return smap_get_def(ext_ids, "ovn-encap-csum", "true");
> > + return get_chassis_external_id_value(ext_ids, chassis_id,
> > + "ovn-encap-csum", "true");
> > }
> >
> > static const char *
> > @@ -187,9 +200,10 @@ get_datapath_type(const struct ovsrec_bridge *br_int)
> > }
> >
> > static bool
> > -get_is_interconn(const struct smap *ext_ids)
> > +get_is_interconn(const struct smap *ext_ids, const char *chassis_id)
> > {
> > - return smap_get_bool(ext_ids, "ovn-is-interconn", false);
> > + return get_chassis_external_id_value_bool(ext_ids, chassis_id,
> > + "ovn-is-interconn", false);
> > }
> >
> > static void
> > @@ -260,6 +274,23 @@ chassis_parse_ovs_iface_types(char **iface_types,
> > size_t n_iface_types,
> > return true;
> > }
> >
> > +const char *
> > +get_ovs_chassis_id(const struct ovsrec_open_vswitch_table *ovs_table)
> > +{
> > + const struct ovsrec_open_vswitch *cfg =
> > + ovsrec_open_vswitch_table_first(ovs_table);
> > + const char *chassis_id = cfg
> > + ? smap_get(&cfg->external_ids, "system-id")
> > + : NULL;
> > +
> > + if (!chassis_id) {
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > + VLOG_WARN_RL(&rl, "'system-id' in Open_vSwitch database is
> > missing.");
> > + }
> > +
> > + return chassis_id;
> > +}
> > +
> > /*
> > * Parse the 'ovs_table' entry and populate 'ovs_cfg'.
> > */
> > @@ -276,30 +307,41 @@ chassis_parse_ovs_config(const struct
> > ovsrec_open_vswitch_table *ovs_table,
> > return false;
> > }
> >
> > - const char *encap_type = smap_get(&cfg->external_ids,
> > "ovn-encap-type");
> > - const char *encap_ips = smap_get(&cfg->external_ids, "ovn-encap-ip");
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > + const char *encap_type =
> > + get_chassis_external_id_value(&cfg->external_ids, chassis_id,
> > + "ovn-encap-type", NULL);
> > +
> > + const char *encap_ips =
> > + get_chassis_external_id_value(&cfg->external_ids, chassis_id,
> > + "ovn-encap-ip", NULL);
> > if (!encap_type || !encap_ips) {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > VLOG_INFO_RL(&rl, "Need to specify an encap type and ip");
> > return false;
> > }
> >
> > - ovs_cfg->hostname = get_hostname(&cfg->external_ids);
> > - ovs_cfg->bridge_mappings = get_bridge_mappings(&cfg->external_ids);
> > + ovs_cfg->hostname = get_hostname(&cfg->external_ids, chassis_id);
> > + ovs_cfg->bridge_mappings =
> > + get_bridge_mappings(&cfg->external_ids, chassis_id);
> > ovs_cfg->datapath_type = get_datapath_type(br_int);
> > - ovs_cfg->encap_csum = get_encap_csum(&cfg->external_ids);
> > - ovs_cfg->cms_options = get_cms_options(&cfg->external_ids);
> > - ovs_cfg->monitor_all = get_monitor_all(&cfg->external_ids);
> > - ovs_cfg->chassis_macs = get_chassis_mac_mappings(&cfg->external_ids);
> > - ovs_cfg->enable_lflow_cache =
> > get_enable_lflow_cache(&cfg->external_ids);
> > - ovs_cfg->limit_lflow_cache = get_limit_lflow_cache(&cfg->external_ids);
> > + ovs_cfg->encap_csum = get_encap_csum(&cfg->external_ids, chassis_id);
> > + ovs_cfg->cms_options = get_cms_options(&cfg->external_ids, chassis_id);
> > + ovs_cfg->monitor_all = get_monitor_all(&cfg->external_ids, chassis_id);
> > + ovs_cfg->chassis_macs =
> > + get_chassis_mac_mappings(&cfg->external_ids, chassis_id);
> > + ovs_cfg->enable_lflow_cache =
> > + get_enable_lflow_cache(&cfg->external_ids, chassis_id);
> > + ovs_cfg->limit_lflow_cache =
> > + get_limit_lflow_cache(&cfg->external_ids, chassis_id);
> > ovs_cfg->memlimit_lflow_cache =
> > - get_memlimit_lflow_cache(&cfg->external_ids);
> > + get_memlimit_lflow_cache(&cfg->external_ids, chassis_id);
> > ovs_cfg->trim_limit_lflow_cache =
> > - get_trim_limit_lflow_cache(&cfg->external_ids);
> > + get_trim_limit_lflow_cache(&cfg->external_ids, chassis_id);
> > ovs_cfg->trim_wmark_perc_lflow_cache =
> > - get_trim_wmark_perc_lflow_cache(&cfg->external_ids);
> > - ovs_cfg->trim_timeout_ms = get_trim_timeout(&cfg->external_ids);
> > + get_trim_wmark_perc_lflow_cache(&cfg->external_ids, chassis_id);
> > + ovs_cfg->trim_timeout_ms =
> > + get_trim_timeout(&cfg->external_ids, chassis_id);
> >
> > if (!chassis_parse_ovs_encap_type(encap_type,
> > &ovs_cfg->encap_type_set)) {
> > return false;
> > @@ -322,7 +364,7 @@ chassis_parse_ovs_config(const struct
> > ovsrec_open_vswitch_table *ovs_table,
> > sset_destroy(&ovs_cfg->encap_ip_set);
> > }
> >
> > - ovs_cfg->is_interconn = get_is_interconn(&cfg->external_ids);
> > + ovs_cfg->is_interconn = get_is_interconn(&cfg->external_ids,
> > chassis_id);
> >
> > return true;
> > }
> > @@ -362,7 +404,7 @@ chassis_other_config_changed(const struct
> > ovs_chassis_cfg *ovs_cfg,
> > const struct sbrec_chassis *chassis_rec)
> > {
> > const char *chassis_bridge_mappings =
> > - get_bridge_mappings(&chassis_rec->other_config);
> > + get_bridge_mappings(&chassis_rec->other_config, chassis_rec->name);
> >
> > if (strcmp(ovs_cfg->bridge_mappings, chassis_bridge_mappings)) {
> > return true;
> > @@ -376,42 +418,44 @@ chassis_other_config_changed(const struct
> > ovs_chassis_cfg *ovs_cfg,
> > }
> >
> > const char *chassis_cms_options =
> > - get_cms_options(&chassis_rec->other_config);
> > + get_cms_options(&chassis_rec->other_config, chassis_rec->name);
> >
> > if (strcmp(ovs_cfg->cms_options, chassis_cms_options)) {
> > return true;
> > }
> >
> > const char *chassis_monitor_all =
> > - get_monitor_all(&chassis_rec->other_config);
> > + get_monitor_all(&chassis_rec->other_config, chassis_rec->name);
> >
> > if (strcmp(ovs_cfg->monitor_all, chassis_monitor_all)) {
> > return true;
> > }
> >
> > const char *chassis_enable_lflow_cache =
> > - get_enable_lflow_cache(&chassis_rec->other_config);
> > + get_enable_lflow_cache(&chassis_rec->other_config,
> > chassis_rec->name);
> >
> > if (strcmp(ovs_cfg->enable_lflow_cache, chassis_enable_lflow_cache)) {
> > return true;
> > }
> >
> > const char *chassis_limit_lflow_cache =
> > - get_limit_lflow_cache(&chassis_rec->other_config);
> > + get_limit_lflow_cache(&chassis_rec->other_config,
> > chassis_rec->name);
> >
> > if (strcmp(ovs_cfg->limit_lflow_cache, chassis_limit_lflow_cache)) {
> > return true;
> > }
> >
> > const char *chassis_memlimit_lflow_cache =
> > - get_memlimit_lflow_cache(&chassis_rec->other_config);
> > + get_memlimit_lflow_cache(&chassis_rec->other_config,
> > + chassis_rec->name);
> >
> > if (strcmp(ovs_cfg->memlimit_lflow_cache,
> > chassis_memlimit_lflow_cache)) {
> > return true;
> > }
> >
> > const char *chassis_trim_limit_lflow_cache =
> > - get_trim_limit_lflow_cache(&chassis_rec->other_config);
> > + get_trim_limit_lflow_cache(&chassis_rec->other_config,
> > + chassis_rec->name);
> >
> > if (strcmp(ovs_cfg->trim_limit_lflow_cache,
> > chassis_trim_limit_lflow_cache)) {
> > @@ -419,7 +463,8 @@ chassis_other_config_changed(const struct
> > ovs_chassis_cfg *ovs_cfg,
> > }
> >
> > const char *chassis_trim_wmark_perc_lflow_cache =
> > - get_trim_wmark_perc_lflow_cache(&chassis_rec->other_config);
> > + get_trim_wmark_perc_lflow_cache(
> > + &chassis_rec->other_config, chassis_rec->name);
> >
> > if (strcmp(ovs_cfg->trim_wmark_perc_lflow_cache,
> > chassis_trim_wmark_perc_lflow_cache)) {
> > @@ -427,14 +472,15 @@ chassis_other_config_changed(const struct
> > ovs_chassis_cfg *ovs_cfg,
> > }
> >
> > const char *chassis_trim_timeout_ms =
> > - get_trim_timeout(&chassis_rec->other_config);
> > + get_trim_timeout(&chassis_rec->other_config, chassis_rec->name);
> >
> > if (strcmp(ovs_cfg->trim_timeout_ms, chassis_trim_timeout_ms)) {
> > return true;
> > }
> >
> > const char *chassis_mac_mappings =
> > - get_chassis_mac_mappings(&chassis_rec->other_config);
> > + get_chassis_mac_mappings(&chassis_rec->other_config,
> > + chassis_rec->name);
> > if (strcmp(ovs_cfg->chassis_macs, chassis_mac_mappings)) {
> > return true;
> > }
> > @@ -754,8 +800,9 @@ chassis_get_mac(const struct sbrec_chassis *chassis_rec,
> > const char *bridge_mapping,
> > struct eth_addr *chassis_mac)
> > {
> > - const char *tokens
> > - = get_chassis_mac_mappings(&chassis_rec->other_config);
> > + const char *tokens =
> > + get_chassis_mac_mappings(&chassis_rec->other_config,
> > + chassis_rec->name);
> > if (!tokens[0]) {
> > return false;
> > }
> > diff --git a/controller/chassis.h b/controller/chassis.h
> > index 18b45a1c5..05a96bb0c 100644
> > --- a/controller/chassis.h
> > +++ b/controller/chassis.h
> > @@ -46,7 +46,9 @@ 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 * get_chassis_mac_mappings(const struct smap *ext_ids);
> > -
> > +const char * get_chassis_mac_mappings(const struct smap *ext_ids,
> > + const char *chassis_id);
> > +const char *
> > +get_ovs_chassis_id(const struct ovsrec_open_vswitch_table *ovs_table);
> >
> > #endif /* controller/chassis.h */
> > diff --git a/controller/encaps.c b/controller/encaps.c
> > index a82088d42..0bc887983 100644
> > --- a/controller/encaps.c
> > +++ b/controller/encaps.c
> > @@ -194,24 +194,29 @@ tunnel_add(struct tunnel_ctx *tc, const struct
> > sbrec_sb_global *sbg,
> > bool set_local_ip = false;
> > if (cfg) {
> > /* If the tos option is configured, get it */
> > - const char *encap_tos = smap_get_def(&cfg->external_ids,
> > - "ovn-encap-tos", "none");
> > + const char *encap_tos =
> > + get_chassis_external_id_value(
> > + &cfg->external_ids, tc->this_chassis->name,
> > + "ovn-encap-tos", "none");
> >
> > if (encap_tos && strcmp(encap_tos, "none")) {
> > smap_add(&options, "tos", encap_tos);
> > }
> >
> > /* If the df_default option is configured, get it */
> > -
> > - const char *encap_df = smap_get(&cfg->external_ids,
> > - "ovn-encap-df_default");
> > + const char *encap_df =
> > + get_chassis_external_id_value(
> > + &cfg->external_ids, tc->this_chassis->name,
> > + "ovn-encap-df_default", NULL);
> > if (encap_df) {
> > smap_add(&options, "df_default", encap_df);
> > }
> >
> > /* If ovn-set-local-ip option is configured, get it */
> > - set_local_ip = smap_get_bool(&cfg->external_ids,
> > "ovn-set-local-ip",
> > - false);
> > + set_local_ip =
> > + get_chassis_external_id_value_bool(
> > + &cfg->external_ids, tc->this_chassis->name,
> > + "ovn-set-local-ip", false);
> > }
> >
> > /* Add auth info if ipsec is enabled. */
> > diff --git a/controller/ovn-controller.8.xml
> > b/controller/ovn-controller.8.xml
> > index eb2950723..19dfa0ff5 100644
> > --- a/controller/ovn-controller.8.xml
> > +++ b/controller/ovn-controller.8.xml
> > @@ -354,6 +354,18 @@
> > </dd>
> > </dl>
> >
> > + <p>
> > + Most of configuration options listed above can also be set for a
> > + particular chassis name (see <code>external_ids:system-id </code>
> > + for more information). This can be achieved by setting
> > + <code>external_ids:option-[chassis]</code> instead of
> > + <code>external_ids:option</code>. For example, set
> > + <code>external_ids:ovn-encap-ip-otherhv</code> to use a particular
> > + IP address for the controller instance named <code>otherhv</code>.
> > + Name specific configuration options always override any global
> > options
> > + set in the database.
> > + </p>
> > +
> > <p>
> > <code>ovn-controller</code> reads the following values from the
> > <code>Open_vSwitch</code> database of the local OVS instance:
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 5c2a4b6ef..f40633517 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -311,9 +311,13 @@ out:;
> > }
> >
> > static const char *
> > -br_int_name(const struct ovsrec_open_vswitch *cfg)
> > +br_int_name(const struct ovsrec_open_vswitch_table *ovs_table)
> > {
> > - return smap_get_def(&cfg->external_ids, "ovn-bridge",
> > DEFAULT_BRIDGE_NAME);
> > + const struct ovsrec_open_vswitch *cfg =
> > + ovsrec_open_vswitch_table_first(ovs_table);
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > + return get_chassis_external_id_value(&cfg->external_ids, chassis_id,
> > + "ovn-bridge",
> > DEFAULT_BRIDGE_NAME);
>
> It's probably a bad idea to allow for the same ovn-bridge to be used by
> multiple chassis on a host. If the chassis are part of the same
> deployment, then we may end up writing duplicate flows into the bridge.
> If they are part of different deployments, then all hell will break loose :)
>
> One way to fix this is to append the chassis index that is calculated in
> patch 1 onto the configured bridge name so that we guarantee separate
> bridges for each chassis. The resulting bridge would not match the
> configured bridge name, but it will guarantee consistent behavior.
Again, I don't feel adding indices everywhere is a great idea. We had
to add indices to tunnel port names because ovs shares the name space
for those between bridges. The bridge name is different and can be
maintained by CMS / deployment tools. But I will update documentation
for ovn-bridge to clarify that it is expected to be unique when
multiple controllers are running on the same chassis.
>
> > }
> >
> > static const struct ovsrec_bridge *
> > @@ -325,7 +329,7 @@ create_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
> > if (!cfg) {
> > return NULL;
> > }
> > - const char *bridge_name = br_int_name(cfg);
> > + const char *bridge_name = br_int_name(ovs_table);
> >
> > ovsdb_idl_txn_add_comment(ovs_idl_txn,
> > "ovn-controller: creating integration bridge '%s'",
> > bridge_name);
> > @@ -410,7 +414,7 @@ get_br_int(const struct ovsrec_bridge_table
> > *bridge_table,
> > return NULL;
> > }
> >
> > - return get_bridge(bridge_table, br_int_name(cfg));
> > + return get_bridge(bridge_table, br_int_name(ovs_table));
> > }
> >
> > static const struct ovsrec_datapath *
> > @@ -449,8 +453,11 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
> > * Otherwise use the datapath-type set in br-int, if any.
> > * Finally, assume "system" datapath if none configured.
> > */
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > const char *datapath_type =
> > - smap_get(&cfg->external_ids, "ovn-bridge-datapath-type");
> > + get_chassis_external_id_value(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-bridge-datapath-type", NULL);
> >
> > if (!datapath_type) {
> > if (br_int->datapath_type[0]) {
> > @@ -478,22 +485,6 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
> > *br_int_ = br_int;
> > }
> >
> > -static const char *
> > -get_ovs_chassis_id(const struct ovsrec_open_vswitch_table *ovs_table)
> > -{
> > - const struct ovsrec_open_vswitch *cfg
> > - = ovsrec_open_vswitch_table_first(ovs_table);
> > - const char *chassis_id = cfg ? smap_get(&cfg->external_ids,
> > "system-id")
> > - : NULL;
> > -
> > - if (!chassis_id) {
> > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > - VLOG_WARN_RL(&rl, "'system-id' in Open_vSwitch database is
> > missing.");
> > - }
> > -
> > - return chassis_id;
> > -}
> > -
> > static void
> > update_ssl_config(const struct ovsrec_ssl_table *ssl_table)
> > {
> > @@ -517,10 +508,16 @@ static int
> > get_ofctrl_probe_interval(struct ovsdb_idl *ovs_idl)
> > {
> > const struct ovsrec_open_vswitch *cfg =
> > ovsrec_open_vswitch_first(ovs_idl);
> > - return !cfg ? OFCTRL_DEFAULT_PROBE_INTERVAL_SEC :
> > - smap_get_int(&cfg->external_ids,
> > - "ovn-openflow-probe-interval",
> > - OFCTRL_DEFAULT_PROBE_INTERVAL_SEC);
> > + if (!cfg) {
> > + return OFCTRL_DEFAULT_PROBE_INTERVAL_SEC;
> > + }
> > +
> > + const struct ovsrec_open_vswitch_table *ovs_table =
> > + ovsrec_open_vswitch_table_get(ovs_idl);
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > + return get_chassis_external_id_value_int(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-openflow-probe-interval", OFCTRL_DEFAULT_PROBE_INTERVAL_SEC);
> > }
> >
> > /* Retrieves the pointer to the OVN Southbound database from 'ovs_idl' and
> > @@ -537,18 +534,26 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct
> > ovsdb_idl *ovnsb_idl,
> > }
> >
> > /* Set remote based on user configuration. */
> > - const char *remote = smap_get(&cfg->external_ids, "ovn-remote");
> > + const struct ovsrec_open_vswitch_table *ovs_table =
> > + ovsrec_open_vswitch_table_get(ovs_idl);
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > + const char *remote =
> > + get_chassis_external_id_value(
> > + &cfg->external_ids, chassis_id, "ovn-remote", NULL);
> > ovsdb_idl_set_remote(ovnsb_idl, remote, true);
> >
> > /* Set probe interval, based on user configuration and the remote. */
> > int default_interval = (remote &&
> > !stream_or_pstream_needs_probes(remote)
> > ? 0 : DEFAULT_PROBE_INTERVAL_MSEC);
> > - int interval = smap_get_int(&cfg->external_ids,
> > - "ovn-remote-probe-interval",
> > default_interval);
> > + int interval =
> > + get_chassis_external_id_value_int(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-remote-probe-interval", default_interval);
> > ovsdb_idl_set_probe_interval(ovnsb_idl, interval);
> >
> > - bool monitor_all = smap_get_bool(&cfg->external_ids, "ovn-monitor-all",
> > - false);
> > + bool monitor_all =
> > + get_chassis_external_id_value_bool(
> > + &cfg->external_ids, chassis_id, "ovn-monitor-all", false);
> > if (monitor_all) {
> > /* Always call update_sb_monitors when monitor_all is true.
> > * Otherwise, don't call it here, because there would be
> > unnecessary
> > @@ -571,25 +576,31 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct
> > ovsdb_idl *ovnsb_idl,
> > }
> >
> > if (ctx) {
> > - lflow_cache_enable(ctx->lflow_cache,
> > - smap_get_bool(&cfg->external_ids,
> > - "ovn-enable-lflow-cache",
> > - true),
> > - smap_get_uint(&cfg->external_ids,
> > - "ovn-limit-lflow-cache",
> > - DEFAULT_LFLOW_CACHE_MAX_ENTRIES),
> > - smap_get_ullong(&cfg->external_ids,
> > - "ovn-memlimit-lflow-cache-kb",
> > - DEFAULT_LFLOW_CACHE_MAX_MEM_KB),
> > - smap_get_uint(&cfg->external_ids,
> > - "ovn-trim-limit-lflow-cache",
> > - DEFAULT_LFLOW_CACHE_TRIM_LIMIT),
> > - smap_get_uint(&cfg->external_ids,
> > - "ovn-trim-wmark-perc-lflow-cache",
> > - DEFAULT_LFLOW_CACHE_WMARK_PERC),
> > - smap_get_uint(&cfg->external_ids,
> > - "ovn-trim-timeout-ms",
> > - DEFAULT_LFLOW_CACHE_TRIM_TO_MS));
> > + lflow_cache_enable(
> > + ctx->lflow_cache,
> > + get_chassis_external_id_value_bool(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-enable-lflow-cache", true),
> > + get_chassis_external_id_value_uint(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-limit-lflow-cache",
> > + DEFAULT_LFLOW_CACHE_MAX_ENTRIES),
> > + get_chassis_external_id_value_ullong(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-memlimit-lflow-cache-kb",
> > + DEFAULT_LFLOW_CACHE_MAX_MEM_KB),
> > + get_chassis_external_id_value_uint(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-trim-limit-lflow-cache",
> > + DEFAULT_LFLOW_CACHE_TRIM_LIMIT),
> > + get_chassis_external_id_value_uint(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-trim-wmark-perc-lflow-cache",
> > + DEFAULT_LFLOW_CACHE_WMARK_PERC),
> > + get_chassis_external_id_value_uint(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-trim-timeout-ms",
> > + DEFAULT_LFLOW_CACHE_TRIM_TO_MS));
> > }
> > }
> >
> > @@ -812,7 +823,7 @@ restore_ct_zones(const struct ovsrec_bridge_table
> > *bridge_table,
> > }
> >
> > const struct ovsrec_bridge *br_int;
> > - br_int = get_bridge(bridge_table, br_int_name(cfg));
> > + br_int = get_bridge(bridge_table, br_int_name(ovs_table));
> > if (!br_int) {
> > /* If the integration bridge hasn't been defined, assume that
> > * any existing ct-zone definitions aren't valid. */
> > @@ -921,7 +932,9 @@ get_transport_zones(const struct
> > ovsrec_open_vswitch_table *ovs_table)
> > {
> > const struct ovsrec_open_vswitch *cfg
> > = ovsrec_open_vswitch_table_first(ovs_table);
> > - return smap_get_def(&cfg->external_ids, "ovn-transport-zones", "");
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > + return get_chassis_external_id_value(&cfg->external_ids, chassis_id,
> > + "ovn-transport-zones", "");
> > }
> >
> > static void
> > @@ -3494,8 +3507,12 @@ check_northd_version(struct ovsdb_idl *ovs_idl,
> > struct ovsdb_idl *ovnsb_idl,
> > static bool version_mismatch;
> >
> > const struct ovsrec_open_vswitch *cfg =
> > ovsrec_open_vswitch_first(ovs_idl);
> > - if (!cfg || !smap_get_bool(&cfg->external_ids,
> > "ovn-match-northd-version",
> > - false)) {
> > + const struct ovsrec_open_vswitch_table *ovs_table =
> > + ovsrec_open_vswitch_table_get(ovs_idl);
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > + if (!cfg || !get_chassis_external_id_value_bool(
> > + &cfg->external_ids, chassis_id,
> > + "ovn-match-northd-version", false)) {
> > version_mismatch = false;
> > return true;
> > }
> > diff --git a/controller/patch.c b/controller/patch.c
> > index 12e0b6f7c..af4e8911f 100644
> > --- a/controller/patch.c
> > +++ b/controller/patch.c
> > @@ -15,6 +15,7 @@
> >
> > #include <config.h>
> >
> > +#include "chassis.h"
> > #include "patch.h"
> > #include "ovsport.h"
> >
> > @@ -131,7 +132,10 @@ add_ovs_bridge_mappings(const struct
> > ovsrec_open_vswitch_table *ovs_table,
> > const char *mappings_cfg;
> > char *cur, *next, *start;
> >
> > - mappings_cfg = smap_get(&cfg->external_ids, "ovn-bridge-mappings");
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > + mappings_cfg =
> > + get_chassis_external_id_value(
> > + &cfg->external_ids, chassis_id, "ovn-bridge-mappings",
> > NULL);
> > if (!mappings_cfg || !mappings_cfg[0]) {
> > return;
> > }
> > diff --git a/controller/physical.c b/controller/physical.c
> > index 705146316..a37e71f46 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -503,7 +503,7 @@ populate_remote_chassis_macs(const struct sbrec_chassis
> > *my_chassis,
> > }
> >
> > const char *tokens
> > - = get_chassis_mac_mappings(&chassis->other_config);
> > + = get_chassis_mac_mappings(&chassis->other_config,
> > chassis->name);
> >
> > if (!strlen(tokens)) {
> > continue;
> > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > index d80db179a..ae9481403 100644
> > --- a/lib/ovn-util.c
> > +++ b/lib/ovn-util.c
> > @@ -935,3 +935,90 @@ daemon_started_recently(void)
> > /* Ensure that at least an amount of time has passed. */
> > return time_wall_msec() - startup_ts <= DAEMON_STARTUP_DELAY_MS;
> > }
> > +
> > +const char *
> > +get_chassis_external_id_value(const struct smap *external_ids,
> > + const char *chassis_id,
> > + const char *option_key,
> > + const char *def)
> > +{
> > + const char *option_value = NULL;
> > + if (chassis_id) {
> > + char *chassis_option_key = xasprintf("%s-%s", option_key,
> > chassis_id);
> > + option_value = smap_get(external_ids, chassis_option_key);
> > + free(chassis_option_key);
> > + }
> > + if (!option_value) {
> > + option_value = smap_get_def(external_ids, option_key, def);
> > + }
> > + return option_value;
> > +}
> > +
> > +int
> > +get_chassis_external_id_value_int(const struct smap *external_ids,
> > + const char *chassis_id,
> > + const char *option_key,
> > + int def)
> > +{
> > + const char *value = get_chassis_external_id_value(
> > + external_ids, chassis_id, option_key, NULL);
> > +
> > + int i_value;
> > + if (!value || !str_to_int(value, 10, &i_value)) {
> > + return def;
> > + }
>
> These functions have an issue that they essentially copy logic that
> already exists in ovs's smap library. This was in an attempt to re-use
> the common get_chassis_external_id_value(), but I think copying the smap
> logic defeats the purpose.
>
> I think a better alternative would be to make a function that calculates
> the chassis-specific key, like:
>
> static char *
> chassis_option_key(const char *option_key, const char *chassis-id)
> {
> return xasprintf("%s-%s", option_key, chassis_id);
> }
>
> Then in each of these handlers, you could do something like:
>
> int
> get_chassis_external_id_value_int(const struct smap *external_ids,
> const char *chassis_id,
> const char *option_key,
> int def)
> {
> char *chassis_key = chassis_option_key(option_key, chassis_id);
> int ret = smap_get_int(external_ids, chassis_key,
> smap_get_int(external_ids, option_key, def));
> free(chassis_key);
> return ret;
> }
>
> I think this should work for each of the various
> get_chassis_external_id_value_* functions.
>
> > +
> > + return i_value;
> > +}
> > +
> > +unsigned int
> > +get_chassis_external_id_value_uint(const struct smap *external_ids,
> > + const char *chassis_id,
> > + const char *option_key,
> > + unsigned int def)
> > +{
> > + const char *value = get_chassis_external_id_value(
> > + external_ids, chassis_id, option_key, NULL);
> > +
> > + unsigned int u_value;
> > +
> > + if (!value || !str_to_uint(value, 10, &u_value)) {
> > + return def;
> > + }
> > +
> > + return u_value;
> > +}
> > +
> > +unsigned long long int
> > +get_chassis_external_id_value_ullong(const struct smap *external_ids,
> > + const char *chassis_id,
> > + const char *option_key,
> > + unsigned long long int def)
> > +{
> > + const char *value = get_chassis_external_id_value(
> > + external_ids, chassis_id, option_key, NULL);
> > +
> > + unsigned long long ull_value;
> > +
> > + if (!value || !str_to_ullong(value, 10, &ull_value)) {
> > + return def;
> > + }
> > +
> > + return ull_value;
> > +}
> > +
> > +bool
> > +get_chassis_external_id_value_bool(const struct smap *external_ids,
> > + const char *chassis_id,
> > + const char *option_key,
> > + bool def)
> > +{
> > + const char *value = get_chassis_external_id_value(
> > + external_ids, chassis_id, option_key, "");
> > +
> > + if (def) {
> > + return strcasecmp("false", value) != 0;
> > + } else {
> > + return !strcasecmp("true", value);
> > + }
> > +}
> > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > index 145f974ed..2a19faa43 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -314,4 +314,30 @@ void daemon_started_recently_ignore(void);
> > bool daemon_started_recently(void);
> > int64_t daemon_startup_ts(void);
> >
> > +const char *
> > +get_chassis_external_id_value(const struct smap *,
> > + const char *chassis_id,
> > + const char *option_key,
> > + const char *def);
> > +int
> > +get_chassis_external_id_value_int(const struct smap *,
> > + const char *chassis_id,
> > + const char *option_key,
> > + int def);
> > +unsigned int
> > +get_chassis_external_id_value_uint(const struct smap *,
> > + const char *chassis_id,
> > + const char *option_key,
> > + unsigned int def);
> > +unsigned long long int
> > +get_chassis_external_id_value_ullong(const struct smap *external_ids,
> > + const char *chassis_id,
> > + const char *option_key,
> > + unsigned long long int def);
> > +bool
> > +get_chassis_external_id_value_bool(const struct smap *,
> > + const char *chassis_id,
> > + const char *option_key,
> > + bool def);
> > +
> > #endif /* OVN_UTIL_H */
> > diff --git a/tests/automake.mk b/tests/automake.mk
> > index dce9c9108..d9f2777f3 100644
> > --- a/tests/automake.mk
> > +++ b/tests/automake.mk
> > @@ -253,6 +253,7 @@ tests_ovstest_SOURCES = \
> > tests_ovstest_LDADD = $(OVS_LIBDIR)/daemon.lo \
> > $(OVS_LIBDIR)/libopenvswitch.la lib/libovn.la \
> > controller/binding.$(OBJEXT) \
> > + controller/chassis.$(OBJEXT) \
> > controller/encaps.$(OBJEXT) \
> > controller/ha-chassis.$(OBJEXT) \
> > controller/if-status.$(OBJEXT) \
> > diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> > index a3c7f8125..35b7b3872 100644
> > --- a/tests/ovn-macros.at
> > +++ b/tests/ovn-macros.at
> > @@ -290,7 +290,7 @@ net_attach () {
> >
> > # ovn_az_attach AZ NETWORK BRIDGE IP [MASKLEN]
> > ovn_az_attach() {
> > - local az=$1 net=$2 bridge=$3 ip=$4 masklen=${5-24}
> > encap=${6-geneve,vxlan}
> > + local az=$1 net=$2 bridge=$3 ip=$4 masklen=${5-24}
> > encap=${6-geneve,vxlan} systemid=${7-$sandbox}
> > net_attach $net $bridge || return 1
> >
> > mac=`ovs-vsctl get Interface $bridge mac_in_use | sed s/\"//g`
> > @@ -310,7 +310,7 @@ ovn_az_attach() {
> > fi
> > ovs-vsctl \
> > -- set Open_vSwitch . external-ids:hostname=$sandbox \
> > - -- set Open_vSwitch . external-ids:system-id=$sandbox \
> > + -- set Open_vSwitch . external-ids:system-id=$systemid \
> > -- set Open_vSwitch . external-ids:ovn-remote=$ovn_remote \
> > -- set Open_vSwitch . external-ids:ovn-encap-type=$encap \
> > -- set Open_vSwitch . external-ids:ovn-encap-ip=$ip \
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index f8b8db4df..9aac857f4 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -32975,3 +32975,44 @@ check ovn-nbctl --wait=hv sync
> > OVN_CLEANUP([hv1])
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([chassis-specific configuration options])
> > +ovn_start
> > +net_add n1
> > +
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +
> > +ovs-vsctl \
> > + -- set Open_vSwitch . external-ids:ovn-encap-type-hv3=geneve \
> > + -- set Open_vSwitch . external-ids:ovn-encap-ip-hv3=192.168.1.1
> > +
> > +as hv1 ovs-vsctl set-ssl \
> > + $PKIDIR/testpki-hv3-privkey.pem \
> > + $PKIDIR/testpki-hv3-cert.pem \
> > + $PKIDIR/testpki-cacert.pem
> > +
> > +ovn_attach n1 br-phys 192.168.0.1 24 vxlan hv3
> > +
> > +sim_add hv2
> > +as hv2
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.2 24 geneve
> > +
> > +# despite that we configured ovn-encap-ip=192.168.0.1, this setting is
> > +# overridden by chassis specific ovn-encap-ip-hv3
> > +OVS_WAIT_UNTIL([
> > + test "1" = "$(ovn-sbctl list Encap | grep -c '192.168.1.1')"
> > +])
> > +
> > +encap_hv3_ip=$(fetch_column Encap ip chassis_name=hv3 type=geneve)
> > +AT_CHECK(test x$encap_hv3_ip == x192.168.1.1)
>
> You can replace everything from the OVS_WAIT_UNTIL up to this point with:
>
> wait_column 192.168.1.1 Encap ip chassis_name=hv3 type=geneve
>
> > +
> > +encap_hv1_ip=$(fetch_column Encap ip chassis_name=hv1 type=vxlan)
> > +AT_CHECK(test x$encap_hv1_ip == x)
>
> You can replace the above two lines with:
>
> check_column "" Encap ip chassis_name=hv1 type=vxlan
>
> > +
> > +OVN_CLEANUP([hv1],[hv2])
> > +AT_CLEANUP
> > +])
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
