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.
> +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?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
