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. 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
