[PATCH net] selftests: bonding: fix failover race in bond_options.sh prio test

[PATCH net] selftests: bonding: fix failover race in bond_options.sh prio test

[Pin-yen Lin]

The prio test intermittently fails in the arp_ip_target and
ns_ip6_target variants:

  TEST: prio (active-backup ns_ip6_target primary_reselect 1)   [FAIL]
  	Current active slave is eth0 but not eth2

The ARP/NS monitor marks a slave down when it has received nothing for
too long. With arp_interval 100 and the default arp_missed_max, that is
about 250ms for the active slave (eth1) and 350ms for a backup slave
(eth0, eth2). Every 100ms, eth1 sends a probe: eth1 receives the reply,
and eth0/eth2 receive a copy forwarded by the gateway bridge. Once eth1
is down, its probes reach no one and no slave receives anything, so
eth1 normally times out first and the new active slave is picked by
prio:

  t=0    probe: eth1 gets the reply, eth0/eth2 get a copy
  t=10   test sets eth1 down
  t=300  monitor: eth1 idle 300ms -> down
                  eth0/eth2 idle 300ms -> up, eth2 picked by prio

But the test takes eth1 down right after bond_check_connection(), whose
ping replies only arrive on eth1:

  t=0    probe: eth1 gets the reply, eth0/eth2 get a copy
  t=70   ping reply on eth1 only
  t=85   test sets eth1 down
  t=300  monitor: eth1 idle 230ms -> still up
  t=400  monitor: eth1 idle 330ms -> down
                  eth2 idle 400ms -> down too

eth2 can then no longer be picked. eth0 becomes the active slave if it
happened to receive some other packet (e.g. an MLD report), otherwise
whichever slave comes back first does. With primary_reselect 1 or 2,
the bond keeps eth0 even after eth2 comes back.

Before taking the active slave down, wait two monitor intervals
(200ms), so that at least one more probe is sent after the last ping
reply and all slaves last received that probe. The miimon variants do
not need the wait, but the extra delay is negligible.

Hit in our CI on GCE VMs (4 of 51 runs); the test only uses veth. On
the same VMs, 7 of 30 loops of the prio tests failed without this
change, and 20 of 20 loops passed with it.

Fixes: 481b56e0391e ("selftests: bonding: re-format bond option tests")
Signed-off-by: Pin-yen Lin <treapking@google.com>
---

 .../drivers/net/bonding/bond_options.sh       | 26 ++++++++++++++++---
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/bonding/bond_options.sh b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
index 187b478d0ddf..16c1c2d97005 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond_options.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
@@ -13,6 +13,24 @@ ALL_TESTS="
 
 lib_dir=$(dirname "$0")
 source ${lib_dir}/bond_topo_3d1c.sh
+
+# The ARP/NS monitor marks a slave down when it has received nothing for too
+# long: ~250ms for the active slave and ~350ms for a backup slave with
+# arp_interval 100. The backups only receive the copies of the active slave's
+# probes that the gateway bridge forwards, but the active slave also gets the
+# replies to the pings in bond_check_connection(). If the active slave goes
+# down right after such a reply, it times out in the same monitor run as the
+# backups, and the new active slave is then not picked by prio. Wait two
+# monitor intervals, so that at least one more probe is sent and all slaves
+# last received that probe.
+bond_link_down()
+{
+	local slave=$1
+
+	sleep 0.2
+	ip -n ${s_ns} link set "$slave" down
+}
+
 c_maddr="33:33:ff:00:00:10"
 g_maddr="33:33:ff:00:02:54"
 
@@ -97,12 +115,12 @@ prio_test()
 	check_active_slave eth1
 
 	# active slave should be the higher prio slave
-	ip -n ${s_ns} link set $active_slave down
+	bond_link_down $active_slave
 	check_active_slave eth2
 	bond_check_connection "fail over"
 
 	# when only 1 slave is up
-	ip -n ${s_ns} link set $active_slave down
+	bond_link_down $active_slave
 	check_active_slave eth0
 	bond_check_connection "only 1 slave up"
 
@@ -134,7 +152,7 @@ prio_test()
 			;;
 		"2")
 			check_active_slave "$pre_active_slave"
-			ip -n ${s_ns} link set $active_slave down
+			bond_link_down $active_slave
 			bond_check_connection "pre_active slave down"
 			check_active_slave "eth1"
 			;;
@@ -153,7 +171,7 @@ prio_test()
 		check_err $? "eth3 prio is not -50"
 		check_active_slave "eth1"
 
-		ip -n ${s_ns} link set $active_slave down
+		bond_link_down $active_slave
 		check_active_slave "eth0"
 		bond_check_connection "change slave prio"
 	fi
-- 
2.56.1.703.gf848ed807c-goog

base-commit: 665b4149e91b781a71bce8637c3a4d7d64dba05f

Re: [PATCH net] selftests: bonding: fix failover race in bond_options.sh prio test

[Xia Yan]

On 11/12/26 01:49, Pin-yen Lin wrote:
> The prio test intermittently fails in the arp_ip_target and
> ns_ip6_target variants:
>
>   TEST: prio (active-backup ns_ip6_target primary_reselect 1)   [FAIL]
>   	Current active slave is eth0 but not eth2

I traced the timing through bond_ab_arp_inspect(). With
arp_interval 100 and the default arp_missed_max of 2, the active
slave is marked down after 200ms without a receive and the backups
after 300ms, matching the timing analysis in the commit message.

All four link-down sites in prio_test() use the new helper, and
the Fixes tag is right.

Reviewed-by: Xia Yan <yanxia@cmss.chinamobile.com>