Message-ID: <ad780c53-9538-4d3f-a02f-1063828fc035@kernel.org>
Date: Fri, 6 Sep 2024 18:26:55 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
 pabeni@...hat.com, ncardwell@...gle.com, shuah@...nel.org,
 linux-kselftest@...r.kernel.org, fw@...len.de,
 Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next v2 2/2] selftests/net: integrate packetdrill with
 ksft

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@...gle.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.

>> (...)
>>
>>> 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.

All good, I'm fine like that.

(...)

>>> 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 @@

(...)

>>> +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.

If we can reproduce the issue :)

Or maybe it is not an issue to store such output in the logs?

But if you tell me that this output has never been helpful in the past,
it is fine for me to mute it, not to have to modify the tests.

>>> +	&& 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.

Ack.

>> 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.

Before the discussions from the RFC, I initially thought that the
selftest itself had to deal with the netns. But then I understood there
was a possibility to force the kselftest infra to execute the different
tests in a dedicated netns. To be honest, it is not clear to me who
should be in charge of creating these netns :)

Maybe that's fine like that then, but it feels strange to me not to have
such isolation :)

In Florian's version, the packetdrill tests are executed in a dedicated
netns using 'unshare -n'. That's an easier alternative than the one I
suggested in my previous email with setup_ns.

(...)

>>> 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.

It makes sense.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Powered by blists - more mailing lists