On Thu, Oct 15, 2026 at 3:25 PM Numan Siddique <[email protected]> wrote:
> On Thu, Oct 15, 2026 at 7:34 AM MJ Ponsonby <[email protected]> > wrote: > > > > From: Xavier Simonart <[email protected]> > > > > Transaction failures could occur as trying to plug multiple times > > the same interface in ovsdb i.e. > > "Transaction causes multiple rows in \"Interface\" table to have > identical values". > > This could for instance occur between the time the VIF is plug first and > > an ofport is created. > > > > Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/2170570 > > Signed-off-by: Xavier Simonart <[email protected]> > > Reviewed-by: Ales Musil <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > (cherry picked from commit fcc2bf154ea591ec267940ba9ba9ef3fa16af6ab) > > Assisted-by: OpenCode (openai/gpt-6-astra) > > Signed-off-by: MJ Ponsonby <[email protected]> > > --- > > Backport to branch-24.03, based on > dd45c938817e116e42f9a4f42be4c501d205bf14. > > > > The linked report describes duplicate OVSDB rows blocking ovn-controller > > transactions on a 24.03.8 DPU deployment. This backport is proposed to > > address that failure mode; validation on the affected hardware is > pending. > > > > Only patch context is refreshed, with the regression test appended to the > > 24.03 test suite. The code and test changes match the upstream commit. > > Hi MJ and Numan, > Thanks for the backport. branch-24.03 is EOL and we have locked this > branch. It is indeed EOL, the only things still accepted into 24.03 are critical fixes for CVEs. I'm afraid that is not the case here. > I guess you need to maintain a downstream version yourself with this fix. > > @Dumitru Ceara correct me if I'm wrong here. > > Numan > Thanks, Ales > > > > > Testing: > > - Built OVN against its pinned OVS revision > > bc99439446a1c0b94498ad7ea44f3d1d933ead90. > > - make check TESTSUITEFLAGS='-j4 -k vif-plug': all 4 tests passed, > > including both variants of the transaction-failure regression test. > > - git diff --check and git apply --check passed. > > > > controller/binding.c | 2 +- > > controller/binding.h | 3 + > > controller/ovn-controller.c | 7 +++ > > controller/vif-plug.c | 117 ++++++++++++++++++++++-------------- > > controller/vif-plug.h | 1 + > > tests/ovn.at | 53 ++++++++++++++++ > > 6 files changed, 137 insertions(+), 46 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 199d08915..4d9715a4f 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -2528,7 +2528,7 @@ is_iface_vif(const struct ovsrec_interface > *iface_rec) > > return true; > > } > > > > -static bool > > +bool > > is_iface_in_int_bridge(const struct ovsrec_interface *iface, > > const struct ovsrec_bridge *br_int) > > { > > diff --git a/controller/binding.h b/controller/binding.h > > index 055b080a9..e954cd6a6 100644 > > --- a/controller/binding.h > > +++ b/controller/binding.h > > @@ -283,4 +283,7 @@ void claimed_lport_set_up(const struct > sbrec_port_binding *pb, > > bool port_binding_is_up(const struct sbrec_chassis *chassis_rec, > > const struct sbrec_port_binding_table *, > > const struct uuid *pb_uuid); > > +bool is_iface_in_int_bridge(const struct ovsrec_interface *iface, > > + const struct ovsrec_bridge *br_int); > > + > > #endif /* controller/binding.h */ > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 88e0b44ed..5ace54e17 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -5268,6 +5268,9 @@ main(int argc, char *argv[]) > > struct ovsdb_idl_index *ovsrec_port_by_qos > > = ovsdb_idl_index_create1(ovs_idl_loop.idl, > > &ovsrec_port_col_qos); > > + struct ovsdb_idl_index *ovsrec_interface_by_name > > + = ovsdb_idl_index_create1(ovs_idl_loop.idl, > > + &ovsrec_interface_col_name); > > struct ovsdb_idl_index *ovsrec_queue_by_external_ids > > = ovsdb_idl_index_create1(ovs_idl_loop.idl, > > &ovsrec_queue_col_external_ids); > > @@ -5658,6 +5661,8 @@ main(int argc, char *argv[]) > > engine_ovsdb_node_add_index(&en_ovs_flow_sample_collector_set, "id", > > ovsrec_flow_sample_collector_set_by_id); > > engine_ovsdb_node_add_index(&en_ovs_port, "qos", > ovsrec_port_by_qos); > > + engine_ovsdb_node_add_index(&en_ovs_interface, "name", > > + ovsrec_interface_by_name); > > engine_ovsdb_node_add_index(&en_ovs_queue, "external_ids", > > ovsrec_queue_by_external_ids); > > > > @@ -6030,6 +6035,8 @@ main(int argc, char *argv[]) > > > sbrec_port_binding_by_requested_chassis, > > .ovsrec_port_by_interfaces = > > ovsrec_port_by_interfaces, > > + .ovsrec_interface_by_name = > > + ovsrec_interface_by_name, > > .ovs_table = ovs_table, > > .br_int = br_int, > > .iface_table = > > diff --git a/controller/vif-plug.c b/controller/vif-plug.c > > index d4c7552ea..1fa536cad 100644 > > --- a/controller/vif-plug.c > > +++ b/controller/vif-plug.c > > @@ -208,6 +208,22 @@ transact_update_port(const struct ovsrec_interface > *iface_rec, > > mtu_request); > > } > > > > +static const struct ovsrec_interface * > > +iface_lookup_by_name(struct ovsdb_idl_index *ovsrec_interface_by_name, > > + char *name) > > +{ > > + struct ovsrec_interface *iface = ovsrec_interface_index_init_row( > > + ovsrec_interface_by_name); > > + ovsrec_interface_set_name(iface, name); > > + > > + const struct ovsrec_interface *retval > > + = ovsrec_interface_index_find(ovsrec_interface_by_name, > > + iface); > > + > > + ovsrec_interface_index_destroy_row(iface); > > + > > + return retval; > > +} > > > > static bool > > consider_unplug_iface(const struct ovsrec_interface *iface, > > @@ -287,11 +303,10 @@ get_plug_mtu_request(const struct smap > *lport_options) > > } > > > > static bool > > -consider_plug_lport_create__(const struct vif_plug_class *vif_plug, > > - const struct smap *iface_external_ids, > > - const struct sbrec_port_binding *pb, > > - struct vif_plug_ctx_in *vif_plug_ctx_in, > > - struct vif_plug_ctx_out *vif_plug_ctx_out) > > +consider_plug_lport__(const struct vif_plug_class *vif_plug, > > + const struct sbrec_port_binding *pb, > > + struct vif_plug_port_ctx **vif_plug_port_ctx_ptr, > > + struct vif_plug_ctx_in *vif_plug_ctx_in) > > { > > if (!vif_plug_ctx_in->chassis_rec || !vif_plug_ctx_in->br_int > > || !vif_plug_ctx_in->ovs_idl_txn) { > > @@ -317,13 +332,22 @@ consider_plug_lport_create__(const struct > vif_plug_class *vif_plug, > > &vif_plug_port_ctx->vif_plug_port_ctx_out)) { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > VLOG_INFO_RL(&rl, > > - "Not plugging lport %s on direction from VIF plug " > > - "provider.", > > + "Not plugging/updating lport %s on direction from > VIF " > > + "plug provider.", > > pb->logical_port); > > destroy_port_ctx(vif_plug_port_ctx); > > return true; > > } > > - > > + *vif_plug_port_ctx_ptr = vif_plug_port_ctx; > > + return true; > > +} > > +static bool > > +consider_plug_lport_create__(const struct smap *iface_external_ids, > > + const struct sbrec_port_binding *pb, > > + const struct vif_plug_port_ctx > *vif_plug_port_ctx, > > + struct vif_plug_ctx_in *vif_plug_ctx_in, > > + struct vif_plug_ctx_out *vif_plug_ctx_out) > > +{ > > VLOG_INFO("Plugging port %s into %s for lport %s on this " > > "chassis.", > > vif_plug_port_ctx->vif_plug_port_ctx_out.name, > > @@ -340,41 +364,12 @@ static bool > > consider_plug_lport_update__(const struct vif_plug_class *vif_plug, > > const struct smap *iface_external_ids, > > const struct sbrec_port_binding *pb, > > - struct local_binding *lbinding, > > + const struct ovsrec_interface *iface_rec, > > + struct vif_plug_port_ctx > *vif_plug_port_ctx, > > struct vif_plug_ctx_in *vif_plug_ctx_in, > > struct vif_plug_ctx_out *vif_plug_ctx_out) > > { > > - if (!vif_plug_ctx_in->chassis_rec || !vif_plug_ctx_in->br_int > > - || !vif_plug_ctx_in->ovs_idl_txn) { > > - /* Some of our prerequisites are not available, ask for a > recompute. */ > > - return false; > > - } > > - /* Our contract with the VIF plug provider is that > vif_plug_port_finish > > - * will be called with vif_plug_port_ctx_in and > vif_plug_port_ctx_out > > - * objects once the transaction commits. > > - * > > - * Since this happens asynchronously we need to allocate memory for > > - * and duplicate any database references so that they stay valid. > > - * > > - * The data is freed with a call to destroy_port_ctx after the > > - * transaction completes at the end of the ovn-controller main > > - * loop. */ > > - struct vif_plug_port_ctx *vif_plug_port_ctx = build_port_ctx( > > - vif_plug, PLUG_OP_CREATE, vif_plug_ctx_in, pb, NULL, NULL); > > - > > - if (!vif_plug_port_prepare(vif_plug, > > - &vif_plug_port_ctx->vif_plug_port_ctx_in, > > - > &vif_plug_port_ctx->vif_plug_port_ctx_out)) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > - VLOG_INFO_RL(&rl, > > - "Not updating lport %s on direction from VIF plug " > > - "provider.", > > - pb->logical_port); > > - destroy_port_ctx(vif_plug_port_ctx); > > - return true; > > - } > > - > > - if (strcmp(lbinding->iface->name, > > + if (strcmp(iface_rec->name, > > vif_plug_port_ctx->vif_plug_port_ctx_out.name)) { > > VLOG_WARN("Attempt of incompatible change to existing " > > "port detected, please recreate port: %s", > > @@ -386,7 +381,7 @@ consider_plug_lport_update__(const struct > vif_plug_class *vif_plug, > > return false; > > } > > VLOG_DBG("updating iface for: %s", pb->logical_port); > > - transact_update_port(lbinding->iface, vif_plug_ctx_in, > vif_plug_ctx_out, > > + transact_update_port(iface_rec, vif_plug_ctx_in, vif_plug_ctx_out, > > vif_plug_port_ctx, iface_external_ids, > > get_plug_mtu_request(&pb->options)); > > > > @@ -438,13 +433,45 @@ consider_plug_lport(const struct > sbrec_port_binding *pb, > > UUID_ARGS(&lbinding->iface->header_.uuid)); > > return false; > > } > > + struct vif_plug_port_ctx *vif_plug_port_ctx = NULL; > > + if (!consider_plug_lport__(vif_plug, pb, &vif_plug_port_ctx, > > + vif_plug_ctx_in)) { > > + return false; > > + } > > + > > ret = consider_plug_lport_update__(vif_plug, > &iface_external_ids, > > - pb, lbinding, > vif_plug_ctx_in, > > + pb, lbinding->iface, > > + vif_plug_port_ctx, > > + vif_plug_ctx_in, > > vif_plug_ctx_out); > > } else { > > - ret = consider_plug_lport_create__(vif_plug, > &iface_external_ids, > > - pb, vif_plug_ctx_in, > > - vif_plug_ctx_out); > > + struct vif_plug_port_ctx *vif_plug_port_ctx = NULL; > > + if (!consider_plug_lport__(vif_plug, pb, &vif_plug_port_ctx, > > + vif_plug_ctx_in)) { > > + return false; > > + } > > + > > + const struct ovsrec_interface *iface_rec = > > + > iface_lookup_by_name(vif_plug_ctx_in->ovsrec_interface_by_name, > > + vif_plug_port_ctx-> > vif_plug_port_ctx_out.name); > > + if (iface_rec && > > + smap_get(&iface_rec->external_ids, "iface-id") && > > + smap_get(&iface_rec->external_ids, > > + OVN_PLUGGED_EXT_ID) && > > + is_iface_in_int_bridge(iface_rec, > vif_plug_ctx_in->br_int)) { > > + > > + ret = consider_plug_lport_update__(vif_plug, > > + &iface_external_ids, > > + pb, iface_rec, > > + vif_plug_port_ctx, > > + vif_plug_ctx_in, > > + vif_plug_ctx_out); > > + } else { > > + ret = consider_plug_lport_create__(&iface_external_ids, > pb, > > + vif_plug_port_ctx, > > + vif_plug_ctx_in, > > + vif_plug_ctx_out); > > + } > > } > > } > > > > diff --git a/controller/vif-plug.h b/controller/vif-plug.h > > index 7a1978e38..0fc7219c4 100644 > > --- a/controller/vif-plug.h > > +++ b/controller/vif-plug.h > > @@ -34,6 +34,7 @@ struct vif_plug_ctx_in { > > struct ovsdb_idl_index *sbrec_port_binding_by_name; > > struct ovsdb_idl_index *sbrec_port_binding_by_requested_chassis; > > struct ovsdb_idl_index *ovsrec_port_by_interfaces; > > + struct ovsdb_idl_index *ovsrec_interface_by_name; > > const struct ovsrec_open_vswitch_table *ovs_table; > > const struct ovsrec_bridge *br_int; > > const struct ovsrec_interface_table *iface_table; > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 6dd62ece1..5133edb87 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -41170,3 +41170,56 @@ OVN_CHECK_PACKETS_UNIQ([hv1/snoopvif-tx.pcap], > [expected]) > > > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn-controller - vif-plug transaction failures]) > > +AT_KEYWORDS([vif-plug]) > > + > > +ovn_start > > + > > +net_add n1 > > +sim_add hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +ovn-appctl vlog/set dbg > > + > > +sim_add hv2 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.2 > > +ovn-appctl vlog/set dbg > > + > > +check ovn-nbctl ls-add lsw0 > > + > > +check ovn-nbctl lsp-add lsw0 lsp1 > > +check ovn-nbctl lsp-set-addresses lsp1 "f0:00:00:00:00:01 172.16.0.101" > > + > > +sleep_ovs hv1 > > + > > +# This used to cause Transaction failures in ovsdb until ofport is > created. > > +check ovn-nbctl lsp-set-options lsp1 \ > > + requested-chassis=hv1 \ > > + vif-plug-type=dummy \ > > + vif-plug-mtu-request=420 > > + > > +check ovn-nbctl lsp-add lsw0 lsp2 > > +check ovn-nbctl lsp-set-addresses lsp2 "f0:00:00:00:00:02 172.16.0.102" > > + > > +sleep_ovs hv2 > > + > > +check ovn-nbctl lsp-set-options lsp2 \ > > + requested-chassis=hv2 \ > > + vif-plug-type=dummy \ > > + vif-plug-mtu-request=420 > > + > > +wake_up_ovs hv1 > > +wait_for_ports_up lsp1 > > + > > +# Exit ovn-controller while last transaction to ovsdb is not completed > > +as hv2 > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > +wake_up_ovs hv2 > > +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > > > base-commit: dd45c938817e116e42f9a4f42be4c501d205bf14 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
