On Fri, Feb 06, 2026 at 08:20:50AM -0500, Aaron Conole wrote: > Adrian Moreno via dev <[email protected]> writes: > > > 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]> > > --- > > Just some minor stuff below. Sorry for the miss here. > > Acked-by: Aaron Conole <[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 > > Should we be doing a time/stop this early? Not that I think it's a > problem, but just feels strange to time/stop before we even add the bond. >
If we do: AT_CHECK([ovs-vsctl add-bond...]) AT_CHECK([ovs-appctl time/stop]) TEST_NEXT_REBALANCE([60000]) The last test might fail becasue the main thread wight have run one or more times decreasing the rebalance time in a few ms. This is a way to make sure the initial rebalance time is the one we configure. If you think it's error-prone I can remove it, but then we'll have to extract the current rebalance time which will likely be 599XX ms and use it to offset the next time/wrap call. > > +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])]) > > +ovs-appctl time/warp 50000 > > +OVS_WAIT_UNTIL([TEST_NEXT_REBALANCE_EQ([10000])]) > > + > > +# Decreasing the rebalance interval to something lower than the > > +# already-consumed interval should make the next rebalance value negative, > > +# i.e: balance will happen ASAP. > > +AT_CHECK([ovs-vsctl set Port bond0 > > other-config:bond-rebalance-interval=10000]) > > +OVS_WAIT_UNTIL([TEST_NEXT_REBALANCE_EQ([-40000])]) > > BTW, maybe it's just me, but it does feel a bit jarring to see a > negative rebalance interval. > It might feel weird for users, I know. Note the actual value is likely positive, only "time_ms()" is subtracted form it only for displaying an offset instead of a huge number. The value of "next_rebalance" can fall behind "time_ms()" for many reasons and it just indicates the rebalance is overdue, which I guess is valuable information to know, e.g: it could point to the main thread being slow for some reason. Adrián > > +ovs-appctl time/warp 1 > > +OVS_WAIT_UNTIL([TEST_NEXT_REBALANCE_EQ([10000])]) > > + > > +ovs-appctl time/warp 5000 > > +OVS_WAIT_UNTIL([TEST_NEXT_REBALANCE_EQ([5000])]) > > + > > +# Increasing the rebalance interval should offset the already consumed > > +# interval. > > +AT_CHECK([ovs-vsctl set Port bond0 > > other-config:bond-rebalance-interval=60000]) > > +OVS_WAIT_UNTIL([TEST_NEXT_REBALANCE_EQ([55000])]) > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > + > > AT_SETUP([ofproto-dpif megaflow - resubmit port action]) > > OVS_VSWITCHD_START > > AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
