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 |
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 |
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>
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 >
> 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 --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 +])
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(-)