On Mon, Feb 26, 2024 at 2:55 PM Mohammad Heib <[email protected]> wrote:
> ovn-controller immediately removes the vport_bindings requests that were > generated by VIFs after handling them locally, this approach is intended > to avoid binding the vport to one VIF only and allocate the vport > between the different VIFs that exist in the vport:virtual-parents. > > Although the behavior mentioned above is correct, in some cases when the > SB Database is busy the transaction that binds this vport to the desired > VIF/chassis can fail and the controller will not re-try to bind the > vport again because we deleted the bind_vport request in the previous > loop/TXN. > > This patch aims to change the above behavior by storing the bind_vport > requests for a bit longer time and this is done by the following: > 1. mark each new bind_vport request as new. > > 2. loop0: ovn-controller will try to handle this bind_vport request > for the first time as usual (no change). > > 3. loop0: ovn-controller will try to delete the already handled > bind_vport > request as usual but first, it will check if this request is marked > as new and > if so the controller will mark this request as an old request and > keep it, > otherwise remove it. > > 4. loop1: ovn-controller will try to commit the same change again for > the old request, if the previous commit in loop0 succeeded the > change will not have any effect on SB, otherwise we will try to > commit the same vport_bind request again. > > 5. loop1: delete the old bind_vport request. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1954659 > Signed-off-by: Mohammad Heib <[email protected]> > --- > V2: > Address comments from Ales in v1. > --- > controller/pinctrl.c | 50 ++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 44 insertions(+), 6 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 98b29de9f..e2f86f299 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -6529,11 +6529,46 @@ struct put_vport_binding { > uint32_t vport_key; > > uint32_t vport_parent_key; > + > + /* This vport record Only relevant if "new_record" is true. */ > + bool new_record; > }; > > /* Contains "struct put_vport_binding"s. */ > static struct hmap put_vport_bindings; > > +/* > + * Validate if the vport_binding record that was added > + * by the pinctrl thread is still relevant and needs > + * to be updated in the SBDB or not. > + * > + * vport_binding record is only relevant and needs to be updated in SB if: > + * 2. The put_vport_binding:new_record is true: > + * The new_record will be set to "true" when this vport record is > created > + * by function "pinctrl_handle_bind_vport". > + * > + * After the first attempt to bind this vport to the chassis and > + * virtual_parent by function "run_put_vport_bindings" we will set > the > + * value of vpb:new_record to "false" and keep it in > "put_vport_bindings" > + * > + * After the second attempt of binding the vpb it will be removed by > + * this function. > + * > + * The above guarantees that we will try to bind the vport twice in > + * a certain amount of time. > + * > +*/ > +static bool > +is_vport_binding_relevant(struct put_vport_binding *vpb) > +{ > + > + if (vpb->new_record) { > + vpb->new_record = false; > + return true; > + } > + return false; > +} > + > static void > init_put_vport_bindings(void) > { > @@ -6541,18 +6576,21 @@ init_put_vport_bindings(void) > } > > static void > -flush_put_vport_bindings(void) > +flush_put_vport_bindings(bool force_flush) > { > struct put_vport_binding *vport_b; > - HMAP_FOR_EACH_POP (vport_b, hmap_node, &put_vport_bindings) { > - free(vport_b); > + HMAP_FOR_EACH_SAFE (vport_b, hmap_node, &put_vport_bindings) { > + if (!is_vport_binding_relevant(vport_b) || force_flush) { > + hmap_remove(&put_vport_bindings, &vport_b->hmap_node); > + free(vport_b); > + } > } > } > > static void > destroy_put_vport_bindings(void) > { > - flush_put_vport_bindings(); > + flush_put_vport_bindings(true); > hmap_destroy(&put_vport_bindings); > } > > @@ -6630,7 +6668,7 @@ run_put_vport_bindings(struct ovsdb_idl_txn > *ovnsb_idl_txn, > sbrec_port_binding_by_key, chassis, vpb); > } > > - flush_put_vport_bindings(); > + flush_put_vport_bindings(false); > } > > /* Called with in the pinctrl_handler thread context. */ > @@ -6668,7 +6706,7 @@ pinctrl_handle_bind_vport( > vpb->dp_key = dp_key; > vpb->vport_key = vport_key; > vpb->vport_parent_key = vport_parent_key; > - > + vpb->new_record = true; > notify_pinctrl_main(); > } > > -- > 2.34.3 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-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
