Re: [Qemu-devel] [PATCH v4 31/32] migration, qmp: new command "migrate-pause"

[Peter Xu]

On Fri, Dec 01, 2017 at 04:53:28PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (address@hidden) wrote:
> > It is used to manually trigger the postcopy pause state.  It works just
> > like when we found the migration stream failed during postcopy, but
> > provide an explicit way for user in case of misterious socket hangs.
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> 
> Can we change the name to something like 'migrate-disconnect' - pause
> is a bit easy to confuse with other things and this is really more
> an explicit network disconnect (Is it worth just making it a flag to
> migrate-cancel?)

Then I would prefer to reuse the migrate_cancel command.  

Actually this reminded me about what would happen now if someone on
src VM sends a "migrate_cancel" during postcopy active.  It should
crash the VM, right?

Considering above, I'm thinking whether we should just make it a
default behavior that when do migrate_cancel during postcopy-active we
just do a pause instead of real cancel. After all it cannot re-start
the VM any more on source, so IMHO a real cancel does not mean much
here.  More importantly, what if someone wants to manually trigger
this pause but accidentally forgot to type that new flag (say,
-D[isconnect])?  It'll crash the VM directly.

What do you think?

> 
> 
> > ---
> >  migration/migration.c | 18 ++++++++++++++++++
> >  qapi/migration.json   | 22 ++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 536a771803..30348a5e27 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1485,6 +1485,24 @@ void qmp_migrate_incoming(const char *uri, Error 
> > **errp)
> >      once = false;
> >  }
> >  
> > +void qmp_migrate_pause(Error **errp)
> > +{
> > +    int ret;
> > +    MigrationState *ms = migrate_get_current();
> > +
> > +    if (ms->state != MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> > +        error_setg(errp, "Migration pause is currently only allowed during"
> > +                   " an active postcopy phase.");
> > +        return;
> > +    }
> > +
> > +    ret = qemu_file_shutdown(ms->to_dst_file);
> > +
> > +    if (ret) {
> > +        error_setg(errp, "Failed to pause migration stream.");
> > +    }
> > +}
> > +
> >  bool migration_is_blocked(Error **errp)
> >  {
> >      if (qemu_savevm_state_blocked(errp)) {
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 4a3eff62f1..52901f7e2e 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1074,6 +1074,28 @@
> >  { 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
> >  
> >  ##
> > +# @migrate-pause:
> > +#
> > +# Pause an migration.  Currently it can only pause a postcopy
> > +# migration.  Pausing a precopy migration is not supported yet.
> > +#
> > +# It is mostly used as a manual way to trigger the postcopy paused
> > +# state when the network sockets hang due to some reason, so that we
> > +# can try a recovery afterward.
> 
> Can we say this explicitly;
> 'Force closes the migration connection to trigger the postcopy paused
>  state when the network sockets hang due to some reason, so that we
> can try a recovery afterwards'

Sure!  I'll just see where I should properly put these sentences.

Thanks,

-- 
Peter Xu