On 10/1/26 8:11 AM, Jaygue Lee wrote: > Hi Dumitru, > Hi Jaygue,
> 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. > Ack, thanks a lot for checking! > 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. I think it's worth fixing this too. I'll try to review v3 as soon as possible. > > 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. >> No worries, I can adjust any of that myself when/if merging the patch. Thanks again! Regards, Dumitru >> 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
