Re: [PATCH] selftests: net: forwarding: cleanup veth peers created via NUM_NETIFS

[Petr Machata <petrm@nvidia.com>]

Aleksei Oladko <aleksey.oladko@virtuozzo.com> writes:

> Some net/forwarding kselftests set NUM_NETIFS, causing lib.sh to create
> the requested number of veth peer interfaces.
>
> These interfaces are not removed when the tests finish, leaving stale
> veth devices in the system. This can cause subsequent tests to fail,
> for example min_max_mtu.sh.

What in particular makes it fail? I tried a bridge_vlan_unaware.sh +
min_max_mtu.sh combination at random, and that seems to work fine.

> Ensure that veth peers created via NUM_NETIFS are properly removed at
> the end of the tests to avoid interference between test runs.
>
> Signed-off-by: Aleksei Oladko <aleksey.oladko@virtuozzo.com>
> ---
>  tools/testing/selftests/net/forwarding/lib.sh | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index a9034f0bb58b..ae7699c0c7e5 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -597,6 +597,10 @@ vrf_cleanup()
>  	ip -6 rule del pref 32765
>  	ip -4 rule add pref 0 table local
>  	ip -4 rule del pref 32765
> +
> +	for ((i = 1; i <= NUM_NETIFS; i=$i+2)); do
> +		ip link delete dev ${NETIFS[p$i]} 2>/dev/null || true
> +	done

This would also erase interfaces that existed before the test was run,
which seems worse than leaving garbage behind, which I agree is not
ideal either. Although yeah, existing tests create all sorts of ad-hoc
interfaces assuming the namespace is available, and then presumably
deleting them. It's all around fairly hacky.

Nevertheless, I think if the test should erase what it created, then it
needs to also track what it created. Then the 2>/dev/null || true
business can go away, because it's supposed to work.

Hmm, that's basically reinventing defer. But we can't use defer, because
few tests are defer-enabled. Or maybe...?


Subject: selftests: lib: Delete created veth pairs after the test

Currently the created veth pairs are not removed after the test finishes.
Ideally we would just use defer to selectively delete exactly what the test
created, but most tests are not defer ready in that they use the EXIT trap
for their own cleanup management.

So instead invoke the cleanup from vrf_cleanup(). Almost all tests use VRFs
and typically call vrf_prepare() first, which makes vrf_cleanup() a good
place to do it.

Defer-enabled tests however schedule vrf_cleanup() by way of
adf_vrf_prepare(). Invoking scope cleanup from within scope cleanup is
obviously not a great idea, so instead add a __-prefixed helper to do the
actual work, have it scheduled by adf_vrf_prepare(), and only invoke the
scope cleanup from a sans-__ wrapper.

This is all admittedly a bit of a hack, but perhaps less so than
handrolling a list of interfaces to delete, when we have a perfectly good
library for exactly that purpose. (And the hand-rolled cleanup would end up
being invoked from vrf_cleanup() anyway.)

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 tools/testing/selftests/net/forwarding/lib.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index a9034f0bb58b..729d6bedf97b 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -392,6 +392,7 @@ create_netif_veth()
 				echo "Failed to create netif"
 				exit 1
 			fi
+			defer ip link del "${NETIFS[p$i]}"
 		fi
 		i=$j
 	done
@@ -591,7 +592,7 @@ vrf_prepare()
 	ip -6 rule del pref 0
 }
 
-vrf_cleanup()
+__vrf_cleanup()
 {
 	ip -6 rule add pref 0 table local
 	ip -6 rule del pref 32765
@@ -599,10 +600,16 @@ vrf_cleanup()
 	ip -4 rule del pref 32765
 }
 
+vrf_cleanup()
+{
+	__vrf_cleanup
+	defer_scopes_cleanup
+}
+
 adf_vrf_prepare()
 {
 	vrf_prepare
-	defer vrf_cleanup
+	defer __vrf_cleanup
 }
 
 __last_tb_id=0


Like your patch, this doesn't fix tests that don't use vrf_cleanup().
There's at least bridge_sticky_fdb.sh, but there might be more in
driver-specific directories, I didn't look. It should be pretty easy to
deferify those tests FWIW, the gist of it is:


diff --git a/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh b/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh
index 1f8ef0eff862..7d51676c886e 100755
--- a/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh
@@ -40,7 +40,7 @@ setup_prepare()
 	switch_create
 }
 
-cleanup()
+setup_cleanup()
 {
 	pre_cleanup
 	switch_destroy
@@ -62,6 +62,8 @@ sticky()
 trap cleanup EXIT
 
 setup_prepare
+defer setup_cleanup
+
 setup_wait
 
 tests_run