On Jul 10, Ales Musil wrote:
> On Tue, Jul 9, 2024 at 6:37 PM Lorenzo Bianconi <lorenzo.bianc...@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.bianc...@redhat.com>
> > ---
> >
>
> Hi Lorenzo,
>
> thank you for the patch, I have a couple of comments down below.
Hi Ales,
thx for the review.
>
>
> > NEWS | 2 ++
> > controller/ct-zone.c | 37 ++++++++++++++++------
> > controller/ct-zone.h | 4 ++-
> > controller/ovn-controller.c | 19 ++++++++---
> > tests/ovn-controller.at | 63 +++++++++++++++++++++++++++++++++++++
> > 5 files changed, 111 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.
> >
>
>
> IMO it would be better if it's just one option specifying range instead of
> two.
ack, I will fix it in v2.
>
>
> > OVN v24.03.0 - 01 Mar 2024
> > --------------------------
> > diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> > index e4f66a52a..5191c0fdf 100644
> > --- a/controller/ct-zone.c
> > +++ b/controller/ct-zone.c
> > @@ -29,7 +29,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);
> >
> > @@ -89,10 +90,11 @@ ct_zones_restore(struct ct_zone_ctx *ctx,
> >
> > 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 = 1, max_ct_zone = MAX_CT_ZONES + 1;
> > 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 +133,18 @@ ct_zones_update(const struct sset *local_lports,
> > free(snat);
> > }
> >
> > + const struct ovsrec_open_vswitch *cfg =
> > + ovsrec_open_vswitch_table_first(ovs_table);
> > + if (cfg) {
> > + min_ct_zone = smap_get_int(&cfg->external_ids, "ct_zone_min_val",
> > 1);
> > + max_ct_zone = smap_get_int(&cfg->external_ids, "ct_zone_max_val",
> > + MAX_CT_ZONES + 1);
> >
>
> This will break when there are two controllers running over the same OvS.
> We need to use the "get_chassis_external_id_value()" helpers here, see
> "get_bridge_mappings()" for example.
ack, I will fix it in v2.
>
>
> > + }
> > +
> > /* 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 +206,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 +307,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 +331,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..94dc1fde1 100644
> > --- a/controller/ct-zone.h
> > +++ b/controller/ct-zone.h
> > @@ -61,6 +61,7 @@ void ct_zones_restore(struct ct_zone_ctx *ctx,
> > 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 +70,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..5f0e902e6 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) {
> > @@ -2283,12 +2285,20 @@ ct_zones_runtime_data_handler(struct engine_node
> > *node, void *data)
> >
> > struct ed_type_ct_zones *ct_zones_data = data;
> >
> > + int scan_start = 1, min_ct_zone = 1, max_ct_zone = MAX_CT_ZONES + 1;
> > struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> > struct tracked_datapath *tdp;
> > - int scan_start = 1;
> >
> > bool updated = false;
> >
> > + const struct ovsrec_open_vswitch *cfg =
> > + ovsrec_open_vswitch_table_first(ovs_table);
> > + if (cfg) {
> > + min_ct_zone = smap_get_int(&cfg->external_ids, "ct_zone_min_val",
> > 1);
> > + max_ct_zone = smap_get_int(&cfg->external_ids, "ct_zone_max_val",
> > + MAX_CT_ZONES + 1);
> > + }
> > +
> > 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 +2322,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..fee7de89e 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -3127,3 +3127,66 @@ 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_min_val=10
> > external_ids:ct_zone_max_val=20
> > +check_ct_zone_min 10
> > +check_ct_zone_max 20
> > +
> > +# reset min boundary
> > +ovs-vsctl set Open_vSwitch . external_ids:ct_zone_min_val=1
> > +
> > +# 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 1
> > +check_ct_zone_max 20
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
> >
>
> The test should also ensure that the requested zone is assigned
> even if it's out of the range.
ack, I will fix it in v2.
Regards,
Lorenzo
>
>
> > --
> > 2.45.2
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> Thanks,
> Ales
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amu...@redhat.com
> <https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
