[PATCH] runstate: ignore exit request in finish migrate state

[PATCH] runstate: ignore exit request in finish migrate state

[Laurent Vivier]

Trying to reboot a VM while a migration is running can
move to the prelaunch state (because of the reset) while
the runstate is in finish migrate state.
As the logical step after the finish migrate is postmigrate,
this can create an invalid state transition from prelaunch state
to postmigrate state and this raises an error and aborts:

    invalid runstate transition: 'prelaunch' -> 'postmigrate'

As we are not able to manage reset in finish migrate state the
best we can do is to ignore any changes and delay them until
the next state which should be postmigrate and which should allow
this kind of transition.

Reported-by: Lukáš Doktor <ldoktor@redhat.com>
Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 vl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/vl.c b/vl.c
index 0a295e5d77d6..dc71c822ba24 100644
--- a/vl.c
+++ b/vl.c
@@ -1744,6 +1744,9 @@ static bool main_loop_should_exit(void)
     RunState r;
     ShutdownCause request;
 
+    if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
+        return false;
+    }
     if (preconfig_exit_requested) {
         if (runstate_check(RUN_STATE_PRECONFIG)) {
             runstate_set(RUN_STATE_PRELAUNCH);
-- 
2.21.0

Re: [PATCH] runstate: ignore exit request in finish migrate state

[Paolo Bonzini]

On 17/10/19 12:18, Laurent Vivier wrote:
> Trying to reboot a VM while a migration is running can
> move to the prelaunch state (because of the reset) while
> the runstate is in finish migrate state.
> As the logical step after the finish migrate is postmigrate,
> this can create an invalid state transition from prelaunch state
> to postmigrate state and this raises an error and aborts:
> 
>     invalid runstate transition: 'prelaunch' -> 'postmigrate'
> 
> As we are not able to manage reset in finish migrate state the
> best we can do is to ignore any changes and delay them until
> the next state which should be postmigrate and which should allow
> this kind of transition.
> 
> Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  vl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index 0a295e5d77d6..dc71c822ba24 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1744,6 +1744,9 @@ static bool main_loop_should_exit(void)
>      RunState r;
>      ShutdownCause request;
>  
> +    if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
> +        return false;
> +    }
>      if (preconfig_exit_requested) {
>          if (runstate_check(RUN_STATE_PRECONFIG)) {
>              runstate_set(RUN_STATE_PRELAUNCH);
> 

Your patch makes sense, but I don't understand this function very much.
 In particular I don't understand why it returns true when
preconfig_exit_requested is true.  Wouldn't that cause main_loop() and
thus QEMU to exit?  Igor, can you help?

Paolo

Re: [PATCH] runstate: ignore exit request in finish migrate state

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 17/10/19 12:18, Laurent Vivier wrote:
> > Trying to reboot a VM while a migration is running can
> > move to the prelaunch state (because of the reset) while
> > the runstate is in finish migrate state.
> > As the logical step after the finish migrate is postmigrate,
> > this can create an invalid state transition from prelaunch state
> > to postmigrate state and this raises an error and aborts:
> > 
> >     invalid runstate transition: 'prelaunch' -> 'postmigrate'
> > 
> > As we are not able to manage reset in finish migrate state the
> > best we can do is to ignore any changes and delay them until
> > the next state which should be postmigrate and which should allow
> > this kind of transition.
> > 
> > Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  vl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/vl.c b/vl.c
> > index 0a295e5d77d6..dc71c822ba24 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1744,6 +1744,9 @@ static bool main_loop_should_exit(void)
> >      RunState r;
> >      ShutdownCause request;
> >  
> > +    if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
> > +        return false;
> > +    }
> >      if (preconfig_exit_requested) {
> >          if (runstate_check(RUN_STATE_PRECONFIG)) {
> >              runstate_set(RUN_STATE_PRELAUNCH);
> > 
> 
> Your patch makes sense, but I don't understand this function very much.
>  In particular I don't understand why it returns true when
> preconfig_exit_requested is true.  Wouldn't that cause main_loop() and
> thus QEMU to exit?  Igor, can you help?

It's because we now run main_loop() twice; once in the preconfig state
and once the main main loop.


4323     /* do monitor/qmp handling at preconfig state if requested */
4324     main_loop();

....

4452     os_setup_post();
4453 
4454     main_loop();
4455 
4456     gdbserver_cleanup();

> Paolo

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] runstate: ignore exit request in finish migrate state

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> On 17/10/19 12:18, Laurent Vivier wrote:
>> > Trying to reboot a VM while a migration is running can
>> > move to the prelaunch state (because of the reset) while
>> > the runstate is in finish migrate state.
>> > As the logical step after the finish migrate is postmigrate,
>> > this can create an invalid state transition from prelaunch state
>> > to postmigrate state and this raises an error and aborts:
>> > 
>> >     invalid runstate transition: 'prelaunch' -> 'postmigrate'
>> > 
>> > As we are not able to manage reset in finish migrate state the
>> > best we can do is to ignore any changes and delay them until
>> > the next state which should be postmigrate and which should allow
>> > this kind of transition.
>> > 
>> > Reported-by: Lukáš Doktor <ldoktor@redhat.com>
>> > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> > ---
>> >  vl.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> > 
>> > diff --git a/vl.c b/vl.c
>> > index 0a295e5d77d6..dc71c822ba24 100644
>> > --- a/vl.c
>> > +++ b/vl.c
>> > @@ -1744,6 +1744,9 @@ static bool main_loop_should_exit(void)
>> >      RunState r;
>> >      ShutdownCause request;
>> >  
>> > +    if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
>> > +        return false;
>> > +    }
>> >      if (preconfig_exit_requested) {
>> >          if (runstate_check(RUN_STATE_PRECONFIG)) {
>> >              runstate_set(RUN_STATE_PRELAUNCH);
>> > 
>> 
>> Your patch makes sense, but I don't understand this function very much.
>>  In particular I don't understand why it returns true when
>> preconfig_exit_requested is true.  Wouldn't that cause main_loop() and
>> thus QEMU to exit?  Igor, can you help?
>
> It's because we now run main_loop() twice; once in the preconfig state
> and once the main main loop.
>
>
> 4323     /* do monitor/qmp handling at preconfig state if requested */
> 4324     main_loop();
>
> ....
>
> 4452     os_setup_post();
> 4453 
> 4454     main_loop();
> 4455 
> 4456     gdbserver_cleanup();

Commit 047f7038f5 "cli: add --preconfig option"

The whole thing is (in my opinion) a cheap hack we accepted for want of
better ideas we could get done in time.  Sometimes you have to hold you
nose and deliver.

https://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg05584.html

Re: [PATCH] runstate: ignore exit request in finish migrate state

[Paolo Bonzini]

On 17/10/19 12:18, Laurent Vivier wrote:
> Trying to reboot a VM while a migration is running can
> move to the prelaunch state (because of the reset) while
> the runstate is in finish migrate state.
> As the logical step after the finish migrate is postmigrate,
> this can create an invalid state transition from prelaunch state
> to postmigrate state and this raises an error and aborts:
> 
>     invalid runstate transition: 'prelaunch' -> 'postmigrate'
> 
> As we are not able to manage reset in finish migrate state the
> best we can do is to ignore any changes and delay them until
> the next state which should be postmigrate and which should allow
> this kind of transition.
> 
> Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Queued, thanks.

Paolo

> ---
>  vl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index 0a295e5d77d6..dc71c822ba24 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1744,6 +1744,9 @@ static bool main_loop_should_exit(void)
>      RunState r;
>      ShutdownCause request;
>  
> +    if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
> +        return false;
> +    }
>      if (preconfig_exit_requested) {
>          if (runstate_check(RUN_STATE_PRECONFIG)) {
>              runstate_set(RUN_STATE_PRELAUNCH);
>