On Mon, Nov 2, 2026 at 4:18 PM Jacob Tanenbaum <[email protected]> wrote:
> Commit da792b85c ("northd: Track router echo request/reply in the
> router port ct zone.") assigned a CT zone to the switch-side patch
> port of every router port on a local logical switch. To keep that
> assignment consistent it made ct_zones_runtime_data_handler() fall
> back to a full recompute whenever any non-ACL patch port was tracked,
> which is far more than the actual dependency: the zone of such a port
> is needed if and only if the port is still present in the peer set of
> a local logical switch.
>
> Track the peer set instead. The runtime_data engine node now
> maintains a shash of tracked peer ports, keyed by logical port name,
> which binding.c fills in whenever a peer relationship is added to or
> removed from an already local datapath. The ct_zones handler
> processes those ports through the existing idempotent
> ct_zone_handle_port_update(), and for non-ACL patch ports found in
> tracked_dp_bindings it reads the need directly from the current peer
> set of the port's datapath -- the same condition ct_zones_update()
> applies on a full recompute -- so zones of ports that lost their peer
> relationship (e.g. a router port that stops being an l3gateway port)
> are still released. Datapaths that become local keep the same
> TRACKED_RESOURCE_NEW fallback, which is also required for the NAT
> zones of newly local router datapaths.
>
> Assisted-by: GLM 5.3, OpenCode
> Signed-off-by: Jacob Tanenbaum <[email protected]>
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 3e8114e7e..e0ebe90e9 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -743,7 +743,8 @@ remove_related_lport(const struct sbrec_port_binding
> *pb,
> */
> static void
> update_ld_peers(const struct sbrec_port_binding *pb,
> - struct hmap *local_datapaths)
> + struct hmap *local_datapaths,
> + struct shash *tracked_peer_ports)
> {
> struct local_datapath *ld =
> get_local_datapath(local_datapaths, pb->datapath->tunnel_key);
> @@ -760,7 +761,8 @@ update_ld_peers(const struct sbrec_port_binding *pb,
> enum en_lport_type type = get_lport_type(pb);
> size_t num_peers = vector_len(&ld->peer_ports);
> if (type != LP_PATCH) {
> - remove_local_datapath_peer_port(pb, ld, local_datapaths);
> + remove_local_datapath_peer_port(pb, ld, local_datapaths,
> + tracked_peer_ports);
> if (num_peers != vector_len(&ld->peer_ports)) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_DBG_RL(&rl,
> @@ -2039,7 +2041,8 @@ consider_nonvif_lport_(const struct
> sbrec_port_binding *pb,
> b_ctx_in->sbrec_port_binding_by_datapath,
> b_ctx_in->sbrec_port_binding_by_name,
> ld, b_ctx_out->local_datapaths,
> - b_ctx_out->tracked_dp_bindings);
> + b_ctx_out->tracked_dp_bindings,
> + b_ctx_out->tracked_peer_ports);
> }
> }
>
> @@ -2073,7 +2076,8 @@ consider_nonvif_lport_(const struct
> sbrec_port_binding *pb,
>
> if (ld) {
> remove_local_datapath_peer_port(pb, ld,
> - b_ctx_out->local_datapaths);
> + b_ctx_out->local_datapaths,
> +
> b_ctx_out->tracked_peer_ports);
> }
> }
>
> @@ -2460,7 +2464,8 @@ remove_pb_from_local_datapath(const struct
> sbrec_port_binding *pb,
> remove_related_lport(pb, b_ctx_out);
> if (!strcmp(pb->type, "patch") ||
> !strcmp(pb->type, "l3gateway")) {
> - remove_local_datapath_peer_port(pb, ld,
> b_ctx_out->local_datapaths);
> + remove_local_datapath_peer_port(pb, ld,
> b_ctx_out->local_datapaths,
> + b_ctx_out->tracked_peer_ports);
> } else if (!strcmp(pb->type, "localnet")) {
> if (ld->localnet_port && !strcmp(ld->localnet_port->logical_port,
> pb->logical_port)) {
> @@ -3122,7 +3127,8 @@ consider_patch_port_for_local_datapaths(const struct
> sbrec_port_binding *pb,
> b_ctx_in->sbrec_port_binding_by_datapath,
> b_ctx_in->sbrec_port_binding_by_name,
> ld, b_ctx_out->local_datapaths,
> - b_ctx_out->tracked_dp_bindings);
> + b_ctx_out->tracked_dp_bindings,
> + b_ctx_out->tracked_peer_ports);
> }
> }
>
> @@ -3443,7 +3449,8 @@ delete_done:
> }
>
> if (sbrec_port_binding_is_updated(pb,
> SBREC_PORT_BINDING_COL_TYPE)) {
> - update_ld_peers(pb, b_ctx_out->local_datapaths);
> + update_ld_peers(pb, b_ctx_out->local_datapaths,
> + b_ctx_out->tracked_peer_ports);
> }
>
> if (!handle_updated_port(b_ctx_in, b_ctx_out, pb)) {
> diff --git a/controller/binding.h b/controller/binding.h
> index 817c4e296..618506286 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -111,6 +111,11 @@ struct binding_ctx_out {
> * the changed datapaths and port bindings. */
> struct hmap *tracked_dp_bindings;
>
> + /* shash of 'struct tracked_peer_port' (keyed by logical port name)
> + * which the callee fills in when the peer relationship of a
> + * switch-side patch port changes. */
> + struct shash *tracked_peer_ports;
> +
> struct if_status_mgr *if_mgr;
>
> struct sset *postponed_ports;
> diff --git a/controller/local_data.c b/controller/local_data.c
> index ad4b92f1f..38aa982f9 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -237,9 +237,13 @@ add_local_datapath_peer_port(
> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> struct local_datapath *ld,
> struct hmap *local_datapaths,
> - struct hmap *tracked_datapaths)
> + struct hmap *tracked_datapaths,
> + struct shash *tracked_peer_ports)
> {
> local_datapath_peer_port_add(ld, pb, peer);
> + if (ld->is_switch) {
> + tracked_peer_port_add(pb, true, tracked_peer_ports);
> + }
>
> struct local_datapath *peer_ld =
> get_local_datapath(local_datapaths,
> @@ -254,6 +258,9 @@ add_local_datapath_peer_port(
> }
>
> local_datapath_peer_port_add(peer_ld, peer, pb);
> + if (peer_ld->is_switch) {
> + tracked_peer_port_add(peer, true, tracked_peer_ports);
> + }
> }
>
> void
> @@ -275,7 +282,8 @@ local_data_dump_peer_ports(struct hmap
> *local_datapaths, struct ds *peer_ports)
> void
> remove_local_datapath_peer_port(const struct sbrec_port_binding *pb,
> struct local_datapath *ld,
> - struct hmap *local_datapaths)
> + struct hmap *local_datapaths,
> + struct shash *tracked_peer_ports)
> {
> size_t i = 0;
> const struct peer_ports *peers;
> @@ -297,6 +305,10 @@ remove_local_datapath_peer_port(const struct
> sbrec_port_binding *pb,
> local_datapath_usage += vector_memory_usage(&ld->peer_ports);
> }
>
> + if (ld->is_switch) {
> + tracked_peer_port_add(pb, false, tracked_peer_ports);
> + }
> +
> const struct sbrec_port_binding *peer = removed.remote;
> struct local_datapath *peer_ld =
> get_local_datapath(local_datapaths, peer->datapath->tunnel_key);
> @@ -304,7 +316,8 @@ remove_local_datapath_peer_port(const struct
> sbrec_port_binding *pb,
> /* Remove the peer port from the peer datapath. The peer
> * datapath also tries to remove its peer lport, but that would
> * be no-op. */
> - remove_local_datapath_peer_port(peer, peer_ld, local_datapaths);
> + remove_local_datapath_peer_port(peer, peer_ld, local_datapaths,
> + tracked_peer_ports);
> }
> }
>
> @@ -436,6 +449,30 @@ tracked_datapath_lport_add(const struct
> sbrec_port_binding *pb,
> lport->tracked_type = tracked_type;
> }
>
> +/* Adds 'pb' to 'tracked_peer_ports' keyed by its logical port name,
> + * with the last write winning if the same logical port was already
> + * tracked (e.g. the peer relationship was added and removed within the
> + * same IDL iteration). A NULL 'tracked_peer_ports' is allowed, in
> + * which case this is a no-op. */
> +void
> +tracked_peer_port_add(const struct sbrec_port_binding *pb,
> + bool zone_needed,
> + struct shash *tracked_peer_ports)
> +{
> + if (!tracked_peer_ports) {
> + return;
> + }
> +
> + struct tracked_peer_port *tpp =
> + shash_find_data(tracked_peer_ports, pb->logical_port);
> + if (!tpp) {
> + tpp = xmalloc(sizeof *tpp);
> + shash_add(tracked_peer_ports, pb->logical_port, tpp);
> + }
> + tpp->pb = pb;
> + tpp->zone_needed = zone_needed;
> +}
> +
> void
> tracked_datapaths_clear(struct hmap *tracked_datapaths)
> {
> diff --git a/controller/local_data.h b/controller/local_data.h
> index 130ae536a..06a73141c 100644
> --- a/controller/local_data.h
> +++ b/controller/local_data.h
> @@ -101,11 +101,13 @@ void add_local_datapath_peer_port(
> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> struct local_datapath *,
> struct hmap *local_datapaths,
> - struct hmap *tracked_datapaths);
> + struct hmap *tracked_datapaths,
> + struct shash *tracked_peer_ports);
>
> void remove_local_datapath_peer_port(const struct sbrec_port_binding *pb,
> struct local_datapath *ld,
> - struct hmap *local_datapaths);
> + struct hmap *local_datapaths,
> + struct shash *tracked_peer_ports);
>
> enum en_tracked_resource_type {
> TRACKED_RESOURCE_NEW,
> @@ -138,6 +140,19 @@ void tracked_datapath_lport_add(const struct
> sbrec_port_binding *,
> void tracked_datapaths_clear(struct hmap *tracked_datapaths);
> void tracked_datapaths_destroy(struct hmap *tracked_datapaths);
>
> +/* A switch-side router patch port whose CT zone assignment may have
> + * changed: 'zone_needed' is true when the connected logical switch is
> + * (still) a local datapath, false when the peer relationship was
> + * removed. */
> +struct tracked_peer_port {
> + const struct sbrec_port_binding *pb;
> + bool zone_needed;
> +};
> +
> +void tracked_peer_port_add(const struct sbrec_port_binding *pb,
> + bool zone_needed,
> + struct shash *tracked_peer_ports);
> +
> /* Maps from a chassis to the OpenFlow port number of the tunnel that can
> be
> * used to reach that chassis. */
> struct chassis_tunnel {
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 56c917e64..0858312e2 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1390,6 +1390,11 @@ struct ed_type_runtime_data {
> bool localnet_learn_fdb_changed;
> struct hmap tracked_dp_bindings;
>
> + /* Tracked peer (switch-side router) patch ports whose CT zone
> + * assignment may have changed. Only meaningful when 'tracked' is
> + * true. */
> + struct shash tracked_peer_ports;
> +
> struct shash local_active_ports_ipv6_pd;
> struct shash local_active_ports_ras;
>
> @@ -1479,6 +1484,7 @@ en_runtime_data_clear_tracked_data(void *data_)
>
> tracked_datapaths_destroy(&data->tracked_dp_bindings);
> hmap_init(&data->tracked_dp_bindings);
> + shash_clear_free_data(&data->tracked_peer_ports);
> data->local_lports_changed = false;
> data->tracked = false;
> }
> @@ -1503,6 +1509,7 @@ en_runtime_data_init(struct engine_node *node
> OVS_UNUSED,
>
> /* Init the tracked data. */
> hmap_init(&data->tracked_dp_bindings);
> + shash_init(&data->tracked_peer_ports);
>
> return data;
> }
> @@ -1522,6 +1529,7 @@ en_runtime_data_cleanup(void *data)
> local_datapaths_destroy(&rt_data->local_datapaths);
> shash_destroy(&rt_data->local_active_ports_ipv6_pd);
> shash_destroy(&rt_data->local_active_ports_ras);
> + shash_destroy_free_data(&rt_data->tracked_peer_ports);
> local_binding_data_destroy(&rt_data->lbinding_data);
> }
>
> @@ -1616,6 +1624,7 @@ init_binding_ctx(struct engine_node *node,
> b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
> b_ctx_out->postponed_ports = rt_data->postponed_ports;
> b_ctx_out->tracked_dp_bindings = NULL;
> + b_ctx_out->tracked_peer_ports = NULL;
> b_ctx_out->if_mgr = ctrl_ctx->if_mgr;
> b_ctx_out->localnet_learn_fdb = rt_data->localnet_learn_fdb;
> b_ctx_out->localnet_learn_fdb_changed = false;
> @@ -1839,6 +1848,7 @@ runtime_data_ovs_interface_shadow_handler(struct
> engine_node *node, void *data)
> init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out);
> rt_data->tracked = true;
> b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> + b_ctx_out.tracked_peer_ports = &rt_data->tracked_peer_ports;
>
> if (!binding_handle_ovs_interface_changes(&b_ctx_in, &b_ctx_out)) {
> return EN_UNHANDLED;
> @@ -1991,6 +2001,7 @@ runtime_data_sb_port_binding_handler(struct
> engine_node *node, void *data)
>
> rt_data->tracked = true;
> b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> + b_ctx_out.tracked_peer_ports = &rt_data->tracked_peer_ports;
>
> if (!binding_handle_port_binding_changes(&b_ctx_in, &b_ctx_out)) {
> return EN_UNHANDLED;
> @@ -2003,7 +2014,8 @@ runtime_data_sb_port_binding_handler(struct
> engine_node *node, void *data)
> b_ctx_out.non_vif_ports_changed ||
> b_ctx_out.local_lports_changed ||
> b_ctx_out.localnet_learn_fdb_changed ||
> - !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) {
> + !hmap_is_empty(b_ctx_out.tracked_dp_bindings) ||
> + !shash_is_empty(b_ctx_out.tracked_peer_ports)) {
> result = EN_HANDLED_UPDATED;
> }
>
> @@ -2873,15 +2885,33 @@ ct_zones_runtime_data_handler(struct engine_node
> *node, void *data)
> continue;
> }
>
> - /* The CT zone of a patch port without the ACL flag depends
> - * on the local datapaths of the connected switches, which is
> - * not tracked incrementally. Fall back to a full recompute
> - * of ct_zones so that zones for router ports are assigned
> - * and released consistently with ct_zones_update(). */
> + /* The CT zone of a patch port without the ACL flag is needed
> + * if and only if the port is the switch-side peer of a router
> + * port on a local logical switch. Read that condition
> + * directly from the peer set of the port's datapath, which is
> + * the same condition ct_zones_update() applies on a full
> + * recompute; ports whose peer relationship was removed are
> + * also tracked in rt_data->tracked_peer_ports below. */
> if (!strcmp(t_lport->pb->type, "patch") &&
> !smap_get_bool(&t_lport->pb->options,
> "enable_router_port_acl", false)) {
> - return EN_UNHANDLED;
> + bool zone_needed = false;
> + struct local_datapath *ld = get_local_datapath(
> + &rt_data->local_datapaths,
> + t_lport->pb->datapath->tunnel_key);
> + if (ld && ld->is_switch) {
> + const struct peer_ports *peers;
> + VECTOR_FOR_EACH_PTR (&ld->peer_ports, peers) {
> + if (peers->local == t_lport->pb) {
> + zone_needed = true;
> + break;
> + }
> + }
> + }
> + updated |= ct_zone_handle_port_update(
> + &ct_zones_data->ctx, t_lport->pb, zone_needed,
> + &scan_start, min_ct_zone, max_ct_zone);
> + continue;
> }
> bool port_updated =
> t_lport->tracked_type == TRACKED_RESOURCE_NEW ||
> @@ -2893,6 +2923,15 @@ ct_zones_runtime_data_handler(struct engine_node
> *node, void *data)
> }
> }
>
> + struct shash_node *tpp_node;
> + SHASH_FOR_EACH (tpp_node, &rt_data->tracked_peer_ports) {
> + struct tracked_peer_port *tpp = tpp_node->data;
> + updated |= ct_zone_handle_port_update(&ct_zones_data->ctx,
> + tpp->pb, tpp->zone_needed,
> + &scan_start, min_ct_zone,
> + max_ct_zone);
> + }
> +
> return updated ? EN_HANDLED_UPDATED : EN_HANDLED_UNCHANGED;
> }
>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index d600ba2cd..d00780478 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2870,6 +2870,85 @@ OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat
> $snat_zone])
> OVN_CLEANUP([hv1])
> AT_CLEANUP
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - I-P for CT zone patch ports])
> +AT_KEYWORDS([ovn-ct-zone-patch-ip])
> +
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +# ls1 and ls2 are local through their VIFs and lr0 is local through
> +# rp-ls1, so attaching lr0 to ls2 only changes the peer set of already
> +# local datapaths.
> +check ovn-nbctl lr-add lr0
> +check ovn-nbctl ls-add ls1
> +check ovn-nbctl lrp-add lr0 rp-ls1 00:00:01:01:02:01 10.0.1.1/24
> +check ovn-nbctl lsp-add-router-port ls1 ls1-rp rp-ls1
> +check ovn-nbctl lsp-add ls1 lsp1 -- lsp-set-addresses lsp1 \
> +"00:11:00:00:00:01 10.0.1.2"
> +check ovn-nbctl ls-add ls2
> +check ovn-nbctl lsp-add ls2 lsp2 -- lsp-set-addresses lsp2 \
> +"00:11:00:00:00:02 10.0.2.2"
> +
> +as hv1
> +check ovs-vsctl add-port br-int vif1 \
> + -- set Interface vif1 external_ids:iface-id=lsp1
> +check ovs-vsctl add-port br-int vif2 \
> + -- set Interface vif2 external_ids:iface-id=lsp2
> +
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +# Attaching lr0 to the already local ls2 assigns the CT zone of the
> +# switch-side patch port incrementally.
> +check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lrp-add lr0 rp-ls2 00:00:01:01:02:02
> 10.0.2.1/24 \
> + -- lsp-add-router-port ls2 ls2-rp rp-ls2
> +OVS_WAIT_UNTIL([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls2-rp])
> +check_controller_engine_stats hv1 runtime_data norecompute compute
> +check_controller_engine_stats hv1 ct_zones norecompute compute
> +
> +# Detaching lr0 from ls2 releases the CT zone incrementally.
> +check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lsp-del ls2-rp -- lrp-del rp-ls2
> +OVS_WAIT_WHILE([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls2-rp])
> +check_controller_engine_stats hv1 runtime_data norecompute compute
> +check_controller_engine_stats hv1 ct_zones norecompute compute
> +
> +# Re-attaching with the same port names re-assigns the zone
> +# incrementally.
> +check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lrp-add lr0 rp-ls2 00:00:01:01:02:02
> 10.0.2.1/24 \
> + -- lsp-add-router-port ls2 ls2-rp rp-ls2
> +OVS_WAIT_UNTIL([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls2-rp])
> +check_controller_engine_stats hv1 ct_zones norecompute compute
> +
> +# ls3 is not local yet: attaching it to the already local lr0 makes the
> +# datapath newly tracked, so a full ct_zones recompute is expected and
> +# the zone is assigned by it.
> +check ovn-nbctl ls-add ls3
> +check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lrp-add lr0 rp-ls3 00:00:01:01:02:03
> 10.0.3.1/24 \
> + -- lsp-add-router-port ls3 ls3-rp rp-ls3
> +OVS_WAIT_UNTIL([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls3-rp])
> +check_controller_engine_stats hv1 ct_zones recompute nocompute
> +
> +# Toggling enable_router_port_acl on a local patch port is still handled
> +# by the tracked lports path of the ct_zones handler.
> +check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
> +check ovn-nbctl --wait=hv lsp-set-options ls1-rp
> enable_router_port_acl=true
> +OVS_WAIT_UNTIL([ovs-vsctl get Bridge br-int external_ids:ct-zone-ls1-rp])
> +check_controller_engine_stats hv1 ct_zones norecompute compute
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([ovn-controller - Local Chassis_Template_Var updates])
> AT_KEYWORDS([templates])
> --
> 2.56.0
>
>
ovsrobot failed to find the patches I am going to retry running the tests
but here is a link to the set passing in my local repo
https://github.com/JacobTanenbaum/ovn/actions/runs/41509499669
ovsrobot failed to find the patches for this commit running a retest to see
if it will at least run the tests
Recheck-request: github-robot
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
