diff mbox series

[kvm-unit-tests,v8,03/35] migration: Add a migrate_skip command

Message ID 20240405083539.374995-4-npiggin@gmail.com (mailing list archive)
State New, archived
Headers show
Series migration, powerpc improvements | expand

Commit Message

Nicholas Piggin April 5, 2024, 8:35 a.m. UTC
Tests that are run with MIGRATION=yes but skip due to some requirement
not being met will show as a failure due to the harness requirement to
see one successful migration. The workaround for this is to migrate in
test's skip path. Add a new command that just tells the harness to not
expect a migration.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 common/selftest-migration.c | 14 ++++++++-----
 lib/migrate.c               | 19 ++++++++++++++++-
 lib/migrate.h               |  2 ++
 powerpc/unittests.cfg       |  6 ++++++
 s390x/unittests.cfg         |  5 +++++
 scripts/arch-run.bash       | 41 +++++++++++++++++++++++++++++--------
 6 files changed, 73 insertions(+), 14 deletions(-)

Comments

Nico Boehr April 8, 2024, 3:59 p.m. UTC | #1
Quoting Nicholas Piggin (2024-04-05 10:35:04)
[...]
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index 39419d4e2..4a1aab48d 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
[...]
> @@ -179,8 +189,11 @@ run_migration ()
>                 # Wait for test exit or further migration messages.
>                 if ! seen_migrate_msg ${src_out} ;  then
>                         sleep 0.1
> -               else
> +               elif grep -q "Now migrate the VM" < ${src_out} ; then
>                         do_migration || return $?
> +               elif [ $skip_migration -eq 0 ] && grep -q "Skipped VM migration" < ${src_out} ; then
> +                       echo > ${src_infifo} # Resume src and carry on.
> +                       break;

If I understand the code correctly, this simply makes the test PASS when
migration is skipped, am I wrong?

If so, can we set ret=77 here so we get a nice SKIP?
Nicholas Piggin April 16, 2024, 3:22 a.m. UTC | #2
On Tue Apr 9, 2024 at 1:59 AM AEST, Nico Boehr wrote:
> Quoting Nicholas Piggin (2024-04-05 10:35:04)
> [...]
> > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> > index 39419d4e2..4a1aab48d 100644
> > --- a/scripts/arch-run.bash
> > +++ b/scripts/arch-run.bash
> [...]
> > @@ -179,8 +189,11 @@ run_migration ()
> >                 # Wait for test exit or further migration messages.
> >                 if ! seen_migrate_msg ${src_out} ;  then
> >                         sleep 0.1
> > -               else
> > +               elif grep -q "Now migrate the VM" < ${src_out} ; then
> >                         do_migration || return $?
> > +               elif [ $skip_migration -eq 0 ] && grep -q "Skipped VM migration" < ${src_out} ; then
> > +                       echo > ${src_infifo} # Resume src and carry on.
> > +                       break;
>
> If I understand the code correctly, this simply makes the test PASS when
> migration is skipped, am I wrong?

This just gets the harness past the wait-for-migration phase, it
otherwise should not change behaviour.

> If so, can we set ret=77 here so we get a nice SKIP?

The harness _should_ still scan the status value printed by the
test case when it exits. Is it not working as expected? We
certainly should be able to make it SKIP.

Thanks,
Nick
Thomas Huth April 16, 2024, 4:50 a.m. UTC | #3
On 16/04/2024 05.22, Nicholas Piggin wrote:
> On Tue Apr 9, 2024 at 1:59 AM AEST, Nico Boehr wrote:
>> Quoting Nicholas Piggin (2024-04-05 10:35:04)
>> [...]
>>> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
>>> index 39419d4e2..4a1aab48d 100644
>>> --- a/scripts/arch-run.bash
>>> +++ b/scripts/arch-run.bash
>> [...]
>>> @@ -179,8 +189,11 @@ run_migration ()
>>>                  # Wait for test exit or further migration messages.
>>>                  if ! seen_migrate_msg ${src_out} ;  then
>>>                          sleep 0.1
>>> -               else
>>> +               elif grep -q "Now migrate the VM" < ${src_out} ; then
>>>                          do_migration || return $?
>>> +               elif [ $skip_migration -eq 0 ] && grep -q "Skipped VM migration" < ${src_out} ; then
>>> +                       echo > ${src_infifo} # Resume src and carry on.
>>> +                       break;
>>
>> If I understand the code correctly, this simply makes the test PASS when
>> migration is skipped, am I wrong?
> 
> This just gets the harness past the wait-for-migration phase, it
> otherwise should not change behaviour.
> 
>> If so, can we set ret=77 here so we get a nice SKIP?
> 
> The harness _should_ still scan the status value printed by the
> test case when it exits. Is it not working as expected? We
> certainly should be able to make it SKIP.

I just gave it a try (by modifying the selftest-migration-skip test 
accordingly), and it seems to work fine, so I think this patch here should 
be ok.

  Thomas
diff mbox series

Patch

diff --git a/common/selftest-migration.c b/common/selftest-migration.c
index 54b5d6b2d..0afd8581c 100644
--- a/common/selftest-migration.c
+++ b/common/selftest-migration.c
@@ -14,14 +14,18 @@ 
 
 int main(int argc, char **argv)
 {
-	int i = 0;
-
 	report_prefix_push("migration");
 
-	for (i = 0; i < NR_MIGRATIONS; i++)
-		migrate_quiet();
+	if (argc > 1 && !strcmp(argv[1], "skip")) {
+		migrate_skip();
+		report(true, "migration skipping");
+	} else {
+		int i;
 
-	report(true, "simple harness stress test");
+		for (i = 0; i < NR_MIGRATIONS; i++)
+			migrate_quiet();
+		report(true, "simple harness stress");
+	}
 
 	report_prefix_pop();
 
diff --git a/lib/migrate.c b/lib/migrate.c
index 92d1d957d..1d22196b7 100644
--- a/lib/migrate.c
+++ b/lib/migrate.c
@@ -39,7 +39,24 @@  void migrate_once(void)
 
 	if (migrated)
 		return;
-
 	migrated = true;
+
 	migrate();
 }
+
+/*
+ * When the test has been started in migration mode, but the test case is
+ * skipped and no migration point is reached, this can be used to tell the
+ * harness not to mark it as a failure to migrate.
+ */
+void migrate_skip(void)
+{
+	static bool did_migrate_skip;
+
+	if (did_migrate_skip)
+		return;
+	did_migrate_skip = true;
+
+	puts("Skipped VM migration (quiet)\n");
+	(void)getchar();
+}
diff --git a/lib/migrate.h b/lib/migrate.h
index 95b9102b0..db6e0c501 100644
--- a/lib/migrate.h
+++ b/lib/migrate.h
@@ -9,3 +9,5 @@ 
 void migrate(void);
 void migrate_quiet(void);
 void migrate_once(void);
+
+void migrate_skip(void);
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index 1559bee98..cae4949e8 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -43,6 +43,12 @@  groups = selftest migration
 # https://lore.kernel.org/qemu-devel/20240219061731.232570-1-npiggin@gmail.com/
 accel = kvm
 
+[selftest-migration-skip]
+file = selftest-migration.elf
+machine = pseries
+groups = selftest migration
+extra_params = -append "skip"
+
 [spapr_hcall]
 file = spapr_hcall.elf
 
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index dac9e4db1..49e3e4608 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -31,6 +31,11 @@  groups = selftest migration
 # https://lore.kernel.org/qemu-devel/20240219061731.232570-1-npiggin@gmail.com/
 accel = kvm
 
+[selftest-migration-skip]
+file = selftest-migration.elf
+groups = selftest migration
+extra_params = -append "skip"
+
 [intercept]
 file = intercept.elf
 
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 39419d4e2..4a1aab48d 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -124,12 +124,17 @@  qmp_events ()
 
 filter_quiet_msgs ()
 {
-	grep -v "Now migrate the VM (quiet)"
+	grep -v "Now migrate the VM (quiet)" |
+	grep -v "Skipped VM migration (quiet)"
 }
 
 seen_migrate_msg ()
 {
-	grep -q -e "Now migrate the VM" < $1
+	if [ $skip_migration -eq 1 ]; then
+		grep -q -e "Now migrate the VM" < $1
+	else
+		grep -q -e "Now migrate the VM" -e "Skipped VM migration" < $1
+	fi
 }
 
 run_migration ()
@@ -142,7 +147,7 @@  run_migration ()
 	migcmdline=$@
 
 	trap 'trap - TERM ; kill 0 ; exit 2' INT TERM
-	trap 'rm -f ${src_out} ${dst_out} ${src_outfifo} ${dst_outfifo} ${dst_incoming} ${src_qmp} ${dst_qmp} ${dst_infifo}' RETURN EXIT
+	trap 'rm -f ${src_out} ${dst_out} ${src_outfifo} ${dst_outfifo} ${dst_incoming} ${src_qmp} ${dst_qmp} ${src_infifo} ${dst_infifo}' RETURN EXIT
 
 	dst_incoming=$(mktemp -u -t mig-helper-socket-incoming.XXXXXXXXXX)
 	src_out=$(mktemp -t mig-helper-stdout1.XXXXXXXXXX)
@@ -151,21 +156,26 @@  run_migration ()
 	dst_outfifo=$(mktemp -u -t mig-helper-fifo-stdout2.XXXXXXXXXX)
 	src_qmp=$(mktemp -u -t mig-helper-qmp1.XXXXXXXXXX)
 	dst_qmp=$(mktemp -u -t mig-helper-qmp2.XXXXXXXXXX)
-	dst_infifo=$(mktemp -u -t mig-helper-fifo-stdin.XXXXXXXXXX)
+	src_infifo=$(mktemp -u -t mig-helper-fifo-stdin1.XXXXXXXXXX)
+	dst_infifo=$(mktemp -u -t mig-helper-fifo-stdin2.XXXXXXXXXX)
 	src_qmpout=/dev/null
 	dst_qmpout=/dev/null
+	skip_migration=0
 
 	mkfifo ${src_outfifo}
 	mkfifo ${dst_outfifo}
 
 	# Holding both ends of the input fifo open prevents opens from
 	# blocking and readers getting EOF when a writer closes it.
+	mkfifo ${src_infifo}
 	mkfifo ${dst_infifo}
+	exec {src_infifo_fd}<>${src_infifo}
 	exec {dst_infifo_fd}<>${dst_infifo}
 
 	eval "$migcmdline" \
 		-chardev socket,id=mon,path=${src_qmp},server=on,wait=off \
-		-mon chardev=mon,mode=control > ${src_outfifo} &
+		-mon chardev=mon,mode=control \
+		< ${src_infifo} > ${src_outfifo} &
 	live_pid=$!
 	cat ${src_outfifo} | tee ${src_out} | filter_quiet_msgs &
 
@@ -179,8 +189,11 @@  run_migration ()
 		# Wait for test exit or further migration messages.
 		if ! seen_migrate_msg ${src_out} ;  then
 			sleep 0.1
-		else
+		elif grep -q "Now migrate the VM" < ${src_out} ; then
 			do_migration || return $?
+		elif [ $skip_migration -eq 0 ] && grep -q "Skipped VM migration" < ${src_out} ; then
+			echo > ${src_infifo} # Resume src and carry on.
+			break;
 		fi
 	done
 
@@ -207,10 +220,10 @@  do_migration ()
 	# "Now migrate VM" or similar console message.
 	while ! seen_migrate_msg ${src_out} ; do
 		if ! ps -p ${live_pid} > /dev/null ; then
-			echo "ERROR: Test exit before migration point." >&2
 			echo > ${dst_infifo}
-			qmp ${src_qmp} '"quit"'> ${src_qmpout} 2>/dev/null
 			qmp ${dst_qmp} '"quit"'> ${dst_qmpout} 2>/dev/null
+			echo "ERROR: Test exit before migration point." >&2
+			qmp ${src_qmp} '"quit"'> ${src_qmpout} 2>/dev/null
 			return 3
 		fi
 		sleep 0.1
@@ -220,6 +233,15 @@  do_migration ()
 	while ! [ -S ${dst_incoming} ] ; do sleep 0.1 ; done
 	while ! [ -S ${dst_qmp} ] ; do sleep 0.1 ; done
 
+	if [ $skip_migration -eq 0 ] && grep -q "Skipped VM migration" < ${src_out} ; then
+		# May not get any migrations, exit to main loop for now...
+		echo > ${dst_infifo}
+		qmp ${dst_qmp} '"quit"'> ${dst_qmpout} 2>/dev/null
+		echo > ${src_infifo} # Resume src and carry on.
+		skip_migration=1
+		return 0
+	fi
+
 	qmp ${src_qmp} '"migrate", "arguments": { "uri": "unix:'${dst_incoming}'" }' > ${src_qmpout}
 
 	# Wait for the migration to complete
@@ -259,6 +281,9 @@  do_migration ()
 	tmp=${src_out}
 	src_out=${dst_out}
 	dst_out=${tmp}
+	tmp=${src_infifo}
+	src_infifo=${dst_infifo}
+	dst_infifo=${tmp}
 	tmp=${src_outfifo}
 	src_outfifo=${dst_outfifo}
 	dst_outfifo=${tmp}