On Tue, Jan 7, 2025 at 10:46 PM Ilya Maximets <[email protected]> wrote:
> OVS allows enabling prefix match optimizations per flow table. This
> enables masked matches whenever possible on fields that otherwise would
> be exact matched in the datapath flow.
>
> By default, however, only nw_src and nw_dst are enabled (L4 ports are
> also always enabled, but this is not configurable). OVN is using mixed
> flow tables that match on both IPv4 and IPv6 addresses, meaning that
> IPv6 traffic generates exact match datapath flows where IPv4 generates
> masked matches, causing datapath flow explosion under heavy IPv6 load.
>
> OVN owns the "br-int" bridge and the flow tables, so it should enable
> appropriate fields per flow table to avoid flow explosion and achieve
> better performance overall.
>
> Example on how the prefixes can be configured manually:
>
> for i in $(seq 0 254); do
> ovs-vsctl set Bridge br-int flow_tables:${i}=@N -- \
> --id=@N create Flow_Table name=t${i} \
> prefixes=nw_src,nw_dst,ipv6_dst,ipv6_src;
> done
>
> Until recently, OVS only supported up to 3 prefixes per flow table, but
> now the limit will be increased to 4 in OVS 3.5 and some newer minor
> releases of older versions down to 3.3.
> Unfortunately, that means that ovn-controller needs to check and choose
> the appropriate number of prefixes. For the 3 we may just add ipv6_src
> and leave ipv6_dst unoptimized.
>
> OVS 3.5 will have all 4 prefixes enabled by default, but OVN will be
> paired with older versions of OVS for a long time, so it's better to
> set these config options to better support older setups.
>
> Unfortunately, IDL doesn't provide a way today to get the type of the
> column from the server side, so it's hard to tell how many prefixes
> are actually supported. A few approaches:
>
> 1. Try 3 and 4 and check if transaction fails.
> 2. Try to get and parse the schema from the server.
> 3. Enhance IDL to provide server column type information.
>
> While the first approach seems simpler, it's actually not trivial to
> figure out why exactly the transaction failed from the application
> level. IDL only has the string representation of the error and doesn't
> provide it to the application.
>
> The third approach is the most clean one, but it requires modifications
> of the IDL and CS layers in order to get this information. This would
> significantly complicate the process of getting this change backported
> to OVN 24.03 LTS, for example.
>
> The second approach is taken by this commit with intention to replace
> the schema parsing with the enhanced IDL API, once it is available.
> This allows for easier backports today with a cleaner solution in the
> future.
>
> IMO, the backportability is important due to increasing importance of
> IPv6 in OVN clusters and the cloud environments in general.
>
> Reported-at: https://issues.redhat.com/browse/FDP-1024
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
>
Hi Ilya,
thank you for the patch, we have briefly discussed that offline,
but I would to hear opinion of others especially maintainers (cced)
whether we should wait a bit for the IDL API enhancement and proceed
without the need for parsing inside ovn-controller.
>
> Posting this version as a continuation to internal discussion with
> Dumitru and Ales on how to detect server-side database types.
>
> However, this version is fully functional and is ready for review if we
> agree on the approach.
>
> Note: For backpotrs, OVS submodules on older OVN branches will need to
> be moved to latest commits on corresponding OVS branches.
>
> NEWS | 3 ++
> TODO.rst | 3 ++
> controller/ovn-controller.c | 79 ++++++++++++++++++++++++++-
> include/ovn/features.h | 4 +-
> lib/features.c | 105 +++++++++++++++++++++++++++++++++++-
> lib/test-ovn-features.c | 6 +--
> tests/ovn-controller.at | 50 +++++++++++++++++
> 7 files changed, 244 insertions(+), 6 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 72c5a6339..3bd57784b 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -28,6 +28,9 @@ Post v24.09.0
> this option was not set). TLS ciphersuites for TLSv1.3 and later
> can
> be configured via --ssl-ciphersuites (--ssl-ciphers only applies to
> TLSv1.2 and earlier).
> + - Improved handling of IPv6 traffic by enabling address prefix tracking
> + in OVS for both IPv4 and IPv6 addresses, whenever possible, reducing
> + the amount of IPv6 datapath flows.
>
> OVN v24.09.0 - 13 Sep 2024
> --------------------------
> diff --git a/TODO.rst b/TODO.rst
> index 3426497a7..2e004b0b1 100644
> --- a/TODO.rst
> +++ b/TODO.rst
> @@ -136,3 +136,6 @@ OVN To-do List
>
> * Remove ssl_ciphersuites workaround for clustered databases from ovn-ctl
> after 26.03 release, assuming it will be an LTS release.
> +
> +* Remove OVS database schema parsing from features.c once server side
> column
> + types are available through IDL.
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index d875ecc1e..fa87d9450 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -497,6 +497,77 @@ create_br_datapath(struct ovsdb_idl_txn *ovs_idl_txn,
> return dp;
> }
>
> +
> +#define N_FLOW_TABLES 255
> +
> +static void
> +update_flow_table_prefixes(struct ovsdb_idl_txn *ovs_idl_txn,
> + const struct ovsrec_bridge *br_int)
> +{
> + size_t max_prefixes = ovs_features_max_flow_table_prefixes_get();
> + struct ds ds = DS_EMPTY_INITIALIZER;
> + const char *prefixes[] = {
> + "ip_src", "ip_dst", "ipv6_src", "ipv6_dst",
> + };
> + struct ovsrec_flow_table *ft;
> + size_t i;
> +
> + /* We must not attempt setting more prefixes than our IDL supports.
> + * Note: This should be a build time assertion, but IDL structures
> + * are not defined as constants. */
> + ovs_assert(
> + ARRAY_SIZE(prefixes) <=
> +
> ovsrec_flow_table_columns[OVSREC_FLOW_TABLE_COL_PREFIXES].type.n_max);
> +
> + if (!max_prefixes) {
> + /* Not discovered yet. */
> + return;
> + }
> +
> + max_prefixes = MIN(max_prefixes, ARRAY_SIZE(prefixes));
> + if (br_int->n_flow_tables == N_FLOW_TABLES &&
> + br_int->value_flow_tables[0]->n_prefixes == max_prefixes) {
> + /* Already up to date. Ideally, we would check every table,
> + * but it seems excessive. */
> + return;
> + }
>
By moving this after we figure out if all flow tables point to the
same row we can check that row and have definite proof that it is
actually correct, WDYT?
+
> + for (i = 1; i < br_int->n_flow_tables; i++) {
> + if (br_int->value_flow_tables[i] != br_int->value_flow_tables[0])
> {
> + break;
> + }
> + }
> + if (i == N_FLOW_TABLES) {
> + /* Correct number of flow tables and all pointing to the same
> row. */
> + ft = br_int->value_flow_tables[0];
> + } else {
> + /* Unexpected configuration. Let's create a new flow table row.
> + * Old ones will be garbage collected by the database. */
> + struct ovsrec_flow_table *values[N_FLOW_TABLES];
> + int64_t keys[N_FLOW_TABLES];
> +
> + ft = ovsrec_flow_table_insert(ovs_idl_txn);
> + for (i = 0; i < ARRAY_SIZE(values); i++) {
> + keys[i] = i;
> + values[i] = ft;
> + }
> + ovsrec_bridge_set_flow_tables(br_int, keys, values,
> + ARRAY_SIZE(values));
> + }
> +
> + ds_put_cstr(&ds, "Setting flow table prefixes:");
> + for (i = 0 ; i < max_prefixes; i++) {
> + ds_put_char(&ds, ' ');
> + ds_put_cstr(&ds, prefixes[i]);
> + ds_put_char(&ds, ',');
> + }
> + ds_chomp(&ds, ',');
> + VLOG_INFO("%s.", ds_cstr_ro(&ds));
> + ds_destroy(&ds);
> +
> + ovsrec_flow_table_set_prefixes(ft, prefixes, max_prefixes);
> +}
> +
> static const struct ovsrec_bridge *
> get_br_int(const struct ovsrec_bridge_table *bridge_table,
> const struct ovsrec_open_vswitch_table *ovs_table)
> @@ -573,6 +644,8 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
> datapath_type);
> }
> }
> +
> + update_flow_table_prefixes(ovs_idl_txn, br_int);
> }
> }
> *br_int_ = br_int;
> @@ -793,8 +866,11 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_ports);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_name);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_fail_mode);
> + ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_flow_tables);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_other_config);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_external_ids);
> + ovsdb_idl_add_table(ovs_idl, &ovsrec_table_flow_table);
> + ovsdb_idl_add_column(ovs_idl, &ovsrec_flow_table_col_prefixes);
> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_ssl);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_bootstrap_ca_cert);
> ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_ca_cert);
> @@ -5593,7 +5669,8 @@ main(int argc, char *argv[])
> && ovs_feature_support_run(br_int_dp ?
> &br_int_dp->capabilities :
> NULL,
> br_int_remote.target,
> - br_int_remote.probe_interval))
> {
> + br_int_remote.probe_interval,
> + ovs_remote)) {
> VLOG_INFO("OVS feature set changed, force recompute.");
> engine_set_force_recompute();
>
> diff --git a/include/ovn/features.h b/include/ovn/features.h
> index 3566ab60f..95f3704ca 100644
> --- a/include/ovn/features.h
> +++ b/include/ovn/features.h
> @@ -56,9 +56,11 @@ enum ovs_feature_value {
> void ovs_feature_support_destroy(void);
> bool ovs_feature_is_supported(enum ovs_feature_value feature);
> bool ovs_feature_support_run(const struct smap *ovs_capabilities,
> - const char *conn_target, int probe_interval);
> + const char *conn_target, int probe_interval,
> + const char *db_target);
> bool ovs_feature_set_discovered(void);
> uint32_t ovs_feature_max_meters_get(void);
> uint32_t ovs_feature_max_select_groups_get(void);
> +size_t ovs_features_max_flow_table_prefixes_get(void);
>
> #endif
> diff --git a/lib/features.c b/lib/features.c
> index 98d56602c..7302599ac 100644
> --- a/lib/features.c
> +++ b/lib/features.c
> @@ -20,6 +20,9 @@
> #include "lib/util.h"
> #include "lib/dirs.h"
> #include "socket-util.h"
> +#include "lib/ovsdb-cs.h"
> +#include "lib/ovsdb-error.h"
> +#include "lib/ovsdb-types.h"
> #include "lib/vswitch-idl.h"
> #include "odp-netlink.h"
> #include "openvswitch/vlog.h"
> @@ -418,17 +421,99 @@ ovs_feature_get_openflow_cap(void)
> return ret;
> }
>
> +/* Monitoring how many prefixes flow tables can have.
> + * TODO: Remove this code once the server column type is available via
> IDL. */
> +static struct ovsdb_cs *vswitch_cs;
> +static size_t max_flow_table_prefixes;
> +
> +static struct json *
> +vswitch_schema_prefixes_find(const struct json *schema_json,
> + void *ctx OVS_UNUSED)
> +{
> + /* We must return a monitor request object, but we do not actually
> want
> + * to monitor the data, so returning an empty one. */
> + struct json *monitor_request = json_object_create();
> + struct ovsdb_idl_column *prefixes_col;
> +
> + prefixes_col =
> &ovsrec_flow_table_columns[OVSREC_FLOW_TABLE_COL_PREFIXES];
> +
> + if (schema_json->type != JSON_OBJECT) {
> + VLOG_WARN_RL(&rl, "OVS database schema is not a JSON object");
> + return monitor_request;
> + }
> +
> + const char *ft_name =
> ovsrec_table_classes[OVSREC_TABLE_FLOW_TABLE].name;
> + const char *nested_objects[] = {
> + "tables", ft_name, "columns", prefixes_col->name, "type",
> + };
> + const struct json *nested = schema_json;
> +
> + for (size_t i = 0; i < ARRAY_SIZE(nested_objects); i++) {
> + nested = shash_find_data(json_object(nested), nested_objects[i]);
> + if (!nested || nested->type != JSON_OBJECT) {
> + VLOG_WARN_RL(&rl, "OVS database schema has no valid '%s'.",
> + nested_objects[i]);
> + return monitor_request;
> + }
> + }
> +
> + struct ovsdb_type server_type;
> + struct ovsdb_error *error;
> +
> + error = ovsdb_type_from_json(&server_type, nested);
> + if (error) {
> + char *msg = ovsdb_error_to_string_free(error);
> +
> + VLOG_WARN_RL(&rl, "Failed to parse type of %s column in %s table:
> %s",
> + prefixes_col->name, ft_name, msg);
> + free(msg);
> + return monitor_request;
> + }
> +
> + if (max_flow_table_prefixes != server_type.n_max) {
> + VLOG_INFO_RL(&rl, "OVS DB schema supports %d flow table prefixes,
> "
> + "our IDL supports: %d",
> + server_type.n_max, prefixes_col->type.n_max);
> + max_flow_table_prefixes = server_type.n_max;
> + }
> +
> + return monitor_request;
> +}
> +
> +static struct ovsdb_cs_ops feature_cs_ops = {
> + .compose_monitor_requests = vswitch_schema_prefixes_find,
> +};
> +
> +static void
> +vswitch_schema_prefixes_run(void)
> +{
> + struct ovsdb_cs_event *event;
> + struct ovs_list events;
> +
> + ovsdb_cs_run(vswitch_cs, &events);
> + LIST_FOR_EACH_POP (event, list_node, &events) {
> + /* We're not really interested in events. We only care about the
> + * monitor request callback being invoked on re-connections or
> + * schema changes. */
> + ovsdb_cs_event_destroy(event);
> + }
> + ovsdb_cs_wait(vswitch_cs);
> +}
> +
> void
> ovs_feature_support_destroy(void)
> {
> rconn_destroy(swconn);
> swconn = NULL;
> + ovsdb_cs_destroy(vswitch_cs);
> + vswitch_cs = NULL;
> }
>
> /* Returns 'true' if the set of tracked OVS features has been updated. */
> bool
> ovs_feature_support_run(const struct smap *ovs_capabilities,
> - const char *conn_target, int probe_interval)
> + const char *conn_target, int probe_interval,
> + const char *db_target)
> {
> static struct smap empty_caps = SMAP_INITIALIZER(&empty_caps);
>
> @@ -449,6 +534,16 @@ ovs_feature_support_run(const struct smap
> *ovs_capabilities,
> updated |= handle_feature_state_update(new_value, feature->value,
> feature->name);
> }
> +
> + if (!vswitch_cs) {
> + vswitch_cs = ovsdb_cs_create(ovsrec_idl_class.database, 3,
> + &feature_cs_ops, NULL);
> + }
> + ovsdb_cs_set_remote(vswitch_cs, db_target, true);
> + /* This feature doesn't affect OpenFlow rules, so not touching
> 'updated'
> + * even if changed. */
> + vswitch_schema_prefixes_run();
> +
> return updated;
> }
>
> @@ -479,3 +574,11 @@ ovs_feature_max_select_groups_get(void)
> {
> return ovs_group_features.max_groups[OFPGT11_SELECT];
> }
> +
> +/* Returns the number of flow table prefixes that can be configured in
> + * the OVS database. */
> +size_t
> +ovs_features_max_flow_table_prefixes_get(void)
> +{
> + return max_flow_table_prefixes;
> +}
> diff --git a/lib/test-ovn-features.c b/lib/test-ovn-features.c
> index cddeae779..40fba6a43 100644
> --- a/lib/test-ovn-features.c
> +++ b/lib/test-ovn-features.c
> @@ -26,15 +26,15 @@ test_ovn_features(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
> struct smap features = SMAP_INITIALIZER(&features);
>
> smap_add(&features, "ct_zero_snat", "false");
> - ovs_assert(!ovs_feature_support_run(&features, NULL, 0));
> + ovs_assert(!ovs_feature_support_run(&features, NULL, 0, NULL));
> ovs_assert(!ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT));
>
> smap_replace(&features, "ct_zero_snat", "true");
> - ovs_assert(ovs_feature_support_run(&features, NULL, 0));
> + ovs_assert(ovs_feature_support_run(&features, NULL, 0, NULL));
> ovs_assert(ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT));
>
> smap_add(&features, "unknown_feature", "true");
> - ovs_assert(!ovs_feature_support_run(&features, NULL, 0));
> + ovs_assert(!ovs_feature_support_run(&features, NULL, 0, NULL));
>
> smap_destroy(&features);
> }
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 2bf8b53d0..de86ced1c 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -3134,6 +3134,56 @@ OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1235:
> connected' hv1/ovn-controller.log])
> OVN_CLEANUP([hv1])
> AT_CLEANUP
>
> +AT_SETUP([ovn-controller - br-int flow table prefixes])
> +AT_KEYWORDS([ovn-controller prefixes])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +ovs-vsctl add-br br-phys
> +
> +dnl Stop the database server before starting ovn-controller and convert
> the
> +dnl schema to support only 3 prefixes.
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +AT_CHECK([sed 's/"max": 4}/"max": 3}/'
> $ovs_srcdir/vswitchd/vswitch.ovsschema > new_schema])
> +AT_CHECK([ovsdb-tool convert hv1/conf.db new_schema])
> +start_daemon ovsdb-server --remote=punix:db.sock
> +
> +dnl Start ovn-controller.
> +ovn_attach n1 br-phys 192.168.0.20
> +
> +dnl Wait for exactly 3 prefixes to be configured.
> +OVS_WAIT_UNTIL(
> + [grep -q 'Setting flow table prefixes: ip_src, ip_dst, ipv6_src.' \
> + hv1/ovn-controller.log])
> +OVS_WAIT_FOR_OUTPUT([ovs-vsctl --columns=prefixes --bare list
> Flow_Table], [0], [dnl
> +ip_dst ip_src ipv6_src
> +])
> +
> +dnl Remember the row UUID.
> +uuid=$(ovs-vsctl --columns=_uuid --bare list Flow_Table)
> +
> +dnl Stop the database server again and convert the schema back to support
> +dnl up to 4 prefixes.
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +AT_CHECK([cat $ovs_srcdir/vswitchd/vswitch.ovsschema > new_schema])
> +AT_CHECK([ovsdb-tool convert hv1/conf.db new_schema])
> +start_daemon ovsdb-server --remote=punix:db.sock
> +
> +dnl Wait for exactly 4 prefixes to be configured.
> +OVS_WAIT_FOR_OUTPUT(
> + [grep -q 'Setting flow table prefixes: ip_src, ip_dst, ipv6_src,
> ipv6_dst.' \
> + hv1/ovn-controller.log])
> +OVS_WAIT_FOR_OUTPUT([ovs-vsctl --columns=prefixes --bare list
> Flow_Table], [0], [dnl
> +ip_dst ip_src ipv6_dst ipv6_src
> +])
> +
> +dnl Check that the record was updated and not replaced.
> +AT_CHECK([test "$(ovs-vsctl --columns=_uuid --bare list Flow_Table)" =
> "${uuid}"])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([ovn-controller - CT zone min/max boundaries])
> ovn_start
> --
> 2.47.0
>
>
Thanks,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
