[ovs-dev] [PATCH ovn v2 4/5] ovn-controller: Detect OVS datapath capabilities.
Mark Gray
mark.d.gray at redhat.com
Thu Jun 10 16:29:08 UTC 2021
On 09/06/2021 13:12, 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 | 115 +++++++++++++++++++++++++++++++++----------
> include/ovn/features.h | 18 +++++++
> lib/automake.mk | 1
> lib/features.c | 84 +++++++++++++++++++++++++++++++
> lib/test-ovn-features.c | 56 +++++++++++++++++++++
> tests/automake.mk | 3 +
> tests/ovn-controller.at | 11 ++--
> tests/ovn-features.at | 8 +++
> tests/testsuite.at | 1
> 9 files changed, 264 insertions(+), 33 deletions(-)
> create mode 100644 lib/features.c
> create mode 100644 lib/test-ovn-features.c
> create mode 100644 tests/ovn-features.at
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index d48ddc7a2..308699820 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
>
> @@ -319,10 +321,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 +384,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 +412,69 @@ 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;
> +
> + ovs_assert(br_int_ && br_int_dp_);
> + 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;
> }
>
> static const char *
> @@ -848,6 +897,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 +920,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 +3029,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 +3063,13 @@ main(int argc, char *argv[])
> &chassis_private);
> }
>
> + /* If any OVS feature support changed, force a full recompute. */
> + if (br_int_dp
> + && ovs_feature_support_update(&br_int_dp->capabilities)) {
> + 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..c35d59b14 100644
> --- a/include/ovn/features.h
> +++ b/include/ovn/features.h
> @@ -16,7 +16,25 @@
> #ifndef OVN_FEATURES_H
> #define OVN_FEATURES_H 1
>
> +#include <stdbool.h>
> +
> +#include "smap.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_value {
> + OVS_CT_ZERO_SNAT_SUPPORT = (1 << OVS_CT_ZERO_SNAT_SUPPORT_BIT),
> +};
> +
> +bool ovs_feature_is_supported(enum ovs_feature_value feature);
> +bool ovs_feature_support_update(const struct smap *ovs_capabilities);
> +
> #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..87d04ee3f
> --- /dev/null
> +++ b/lib/features.c
> @@ -0,0 +1,84 @@
> +/* 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 <stdint.h>
> +#include <stdlib.h>
> +
> +#include "lib/util.h"
> +#include "openvswitch/vlog.h"
> +#include "ovn/features.h"
> +
> +VLOG_DEFINE_THIS_MODULE(features);
> +
> +struct ovs_feature {
> + enum ovs_feature_value value;
> + const char *name;
> +};
> +
> +static struct ovs_feature all_ovs_features[] = {
> + {
> + .value = OVS_CT_ZERO_SNAT_SUPPORT,
> + .name = "ct_zero_snat"
> + },
> +};
> +
> +/* A bitmap of OVS features that have been detected as 'supported'. */
> +static uint32_t supported_ovs_features;
> +
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> +
> +static bool
> +ovs_feature_is_valid(enum ovs_feature_value feature)
> +{
> + switch (feature) {
> + case OVS_CT_ZERO_SNAT_SUPPORT:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +bool
> +ovs_feature_is_supported(enum ovs_feature_value feature)
> +{
> + ovs_assert(ovs_feature_is_valid(feature));
> + return supported_ovs_features & feature;
> +}
> +
> +/* Returns 'true' if the set of tracked OVS features has been updated. */
> +bool
> +ovs_feature_support_update(const struct smap *ovs_capabilities)
> +{
> + bool updated = false;
> +
> + for (size_t i = 0; i < ARRAY_SIZE(all_ovs_features); i++) {
> + enum ovs_feature_value value = all_ovs_features[i].value;
> + const char *name = all_ovs_features[i].name;
> + bool old_state = supported_ovs_features & value;
> + bool new_state = smap_get_bool(ovs_capabilities, name, false);
> + if (new_state != old_state) {
> + updated = true;
> + if (new_state) {
> + supported_ovs_features |= value;
> + } else {
> + supported_ovs_features &= ~value;
> + }
> + VLOG_INFO_RL(&rl, "OVS Feature: %s, state: %s", name,
> + new_state ? "supported" : "not supported");
> + }
> + }
> + return updated;
> +}
> diff --git a/lib/test-ovn-features.c b/lib/test-ovn-features.c
> new file mode 100644
> index 000000000..deb97581e
> --- /dev/null
> +++ b/lib/test-ovn-features.c
> @@ -0,0 +1,56 @@
> +/* 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"
> +#include "tests/ovstest.h"
> +
> +static void
> +test_ovn_features(struct ovs_cmdl_context *ctx OVS_UNUSED)
> +{
> + ovs_assert(!ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT));
> +
> + struct smap features = SMAP_INITIALIZER(&features);
> +
> + smap_add(&features, "ct_zero_snat", "false");
> + ovs_assert(!ovs_feature_support_update(&features));
> + ovs_assert(!ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT));
> +
> + smap_replace(&features, "ct_zero_snat", "true");
> + ovs_assert(ovs_feature_support_update(&features));
> + ovs_assert(ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT));
> +
> + smap_add(&features, "unknown_feature", "true");
> + ovs_assert(!ovs_feature_support_update(&features));
> +
> + smap_destroy(&features);
> +}
> +
> +static void
> +test_ovn_features_main(int argc, char *argv[])
> +{
> + set_program_name(argv[0]);
> + static const struct ovs_cmdl_command commands[] = {
> + {"run", NULL, 0, 0, test_ovn_features, OVS_RO},
> + {NULL, NULL, 0, 0, NULL, OVS_RO},
> + };
> + struct ovs_cmdl_context ctx;
> + ctx.argc = argc - 1;
> + ctx.argv = argv + 1;
> + ovs_cmdl_run_command(&ctx, commands);
> +}
> +
> +OVSTEST_REGISTER("test-ovn-features", test_ovn_features_main);
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 742e5cff2..a8ec64212 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -34,6 +34,7 @@ TESTSUITE_AT = \
> tests/ovn-performance.at \
> tests/ovn-ofctrl-seqno.at \
> tests/ovn-ipam.at \
> + tests/ovn-features.at \
> tests/ovn-lflow-cache.at \
> tests/ovn-ipsec.at
>
> @@ -207,6 +208,7 @@ $(srcdir)/package.m4: $(top_srcdir)/configure.ac
>
> noinst_PROGRAMS += tests/ovstest
> tests_ovstest_SOURCES = \
> + include/ovn/features.h \
> tests/ovstest.c \
> tests/ovstest.h \
> tests/test-utils.c \
> @@ -218,6 +220,7 @@ tests_ovstest_SOURCES = \
> controller/lflow-cache.h \
> controller/ofctrl-seqno.c \
> controller/ofctrl-seqno.h \
> + lib/test-ovn-features.c \
> northd/test-ipam.c \
> northd/ipam.c \
> northd/ipam.h
> 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 '[[]] ""')
> diff --git a/tests/ovn-features.at b/tests/ovn-features.at
> new file mode 100644
> index 000000000..36bd83055
> --- /dev/null
> +++ b/tests/ovn-features.at
> @@ -0,0 +1,8 @@
> +#
> +# Unit tests for the lib/features.c module.
> +#
> +AT_BANNER([OVN unit tests - features])
> +
> +AT_SETUP([ovn -- unit test -- OVS feature detection tests])
> +AT_CHECK([ovstest test-ovn-features run], [0], [])
> +AT_CLEANUP
> diff --git a/tests/testsuite.at b/tests/testsuite.at
> index ddc3f11d6..b716a1ad9 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -27,6 +27,7 @@ m4_include([tests/ovn.at])
> m4_include([tests/ovn-performance.at])
> m4_include([tests/ovn-northd.at])
> m4_include([tests/ovn-nbctl.at])
> +m4_include([tests/ovn-features.at])
> m4_include([tests/ovn-lflow-cache.at])
> m4_include([tests/ovn-ofctrl-seqno.at])
> m4_include([tests/ovn-sbctl.at])
>
Thanks!
Acked-by: Mark D. Gray <mark.d.gray at redhat.com>
More information about the dev
mailing list