Re: [ovs-dev] [PATCH ovn] ovn-controller: Stop dropping bind_vport requests immediately after handling.
Hi Ales, Thank you for the review i addressed all your comments in v2. regarding the test actually, it's a bit hard to test that using a unit test because we need to let ovn-sb ignore the first binding request which is not applicable using a unit test, i was testing that manually by dropping the connection to the SB that will drop the first bind request but it a bit hard to do that i an unit test. On Fri, Feb 2, 2024 at 1:21 PM Ales Musil wrote: > > > On Fri, Feb 2, 2024 at 12:19 PM Ales Musil wrote: > >> >> >> On Tue, Jan 30, 2024 at 2:59 PM Mohammad Heib 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. add relevancy_time for each new bind_vport request and >>>mark this 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 the relevancy_time is still valid 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 >>> --- >>> >> >> Hi Mohammad, >> >> overall the change makes sense, I have a couple of comments see down >> below. >> >> controller/pinctrl.c | 58 +++- >>> 1 file changed, 52 insertions(+), 6 deletions(-) >>> >>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >>> index bd3bd3d81..152962448 100644 >>> --- a/controller/pinctrl.c >>> +++ b/controller/pinctrl.c >>> @@ -6519,10 +6519,52 @@ struct put_vport_binding { >>> uint32_t vport_key; >>> >>> uint32_t vport_parent_key; >>> + >>> +/* This vport record Only relevant if "relevancy_time" >>> + * is earlier than the current_time, "new_record" is true. >>> + */ >>> +long long int relevancy_time; >>> >> >> The intention of the variable should be probably clearer e.g. >> relevant_until_ms. >> >> Also reading through the rest of the code it doesn't seem possible that >> the binding wouldn't be deleted, hence I think there isn't any need for the >> relevancy time, it should be enough to have a flag that will be flipped. In >> any case we will try to commit twice. I would leave out the whole relevancy >> and keep the flag flipping it on first commit WDYT? >> >> >>> +bool new_record; >>> }; >>> >>> /* Contains "struct put_vport_binding"s. */ >>> static struct hmap put_vport_bindings; >>> +/* the relevance time in ms of vport record before deleteing. */ >>> +#define VPORT_RELEVANCE_TIME 1500 >>> + >>> +/* >>> + * 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: >>> + * 1. The put_vport_binding:relevancy_time still valid. >>> + * 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) >>> +{ >>> +long long int cur_time = time_msec(); >>> + >>> +if
Re: [ovs-dev] [PATCH ovn] ovn-controller: Stop dropping bind_vport requests immediately after handling.
On Fri, Feb 2, 2024 at 12:19 PM Ales Musil wrote: > > > On Tue, Jan 30, 2024 at 2:59 PM Mohammad Heib 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. add relevancy_time for each new bind_vport request and >>mark this 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 the relevancy_time is still valid 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 >> --- >> > > Hi Mohammad, > > overall the change makes sense, I have a couple of comments see down below. > > controller/pinctrl.c | 58 +++- >> 1 file changed, 52 insertions(+), 6 deletions(-) >> >> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >> index bd3bd3d81..152962448 100644 >> --- a/controller/pinctrl.c >> +++ b/controller/pinctrl.c >> @@ -6519,10 +6519,52 @@ struct put_vport_binding { >> uint32_t vport_key; >> >> uint32_t vport_parent_key; >> + >> +/* This vport record Only relevant if "relevancy_time" >> + * is earlier than the current_time, "new_record" is true. >> + */ >> +long long int relevancy_time; >> > > The intention of the variable should be probably clearer e.g. > relevant_until_ms. > > Also reading through the rest of the code it doesn't seem possible that > the binding wouldn't be deleted, hence I think there isn't any need for the > relevancy time, it should be enough to have a flag that will be flipped. In > any case we will try to commit twice. I would leave out the whole relevancy > and keep the flag flipping it on first commit WDYT? > > >> +bool new_record; >> }; >> >> /* Contains "struct put_vport_binding"s. */ >> static struct hmap put_vport_bindings; >> +/* the relevance time in ms of vport record before deleteing. */ >> +#define VPORT_RELEVANCE_TIME 1500 >> + >> +/* >> + * 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: >> + * 1. The put_vport_binding:relevancy_time still valid. >> + * 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) >> +{ >> +long long int cur_time = time_msec(); >> + >> +if (vpb->new_record && vpb->relevancy_time > cur_time) { >> +vpb->new_record = false; >> > > This is a nasty side effect that I wouldn't expect from starting with is_. > > >> +return true; >> +} >> +return false; >> +} >> >> static void >> init_put_vport_bindings(void) >> @@ -6531,18 +6573,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) { >> -
Re: [ovs-dev] [PATCH ovn] ovn-controller: Stop dropping bind_vport requests immediately after handling.
On Tue, Jan 30, 2024 at 2:59 PM Mohammad Heib 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. add relevancy_time for each new bind_vport request and >mark this 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 the relevancy_time is still valid 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 > --- > Hi Mohammad, overall the change makes sense, I have a couple of comments see down below. controller/pinctrl.c | 58 +++- > 1 file changed, 52 insertions(+), 6 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index bd3bd3d81..152962448 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -6519,10 +6519,52 @@ struct put_vport_binding { > uint32_t vport_key; > > uint32_t vport_parent_key; > + > +/* This vport record Only relevant if "relevancy_time" > + * is earlier than the current_time, "new_record" is true. > + */ > +long long int relevancy_time; > The intention of the variable should be probably clearer e.g. relevant_until_ms. Also reading through the rest of the code it doesn't seem possible that the binding wouldn't be deleted, hence I think there isn't any need for the relevancy time, it should be enough to have a flag that will be flipped. In any case we will try to commit twice. I would leave out the whole relevancy and keep the flag flipping it on first commit WDYT? > +bool new_record; > }; > > /* Contains "struct put_vport_binding"s. */ > static struct hmap put_vport_bindings; > +/* the relevance time in ms of vport record before deleteing. */ > +#define VPORT_RELEVANCE_TIME 1500 > + > +/* > + * 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: > + * 1. The put_vport_binding:relevancy_time still valid. > + * 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) > +{ > +long long int cur_time = time_msec(); > + > +if (vpb->new_record && vpb->relevancy_time > cur_time) { > +vpb->new_record = false; > This is a nasty side effect that I wouldn't expect from starting with is_. > +return true; > +} > +return false; > +} > > static void > init_put_vport_bindings(void) > @@ -6531,18 +6573,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, &vp
Re: [ovs-dev] [PATCH ovn] ovn-controller: Stop dropping bind_vport requests immediately after handling.
Bleep bloop. Greetings Mohammad Heib, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: The subject, ': ', is over 70 characters, i.e., 77. Subject: ovn-controller: Stop dropping bind_vport requests immediately after handling. Lines checked: 148, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev