| Message ID | 0378c55466d8d1f7b6d99d581811d49429e1f4e7.1646691728.git.gnault@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] selftests: pmtu.sh: Kill tcpdump processes launched by subshell. | expand |
On 3/7/22 3:38 PM, Guillaume Nault wrote: > The cleanup() function takes care of killing processes launched by the > test functions. It relies on variables like ${tcpdump_pids} to get the > relevant PIDs. But tests are run in their own subshell, so updated > *_pids values are invisible to other shells. Therefore cleanup() never > sees any process to kill: > > $ ./tools/testing/selftests/net/pmtu.sh -t pmtu_ipv4_exception > TEST: ipv4: PMTU exceptions [ OK ] > TEST: ipv4: PMTU exceptions - nexthop objects [ OK ] > > $ pgrep -af tcpdump > 6084 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap > 6085 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap > 6086 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap > 6087 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap > 6088 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap > 6089 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap > 6090 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap > 6091 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap > 6228 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap > 6229 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap > 6230 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap > 6231 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap > 6232 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap > 6233 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap > 6234 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap > 6235 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap > > Fix this by running cleanup() in the context of the test subshell. > Now that each test cleans the environment after completion, there's no > need for calling cleanup() again when the next test starts. So let's > drop it from the setup() function. This is okay because cleanup() is > also called when pmtu.sh starts, so even the first test starts in a > clean environment. > > Note: PAUSE_ON_FAIL is still evaluated before cleanup(), so one can > still inspect the test environment upon failure when using -p. > > Fixes: a92a0a7b8e7c ("selftests: pmtu: Simplify cleanup and namespace names") > Signed-off-by: Guillaume Nault <gnault@redhat.com> > --- > tools/testing/selftests/net/pmtu.sh | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > Reviewed-by: David Ahern <dsahern@kernel.org>
On Tue, Mar 08, 2022 at 11:21:58AM -0700, David Ahern wrote: > On 3/7/22 3:38 PM, Guillaume Nault wrote: > > The cleanup() function takes care of killing processes launched by the > > test functions. It relies on variables like ${tcpdump_pids} to get the > > relevant PIDs. But tests are run in their own subshell, so updated > > *_pids values are invisible to other shells. Therefore cleanup() never > > sees any process to kill: > > > > $ ./tools/testing/selftests/net/pmtu.sh -t pmtu_ipv4_exception > > TEST: ipv4: PMTU exceptions [ OK ] > > TEST: ipv4: PMTU exceptions - nexthop objects [ OK ] > > > > $ pgrep -af tcpdump > > 6084 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap > > 6085 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap > > 6086 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap > > 6087 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap > > 6088 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap > > 6089 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap > > 6090 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap > > 6091 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap > > 6228 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap > > 6229 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap > > 6230 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap > > 6231 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap > > 6232 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap > > 6233 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap > > 6234 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap > > 6235 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap > > > > Fix this by running cleanup() in the context of the test subshell. > > Now that each test cleans the environment after completion, there's no > > need for calling cleanup() again when the next test starts. So let's > > drop it from the setup() function. This is okay because cleanup() is > > also called when pmtu.sh starts, so even the first test starts in a > > clean environment. > > > > Note: PAUSE_ON_FAIL is still evaluated before cleanup(), so one can > > still inspect the test environment upon failure when using -p. > > > > Fixes: a92a0a7b8e7c ("selftests: pmtu: Simplify cleanup and namespace names") > > Signed-off-by: Guillaume Nault <gnault@redhat.com> > > --- > > tools/testing/selftests/net/pmtu.sh | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > Reviewed-by: David Ahern <dsahern@kernel.org> Hum, looks like for such short lived tcpdump sessions, we also need to disable buffered mode. Otherwise tcpdump can be killed before it actually processed the packets. Also there's another problem, with the xfrmudp tests not recording the correct PID in ${nettest_pids}. I'll send v2 to also fix all these problems.
diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh index 543ad7513a8e..1db670a01f9b 100755 --- a/tools/testing/selftests/net/pmtu.sh +++ b/tools/testing/selftests/net/pmtu.sh @@ -865,7 +865,6 @@ setup_ovs_bridge() { setup() { [ "$(id -u)" -ne 0 ] && echo " need to run as root" && return $ksft_skip - cleanup for arg do eval setup_${arg} || { echo " ${arg} not supported"; return 1; } done @@ -1836,6 +1835,10 @@ run_test() { unset IFS + # Since cleanup() relies on variables modified by this subshell, it + # has to run in this context. + trap cleanup EXIT + if [ "$VERBOSE" = "1" ]; then printf "\n##########################################################################\n\n" fi
The cleanup() function takes care of killing processes launched by the test functions. It relies on variables like ${tcpdump_pids} to get the relevant PIDs. But tests are run in their own subshell, so updated *_pids values are invisible to other shells. Therefore cleanup() never sees any process to kill: $ ./tools/testing/selftests/net/pmtu.sh -t pmtu_ipv4_exception TEST: ipv4: PMTU exceptions [ OK ] TEST: ipv4: PMTU exceptions - nexthop objects [ OK ] $ pgrep -af tcpdump 6084 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap 6085 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap 6086 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap 6087 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap 6088 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap 6089 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap 6090 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap 6091 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap 6228 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap 6229 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap 6230 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap 6231 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap 6232 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap 6233 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap 6234 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap 6235 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap Fix this by running cleanup() in the context of the test subshell. Now that each test cleans the environment after completion, there's no need for calling cleanup() again when the next test starts. So let's drop it from the setup() function. This is okay because cleanup() is also called when pmtu.sh starts, so even the first test starts in a clean environment. Note: PAUSE_ON_FAIL is still evaluated before cleanup(), so one can still inspect the test environment upon failure when using -p. Fixes: a92a0a7b8e7c ("selftests: pmtu: Simplify cleanup and namespace names") Signed-off-by: Guillaume Nault <gnault@redhat.com> --- tools/testing/selftests/net/pmtu.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)