Re: [Devel] [PATCH vz10 4/4] selftests: drv-net-hw: skip the tests that need a real interface

[Konstantin Khorenko <khorenko@virtuozzo.com>]

> ethtool, ethtool_extended_state, hw_stats_l3 and hw_stats_l3_gre need a
> device that can set link speed and autonegotiation, report an extended
> link state, or offload L3 statistics. Where none is configured, lib.sh
> creates a veth pair instead, which supports none of that, so every case
> fails on something the kernel correctly refuses.
> 
> Upstream answers this by keeping such tests in drivers/net/hw and not
> running them on veth at all: commit 0c499a351777 ("selftests:
> forwarding: Ditch skip_on_veth()") removed the equivalent check once the
> directory was split out. This guard is therefore not for upstream, it
> only stops a run that substitutes veth pairs from reporting failures.
> 
> Skip before sourcing lib.sh, because lib.sh makes the veth pair as it is
> sourced. Exiting afterwards leaves the devices behind - vrf_cleanup()
> removes them and a test that skips never gets there - and a later test
> that adds a veth by one of those names then cannot. hw_stats_l3_gre
> asks for six interfaces, so one run left veth0 to veth5 in the initial
> namespace and the netfilter suite failed on it.
> 
> 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/hw/ethtool.sh b/tools/testing/selftests/drivers/net/hw/ethtool.sh
> index fa6953de6b6dd..b1744758627c2 100755
> --- a/tools/testing/selftests/drivers/net/hw/ethtool.sh
> +++ b/tools/testing/selftests/drivers/net/hw/ethtool.sh
> @@ -11,6 +11,22 @@ ALL_TESTS="
>  "
>  NUM_NETIFS=2
>  lib_dir=$(dirname "$0")
> +
> +# The test needs a device capable of setting link speed and
> +# autonegotiation. Where no real interfaces are configured, lib.sh makes
> +# a veth pair instead, which cannot, and every case then fails on
> +# something the kernel correctly refuses. Check this before sourcing
> +# lib.sh: it creates that pair as it is sourced, so exiting afterwards
> +# leaves the devices behind, and the next test that wants one of those
> +# names cannot create it.

This is the commit message pasted into the test, four times. The
rationale belongs in the commit message only. In the code one line is
enough, something like:


> +
> +: "${NETIF_CREATE:=yes}"
> +: "${NETIF_TYPE:=veth}"
> +if [[ "$NETIF_CREATE" = yes && "$NETIF_TYPE" = veth ]]; then
> +	echo "SKIP: needs a real interface, set NETIFS in forwarding.config"
> +	exit 4 # ksft_skip, lib.sh is not sourced yet
> +fi
> +
>  source "$lib_dir"/../../../net/forwarding/lib.sh
>  source ethtool_lib.sh
>  
> diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
> index a7584448416ea..c6c96117b1f12 100755
> --- a/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
> +++ b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
> @@ -9,6 +9,22 @@ ALL_TESTS="
>  
>  NUM_NETIFS=2
>  lib_dir=$(dirname "$0")
> +
> +# The test needs a device capable of reporting an extended link state.
> +# Where no real interfaces are configured, lib.sh makes a veth pair
> +# instead, which cannot, and every case then fails on something the
> +# kernel correctly refuses. Check this before sourcing lib.sh: it
> +# creates that pair as it is sourced, so exiting afterwards leaves the
> +# devices behind, and the next test that wants one of those names cannot
> +# create it.
> +
> +: "${NETIF_CREATE:=yes}"
> +: "${NETIF_TYPE:=veth}"
> +if [[ "$NETIF_CREATE" = yes && "$NETIF_TYPE" = veth ]]; then
> +	echo "SKIP: needs a real interface, set NETIFS in forwarding.config"
> +	exit 4 # ksft_skip, lib.sh is not sourced yet
> +fi
> +
>  source "$lib_dir"/../../../net/forwarding/lib.sh
>  source ethtool_lib.sh
>  
> diff --git a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
> index 67fafefc80bea..1776a4954601b 100755
> --- a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
> +++ b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
> @@ -49,6 +49,22 @@ ALL_TESTS="
>  "
>  NUM_NETIFS=4
>  lib_dir=$(dirname "$0")
> +
> +# The test needs a device capable of offloading L3 statistics. Where no
> +# real interfaces are configured, lib.sh makes a veth pair instead,
> +# which cannot, and every case then fails on something the kernel
> +# correctly refuses. Check this before sourcing lib.sh: it creates that
> +# pair as it is sourced, so exiting afterwards leaves the devices
> +# behind, and the next test that wants one of those names cannot create
> +# it.
> +
> +: "${NETIF_CREATE:=yes}"
> +: "${NETIF_TYPE:=veth}"
> +if [[ "$NETIF_CREATE" = yes && "$NETIF_TYPE" = veth ]]; then
> +	echo "SKIP: needs a real interface, set NETIFS in forwarding.config"
> +	exit 4 # ksft_skip, lib.sh is not sourced yet
> +fi
> +

The check does not see forwarding.config. lib.sh sets these defaults
(lib.sh:54-55), then sources net/forwarding/forwarding.config
(lib.sh:126), and only then creates the interfaces (lib.sh:433). Here
lib.sh has not been sourced yet, so the config file has not been read
and the two variables fall back to yes/veth unless they are exported in
the environment.

So a user who does what the SKIP line tells them to, puts real NETIFS
into forwarding.config and sets NETIF_CREATE=no there, still gets the
skip: on real hardware all four tests never run. Only an exported
NETIF_CREATE=no would get past it, and forwarding.config is not
sourced into the environment anywhere.

Read the same file lib.sh reads before deciding:

forwarding_dir=$lib_dir/../../../net/forwarding
[[ -f $forwarding_dir/forwarding.config ]] && \
      source "$forwarding_dir/forwarding.config"
: "${NETIF_CREATE:=yes}"
: "${NETIF_TYPE:=veth}"

Since the same block goes into four files, put it into a small helper
next to ethtool_lib.sh and source that before lib.sh, instead of
copying it.

>  source "$lib_dir"/../../../net/forwarding/lib.sh
>  source "$lib_dir"/../../../net/forwarding/tc_common.sh
>  

Two more notes on the set of tests:

ethtool_rmon.sh in the same directory reads rmon histogram counters via
"ethtool -S --groups rmon" and has no skip of its own, so on veth it
fails the same way. If it fails in your runs too, it belongs in this
patch. ethtool_mm.sh and loopback.sh already skip by themselves.

The reference to 0c499a351777 is correct and the "why not upstream"
paragraph is useful, keep it.

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