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. > +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. > +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
