On Tue, Sep 3, 2024 at 2:02 PM Xavier Simonart <[email protected]> wrote:
> 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]> > --- > There are a few formatting issues. 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) > nit: Wrong format. > +{ > + 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); > nit: Wrong format. > + > + 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)) { > nit: Wrong format. > + 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 > > With that being addressed: Reviewed-by: Ales Musil <[email protected]> -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
