Re: [Devel] [PATCH vz10 3/3] selftests: bonding: do not build on the namespaces of a killed run

[Konstantin Khorenko <khorenko@virtuozzo.com>]

> lag_setup2x2() creates lag_node1 and lag_node2 and then moves a veth
> into each with
> 
>   ip link set dev lag1 netns lag_node1 down name eth0
> 
> which asks for the move, the state and the rename in one request. The
> namespaces are removed by lag_cleanup() from an exit trap, so a run
> that is killed instead, by the timeout of the suite for one, leaves
> them behind. The next run then finds them: ip netns add fails, the
> veth is moved into a namespace that already has an eth0, the rename
> fails after the move has been committed, and the kernel says so:
> 
>   A link change request failed with some changes committed already.
>   Interface lag1 may have been left with an inconsistent configuration,
>   please check.
> 
> What is left is half a topology, and a bond built on it does not come
> back:
> 
>   TEST: mode (2) bond recovery                              [FAIL]
>   	Bond failed to recover
> 
> Only some of the cases fail, which is what makes it look like a timing
> problem. With lag_node1 and lag_node2 left over on purpose, one case
> of the seven in mode-2-recovery-updelay.sh fails; with them removed
> first, three runs of seven pass, and so does
> mode-1-recovery-updelay.sh, which uses the same library.

Measured numbers do not belong in a commit message, they are true only
on the machine they came from. "one case of the seven", "three runs of
seven" go to the ticket. Here it is enough to say that the failure is
intermittent and goes away once the leftover namespaces are removed.

Also the message describes one change, the namespace cleanup, while
the patch does two, see the slowwait hunk below.

> 
> https://virtuozzo.atlassian.net/browse/VSTOR-139651
> Feature: fix selftests
> Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
>
> diff --git a/tools/testing/selftests/drivers/net/bonding/lag_lib.sh b/tools/testing/selftests/drivers/net/bonding/lag_lib.sh
> index bf9bcd1b5ec04..7ac6c4d055eb2 100644
> --- a/tools/testing/selftests/drivers/net/bonding/lag_lib.sh
> +++ b/tools/testing/selftests/drivers/net/bonding/lag_lib.sh
> @@ -90,9 +90,18 @@ lag_setup2x2()
>  	local state=${1:-down}
>  	local namespaces="lag_node1 lag_node2"
>  
> -	# create namespaces
> +	# create namespaces.  A run of one of these tests that was killed,
> +	# by the timeout of the suite for one, does not get to its cleanup
> +	# and leaves these behind; the ip link set below then moves a veth
> +	# into a namespace that already has an eth0, fails on the rename it
> +	# is asked for in the same request, and leaves half a topology:
> +	#   A link change request failed with some changes committed already.
> +	#   Interface lag1 may have been left with an inconsistent
> +	#   configuration, please check.
> +	# Take what is left over before building on top of it.

This repeats the commit message. In a test one line is enough, e.g.

      # a killed run leaves the namespaces behind, start clean

Also double space after the period ("namespaces.  A run"), single space
please, here and below.

>  	for n in ${namespaces}; do
> -		ip netns add ${n}
> +		ip netns del ${n} 2>/dev/null
> +		ip netns add ${n} || return 1

The del before add is fine: the old veths go away with the namespaces
and the new pair is created in the root namespace and moved right away,
nothing collides.

The "|| return 1" does nothing though: lag_setup_network() does not look
at the return value and these scripts do not run under set -e. Either
drop it or make the caller act on it.

>  	done
>  
>  	# wire up namespaces
> @@ -167,8 +176,12 @@ test_bond_recovery()
>  	ip netns exec ${SWITCH} ip link set eth0 up
>  	ip netns exec ${SWITCH} ip link set eth1 down
>  
> -	# re-verify connectivity
> -	slowwait 2 ip netns exec ${CLIENT} ping ${SWITCHIP} -c 2 -W 0.1 &> /dev/null
> +	# re-verify connectivity.  With no member up the bond is supposed to
> +	# bring one up at once instead of waiting out updelay, so this must
> +	# stay below the smallest updelay the callers rely on, 5000ms, while
> +	# leaving enough room for a busy machine: two seconds is not enough
> +	# and the case with updelay 1000 fails now and then.
> +	slowwait 4 ip netns exec ${CLIENT} ping ${SWITCHIP} -c 2 -W 0.1 &> /dev/null

This is a separate change and it is not mentioned in the subject or the
body, only this comment explains it. Please split it into its own patch
with its own justification, or at least describe it in the message.

And the justification is what I am missing. bond_miimon_inspect()
ignores updelay as soon as there is no active member
(usable_slaves->count == 0 for xor, curr_active_slave == NULL for
active-backup), so once eth1 goes down eth0 is brought up on the next
miimon tick. With miimon 100 that is a few hundred milliseconds, not
seconds. If the updelay 1000 case fails at 2 seconds now and then,
either the box is heavily loaded or the kernel really waits somewhere,
and a longer timeout would hide that. How long did the recovery take in
the failed runs? Was that with the leftover namespaces already removed?

Side effect to keep in mind: at 4 seconds the updelay 2000 case stops
catching a bond that sits out its updelay, it would recover in ~2.1s
and pass. Only the 5000 and 10000 cases remain a real check after this.

The comment itself is again the commit message pasted into the test,
one line at most, e.g.

      # must stay below the smallest updelay the callers use

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel