Hi Jacob, I have one question below. On Mon, Nov 2, 2026 at 8:20 PM Jacob Tanenbaum via dev <[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;
Should we check enable_router_port_acl here before calling ct_zone_handle_port_update()? > + 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 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
