Hi Ruairi, thanks for the bug fix!
Acked-by: Rosemarie O'Riorden <[email protected]>
On 11/2/26 9:37 AM, Ruairi Carroll via dev wrote:
> A port can be added to the postponed set from two places:
> claim_lport() and the OIF_MARK_UP/OIF_INSTALL_FLOWS handling in
> if-status.c, both via lport_maybe_postpone(). In the if-status case,
> once the claim threshold has passed, if-status sets pb->chassis itself
> through local_binding_set_pb() but never removes the port from the
> postponed set.
>
> From then on the postponed_ports engine node is non-empty forever, so
> every engine run processes the port again through
> runtime_data_postponed_ports_handler() -> handle_updated_port() ->
> claim_lport(). Since pb->chassis is already this chassis,
> claim_lport() takes the "already claimed" branch, which does not remove
> the port from the set either. The port is added to the tracked
> bindings each time, so all port groups containing it are marked
> updated and every logical flow referencing those port groups is
> re-processed on every iteration.
>
> The sequence that leaves a port in the set is:
>
> 1. This chassis claims a port, and its interface is still in the
> OIF_INSTALL_FLOWS or OIF_MARK_UP state.
> 2. Within the claim threshold (500ms), another chassis sets
> pb->chassis to itself.
> 3. Both binding (claim_lport()) and if-status postpone claiming the
> port back, and it is added to the postponed set.
> 4. Once the threshold has passed, if-status sets pb->chassis back to
> this chassis before binding handles the postponed claim. Binding
> normally runs first, as engine_run() is called before
> if_status_mgr_update() in the main loop, but if the engine run is
> cancelled (e.g. a recompute is pending while recomputes are not
> allowed because of an OpenFlow backlog), if-status claims the port
> on its own.
> 5. From then on claim_lport() only sees a port that is already bound
> to this chassis, so the port is never removed from the set.
>
> Step 2 typically happens when two chassis briefly claim the same port,
> e.g. around a VM live migration, a VM re-created on another server
> with the same port, or a stale interface with the same iface-id left
> behind on another server. The postponed state survives until
> ovn-controller is restarted.
>
> On servers with large ACL port groups this kept ovn-controller at
> 100% CPU indefinitely (observed for weeks, until restart), with each
> iteration spending ~200ms re-processing ACL flows. Combined with the
> conjunction ordering issue fixed separately in ofctrl, every iteration
> also removed and re-added thousands of unchanged OpenFlow flows.
>
> Fix it by removing the port from the postponed set in the "already
> claimed by this chassis" branch of claim_lport(), as the claim is
> complete at that point.
>
> The new test reproduces the stuck state deterministically: it makes
> ovn-controller postpone a claim by binding the port to another chassis
> in the SB DB, then binds it back to this chassis while ovn-controller
> is paused, as if-status would, and checks that the postponed_ports node
> no longer reports changes on subsequent engine runs.
>
> Fixes: f64818d43933 ("controller: throttle port claim attempts from
> if-status")
> Assisted-by: Claude Opus 5.5, Claude Code
> Signed-off-by: Ruairi Carroll <[email protected]>
> ---
> controller/binding.c | 5 ++++
> tests/ovn.at | 58 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 63 insertions(+)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 3e8114e7ec..c14f9ee2f6 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1473,6 +1473,11 @@ claim_lport(const struct sbrec_port_binding *pb,
> register_claim_timestamp(pb->logical_port, now);
> sset_find_and_delete(postponed_ports, pb->logical_port);
> } else {
> + /* The port may have been postponed by if-status, which then set
> + * pb->chassis itself without going through this function. The
> + * claim is done, so the port must not stay postponed, otherwise
> + * it is re-processed on every engine run. */
> + sset_find_and_delete(postponed_ports, pb->logical_port);
> update_tracked = true;
> if ((pb->n_up && !pb->up[0]) ||
> (is_vif && !smap_get_bool(&iface_rec->external_ids,
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 7801494674..8621766730 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -43462,6 +43462,64 @@ ignored_dp=ls0])
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([Port claimed outside of binding does not stay postponed])
> +AT_KEYWORDS([claim postpone])
> +ovn_start
> +
> +check ovn-nbctl ls-add ls0
> +check ovn-nbctl lsp-add ls0 lsp0
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +check ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +# A chassis that is not running ovn-controller, so that nothing releases or
> +# claims the ports bound to it.
> +check ovn-sbctl chassis-add fake geneve 192.168.0.100
> +hv1_uuid=$(fetch_column Chassis _uuid name=hv1)
> +fake_uuid=$(fetch_column Chassis _uuid name=fake)
> +
> +as hv1 check ovn-appctl -t ovn-controller vlog/set binding:file:dbg
> +as hv1 check ovs-vsctl add-port br-int vif -- \
> + set Interface vif external_ids:iface-id=lsp0
> +wait_for_ports_up lsp0
> +pb_uuid=$(fetch_column Port_Binding _uuid logical_port=lsp0)
> +
> +# Bind lsp0 to the other chassis shortly after hv1 claimed it, so that hv1
> +# postpones claiming it back. If hv1 was too late to postpone, it claims
> +# lsp0 back right away, so just try again.
> +OVS_WAIT_UNTIL([
> + check ovn-sbctl set Port_Binding $pb_uuid chassis=$fake_uuid
> + sleep 0.1
> + grep -q "Postponed claim on logical port lsp0" hv1/ovn-controller.log
> +])
> +
> +# Bind lsp0 back to hv1 without going through binding, as if-status does
> +# once the claim threshold has passed. hv1 sees lsp0 bound to itself, so
> +# the claim is complete.
> +sleep_controller hv1
> +check ovn-sbctl set Port_Binding $pb_uuid chassis=$hv1_uuid
> +wake_up_controller hv1
> +wait_column "$hv1_uuid" Port_Binding chassis logical_port=lsp0
> +check ovn-nbctl --wait=hv sync
> +
> +# lsp0 must not stay postponed, otherwise it is processed again on every
> +# engine run.
> +as hv1 check ovn-appctl -t ovn-controller vlog/set inc_proc_eng:file:dbg
> +check ovn-nbctl --wait=hv sync
> +check ovn-nbctl --wait=hv sync
> +as hv1 check ovn-appctl -t ovn-controller vlog/set inc_proc_eng:file:info
> +AT_CHECK([grep -c "node: postponed_ports, old_state Stale, new_state
> Updated" \
> + hv1/ovn-controller.log], [1], [0
> +])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> +
> dnl This test checks that the megaflows translated by ovs-vswitchd
> dnl don't match on IPv6 source and destination addresses for
> dnl simple switching.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
