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
