| Message ID | 20240917193516.594830-1-mkp@redhat.com |
|---|---|
| State | Accepted |
| Commit | 2d14266cb9426436449e5b222d078661c0f87749 |
| Delegated to: | Kevin Traynor |
| Headers | show |
| Series | [ovs-dev,branch-3.3,v1] ofproto-dpif-mirror: Always revalidate on mirror update. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 17 Sep 2024, at 21:35, Mike Pattrick wrote: > Previously updating mirror settings would not trigger a revalidation, > this could result in impactful changes to mirrors taking a long time to > take effect. > > This change sets need_revalidate whenever a setting is successfully set > on a mirror. > > Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.") > Reported-at: https://issues.redhat.com/browse/FDP-788 > Tested-by: Kevin Traynor <ktraynor@redhat.com> > Acked-by: Kevin Traynor <ktraynor@redhat.com> > Signed-off-by: Mike Pattrick <mkp@redhat.com> Change looks good to me. Should we backport this all the way down to 2.17? Cheers, Eelco > --- > ofproto/ofproto-dpif-mirror.c | 2 +- > ofproto/ofproto-dpif.c | 10 ++++++++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c > index 343b75f0e..45024580a 100644 > --- a/ofproto/ofproto-dpif-mirror.c > +++ b/ofproto/ofproto-dpif-mirror.c > @@ -265,7 +265,7 @@ mirror_set(struct mbridge *mbridge, void *aux, const char *name, > { > hmapx_destroy(&srcs_map); > hmapx_destroy(&dsts_map); > - return 0; > + return ECANCELED; > } > > /* XXX: Not sure if these need to be thread safe. */ > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index fe034f971..7e300c3f9 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -3669,6 +3669,16 @@ mirror_set__(struct ofproto *ofproto_, void *aux, > s->n_dsts, s->src_vlans, > bundle_lookup(ofproto, s->out_bundle), > s->snaplen, s->out_vlan); > + > + if (!error) { > + ofproto->backer->need_revalidate = REV_RECONFIGURE; > + } else if (error == ECANCELED) { > + /* The user requested a change that is identical to the current state, > + * the reconfiguration is canceled, but don't log an error message > + * about that. */ > + error = 0; > + } > + > free(srcs); > free(dsts); > return error; > -- > 2.43.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 20 Sep 2024, at 8:58, Eelco Chaudron wrote: > On 17 Sep 2024, at 21:35, Mike Pattrick wrote: > >> Previously updating mirror settings would not trigger a revalidation, >> this could result in impactful changes to mirrors taking a long time to >> take effect. >> >> This change sets need_revalidate whenever a setting is successfully set >> on a mirror. >> >> Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.") >> Reported-at: https://issues.redhat.com/browse/FDP-788 >> Tested-by: Kevin Traynor <ktraynor@redhat.com> >> Acked-by: Kevin Traynor <ktraynor@redhat.com> >> Signed-off-by: Mike Pattrick <mkp@redhat.com> > > Change looks good to me. Should we backport this all the way down to 2.17? > > Cheers, > > Eelco Forgot to add my ACK :( Acked-by: Eelco Chaudron <echaudro@redhat.com> > >> --- >> ofproto/ofproto-dpif-mirror.c | 2 +- >> ofproto/ofproto-dpif.c | 10 ++++++++++ >> 2 files changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c >> index 343b75f0e..45024580a 100644 >> --- a/ofproto/ofproto-dpif-mirror.c >> +++ b/ofproto/ofproto-dpif-mirror.c >> @@ -265,7 +265,7 @@ mirror_set(struct mbridge *mbridge, void *aux, const char *name, >> { >> hmapx_destroy(&srcs_map); >> hmapx_destroy(&dsts_map); >> - return 0; >> + return ECANCELED; >> } >> >> /* XXX: Not sure if these need to be thread safe. */ >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index fe034f971..7e300c3f9 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -3669,6 +3669,16 @@ mirror_set__(struct ofproto *ofproto_, void *aux, >> s->n_dsts, s->src_vlans, >> bundle_lookup(ofproto, s->out_bundle), >> s->snaplen, s->out_vlan); >> + >> + if (!error) { >> + ofproto->backer->need_revalidate = REV_RECONFIGURE; >> + } else if (error == ECANCELED) { >> + /* The user requested a change that is identical to the current state, >> + * the reconfiguration is canceled, but don't log an error message >> + * about that. */ >> + error = 0; >> + } >> + >> free(srcs); >> free(dsts); >> return error; >> -- >> 2.43.5 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 20/09/2024 12:40, Eelco Chaudron wrote: > > On 20 Sep 2024, at 8:58, Eelco Chaudron wrote: > >> On 17 Sep 2024, at 21:35, Mike Pattrick wrote: >> >>> Previously updating mirror settings would not trigger a revalidation, >>> this could result in impactful changes to mirrors taking a long time to >>> take effect. >>> >>> This change sets need_revalidate whenever a setting is successfully set >>> on a mirror. >>> >>> Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.") >>> Reported-at: https://issues.redhat.com/browse/FDP-788 >>> Tested-by: Kevin Traynor <ktraynor@redhat.com> >>> Acked-by: Kevin Traynor <ktraynor@redhat.com> >>> Signed-off-by: Mike Pattrick <mkp@redhat.com> >> Change looks good to me. Should we backport this all the way down to 2.17? >> >> Cheers, >> >> Eelco > Forgot to add my ACK :( > > Acked-by: Eelco Chaudron <echaudro@redhat.com> Thanks Mike and Eelco. Pushed to branch-3.3 and down as far as branch-2.17.
diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c index 343b75f0e..45024580a 100644 --- a/ofproto/ofproto-dpif-mirror.c +++ b/ofproto/ofproto-dpif-mirror.c @@ -265,7 +265,7 @@ mirror_set(struct mbridge *mbridge, void *aux, const char *name, { hmapx_destroy(&srcs_map); hmapx_destroy(&dsts_map); - return 0; + return ECANCELED; } /* XXX: Not sure if these need to be thread safe. */ diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index fe034f971..7e300c3f9 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3669,6 +3669,16 @@ mirror_set__(struct ofproto *ofproto_, void *aux, s->n_dsts, s->src_vlans, bundle_lookup(ofproto, s->out_bundle), s->snaplen, s->out_vlan); + + if (!error) { + ofproto->backer->need_revalidate = REV_RECONFIGURE; + } else if (error == ECANCELED) { + /* The user requested a change that is identical to the current state, + * the reconfiguration is canceled, but don't log an error message + * about that. */ + error = 0; + } + free(srcs); free(dsts); return error;
Previously updating mirror settings would not trigger a revalidation, this could result in impactful changes to mirrors taking a long time to take effect. This change sets need_revalidate whenever a setting is successfully set on a mirror. Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.") Reported-at: https://issues.redhat.com/browse/FDP-788 Tested-by: Kevin Traynor <ktraynor@redhat.com> Acked-by: Kevin Traynor <ktraynor@redhat.com> Signed-off-by: Mike Pattrick <mkp@redhat.com> --- ofproto/ofproto-dpif-mirror.c | 2 +- ofproto/ofproto-dpif.c | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-)