Hi Ihar,

see some comments below.


On Tue, Sep 20, 2022 at 2:05 AM Ihar Hrachyshka <[email protected]> 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        | 151 +++++++++++++++++++++++-------------
>  controller/chassis.h        |   6 +-
>  controller/encaps.c         |  13 ++--
>  controller/ovn-controller.c | 121 ++++++++++++++++-------------
>  controller/patch.c          |   5 +-
>  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 ++++++++++
>  11 files changed, 337 insertions(+), 120 deletions(-)
>
> diff --git a/controller/chassis.c b/controller/chassis.c
> index 685d9b2ae..241913d1f 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", "");
>

>      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,22 @@ 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);
>

This has a wrong formatting:
    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;
>

Formatting:

 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 +306,39 @@ 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);
>

This should be formatted as follows:
    const char *encap_type =
        get_chassis_external_id_value(&cfg->external_ids, chassis_id,
                                      "ovn-encap-type", NULL);

This pattern repeats throughout the whole change, please fix it.


> +
> +    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 +361,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 +401,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 +415,42 @@ 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);
> +    const char *chassis_memlimit_lflow_cache = 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);
> +    const char *chassis_trim_limit_lflow_cache =
> 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 +458,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 +467,14 @@ 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);
> +    const char *chassis_mac_mappings = get_chassis_mac_mappings(
> +        &chassis_rec->other_config, chassis_rec->name);
>      if (strcmp(ovs_cfg->chassis_macs, chassis_mac_mappings)) {
>          return true;
>      }
> @@ -755,7 +795,8 @@ chassis_get_mac(const struct sbrec_chassis
> *chassis_rec,
>                  struct eth_addr *chassis_mac)
>  {
>      const char *tokens
> -        = get_chassis_mac_mappings(&chassis_rec->other_config);
> +        = 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 e6b2aa074..572c38542 100644
> --- a/controller/encaps.c
> +++ b/controller/encaps.c
> @@ -184,7 +184,8 @@ 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,
> +        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")) {
> @@ -193,15 +194,17 @@ tunnel_add(struct tunnel_ctx *tc, const struct
> sbrec_sb_global *sbg,
>
>          /* 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.c b/controller/ovn-controller.c
> index 43fbf2ba3..59ae732b3 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -310,9 +310,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);
>  }
>
>  static const struct ovsrec_bridge *
> @@ -324,7 +328,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);
> @@ -409,7 +413,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 *
> @@ -448,8 +452,10 @@ 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 *datapath_type =
> -                smap_get(&cfg->external_ids, "ovn-bridge-datapath-type");
> +            const char *chassis_id = get_ovs_chassis_id(ovs_table);
> +            const char *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]) {
> @@ -477,22 +483,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)
>  {
> @@ -516,10 +506,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
> @@ -536,18 +532,23 @@ 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
> @@ -570,25 +571,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));
>      }
>  }
>
> @@ -811,7 +818,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. */
> @@ -920,7 +927,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
> @@ -3464,8 +3473,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..166fb4678 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,9 @@ 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 f3c8bddce..6ce790f14 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;
> +    }
> +
> +    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);
> +    }
> +}
>

This feels like we are duplicating a bunch of code, but unfortunately I
don't have a better idea.


> 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 c03ff4f17..3614601e2 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -32889,3 +32889,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)
> +
> +encap_hv1_ip=$(fetch_column Encap ip chassis_name=hv1 type=vxlan)
> +AT_CHECK(test x$encap_hv1_ip == x)
> +
> +OVN_CLEANUP([hv1],[hv2])
> +AT_CLEANUP
> +])
> --
> 2.34.1
>
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Thanks,
Ales

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA <https://www.redhat.com>

[email protected]    IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to