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

Dumitru Ceara dceara at redhat.com
Fri Jun 11 10:11:17 UTC 2021


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.

Acked-by: Mark D. Gray <mark.d.gray at redhat.com>
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 07c6fcfd1..3dfb04ef6 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);
@@ -2981,8 +3033,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) {
@@ -3013,6 +3067,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])



More information about the dev mailing list