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

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

Jakub Kicinski wrote:
> On Wed, 28 Aug 2024 10:03:55 -0400 Willem de Bruijn wrote:
> > > > 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.  
> > > 
> > > If I read correctly, this runs the scripts in the given directory 
> > > sequentially (as opposed to the default pktdrill run_all.py behavior 
> > > that uses many concurrent threads).
> > > 
> > > I guess/fear that running all the pktdrill tests in a single batch would 
> > > take quite a long time, which in turn could be not so good for CI 
> > > integration. Currently there are a couple of CI test-cases with runtime   
> > >  > 1h, but that is bad ;)  
> > 
> > Very good point, thanks! This is the third packetdrill runner that I'm
> > writing. I should know this by now.. Let me see whether I can use
> > run_all.py rather than reinvent the wheel here.
> 
> Do we have to worry about this now? If you're planning to add a runner
> specifically for packetdrill... IDK if we should. We already have a few
> runners, not to mention that run_kselftest.sh itself can run all the
> test cases in parallel in separate network namespaces!
> 
> What I was wondering is whether we can use shebang to direct the .pkt
> files to be "executed" by the python script. Alternatively we can add
> support to specifying "interpreter" for a given test in ksft infra
> (kinda like we can pass cmd line arguments to a test). Completely
> untested but it should give better idea what I mean than a description:
> 
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index 74954f6a8f94..429c279e9c6e 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -56,6 +56,7 @@ run_one()
>  	export kselftest_timeout="$kselftest_default_timeout"
>  
>  	# Safe default if tr not available
> +	kselftest_interp_ref="KSELFTEST_INTERP"
>  	kselftest_cmd_args_ref="KSELFTEST_ARGS"
>  
>  	# Optional arguments for this command, possibly defined as an
> @@ -78,6 +79,14 @@ run_one()
>  					$TR_CMD [:lower:] [:upper:])
>  		kselftest_cmd_args_ref="KSELFTEST_${BASENAME_SANITIZED}_ARGS"
>  	fi
> +	# Optional interpreter to run the test case
> +	if [ -n "$TR_CMD" ]; then
> +		SUFFIX_SANITIZED=$(echo "${BASENAME_TEST#*.}" | \
> +				       $TR_CMD -d "[:blank:][:cntrl:]" | \
> +				       $TR_CMD -c "[:alnum:]_" "_" | \
> +				       $TR_CMD [:lower:] [:upper:])
> +		kselftest_interp_ref="KSELFTEST_${SUFFIX_SANITIZED}_INTERP"
> +	fi
>  
>  	# Load per-test-directory kselftest "settings" file.
>  	settings="$BASE_DIR/$DIR/settings"
> @@ -110,8 +119,12 @@ run_one()
>  		if [ -x /usr/bin/stdbuf ]; then
>  			stdbuf="/usr/bin/stdbuf --output=L "
>  		fi
> +		eval kselftest_interp="\$${kselftest_interp_ref:-}"
> +		# Add a space at the end if interpreter is set to work in $cmd
> +		[ -n "$kselftest_interp" ] && \
> +		    kselftest_interp="$kselftest_interp "
>  		eval kselftest_cmd_args="\$${kselftest_cmd_args_ref:-}"
> -		cmd="$stdbuf ./$BASENAME_TEST $kselftest_cmd_args"
> +		cmd="$stdbuf ./$kselftest_interp$BASENAME_TEST $kselftest_cmd_args"
>  		if [ ! -x "$TEST" ]; then
>  			echo "# Warning: file $TEST is not executable"

That could work.

Is reporting one KTAP and exitcode per directory vs per packetdrill
invocation good/bad/neither?

Three other issues if this is calling packetdrill directly is
- passing the non-trivial IP specific flags
- running twice, for IPv4 and IPv6
- chdir into the directory of the pkt file

That can be addressed by instead calling a small wrapper shell script.

That would do the test_func_builder part of packetdrill_ksft.py.
But without the need to handle netns, popen/cmd, etc, and thus the
ksft dependencies.