On Wed, Oct 2, 2024 at 4:43 AM Ales Musil <[email protected]> wrote: > > 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]>
Fixed the formatting and applied the patch to main, Numan > > -- > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
