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.
Thanks for the backport. branch-24.03 is EOL and we have locked this branch.
I guess you need to maintain a downstream version yourself with this fix.
@Dumitru Ceara correct me if I'm wrong here.
Numan
>
> 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
