On Sat, Nov 4, 2023 at 5:45 AM Han Zhou <[email protected]> wrote:
>
>
> On Fri, Nov 3, 2023 at 1:08 AM Ales Musil <[email protected]> wrote:
> >
> >
> >
> > On Tue, Oct 24, 2023 at 9:36 PM Han Zhou <[email protected]> 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 <[email protected]>
> >> ---
> >
> >
> > 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).
> 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.
>
> 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
> >> [email protected]
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >
> > Thanks,
> > Ales
> >
> > --
> >
> > Ales Musil
> >
> > Senior Software Engineer - OVN Core
> >
> > Red Hat EMEA
> >
> > [email protected]
>
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected]
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
