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 <[email protected]> > --- > 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 <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
