On Sun, Nov 5, 2023 at 10:59 PM Ales Musil <amu...@redhat.com> wrote:
>
>
>
> On Sat, Nov 4, 2023 at 5:45 AM Han Zhou <hz...@ovn.org> wrote:
>>
>>
>>
>> On Fri, Nov 3, 2023 at 1:08 AM Ales Musil <amu...@redhat.com> wrote:
>> >
>> >
>> >
>> > On Tue, Oct 24, 2023 at 9:36 PM Han Zhou <hz...@ovn.org> wrote:
>> >>
>> >> Enhance MAC_Binding aging to allow CIDR-based threshold
configurations.
>> >> This enables distinct threshold settings for different IP ranges,
>> >> applying the longest prefix matching for overlapping ranges.
>> >>
>> >> A common use case involves setting a default threshold for all IPs,
while
>> >> disabling aging for a specific range and potentially excluding a
subset
>> >> within that range.
>> >>
>> >> Signed-off-by: Han Zhou <hz...@ovn.org>
>> >> ---
>> >
>> >
>> > Hi Han,
>> >
>> > thank you for the patch, I have a couple of comments down below.
>>
>> Thanks for your review!
>>
>> >
>> >>
>> >> northd/aging.c | 297
+++++++++++++++++++++++++++++++++++++++++++++---
>> >> northd/aging.h | 3 +
>> >> northd/northd.c | 11 +-
>> >> ovn-nb.xml | 63 +++++++++-
>> >> tests/ovn.at | 60 ++++++++++
>> >> 5 files changed, 413 insertions(+), 21 deletions(-)
>> >>
>> >> diff --git a/northd/aging.c b/northd/aging.c
>> >> index f626c72c8ca3..e5868211a63b 100644
>> >> --- a/northd/aging.c
>> >> +++ b/northd/aging.c
>> >> @@ -47,12 +47,253 @@ aging_waker_schedule_next_wake(struct
aging_waker *waker, int64_t next_wake_ms)
>> >> }
>> >> }
>> >>
>> >> +struct threshold_entry {
>> >> + union {
>> >> + ovs_be32 ipv4;
>> >> + struct in6_addr ipv6;
>> >> + } prefix;
>> >> + bool is_v4;
>> >
>> >
>> > We can avoid the is_v4 whatsover by storing the address as mapped v4
in 'in6_addr', I saw the concern and we can still have separate arrays for
v4 and v6. The parsing IMO becomes much simpler if we use the mapped v4 for
both see down below.
>> >
>>
>> I did consider using in6_addr, but it would make the
find_threshold_for_ip() slightly more tedious because for every v4 entry,
even if stored in a separate array, we will still need to call
in6_addr_get_mapped_ipv4() to convert it. While using the union we can
directly access what we need (ipv4 or ipv6).
>
>
> We wouldn't have to actually call in6_addr_get_mapped_ipv4() for the v4
variant, that's why I suggested that approach. We can adjust the mask and
then use the same logic as for ipv6 (entry->plen += 96 should do the trick).
>
Understood, and my reply below was about that:
>> The search may be simplified a little in find_threshold_for_ip() with
adjusted prefix for v4, only if we combine the two arrays to a single
array. Otherwise we will need to have a function to abstract and reuse the
ipv6 matching logic, while the ipv4 matching is really just 3 lines of code
:)
>>
>> The is_v4 member was needed when using a single array, but not necessary
any more when storing v4 and v6 entries separately, but I kept it just for
convenience, so that I don't need to use another arg in
parse_threshold_entry() to return the AF type. Using in6_addr can avoid the
extra arg, too, but similarly, we would have to always call the macro
IN6_IS_ADDR_V4MAPPED, which doesn't seem cleaner than the is_v4 field to
me. Using ip46_parse_cidr does save several lines of code, but that code
snippet itself is quite small. So with all these being considered, I think
either approach doesn't really make too much difference. I am totally fine
to change it if you still think a single in6_addr is better than the union.
>>
>> >>
>> >> + unsigned int plen;
>> >> + unsigned int threshold;
>> >
>> >
>> > I personally prefer sized integers.
>> >
>> I think it may be better to match the type of related helper functions.
>> For plen, it is ip[v6]_parse_cidr().
>> For threshold, it is str_to_uint(), or even smap_get_uint() returns
unsigned int.
>>
>> >>
>> >> +};
>> >> +
>> >> +/* Contains CIDR-based aging threshold configuration parsed from
>> >> + * "Logical_Router:options:mac_binding_age_threshold".
>> >> + *
>> >> + * This struct is also used for non-CIDR-based threshold, e.g. the
ones from
>> >> + * "NB_Global:other_config:fdb_age_threshold" for the common
aging_context
>> >> + * interface.
>> >> + *
>> >> + * - The arrays `v4_entries` and `v6_entries` are populated with
parsed entries
>> >> + * for IPv4 and IPv6 CIDRs, respectively, along with their
associated
>> >> + * thresholds. Entries within these arrays are sorted by prefix
length,
>> >> + * starting with the longest.
>> >> + *
>> >> + * - If a threshold is provided without an accompanying prefix, it's
captured
>> >> + * in `default_threshold`. In cases with multiple unprefixed
thresholds,
>> >> + * `default_threshold` will only store the last one. */
>> >> +struct threshold_config {
>> >> + struct threshold_entry *v4_entries;
>> >> + int n_v4_entries;
>> >> + struct threshold_entry *v6_entries;
>> >> + int n_v6_entries;
>> >> + unsigned int default_threshold;
>> >> +};
>> >> +
>> >> +static int
>> >> +compare_entries_by_prefix_length(const void *a, const void *b)
>> >> +{
>> >> + const struct threshold_entry *entry_a = a;
>> >> + const struct threshold_entry *entry_b = b;
>> >> +
>> >> + return entry_b->plen - entry_a->plen;
>> >> +}
>> >> +
>> >> +/* Parse an ENTRY in the threshold option, with the format:
>> >> + * [CIDR:]THRESHOLD
>> >> + *
>> >> + * Returns true if successful, false if failed. */
>> >> +static bool
>> >> +parse_threshold_entry(const char *str, struct threshold_entry *entry)
>> >> +{
>> >> + char *colon_ptr;
>> >> + unsigned int value;
>> >> + const char *threshold_str;
>> >> +
>> >> + colon_ptr = strrchr(str, ':');
>> >
>> >
>> > I find this a bit confusing and probably error prone, could we use
different delimiters for the threshold? I guess we could change it to
'cidr;threshold,cidr;threshold' WDYT? This would allow us to use
'strtok_r()' here and we could spare the multiple copies and replace with a
single one.
>> >
>> I thought about that, too, but decided to use ":" instead of ";" because
"A:B" looks more natural to represent "A has a value B", and ";" is more
like a delimiter to separate two different elements, just like ",". So I
feel that ":" might be more friendly to end users. I'd love to hear
opinions from more people.
>> If ";" or something else turned out to be a better option for the end
user, we can use strtok_r() here but I think it shouldn't be a concern if
we can't use it.
>>
>> >>
>> >> + if (!colon_ptr) {
>> >> + threshold_str = str;
>> >> + entry->plen = 0;
>> >> + } else {
>> >> + threshold_str = colon_ptr + 1;
>> >> + }
>> >> +
>> >> + if (!str_to_uint(threshold_str, 10, &value)) {
>> >> + return false;
>> >> + }
>> >> + entry->threshold = value;
>> >> +
>> >> + if (!colon_ptr) {
>> >> + return true;
>> >> + }
>> >> +
>> >> + /* ":" was found, so parse the string before ":" as a cidr. */
>> >> + char ip_cidr[128];
>> >> + ovs_strzcpy(ip_cidr, str, MIN(colon_ptr - str + 1, sizeof
ip_cidr));
>> >> + char *error = ip_parse_cidr(ip_cidr, &entry->prefix.ipv4,
&entry->plen);
>> >
>> >
>> > As mentioned earlier by storing it in 'in6_addr' we could use
'ip46_parse_cidr()' which would simplify the logic. We can also adjust the
prefix for v4 so the search is the same for both.
>>
>> The search may be simplified a little in find_threshold_for_ip() with
adjusted prefix for v4, only if we combine the two arrays to a single
array. Otherwise we will need to have a function to abstract and reuse the
ipv6 matching logic, while the ipv4 matching is really just 3 lines of code
:)
>> But if you prefer to use a single array for both v4 and v6, I can try
that in v2.
>>
>> >
>> >>
>> >> + if (!error) {
>> >> + entry->is_v4 = true;
>> >> + return true;
>> >> + }
>> >> + free(error);
>> >> + error = ipv6_parse_cidr(ip_cidr, &entry->prefix.ipv6,
&entry->plen);
>> >> + if (!error) {
>> >> + entry->is_v4 = false;
>> >> + return true;
>> >> + }
>> >> + free(error);
>> >> + return false;
>> >> +}
>> >> +
>> >> +static void
>> >> +threshold_config_destroy(struct threshold_config *config)
>> >> +{
>> >> + free(config->v4_entries);
>> >> + free(config->v6_entries);
>> >> + config->v4_entries = config->v6_entries = NULL;
>> >> + config->n_v4_entries = config->n_v6_entries = 0;
>> >> + config->default_threshold = 0;
>> >> +}
>> >> +
>> >> +/* Parse the threshold option string, which has the format:
>> >> + * ENTRY[;ENTRY[...]]
>> >> + *
>> >> + * For the exact format of ENTRY, refer to the function
>> >> + * `parse_threshold_entry`.
>> >> + *
>> >> + * The parsed data is populated to the struct threshold_config.
>> >> + * See the comments of struct threshold_config for details.
>> >> + *
>> >> + * Return Values:
>> >> + * - Returns `false` if the input does not match the expected format.
>> >> + * Consequently, no entries will be populated.
>> >> + * - Returns `true` upon successful parsing. The caller is
responsible for
>> >> + * releasing the allocated memory by calling
threshold_config_destroy. */
>> >> +static bool
>> >> +parse_aging_threshold(const char *opt,
>> >> + struct threshold_config *config)
>> >> +{
>> >> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> >> + struct threshold_entry e;
>> >> + const char *start = opt, *end;
>> >> + memset(config, 0, sizeof *config);
>> >> +
>> >> + while (start && *start) {
>> >> + end = strchr(start, ';');
>> >
>> >
>> > We should use 'strtok_r()' this again allows us to avoid the constant
copies replaced by single one.
>>
>> Make sense. I will change this in v2.
>>
>> >
>> >>
>> >> + if (end) {
>> >> + /* Big enough for any <ipv6>/<ipv6 mask>:<threshold> */
>> >> + char buf[128];
>> >> + ovs_strzcpy(buf, start, MIN(end - start + 1, sizeof
buf));
>> >> + if (!parse_threshold_entry(buf, &e)) {
>> >> + VLOG_WARN_RL(&rl, "Parsing aging threshold '%s'
failed.", buf);
>> >> + goto err;
>> >> + }
>> >> + start = end + 1;
>> >> + } else {
>> >> + if (!parse_threshold_entry(start, &e)) {
>> >> + VLOG_WARN_RL(&rl, "Parsing aging threshold '%s'
failed.",
>> >> + start);
>> >> + goto err;
>> >> + }
>> >> + start += strlen(start);
>> >> + }
>> >> + if (!e.plen) {
>> >> + config->default_threshold = e.threshold;
>> >> + } else if (e.is_v4) {
>> >> + config->n_v4_entries++;
>> >> + config->v4_entries = xrealloc(config->v4_entries,
>> >> + config->n_v4_entries *
sizeof e);
>> >> + config->v4_entries[config->n_v4_entries - 1] = e;
>> >> + } else {
>> >> + config->n_v6_entries++;
>> >> + config->v6_entries = xrealloc(config->v6_entries,
>> >> + config->n_v6_entries *
sizeof e);
>> >> + config->v6_entries[config->n_v6_entries - 1] = e;
>> >> + }
>> >> + }
>> >> +
>> >> + if (config->n_v4_entries > 0) {
>> >> + qsort(config->v4_entries, config->n_v4_entries, sizeof e,
>> >> + compare_entries_by_prefix_length);
>> >> + }
>> >> +
>> >> + if (config->n_v6_entries > 0) {
>> >> + qsort(config->v6_entries, config->n_v6_entries, sizeof e,
>> >> + compare_entries_by_prefix_length);
>> >> + }
>> >> +
>> >> + return true;
>> >> +
>> >> +err:
>> >> + threshold_config_destroy(config);
>> >> + return false;
>> >> +}
>> >> +
>> >> +static unsigned int
>> >> +find_threshold_for_ip(const char *ip_str,
>> >> + const struct threshold_config *config)
>> >> +{
>> >> + if (!ip_str) {
>> >> + return config->default_threshold;
>> >> + }
>> >> +
>> >> + ovs_be32 ipv4;
>> >> + struct in6_addr ipv6;
>> >> + if (ip_parse(ip_str, &ipv4)) {
>> >> + for (int i = 0; i < config->n_v4_entries; i++) {
>> >> + ovs_be32 masked_ip = ipv4 &
>> >> + be32_prefix_mask(config->v4_entries[i].plen);
>> >> + if (masked_ip == config->v4_entries[i].prefix.ipv4) {
>> >> + return config->v4_entries[i].threshold;
>> >> + }
>> >> + }
>> >> + } else if (ipv6_parse(ip_str, &ipv6)) {
>> >> + for (int i = 0; i < config->n_v6_entries; i++) {
>> >> + struct in6_addr v6_mask =
>> >> + ipv6_create_mask(config->v6_entries[i].plen);
>> >> + struct in6_addr masked_ip = ipv6_addr_bitand(&ipv6,
&v6_mask);
>> >> + if (ipv6_addr_equals(&masked_ip,
>> >> +
&config->v6_entries[i].prefix.ipv6)) {
>> >> + return config->v6_entries[i].threshold;
>> >> + }
>> >> + }
>> >> + }
>> >> + return config->default_threshold;
>> >> +}
>> >> +
>> >> +/* Parse the threshold option string (see the comment of the function
>> >> + * parse_aging_threshold), and returns the smallest threshold. */
>> >> +unsigned int
>> >> +min_mac_binding_age_threshold(const char *opt)
>> >> +{
>> >> + struct threshold_config config;
>> >> + if (!parse_aging_threshold(opt, &config)) {
>> >> + return 0;
>> >> + }
>> >> +
>> >> + unsigned int threshold = UINT_MAX;
>> >> + unsigned int t;
>> >> +
>> >> + for (int i = 0; i < config.n_v4_entries; i++) {
>> >> + t = config.v4_entries[i].threshold;
>> >> + if (t && t < threshold) {
>> >> + threshold = t;
>> >> + }
>> >> + }
>> >> +
>> >> + for (int i = 0; i < config.n_v6_entries; i++) {
>> >> + t = config.v6_entries[i].threshold;
>> >> + if (t && t < threshold) {
>> >> + threshold = t;
>> >> + }
>> >> + }
>> >> +
>> >> + t = config.default_threshold;
>> >> + if (t && t < threshold) {
>> >> + threshold = t;
>> >> + }
>> >> +
>> >> + threshold_config_destroy(&config);
>> >> +
>> >> + return threshold == UINT_MAX ? 0 : threshold;
>> >> +}
>> >> +
>> >> struct aging_context {
>> >> int64_t next_wake_ms;
>> >> int64_t time_wall_now;
>> >> uint32_t removal_limit;
>> >> uint32_t n_removed;
>> >> - uint64_t threshold;
>> >> + struct threshold_config *threshold;
>> >> };
>> >>
>> >> static struct aging_context
>> >> @@ -63,13 +304,14 @@ aging_context_init(uint32_t removal_limit)
>> >> .time_wall_now = time_wall_msec(),
>> >> .removal_limit = removal_limit,
>> >> .n_removed = 0,
>> >> - .threshold = 0,
>> >> + .threshold = NULL,
>> >> };
>> >> return ctx;
>> >> }
>> >>
>> >> static void
>> >> -aging_context_set_threshold(struct aging_context *ctx, uint64_t
threshold)
>> >> +aging_context_set_threshold(struct aging_context *ctx,
>> >> + struct threshold_config *threshold)
>> >> {
>> >> ctx->threshold = threshold;
>> >> }
>> >> @@ -81,19 +323,27 @@ aging_context_is_at_limit(struct aging_context
*ctx)
>> >> }
>> >>
>> >> static bool
>> >> -aging_context_handle_timestamp(struct aging_context *ctx, int64_t
timestamp)
>> >> +aging_context_handle_timestamp(struct aging_context *ctx, int64_t
timestamp,
>> >> + const char *ip)
>> >> {
>> >> int64_t elapsed = ctx->time_wall_now - timestamp;
>> >> if (elapsed < 0) {
>> >> return false;
>> >> }
>> >>
>> >> - if (elapsed >= ctx->threshold) {
>> >> + ovs_assert(ctx->threshold);
>> >> + uint64_t threshold = 1000 * find_threshold_for_ip(ip,
ctx->threshold);
>> >> +
>> >> + if (!threshold) {
>> >> + return false;
>> >> + }
>> >> +
>> >> + if (elapsed >= threshold) {
>> >> ctx->n_removed++;
>> >> return true;
>> >> }
>> >>
>> >> - ctx->next_wake_ms = MIN(ctx->next_wake_ms, (ctx->threshold -
elapsed));
>> >> + ctx->next_wake_ms = MIN(ctx->next_wake_ms, (threshold -
elapsed));
>> >> return false;
>> >> }
>> >>
>> >> @@ -121,13 +371,18 @@ mac_binding_aging_run_for_datapath(const struct
sbrec_datapath_binding *dp,
>> >> return;
>> >> }
>> >>
>> >> + if (!ctx->threshold->n_v4_entries &&
!ctx->threshold->n_v6_entries
>> >> + && !ctx->threshold->default_threshold) {
>> >> + return;
>> >> + }
>> >> +
>> >> struct sbrec_mac_binding *mb_index_row =
>> >> sbrec_mac_binding_index_init_row(mb_by_datapath);
>> >> sbrec_mac_binding_index_set_datapath(mb_index_row, dp);
>> >>
>> >> const struct sbrec_mac_binding *mb;
>> >> SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row,
mb_by_datapath) {
>> >> - if (aging_context_handle_timestamp(ctx, mb->timestamp)) {
>> >> + if (aging_context_handle_timestamp(ctx, mb->timestamp,
mb->ip)) {
>> >> sbrec_mac_binding_delete(mb);
>> >> if (aging_context_is_at_limit(ctx)) {
>> >> break;
>> >> @@ -166,13 +421,20 @@ en_mac_binding_aging_run(struct engine_node
*node, void *data OVS_UNUSED)
>> >> continue;
>> >> }
>> >>
>> >> - uint64_t threshold = smap_get_uint(&od->nbr->options,
>> >> -
"mac_binding_age_threshold", 0);
>> >> - aging_context_set_threshold(&ctx, threshold * 1000);
>> >> + struct threshold_config threshold_config;
>> >> + if (!parse_aging_threshold(smap_get(&od->nbr->options,
>> >> +
"mac_binding_age_threshold"),
>> >> + &threshold_config)) {
>> >> + return;
>> >> + }
>> >> +
>> >> + aging_context_set_threshold(&ctx, &threshold_config);
>> >>
>> >> mac_binding_aging_run_for_datapath(od->sb,
>> >>
sbrec_mac_binding_by_datapath,
>> >> &ctx);
>> >> + threshold_config_destroy(&threshold_config);
>> >> +
>> >> if (aging_context_is_at_limit(&ctx)) {
>> >> /* Schedule the next run after specified delay. */
>> >> ctx.next_wake_ms = AGING_BULK_REMOVAL_DELAY_MSEC;
>> >> @@ -255,7 +517,7 @@ fdb_run_for_datapath(const struct
sbrec_datapath_binding *dp,
>> >>
>> >> const struct sbrec_fdb *fdb;
>> >> SBREC_FDB_FOR_EACH_EQUAL (fdb, fdb_index_row, fdb_by_dp_key) {
>> >> - if (aging_context_handle_timestamp(ctx, fdb->timestamp)) {
>> >> + if (aging_context_handle_timestamp(ctx, fdb->timestamp,
NULL)) {
>> >> sbrec_fdb_delete(fdb);
>> >> if (aging_context_is_at_limit(ctx)) {
>> >> break;
>> >> @@ -293,11 +555,16 @@ en_fdb_aging_run(struct engine_node *node, void
*data OVS_UNUSED)
>> >> continue;
>> >> }
>> >>
>> >> - uint64_t threshold =
>> >> - smap_get_uint(&od->nbs->other_config,
"fdb_age_threshold", 0);
>> >> - aging_context_set_threshold(&ctx, threshold * 1000);
>> >> -
>> >> + struct threshold_config threshold_config;
>> >> + if (!parse_aging_threshold(smap_get(&od->nbs->other_config,
>> >> + "fdb_age_threshold"),
>> >> + &threshold_config)) {
>> >
>> >
>> > This would mean that fdb aging would accept the same arguments, but
that doesn't seem right. We should set only the default value instead of
the whole parsing.
>> >
>> Calling parse_aging_threshold here is just a convenient way to
initialize the other fields of the structure while setting the default
value. I agree it may be just more clear to explicitly initialize the
threshold_config and set the default value by calling smap_get_uint(). I
will do that in v2.
>>
>> I will wait for the consensus on the CIDR option's format and the
separate v.s. combined array before sending v2.
>
>
> I definitely don't want to block this patch on something that is more or
less cosmetic change so let's see what others think.
>
The option format may be more important since it is user facing. With an
example, we need to choose between:
A: 192.168.0.0/16:300;192.168.10.0/24:0;fe80::/10:600;1200
or
B: 192.168.0.0/16;300,192.168.10.0/24;0,fe80::/10;600,1200
Thanks,
Han
>>
>>
>> Thanks,
>> Han
>>
>> >>
>> >> + return;
>> >> + }
>> >> + aging_context_set_threshold(&ctx, &threshold_config);
>> >> fdb_run_for_datapath(od->sb, sbrec_fdb_by_dp_key, &ctx);
>> >> + threshold_config_destroy(&threshold_config);
>> >> +
>> >> if (aging_context_is_at_limit(&ctx)) {
>> >> /* Schedule the next run after specified delay. */
>> >> ctx.next_wake_ms = AGING_BULK_REMOVAL_DELAY_MSEC;
>> >> diff --git a/northd/aging.h b/northd/aging.h
>> >> index 650990e244aa..2cfe2fd44457 100644
>> >> --- a/northd/aging.h
>> >> +++ b/northd/aging.h
>> >> @@ -30,6 +30,9 @@ void *en_mac_binding_aging_waker_init(struct
engine_node *node,
>> >> struct engine_arg *arg);
>> >> void en_mac_binding_aging_waker_cleanup(void *data);
>> >>
>> >> +/* The MAC binding aging helper function. */
>> >> +unsigned int min_mac_binding_age_threshold(const char *opt);
>> >> +
>> >> /* The FDB aging node functions. */
>> >> void en_fdb_aging_run(struct engine_node *node, void *data);
>> >> void *en_fdb_aging_init(struct engine_node *node, struct engine_arg
*arg);
>> >> diff --git a/northd/northd.c b/northd/northd.c
>> >> index f8b046d83e85..4bc898982cf2 100644
>> >> --- a/northd/northd.c
>> >> +++ b/northd/northd.c
>> >> @@ -17,6 +17,7 @@
>> >> #include <stdlib.h>
>> >> #include <stdio.h>
>> >>
>> >> +#include "aging.h"
>> >> #include "debug.h"
>> >> #include "bitmap.h"
>> >> #include "coverage.h"
>> >> @@ -1166,8 +1167,14 @@ ovn_datapath_update_external_ids(struct
ovn_datapath *od)
>> >> smap_add(&ids, "always_learn_from_arp_request", "false");
>> >> }
>> >>
>> >> - uint32_t age_threshold = smap_get_uint(&od->nbr->options,
>> >> -
"mac_binding_age_threshold", 0);
>> >> + /* For timestamp refreshing, the smallest threshold of the
option is
>> >> + * set to SB to make sure all entries are refreshed in time.
>> >> + * XXX: This approach simplifies processing in
ovn-controller, but it
>> >> + * may be enhanced, if necessary, to parse the complete
CIDR-based
>> >> + * threshold configurations to SB to reduce unnecessary
refreshes. */
>> >> + uint32_t age_threshold = min_mac_binding_age_threshold(
>> >> + smap_get(&od->nbr->options,
>> >> +
"mac_binding_age_threshold"));
>> >> if (age_threshold) {
>> >> smap_add_format(&ids, "mac_binding_age_threshold",
>> >> "%u", age_threshold);
>> >> diff --git a/ovn-nb.xml b/ovn-nb.xml
>> >> index 1de0c30416ce..80733273484e 100644
>> >> --- a/ovn-nb.xml
>> >> +++ b/ovn-nb.xml
>> >> @@ -2686,10 +2686,65 @@ or
>> >> </column>
>> >>
>> >> <column name="options" key="mac_binding_age_threshold"
>> >> - type='{"type": "integer", "minInteger": 0,
"maxInteger": 4294967295}'>
>> >> - MAC binding aging <code>threshold</code> value in seconds.
MAC binding
>> >> - exceeding this timeout will be automatically removed. The
value
>> >> - defaults to 0, which means disabled.
>> >> + type='{"type": "string"}'>
>> >> + <p>
>> >> + Specifies the MAC binding aging thresholds based on
CIDRs, with the
>> >> + format:
<var>entry</var>[<code>;</code><var>entry</var>[...]],
>> >> + where each <var>entry</var> has the format:
>> >> + [<var>cidr</var><code>:</code>]<var>threshold</var>
>> >> + </p>
>> >> +
>> >> + <ul>
>> >> + <li>
>> >> + <var>cidr</var>: Can be either an IPv4 or IPv6 CIDR.
>> >> + </li>
>> >> + <li>
>> >> + <var>threshold</var>: Threshold value in seconds. MAC
bindings with
>> >> + IP addresses matching the specified CIDR that exceed
this timeout
>> >> + will be automatically removed.
>> >> + </li>
>> >> + </ul>
>> >> +
>> >> + <p>
>> >> + If an <var>entry</var> is provided without an CIDR (just
the
>> >> + threshold value), it specifies the default threshold for
MAC
>> >> + bindings that don't match any of the given CIDRs. If
there are
>> >> + multiple default threshold entries in the option, the
behavior is
>> >> + undefined.
>> >> + </p>
>> >> +
>> >> + <p>
>> >> + If there are multiple CIDRs matching a MAC binding IP,
the one with
>> >> + the longest prefix length takes effect. If there are
multiple
>> >> + entries with the same CIDR in the option, the behavior is
>> >> + undefined.
>> >> + </p>
>> >> +
>> >> + <p>
>> >> + If no matching CIDR is found for a MAC binding IP, and
no default
>> >> + threshold is specified, the behavior defaults to the
original: the
>> >> + binding will not be removed based on age.
>> >> + </p>
>> >> +
>> >> + <p>
>> >> + The value can also default to an empty string, which
means that the
>> >> + aging threshold is disabled. Any string not in the above
format is
>> >> + regarded as invalid and the aging is disabled.
>> >> + </p>
>> >> +
>> >> + <p>
>> >> + Example:
>> >> + <code>
192.168.0.0/16:300;192.168.10.0/24:0;fe80::/10:600;1200</code>
>> >> + </p>
>> >> +
>> >> + <p>
>> >> + This sets a threshold of 300 seconds for MAC bindings
with IP
>> >> + addresses in the 192.168.0.0/16 range, excluding the
192.168.1.0/24
>> >> + range (for which the aging is disabled), a threshold of
600 seconds
>> >> + for MAC bindings with IP addresses in the fe80::/10 IPv6
range, and
>> >> + a default threshold of 1200 seconds for all other MAC
bindings.
>> >> + </p>
>> >> +
>> >> </column>
>> >> </group>
>> >>
>> >> diff --git a/tests/ovn.at b/tests/ovn.at
>> >> index 637d92bedbc7..07b5c1ec57fc 100644
>> >> --- a/tests/ovn.at
>> >> +++ b/tests/ovn.at
>> >> @@ -34676,6 +34676,66 @@ OVS_WAIT_UNTIL([
>> >>
>> >> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=79 --no-stats |
strip_cookie], [0], [])
>> >>
>> >> +# Test CIDR-based threshold configuration
>> >> +check ovn-nbctl set logical_router gw
options:mac_binding_age_threshold="
192.168.10.0/255.255.255.0:2;192.168.10.64/26:0;192.168.10.20:0"
>> >> +check ovn-nbctl --wait=sb sync
>> >> +uuid=$(fetch_column datapath _uuid external_ids:name=gw)
>> >> +AT_CHECK([ovn-sbctl get datapath $uuid
external_ids:mac_binding_age_threshold], [0], [dnl
>> >> +"2"
>> >> +])
>> >> +
>> >> +# Send GARP to populate MAC binding table records
>> >> +send_garp hv1 ext1 10 # belong to 192.168.10.0/24
>> >> +send_garp hv2 ext2 20 # belong to 192.168.10.20/32
>> >> +send_garp hv2 ext2 65 # belong to 192.168.10.64/26
>> >> +
>> >> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q
"192.168.10.10"])
>> >> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q
"192.168.10.20"])
>> >> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q
"192.168.10.65"])
>> >> +
>> >> +OVS_WAIT_UNTIL([
>> >> + test "0" = "$(ovn-sbctl list mac_binding | grep -c
'192.168.10.10')"
>> >> +])
>> >> +# The other two should remain because the corresponding prefixes
have threshold 0
>> >> +AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.20"])
>> >> +AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.65"])
>> >> +check ovn-sbctl --all destroy mac_binding
>> >> +
>> >> +# Set the aging threshold mixed with IPv6 prefixes and default
threshold
>> >> +check ovn-nbctl set logical_router gw
options:mac_binding_age_threshold="1;192.168.10.64/26:0;ff00:1234::/32:888"
>> >> +check ovn-nbctl --wait=sb sync
>> >> +uuid=$(fetch_column datapath _uuid external_ids:name=gw)
>> >> +AT_CHECK([ovn-sbctl get datapath $uuid
external_ids:mac_binding_age_threshold], [0], [dnl
>> >> +"1"
>> >> +])
>> >> +
>> >> +# Send GARP to populate MAC binding table records
>> >> +send_garp hv1 ext1 10 # belong to default
>> >> +send_garp hv2 ext2 65 # belong to 192.168.10.64/26
>> >> +
>> >> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q
"192.168.10.10"])
>> >> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q
"192.168.10.65"])
>> >> +
>> >> +OVS_WAIT_UNTIL([
>> >> + test "0" = "$(ovn-sbctl list mac_binding | grep -c
'192.168.10.10')"
>> >> +])
>> >> +AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.65"])
>> >> +check ovn-sbctl --all destroy mac_binding
>> >> +
>> >> +# Set the aging threshold with invalid format
>> >> +check ovn-nbctl set logical_router gw
options:mac_binding_age_threshold="1;abc/26:0"
>> >> +check ovn-nbctl --wait=sb sync
>> >> +uuid=$(fetch_column datapath _uuid external_ids:name=gw)
>> >> +AT_CHECK([ovn-sbctl get datapath $uuid
external_ids:mac_binding_age_threshold], [1], [ignore], [ignore])
>> >> +
>> >> +# Send GARP to populate MAC binding table records
>> >> +send_garp hv1 ext1 10
>> >> +
>> >> +OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q
"192.168.10.10"])
>> >> +# The record is not deleted
>> >> +sleep 5
>> >> +AT_CHECK([ovn-sbctl list mac_binding | grep -q "192.168.10.10"])
>> >> +
>> >> OVN_CLEANUP([hv1], [hv2])
>> >> AT_CLEANUP
>> >> ])
>> >> --
>> >> 2.38.1
>> >>
>> >> _______________________________________________
>> >> dev mailing list
>> >> d...@openvswitch.org
>> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >>
>> >
>> > Thanks,
>> > Ales
>> >
>> > --
>> >
>> > Ales Musil
>> >
>> > Senior Software Engineer - OVN Core
>> >
>> > Red Hat EMEA
>> >
>> > amu...@redhat.com
>
>
> Thanks,
> Ales
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA
>
> amu...@redhat.com
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
