[RFC,1/2] migrate: Allow incoming migration without defer

Allow a QEMU instance which has been started and used without the "-incoming"
flag to accept an incoming migration with the "migrate-incoming" QMP
command. This allows the user to dump the VM state to an external file then
revert to that state at a later time without restarting QEMU.
---
 migration/migration.c | 12 +++++-------
 vl.c                  |  2 ++
 2 files changed, 7 insertions(+), 7 deletions(-)

* Richard Palethorpe (rpalethorpe@suse.com) wrote:
> Allow a QEMU instance which has been started and used without the "-incoming"
> flag to accept an incoming migration with the "migrate-incoming" QMP
> command. This allows the user to dump the VM state to an external file then
> revert to that state at a later time without restarting QEMU.
> ---
>  migration/migration.c | 12 +++++-------
>  vl.c                  |  2 ++
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0aa596f867..05595a4cec 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1321,14 +1321,14 @@ void migrate_del_blocker(Error *reason)
>  void qmp_migrate_incoming(const char *uri, Error **errp)
>  {
>      Error *local_err = NULL;
> -    static bool once = true;
>  
> -    if (!deferred_incoming) {
> -        error_setg(errp, "For use with '-incoming defer'");
> +    if (runstate_check(RUN_STATE_INMIGRATE)) {
> +        error_setg(errp, "The incoming migration has already been started");
>          return;
>      }
> -    if (!once) {
> -        error_setg(errp, "The incoming migration has already been started");
> +
> +    if (!deferred_incoming) {
> +        vm_stop(RUN_STATE_INMIGRATE);
>      }
>  
>      qemu_start_incoming_migration(uri, &local_err);
> @@ -1337,8 +1337,6 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    once = false;
>  }

That's...interesting.  I think it will work, but I bet there's a whole
bunch of corner cases [many of which loadvm will hit as well!].
For starters there's probably devices that are active and still looking
at RAM, even though vm_stop is in place (hopefully none of them are
writing to it, but I wouldn't actually trust them).  Also, you probably
need to inactivate the block devices?

Also, I think this means there's no protection against this happening
during an outgoing migration (which has an existing check to make sure
you can't start an outgoing migration while an incoming one is waiting).

>  bool migration_is_blocked(Error **errp)
> diff --git a/vl.c b/vl.c
> index 9e7235df6d..a91eda039e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -634,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
>      { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
>      { RUN_STATE_PAUSED, RUN_STATE_COLO},
> +    { RUN_STATE_PAUSED, RUN_STATE_INMIGRATE },
>  
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
> @@ -665,6 +666,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
>      { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
>      { RUN_STATE_RUNNING, RUN_STATE_COLO},
> +    { RUN_STATE_RUNNING, RUN_STATE_INMIGRATE },

Experience suggests that you'll find out the hard way that there's other
states you'll need to allow or check for.
For example, do you want to be able to loadvm from a GUEST_PANICKED or
PRELAUNCH state?

Dave

>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>  
> -- 
> 2.16.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

hello David,

Dr. David Alan Gilbert writes:

> * Richard Palethorpe (rpalethorpe@suse.com) wrote:
>> Allow a QEMU instance which has been started and used without the "-incoming"
>> flag to accept an incoming migration with the "migrate-incoming" QMP
>> command. This allows the user to dump the VM state to an external file then
>> revert to that state at a later time without restarting QEMU.
>> ---
>>  migration/migration.c | 12 +++++-------
>>  vl.c                  |  2 ++
>>  2 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 0aa596f867..05595a4cec 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1321,14 +1321,14 @@ void migrate_del_blocker(Error *reason)
>>  void qmp_migrate_incoming(const char *uri, Error **errp)
>>  {
>>      Error *local_err = NULL;
>> -    static bool once = true;
>>
>> -    if (!deferred_incoming) {
>> -        error_setg(errp, "For use with '-incoming defer'");
>> +    if (runstate_check(RUN_STATE_INMIGRATE)) {
>> +        error_setg(errp, "The incoming migration has already been started");
>>          return;
>>      }
>> -    if (!once) {
>> -        error_setg(errp, "The incoming migration has already been started");
>> +
>> +    if (!deferred_incoming) {
>> +        vm_stop(RUN_STATE_INMIGRATE);
>>      }
>>
>>      qemu_start_incoming_migration(uri, &local_err);
>> @@ -1337,8 +1337,6 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>> -
>> -    once = false;
>>  }
>
> That's...interesting.  I think it will work, but I bet there's a whole
> bunch of corner cases [many of which loadvm will hit as well!].

It wouldn't surprise me if we have hit some of those corner cases with
our extensive use of loadvm :-) Unfortunately I don't have much data to
show what went wrong though.

> For starters there's probably devices that are active and still looking
> at RAM, even though vm_stop is in place (hopefully none of them are
> writing to it, but I wouldn't actually trust them).

I suppose that might be a showstopper.

> Also, you probably
> need to inactivate the block devices?

They are at least flushed by vm_stop. I would expect the user to
recreate them, or at least recreate the active layer before starting the
migration.

>
> Also, I think this means there's no protection against this happening
> during an outgoing migration (which has an existing check to make sure
> you can't start an outgoing migration while an incoming one is waiting).
>
>>  bool migration_is_blocked(Error **errp)
>> diff --git a/vl.c b/vl.c
>> index 9e7235df6d..a91eda039e 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -634,6 +634,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>>      { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
>>      { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
>>      { RUN_STATE_PAUSED, RUN_STATE_COLO},
>> +    { RUN_STATE_PAUSED, RUN_STATE_INMIGRATE },
>>
>>      { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
>>      { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
>> @@ -665,6 +666,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>>      { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
>>      { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
>>      { RUN_STATE_RUNNING, RUN_STATE_COLO},
>> +    { RUN_STATE_RUNNING, RUN_STATE_INMIGRATE },
>
> Experience suggests that you'll find out the hard way that there's other
> states you'll need to allow or check for.
> For example, do you want to be able to loadvm from a GUEST_PANICKED or
> PRELAUNCH state?

Yes, I would add GUEST_PANICKED at least.

>
> Dave
>
>>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>>
>> --
>> 2.16.2
>>


--
Thank you,
Richard.