| Message ID | 1423584999-13946-3-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 10, 2015 at 04:16:38PM +0000, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > 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 <dgilbert@redhat.com> > --- > 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 > -@item migrate [-d] [-b] [-i] @var{uri} > +@item 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. Regards, Daniel
On 02/10/2015 09:47 AM, Daniel P. Berrange wrote: > On Tue, Feb 10, 2015 at 04:16:38PM +0000, Dr. David Alan Gilbert (git) wrote: >> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> >> Once a qemu has been started with -incoming pause the s/pause the/pause, the/ >> migration can be started by issuing: >> >> migrate -u uri >> >> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> --- > > 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. Agree, a new command for QMP would be better (it serves as both the new command to use, and the witness that the '-incoming pause:' command line works). The HMP 'migrate -u' is just fine, though (it's fine to have a single HMP command smart enough to call out to two different QMP commands).
* Daniel P. Berrange (berrange@redhat.com) wrote: > On Tue, Feb 10, 2015 at 04:16:38PM +0000, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > 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 <dgilbert@redhat.com> > > --- > > 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 > > -@item migrate [-d] [-b] [-i] @var{uri} > > +@item 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 / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > 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 <dgilbert@redhat.com> > - "(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.", Spaces vs tabs. > + -u to unpause an incoming migration started with -incoming pause more spaces > - qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err); > + qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, !!unpause, unpause, > + &err); I don't claim to understand QMP, but this whole bussines of !!foo, foo is getting confusing, no? No, this is not relaced to this patch. > { > 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; > } > I would preffer something like: if (runstate_check(RUN_STATE_INMIGRATE)) { if (unpause) { ... unpause code } } else { error_setg(errp, "Guest is waiting for an incoming migration"); return; } if (unpause) { error_setg(errp, "Guest is waiting for an incoming migration"); return; } if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP || s->state == MIG_STATE_CANCELLING) { error_set(errp, QERR_MIGRATION_ACTIVE); return; } if (qemu_savevm_state_blocked(errp)) { return; } .... and now continue with the rest ... Thinking more about this problem, I am not sure this is the "cleanest approach". What do you think of: - create RUN_STATE_INMIGRATE_PAUSED bonus: no need of paused_incoming variable - create a new migrate_incoming command And then we have cleaner separation of what we are doing? Later, Juan.
* Eric Blake (eblake@redhat.com) wrote: > On 02/10/2015 09:47 AM, Daniel P. Berrange wrote: > > On Tue, Feb 10, 2015 at 04:16:38PM +0000, Dr. David Alan Gilbert (git) wrote: > >> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >> > >> Once a qemu has been started with -incoming pause the > > s/pause the/pause, the/ > > >> migration can be started by issuing: > >> > >> migrate -u uri > >> > >> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >> --- > > > > 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. > > Agree, a new command for QMP would be better (it serves as both the new > command to use, and the witness that the '-incoming pause:' command line > works). The HMP 'migrate -u' is just fine, though (it's fine to have a > single HMP command smart enough to call out to two different QMP commands). I've split it out as you all suggested; I made it migrate-incoming and also made an HMP migrate_incoming; although as you say you can merge them, I just find it easier when they match - if you're used to using an HMP command it's easier when you need to find the matching QMP command. Dave > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > 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 <dgilbert@redhat.com> > > > - "(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.", > > Spaces vs tabs. > > > + -u to unpause an incoming migration started with -incoming pause > > more spaces All this is reworked anyway since I split it out. > > - qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err); > > + qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, !!unpause, unpause, > > + &err); > > I don't claim to understand QMP, but this whole bussines of !!foo, foo > is getting confusing, no? Yes, and it's very very easy to screw up and get them in the wrong order. > No, this is not relaced to this patch. > > > { > > 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; > > } > > > > I would preffer something like: > > if (runstate_check(RUN_STATE_INMIGRATE)) { > if (unpause) { > ... unpause code > } > } else { > error_setg(errp, "Guest is waiting for an incoming migration"); > return; > } > > if (unpause) { > error_setg(errp, "Guest is waiting for an incoming migration"); > return; > } > > if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP || > s->state == MIG_STATE_CANCELLING) { > error_set(errp, QERR_MIGRATION_ACTIVE); > return; > } > > if (qemu_savevm_state_blocked(errp)) { > return; > } > > .... and now continue with the rest ... Again, all gone in the new version. > Thinking more about this problem, I am not sure this is the "cleanest > approach". What do you think of: > > - create RUN_STATE_INMIGRATE_PAUSED > bonus: no need of paused_incoming variable That we could do separately; doing that means carefully looking at the existing users of RUN_STATE_INMIGRATE, and since it's a visible state that includes anything that uses the interface. > - create a new migrate_incoming command > > And then we have cleaner separation of what we are doing? Done. Dave > > Later, Juan. > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 02/11/2015 09:48 AM, Dr. David Alan Gilbert wrote: >> Agree, a new command for QMP would be better (it serves as both the new >> command to use, and the witness that the '-incoming pause:' command line >> works). The HMP 'migrate -u' is just fine, though (it's fine to have a >> single HMP command smart enough to call out to two different QMP commands). > > I've split it out as you all suggested; I made it migrate-incoming and > also made an HMP migrate_incoming; although as you say you can merge them, > I just find it easier when they match - if you're used to using an HMP > command it's easier when you need to find the matching QMP command. Fair enough; HMP exists for ease-of-use, so whichever way you found easier to code is sufficient :)
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 -@item migrate [-d] [-b] [-i] @var{uri} +@item 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; diff --git a/qapi-schema.json b/qapi-schema.json index e16f8eb..8dd0b88 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1731,12 +1731,15 @@ # @detach: this argument exists only for compatibility reasons and # is ignored by QEMU # +# @unpause: Start an inward migration initiated with -incoming pause +# # Returns: nothing on success # # Since: 0.14.0 ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } } + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool', + '*unpause': 'bool' } } # @xen-save-devices-state: # diff --git a/qmp-commands.hx b/qmp-commands.hx index a85d847..76673c5 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -610,7 +610,7 @@ EQMP { .name = "migrate", - .args_type = "detach:-d,blk:-b,inc:-i,uri:s", + .args_type = "detach:-d,blk:-b,inc:-i,unpause:-u,uri:s", .mhandler.cmd_new = qmp_marshal_input_migrate, }, @@ -624,6 +624,7 @@ Arguments: - "blk": block migration, full disk copy (json-bool, optional) - "inc": incremental disk copy (json-bool, optional) +- "unpause": Unpause an incoming migration (json-bool, optional) - "uri": Destination URI (json-string) Example: