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 left unbound but 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. Ports
bound to another chassis are left alone. The function now takes the
Port_Binding instead of its name.
Such an interface has no OVS interface name, so also make
if_status_mgr_delete_iface() cope with a NULL 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]>
---
v3:
- Only take over ports left unbound; v2 also set down ports bound to
another chassis, which broke "Deleting vif while controller fight for
port claim".
- Handle the NULL interface name in if_status_mgr_delete_iface()
(sanitizer report from CI).
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.
controller/binding.c | 6 +++---
controller/if-status.c | 21 ++++++++++++++++-----
controller/if-status.h | 3 ++-
tests/ovn-controller.at | 38 ++++++++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+), 9 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..5d773f9b7d 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->chassis && pb->n_up && pb->up[0]) {
+ /* Unbound but still up, e.g. bound by a previous ovn-controller
+ * instance and never claimed by this one: 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;
}
@@ -436,9 +446,10 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr,
const char *iface_id,
return;
}
- if (iface_rec && strcmp(iface->name, iface_rec->name)) {
+ if (iface_rec && (!iface->name || strcmp(iface->name, iface_rec->name))) {
VLOG_DBG("Interface %s not deleted as port %s bound to %s",
- iface_rec->name, iface_id, iface->name);
+ iface_rec->name, iface_id,
+ iface->name ? iface->name : "no interface");
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
--
2.49.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
