| Message ID | 1380485683-4626-3-git-send-email-junqing.wang@cs2c.com.cn |
|---|---|
| State | New |
| Headers | show |
On 09/29/2013 02:14 PM, Jules Wang wrote: > Add an option '-f' to migration cmdline. > Indicating whether to enable fault tolerant or not. > > Signed-off-by: Jules Wang <junqing.wang@cs2c.com.cn> > --- > .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 -f for fault tolerant, this is another " > + "feature rather than migrate", That sounds awkward, and overly long. Maybe go with just: -f for fault tolerance mode and let the user then read the full documentation for what it entails. > -@item migrate [-d] [-b] [-i] @var{uri} > +@item migrate [-d] [-b] [-i] [-f] @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) > + -f for fault tolerant Can -d and -f be used at the same time, or are they exclusive? > +++ b/hmp.c > @@ -1213,10 +1213,11 @@ 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 ft = qdict_get_try_bool(qdict, "ft", 0); Why two spaces? > +++ b/qapi-schema.json > @@ -2420,7 +2420,8 @@ > # Since: 0.14.0 > ## > { 'command': 'migrate', > - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } } > + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool', > + '*ft': 'bool' } } Missing documentation, including mention that the new option was only made available in 1.7. We still don't have introspection; is there some other means by which libvirt and other management apps can tell whether this feature is available? Furthermore, 'ft' is an awfully short name; for QMP, we prefer to use full words where possible, such as 'fault-tolerant'.
At 2013-10-01 06:16:34,"Eric Blake" <eblake@redhat.com> wrote: >On 09/29/2013 02:14 PM, Jules Wang wrote: >> Add an option '-f' to migration cmdline. >> Indicating whether to enable fault tolerant or not. >> >> Signed-off-by: Jules Wang <junqing.wang@cs2c.com.cn> >> --- >> .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 -f for fault tolerant, this is another " >> + "feature rather than migrate", > >That sounds awkward, and overly long. Maybe go with just: > >-f for fault tolerance mode > >and let the user then read the full documentation for what it entails. Agree. >> -@item migrate [-d] [-b] [-i] @var{uri} >> +@item migrate [-d] [-b] [-i] [-f] @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) >> + -f for fault tolerant > >Can -d and -f be used at the same time, or are they exclusive? AFAK, The migration is always detached(In the code, the -d option is always false), -d and -f can be used at the same time with no doubt. qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, !!ft, ft, &err); By the way, neither -b nor -i could be used at the same time with -f, fault tolerant needs shared storage. >> +++ b/hmp.c >> @@ -1213,10 +1213,11 @@ 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 ft = qdict_get_try_bool(qdict, "ft", 0); > >Why two spaces? To align the '=', I will remove them if you like. > >> +++ b/qapi-schema.json >> @@ -2420,7 +2420,8 @@ >> # Since: 0.14.0 >> ## >> { 'command': 'migrate', >> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } } >> + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool', >> + '*ft': 'bool' } } > >Missing documentation, including mention that the new option was only >made available in 1.7. We still don't have introspection; is there some >other means by which libvirt and other management apps can tell whether >this feature is available? I'm not clear about how to do that, could you pls give me some hints, where to add code and documentation. >Furthermore, 'ft' is an awfully short name; >for QMP, we prefer to use full words where possible, such as >'fault-tolerant'. Agree. >-- >Eric Blake eblake redhat com +1-919-301-3266 >Libvirt virtualization library http://libvirt.org >
[your emailer munged the reply, making it a bit hard to read. Are you set for plain-text-only mail to the list?] On 10/09/2013 12:49 AM, junqing.wang@cs2c.com.cn wrote: > >> +++ b/hmp.c >>> @@ -1213,10 +1213,11 @@ 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 ft = qdict_get_try_bool(qdict, "ft", 0); >> >> Why two spaces? > > To align the '=', I will remove them if you like. It's not a problem with me either way, other than we have a lot of code that doesn't care about alignment and consistently uses one space, and a fair amount of code where everything in a block of code is consistently aligned. But your patch was neither, in the context of the block it lives within - if you're going to align, then line up everything with the longest line 'int detach' (including blk and inc). > > > >>> +++ b/qapi-schema.json >>> @@ -2420,7 +2420,8 @@ >>> # Since: 0.14.0 >>> ## >>> { 'command': 'migrate', >>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } } >>> + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool', >>> + '*ft': 'bool' } } >> >> Missing documentation, including mention that the new option was only >> made available in 1.7. We still don't have introspection; is there some >> other means by which libvirt and other management apps can tell whether >> this feature is available? > > I'm not clear about how to do that, could you pls give me some hints, where to > add code and documentation. As for the documentation, qapi-schema.json has plenty of examples (look for a field with "(since 1.7)" as a hint for how to document an optional field added in a later release than the main struct). As for the introspection, Amos Kong was most recently working on trying to add that (but missed the 1.6 deadline, and I haven't seen work on it since). Introspection is not a hard requirement, but it makes it harder for libvirt to know if it can use 'ft':true if there is no other 'query-*' command that it can call first that would give it a hint that this is a new enough qemu to support 'ft' during migration. Maybe even having something listed under query-migrate-capabilities would be sufficient (ie. modify the 'MigrationCapability' enum to advertise a new capability).
On Wed, 2013-10-09 at 06:02 -0600, Eric Blake wrote: > [your emailer munged the reply, making it a bit hard to read. Are you > set for plain-text-only mail to the list?] Thanks VERY much for remind me that, I'm using another client now. > On 10/09/2013 12:49 AM, junqing.wang@cs2c.com.cn wrote: > > > >> +++ b/hmp.c > >>> @@ -1213,10 +1213,11 @@ 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 ft = qdict_get_try_bool(qdict, "ft", 0); > >> > >> Why two spaces? > > > > To align the '=', I will remove them if you like. > > It's not a problem with me either way, other than we have a lot of code > that doesn't care about alignment and consistently uses one space, and a > fair amount of code where everything in a block of code is consistently > aligned. But your patch was neither, in the context of the block it > lives within - if you're going to align, then line up everything with > the longest line 'int detach' (including blk and inc). > oh, I got it, you are right, I missed the longest line 'int detach ...' I'm not going to align them. > > > > > > >>> +++ b/qapi-schema.json > >>> @@ -2420,7 +2420,8 @@ > >>> # Since: 0.14.0 > >>> ## > >>> { 'command': 'migrate', > >>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } } > >>> + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool', > >>> + '*ft': 'bool' } } > >> > >> Missing documentation, including mention that the new option was only > >> made available in 1.7. We still don't have introspection; is there some > >> other means by which libvirt and other management apps can tell whether > >> this feature is available? > > > > I'm not clear about how to do that, could you pls give me some hints, where to > > add code and documentation. > > As for the documentation, qapi-schema.json has plenty of examples (look > for a field with "(since 1.7)" as a hint for how to document an optional > field added in a later release than the main struct). I see. Thanks. > > As for the introspection, Amos Kong was most recently working on trying > to add that (but missed the 1.6 deadline, and I haven't seen work on it > since). Introspection is not a hard requirement, but it makes it harder > for libvirt to know if it can use 'ft':true if there is no other > 'query-*' command that it can call first that would give it a hint that > this is a new enough qemu to support 'ft' during migration. Maybe even > having something listed under query-migrate-capabilities would be > sufficient (ie. modify the 'MigrationCapability' enum to advertise a new > capability). Adding a new migration capability is a work-around method. we turn on ft by using the -f option instead of setting fault-tolerant-capability to true. I hesitate to add it. What about adding a query for the options of migration similar to @query-command-line-options?
diff --git a/hmp-commands.hx b/hmp-commands.hx index 65b7f60..8418d37 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -877,23 +877,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,ft:-f,uri:s", + .params = "[-d] [-b] [-i] [-f] 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 -f for fault tolerant, this is another " + "feature rather than migrate", .mhandler.cmd = hmp_migrate, }, STEXI -@item migrate [-d] [-b] [-i] @var{uri} +@item migrate [-d] [-b] [-i] [-f] @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) + -f for fault tolerant ETEXI { diff --git a/hmp.c b/hmp.c index fcca6ae..91beae9 100644 --- a/hmp.c +++ b/hmp.c @@ -1213,10 +1213,11 @@ 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 ft = qdict_get_try_bool(qdict, "ft", 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, !!ft, ft, &err); if (err) { monitor_printf(mon, "migrate: %s\n", error_get_pretty(err)); error_free(err); diff --git a/include/migration/migration.h b/include/migration/migration.h index 140e6b4..fc2b066 100644 --- a/include/migration/migration.h +++ b/include/migration/migration.h @@ -25,6 +25,7 @@ struct MigrationParams { bool blk; + bool ft; bool shared; }; diff --git a/migration.c b/migration.c index 200d404..8989a51 100644 --- a/migration.c +++ b/migration.c @@ -394,7 +394,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_ft, bool ft, Error **errp) { Error *local_err = NULL; MigrationState *s = migrate_get_current(); @@ -403,6 +403,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, params.blk = has_blk && blk; params.shared = has_inc && inc; + params.ft = has_ft && ft; if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP) { error_set(errp, QERR_MIGRATION_ACTIVE); diff --git a/qapi-schema.json b/qapi-schema.json index a51f7d2..a8187cf 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2420,7 +2420,8 @@ # Since: 0.14.0 ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } } + 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool', + '*ft': 'bool' } } # @xen-save-devices-state: # diff --git a/qmp-commands.hx b/qmp-commands.hx index 8a8f342..1fa0e60 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -611,7 +611,7 @@ EQMP { .name = "migrate", - .args_type = "detach:-d,blk:-b,inc:-i,uri:s", + .args_type = "detach:-d,blk:-b,inc:-i,ft:-f,uri:s", .mhandler.cmd_new = qmp_marshal_input_migrate, }, @@ -625,6 +625,7 @@ Arguments: - "blk": block migration, full disk copy (json-bool, optional) - "inc": incremental disk copy (json-bool, optional) +- "ft" : fault tolerant (json-bool, optional) - "uri": Destination URI (json-string) Example:
Add an option '-f' to migration cmdline. Indicating whether to enable fault tolerant or not. Signed-off-by: Jules Wang <junqing.wang@cs2c.com.cn> --- hmp-commands.hx | 11 +++++++---- hmp.c | 3 ++- include/migration/migration.h | 1 + migration.c | 3 ++- qapi-schema.json | 3 ++- qmp-commands.hx | 3 ++- 6 files changed, 16 insertions(+), 8 deletions(-)