Aleksei Oladko <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> --- 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
