On Fri, Feb 06, 2026 at 11:58:14PM +0100, Ilya Maximets wrote:
> On 2/5/26 6:03 PM, Adrian Moreno via dev wrote:
> > The current way of calculating "next_rebalance" when there is a change
> > in the bond rebalance period applies the current rebalance interval
> > (again) and subtracts the new one.
> >
> > This yields an unexpected new value for "next_rebalance": say the
> > current interval is 60s and we reduce it to 10s (and rebalancing has
> > just occurred), the current logic will set the next rebalancing in 110s
> > which is probably not what the user expects.
> >
> > Swap current and new rebalance intervals in the offset calculation
> > logic.
> >
> > Fixes: 9a2169a3f7bb ("bond: Do not flag a revalidation when adjusting
> > time.")
> > Reported-by: Minxi Hou <[email protected]>
> > Signed-off-by: Adrian Moreno <[email protected]>
> > ---
> > ofproto/bond.c | 4 ++--
> > tests/ofproto-dpif.at | 38 ++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/ofproto/bond.c b/ofproto/bond.c
> > index 584c38f07..ef481a360 100644
> > --- a/ofproto/bond.c
> > +++ b/ofproto/bond.c
> > @@ -468,9 +468,9 @@ bond_reconfigure(struct bond *bond, const struct
> > bond_settings *s)
> > * we can skip triggering the rest of the port
> > reconfiguration. */
> > if (bond->next_rebalance) {
> > long long int old_start_time =
> > - bond->next_rebalance - s->rebalance_interval;
> > + bond->next_rebalance - bond->rebalance_interval;
> > bond->next_rebalance =
> > - old_start_time + bond->rebalance_interval;
> > + old_start_time + s->rebalance_interval;
> > }
> > } else {
> > /* When the bond is doing a disable/enable of the rebalance
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index 15dbfd60c..6f198c06e 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -10551,6 +10551,44 @@
> > recirc_id(0),in_port(7),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:0b,dst=50:
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +AT_SETUP([ofproto-dpif - bond rebalance interval reconfiguration])
> > +OVS_VSWITCHD_START
> > +
> > +ovs-appctl time/stop
>
> We should wrap all the time managing commands into AT_CHECK.
> Old tests don't do that, but these commands are part of the
> test and it's better if they are in the test log.
>
> The warp commands will need [0], [ignore] at the end as they
> produce the 'warped' word.
OK, thanks.
>
> > +AT_CHECK([ovs-vsctl add-bond br0 bond0 p1 p2 bond_mode=balance-slb \
> > + other-config:bond-rebalance-interval=60000 -- \
> > + set interface p1 type=dummy ofport_request=1 -- \
> > + set interface p2 type=dummy ofport_request=2])
> > +
> > +# TEST_NEXT_REBALANCE([num])
> > +# Test the next rebalance value is equal to a number.
> > +m4_define([TEST_NEXT_REBALANCE_EQ], [
> > +rebalance=$(ovs-appctl bond/show bond0 | sed -n 's/^next rebalance:
> > \([[0-9-]]*\) ms$/\1/p')
> > +test "$rebalance" -eq "$1"
> > +])
> > +
> > +OVS_WAIT_UNTIL([TEST_NEXT_REBALANCE_EQ([60000])])
>
> Do we need to wait? Rebalancing and the configuration updates
> are the job of a main thread. ovs-vsctl always waits for the
> ovs-vswitchd to reply and every time warp appctl triggers the
> main loop run. We can also do multiple loop runs by using the
> staged warp, if needed.
>
> And if we don't need to wait, then we can use normal AT_CHECK
> stuff inside the macro instead as falling back to pure shell.
> E.g.:
>
> m4_define([TEST_NEXT_REBALANCE_EQ], [
> AT_CHECK([ovs-appctl bond/show bond0 | grep 'next rebalance:'], [0],
> [stdout])
> AT_CHECK([cat stdout], [0], [next rebalance: $1 ms
> ])])
>
> This way all the commands also end up in the log.
>
> WDYT?
I have had several timing issues and I default to always use WAIT_UNTIL
but you're right it would improve debuggability in this case. I'll give
it a try. Thanks.
>
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
