Hi Dumitru, Thinking about it more after your suggestion: with patch 1 applied, every consumer of Port_Binding.up I could find also checks the chassis, so a port left unbound but up no longer affects service monitors or anything else I know of. What this patch still fixes is the stale "up" in the Southbound record and the "Trying to release unknown interface" warning.
If you think that is not worth touching if-status for, I'm fine with dropping it. Otherwise v3 is ready for review; the remaining CI failure is "virtual port claim postpone", which passes locally and also failed on an unrelated series on 9/21. Regards, Jaygue On Thu, Oct 1, 2026 at 10:42 AM Jaygue Lee <[email protected]> wrote: > Sorry for the noise. > > The checkpatch error the robot reported on v3 comes from my mail > client's display name ("jay"), which patchwork picked up; the patch > itself is unchanged and signed off as Jaygue Lee. > > Regards, > Jaygue > > On Thu, Oct 1, 2026 at 10:01 AM jay <[email protected]> wrote: > >> Hi Dumitru, >> >> My bad for not following up on the CI failure >> sooner; I'm in KST and only saw it this morning. >> >> v2 also took over ports bound to another chassis, so in "Deleting vif >> while controller fight for port claim" hv1 set hv2's port down, and the >> entry it added has no interface name, which crashed in >> if_status_mgr_delete_iface(). I only ran ovn-controller.at before >> sending v2 and missed it. >> >> v3 only adds the entry when the port is left unbound, and handles the >> NULL name in if_status_mgr_delete_iface(). The full testsuite passes >> locally. >> >> Regards, >> Jaygue >> >> On Wed, Sep 30, 2026 at 8:56 PM Dumitru Ceara <[email protected]> wrote: >> >>> 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
