Hello Aditya Thanks for the patch and for v2. I only have a minor comment below that the maintainer can address when applying the fix.
Acked-by: Xavier Simonart <[email protected]> Thanks Xavier On Fri, Jun 5, 2026 at 7:41 PM Aditya Mehakare <[email protected]> wrote: > When a container (or virtual/mirror) child port is deleted, > handle_deleted_vif_lport() calls if_status_mgr_remove_ovn_installed() > on the associated lbinding's OVS interface. For child port types, > lbinding points to the parent's local_binding, so this incorrectly > strips ovn-installed from the parent's OVS interface. > > This causes a cascade of problems: > 1. ovn-installed is removed from the parent's OVS interface. > 2. On the next full recompute (triggered by any SB commit failure, > IDL reconnection, etc.), claim_lport() notices the missing > ovn-installed and re-claims the parent port. > 3. The parent port re-enters OIF_CLAIMED -> OIF_INSTALL_FLOWS, > during which local_binding_set_down() marks the parent AND all > remaining children as down in the Southbound database. > 4. After flows are re-installed, all ports are set back up. > > This results in a disruptive and unnecessary down/up bounce of the > parent port and all sibling container ports whenever a single child > port is deleted. > > Fix this by skipping ovn-installed removal for port types that are > bound through a parent's local_binding rather than their own, so > that deleting a child port no longer disturbs the parent's interface > state. > > Fixes: ec1db7ae09fa ("ovn-controller: fixed ovn-installed not always > properly added or removed.") > Signed-off-by: Aditya Mehakare <[email protected]> > Acked-by: Naveen Yerramneni <[email protected]> > --- > V2: > - Replace explicit LP_VIF check with is_lport_type_child() helper > and relocate test. > --- > controller/binding.c | 12 ++++++- > tests/ovn-controller.at | 71 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 82 insertions(+), 1 deletion(-) > > diff --git a/controller/binding.c b/controller/binding.c > index fa1984ee7..de51be823 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1612,6 +1612,15 @@ is_binding_lport_this_chassis(struct binding_lport > *b_lport, > || is_postponed_port(b_lport->pb->logical_port))); > } > > +/* Returns 'true' if an lport of this type is bound through a parent's > + * local_binding rather than its own. Container, virtual and mirror ports > + * share the parent VIF's OVS interface and local_binding. */ > +static bool > +is_lport_type_child(enum en_lport_type type) > +{ > + return type == LP_CONTAINER || type == LP_VIRTUAL || type == > LP_MIRROR; > +} > + > /* Returns 'true' if the 'lbinding' has binding lports of type > * LP_CONTAINER/LP_MIRROR, 'false' otherwise. */ > static bool > @@ -2988,7 +2997,8 @@ handle_deleted_vif_lport(const struct > sbrec_port_binding *pb, > } > > handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > - if (lbinding && lbinding->iface && lbinding->iface->name) { > + if (!is_lport_type_child(lport_type) && > + lbinding && lbinding->iface && lbinding->iface->name) { > if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr, > lbinding->iface); > } > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index fd63399f1..c006f3219 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -4108,3 +4108,74 @@ wait_for_ports_up > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-controller - Delete container child port does not bounce > parent]) > +ovn_start > + > +net_add n1 > + > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +as hv1 check ovs-vsctl -- add-port br-int vif \ > + -- set Interface vif external-ids:iface-id=lsp0 > + > +check ovn-nbctl ls-add ls0 > +check ovn-nbctl lsp-add ls0 lsp0 > +for i in 1 2 3; do > + check ovn-nbctl lsp-add ls0 lsp-cont$i lsp0 $i > +done > + > +wait_for_ports_up > +ch=$(fetch_column Chassis _uuid name=hv1) > +for port in lsp0 lsp-cont1 lsp-cont2 lsp-cont3; do > + wait_row_count Port_Binding 1 logical_port=$port chassis=$ch > +done > + > +OVS_WAIT_UNTIL([test `as hv1 ovs-vsctl get Interface vif > external_ids:ovn-installed` = '"true"']) > + > +# Case 1: delete one child (lsp-cont3); two siblings remain. > +check ovn-nbctl --wait=hv lsp-del lsp-cont3 > +for port in lsp0 lsp-cont1 lsp-cont2; do > + wait_column "true" sb:Port_Binding up logical_port=$port > +done > +AT_CHECK([test `as hv1 ovs-vsctl get Interface vif > external_ids:ovn-installed` = '"true"']) > + > +# Force a full recompute and re-check. > +as hv1 ovn-appctl -t ovn-controller recompute > +check ovn-nbctl --wait=hv sync > +for port in lsp0 lsp-cont1 lsp-cont2; do > + wait_column "true" sb:Port_Binding up logical_port=$port > +done > +AT_CHECK([test `as hv1 ovs-vsctl get Interface vif > external_ids:ovn-installed` = '"true"']) > + > +# Case 2: delete remaining children; parent has zero children. > +for i in 1 2; do > + check ovn-nbctl --wait=hv lsp-del lsp-cont$i > +done > +wait_column "true" sb:Port_Binding up logical_port=lsp0 > +AT_CHECK([test `as hv1 ovs-vsctl get Interface vif > external_ids:ovn-installed` = '"true"']) > + > +# Force recompute with no children and verify parent is still stable. > +as hv1 ovn-appctl -t ovn-controller recompute > +check ovn-nbctl --wait=hv sync > +wait_column "true" sb:Port_Binding up logical_port=lsp0 > +AT_CHECK([test `as hv1 ovs-vsctl get Interface vif > external_ids:ovn-installed` = '"true"']) > + > +# Throughout the whole test, the parent's vif must never have its > +# ovn-installed external-id cleared, and neither the parent nor any > +# child should have been bounced down in the Southbound DB. > +AT_CHECK([grep -q "Removing iface vif ovn-installed in OVS" > hv1/ovn-controller.log], [1], []) > +for port in lsp0 lsp-cont1 lsp-cont2 lsp-cont3; do > + AT_CHECK([grep -q "Setting lport $port down in Southbound" > hv1/ovn-controller.log], [1], []) > +done > + > +# Make sure that ovn-controller has not crashed. > +AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)]) > I do not think this is necessary; OVN_CLEANUP now performs recompute, which catches an ovn-controller crash. > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.43.5 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
