[ovs-dev] [PATCH ovn 3/4] ovn-controller: Detect OVS datapath capabilities.

Dumitru Ceara dceara at redhat.com
Fri Jun 4 16:05:00 UTC 2021


On 6/4/21 5:55 PM, Mark Gray wrote:
> On 03/06/2021 16:05, Dumitru Ceara wrote:
>> Automatically create an OVS Datapath record if none exists for the
>> current br-int datapath type.
>>
>> Add a 'features' module to track which OVS features are available in
>> the datapath currently being used.  For now, only ct_zero_snat is
>> tracked, all other features are assumed to be on-par between all
>> datapaths.
>>
>> A future commit will make use of the 'features' module to conditionally
>> program openflows based on available datapath features.
>>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>> ---
>>  controller/ovn-controller.c |  125 ++++++++++++++++++++++++++++++++++---------
>>  include/ovn/features.h      |   16 ++++++
>>  lib/automake.mk             |    1 
>>  lib/features.c              |   39 +++++++++++++
>>  tests/ovn-controller.at     |   11 ++--
>>  5 files changed, 159 insertions(+), 33 deletions(-)
>>  create mode 100644 lib/features.c
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index d48ddc7a2..15af7a13c 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -46,6 +46,7 @@
>>  #include "openvswitch/vconn.h"
>>  #include "openvswitch/vlog.h"
>>  #include "ovn/actions.h"
>> +#include "ovn/features.h"
>>  #include "lib/chassis-index.h"
>>  #include "lib/extend-table.h"
>>  #include "lib/ip-mcast-index.h"
>> @@ -88,6 +89,7 @@ static unixctl_cb_func lflow_cache_show_stats_cmd;
>>  static unixctl_cb_func debug_delay_nb_cfg_report;
>>  
>>  #define DEFAULT_BRIDGE_NAME "br-int"
>> +#define DEFAULT_DATAPATH "system"
>>  #define DEFAULT_PROBE_INTERVAL_MSEC 5000
>>  #define OFCTRL_DEFAULT_PROBE_INTERVAL_SEC 0
>>  
>> @@ -95,6 +97,8 @@ static unixctl_cb_func debug_delay_nb_cfg_report;
>>  
>>  #define OVS_NB_CFG_NAME "ovn-nb-cfg"
>>  
>> +#define OVS_CT_ZERO_SNAT_FEATURE "ct_zero_snat"
>> +
>>  static char *parse_options(int argc, char *argv[]);
>>  OVS_NO_RETURN static void usage(void);
>>  
>> @@ -319,10 +323,6 @@ static const struct ovsrec_bridge *
>>  create_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>>                const struct ovsrec_open_vswitch_table *ovs_table)
>>  {
>> -    if (!ovs_idl_txn) {
>> -        return NULL;
>> -    }
>> -
>>      const struct ovsrec_open_vswitch *cfg;
>>      cfg = ovsrec_open_vswitch_table_first(ovs_table);
>>      if (!cfg) {
>> @@ -386,6 +386,21 @@ create_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>>      return bridge;
>>  }
>>  
>> +static const struct ovsrec_datapath *
>> +create_br_datapath(struct ovsdb_idl_txn *ovs_idl_txn,
>> +                   const struct ovsrec_open_vswitch *cfg,
>> +                   const char *datapath_type)
>> +{
>> +    ovsdb_idl_txn_add_comment(ovs_idl_txn,
>> +                              "ovn-controller: creating bridge datapath '%s'",
>> +                              datapath_type);
>> +
>> +    struct ovsrec_datapath *dp = ovsrec_datapath_insert(ovs_idl_txn);
>> +    ovsrec_open_vswitch_verify_datapaths(cfg);
>> +    ovsrec_open_vswitch_update_datapaths_setkey(cfg, datapath_type, dp);
>> +    return dp;
>> +}
>> +
>>  static const struct ovsrec_bridge *
>>  get_br_int(const struct ovsrec_bridge_table *bridge_table,
>>             const struct ovsrec_open_vswitch_table *ovs_table)
>> @@ -399,33 +414,78 @@ get_br_int(const struct ovsrec_bridge_table *bridge_table,
>>      return get_bridge(bridge_table, br_int_name(cfg));
>>  }
>>  
>> -static const struct ovsrec_bridge *
>> +static const struct ovsrec_datapath *
>> +get_br_datapath(const struct ovsrec_open_vswitch *cfg,
>> +                const char *datapath_type)
>> +{
>> +    for (size_t i = 0; i < cfg->n_datapaths; i++) {
>> +        if (!strcmp(cfg->key_datapaths[i], datapath_type)) {
>> +            return cfg->value_datapaths[i];
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>> +static void
>>  process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>>                 const struct ovsrec_bridge_table *bridge_table,
>> -               const struct ovsrec_open_vswitch_table *ovs_table)
>> +               const struct ovsrec_open_vswitch_table *ovs_table,
>> +               const struct ovsrec_bridge **br_int_,
>> +               const struct ovsrec_datapath **br_int_dp_)
>>  {
>> -    const struct ovsrec_bridge *br_int = get_br_int(bridge_table,
>> -                                                    ovs_table);
>> -    if (!br_int) {
>> -        br_int = create_br_int(ovs_idl_txn, ovs_table);
>> -    }
>> -    if (br_int && ovs_idl_txn) {
>> -        const struct ovsrec_open_vswitch *cfg;
>> -        cfg = ovsrec_open_vswitch_table_first(ovs_table);
>> -        ovs_assert(cfg);
>> -        const char *datapath_type = smap_get(&cfg->external_ids,
>> -                                             "ovn-bridge-datapath-type");
>> -        /* Check for the datapath_type and set it only if it is defined in
>> -         * cfg. */
>> -        if (datapath_type && strcmp(br_int->datapath_type, datapath_type)) {
>> -            ovsrec_bridge_set_datapath_type(br_int, datapath_type);
>> +    const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
>> +    const struct ovsrec_datapath *br_int_dp = NULL;
>> +    if (ovs_idl_txn) {
>> +        if (!br_int) {
>> +            br_int = create_br_int(ovs_idl_txn, ovs_table);
>>          }
>> -        if (!br_int->fail_mode || strcmp(br_int->fail_mode, "secure")) {
>> -            ovsrec_bridge_set_fail_mode(br_int, "secure");
>> -            VLOG_WARN("Integration bridge fail-mode changed to 'secure'.");
>> +
>> +        if (br_int) {
>> +            const struct ovsrec_open_vswitch *cfg =
>> +                ovsrec_open_vswitch_table_first(ovs_table);
>> +            ovs_assert(cfg);
>> +
>> +            /* Propagate "ovn-bridge-datapath-type" from OVS table, if any.
>> +             * 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");
>> +
>> +            if (!datapath_type) {
>> +                if (br_int->datapath_type[0]) {
>> +                    datapath_type = br_int->datapath_type;
>> +                } else {
>> +                    datapath_type = DEFAULT_DATAPATH;
>> +                }
>> +            }
>> +            if (strcmp(br_int->datapath_type, datapath_type)) {
>> +                ovsrec_bridge_set_datapath_type(br_int, datapath_type);
>> +            }
>> +            if (!br_int->fail_mode || strcmp(br_int->fail_mode, "secure")) {
>> +                ovsrec_bridge_set_fail_mode(br_int, "secure");
>> +                VLOG_WARN("Integration bridge fail-mode changed to 'secure'.");
>> +            }
>> +            br_int_dp = get_br_datapath(cfg, datapath_type);
>> +            if (!br_int_dp) {
>> +                br_int_dp = create_br_datapath(ovs_idl_txn, cfg,
>> +                                               datapath_type);
>> +            }
>>          }
>>      }
>> -    return br_int;
>> +    *br_int_ = br_int;
>> +    *br_int_dp_ = br_int_dp;
> 
> Bit of a nit but we don't check for NULL for these. Maybe you could assert.

Sure, will do.

> 
>> +}
>> +
>> +/* Returns 'true' if the feature set supported by 'ovs_datapath' have been
>> + * updated since the last call.
>> + */
>> +static bool
>> +process_ovs_datapath(const struct ovsrec_datapath *ovs_datapath)
>> +{
>> +    bool ct_zero_snat = smap_get_bool(&ovs_datapath->capabilities,
>> +                                      OVS_CT_ZERO_SNAT_FEATURE, false);
>> +    return ovs_feature_support_update(ct_zero_snat);
> 
> The function prototype for ovs_feature_support_update() expects 'enum
> ovs_feature_support' rather than a bool.

Oh, this is a bug, sorry about that.  I had switched between bool and
enum mid-way.  I'll rework this part in v2.

>>  }
>>  
>>  static const char *
>> @@ -848,6 +908,7 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>>      ovsdb_idl_add_table(ovs_idl, &ovsrec_table_open_vswitch);
>>      ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_external_ids);
>>      ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_bridges);
>> +    ovsdb_idl_add_column(ovs_idl, &ovsrec_open_vswitch_col_datapaths);
>>      ovsdb_idl_add_table(ovs_idl, &ovsrec_table_interface);
>>      ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
>>      ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd);
>> @@ -870,6 +931,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>>      ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_ca_cert);
>>      ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_certificate);
>>      ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_private_key);
>> +    ovsdb_idl_add_table(ovs_idl, &ovsrec_table_datapath);
>> +    ovsdb_idl_add_column(ovs_idl, &ovsrec_datapath_col_capabilities);
>>      chassis_register_ovs_idl(ovs_idl);
>>      encaps_register_ovs_idl(ovs_idl);
>>      binding_register_ovs_idl(ovs_idl);
>> @@ -2977,8 +3040,10 @@ main(int argc, char *argv[])
>>              ovsrec_bridge_table_get(ovs_idl_loop.idl);
>>          const struct ovsrec_open_vswitch_table *ovs_table =
>>              ovsrec_open_vswitch_table_get(ovs_idl_loop.idl);
>> -        const struct ovsrec_bridge *br_int =
>> -            process_br_int(ovs_idl_txn, bridge_table, ovs_table);
>> +        const struct ovsrec_bridge *br_int = NULL;
>> +        const struct ovsrec_datapath *br_int_dp = NULL;
>> +        process_br_int(ovs_idl_txn, bridge_table, ovs_table,
>> +                       &br_int, &br_int_dp);
>>  
>>          if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) &&
>>              northd_version_match) {
>> @@ -3009,6 +3074,12 @@ main(int argc, char *argv[])
>>                                        &chassis_private);
>>              }
>>  
>> +            /* If any OVS feature support changed, force a full recompute. */
>> +            if (br_int_dp && process_ovs_datapath(br_int_dp)) {
>> +                VLOG_INFO("OVS feature set changed, force recompute.");
>> +                engine_set_force_recompute(true);
>> +            }
>> +
>>              if (br_int) {
>>                  ct_zones_data = engine_get_data(&en_ct_zones);
>>                  if (ct_zones_data) {
>> diff --git a/include/ovn/features.h b/include/ovn/features.h
>> index 10ee46fcd..8a728a847 100644
>> --- a/include/ovn/features.h
>> +++ b/include/ovn/features.h
>> @@ -16,7 +16,23 @@
>>  #ifndef OVN_FEATURES_H
>>  #define OVN_FEATURES_H 1
>>  
>> +#include <stdbool.h>
>> +
>>  /* ovn-controller supported feature names. */
>>  #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif"
>>  
>> +/* OVS datapath supported features.  Based on availability OVN might generate
>> + * different types of openflows.
>> + */
>> +enum ovs_feature_support_bits {
>> +    OVS_CT_ZERO_SNAT_SUPPORT_BIT,
>> +};
>> +
>> +enum ovs_feature_support {
>> +    OVS_CT_ZERO_SNAT_SUPPORT = (1 << OVS_CT_ZERO_SNAT_SUPPORT_BIT),
>> +};
>> +
>> +enum ovs_feature_support ovs_feature_support_get(void);
>> +bool ovs_feature_support_update(enum ovs_feature_support features);
> 
> I think the intention is that "feature.c/h" will maintain a bitmap of
> supported features? I don't think this will work like that as it will
> only maintain a discrete value (i.e. the last call to
> ovs_feature_support_update(enum ovs_feature_support features). Maybe I
> am reading this wrong?

There's only one feature we test for now but yes, in the future there
might be more and the idea was to build a bitmap and pass it to
ovs_feature_support_update().

> 
> Also, it might be good for this to somehow maintain the mapping between
> the feature name from the ovs table to the equivalent feature bit in
> OVN. What do you think?

Sure, I can try to do that.  I'll also pass the 'capabilities' smap as
an argument to ovs_feature_support_update() instead.  Like that the code
might be more generic.

Thanks,
Dumitru



More information about the dev mailing list