Jakub Kicinski wrote:
Very exciting to see packetdrill tests making their way upstream!
On Tue, 27 Aug 2024 15:32:29 -0400 Willem de Bruijn wrote:
RFC points for discussion
ksft: the choice for this python framework introduces a dependency on the YNL scripts, and some non-obvious code:
- to include the net/lib dep in tools/testing/selftests/Makefile
- a boilerplate lib/py/__init__.py that each user of ksft will need
It seems preferable to me to use ksft.py over reinventing the wheel, e.g., to print KTAP output. But perhaps we can make it more obvious for future ksft users, and make the dependency on YNL optional.
No preference here, the wrapper script uses little of the python code, and use of YNL seems unlikely, so it's a coin toss whether it's worth linking up to net/lib/py or just writing a bit of bash.
kselftest-per-directory: copying packetdrill_ksft.py to create a separate script per dir is a bit of a hack. A single script is much simpler, optionally with nested KTAP (not supported yet by ksft). But, I'm afraid that running time without intermediate output will be very long when we integrate all packetdrill scripts.
Not sure what you mean by intermediate output (the perl wrapper prints immediately, do you have perl?). We do have some nested ktap parsing in nipa, but small tweaks would be necessary to "decapsulate" the first layer completely. My bigger concern would be runtime, if the time it takes to run all tests grows we spawn multiple VMs and load balance the test cases.
All in all, trying to support single script is probably not worth the extra effort.
nf_conntrack: we can dedup the common.sh.
*pkt files: which of the 150+ scripts on github are candidates for kselftests, all or a subset? To avoid change detector tests.
Other than avoiding known flakes - no concerns. Is the distinction between "change detector" vs hard tests (uAPI change / RFC compliance), well defined? Not sure if that's really possible but if so it would be nice to "sort" the tests into different dirs.
It's not as clear-cut as API/RFC.
The major change detector issues are with timing, as packetdrill scripts have timestamped lines. I think usec TCP timestamps were an issue at some point. On-going is that we increase allowed variance on debug builds. Note to self that that is missing here.
I think that all tests we open sourced to github so far were chosen to be robust. Will double check and err on the side of caution.
And what is the best way to eventually send up to 150 files, 7K LoC.
Just send them, slice into a handful (<10) of patches if you can. 7k LoC is same order of magnitude as initial drivers we merge.
To be clear let's start with a small patch like this one. Since this is a new target I'll have to reconfigure the runners. So we'll see how well this works once it's merged. Spinning up new runners is a bit tedious but here new target seems quite justified.
Ack. Thanks for that guidance.
tools/testing/selftests/Makefile | 7 +- .../selftests/net/packetdrill/.gitignore | 1 + .../selftests/net/packetdrill/Makefile | 28 ++++++ .../net/packetdrill/lib/py/__init__.py | 15 ++++ .../net/packetdrill/packetdrill_ksft.py | 90 +++++++++++++++++++ .../net/packetdrill/tcp/common/defaults.sh | 63 +++++++++++++ .../net/packetdrill/tcp/common/set_sysctls.py | 38 ++++++++ .../net/packetdrill/tcp/inq/client.pkt | 51 +++++++++++ .../net/packetdrill/tcp/inq/server.pkt | 51 +++++++++++ .../tcp/md5/md5-only-on-client-ack.pkt | 28 ++++++
prolly need a config file to enable kconfig for MD5 ? perils of adding new targets
diff --git a/tools/testing/selftests/net/packetdrill/.gitignore b/tools/testing/selftests/net/packetdrill/.gitignore new file mode 100644 index 0000000000000..a40f1a600eb94 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/.gitignore @@ -0,0 +1 @@ +tcp*sh
Is this right or should it be tcp_*.py ?
diff --git a/tools/testing/selftests/net/packetdrill/Makefile b/tools/testing/selftests/net/packetdrill/Makefile new file mode 100644 index 0000000000000..d94c51098d1f0 --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/Makefile @@ -0,0 +1,28 @@ +# SPDX-License-Identifier: GPL-2.0
Would be nice to add a few lines here with an overview of how the packetdrill tests get executed. People may jump in here to try to add new tests, since that's how ksft usually operates.
+def scriptname_to_testdir(filepath):
- """Extract the directory to run from this filename."""
- suffix = ".sh"
.sh again ?
diff --git a/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh b/tools/testing/selftests/net/packetdrill/tcp/common/defaults.sh
+# 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
nit: double new line
diff --git a/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py new file mode 100755 index 0000000000000..5ddf456ae973a --- /dev/null +++ b/tools/testing/selftests/net/packetdrill/tcp/common/set_sysctls.py
What calls this one? I can't grep it out.
Whoops. I changed test directories and apparently these ones don't rely on this.
Thanks for the review. I'll take a quick stab at a standalone script to see whether that is easier. Else will resubmit with these and Paolo's feedback addressed.
In particular to Paolo's feedback: running multiple tests in parallel like run_all.py can. Come to think of it, maybe I should import run_all.py, modifying it to output KTAP and ksft return codes.