diff mbox series

[ovs-dev,v2] controller: Add the capability to specify a min/max value for ct_zone.

Message ID 15f137a56c00a291e70d7580afd9f9816439e9b3.1720621898.git.lorenzo.bianconi@redhat.com
State Superseded
Headers show
Series [ovs-dev,v2] controller: Add the capability to specify a min/max value for ct_zone. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Lorenzo Bianconi July 10, 2024, 2:34 p.m. UTC 
Introduce the capability to specify boundaries (max and min values) for
the ct_zones dynamically selected by ovn-controller.

Reported-at: https://issues.redhat.com/browse/FDP-383
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- rely on get_chassis_external_id_value() to get ct_zone_range
- allow the user to configure the ct_zone_range instead of min and max values
- improve unit-test
---
 NEWS                        |  2 +
 controller/ct-zone.c        | 81 ++++++++++++++++++++++++++++++++-----
 controller/ct-zone.h        |  6 ++-
 controller/ovn-controller.c | 14 +++++--
 tests/ovn-controller.at     | 67 ++++++++++++++++++++++++++++++
 5 files changed, 156 insertions(+), 14 deletions(-)

Comments

Ales Musil July 24, 2024, 7:56 a.m. UTC | #1 
On Wed, Jul 10, 2024 at 4:34 PM Lorenzo Bianconi <
lorenzo.bianconi@redhat.com> wrote:

> Introduce the capability to specify boundaries (max and min values) for
> the ct_zones dynamically selected by ovn-controller.
>
> Reported-at: https://issues.redhat.com/browse/FDP-383
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v1:
> - rely on get_chassis_external_id_value() to get ct_zone_range
> - allow the user to configure the ct_zone_range instead of min and max
> values
> - improve unit-test
> ---
>  NEWS                        |  2 +
>  controller/ct-zone.c        | 81 ++++++++++++++++++++++++++++++++-----
>  controller/ct-zone.h        |  6 ++-
>  controller/ovn-controller.c | 14 +++++--
>  tests/ovn-controller.at     | 67 ++++++++++++++++++++++++++++++
>  5 files changed, 156 insertions(+), 14 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 3e392ff08..421f3cd0a 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -38,6 +38,8 @@ Post v24.03.0
>      ability to disable "VXLAN mode" to extend available tunnel IDs space
> for
>      datapaths from 4095 to 16711680.  For more details see man ovn-nb(5)
> for
>      mentioned option.
> +  - Added support to define boundaries (min and max values) for selected
> ct
> +    zones.
>
>  OVN v24.03.0 - 01 Mar 2024
>  --------------------------
> diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> index e4f66a52a..288ebe816 100644
> --- a/controller/ct-zone.c
> +++ b/controller/ct-zone.c
> @@ -14,7 +14,9 @@
>   */
>
>  #include <config.h>
> +#include <errno.h>
>
> +#include "chassis.h"
>  #include "ct-zone.h"
>  #include "local_data.h"
>  #include "openvswitch/vlog.h"
> @@ -29,7 +31,8 @@ static void ct_zone_add_pending(struct shash
> *pending_ct_zones,
>                                  int zone, bool add, const char *name);
>  static int ct_zone_get_snat(const struct sbrec_datapath_binding *dp);
>  static bool ct_zone_assign_unused(struct ct_zone_ctx *ctx,
> -                                  const char *zone_name, int *scan_start);
> +                                  const char *zone_name,
> +                                  int *scan_start, int scan_stop);
>  static bool ct_zone_remove(struct ct_zone_ctx *ctx,
>                             struct simap_node *ct_zone);
>
> @@ -87,12 +90,61 @@ ct_zones_restore(struct ct_zone_ctx *ctx,
>      }
>  }
>
> +void
> +ct_zones_parse_range(const struct ovsrec_open_vswitch_table *ovs_table,
> +                     int *min_ct_zone, int *max_ct_zone)
> +{
> +    /* Set default values. */
> +    *min_ct_zone = 1;
> +    *max_ct_zone = MAX_CT_ZONES + 1;
> +
> +    const struct ovsrec_open_vswitch *cfg =
> +        ovsrec_open_vswitch_table_first(ovs_table);
> +    if (!cfg) {
> +        return;
> +    }
> +
> +    const char *chassis_id = get_ovs_chassis_id(ovs_table);
> +    const char *range = get_chassis_external_id_value(&cfg->external_ids,
> +                                                      chassis_id,
> +                                                      "ct_zone_range",
> NULL);
> +    if (!range) {
> +        return;
> +    }
> +
> +    char *ptr = NULL, *tokstr = xstrdup(range);
> +    char *range_min = strtok_r(tokstr, "-", &ptr);
> +    if (!range_min) {
> +        goto out;
> +    }
> +
> +    int min = strtol(range_min, NULL, 10);
> +    if (errno == EINVAL || min < 1) {
> +        goto out;
> +    }
> +    *min_ct_zone = min;
> +
> +    char *range_max = strtok_r(NULL, "-", &ptr);
> +    if (!range_max) {
> +        goto out;
> +    }
> +
> +    int max = strtol(range_max, NULL, 10);
> +    if (errno == EINVAL || max > MAX_CT_ZONES + 1) {
> +        goto out;
> +    }
> +    *max_ct_zone = max;
> +out:
> +    free(tokstr);
> +}
> +
>  void
>  ct_zones_update(const struct sset *local_lports,
> +                const struct ovsrec_open_vswitch_table *ovs_table,
>                  const struct hmap *local_datapaths, struct ct_zone_ctx
> *ctx)
>  {
> +    int min_ct_zone, max_ct_zone;
>      struct simap_node *ct_zone;
> -    int scan_start = 1;
>      const char *user;
>      struct sset all_users = SSET_INITIALIZER(&all_users);
>      struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones);
> @@ -131,9 +183,12 @@ ct_zones_update(const struct sset *local_lports,
>          free(snat);
>      }
>
> +    ct_zones_parse_range(ovs_table, &min_ct_zone, &max_ct_zone);
> +
>      /* Delete zones that do not exist in above sset. */
>      SIMAP_FOR_EACH_SAFE (ct_zone, &ctx->current) {
> -        if (!sset_contains(&all_users, ct_zone->name)) {
> +        if (!sset_contains(&all_users, ct_zone->name) ||
> +            ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone) {
>              ct_zone_remove(ctx, ct_zone);
>          } else if (!simap_find(&req_snat_zones, ct_zone->name)) {
>              bitmap_set1(unreq_snat_zones_map, ct_zone->data);
> @@ -195,7 +250,7 @@ ct_zones_update(const struct sset *local_lports,
>              continue;
>          }
>
> -        ct_zone_assign_unused(ctx, user, &scan_start);
> +        ct_zone_assign_unused(ctx, user, &min_ct_zone, max_ct_zone);
>      }
>
>      simap_destroy(&req_snat_zones);
> @@ -296,11 +351,19 @@ ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
>  /* Returns "true" if there was an update to the context. */
>  bool
>  ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
> -                           bool updated, int *scan_start)
> +                           bool updated, int *scan_start,
> +                           int min_ct_zone, int max_ct_zone)
>  {
>      struct simap_node *ct_zone = simap_find(&ctx->current, name);
> +
> +    if (ct_zone &&
> +        (ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone)) {
> +        ct_zone_remove(ctx, ct_zone);
> +        ct_zone = NULL;
> +    }
> +
>      if (updated && !ct_zone) {
> -        ct_zone_assign_unused(ctx, name, scan_start);
> +        ct_zone_assign_unused(ctx, name, scan_start, max_ct_zone);
>          return true;
>      } else if (!updated && ct_zone_remove(ctx, ct_zone)) {
>          return true;
> @@ -312,11 +375,11 @@ ct_zone_handle_port_update(struct ct_zone_ctx *ctx,
> const char *name,
>
>  static bool
>  ct_zone_assign_unused(struct ct_zone_ctx *ctx, const char *zone_name,
> -                      int *scan_start)
> +                      int *scan_start, int scan_stop)
>  {
>      /* We assume that there are 64K zones and that we own them all. */
> -    int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, MAX_CT_ZONES + 1);
> -    if (zone == MAX_CT_ZONES + 1) {
> +    int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, scan_stop);
> +    if (zone == scan_stop) {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>          VLOG_WARN_RL(&rl, "exhausted all ct zones");
>          return false;
> diff --git a/controller/ct-zone.h b/controller/ct-zone.h
> index 889bdf2fc..690b2ec7c 100644
> --- a/controller/ct-zone.h
> +++ b/controller/ct-zone.h
> @@ -56,11 +56,14 @@ struct ct_zone_pending_entry {
>      enum ct_zone_pending_state state;
>  };
>
> +void ct_zones_parse_range(const struct ovsrec_open_vswitch_table
> *ovs_table,
> +                          int *min_ct_zone, int *max_ct_zone);
>  void ct_zones_restore(struct ct_zone_ctx *ctx,
>                        const struct ovsrec_open_vswitch_table *ovs_table,
>                        const struct sbrec_datapath_binding_table *dp_table,
>                        const struct ovsrec_bridge *br_int);
>  void ct_zones_update(const struct sset *local_lports,
> +                     const struct ovsrec_open_vswitch_table *ovs_table,
>                       const struct hmap *local_datapaths,
>                       struct ct_zone_ctx *ctx);
>  void ct_zones_commit(const struct ovsrec_bridge *br_int,
> @@ -69,6 +72,7 @@ void ct_zones_pending_clear_commited(struct shash
> *pending);
>  bool ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
>                                const struct sbrec_datapath_binding *dp);
>  bool ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
> -                                bool updated, int *scan_start);
> +                                bool updated, int *scan_start,
> +                                int min_ct_zone, int max_ct_zone);
>
>  #endif /* controller/ct-zone.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index d6d001b1a..54b3a1cd5 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2230,8 +2230,8 @@ en_ct_zones_run(struct engine_node *node, void *data)
>      const struct ovsrec_bridge *br_int = get_br_int(bridge_table,
> ovs_table);
>
>      ct_zones_restore(&ct_zones_data->ctx, ovs_table, dp_table, br_int);
> -    ct_zones_update(&rt_data->local_lports, &rt_data->local_datapaths,
> -                    &ct_zones_data->ctx);
> +    ct_zones_update(&rt_data->local_lports, ovs_table,
> +                    &rt_data->local_datapaths, &ct_zones_data->ctx);
>
>      ct_zones_data->recomputed = true;
>      engine_set_node_state(node, EN_UPDATED);
> @@ -2275,6 +2275,8 @@ ct_zones_runtime_data_handler(struct engine_node
> *node, void *data)
>  {
>      struct ed_type_runtime_data *rt_data =
>          engine_get_input_data("runtime_data", node);
> +    const struct ovsrec_open_vswitch_table *ovs_table =
> +        EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
>
>      /* There is no tracked data. Fall back to full recompute of ct_zones.
> */
>      if (!rt_data->tracked) {
> @@ -2284,11 +2286,14 @@ ct_zones_runtime_data_handler(struct engine_node
> *node, void *data)
>      struct ed_type_ct_zones *ct_zones_data = data;
>
>      struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> +    int scan_start, min_ct_zone, max_ct_zone;
>      struct tracked_datapath *tdp;
> -    int scan_start = 1;
>
>      bool updated = false;
>
> +    ct_zones_parse_range(ovs_table, &min_ct_zone, &max_ct_zone);
> +    scan_start = min_ct_zone;
> +
>      HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
>          if (tdp->tracked_type == TRACKED_RESOURCE_NEW) {
>              /* A new datapath has been added. Fall back to full
> recompute. */
> @@ -2312,7 +2317,8 @@ ct_zones_runtime_data_handler(struct engine_node
> *node, void *data)
>                      t_lport->tracked_type == TRACKED_RESOURCE_UPDATED;
>              updated |= ct_zone_handle_port_update(&ct_zones_data->ctx,
>
>  t_lport->pb->logical_port,
> -                                                  port_updated,
> &scan_start);
> +                                                  port_updated,
> &scan_start,
> +                                                  min_ct_zone,
> max_ct_zone);
>          }
>      }
>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 2cb86dc98..fe575a014 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -3127,3 +3127,70 @@ OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1235:
> connected' hv1/ovn-controller.log])
>
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - CT zone min/max boundaries])
> +ovn_start
> +
> +check_ct_zone_min() {
> +    min_val=$1
> +    OVS_WAIT_UNTIL([test $(ovn-appctl -t ovn-controller ct-zone-list |
> awk '{print $2}' | sort | head -n1) -ge ${min_val}])
> +}
> +
> +check_ct_zone_max() {
> +    max_val=$1
> +    AT_CHECK([test $(ovn-appctl -t ovn-controller ct-zone-list | awk
> '{print $2}' | sort | tail -n1) -le ${max_val}])
> +}
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +check ovn-appctl -t ovn-controller vlog/set dbg:ct_zone
> +
> +check ovs-vsctl add-port br-int lsp0 \
> +    -- set Interface lsp0 external-ids:iface-id=lsp0
> +
> +check ovn-nbctl lr-add lr
> +
> +check ovn-nbctl ls-add ls
> +check ovn-nbctl lsp-add ls ls-lr
> +check ovn-nbctl lsp-set-type ls-lr router
> +check ovn-nbctl lsp-set-addresses ls-lr router
> +check ovn-nbctl lrp-add lr lr-ls 00:00:00:00:00:01 10.0.0.1
> +
> +check ovn-nbctl lsp-add ls lsp0
> +check ovn-nbctl lsp-set-addresses lsp0 "00:00:00:00:00:02 10.0.0.2"
> +
> +check ovn-nbctl lrp-add lr lrp-gw 01:00:00:00:00:01 172.16.0.1
> +check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1
> +
> +# check regular boundaries
> +check_ct_zone_min 1
> +check_ct_zone_max 10
> +
> +# increase boundaries
> +ovs-vsctl set Open_vSwitch . external_ids:ct_zone_range=\"10-20\"
> +check_ct_zone_min 10
> +check_ct_zone_max 20
> +
> +# reset min boundary
> +ovs-vsctl set Open_vSwitch . external_ids:ct_zone_range=\"5-20\"
> +
> +# add a new port to the switch
> +check ovs-vsctl add-port br-int lsp1 \
> +    -- set Interface lsp1 external-ids:iface-id=lsp1
> +check ovn-nbctl lsp-add ls lsp1
> +check ovn-nbctl lsp-set-addresses lsp1 "00:00:00:00:00:03 10.0.0.3"
> +check_ct_zone_min 5
> +check_ct_zone_max 20
> +
> +check ovn-nbctl set logical_router lr options:snat-ct-zone=2
> +check_ct_zone_min 2
> +check_ct_zone_max 20
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> --
> 2.45.2
>
>
Looks good to me, thanks.

Acked-by: Ales Musil <amusil@redhat.com>
Xavier Simonart July 25, 2024, 1:17 p.m. UTC | #2 
Hi Lorenzo

Thanks for the patch!
I have some small comments below.


On Wed, Jul 24, 2024 at 10:01 AM Ales Musil <amusil@redhat.com> wrote:

> On Wed, Jul 10, 2024 at 4:34 PM Lorenzo Bianconi <
> lorenzo.bianconi@redhat.com> wrote:
>
> > Introduce the capability to specify boundaries (max and min values) for
> > the ct_zones dynamically selected by ovn-controller.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-383
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> > Changes since v1:
> > - rely on get_chassis_external_id_value() to get ct_zone_range
> > - allow the user to configure the ct_zone_range instead of min and max
> > values
> > - improve unit-test
> > ---
> >  NEWS                        |  2 +
> >  controller/ct-zone.c        | 81 ++++++++++++++++++++++++++++++++-----
> >  controller/ct-zone.h        |  6 ++-
> >  controller/ovn-controller.c | 14 +++++--
> >  tests/ovn-controller.at     | 67 ++++++++++++++++++++++++++++++
> >  5 files changed, 156 insertions(+), 14 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 3e392ff08..421f3cd0a 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -38,6 +38,8 @@ Post v24.03.0
> >      ability to disable "VXLAN mode" to extend available tunnel IDs space
> > for
> >      datapaths from 4095 to 16711680.  For more details see man ovn-nb(5)
> > for
> >      mentioned option.
> > +  - Added support to define boundaries (min and max values) for selected
> > ct
> > +    zones.
> >
>
Should this be documented also in the xml doc?
I think we should also explain whether the "max" in the range is included
or not (i.e. does range="5-10"means zones 5 to 9 are supported, or 5 to 10).
I might be misreading the code below, but it looks unclear to me.
We could probably also document the maximum supported value in the range
i.e. 65535)

>  OVN v24.03.0 - 01 Mar 2024
> >  --------------------------
> > diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> > index e4f66a52a..288ebe816 100644
> > --- a/controller/ct-zone.c
> > +++ b/controller/ct-zone.c
> > @@ -14,7 +14,9 @@
> >   */
> >
> >  #include <config.h>
> > +#include <errno.h>
> >
> > +#include "chassis.h"
> >  #include "ct-zone.h"
> >  #include "local_data.h"
> >  #include "openvswitch/vlog.h"
> > @@ -29,7 +31,8 @@ static void ct_zone_add_pending(struct shash
> > *pending_ct_zones,
> >                                  int zone, bool add, const char *name);
> >  static int ct_zone_get_snat(const struct sbrec_datapath_binding *dp);
> >  static bool ct_zone_assign_unused(struct ct_zone_ctx *ctx,
> > -                                  const char *zone_name, int
> *scan_start);
> > +                                  const char *zone_name,
> > +                                  int *scan_start, int scan_stop);
> >  static bool ct_zone_remove(struct ct_zone_ctx *ctx,
> >                             struct simap_node *ct_zone);
> >
> > @@ -87,12 +90,61 @@ ct_zones_restore(struct ct_zone_ctx *ctx,
> >      }
> >  }
> >
> > +void
> > +ct_zones_parse_range(const struct ovsrec_open_vswitch_table *ovs_table,
> > +                     int *min_ct_zone, int *max_ct_zone)
> > +{
> > +    /* Set default values. */
> > +    *min_ct_zone = 1;
> > +    *max_ct_zone = MAX_CT_ZONES + 1;
> > +
> > +    const struct ovsrec_open_vswitch *cfg =
> > +        ovsrec_open_vswitch_table_first(ovs_table);
> > +    if (!cfg) {
> > +        return;
> > +    }
> > +
> > +    const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > +    const char *range =
> get_chassis_external_id_value(&cfg->external_ids,
> > +                                                      chassis_id,
> > +                                                      "ct_zone_range",
> > NULL);
> > +    if (!range) {
> > +        return;
> > +    }
> > +
> > +    char *ptr = NULL, *tokstr = xstrdup(range);
> > +    char *range_min = strtok_r(tokstr, "-", &ptr);
> > +    if (!range_min) {
> > +        goto out;
> > +    }
> > +
> > +    int min = strtol(range_min, NULL, 10);
> > +    if (errno == EINVAL || min < 1) {
> > +        goto out;
> > +    }
> > +    *min_ct_zone = min;
> > +
> > +    char *range_max = strtok_r(NULL, "-", &ptr);
> > +    if (!range_max) {
> > +        goto out;
> > +    }
> > +
> > +    int max = strtol(range_max, NULL, 10);
> > +    if (errno == EINVAL || max > MAX_CT_ZONES + 1) {
> > +        goto out;
> > +    }
> > +    *max_ct_zone = max;
> > +out:
> > +    free(tokstr);
> > +}
> > +
> >  void
> >  ct_zones_update(const struct sset *local_lports,
> > +                const struct ovsrec_open_vswitch_table *ovs_table,
> >                  const struct hmap *local_datapaths, struct ct_zone_ctx
> > *ctx)
> >  {
> > +    int min_ct_zone, max_ct_zone;
> >      struct simap_node *ct_zone;
> > -    int scan_start = 1;
> >      const char *user;
> >      struct sset all_users = SSET_INITIALIZER(&all_users);
> >      struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones);
> > @@ -131,9 +183,12 @@ ct_zones_update(const struct sset *local_lports,
> >          free(snat);
> >      }
> >
> > +    ct_zones_parse_range(ovs_table, &min_ct_zone, &max_ct_zone);
> > +
> >      /* Delete zones that do not exist in above sset. */
> >      SIMAP_FOR_EACH_SAFE (ct_zone, &ctx->current) {
> > -        if (!sset_contains(&all_users, ct_zone->name)) {
> > +        if (!sset_contains(&all_users, ct_zone->name) ||
> > +            ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone)
> {
> >              ct_zone_remove(ctx, ct_zone);
> >          } else if (!simap_find(&req_snat_zones, ct_zone->name)) {
> >              bitmap_set1(unreq_snat_zones_map, ct_zone->data);
> > @@ -195,7 +250,7 @@ ct_zones_update(const struct sset *local_lports,
> >              continue;
> >          }
> >
> > -        ct_zone_assign_unused(ctx, user, &scan_start);
> > +        ct_zone_assign_unused(ctx, user, &min_ct_zone, max_ct_zone);
> >      }
> >
> >      simap_destroy(&req_snat_zones);
> > @@ -296,11 +351,19 @@ ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
> >  /* Returns "true" if there was an update to the context. */
> >  bool
> >  ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
> > -                           bool updated, int *scan_start)
> > +                           bool updated, int *scan_start,
> > +                           int min_ct_zone, int max_ct_zone)
> >  {
> >      struct simap_node *ct_zone = simap_find(&ctx->current, name);
> > +
> > +    if (ct_zone &&
> > +        (ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone)) {
>
So, IIUC a zone range "5-10" means zones 5 to 10 are supported (a zone=10
would not be removed if configuring range="5-10")?

> > +        ct_zone_remove(ctx, ct_zone);
> > +        ct_zone = NULL;
> > +    }
> > +
> >      if (updated && !ct_zone) {
> > -        ct_zone_assign_unused(ctx, name, scan_start);
> > +        ct_zone_assign_unused(ctx, name, scan_start, max_ct_zone);
> >          return true;
> >      } else if (!updated && ct_zone_remove(ctx, ct_zone)) {
> >          return true;
> > @@ -312,11 +375,11 @@ ct_zone_handle_port_update(struct ct_zone_ctx *ctx,
> > const char *name,
> >
> >  static bool
> >  ct_zone_assign_unused(struct ct_zone_ctx *ctx, const char *zone_name,
> > -                      int *scan_start)
> > +                      int *scan_start, int scan_stop)
> >  {
> >      /* We assume that there are 64K zones and that we own them all. */
> > -    int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, MAX_CT_ZONES +
> 1);
> > -    if (zone == MAX_CT_ZONES + 1) {
> > +    int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, scan_stop);
>
> +    if (zone == scan_stop) {
>
But here, IIUC, a range "5-10" means only zones 5 to 9 can be assigned ?

> >          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> >          VLOG_WARN_RL(&rl, "exhausted all ct zones");
> >          return false;
> > diff --git a/controller/ct-zone.h b/controller/ct-zone.h
> > index 889bdf2fc..690b2ec7c 100644
> > --- a/controller/ct-zone.h
> > +++ b/controller/ct-zone.h
> > @@ -56,11 +56,14 @@ struct ct_zone_pending_entry {
> >      enum ct_zone_pending_state state;
> >  };
> >
> > +void ct_zones_parse_range(const struct ovsrec_open_vswitch_table
> > *ovs_table,
> > +                          int *min_ct_zone, int *max_ct_zone);
> >  void ct_zones_restore(struct ct_zone_ctx *ctx,
> >                        const struct ovsrec_open_vswitch_table *ovs_table,
> >                        const struct sbrec_datapath_binding_table
> *dp_table,
> >                        const struct ovsrec_bridge *br_int);
> >  void ct_zones_update(const struct sset *local_lports,
> > +                     const struct ovsrec_open_vswitch_table *ovs_table,
> >                       const struct hmap *local_datapaths,
> >                       struct ct_zone_ctx *ctx);
> >  void ct_zones_commit(const struct ovsrec_bridge *br_int,
> > @@ -69,6 +72,7 @@ void ct_zones_pending_clear_commited(struct shash
> > *pending);
> >  bool ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
> >                                const struct sbrec_datapath_binding *dp);
> >  bool ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char
> *name,
> > -                                bool updated, int *scan_start);
> > +                                bool updated, int *scan_start,
> > +                                int min_ct_zone, int max_ct_zone);
> >
> >  #endif /* controller/ct-zone.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index d6d001b1a..54b3a1cd5 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -2230,8 +2230,8 @@ en_ct_zones_run(struct engine_node *node, void
> *data)
> >      const struct ovsrec_bridge *br_int = get_br_int(bridge_table,
> > ovs_table);
> >
> >      ct_zones_restore(&ct_zones_data->ctx, ovs_table, dp_table, br_int);
> > -    ct_zones_update(&rt_data->local_lports, &rt_data->local_datapaths,
> > -                    &ct_zones_data->ctx);
> > +    ct_zones_update(&rt_data->local_lports, ovs_table,
> > +                    &rt_data->local_datapaths, &ct_zones_data->ctx);
> >
> >      ct_zones_data->recomputed = true;
> >      engine_set_node_state(node, EN_UPDATED);
> > @@ -2275,6 +2275,8 @@ ct_zones_runtime_data_handler(struct engine_node
> > *node, void *data)
> >  {
> >      struct ed_type_runtime_data *rt_data =
> >          engine_get_input_data("runtime_data", node);
> > +    const struct ovsrec_open_vswitch_table *ovs_table =
> > +        EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
> >
> >      /* There is no tracked data. Fall back to full recompute of
> ct_zones.
> > */
> >      if (!rt_data->tracked) {
> > @@ -2284,11 +2286,14 @@ ct_zones_runtime_data_handler(struct engine_node
> > *node, void *data)
> >      struct ed_type_ct_zones *ct_zones_data = data;
> >
> >      struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> > +    int scan_start, min_ct_zone, max_ct_zone;
> >      struct tracked_datapath *tdp;
> > -    int scan_start = 1;
> >
> >      bool updated = false;
> >
> > +    ct_zones_parse_range(ovs_table, &min_ct_zone, &max_ct_zone);
> > +    scan_start = min_ct_zone;
> > +
> >      HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
> >          if (tdp->tracked_type == TRACKED_RESOURCE_NEW) {
> >              /* A new datapath has been added. Fall back to full
> > recompute. */
> > @@ -2312,7 +2317,8 @@ ct_zones_runtime_data_handler(struct engine_node
> > *node, void *data)
> >                      t_lport->tracked_type == TRACKED_RESOURCE_UPDATED;
> >              updated |= ct_zone_handle_port_update(&ct_zones_data->ctx,
> >
> >  t_lport->pb->logical_port,
> > -                                                  port_updated,
> > &scan_start);
> > +                                                  port_updated,
> > &scan_start,
> > +                                                  min_ct_zone,
> > max_ct_zone);
> >          }
> >      }
> >
> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > index 2cb86dc98..fe575a014 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -3127,3 +3127,70 @@ OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1235:
> > connected' hv1/ovn-controller.log])
> >
> >  OVN_CLEANUP([hv1])
> >  AT_CLEANUP
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ovn-controller - CT zone min/max boundaries])
> > +ovn_start
> > +
> > +check_ct_zone_min() {
> > +    min_val=$1
> > +    OVS_WAIT_UNTIL([test $(ovn-appctl -t ovn-controller ct-zone-list |
> > awk '{print $2}' | sort | head -n1) -ge ${min_val}])
> > +}
> > +
> > +check_ct_zone_max() {
> > +    max_val=$1
> > +    AT_CHECK([test $(ovn-appctl -t ovn-controller ct-zone-list | awk
> > '{print $2}' | sort | tail -n1) -le ${max_val}])
> > +}
> > +
> > +net_add n1
> > +sim_add hv1
> > +as hv1
> > +check ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +
> > +check ovn-appctl -t ovn-controller vlog/set dbg:ct_zone
> > +
> > +check ovs-vsctl add-port br-int lsp0 \
> > +    -- set Interface lsp0 external-ids:iface-id=lsp0
> > +
> > +check ovn-nbctl lr-add lr
> > +
> > +check ovn-nbctl ls-add ls
> > +check ovn-nbctl lsp-add ls ls-lr
> > +check ovn-nbctl lsp-set-type ls-lr router
> > +check ovn-nbctl lsp-set-addresses ls-lr router
> > +check ovn-nbctl lrp-add lr lr-ls 00:00:00:00:00:01 10.0.0.1
> > +
> > +check ovn-nbctl lsp-add ls lsp0
> > +check ovn-nbctl lsp-set-addresses lsp0 "00:00:00:00:00:02 10.0.0.2"
> > +
> > +check ovn-nbctl lrp-add lr lrp-gw 01:00:00:00:00:01 172.16.0.1
> > +check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1
> > +
> > +# check regular boundaries
> > +check_ct_zone_min 1
> > +check_ct_zone_max 10
> > +
> > +# increase boundaries
> > +ovs-vsctl set Open_vSwitch . external_ids:ct_zone_range=\"10-20\"
> > +check_ct_zone_min 10
> > +check_ct_zone_max 20
>
> +
> > +# reset min boundary
> > +ovs-vsctl set Open_vSwitch . external_ids:ct_zone_range=\"5-20\"
> > +
> > +# add a new port to the switch
> > +check ovs-vsctl add-port br-int lsp1 \
> > +    -- set Interface lsp1 external-ids:iface-id=lsp1
> > +check ovn-nbctl lsp-add ls lsp1
> > +check ovn-nbctl lsp-set-addresses lsp1 "00:00:00:00:00:03 10.0.0.3"
> > +check_ct_zone_min 5
> > +check_ct_zone_max 20
> > +
> > +check ovn-nbctl set logical_router lr options:snat-ct-zone=2
> > +check_ct_zone_min 2
> > +check_ct_zone_max 20
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
> > --
> > 2.45.2
> >
> >
> Looks good to me, thanks.
>
> Acked-by: Ales Musil <amusil@redhat.com>
>
> Thanks
Xavier

> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amusil@redhat.com
> <https://red.ht/sig>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Lorenzo Bianconi July 26, 2024, 10:34 a.m. UTC | #3 
> Hi Lorenzo
> 
> Thanks for the patch!
> I have some small comments below.

Hi Xavier,

thx for the review.

> 
> 
> On Wed, Jul 24, 2024 at 10:01 AM Ales Musil <amusil@redhat.com> wrote:
> 
> > On Wed, Jul 10, 2024 at 4:34 PM Lorenzo Bianconi <
> > lorenzo.bianconi@redhat.com> wrote:
> >
> > > Introduce the capability to specify boundaries (max and min values) for
> > > the ct_zones dynamically selected by ovn-controller.
> > >
> > > Reported-at: https://issues.redhat.com/browse/FDP-383
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > > ---
> > > Changes since v1:
> > > - rely on get_chassis_external_id_value() to get ct_zone_range
> > > - allow the user to configure the ct_zone_range instead of min and max
> > > values
> > > - improve unit-test
> > > ---
> > >  NEWS                        |  2 +
> > >  controller/ct-zone.c        | 81 ++++++++++++++++++++++++++++++++-----
> > >  controller/ct-zone.h        |  6 ++-
> > >  controller/ovn-controller.c | 14 +++++--
> > >  tests/ovn-controller.at     | 67 ++++++++++++++++++++++++++++++
> > >  5 files changed, 156 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/NEWS b/NEWS
> > > index 3e392ff08..421f3cd0a 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -38,6 +38,8 @@ Post v24.03.0
> > >      ability to disable "VXLAN mode" to extend available tunnel IDs space
> > > for
> > >      datapaths from 4095 to 16711680.  For more details see man ovn-nb(5)
> > > for
> > >      mentioned option.
> > > +  - Added support to define boundaries (min and max values) for selected
> > > ct
> > > +    zones.
> > >
> >
> Should this be documented also in the xml doc?
> I think we should also explain whether the "max" in the range is included
> or not (i.e. does range="5-10"means zones 5 to 9 are supported, or 5 to 10).
> I might be misreading the code below, but it looks unclear to me.
> We could probably also document the maximum supported value in the range
> i.e. 65535)

ack, I will add it in v3.

> 
> >  OVN v24.03.0 - 01 Mar 2024
> > >  --------------------------
> > > diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> > > index e4f66a52a..288ebe816 100644
> > > --- a/controller/ct-zone.c
> > > +++ b/controller/ct-zone.c
> > > @@ -14,7 +14,9 @@
> > >   */
> > >
> > >  #include <config.h>
> > > +#include <errno.h>
> > >
> > > +#include "chassis.h"
> > >  #include "ct-zone.h"
> > >  #include "local_data.h"
> > >  #include "openvswitch/vlog.h"
> > > @@ -29,7 +31,8 @@ static void ct_zone_add_pending(struct shash
> > > *pending_ct_zones,
> > >                                  int zone, bool add, const char *name);
> > >  static int ct_zone_get_snat(const struct sbrec_datapath_binding *dp);
> > >  static bool ct_zone_assign_unused(struct ct_zone_ctx *ctx,
> > > -                                  const char *zone_name, int
> > *scan_start);
> > > +                                  const char *zone_name,
> > > +                                  int *scan_start, int scan_stop);
> > >  static bool ct_zone_remove(struct ct_zone_ctx *ctx,
> > >                             struct simap_node *ct_zone);
> > >
> > > @@ -87,12 +90,61 @@ ct_zones_restore(struct ct_zone_ctx *ctx,
> > >      }
> > >  }
> > >
> > > +void
> > > +ct_zones_parse_range(const struct ovsrec_open_vswitch_table *ovs_table,
> > > +                     int *min_ct_zone, int *max_ct_zone)
> > > +{
> > > +    /* Set default values. */
> > > +    *min_ct_zone = 1;
> > > +    *max_ct_zone = MAX_CT_ZONES + 1;
> > > +
> > > +    const struct ovsrec_open_vswitch *cfg =
> > > +        ovsrec_open_vswitch_table_first(ovs_table);
> > > +    if (!cfg) {
> > > +        return;
> > > +    }
> > > +
> > > +    const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > > +    const char *range =
> > get_chassis_external_id_value(&cfg->external_ids,
> > > +                                                      chassis_id,
> > > +                                                      "ct_zone_range",
> > > NULL);
> > > +    if (!range) {
> > > +        return;
> > > +    }
> > > +
> > > +    char *ptr = NULL, *tokstr = xstrdup(range);
> > > +    char *range_min = strtok_r(tokstr, "-", &ptr);
> > > +    if (!range_min) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    int min = strtol(range_min, NULL, 10);
> > > +    if (errno == EINVAL || min < 1) {
> > > +        goto out;
> > > +    }
> > > +    *min_ct_zone = min;
> > > +
> > > +    char *range_max = strtok_r(NULL, "-", &ptr);
> > > +    if (!range_max) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    int max = strtol(range_max, NULL, 10);
> > > +    if (errno == EINVAL || max > MAX_CT_ZONES + 1) {
> > > +        goto out;
> > > +    }
> > > +    *max_ct_zone = max;
> > > +out:
> > > +    free(tokstr);
> > > +}
> > > +
> > >  void
> > >  ct_zones_update(const struct sset *local_lports,
> > > +                const struct ovsrec_open_vswitch_table *ovs_table,
> > >                  const struct hmap *local_datapaths, struct ct_zone_ctx
> > > *ctx)
> > >  {
> > > +    int min_ct_zone, max_ct_zone;
> > >      struct simap_node *ct_zone;
> > > -    int scan_start = 1;
> > >      const char *user;
> > >      struct sset all_users = SSET_INITIALIZER(&all_users);
> > >      struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones);
> > > @@ -131,9 +183,12 @@ ct_zones_update(const struct sset *local_lports,
> > >          free(snat);
> > >      }
> > >
> > > +    ct_zones_parse_range(ovs_table, &min_ct_zone, &max_ct_zone);
> > > +
> > >      /* Delete zones that do not exist in above sset. */
> > >      SIMAP_FOR_EACH_SAFE (ct_zone, &ctx->current) {
> > > -        if (!sset_contains(&all_users, ct_zone->name)) {
> > > +        if (!sset_contains(&all_users, ct_zone->name) ||
> > > +            ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone)
> > {
> > >              ct_zone_remove(ctx, ct_zone);
> > >          } else if (!simap_find(&req_snat_zones, ct_zone->name)) {
> > >              bitmap_set1(unreq_snat_zones_map, ct_zone->data);
> > > @@ -195,7 +250,7 @@ ct_zones_update(const struct sset *local_lports,
> > >              continue;
> > >          }
> > >
> > > -        ct_zone_assign_unused(ctx, user, &scan_start);
> > > +        ct_zone_assign_unused(ctx, user, &min_ct_zone, max_ct_zone);
> > >      }
> > >
> > >      simap_destroy(&req_snat_zones);
> > > @@ -296,11 +351,19 @@ ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
> > >  /* Returns "true" if there was an update to the context. */
> > >  bool
> > >  ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
> > > -                           bool updated, int *scan_start)
> > > +                           bool updated, int *scan_start,
> > > +                           int min_ct_zone, int max_ct_zone)
> > >  {
> > >      struct simap_node *ct_zone = simap_find(&ctx->current, name);
> > > +
> > > +    if (ct_zone &&
> > > +        (ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone)) {
> >
> So, IIUC a zone range "5-10" means zones 5 to 10 are supported (a zone=10
> would not be removed if configuring range="5-10")?

ack, I will fix it in v3.

Regards,
Lorenzo

> 
> > > +        ct_zone_remove(ctx, ct_zone);
> > > +        ct_zone = NULL;
> > > +    }
> > > +
> > >      if (updated && !ct_zone) {
> > > -        ct_zone_assign_unused(ctx, name, scan_start);
> > > +        ct_zone_assign_unused(ctx, name, scan_start, max_ct_zone);
> > >          return true;
> > >      } else if (!updated && ct_zone_remove(ctx, ct_zone)) {
> > >          return true;
> > > @@ -312,11 +375,11 @@ ct_zone_handle_port_update(struct ct_zone_ctx *ctx,
> > > const char *name,
> > >
> > >  static bool
> > >  ct_zone_assign_unused(struct ct_zone_ctx *ctx, const char *zone_name,
> > > -                      int *scan_start)
> > > +                      int *scan_start, int scan_stop)
> > >  {
> > >      /* We assume that there are 64K zones and that we own them all. */
> > > -    int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, MAX_CT_ZONES +
> > 1);
> > > -    if (zone == MAX_CT_ZONES + 1) {
> > > +    int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, scan_stop);
> >
> > +    if (zone == scan_stop) {
> >
> But here, IIUC, a range "5-10" means only zones 5 to 9 can be assigned ?
> 
> > >          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> > >          VLOG_WARN_RL(&rl, "exhausted all ct zones");
> > >          return false;
> > > diff --git a/controller/ct-zone.h b/controller/ct-zone.h
> > > index 889bdf2fc..690b2ec7c 100644
> > > --- a/controller/ct-zone.h
> > > +++ b/controller/ct-zone.h
> > > @@ -56,11 +56,14 @@ struct ct_zone_pending_entry {
> > >      enum ct_zone_pending_state state;
> > >  };
> > >
> > > +void ct_zones_parse_range(const struct ovsrec_open_vswitch_table
> > > *ovs_table,
> > > +                          int *min_ct_zone, int *max_ct_zone);
> > >  void ct_zones_restore(struct ct_zone_ctx *ctx,
> > >                        const struct ovsrec_open_vswitch_table *ovs_table,
> > >                        const struct sbrec_datapath_binding_table
> > *dp_table,
> > >                        const struct ovsrec_bridge *br_int);
> > >  void ct_zones_update(const struct sset *local_lports,
> > > +                     const struct ovsrec_open_vswitch_table *ovs_table,
> > >                       const struct hmap *local_datapaths,
> > >                       struct ct_zone_ctx *ctx);
> > >  void ct_zones_commit(const struct ovsrec_bridge *br_int,
> > > @@ -69,6 +72,7 @@ void ct_zones_pending_clear_commited(struct shash
> > > *pending);
> > >  bool ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
> > >                                const struct sbrec_datapath_binding *dp);
> > >  bool ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char
> > *name,
> > > -                                bool updated, int *scan_start);
> > > +                                bool updated, int *scan_start,
> > > +                                int min_ct_zone, int max_ct_zone);
> > >
> > >  #endif /* controller/ct-zone.h */
> > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > index d6d001b1a..54b3a1cd5 100644
> > > --- a/controller/ovn-controller.c
> > > +++ b/controller/ovn-controller.c
> > > @@ -2230,8 +2230,8 @@ en_ct_zones_run(struct engine_node *node, void
> > *data)
> > >      const struct ovsrec_bridge *br_int = get_br_int(bridge_table,
> > > ovs_table);
> > >
> > >      ct_zones_restore(&ct_zones_data->ctx, ovs_table, dp_table, br_int);
> > > -    ct_zones_update(&rt_data->local_lports, &rt_data->local_datapaths,
> > > -                    &ct_zones_data->ctx);
> > > +    ct_zones_update(&rt_data->local_lports, ovs_table,
> > > +                    &rt_data->local_datapaths, &ct_zones_data->ctx);
> > >
> > >      ct_zones_data->recomputed = true;
> > >      engine_set_node_state(node, EN_UPDATED);
> > > @@ -2275,6 +2275,8 @@ ct_zones_runtime_data_handler(struct engine_node
> > > *node, void *data)
> > >  {
> > >      struct ed_type_runtime_data *rt_data =
> > >          engine_get_input_data("runtime_data", node);
> > > +    const struct ovsrec_open_vswitch_table *ovs_table =
> > > +        EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
> > >
> > >      /* There is no tracked data. Fall back to full recompute of
> > ct_zones.
> > > */
> > >      if (!rt_data->tracked) {
> > > @@ -2284,11 +2286,14 @@ ct_zones_runtime_data_handler(struct engine_node
> > > *node, void *data)
> > >      struct ed_type_ct_zones *ct_zones_data = data;
> > >
> > >      struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> > > +    int scan_start, min_ct_zone, max_ct_zone;
> > >      struct tracked_datapath *tdp;
> > > -    int scan_start = 1;
> > >
> > >      bool updated = false;
> > >
> > > +    ct_zones_parse_range(ovs_table, &min_ct_zone, &max_ct_zone);
> > > +    scan_start = min_ct_zone;
> > > +
> > >      HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
> > >          if (tdp->tracked_type == TRACKED_RESOURCE_NEW) {
> > >              /* A new datapath has been added. Fall back to full
> > > recompute. */
> > > @@ -2312,7 +2317,8 @@ ct_zones_runtime_data_handler(struct engine_node
> > > *node, void *data)
> > >                      t_lport->tracked_type == TRACKED_RESOURCE_UPDATED;
> > >              updated |= ct_zone_handle_port_update(&ct_zones_data->ctx,
> > >
> > >  t_lport->pb->logical_port,
> > > -                                                  port_updated,
> > > &scan_start);
> > > +                                                  port_updated,
> > > &scan_start,
> > > +                                                  min_ct_zone,
> > > max_ct_zone);
> > >          }
> > >      }
> > >
> > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > > index 2cb86dc98..fe575a014 100644
> > > --- a/tests/ovn-controller.at
> > > +++ b/tests/ovn-controller.at
> > > @@ -3127,3 +3127,70 @@ OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1235:
> > > connected' hv1/ovn-controller.log])
> > >
> > >  OVN_CLEANUP([hv1])
> > >  AT_CLEANUP
> > > +
> > > +OVN_FOR_EACH_NORTHD([
> > > +AT_SETUP([ovn-controller - CT zone min/max boundaries])
> > > +ovn_start
> > > +
> > > +check_ct_zone_min() {
> > > +    min_val=$1
> > > +    OVS_WAIT_UNTIL([test $(ovn-appctl -t ovn-controller ct-zone-list |
> > > awk '{print $2}' | sort | head -n1) -ge ${min_val}])
> > > +}
> > > +
> > > +check_ct_zone_max() {
> > > +    max_val=$1
> > > +    AT_CHECK([test $(ovn-appctl -t ovn-controller ct-zone-list | awk
> > > '{print $2}' | sort | tail -n1) -le ${max_val}])
> > > +}
> > > +
> > > +net_add n1
> > > +sim_add hv1
> > > +as hv1
> > > +check ovs-vsctl add-br br-phys
> > > +ovn_attach n1 br-phys 192.168.0.1
> > > +
> > > +check ovn-appctl -t ovn-controller vlog/set dbg:ct_zone
> > > +
> > > +check ovs-vsctl add-port br-int lsp0 \
> > > +    -- set Interface lsp0 external-ids:iface-id=lsp0
> > > +
> > > +check ovn-nbctl lr-add lr
> > > +
> > > +check ovn-nbctl ls-add ls
> > > +check ovn-nbctl lsp-add ls ls-lr
> > > +check ovn-nbctl lsp-set-type ls-lr router
> > > +check ovn-nbctl lsp-set-addresses ls-lr router
> > > +check ovn-nbctl lrp-add lr lr-ls 00:00:00:00:00:01 10.0.0.1
> > > +
> > > +check ovn-nbctl lsp-add ls lsp0
> > > +check ovn-nbctl lsp-set-addresses lsp0 "00:00:00:00:00:02 10.0.0.2"
> > > +
> > > +check ovn-nbctl lrp-add lr lrp-gw 01:00:00:00:00:01 172.16.0.1
> > > +check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1
> > > +
> > > +# check regular boundaries
> > > +check_ct_zone_min 1
> > > +check_ct_zone_max 10
> > > +
> > > +# increase boundaries
> > > +ovs-vsctl set Open_vSwitch . external_ids:ct_zone_range=\"10-20\"
> > > +check_ct_zone_min 10
> > > +check_ct_zone_max 20
> >
> > +
> > > +# reset min boundary
> > > +ovs-vsctl set Open_vSwitch . external_ids:ct_zone_range=\"5-20\"
> > > +
> > > +# add a new port to the switch
> > > +check ovs-vsctl add-port br-int lsp1 \
> > > +    -- set Interface lsp1 external-ids:iface-id=lsp1
> > > +check ovn-nbctl lsp-add ls lsp1
> > > +check ovn-nbctl lsp-set-addresses lsp1 "00:00:00:00:00:03 10.0.0.3"
> > > +check_ct_zone_min 5
> > > +check_ct_zone_max 20
> > > +
> > > +check ovn-nbctl set logical_router lr options:snat-ct-zone=2
> > > +check_ct_zone_min 2
> > > +check_ct_zone_max 20
> > > +
> > > +OVN_CLEANUP([hv1])
> > > +AT_CLEANUP
> > > +])
> > > --
> > > 2.45.2
> > >
> > >
> > Looks good to me, thanks.
> >
> > Acked-by: Ales Musil <amusil@redhat.com>
> >
> > Thanks
> Xavier
> 
> > --
> >
> > Ales Musil
> >
> > Senior Software Engineer - OVN Core
> >
> > Red Hat EMEA <https://www.redhat.com>
> >
> > amusil@redhat.com
> > <https://red.ht/sig>
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 3e392ff08..421f3cd0a 100644
--- a/NEWS
+++ b/NEWS
@@ -38,6 +38,8 @@  Post v24.03.0
     ability to disable "VXLAN mode" to extend available tunnel IDs space for
     datapaths from 4095 to 16711680.  For more details see man ovn-nb(5) for
     mentioned option.
+  - Added support to define boundaries (min and max values) for selected ct
+    zones.
 
 OVN v24.03.0 - 01 Mar 2024
 --------------------------
diff --git a/controller/ct-zone.c b/controller/ct-zone.c
index e4f66a52a..288ebe816 100644
--- a/controller/ct-zone.c
+++ b/controller/ct-zone.c
@@ -14,7 +14,9 @@ 
  */
 
 #include <config.h>
+#include <errno.h>
 
+#include "chassis.h"
 #include "ct-zone.h"
 #include "local_data.h"
 #include "openvswitch/vlog.h"
@@ -29,7 +31,8 @@  static void ct_zone_add_pending(struct shash *pending_ct_zones,
                                 int zone, bool add, const char *name);
 static int ct_zone_get_snat(const struct sbrec_datapath_binding *dp);
 static bool ct_zone_assign_unused(struct ct_zone_ctx *ctx,
-                                  const char *zone_name, int *scan_start);
+                                  const char *zone_name,
+                                  int *scan_start, int scan_stop);
 static bool ct_zone_remove(struct ct_zone_ctx *ctx,
                            struct simap_node *ct_zone);
 
@@ -87,12 +90,61 @@  ct_zones_restore(struct ct_zone_ctx *ctx,
     }
 }
 
+void
+ct_zones_parse_range(const struct ovsrec_open_vswitch_table *ovs_table,
+                     int *min_ct_zone, int *max_ct_zone)
+{
+    /* Set default values. */
+    *min_ct_zone = 1;
+    *max_ct_zone = MAX_CT_ZONES + 1;
+
+    const struct ovsrec_open_vswitch *cfg =
+        ovsrec_open_vswitch_table_first(ovs_table);
+    if (!cfg) {
+        return;
+    }
+
+    const char *chassis_id = get_ovs_chassis_id(ovs_table);
+    const char *range = get_chassis_external_id_value(&cfg->external_ids,
+                                                      chassis_id,
+                                                      "ct_zone_range", NULL);
+    if (!range) {
+        return;
+    }
+
+    char *ptr = NULL, *tokstr = xstrdup(range);
+    char *range_min = strtok_r(tokstr, "-", &ptr);
+    if (!range_min) {
+        goto out;
+    }
+
+    int min = strtol(range_min, NULL, 10);
+    if (errno == EINVAL || min < 1) {
+        goto out;
+    }
+    *min_ct_zone = min;
+
+    char *range_max = strtok_r(NULL, "-", &ptr);
+    if (!range_max) {
+        goto out;
+    }
+
+    int max = strtol(range_max, NULL, 10);
+    if (errno == EINVAL || max > MAX_CT_ZONES + 1) {
+        goto out;
+    }
+    *max_ct_zone = max;
+out:
+    free(tokstr);
+}
+
 void
 ct_zones_update(const struct sset *local_lports,
+                const struct ovsrec_open_vswitch_table *ovs_table,
                 const struct hmap *local_datapaths, struct ct_zone_ctx *ctx)
 {
+    int min_ct_zone, max_ct_zone;
     struct simap_node *ct_zone;
-    int scan_start = 1;
     const char *user;
     struct sset all_users = SSET_INITIALIZER(&all_users);
     struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones);
@@ -131,9 +183,12 @@  ct_zones_update(const struct sset *local_lports,
         free(snat);
     }
 
+    ct_zones_parse_range(ovs_table, &min_ct_zone, &max_ct_zone);
+
     /* Delete zones that do not exist in above sset. */
     SIMAP_FOR_EACH_SAFE (ct_zone, &ctx->current) {
-        if (!sset_contains(&all_users, ct_zone->name)) {
+        if (!sset_contains(&all_users, ct_zone->name) ||
+            ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone) {
             ct_zone_remove(ctx, ct_zone);
         } else if (!simap_find(&req_snat_zones, ct_zone->name)) {
             bitmap_set1(unreq_snat_zones_map, ct_zone->data);
@@ -195,7 +250,7 @@  ct_zones_update(const struct sset *local_lports,
             continue;
         }
 
-        ct_zone_assign_unused(ctx, user, &scan_start);
+        ct_zone_assign_unused(ctx, user, &min_ct_zone, max_ct_zone);
     }
 
     simap_destroy(&req_snat_zones);
@@ -296,11 +351,19 @@  ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
 /* Returns "true" if there was an update to the context. */
 bool
 ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
-                           bool updated, int *scan_start)
+                           bool updated, int *scan_start,
+                           int min_ct_zone, int max_ct_zone)
 {
     struct simap_node *ct_zone = simap_find(&ctx->current, name);
+
+    if (ct_zone &&
+        (ct_zone->data < min_ct_zone || ct_zone->data > max_ct_zone)) {
+        ct_zone_remove(ctx, ct_zone);
+        ct_zone = NULL;
+    }
+
     if (updated && !ct_zone) {
-        ct_zone_assign_unused(ctx, name, scan_start);
+        ct_zone_assign_unused(ctx, name, scan_start, max_ct_zone);
         return true;
     } else if (!updated && ct_zone_remove(ctx, ct_zone)) {
         return true;
@@ -312,11 +375,11 @@  ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
 
 static bool
 ct_zone_assign_unused(struct ct_zone_ctx *ctx, const char *zone_name,
-                      int *scan_start)
+                      int *scan_start, int scan_stop)
 {
     /* We assume that there are 64K zones and that we own them all. */
-    int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, MAX_CT_ZONES + 1);
-    if (zone == MAX_CT_ZONES + 1) {
+    int zone = bitmap_scan(ctx->bitmap, 0, *scan_start, scan_stop);
+    if (zone == scan_stop) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
         VLOG_WARN_RL(&rl, "exhausted all ct zones");
         return false;
diff --git a/controller/ct-zone.h b/controller/ct-zone.h
index 889bdf2fc..690b2ec7c 100644
--- a/controller/ct-zone.h
+++ b/controller/ct-zone.h
@@ -56,11 +56,14 @@  struct ct_zone_pending_entry {
     enum ct_zone_pending_state state;
 };
 
+void ct_zones_parse_range(const struct ovsrec_open_vswitch_table *ovs_table,
+                          int *min_ct_zone, int *max_ct_zone);
 void ct_zones_restore(struct ct_zone_ctx *ctx,
                       const struct ovsrec_open_vswitch_table *ovs_table,
                       const struct sbrec_datapath_binding_table *dp_table,
                       const struct ovsrec_bridge *br_int);
 void ct_zones_update(const struct sset *local_lports,
+                     const struct ovsrec_open_vswitch_table *ovs_table,
                      const struct hmap *local_datapaths,
                      struct ct_zone_ctx *ctx);
 void ct_zones_commit(const struct ovsrec_bridge *br_int,
@@ -69,6 +72,7 @@  void ct_zones_pending_clear_commited(struct shash *pending);
 bool ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
                               const struct sbrec_datapath_binding *dp);
 bool ct_zone_handle_port_update(struct ct_zone_ctx *ctx, const char *name,
-                                bool updated, int *scan_start);
+                                bool updated, int *scan_start,
+                                int min_ct_zone, int max_ct_zone);
 
 #endif /* controller/ct-zone.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index d6d001b1a..54b3a1cd5 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2230,8 +2230,8 @@  en_ct_zones_run(struct engine_node *node, void *data)
     const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
 
     ct_zones_restore(&ct_zones_data->ctx, ovs_table, dp_table, br_int);
-    ct_zones_update(&rt_data->local_lports, &rt_data->local_datapaths,
-                    &ct_zones_data->ctx);
+    ct_zones_update(&rt_data->local_lports, ovs_table,
+                    &rt_data->local_datapaths, &ct_zones_data->ctx);
 
     ct_zones_data->recomputed = true;
     engine_set_node_state(node, EN_UPDATED);
@@ -2275,6 +2275,8 @@  ct_zones_runtime_data_handler(struct engine_node *node, void *data)
 {
     struct ed_type_runtime_data *rt_data =
         engine_get_input_data("runtime_data", node);
+    const struct ovsrec_open_vswitch_table *ovs_table =
+        EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
 
     /* There is no tracked data. Fall back to full recompute of ct_zones. */
     if (!rt_data->tracked) {
@@ -2284,11 +2286,14 @@  ct_zones_runtime_data_handler(struct engine_node *node, void *data)
     struct ed_type_ct_zones *ct_zones_data = data;
 
     struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings;
+    int scan_start, min_ct_zone, max_ct_zone;
     struct tracked_datapath *tdp;
-    int scan_start = 1;
 
     bool updated = false;
 
+    ct_zones_parse_range(ovs_table, &min_ct_zone, &max_ct_zone);
+    scan_start = min_ct_zone;
+
     HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
         if (tdp->tracked_type == TRACKED_RESOURCE_NEW) {
             /* A new datapath has been added. Fall back to full recompute. */
@@ -2312,7 +2317,8 @@  ct_zones_runtime_data_handler(struct engine_node *node, void *data)
                     t_lport->tracked_type == TRACKED_RESOURCE_UPDATED;
             updated |= ct_zone_handle_port_update(&ct_zones_data->ctx,
                                                   t_lport->pb->logical_port,
-                                                  port_updated, &scan_start);
+                                                  port_updated, &scan_start,
+                                                  min_ct_zone, max_ct_zone);
         }
     }
 
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 2cb86dc98..fe575a014 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -3127,3 +3127,70 @@  OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1235: connected' hv1/ovn-controller.log])
 
 OVN_CLEANUP([hv1])
 AT_CLEANUP
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-controller - CT zone min/max boundaries])
+ovn_start
+
+check_ct_zone_min() {
+    min_val=$1
+    OVS_WAIT_UNTIL([test $(ovn-appctl -t ovn-controller ct-zone-list | awk '{print $2}' | sort | head -n1) -ge ${min_val}])
+}
+
+check_ct_zone_max() {
+    max_val=$1
+    AT_CHECK([test $(ovn-appctl -t ovn-controller ct-zone-list | awk '{print $2}' | sort | tail -n1) -le ${max_val}])
+}
+
+net_add n1
+sim_add hv1
+as hv1
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+
+check ovn-appctl -t ovn-controller vlog/set dbg:ct_zone
+
+check ovs-vsctl add-port br-int lsp0 \
+    -- set Interface lsp0 external-ids:iface-id=lsp0
+
+check ovn-nbctl lr-add lr
+
+check ovn-nbctl ls-add ls
+check ovn-nbctl lsp-add ls ls-lr
+check ovn-nbctl lsp-set-type ls-lr router
+check ovn-nbctl lsp-set-addresses ls-lr router
+check ovn-nbctl lrp-add lr lr-ls 00:00:00:00:00:01 10.0.0.1
+
+check ovn-nbctl lsp-add ls lsp0
+check ovn-nbctl lsp-set-addresses lsp0 "00:00:00:00:00:02 10.0.0.2"
+
+check ovn-nbctl lrp-add lr lrp-gw 01:00:00:00:00:01 172.16.0.1
+check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1
+
+# check regular boundaries
+check_ct_zone_min 1
+check_ct_zone_max 10
+
+# increase boundaries
+ovs-vsctl set Open_vSwitch . external_ids:ct_zone_range=\"10-20\"
+check_ct_zone_min 10
+check_ct_zone_max 20
+
+# reset min boundary
+ovs-vsctl set Open_vSwitch . external_ids:ct_zone_range=\"5-20\"
+
+# add a new port to the switch
+check ovs-vsctl add-port br-int lsp1 \
+    -- set Interface lsp1 external-ids:iface-id=lsp1
+check ovn-nbctl lsp-add ls lsp1
+check ovn-nbctl lsp-set-addresses lsp1 "00:00:00:00:00:03 10.0.0.3"
+check_ct_zone_min 5
+check_ct_zone_max 20
+
+check ovn-nbctl set logical_router lr options:snat-ct-zone=2
+check_ct_zone_min 2
+check_ct_zone_max 20
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])