[PATCH net-next] selftests: pmtu: Simplify cleanup and namespace names

[PATCH net-next] selftests: pmtu: Simplify cleanup and namespace names

[David Ahern]

From: David Ahern <dsahern@gmail.com>

The point of the pause-on-fail argument is to leave the setup as is after
a test fails to allow a user to debug why it failed. Move the cleanup
after posting the result to the user to make it so.

Random names for the namespaces are not user friendly when trying to
debug a failure. Make them simpler and more direct for the tests. Run
cleanup at the beginning to ensure they are cleaned up if they already
exist.

Remove cleanup_done. There is no harm in doing cleanup twice; just
ignore any errors related to not existing - which is already done.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 tools/testing/selftests/net/pmtu.sh | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
index b9171a7b3aaa..ab77e6344d17 100755
--- a/tools/testing/selftests/net/pmtu.sh
+++ b/tools/testing/selftests/net/pmtu.sh
@@ -152,10 +152,10 @@ tests="
 	cleanup_ipv4_exception		ipv4: cleanup of cached exceptions
 	cleanup_ipv6_exception		ipv6: cleanup of cached exceptions"
 
-NS_A="ns-$(mktemp -u XXXXXX)"
-NS_B="ns-$(mktemp -u XXXXXX)"
-NS_R1="ns-$(mktemp -u XXXXXX)"
-NS_R2="ns-$(mktemp -u XXXXXX)"
+NS_A="ns-A"
+NS_B="ns-B"
+NS_R1="ns-R1"
+NS_R2="ns-R2"
 ns_a="ip netns exec ${NS_A}"
 ns_b="ip netns exec ${NS_B}"
 ns_r1="ip netns exec ${NS_R1}"
@@ -212,7 +212,6 @@ dummy6_0_addr="fc00:1000::0"
 dummy6_1_addr="fc00:1001::0"
 dummy6_mask="64"
 
-cleanup_done=1
 err_buf=
 tcpdump_pids=
 
@@ -495,7 +494,7 @@ setup_routing() {
 setup() {
 	[ "$(id -u)" -ne 0 ] && echo "  need to run as root" && return $ksft_skip
 
-	cleanup_done=0
+	cleanup
 	for arg do
 		eval setup_${arg} || { echo "  ${arg} not supported"; return 1; }
 	done
@@ -519,11 +518,9 @@ cleanup() {
 	done
 	tcpdump_pids=
 
-	[ ${cleanup_done} -eq 1 ] && return
 	for n in ${NS_A} ${NS_B} ${NS_R1} ${NS_R2}; do
 		ip netns del ${n} 2> /dev/null
 	done
-	cleanup_done=1
 }
 
 mtu() {
@@ -1136,6 +1133,9 @@ done
 
 trap cleanup EXIT
 
+# start clean
+cleanup
+
 for t in ${tests}; do
 	[ $desc -eq 0 ] && name="${t}" && desc=1 && continue || desc=0
 
@@ -1156,7 +1156,6 @@ for t in ${tests}; do
 
 		eval test_${name}
 		ret=$?
-		cleanup
 
 		if [ $ret -eq 0 ]; then
 			printf "TEST: %-60s  [ OK ]\n" "${t}"
-- 
2.11.0

Re: [PATCH net-next] selftests: pmtu: Simplify cleanup and namespace names

[David Miller]

From: David Ahern <dsahern@kernel.org>
Date: Wed, 22 May 2019 12:11:06 -0700

> From: David Ahern <dsahern@gmail.com>
> 
> The point of the pause-on-fail argument is to leave the setup as is after
> a test fails to allow a user to debug why it failed. Move the cleanup
> after posting the result to the user to make it so.
> 
> Random names for the namespaces are not user friendly when trying to
> debug a failure. Make them simpler and more direct for the tests. Run
> cleanup at the beginning to ensure they are cleaned up if they already
> exist.
> 
> Remove cleanup_done. There is no harm in doing cleanup twice; just
> ignore any errors related to not existing - which is already done.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

Applied.

Re: [PATCH net-next] selftests: pmtu: Simplify cleanup and namespace names

[Stefano Brivio]

Hi David,

On Wed, 22 May 2019 12:11:06 -0700
David Ahern <dsahern@kernel.org> wrote:

> From: David Ahern <dsahern@gmail.com>
> 
> The point of the pause-on-fail argument is to leave the setup as is after
> a test fails to allow a user to debug why it failed. Move the cleanup
> after posting the result to the user to make it so.
> 
> Random names for the namespaces are not user friendly when trying to
> debug a failure. Make them simpler and more direct for the tests. Run
> cleanup at the beginning to ensure they are cleaned up if they already
> exist.

The reasons for picking per-instance unique names were:

- one can run multiple instances of the script in parallel. I
  couldn't trigger any bug this way *so far*, though

- cleanup might fail because of e.g. device reference count leaks (this
  happened quite frequently in the past), which are anyway visible in
  kernel logs. Unique names avoid the need to reboot

Sure, it's a trade-off with usability, and I also see the value of
having fixed names, so I'm fine with this too. I just wanted to make
sure you considered these points.

By the way, the comment to nsname() (that I would keep, it's still
somewhat convenient) is now inconsistent.

-- 
Stefano

Re: [PATCH net-next] selftests: pmtu: Simplify cleanup and namespace names

[David Ahern]

On 5/23/19 1:58 AM, Stefano Brivio wrote:
> Hi David,
> 
> On Wed, 22 May 2019 12:11:06 -0700
> David Ahern <dsahern@kernel.org> wrote:
> 
>> From: David Ahern <dsahern@gmail.com>
>>
>> The point of the pause-on-fail argument is to leave the setup as is after
>> a test fails to allow a user to debug why it failed. Move the cleanup
>> after posting the result to the user to make it so.
>>
>> Random names for the namespaces are not user friendly when trying to
>> debug a failure. Make them simpler and more direct for the tests. Run
>> cleanup at the beginning to ensure they are cleaned up if they already
>> exist.
> 
> The reasons for picking per-instance unique names were:
> 
> - one can run multiple instances of the script in parallel. I
>   couldn't trigger any bug this way *so far*, though
> 
> - cleanup might fail because of e.g. device reference count leaks (this
>   happened quite frequently in the past), which are anyway visible in
>   kernel logs. Unique names avoid the need to reboot
> 
> Sure, it's a trade-off with usability, and I also see the value of
> having fixed names, so I'm fine with this too. I just wanted to make
> sure you considered these points.
> 
> By the way, the comment to nsname() (that I would keep, it's still
> somewhat convenient) is now inconsistent.
> 

I have been using the namespace override for a while now. I did consider
impacts to the above, but my thinking is this: exceptions are per FIB
entry (per fib6_nh with my latest patch set, but point still holds), FIB
entries are per FIB table, FIB tables are per network namespace. Running
multiple pmtu.sh sessions in parallel can not trigger an interdependent
bug because of that separation. The cleanup within a namespace teardown
(reference count leaks) should not be affected.

Now that we have good set of functional tests, we do need more complex
tests but those will still be contained within the namespace separation.
If you look at my current patch set on the list I add an icmp_redirect
test script. It actually does redirect, verify, mtu on top of redirect,
verify and then resets and inverts the order - going after an exception
entry with an update for both use cases.

For the pmtu script, perhaps the next step is something as simple as
configuring the setup and routing once and then run all of the
individual tests (or multiple of them) to generate multiple exceptions
within a single FIB table and then tests to generate multiple exceptions
with different addresses per entry.

Re: [PATCH net-next] selftests: pmtu: Simplify cleanup and namespace names

[Stefano Brivio]

On Thu, 23 May 2019 09:41:59 -0600
David Ahern <dsahern@gmail.com> wrote:

> I have been using the namespace override for a while now. I did consider
> impacts to the above, but my thinking is this: exceptions are per FIB
> entry (per fib6_nh with my latest patch set, but point still holds), FIB
> entries are per FIB table, FIB tables are per network namespace. Running
> multiple pmtu.sh sessions in parallel can not trigger an interdependent
> bug because of that separation. The cleanup within a namespace teardown
> (reference count leaks) should not be affected.

I see, I guess it makes sense.

> Now that we have good set of functional tests, we do need more complex
> tests but those will still be contained within the namespace separation.
> If you look at my current patch set on the list I add an icmp_redirect
> test script. It actually does redirect, verify, mtu on top of redirect,
> verify and then resets and inverts the order - going after an exception
> entry with an update for both use cases.
> 
> For the pmtu script, perhaps the next step is something as simple as
> configuring the setup and routing once and then run all of the
> individual tests (or multiple of them) to generate multiple exceptions
> within a single FIB table and then tests to generate multiple exceptions
> with different addresses per entry.

I think, especially given your new icmp_redirect test script, that
another sensible next step would be turning the setup part in pmtu.sh
into some kind of library (also including the VRF setup) that could be
sourced from both scripts. Right now, that looks like a lot of
duplication.

-- 
Stefano