On 8/4/26 04:42, Ilya Maximets wrote:
On 7/30/26 12:48 PM, Jun Gu wrote:
Patch (localnet / L2 gateway) port ofport changes force a full recompute
of the logical flow output through two independent engine edges, even
though logical flows never depend on patch ofports:
- non_vif_data bundles patch ofports together with tunnel data, and
en_lflow_output has no change handler for non_vif_data, so any patch
ofport change invalidates it too.
- Patch ports never carry an iface-id, so their ofport changes also hit
the "unhandled" branch of binding_handle_ovs_interface_changes(),
forcing a full recompute of runtime_data and, transitively, of
lflow_output again.
This is hit on the datapath every time a bridged / L2 logical port is
bound or moved between chassis, since the peer patch port is (re)created
and gets a new ofport. In deployments with many logical flows each
recompute can take several seconds, dominating the port's dataplane
downtime.
Fix both edges:
- Move patch ofports into their own engine node (en_patch_port_data)
that feeds only the physical flow output. en_non_vif_data keeps
feeding en_lflow_output, but now only changes on tunnel ofport
changes.
- Recognize patch-type OVS interfaces in
binding_handle_ovs_interface_changes() and skip them instead of
treating them as unhandled: their ofport is already tracked by
en_patch_port_data.
Tunnel (geneve/vxlan) interfaces are deliberately left alone in the
latter fix. runtime_data's 'active_tunnels' (BFD-derived reachability,
used for ECMP / gateway-chassis flow selection) has no incremental
tracking and is only recalculated on a full recompute; skipping tunnel
ofport changes there would silently stop refreshing it on BFD status
changes.
Recognizing patch ports in binding_handle_ovs_interface_changes() also
exposes a pre-existing gap: unlike binding_handle_port_binding_changes(),
it never re-scans a newly-local datapath's sibling ports (localnet /
external / vtep / multichassis) after calling add_local_datapath(). This
used to be masked by the very recompute this patch removes. Extract the
existing catch-up logic into catch_up_new_local_datapaths() and call it
from both incremental handlers.
Assisted-by: Claude Opus 4.8, Claude Code
Signed-off-by: Jun Gu <[email protected]>
---
controller/binding.c | 122 +++++++++++++++------
controller/local_data.c | 180 ++++++++++++++++++++-----------
controller/local_data.h | 22 ++--
controller/ovn-controller.c | 94 +++++++++++++---
tests/ovn-controller.at | 81 ++++++++++++++
tests/ovn-inc-proc-graph-dump.at | 5 +
6 files changed, 387 insertions(+), 117 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index de51be823..a91193aa0 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -2706,6 +2706,30 @@ is_iface_vif(const struct ovsrec_interface *iface_rec)
return true;
}
+/* Patch (localnet / L2 gateway) ports are OVS interfaces that
+ * ovn-controller itself creates and manages. They never carry an iface-id
+ * and are of no interest to port binding processing: their ofport changes
+ * are already tracked by the patch_port_data engine node. Recognizing them
+ * here lets binding_handle_ovs_interface_changes() skip them instead of
+ * treating them as an unhandled change and forcing a full recompute of
+ * runtime_data (and, transitively, of lflow_output) on every patch port
+ * bind / migration.
This doesn't seem right. What prevents someone from adding an LSP using
a patch port? It will have iface-id and it will be externally managed by
the user or CMS. Is there something that doesn't allow that?
Best regards, Ilya Maximets.
Hi Ilya, thanks for looking at this.
Nothing prevents that, and the patch doesn't break it. The new check is
never reached for an interface that has, or had, an iface-id:
const char *iface_id = smap_get(&iface_rec->external_ids, "iface-id");
const char *old_iface_id = smap_get(b_ctx_out->local_iface_ids,
iface_rec->name);
if (!iface_id && !old_iface_id && !is_iface_vif(iface_rec)) {
if (is_iface_patch(iface_rec)) {
continue;
}
handled = false;
break;
}
We only skip an interface that has no iface-id now and had none the last
time we looked at it, i.e. one there is nothing to claim or release for.
The claim loop below is gated on 'iface_id && ofport > 0 &&
is_iface_in_int_bridge()' and is untouched, so an LSP backed by a
user/CMS-managed OVS patch port is claimed, followed and released exactly
as before.
The same holds for the engine node split: en_patch_port_data only collects
ports carrying external_ids:ovn-localnet-port / ovn-l2gateway-port, i.e.
the patch ports ovn-controller creates itself, and physical.c only consults
that simap for LP_LOCALNET / LP_L2GATEWAY. A VIF port binding backed by a
patch interface still gets its ofport from local_binding_get_lport_ofport().
What is wrong is my wording: both the commit message and the comment above
is_iface_patch() claim that "patch ports never carry an iface-id", which is
exactly the assumption you are objecting to and is not what the code relies
on. v3 rewords both and adds a test that backs an LSP with a CMS-managed
OVS patch port and checks it is claimed, follows ofport changes, and is
released when the iface-id is cleared.
Thanks,
Jun
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
