On 3/26/25 11:30 AM, Mansi Sharma wrote:
> This change will be useful for migration cases, where it can be
> used to sync ct-entries before port is up on new chassis,
> resulting in reduced network package drops.
> It also fulfills the need of any other service which might need
> advance ct-zone reservation in future.
>
> ---
> v10->v11
> 1. Added in details in news and ovn-controller.8.xml
> 2. Resolved requested changes in definition
> declartions, and memory issues.
> v11->v12
> 1. Fixed xml file format issues.
> ---
Hi Mansi,
Sorry for the delay in reviews.
>
> Signed-off-by: Mansi Sharma <mansi.sha...@nutanix.com>
Please add the signed-off-by before the "---". git-am ignores
everything in between "---" and the actuall diff so it will
fail to add your sign-off to the commit.
> ---
> NEWS | 2 +
> controller/ct-zone.c | 54 +++++++++++++--
> controller/ct-zone.h | 9 ++-
> controller/ovn-controller.8.xml | 13 ++++
> controller/ovn-controller.c | 18 +++--
> tests/ovn-controller.at | 119 +++++++++++++++++++++++++++++---
> 6 files changed, 193 insertions(+), 22 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 656176d20..169cccada 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,8 @@ Post v25.03.0
> external-ids, this option allows to specify if ovn-controller should
> perform cleanup when exiting. The "--restart" exit always has priority
> to keep the backward compatibility.
> + - Added support to reserve ct-zones for upcoming ports via setting
> + external_ids:reserve_ct_zones option in Open_vSwitch table.
>
> OVN v25.03.0 - 07 Mar 2025
> --------------------------
> diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> index 469a8fc54..763837a54 100644
> --- a/controller/ct-zone.c
> +++ b/controller/ct-zone.c
> @@ -118,6 +118,37 @@ ct_zones_restore(struct ct_zone_ctx *ctx,
> }
> }
>
> +void
> +ct_zones_reserved_lports(const struct ovsrec_open_vswitch_table *ovs_table,
> + struct sset * reserved_lports_set)
Nit, should be: "struct sset *reserved_lports_set".
> +{
> + const struct ovsrec_open_vswitch *cfg;
> + cfg = ovsrec_open_vswitch_table_first(ovs_table);
> +
> + if (cfg == NULL) {
> + return;
> + }
> +
> + const char *reserve_ct_zone_request_list = smap_get(
> + &cfg->external_ids, "reserve_ct_zones");
> +
> + if (reserve_ct_zone_request_list == NULL) {
> + return;
> + }
Nit: I'd rewrite this as:
const char *req_list = smap_get(&cfg->external_ids, "reserve_ct_zones");
if (req_list == NULL) {
return;
}
> +
> + char *dup_reserve_list = xstrdup(reserve_ct_zone_request_list);
> + char *reserve_port;
> + char *save_ptr = NULL;
> +
> + for (reserve_port = strtok_r(dup_reserve_list, ",", &save_ptr);
> + reserve_port != NULL;
> + reserve_port = strtok_r(NULL, ",", &save_ptr)) {
> + sset_add(reserved_lports_set, reserve_port);
> + }
> + free(reserve_port);
> + free(dup_reserve_list);
Let's use the existing library functions. This whole chunk can be
written as:
sset_from_delimited_string(reserved_lports_set, req_list, ",");
> +}
> +
> void
> ct_zones_parse_range(const struct ovsrec_open_vswitch_table *ovs_table,
> int *min_ct_zone, int *max_ct_zone)
> @@ -169,7 +200,8 @@ out:
> 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)
> + const struct hmap *local_datapaths, struct ct_zone_ctx *ctx,
> + struct sset *reserved_lports)
> {
> int min_ct_zone, max_ct_zone;
> const char *user;
> @@ -183,6 +215,13 @@ ct_zones_update(const struct sset *local_lports,
> sset_add(&all_users, local_lport);
> }
>
> + if (!sset_is_empty(reserved_lports)) {
> + const char *reserved_lport;
> + SSET_FOR_EACH (reserved_lport, reserved_lports) {
> + sset_add(&all_users, reserved_lport);
> + }
> + }
> +
> /* Local patched datapath (gateway routers) need zones assigned. */
> const struct local_datapath *ld;
> HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> @@ -397,12 +436,15 @@ 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 struct sbrec_port_binding *pb,
> - bool updated, int *scan_start,
> - int min_ct_zone, int max_ct_zone)
> +ct_zone_handle_port_update(
> + struct ct_zone_ctx *ctx,
> + const struct sbrec_port_binding *pb,
> + bool updated, int *scan_start,
> + int min_ct_zone, int max_ct_zone,
> + struct sset *reserved_lports)
> {
> struct shash_node *node = shash_find(&ctx->current, pb->logical_port);
> + bool is_reserved = node && sset_contains(reserved_lports, node->name);
>
> if (node) {
> struct ct_zone *ct_zone = node->data;
> @@ -419,6 +461,8 @@ ct_zone_handle_port_update(struct ct_zone_ctx *ctx,
> }
> ct_zone_limit_update(ctx, pb->logical_port,
> ct_zone_get_pb_limit(pb));
> return true;
> + } else if (node && is_reserved) {
> + return true;
> } else if (node && ct_zone_remove(ctx, node->name)) {
> return true;
> }
> diff --git a/controller/ct-zone.h b/controller/ct-zone.h
> index 6df03975c..bf26a467d 100644
> --- a/controller/ct-zone.h
> +++ b/controller/ct-zone.h
> @@ -64,6 +64,9 @@ struct ct_zone_pending_entry {
>
> void ct_zone_ctx_init(struct ct_zone_ctx *ctx);
> void ct_zone_ctx_destroy(struct ct_zone_ctx *ctx);
> +void ct_zones_reserved_lports(
> + const struct ovsrec_open_vswitch_table *,
> + struct sset *reserved_lports_set);
> 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,
> @@ -73,7 +76,8 @@ void 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);
> + struct ct_zone_ctx *ctx,
> + struct sset *reserved_lports);
> void ct_zones_commit(const struct ovsrec_bridge *br_int,
> const struct ovsrec_datapath *ovs_dp,
> struct ovsdb_idl_txn *ovs_idl_txn,
> @@ -85,7 +89,8 @@ bool ct_zone_handle_dp_update(struct ct_zone_ctx *ctx,
> bool ct_zone_handle_port_update(struct ct_zone_ctx *ctx,
> const struct sbrec_port_binding *pb,
> bool updated, int *scan_start,
> - int min_ct_zone, int max_ct_zone);
> + int min_ct_zone, int max_ct_zone,
> + struct sset *reserved_lports);
> uint16_t ct_zone_find_zone(const struct shash *ct_zones, const char *name);
> void ct_zones_limits_sync(struct ct_zone_ctx *ctx,
> const struct hmap *local_datapaths,
> diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
> index 31f790875..6c9fb6314 100644
> --- a/controller/ovn-controller.8.xml
> +++ b/controller/ovn-controller.8.xml
> @@ -664,6 +664,19 @@
> <code>external_ids:ovn-installed-ts</code>.
> </p>
> </dd>
> +
> + <dt>
> + <code>external-ids:reserve_ct_zones</code> in the <code>Bridge</code>
> + table
> + </dt>
> +
> + <dd>
> + <p>
> + This key represents list of ports which are supposed to come up on
> + the chassis, and hence need advance reservation of ct-zones.
> + It is comma seprated list of port names.
> + </p>
> + </dd>
> </dl>
>
> <h1>OVN Southbound Database Usage</h1>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 0b09c98bd..70be0734e 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2368,10 +2368,13 @@ en_ct_zones_run(struct engine_node *node, void *data)
> EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
>
> const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
> -
> + struct sset reserved_lports = SSET_INITIALIZER(&reserved_lports);
> + ct_zones_reserved_lports(ovs_table, &reserved_lports);
> ct_zones_restore(&ct_zones_data->ctx, ovs_table, dp_table, br_int);
> ct_zones_update(&rt_data->local_lports, ovs_table,
> - &rt_data->local_datapaths, &ct_zones_data->ctx);
> + &rt_data->local_datapaths, &ct_zones_data->ctx,
> + &reserved_lports);
> + sset_destroy(&reserved_lports);
> ct_zones_limits_sync(&ct_zones_data->ctx, &rt_data->local_datapaths,
> &rt_data->lbinding_data.lports);
>
> @@ -2438,6 +2441,9 @@ ct_zones_runtime_data_handler(struct engine_node *node,
> void *data)
> ct_zones_parse_range(ovs_table, &min_ct_zone, &max_ct_zone);
> scan_start = min_ct_zone;
>
> + struct sset reserved_lports = SSET_INITIALIZER(&reserved_lports);
> + ct_zones_reserved_lports(ovs_table, &reserved_lports);
> +
> 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. */
The actual code returns here:
if (tdp->tracked_type == TRACKED_RESOURCE_NEW) {
/* A new datapath has been added. Fall back to full recompute. */
return false;
}
So we'll leak "reserved_lports". This was caught by CI:
https://github.com/ovsrobot/ovn/actions/runs/14081256140/job/39434720061#step:10:5838
Please make sure you check CI results. If you wish, you can get these
early by pushing to your own fork of the OVN repo on GitHub.
> @@ -2458,7 +2464,8 @@ ct_zones_runtime_data_handler(struct engine_node *node,
> void *data)
> updated |=
> ct_zone_handle_port_update(&ct_zones_data->ctx,
> t_lport->pb,
> false, &scan_start,
> - min_ct_zone, max_ct_zone);
> + min_ct_zone, max_ct_zone,
> + &reserved_lports);
> }
>
> continue;
> @@ -2470,10 +2477,13 @@ ct_zones_runtime_data_handler(struct engine_node
> *node, void *data)
> updated |= ct_zone_handle_port_update(&ct_zones_data->ctx,
> t_lport->pb,
> port_updated, &scan_start,
> - min_ct_zone, max_ct_zone);
> + min_ct_zone, max_ct_zone,
> + &reserved_lports);
> }
> }
>
> + sset_destroy(&reserved_lports);
> +
> if (updated) {
> engine_set_node_state(node, EN_UPDATED);
> }
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 2ebf879f1..94c75c6b6 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2633,7 +2633,7 @@ ovn_attach n1 br-phys 192.168.0.1
> get_zone_num () {
> output=$1
> name=$2
> - printf "$output" | grep $name | cut -d ' ' -f 2
> + echo "$output" | grep $name | cut -d ' ' -f 2
> }
>
> check_ovsdb_zone() {
> @@ -2643,8 +2643,14 @@ check_ovsdb_zone() {
> test $ct_zone -eq $db_zone
> }
>
> +check_duplicates() {
> + output=$1
> + AT_CHECK([echo "$output" | tr ' ' '\n' | sort | uniq -d | grep .], [1])
> +}
> +
> check ovs-vsctl add-port br-int ls0-hv1 -- set Interface ls0-hv1
> external-ids:iface-id=ls0-hv1
> check ovs-vsctl add-port br-int ls0-hv2 -- set Interface ls0-hv2
> external-ids:iface-id=ls0-hv2
> +check ovs-vsctl set Open_vSwitch .
> external_ids:reserve_ct_zones=ls0-req-hv3,ls0-req-hv4
>
> check ovn-nbctl lr-add lr0
>
> @@ -2666,21 +2672,23 @@ check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1
> check ovn-nbctl --wait=hv sync
>
> ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> -echo "$ct_zones"
> +
>
> port1_zone=$(get_zone_num "$ct_zones" ls0-hv1)
> port2_zone=$(get_zone_num "$ct_zones" ls0-hv2)
> +req_port3_zone=$(get_zone_num "$ct_zones" ls0-req-hv3)
> +req_port4_zone=$(get_zone_num "$ct_zones" ls0-req-hv4)
>
> snat_zone=$(get_zone_num "$ct_zones" lr0_snat)
> echo "snat_zone is $snat_zone"
>
> -check test "$port1_zone" -ne "$port2_zone"
> -check test "$port2_zone" -ne "$snat_zone"
> -check test "$port1_zone" -ne "$snat_zone"
> -
> OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv1 $port1_zone])
> OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv2 $port2_zone])
> OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv3 $req_port3_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv4 $req_port4_zone])
> +
> +check_duplicates "$ct_zones"
>
> # Now purposely request an SNAT zone for lr0 that conflicts with a zone
> # currently assigned to a logical port
> @@ -2690,20 +2698,110 @@ check ovn-nbctl set Logical_Router lr0
> options:snat-ct-zone=$snat_req_zone
> check ovn-nbctl --wait=hv sync
>
> ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> -echo "$ct_zones"
>
> port1_zone=$(get_zone_num "$ct_zones" ls0-hv1)
> port2_zone=$(get_zone_num "$ct_zones" ls0-hv2)
> snat_zone=$(get_zone_num "$ct_zones" lr0_snat)
> +req_port3_zone=$(get_zone_num "$ct_zones" ls0-req-hv3)
> +req_port4_zone=$(get_zone_num "$ct_zones" ls0-req-hv4)
> +
>
> check test "$snat_zone" -eq "$snat_req_zone"
> -check test "$port1_zone" -ne "$port2_zone"
> -check test "$port2_zone" -ne "$snat_zone"
> -check test "$port1_zone" -ne "$snat_zone"
> +check_duplicates "$ct_zones"
> +
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv1 $port1_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv2 $port2_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv3 $req_port3_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv4 $req_port4_zone])
> +
> +# Add port named ls0-req-hv3 and check if same zone assigned
> +# previously get assigned to it this time as well.
> +
> +check ovn-nbctl lsp-add ls0 ls0-req-hv3
> +check ovs-vsctl -- add-port br-int hv3-vif3 -- \
> + set interface hv3-vif3 external-ids:iface-id=ls0-req-hv3
> +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> +
> +check ovn-nbctl --wait=hv sync
> +
> +req_port3_zone_new=$(get_zone_num "$ct_zones" ls0-req-hv3)
> +check test "$req_port3_zone" -eq "$req_port3_zone_new"
> +check_duplicates "$ct_zones"
>
> OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv1 $port1_zone])
> OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv2 $port2_zone])
> OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv3 $req_port3_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv4 $req_port4_zone])
> +
> +AT_CHECK([ovs-vsctl get bridge br-int external_ids:ct-zone-ls0-req-hv3],
> +[0], [ignore])
> +
> +# Checks for two cases after removing entry from ovs_vswitch table -
> +# 1. If port is already up, ct-zone should be reserved.
> +# 2. If port is not up yet, ct-zone should not be reserved.
> +
> +check ovs-vsctl remove Open_vSwitch . external_ids reserve_ct_zones
> +
> +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> +
> +req_port3_zone_after_delete=$(get_zone_num "$ct_zones" ls0-req-hv3)
> +req_port4_zone_after_delete=$(get_zone_num "$ct_zones" ls0-req-hv4)
> +
> +check test "$req_port3_zone_new" -eq "$req_port3_zone_after_delete"
> +
> +AT_CHECK([ovs-vsctl get bridge br-int external_ids:ct-zone-ls0-req-hv3],
> +[0], [ignore])
> +
> +AT_CHECK([ovs-vsctl get bridge br-int external_ids:ct-zone-ls0-req-hv4],
> +[1], [ignore], [ignore])
> +
> +# check test "$req_port4_zone_after_delete" == ""
> +
> +# Checks for case when a ct-zone is reserved it comes up on that chassis, and
> +# gets deleted, but its persisted in ovs_vswitch table, it should persist the
> +# same zone throughout.
> +
> +check ovs-vsctl set Open_vSwitch . external_ids:reserve_ct_zones=ls0-req-hv5
> +check ovn-nbctl lsp-add ls0 ls0-req-hv5
> +check ovs-vsctl -- add-port br-int hv5-vif5 -- \
> + set interface hv5-vif5 external-ids:iface-id=ls0-req-hv5
> +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> +req_port5_zone=$(get_zone_num "$ct_zones" ls0-req-hv5)
> +AT_CHECK([ovs-vsctl get bridge br-int external_ids:ct-zone-ls0-req-hv5],
> +[0], [ignore])
> +
> +check ovs-vsctl remove interface hv5-vif5 external_ids iface-id
> +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> +req_port5_zone_new=$(get_zone_num "$ct_zones" ls0-req-hv5)
> +AT_CHECK([ovs-vsctl get bridge br-int external_ids:ct-zone-ls0-req-hv5],
> +[0], [ignore])
> +check test "$req_port5_zone" -eq "$req_port5_zone_new"
> +check_duplicates "$ct_zones"
> +
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv1 $port1_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv2 $port2_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone])
> +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv3 $req_port3_zone])
> +
> +check ovs-vsctl set Open_vSwitch . external_ids:reserve_ct_zones=ls0-req-hv6
> +check ovn-nbctl lsp-add ls0 ls0-req-hv6
> +check ovs-vsctl -- add-port br-int hv6-vif6 -- \
> + set interface hv6-vif6 external-ids:iface-id=ls0-req-hv6
> +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> +req_port6_zone=$(get_zone_num "$ct_zones" ls0-req-hv6)
> +
> +AT_CHECK([ovs-vsctl get bridge br-int external_ids:ct-zone-ls0-req-hv6],
> +[0], [ignore])
> +check ovn-nbctl --wait=hv lsp-del ls0-req-hv6
> +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> +AT_CHECK([ovs-vsctl get bridge br-int external_ids:ct-zone-ls0-req-hv6],
> +[0], [ignore])
> +req_port6_zone_new=$(get_zone_num "$ct_zones" ls0-req-hv6)
> +check_duplicates "$ct_zones"
> +check test "$req_port6_zone" -eq "$req_port6_zone_new"
> +check_duplicates "$ct_zones"
>
> # Now create a conflict in the OVSDB and restart ovn-controller.
>
> @@ -2713,7 +2811,6 @@ ovs-vsctl set bridge br-int
> external_ids:ct-zone-ls0-hv2="$snat_req_zone"
> ovn-appctl -t ovn-controller inc-engine/recompute
>
> ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> -echo "$ct_zones"
>
> port1_zone=$(get_zone_num "$ct_zones" ls0-hv1)
> port2_zone=$(get_zone_num "$ct_zones" ls0-hv2)
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
