Re: [PATCH net-next RFC] selftests/net: integrate packetdrill with ksft

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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.