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.
Signed-off-by: Xavier Simonart <[email protected]> --- 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 bfdeb99b9..31d73d6a9 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2529,7 +2529,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 f44f95833..d13ae36c7 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -284,4 +284,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 9101dfa8d..48ecadb64 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -4872,6 +4872,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); @@ -5254,6 +5257,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); @@ -5608,6 +5613,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..662ae52c4 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 acf18c4e0..01064a37f 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -38924,3 +38924,56 @@ OVN_CHECK_PACKETS([hv/vif1-tx.pcap], [expected-vif1]) 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 +]) -- 2.31.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
