On 1/17/25 11:03, Ales Musil wrote:
> 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?
I didn't want to check all the tables to avoid burning CPU cycles for
nothing. It may not be a big deal, but it felt excessive to check 255
tabes every time ovn-controller wakes up. Chances that all the tables
are set, one of them is configured properly, but others are not, should
be negligeable, unless done intentionally by the user.
>
> +
>> + 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
