From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Brivio To: passt-dev@passt.top Subject: Re: [PATCH 8/8] test: Add log file tests for pasta plus corresponding layout and setup Date: Fri, 07 Oct 2022 11:47:00 +0200 Message-ID: <20221007114700.181b9514@elisabeth> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6309428393289205360==" --===============6309428393289205360== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Fri, 7 Oct 2022 19:37:49 +1100 David Gibson 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. > >=20 > > 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. > >=20 > > 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. =20 >=20 > 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. >=20 > 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. > >=20 > > Further, we need a 'pint' directive, to send an interrupt to the > > passt pane: add that in lib/test. > >=20 > > 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. > >=20 > > While at it, fix the comment to the teardown_pasta() function. > >=20 > > The new test set can be semi-conveniently run as: > >=20 > > ./run pasta_options/log_to_file > >=20 > > 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. > >=20 > > Signed-off-by: Stefano Brivio > > --- > > 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 > >=20 > > 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 > > } > > =20 > > +# 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=3D0 > > + PANE_HOST=3D1 > > + PANE_INFO=3D2 > > + > > + get_info_cols > > + > > + tmux send-keys -l -t ${PANE_INFO} 'while cat '"$STATEBASE/log_pipe"'; d= o :; 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" ] > > } > > =20 > > +# setup_pasta_options() - Set up layout and host context without startin= g pasta > > +setup_pasta_options() { > > + context_setup_host host > > + > > + layout_pasta_simple > > +} > > + > > # setup_passt_in_ns() - Set up namespace (with pasta), run qemu and pass= t into it > > setup_passt_in_ns() { > > context_setup_host host > > @@ -294,7 +301,7 @@ teardown_passt() { > > teardown_context_watch ${PANE_GUEST} qemu guest > > } > > =20 > > -# 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 > > } > > =20 > > +# teardown_pasta_options() - Tear down pasta and host context, no namesp= ace > > +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 p= id 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=3D1 > > ;; > > + "pint") > > + tmux send-keys -t ${PANE_PASST} "C-c" > > + ;; > > "pout") > > __varname=3D"${__arg%% *}" > > __output=3D"$(pane_or_context_output passt "${__arg#* }")" > > diff --git a/test/pasta_options/log_to_file b/test/pasta_options/log_to_f= ile > > 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 con= sistency > > +# > > +# Copyright (c) 2022 Red Hat GmbH > > +# Author: Stefano Brivio > > + > > +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__ =20 >=20 > 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 =20 >=20 > 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 * 10= 24)) -- sh -c 'while true; do tcp_crr --nolog -P 10001 -C 10002 -6; done' =20 >=20 > 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))= =20 >=20 > 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. --=20 Stefano --===============6309428393289205360==--