Re: [Qemu-devel] [PATCH 2/3] Add migrate -u option for -incoming pause

[Dr. David Alan Gilbert]

* Daniel P. Berrange (address@hidden) wrote:
> On Tue, Feb 10, 2015 at 04:16:38PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > Once a qemu has been started with -incoming pause   the
> > migration can be started by issuing:
> > 
> > migrate -u uri
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  hmp-commands.hx       | 11 +++++++----
> >  hmp.c                 |  6 ++++--
> >  migration/migration.c | 21 ++++++++++++++++++++-
> >  qapi-schema.json      |  5 ++++-
> >  qmp-commands.hx       |  3 ++-
> >  5 files changed, 37 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index e37bc8b..45f293a 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -887,23 +887,26 @@ ETEXI
> >  
> >      {
> >          .name       = "migrate",
> > -        .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
> > -        .params     = "[-d] [-b] [-i] uri",
> > +        .args_type  = "detach:-d,blk:-b,inc:-i,unpause:-u,uri:s",
> > +        .params     = "[-d] [-b] [-i] [-u] uri",
> >          .help       = "migrate to URI (using -d to not wait for 
> > completion)"
> >                   "\n\t\t\t -b for migration without shared storage with"
> >                   " full copy of disk\n\t\t\t -i for migration without "
> >                   "shared storage with incremental copy of disk "
> > -                 "(base image shared between src and destination)",
> > +                 "(base image shared between src and destination)"
> > +                      "\n\t\t\t -u unpauses an incoming migration started 
> > with "
> > +                      "-incoming pause using the given uri.",
> >          .mhandler.cmd = hmp_migrate,
> >      },
> >  
> >  
> >  STEXI
> > address@hidden migrate [-d] [-b] [-i] @var{uri}
> > address@hidden migrate [-d] [-b] [-i] [-u] @var{uri}
> >  @findex migrate
> >  Migrate to @var{uri} (using -d to not wait for completion).
> >     -b for migration with full copy of disk
> >     -i for migration with incremental copy of disk (base image is shared)
> > +        -u to unpause an incoming migration started with -incoming pause
> >  ETEXI
> >  
> >      {
> > diff --git a/hmp.c b/hmp.c
> > index b47f331..fb0cde1 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1344,17 +1344,19 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
> >      int detach = qdict_get_try_bool(qdict, "detach", 0);
> >      int blk = qdict_get_try_bool(qdict, "blk", 0);
> >      int inc = qdict_get_try_bool(qdict, "inc", 0);
> > +    int unpause = qdict_get_try_bool(qdict, "unpause", 0);
> >      const char *uri = qdict_get_str(qdict, "uri");
> >      Error *err = NULL;
> >  
> > -    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, !!unpause, 
> > unpause,
> > +                &err);
> >      if (err) {
> >          monitor_printf(mon, "migrate: %s\n", error_get_pretty(err));
> >          error_free(err);
> >          return;
> >      }
> >  
> > -    if (!detach) {
> > +    if (!detach && !unpause) {
> >          MigrationStatus *status;
> >  
> >          if (monitor_suspend(mon) < 0) {
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 77263a5..2308067 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -434,7 +434,7 @@ void migrate_del_blocker(Error *reason)
> >  
> >  void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >                   bool has_inc, bool inc, bool has_detach, bool detach,
> > -                 Error **errp)
> > +                 bool has_unpause, bool unpause, Error **errp)
> >  {
> >      Error *local_err = NULL;
> >      MigrationState *s = migrate_get_current();
> > @@ -450,6 +450,25 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> > blk,
> >          return;
> >      }
> >  
> > +    if (unpause) {
> > +        /* Unpause is starting up an *incoming* migration */
> > +        if (detach || inc || blk) {
> > +            error_setg(errp, "Other flags are not meaningful for unpause");
> > +            return;
> > +        }
> > +
> > +        if (!paused_incoming) {
> > +            error_setg(errp, "Unpause can only be used with -incoming 
> > pause");
> > +            return;
> > +        }
> > +
> > +        qemu_start_incoming_migration(uri, errp);
> > +        if (!errp || !*errp) {
> > +            paused_incoming = false;
> > +        }
> > +        return;
> > +    }
> > +
> >      if (runstate_check(RUN_STATE_INMIGRATE)) {
> >          error_setg(errp, "Guest is waiting for an incoming migration");
> >          return;
> 
> Hmm, the 'unpause' codepath doesn't really share anything with the existing
> codepath. Also the URIs for the existing migrate command are not quite the
> same as the URIs for the incoming migrate side. This would suggest to me
> that it might be better to have a separate 'migrate-incoming' command in
> the monitor rather than overload the existing 'migrate' command.
> 
> Also having a separate command will make it possible to detect that this
> feature is supported from libvirt, since I don't think QMP introspection
> provides enough info to detect it based on the new arg to existing
> commands.

OK, that would be easy enough for me to split out.

I'm not completely convinced that the URI parsing is currently different between
incoming and migrate; for example the 'migrate' command seems to blindly
take ,to=4445  which I think it then ignores.  But that's more of a bug.

Dave

> 
> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK