On 9/30/26 8:36 AM, Jaygue Lee wrote:
> if_status_mgr_release_iface() only moves an interface to OIF_MARK_DOWN,
> and therefore only clears Port_Binding "up", for interfaces this
> ovn-controller instance claimed itself. Ports that were already bound to
> this chassis when ovn-controller started are adopted without being
> tracked, so when the recompute path releases one of them, for example
> after a hypervisor crash where the VM's tap is gone but the Port_Binding
> still points at this chassis, the chassis is cleared but "up" stays true.
>
> That leaves the port unbound but up, a state that cannot happen
> otherwise. Since commit e180d57ee6 ("northd: Mark unbound ports' service
> monitors offline.") northd no longer trusts "up" alone for service
> monitors, but the Southbound record stays wrong until the port is claimed
> again, and anything else reading Port_Binding.up is misled.
>
> When if_status_mgr_release_iface() is asked to release an interface it
> does not track and the Port_Binding is still up, add it in
> OIF_UPDATE_PORT, the state the tracked path uses for a released port with
> no local binding, so that if_status_mgr_update() sets it down. The
> function now takes the Port_Binding instead of its name.
>
> CC: Dumitru Ceara <[email protected]>
> Fixes: 5c3371922994 ("if-status: Add OVS interface status management module.")
> Assisted-by: Claude Opus 5, Claude Code
> Signed-off-by: Jaygue Lee <[email protected]>
> ---
> v2:
> - Fix it in if_status_mgr_release_iface() instead of release_lport()
> (Dumitru).
> - Use wait_for_ports_up in the test (Dumitru).
> - Such ports no longer log "Trying to release unknown interface", so
> the test no longer ignores that warning.
> - Dropped Mairtin's Acked-by since the fix changed.
>
Hi Jaygue,
Thanks for the v2!
Unfortunately, this fails in CI:
https://github.com/ovsrobot/ovn/actions/runs/36680140284/job/109774567298#step:13:5816
Artifacts:
https://github.com/ovsrobot/ovn/actions/runs/36680140284/artifacts/11081989614
Due to a null pointer dereferencing:
controller/if-status.c:449:29: runtime error: null pointer passed as
argument 1, which is declared to never be null
/usr/include/string.h:157:33: note: nonnull attribute specified here
#0 0x55a203f415e8 in if_status_mgr_delete_iface
/workspace/ovn-tmp/controller/if-status.c:449:22
#1 0x55a203f068ab in local_binding_delete
/workspace/ovn-tmp/controller/binding.c:3561:5
#2 0x55a203f068ab in consider_iface_release
/workspace/ovn-tmp/controller/binding.c:2685:13
#3 0x55a203f068ab in binding_handle_ovs_interface_changes
/workspace/ovn-tmp/controller/binding.c:2857:23
#4 0x55a20401c05c in runtime_data_ovs_interface_shadow_handler
/workspace/ovn-tmp/controller/ovn-controller.c:1721:10
#5 0x55a2041ba568 in run_change_handler
/workspace/ovn-tmp/lib/inc-proc-eng.c:519:11
#6 0x55a2041ba568 in engine_compute
/workspace/ovn-tmp/lib/inc-proc-eng.c:584:23
#7 0x55a2041ba568 in engine_run_node
/workspace/ovn-tmp/lib/inc-proc-eng.c:656:14
#8 0x55a2041ba568 in engine_run
/workspace/ovn-tmp/lib/inc-proc-eng.c:685:9
#9 0x55a204003963 in main
/workspace/ovn-tmp/controller/ovn-controller.c:8456:21
#10 0x7f1c570df1c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9)
(BuildId: a4a7992a8e66555c8141ab2a08a8465ff6e0ea65)
#11 0x7f1c570df28a in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId:
a4a7992a8e66555c8141ab2a08a8465ff6e0ea65)
#12 0x55a203e12e94 in _start
(/workspace/ovn-tmp/controller/ovn-controller+0x2f7e94) (BuildId:
a4094ff66824dc71f3133aa07fcaf2e4328d0206)
Regards,
Dumitru
> controller/binding.c | 6 +++---
> controller/if-status.c | 16 +++++++++++++---
> controller/if-status.h | 3 ++-
> tests/ovn-controller.at | 38 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 56 insertions(+), 7 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index c2bce665e7..5812641a44 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1593,7 +1593,7 @@ release_lport(const struct sbrec_port_binding *pb,
> VLOG_INFO("Releasing lport %s", pb->logical_port);
> }
> update_lport_tracking(pb, tracked_datapaths, false);
> - if_status_mgr_release_iface(if_mgr, pb->logical_port);
> + if_status_mgr_release_iface(if_mgr, pb);
> return true;
> }
>
> @@ -2925,7 +2925,7 @@ handle_deleted_lport(const struct sbrec_port_binding
> *pb,
> * it is seen as never claimed.
> */
> if (if_status_is_port_claimed(b_ctx_out->if_mgr, pb->logical_port)) {
> - if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
> + if_status_mgr_release_iface(b_ctx_out->if_mgr, pb);
> }
> return;
> }
> @@ -2948,7 +2948,7 @@ handle_deleted_lport(const struct sbrec_port_binding
> *pb,
> ld);
> }
> if (if_status_is_port_claimed(b_ctx_out->if_mgr, pb->logical_port)) {
> - if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
> + if_status_mgr_release_iface(b_ctx_out->if_mgr, pb);
> }
> }
> }
> diff --git a/controller/if-status.c b/controller/if-status.c
> index 6c6e9b27b1..2d1a694c54 100644
> --- a/controller/if-status.c
> +++ b/controller/if-status.c
> @@ -390,13 +390,23 @@ get_claimed_cr(struct if_status_mgr *mgr)
> }
>
> void
> -if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id)
> +if_status_mgr_release_iface(struct if_status_mgr *mgr,
> + const struct sbrec_port_binding *pb)
> {
> - struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
> + struct ovs_iface *iface = shash_find_data(&mgr->ifaces,
> pb->logical_port);
>
> if (!iface) {
> + if (pb->n_up && pb->up[0]) {
> + /* Bound by a previous ovn-controller instance, never claimed by
> + * this one: still set it down. */
> + iface = ovs_iface_create(mgr, pb->logical_port, NULL,
> + OIF_UPDATE_PORT);
> + iface->pb_uuid = pb->header_.uuid;
> + return;
> + }
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> - VLOG_WARN_RL(&rl, "Trying to release unknown interface %s",
> iface_id);
> + VLOG_WARN_RL(&rl, "Trying to release unknown interface %s",
> + pb->logical_port);
> return;
> }
>
> diff --git a/controller/if-status.h b/controller/if-status.h
> index 75c7bf71c7..eaf7127aa0 100644
> --- a/controller/if-status.h
> +++ b/controller/if-status.h
> @@ -36,7 +36,8 @@ void if_status_mgr_claim_iface(struct if_status_mgr *,
> bool sb_readonly, enum can_bind bind_type,
> bool notify_up,
> const struct sbrec_port_binding *parent_pb);
> -void if_status_mgr_release_iface(struct if_status_mgr *, const char
> *iface_id);
> +void if_status_mgr_release_iface(struct if_status_mgr *,
> + const struct sbrec_port_binding *);
> void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id,
> const struct ovsrec_interface *iface_rec);
>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index a7b79fc670..c4a0837566 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -3070,6 +3070,44 @@ OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - released untracked port is set down])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl lsp-add sw0 sw0-p1 -- lsp-set-addresses sw0-p1 \
> +"00:00:00:00:00:01 10.0.0.1"
> +check ovs-vsctl -- add-port br-int hv1-vif1 -- \
> + set interface hv1-vif1 external-ids:iface-id=sw0-p1
> +wait_for_ports_up sw0-p1
> +
> +# Stop ovn-controller without releasing anything (as after a crash), remove
> +# the VIF while it is stopped (the VM did not come back) and start it again.
> +# The port is still bound to hv1 in the SB but this ovn-controller instance
> +# never claimed it, so if-status does not track it.
> +check ovn-appctl -t ovn-controller exit --restart
> +check ovs-vsctl del-port br-int hv1-vif1
> +start_daemon ovn-controller
> +
> +# The port must be released and set down.
> +wait_row_count Port_Binding 1 logical_port=sw0-p1 'chassis=[[]]' 'up=false'
> +wait_row_count nb:Logical_Switch_Port 1 name=sw0-p1 'up=false'
> +
> +# Adding the VIF back claims the port again as usual.
> +check ovs-vsctl -- add-port br-int hv1-vif1 -- \
> + set interface hv1-vif1 external-ids:iface-id=sw0-p1
> +wait_for_ports_up sw0-p1
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> +
> OVN_FOR_EACH_NORTHD([
> AT_SETUP([Encap enforce local_ip])
> ovn_start
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
