[ovs-dev] [PATCH ovn 3/4] ovn-controller: Detect OVS datapath capabilities.
Mark Gray
mark.d.gray at redhat.com
Fri Jun 4 15:55:39 UTC 2021
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.
> +}
> +
> +/* 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.
> }
>
> 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?
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?
> +
> #endif
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 781be2109..917b28e1e 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -13,6 +13,7 @@ lib_libovn_la_SOURCES = \
> lib/expr.c \
> lib/extend-table.h \
> lib/extend-table.c \
> + lib/features.c \
> lib/ovn-parallel-hmap.h \
> lib/ovn-parallel-hmap.c \
> lib/ip-mcast-index.c \
> diff --git a/lib/features.c b/lib/features.c
> new file mode 100644
> index 000000000..697fae112
> --- /dev/null
> +++ b/lib/features.c
> @@ -0,0 +1,39 @@
> +/* Copyright (c) 2021, Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +
> +#include "ovn/features.h"
> +
> +static enum ovs_feature_support ovs_features;
> +
> +enum ovs_feature_support
> +ovs_feature_support_get(void)
> +{
> + return ovs_features;
> +}
> +
> +/* Returns 'true' if the OVS feature set has been updated since the last
> + * call.
> + */
> +bool
> +ovs_feature_support_update(enum ovs_feature_support features)
> +{
> + if (features != ovs_features) {
> + ovs_features = features;
> + return true;
> + }
> + return false;
> +}
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 72c07b3fa..9c25193e8 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -151,23 +151,24 @@ sysid=$(ovs-vsctl get Open_vSwitch . external_ids:system-id)
> check_datapath_type () {
> datapath_type=$1
> chassis_datapath_type=$(ovn-sbctl get Chassis ${sysid} other_config:datapath-type | sed -e 's/"//g') #"
> - test "${datapath_type}" = "${chassis_datapath_type}"
> + ovs_datapath_type=$(ovs-vsctl get Bridge br-int datapath-type)
> + test "${datapath_type}" = "${chassis_datapath_type}" && test "${datapath_type}" = "${ovs_datapath_type}"
> }
>
> -OVS_WAIT_UNTIL([check_datapath_type ""])
> +OVS_WAIT_UNTIL([check_datapath_type system])
>
> ovs-vsctl set Bridge br-int datapath-type=foo
> OVS_WAIT_UNTIL([check_datapath_type foo])
>
> # Change "ovn-bridge-mappings" value. It should not change the "datapath-type".
> ovs-vsctl set Open_vSwitch . external_ids:ovn-bridge-mappings=foo-mapping
> -check_datapath_type foo
> +AT_CHECK([check_datapath_type foo])
>
> ovs-vsctl set Bridge br-int datapath-type=bar
> OVS_WAIT_UNTIL([check_datapath_type bar])
>
> ovs-vsctl set Bridge br-int datapath-type=\"\"
> -OVS_WAIT_UNTIL([check_datapath_type ""])
> +OVS_WAIT_UNTIL([check_datapath_type system])
>
> # Set the datapath_type in external_ids:ovn-bridge-datapath-type.
> ovs-vsctl set Open_vSwitch . external_ids:ovn-bridge-datapath-type=foo
> @@ -176,11 +177,9 @@ OVS_WAIT_UNTIL([check_datapath_type foo])
> # Change the br-int's datapath type to bar.
> # It should be reset to foo since ovn-bridge-datapath-type is configured.
> ovs-vsctl set Bridge br-int datapath-type=bar
> -OVS_WAIT_UNTIL([test foo = `ovs-vsctl get Bridge br-int datapath-type`])
> OVS_WAIT_UNTIL([check_datapath_type foo])
>
> ovs-vsctl set Open_vSwitch . external_ids:ovn-bridge-datapath-type=foobar
> -OVS_WAIT_UNTIL([test foobar = `ovs-vsctl get Bridge br-int datapath-type`])
> OVS_WAIT_UNTIL([check_datapath_type foobar])
>
> expected_iface_types=$(ovs-vsctl get Open_vSwitch . iface_types | tr -d '[[]] ""')
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list