[kvm-unit-tests PATCH 0/3] migration: fixes and multiple migration

[kvm-unit-tests PATCH 0/3] migration: fixes and multiple migration

[Nicholas Piggin]

Spent too long looking at bash code after posting previous RFC, but
got it into shape.

The first 2 seem to be real bugs you can trigger by making a test
case fail. Third IMO is pretty solid now but no users yet so I can
keep it around and resubmit with a user some time later.

Thanks,
Nick

Nicholas Piggin (3):
  migration: Fix test harness hang on fifo
  migration: Fix test harness hang if source does not reach migration
    point
  arch-run: Support multiple migrations

 lib/migrate.c         |  8 +++---
 lib/migrate.h         |  1 +
 scripts/arch-run.bash | 65 ++++++++++++++++++++++++++++++++++++-------
 3 files changed, 60 insertions(+), 14 deletions(-)

-- 
2.40.1

[kvm-unit-tests PATCH 1/3] migration: Fix test harness hang on fifo

[Nicholas Piggin]

If the test fails before migration is complete, the input fifo for the
destination machine is not written to and that can leave cat waiting
for input.

Clear that condition in the error handler so the harness doesn't hang
in this case.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/arch-run.bash | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 5e7e4201..518607f4 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -165,6 +165,7 @@ run_migration ()
 		migstatus=`qmp ${qmp1} '"query-migrate"' | grep return`
 		if grep -q '"failed"' <<<"$migstatus" ; then
 			echo "ERROR: Migration failed." >&2
+			echo > ${fifo}
 			qmp ${qmp1} '"quit"'> ${qmpout1} 2>/dev/null
 			qmp ${qmp2} '"quit"'> ${qmpout2} 2>/dev/null
 			return 2
-- 
2.40.1

Re: [kvm-unit-tests PATCH 1/3] migration: Fix test harness hang on fifo

[Nico Boehr]

Quoting Nicholas Piggin (2023-07-25 05:39:35)
> If the test fails before migration is complete, the input fifo for the
> destination machine is not written to and that can leave cat waiting
> for input.
> 
> Clear that condition in the error handler so the harness doesn't hang
> in this case.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

[kvm-unit-tests PATCH 2/3] migration: Fix test harness hang if source does not reach migration point

[Nicholas Piggin]

After starting the test, the harness waits polling for "migrate" in the
output. If the test does not print for some reason, the harness hangs.

Test that the pid is still alive while polling to fix this hang.

While here, wait for the full string "Now migrate the VM", which I think
makes it more obvious to read and could avoid an unfortunate collision
with some debugging output in a test case.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 scripts/arch-run.bash | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 518607f4..30e535c7 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -142,6 +142,7 @@ run_migration ()
 
 	eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
 		-mon chardev=mon1,mode=control | tee ${migout1} &
+	live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
 
 	# We have to use cat to open the named FIFO, because named FIFO's, unlike
 	# pipes, will block on open() until the other end is also opened, and that
@@ -152,7 +153,14 @@ run_migration ()
 	incoming_pid=`jobs -l %+ | awk '{print$2}'`
 
 	# The test must prompt the user to migrate, so wait for the "migrate" keyword
-	while ! grep -q -i "migrate" < ${migout1} ; do
+	while ! grep -q -i "Now migrate the VM" < ${migout1} ; do
+		if ! ps -p ${live_pid} > /dev/null ; then
+			echo "ERROR: Test exit before migration point." >&2
+			echo > ${fifo}
+			qmp ${qmp1} '"quit"'> ${qmpout1} 2>/dev/null
+			qmp ${qmp2} '"quit"'> ${qmpout2} 2>/dev/null
+			return 3
+		fi
 		sleep 1
 	done
 
-- 
2.40.1

Re: [kvm-unit-tests PATCH 2/3] migration: Fix test harness hang if source does not reach migration point

[Nico Boehr]

Quoting Nicholas Piggin (2023-07-25 05:39:36)
> After starting the test, the harness waits polling for "migrate" in the
> output. If the test does not print for some reason, the harness hangs.
> 
> Test that the pid is still alive while polling to fix this hang.
> 
> While here, wait for the full string "Now migrate the VM", which I think
> makes it more obvious to read and could avoid an unfortunate collision
> with some debugging output in a test case.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Thanks for attempting to fix this!

> ---
>  scripts/arch-run.bash | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index 518607f4..30e535c7 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -142,6 +142,7 @@ run_migration ()
>  
>         eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
>                 -mon chardev=mon1,mode=control | tee ${migout1} &
> +       live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`

Pardon my ignorance, but why would $! not work here?

Re: [kvm-unit-tests PATCH 2/3] migration: Fix test harness hang if source does not reach migration point

[Nicholas Piggin]

On Fri Jul 28, 2023 at 5:34 PM AEST, Nico Boehr wrote:
> Quoting Nicholas Piggin (2023-07-25 05:39:36)
> > After starting the test, the harness waits polling for "migrate" in the
> > output. If the test does not print for some reason, the harness hangs.
> > 
> > Test that the pid is still alive while polling to fix this hang.
> > 
> > While here, wait for the full string "Now migrate the VM", which I think
> > makes it more obvious to read and could avoid an unfortunate collision
> > with some debugging output in a test case.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
> Thanks for attempting to fix this!
>
> > ---
> >  scripts/arch-run.bash | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> > index 518607f4..30e535c7 100644
> > --- a/scripts/arch-run.bash
> > +++ b/scripts/arch-run.bash
> > @@ -142,6 +142,7 @@ run_migration ()
> >  
> >         eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
> >                 -mon chardev=mon1,mode=control | tee ${migout1} &
> > +       live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
>
> Pardon my ignorance, but why would $! not work here?

My mastery of bash is poor, I copied the incoming_pid line. It seems
to work, but if you think $! is better I can try it.

Thanks,
Nick

Re: [kvm-unit-tests PATCH 2/3] migration: Fix test harness hang if source does not reach migration point

[Nico Boehr]

Quoting Nicholas Piggin (2023-07-30 12:03:36)
> On Fri Jul 28, 2023 at 5:34 PM AEST, Nico Boehr wrote:
> > Quoting Nicholas Piggin (2023-07-25 05:39:36)
> > > After starting the test, the harness waits polling for "migrate" in the
> > > output. If the test does not print for some reason, the harness hangs.
> > > 
> > > Test that the pid is still alive while polling to fix this hang.
> > > 
> > > While here, wait for the full string "Now migrate the VM", which I think
> > > makes it more obvious to read and could avoid an unfortunate collision
> > > with some debugging output in a test case.
> > > 
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >
> > Thanks for attempting to fix this!
> >
> > > ---
> > >  scripts/arch-run.bash | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> > > index 518607f4..30e535c7 100644
> > > --- a/scripts/arch-run.bash
> > > +++ b/scripts/arch-run.bash
> > > @@ -142,6 +142,7 @@ run_migration ()
> > >  
> > >         eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
> > >                 -mon chardev=mon1,mode=control | tee ${migout1} &
> > > +       live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
> >
> > Pardon my ignorance, but why would $! not work here?
> 
> My mastery of bash is poor, I copied the incoming_pid line. It seems
> to work, but if you think $! is better I can try it.

Sorry, this fell off of my radar after going to summer holiday...

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 2/3] migration: Fix test harness hang if source does not reach migration point

[Thomas Huth]

On 25/09/2023 13.14, Nico Boehr wrote:
> Quoting Nicholas Piggin (2023-07-30 12:03:36)
>> On Fri Jul 28, 2023 at 5:34 PM AEST, Nico Boehr wrote:
>>> Quoting Nicholas Piggin (2023-07-25 05:39:36)
>>>> After starting the test, the harness waits polling for "migrate" in the
>>>> output. If the test does not print for some reason, the harness hangs.
>>>>
>>>> Test that the pid is still alive while polling to fix this hang.
>>>>
>>>> While here, wait for the full string "Now migrate the VM", which I think
>>>> makes it more obvious to read and could avoid an unfortunate collision
>>>> with some debugging output in a test case.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>
>>> Thanks for attempting to fix this!
>>>
>>>> ---
>>>>   scripts/arch-run.bash | 10 +++++++++-
>>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
>>>> index 518607f4..30e535c7 100644
>>>> --- a/scripts/arch-run.bash
>>>> +++ b/scripts/arch-run.bash
>>>> @@ -142,6 +142,7 @@ run_migration ()
>>>>   
>>>>          eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
>>>>                  -mon chardev=mon1,mode=control | tee ${migout1} &
>>>> +       live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
>>>
>>> Pardon my ignorance, but why would $! not work here?
>>
>> My mastery of bash is poor, I copied the incoming_pid line. It seems
>> to work, but if you think $! is better I can try it.
> 
> Sorry, this fell off of my radar after going to summer holiday...
> 
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

  Hi Nicholas & Nico,

do you want me to pick up this patch as is, or do you want to respin with $! 
instead?

  Thomas

Re: [kvm-unit-tests PATCH 2/3] migration: Fix test harness hang if source does not reach migration point

[Nico Boehr]

Quoting Thomas Huth (2023-10-16 20:32:47)
> On 25/09/2023 13.14, Nico Boehr wrote:
> > Quoting Nicholas Piggin (2023-07-30 12:03:36)
> >> On Fri Jul 28, 2023 at 5:34 PM AEST, Nico Boehr wrote:
> >>> Quoting Nicholas Piggin (2023-07-25 05:39:36)
> >>>> After starting the test, the harness waits polling for "migrate" in the
> >>>> output. If the test does not print for some reason, the harness hangs.
> >>>>
> >>>> Test that the pid is still alive while polling to fix this hang.
> >>>>
> >>>> While here, wait for the full string "Now migrate the VM", which I think
> >>>> makes it more obvious to read and could avoid an unfortunate collision
> >>>> with some debugging output in a test case.
> >>>>
> >>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >>>
> >>> Thanks for attempting to fix this!
> >>>
> >>>> ---
> >>>>   scripts/arch-run.bash | 10 +++++++++-
> >>>>   1 file changed, 9 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> >>>> index 518607f4..30e535c7 100644
> >>>> --- a/scripts/arch-run.bash
> >>>> +++ b/scripts/arch-run.bash
> >>>> @@ -142,6 +142,7 @@ run_migration ()
> >>>>   
> >>>>          eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
> >>>>                  -mon chardev=mon1,mode=control | tee ${migout1} &
> >>>> +       live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
> >>>
> >>> Pardon my ignorance, but why would $! not work here?
> >>
> >> My mastery of bash is poor, I copied the incoming_pid line. It seems
> >> to work, but if you think $! is better I can try it.
> > 
> > Sorry, this fell off of my radar after going to summer holiday...
> > 
> > Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
> 
>   Hi Nicholas & Nico,
> 
> do you want me to pick up this patch as is, or do you want to respin with $! 
> instead?

Let's not discuss too much and get this fixed, I am fine with this as-is.
Thanks.

Re: [kvm-unit-tests PATCH 2/3] migration: Fix test harness hang if source does not reach migration point

[Thomas Huth]

On 19/10/2023 09.55, Nico Boehr wrote:
> Quoting Thomas Huth (2023-10-16 20:32:47)
>> On 25/09/2023 13.14, Nico Boehr wrote:
>>> Quoting Nicholas Piggin (2023-07-30 12:03:36)
>>>> On Fri Jul 28, 2023 at 5:34 PM AEST, Nico Boehr wrote:
>>>>> Quoting Nicholas Piggin (2023-07-25 05:39:36)
>>>>>> After starting the test, the harness waits polling for "migrate" in the
>>>>>> output. If the test does not print for some reason, the harness hangs.
>>>>>>
>>>>>> Test that the pid is still alive while polling to fix this hang.
>>>>>>
>>>>>> While here, wait for the full string "Now migrate the VM", which I think
>>>>>> makes it more obvious to read and could avoid an unfortunate collision
>>>>>> with some debugging output in a test case.
>>>>>>
>>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>>
>>>>> Thanks for attempting to fix this!
>>>>>
>>>>>> ---
>>>>>>    scripts/arch-run.bash | 10 +++++++++-
>>>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
>>>>>> index 518607f4..30e535c7 100644
>>>>>> --- a/scripts/arch-run.bash
>>>>>> +++ b/scripts/arch-run.bash
>>>>>> @@ -142,6 +142,7 @@ run_migration ()
>>>>>>    
>>>>>>           eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
>>>>>>                   -mon chardev=mon1,mode=control | tee ${migout1} &
>>>>>> +       live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
>>>>>
>>>>> Pardon my ignorance, but why would $! not work here?
>>>>
>>>> My mastery of bash is poor, I copied the incoming_pid line. It seems
>>>> to work, but if you think $! is better I can try it.
>>>
>>> Sorry, this fell off of my radar after going to summer holiday...
>>>
>>> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
>>
>>    Hi Nicholas & Nico,
>>
>> do you want me to pick up this patch as is, or do you want to respin with $!
>> instead?
> 
> Let's not discuss too much and get this fixed, I am fine with this as-is.
> Thanks.

Ok, pushed it now.

  Thomas

[kvm-unit-tests PATCH 3/3] arch-run: Support multiple migrations

[Nicholas Piggin]

Support multiple migrations by flipping dest file/socket variables to
source after the migration is complete, ready to start again. A new
destination is created if the test outputs the migrate line again.
Test cases may now switch to calling migrate() one or more times.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/migrate.c         |  8 +++----
 lib/migrate.h         |  1 +
 scripts/arch-run.bash | 54 +++++++++++++++++++++++++++++++++++--------
 3 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/lib/migrate.c b/lib/migrate.c
index 527e63ae..b7721659 100644
--- a/lib/migrate.c
+++ b/lib/migrate.c
@@ -8,8 +8,10 @@
 #include <libcflat.h>
 #include "migrate.h"
 
-/* static for now since we only support migrating exactly once per test. */
-static void migrate(void)
+/*
+ * Initiate migration and wait for it to complete.
+ */
+void migrate(void)
 {
 	puts("Now migrate the VM, then press a key to continue...\n");
 	(void)getchar();
@@ -19,8 +21,6 @@ static void migrate(void)
 /*
  * Initiate migration and wait for it to complete.
  * If this function is called more than once, it is a no-op.
- * Since migrate_cmd can only migrate exactly once this function can
- * simplify the control flow, especially when skipping tests.
  */
 void migrate_once(void)
 {
diff --git a/lib/migrate.h b/lib/migrate.h
index 3c94e6af..2af06a72 100644
--- a/lib/migrate.h
+++ b/lib/migrate.h
@@ -6,4 +6,5 @@
  * Author: Nico Boehr <nrb@linux.ibm.com>
  */
 
+void migrate(void);
 void migrate_once(void);
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 30e535c7..e3155104 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -131,26 +131,55 @@ run_migration ()
 
 	migsock=$(mktemp -u -t mig-helper-socket.XXXXXXXXXX)
 	migout1=$(mktemp -t mig-helper-stdout1.XXXXXXXXXX)
+	migout2=$(mktemp -t mig-helper-stdout2.XXXXXXXXXX)
 	qmp1=$(mktemp -u -t mig-helper-qmp1.XXXXXXXXXX)
 	qmp2=$(mktemp -u -t mig-helper-qmp2.XXXXXXXXXX)
 	fifo=$(mktemp -u -t mig-helper-fifo.XXXXXXXXXX)
 	qmpout1=/dev/null
 	qmpout2=/dev/null
+	migcmdline=$@
 
 	trap 'kill 0; exit 2' INT TERM
-	trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT
+	trap 'rm -f ${migout1} ${migout2} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT
 
-	eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
+	eval "$migcmdline" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \
 		-mon chardev=mon1,mode=control | tee ${migout1} &
 	live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'`
 
+	# This starts the first source QEMU in advance of the test reaching the
+	# migration point, since we expect at least one migration. Subsequent
+	# sources are started as the test hits migrate keywords.
+	do_migration || return $?
+
+	while ps -p ${live_pid} > /dev/null ; do
+		# Wait for EXIT or further migrations
+		if ! grep -q -i "Now migrate the VM" < ${migout1} ; then
+			sleep 0.5
+		else
+			do_migration || return $?
+		fi
+	done
+
+	wait ${live_pid}
+	ret=$?
+
+	while (( $(jobs -r | wc -l) > 0 )); do
+		sleep 0.5
+	done
+
+	return $ret
+}
+
+do_migration ()
+{
 	# We have to use cat to open the named FIFO, because named FIFO's, unlike
 	# pipes, will block on open() until the other end is also opened, and that
 	# totally breaks QEMU...
 	mkfifo ${fifo}
-	eval "$@" -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \
-		-mon chardev=mon2,mode=control -incoming unix:${migsock} < <(cat ${fifo}) &
-	incoming_pid=`jobs -l %+ | awk '{print$2}'`
+
+	eval "$migcmdline" -chardev socket,id=mon2,path=${qmp2},server=on,wait=off \
+		-mon chardev=mon2,mode=control -incoming unix:${migsock} < <(cat ${fifo}) | tee ${migout2} &
+	incoming_pid=`jobs -l %+ | grep Running | awk '{print$2}'`
 
 	# The test must prompt the user to migrate, so wait for the "migrate" keyword
 	while ! grep -q -i "Now migrate the VM" < ${migout1} ; do
@@ -181,12 +210,19 @@ run_migration ()
 	done
 	qmp ${qmp1} '"quit"'> ${qmpout1} 2>/dev/null
 	echo > ${fifo}
-	wait $incoming_pid
+	rm ${fifo}
+
+	wait ${live_pid}
 	ret=$?
 
-	while (( $(jobs -r | wc -l) > 0 )); do
-		sleep 0.5
-	done
+	# Now flip the variables because dest becomes source
+	live_pid=${incoming_pid}
+	tmp=${migout1}
+	migout1=${migout2}
+	migout2=${tmp}
+	tmp=${qmp1}
+	qmp1=${qmp2}
+	qmp2=${tmp}
 
 	return $ret
 }
-- 
2.40.1

Re: [kvm-unit-tests PATCH 0/3] migration: fixes and multiple migration

[Shaoqin Huang]

Hi Nicholas,

With the patch3 arch-run: Support multiple migrations.

The run_test.sh will fail on the migration test on arm64 platform. But 
the patch1 and patch2 looks good, only apply these two patches the 
migration test works good.

I haven't had time to investigate it. But first let you know that.

Thanks,
Shaoqin

On 7/25/23 11:39, Nicholas Piggin wrote:
> Spent too long looking at bash code after posting previous RFC, but
> got it into shape.
> 
> The first 2 seem to be real bugs you can trigger by making a test
> case fail. Third IMO is pretty solid now but no users yet so I can
> keep it around and resubmit with a user some time later.
> 
> Thanks,
> Nick
> 
> Nicholas Piggin (3):
>    migration: Fix test harness hang on fifo
>    migration: Fix test harness hang if source does not reach migration
>      point
>    arch-run: Support multiple migrations
> 
>   lib/migrate.c         |  8 +++---
>   lib/migrate.h         |  1 +
>   scripts/arch-run.bash | 65 ++++++++++++++++++++++++++++++++++++-------
>   3 files changed, 60 insertions(+), 14 deletions(-)
> 

-- 
Shaoqin

Re: [kvm-unit-tests PATCH 0/3] migration: fixes and multiple migration

[Nicholas Piggin]

On Fri Jul 28, 2023 at 6:44 PM AEST, Shaoqin Huang wrote:
> Hi Nicholas,
>
> With the patch3 arch-run: Support multiple migrations.
>
> The run_test.sh will fail on the migration test on arm64 platform. But 
> the patch1 and patch2 looks good, only apply these two patches the 
> migration test works good.
>
> I haven't had time to investigate it. But first let you know that.

Hey, thanks for testing. I noticed some problem with it too. Let's
leave patch 3 out for now, I'll post an update with some users
added and hopefully bugs fixed.

Thanks,
Nick