Re: [PATCH 8/8] test: Add log file tests for pasta plus corresponding layout and setup

[Stefano Brivio <sbrivio@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 10801 bytes --]

On Fri, 7 Oct 2022 19:37:49 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> On Fri, Oct 07, 2022 at 02:47:42AM +0200, Stefano Brivio wrote:
> > To test log files on a tmpfs mount, we need to unshare the mount
> > namespace, which means using a context for the passt pane is not
> > really practical at the moment, as we can't open a shell there, so
> > we would have to encapsulate all the commands under 'unshare -rUm',
> > plus the "inner" pasta command, running in turn a tcp_rr server.
> > 
> > It might be worth fixing this by e.g. detecting we are trying to
> > spawn an interactive shell and adding a special path in the context
> > setup with some form of stdin redirection -- I'm not sure it's doable
> > though.
> > 
> > For this reason, add a new layout, using a context only for the host
> > pane, while keeping the old command dispatch mechanism for the passt
> > pane.  
> 
> Blecch.  I really hate going backwards with the test mechanisms like
> this.  .. and I don't think you need to, though it will require a
> slight rearrangement.
> 
> AIUI, you want to run basically the same tests against a logfile on
> the main fs (probably ext4 or xfs) and on tmpfs.  So, I'd suggest one
> test file that does the core tests, with two different setup functions
> for the two cases.  For the normal fs test, set the passt context to
> be a second host context, for the tmpfs test, set the passt context to
> nsenter an unshare -rUm.

Sure, I can do it, but I wonder: wouldn't it be worth to eventually try
and allow spawning an interactive shell from contexts, so that I can
have a single setup function -- which looks definitely better to me,
especially given the premises of the patch enabling test selection I
just posted?

Because, if that's feasible, I would rather keep this temporary
regression, for simplicity.

> I don't think you need another context to run things within the pasta
> environment, because AFAICT you can just use a fixed command for each
> pasta invocation rather than a shell (see details below).

Of course, I tried, and:

> > We also need a new setup function that doesn't start pasta: we want
> > to start and restart it with different options.
> > 
> > Further, we need a 'pint' directive, to send an interrupt to the
> > passt pane: add that in lib/test.
> > 
> > All the tests before the one involving tmpfs and a detached mount
> > namespace were also tested with the context mechanism. To make an
> > eventual conversion easier, pass tcp_crr directly as a command on
> > pasta's command line where feasible.
> > 
> > While at it, fix the comment to the teardown_pasta() function.
> > 
> > The new test set can be semi-conveniently run as:
> > 
> >   ./run pasta_options/log_to_file
> > 
> > and it checks basic log creation, size of the log file after flooding
> > it with debug entries, rotations, and basic consistency after
> > rotations, on both an existing filesystem and a tmpfs, chosen as
> > it doesn't support collapsing data ranges via fallocate(), hence
> > triggering the fall-back mechanism for logging rotation.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
> > ---
> >  test/lib/layout                | 39 +++++++++++++++
> >  test/lib/setup                 | 15 +++++-
> >  test/lib/test                  |  3 ++
> >  test/pasta_options/log_to_file | 90 ++++++++++++++++++++++++++++++++++
> >  test/run                       |  4 ++
> >  5 files changed, 150 insertions(+), 1 deletion(-)
> >  create mode 100644 test/pasta_options/log_to_file
> > 
> > diff --git a/test/lib/layout b/test/lib/layout
> > index 79a6c80..fcd1db4 100644
> > --- a/test/lib/layout
> > +++ b/test/lib/layout
> > @@ -43,6 +43,45 @@ layout_host() {
> >  	sleep 1
> >  }
> >  
> > +# layout_pasta_simple() - Panes for host and pasta
> > +layout_pasta_simple() {
> > +	sleep 3
> > +
> > +	tmux kill-pane -a -t 0
> > +	cmd_write 0 clear
> > +
> > +	tmux split-window -v -t passt_test
> > +	tmux split-window -h -t passt_test
> > +
> > +	PANE_PASST=0
> > +	PANE_HOST=1
> > +	PANE_INFO=2
> > +
> > +	get_info_cols
> > +
> > +	tmux send-keys -l -t ${PANE_INFO} 'while cat '"$STATEBASE/log_pipe"'; do :; done'
> > +	tmux send-keys -t ${PANE_INFO} -N 100 C-m
> > +	tmux select-pane -t ${PANE_INFO} -T "test log"
> > +
> > +	if context_exists host; then
> > +		pane_watch_contexts ${PANE_HOST} host host
> > +	else
> > +		tmux pipe-pane -O -t ${PANE_HOST} "cat >> ${LOGDIR}/pane_host.log"
> > +		tmux select-pane -t ${PANE_HOST} -T "host"
> > +	fi
> > +
> > +	if context_exists passt; then
> > +		pane_watch_contexts ${PANE_PASST} host host
> > +	else
> > +		tmux pipe-pane -O -t ${PANE_PASST} "cat >> ${LOGDIR}/pane_passt.log"
> > +		tmux select-pane -t ${PANE_PASST} -T "pasta"
> > +	fi
> > +
> > +	info_layout "single pasta instance"
> > +
> > +	sleep 1
> > +}
> > +
> >  # layout_pasta() - Panes for host, pasta, and separate one for namespace
> >  layout_pasta() {
> >  	sleep 3
> > diff --git a/test/lib/setup b/test/lib/setup
> > index 071e11d..e2d0ff0 100755
> > --- a/test/lib/setup
> > +++ b/test/lib/setup
> > @@ -106,6 +106,13 @@ setup_pasta() {
> >  	wait_for [ -f "${STATESETUP}/passt.pid" ]
> >  }
> >  
> > +# setup_pasta_options() - Set up layout and host context without starting pasta
> > +setup_pasta_options() {
> > +	context_setup_host host
> > +
> > +	layout_pasta_simple
> > +}
> > +
> >  # setup_passt_in_ns() - Set up namespace (with pasta), run qemu and passt into it
> >  setup_passt_in_ns() {
> >  	context_setup_host host
> > @@ -294,7 +301,7 @@ teardown_passt() {
> >  	teardown_context_watch ${PANE_GUEST} qemu guest
> >  }
> >  
> > -# teardown_passt() - Exit namespace, kill pasta process
> > +# teardown_pasta() - Exit namespace, kill pasta process
> >  teardown_pasta() {
> >  	${NSHOLDER} "${STATESETUP}/ns.hold" stop
> >  	context_wait unshare
> > @@ -304,6 +311,12 @@ teardown_pasta() {
> >  	teardown_context_watch ${PANE_NS} unshare ns
> >  }
> >  
> > +# teardown_pasta_options() - Tear down pasta and host context, no namespace
> > +teardown_pasta_options() {
> > +	teardown_context_watch ${PANE_HOST} host
> > +	teardown_context_watch ${PANE_PASST} passt
> > +}
> > +
> >  # teardown_passt_in_ns() - Exit namespace, kill qemu and pasta, remove pid file
> >  teardown_passt_in_ns() {
> >  	context_run ns kill $(cat "${STATESETUP}/qemu.pid")
> > diff --git a/test/lib/test b/test/lib/test
> > index 558d0f0..4c271a5 100755
> > --- a/test/lib/test
> > +++ b/test/lib/test
> > @@ -137,6 +137,9 @@ test_one_line() {
> >  	"passtw")
> >  		pane_or_context_wait passt || TEST_ONE_nok=1
> >  		;;
> > +	"pint")
> > +		tmux send-keys -t ${PANE_PASST} "C-c"
> > +		;;
> >  	"pout")
> >  		__varname="${__arg%% *}"
> >  		__output="$(pane_or_context_output passt "${__arg#* }")"
> > diff --git a/test/pasta_options/log_to_file b/test/pasta_options/log_to_file
> > new file mode 100644
> > index 0000000..587bf8e
> > --- /dev/null
> > +++ b/test/pasta_options/log_to_file
> > @@ -0,0 +1,90 @@
> > +# SPDX-License-Identifier: AGPL-3.0-or-later
> > +#
> > +# PASST - Plug A Simple Socket Transport
> > +#  for qemu/UNIX domain socket mode
> > +#
> > +# PASTA - Pack A Subtle Tap Abstraction
> > +#  for network namespace/tap device mode
> > +#
> > +# test/pasta_options/log_to_file - Check log creation, rotations and consistency
> > +#
> > +# Copyright (c) 2022 Red Hat GmbH
> > +# Author: Stefano Brivio <sbrivio(a)redhat.com>
> > +
> > +htools	wc tcp_rr tail cut tr sort
> > +
> > +def	flood_log_server
> > +passtb	tcp_crr --nolog -P 10001 -C 10002 -6
> > +sleep	1
> > +endef
> > +
> > +def	flood_log_client
> > +host	tcp_crr --nolog -P 10001 -C 10002 -6 -c -H ::1
> > +endef
> > +
> > +def	check_log_size_mountns
> > +pout	SIZE cat __LOG_FILE__ | wc -c
> > +check	[ __SIZE__ -gt $((50 * 1024)) ]
> > +check	[ __SIZE__ -lt $((100 * 1024)) ]
> > +endef
> > +
> > +test	Log creation
> > +
> > +set	PORTS -t 10001,10002 -u 10001,10002
> > +set	LOG_FILE __STATEDIR__/pasta.log
> > +
> > +passt	./pasta -l __LOG_FILE__  
> 
> Here you can use 'true' instead of starting a shell then exiting.

Sure. This is not even a problem actually, the shell will terminate and
I have my small log file.

> > +passt	exit
> > +check	[ -s __LOG_FILE__ ]
> > +
> > +test	Log truncated on creation
> > +passt	./pasta -l __LOG_FILE__
> > +passt	exit  
> 
> Same here.

Same here.

> > +check	[ $(cat __LOG_FILE__ | wc -l) -eq 1 ]
> > +
> > +test	Maximum log size
> > +passtb	./pasta --config-net -d -f -l __LOG_FILE__ --log-size $((100 * 1024)) -- sh -c 'while true; do tcp_crr --nolog -P 10001 -C 10002 -6; done'  
> 
> Here you're already using a command.

Yes, and it works with the context approach, I tested it until this
point.

> > +sleep	1
> > +
> > +flood_log_client
> > +check	[ $(cat __LOG_FILE__ | wc -c) -gt $((50 * 1024)) ]
> > +check	[ $(cat __LOG_FILE__ | wc -c) -lt $((100 * 1024)) ]
> > +
> > +flood_log_client
> > +check	[ $(cat __LOG_FILE__ | wc -c) -gt $((50 * 1024)) ]
> > +check	[ $(cat __LOG_FILE__ | wc -c) -lt $((100 * 1024)) ]
> > +
> > +flood_log_client
> > +check	[ $(cat __LOG_FILE__ | wc -c) -gt $((50 * 1024)) ]
> > +check	[ $(cat __LOG_FILE__ | wc -c) -lt $((100 * 1024)) ]
> > +
> > +pint
> > +
> > +test	Timestamp consistency after rotations
> > +check	tail -n +2 __LOG_FILE__ | cut -f1 -d' ' | tr -d [.:] | sort -c
> > +
> > +test	Maximum log size on tmpfs (no FALLOC_FL_COLLAPSE_RANGE)
> > +passt	unshare -rUm
> > +passt	mkdir __STATEDIR__/t
> > +passt	mount -t tmpfs none __STATEDIR__/t
> > +set	LOG_FILE __STATEDIR__/t/log
> > +passt	./pasta --config-net -d -l __LOG_FILE__ --log-size $((100 * 1024))  
> 
> Here I think you can use the same while true trick as above, rather
> than starting the server repeatedly.

Not really, because I want to check the size of the log file several
times during the test, and I can't do it from outside the mount
namespace, hence this:

	pout	SIZE cat __LOG_FILE__ | wc -c

in check_log_size_mountns. This would be solved by a separate
setup function like the one you mentioned, though. Having a separate
context/pane to just check that looks a bit bad for usability (in terms
of showing what's going on).

So the question here is really if we can avoid having a separate setup
function, by means of adapting the context mechanism to enable
interactive shells. If we can't, fine.

But if we can, I'd leave this as a temporary hack, because it's more
usable than the alternative -- for example, I don't have to select two
separate tests for this.

-- 
Stefano