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

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

Matthieu Baerts wrote:
> On 06/09/2024 17:36, Willem de Bruijn wrote:
> > 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.
> 
> You are welcome!
> 
> >> 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 
> 
> But then it means that by default, the tests are not isolated. I think
> many (most?) selftests are running in a dedicated netns by default, no?
> 
> To be honest, that's the first time I hear about:
> 
>   ./run_kselftest.sh --netns
> 
> I don't know if it is common to use it, nor if we can enable this
> feature when using 'make run_tests'. And I don't know if many CI runs
> multiple selftests in parallel from the same VM.
> 
> >> 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
> 
> Good point. But I have CONFIG_NET_NS=y on my side. I didn't investigate
> more, I was first wondering if other people tried this option.
> 
> >> 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.
> 
> OK, I understood from the discussions from the RFC that by using the
> kselftest infra, the tests would be automatically executed in dedicated
> netns, and it could also help running tests in parallel. That sounded
> great to me, but that's not the case by default from what I see.

Perhaps that's something to change in the defaults for run_tests.

Since the infra exist, that is preferable over reimplementing it for
one particular subset of tests.

Or if not all kselftests can run in netns (quite likely), this needs
to be opt-in. Then a variable defined in the Makefile perhaps. To
tell kselftest to enable the feature for this target.