Matthieu Baerts wrote:
Hi Willem,
On 06/09/2024 01:15, Willem de Bruijn wrote:
From: Willem de Bruijn willemb@google.com
Lay the groundwork to import into kselftests the over 150 packetdrill TCP/IP conformance tests on github.com/google/packetdrill.
Florian recently added support for packetdrill tests in nf_conntrack, in commit a8a388c2aae49 ("selftests: netfilter: add packetdrill based conntrack tests").
This patch takes a slightly different approach. It relies on ksft_runner.sh to run every *.pkt file in the directory.
Thank you for adding this support! I'm looking forward to seeing more packetdrill tests being validated by the CI, and I hope that will encourage people to add more tests, and increase the code coverage!
Thanks for taking a look and your feedback.
I have some questions about how the packetdrill should be executed: should they be isolated in dedicated netns?
Yes. The runner decides that, by passing -n
There are some other comments, but feel free to ignore them if they are not relevant, or if they can be fixed later.
Tested: make -C tools/testing/selftests \ TARGETS=net/packetdrill \ run_tests
Please note that this will not run the packetdrill tests in a dedicated netns. I don't think that's a good idea. Especially when sysctl knobs are being changed during the tests, and more.
make -C tools/testing/selftests \ TARGETS=net/packetdrill \ install INSTALL_PATH=$KSFT_INSTALL_PATH
# in virtme-ng ./run_kselftest.sh -c net/packetdrill ./run_kselftest.sh -t net/packetdrill:tcp_inq_client.pkt
I see that ./run_kselftest.sh can be executed with the "-n | --netns" option to run each test in a dedicated net namespace, but it doesn't seem to work:
# ./run_kselftest.sh -n -c net/packetdrill [ 411.087208] kselftest: Running tests in net/packetdrill TAP version 13 1..3 Cannot open network namespace "tcp_inq_client.pkt-TaQ8iN": No such file or directory setting the network namespace "tcp_inq_server.pkt-sW8YCz" failed: Invalid argument Cannot open network namespace "tcp_md5_md5-only-on-client-ack.pkt-YzJal6": No such file or directory
Ah, I guess this requires adding CONFIG_NET_NS=y to tools/testing/selftests/net/packetdrill/config
But maybe it would be better to create the netns in ksft_runner.sh? Please see below.
No, we opted for this design exactly to use existing kselftest infra, rather than reimplementing that in our wrapper, as I did in the RFC.
(...)
diff --git a/tools/testing/selftests/net/packetdrill/defaults.sh b/tools/testing/selftests/net/packetdrill/defaults.sh new file mode 100755 index 0000000000000..1095a7b22f44d --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/defaults.sh @@ -0,0 +1,63 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Set standard production config values that relate to TCP behavior.
+# Flush old cached data (fastopen cookies). +ip tcp_metrics flush all > /dev/null 2>&1
(Why ignoring errors if any?)
I don't know. Just importing this verbatim from github. As that is debugged over a long time and proven to work.
+# TCP min, default, and max receive and send buffer sizes. +sysctl -q net.ipv4.tcp_rmem="4096 540000 $((15*1024*1024))" +sysctl -q net.ipv4.tcp_wmem="4096 $((256*1024)) 4194304"
+# TCP timestamps. +sysctl -q net.ipv4.tcp_timestamps=1
+# TCP SYN(ACK) retry thresholds +sysctl -q net.ipv4.tcp_syn_retries=5 +sysctl -q net.ipv4.tcp_synack_retries=5
+# TCP Forward RTO-Recovery, RFC 5682. +sysctl -q net.ipv4.tcp_frto=2
+# TCP Selective Acknowledgements (SACK) +sysctl -q net.ipv4.tcp_sack=1
+# TCP Duplicate Selective Acknowledgements (DSACK) +sysctl -q net.ipv4.tcp_dsack=1
+# TCP FACK (Forward Acknowldgement) +sysctl -q net.ipv4.tcp_fack=0
+# TCP reordering degree ("dupthresh" threshold for entering Fast Recovery). +sysctl -q net.ipv4.tcp_reordering=3
+# TCP congestion control. +sysctl -q net.ipv4.tcp_congestion_control=cubic
(maybe safer to add CONFIG_TCP_CONG_CUBIC=y?)
Ack, thanks. Also below.
+# TCP slow start after idle. +sysctl -q net.ipv4.tcp_slow_start_after_idle=0
+# TCP RACK and TLP. +sysctl -q net.ipv4.tcp_early_retrans=4 net.ipv4.tcp_recovery=1
+# TCP method for deciding when to defer sending to accumulate big TSO packets. +sysctl -q net.ipv4.tcp_tso_win_divisor=3
+# TCP Explicit Congestion Notification (ECN) +sysctl -q net.ipv4.tcp_ecn=0
+sysctl -q net.ipv4.tcp_pacing_ss_ratio=200 +sysctl -q net.ipv4.tcp_pacing_ca_ratio=120 +sysctl -q net.ipv4.tcp_notsent_lowat=4294967295 > /dev/null 2>&1
+sysctl -q net.ipv4.tcp_fastopen=0x70403 +sysctl -q net.ipv4.tcp_fastopen_key=a1a1a1a1-b2b2b2b2-c3c3c3c3-d4d4d4d4
+sysctl -q net.ipv4.tcp_syncookies=1
(maybe safer to add CONFIG_SYN_COOKIES=y?)
+# Override the default qdisc on the tun device. +# Many tests fail with timing errors if the default +# is FQ and that paces their flows. +tc qdisc add dev tun0 root pfifo
(when applying your patches with 'b4 shazam', I got the following error, I guess it was due to this blank line at the end)
Applying: selftests: support interpreted scripts with ksft_runner.sh Applying: selftests/net: integrate packetdrill with ksft .git/rebase-apply/patch:142: new blank line at EOF.
diff --git a/tools/testing/selftests/net/packetdrill/ksft_runner.sh b/tools/testing/selftests/net/packetdrill/ksft_runner.sh new file mode 100755 index 0000000000000..2f62caccbbbc5 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/ksft_runner.sh @@ -0,0 +1,41 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0
+source "$(dirname $(realpath $0))/../../kselftest/ktap_helpers.sh"
+readonly ipv4_args=('--ip_version=ipv4 '
'--local_ip=192.168.0.1 ''--gateway_ip=192.168.0.1 ''--netmask_ip=255.255.0.0 ''--remote_ip=192.0.2.1 ''-D CMSG_LEVEL_IP=SOL_IP ''-D CMSG_TYPE_RECVERR=IP_RECVERR ')+readonly ipv6_args=('--ip_version=ipv6 '
'--mtu=1520 ''--local_ip=fd3d:0a0b:17d6::1 ''--gateway_ip=fd3d:0a0b:17d6:8888::1 ''--remote_ip=fd3d:fa7b:d17d::1 ''-D CMSG_LEVEL_IP=SOL_IPV6 ''-D CMSG_TYPE_RECVERR=IPV6_RECVERR ')(nit: that's a strange Bash array with spaces in the strings :) You can simply remove the quotes, then below, you can use the variable with double quotes: "${ipv4_args[@]}" and shellcheck will stop reporting an error for that)
Thanks!
+if [ $# -ne 1 ]; then
- ktap_exit_fail_msg "usage: $0 <script>"
- exit "$KSFT_FAIL"
+fi +script="$1"
+if [ -z "$(which packetdrill)" ]; then
- ktap_skip_all "packetdrill not found in PATH"
- exit "$KSFT_SKIP"
+fi
+ktap_print_header +ktap_set_plan 2
+packetdrill ${ipv4_args[@]} $(basename $script) > /dev/null \
(Why muting stdout? I see that the TCP MD5 test output lines from /proc/net/tcp*, is it why? Is this info useful? Or should it be removed from the test?)
Indeed that's why. If a test fails in automated testing we can run manually to inspect such output.
- && ktap_test_pass "ipv4" || ktap_test_fail "ipv4"
+packetdrill ${ipv6_args[@]} $(basename $script) > /dev/null \
- && ktap_test_pass "ipv6" || ktap_test_fail "ipv6"
Even if "run_kselftest.sh" creates dedicated netns before running this script (RUN_IN_NETNS=1), it looks like the tests in v4 and in v6 will share the same netns. Is it OK? It means that if a packetdrill test sets something that is not reset by 'defaults.sh', it might break the following v6 test.
That should be fine. If a test cares about a sysctl, then it needs to set it at the start. In this case, they both will set exactly the same anyway.
Why not having "ksft_runner.sh" creating the netns? It should be easy to do so, using helpers from the "../lib.sh" file:
See above.
ns_v4= ns_v6=
trap cleanup_all_ns EXIT if ! setup_ns ns_v4 ns_v6; then (...) # fail + exit fi
ip netns exec "${ns_v4}" packetdrill "${ipv4_args[@]}" \ "$(basename "${script}")" (...)
(Note that if these tests are isolated in dedicated netns, and if later we want to accelerate their execution, it should be easy to run these two tests in parallel, something like the following)
ip netns exec "${ns_v4}" (...) & pid_v4=$! ip netns exec "${ns_v6}" (...) & pid_v6=$!
wait ${pid_v4} && tap_test_pass "ipv4" || ktap_test_fail "ipv4" wait ${pid_v6} && tap_test_pass "ipv6" || ktap_test_fail "ipv6"
+ktap_finished diff --git a/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt new file mode 100644 index 0000000000000..df49c67645ac8 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/tcp_inq_client.pkt @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0 +// Test TCP_INQ and TCP_CM_INQ on the client side. +`./defaults.sh +`
(I guess you prefer not to modify these tests, and keep them self-contained, but just in case it is easier for you, this line could be removed, and have ksft_runner.sh sourcing this file before executing the packetdrill test.)
Future packetdrill tests can have different shell preambles. Let's indeed leave it to the tests themselves.
Cheers, Matt -- Sponsored by the NGI0 Core fund.