| Message ID | 1392713429-18201-11-git-send-email-mrhines@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 02/18/2014 01:50 AM, mrhines@linux.vnet.ibm.com wrote: > From: "Michael R. Hines" <mrhines@us.ibm.com> > > This exposes a QMP command that allows the management software > or policy to control the frequency of micro-checkpointing. > > Signed-off-by: Michael R. Hines <mrhines@us.ibm.com> > --- > hmp-commands.hx | 16 +++++++++++++++- > hmp.c | 6 ++++++ > hmp.h | 1 + > qapi-schema.json | 13 +++++++++++++ > qmp-commands.hx | 23 +++++++++++++++++++++++ > 5 files changed, 58 insertions(+), 1 deletion(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index f3fc514..2066c76 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -888,7 +888,7 @@ ETEXI > "\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)", Spurious hunk. Oh, I see - you managed to take TAB damage and make it worse with a space-TAB (I guess this file isn't tab-clean, like the .json file is). Eww. > .mhandler.cmd = hmp_migrate, > }, > > @@ -965,6 +965,20 @@ Set maximum tolerated downtime (in seconds) for migration. > ETEXI > > { > + .name = "migrate-set-mc-delay", We're building up a LOT of migrate- tunable commands. Maybe it's time to think about building a more generic migrate-set-parameter, which takes both the name of the parameter to set and its value, so that a single command serves all parameters, instead of needing a proliferation of commands. Of course, for that to be useful, we also need a way to introspect which parameters can be tuned; whereas with the current approach of one command per parameter (well, 2 for set vs. get) the introspection is based on whether the command exists. > +++ b/qapi-schema.json > @@ -2160,6 +2160,19 @@ > { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} } > > ## > +# @migrate-set-mc-delay > +# > +# Set delay (in milliseconds) between micro checkpoints. > +# > +# @value: maximum delay in milliseconds > +# > +# Returns: nothing on success > +# > +# Since: 2.x > +## > +{ 'command': 'migrate-set-mc-delay', 'data': {'value': 'int'} } > + > +## I hate write-only interfaces. If I can set the parameter, I _also_ need a way to query the current value of the parameter. Either an existing migration statistics output should be modified to include this new information, or you need to add a counterpart migrate-get-mc-delay command.
Eric Blake <eblake@redhat.com> wrote: > On 02/18/2014 01:50 AM, mrhines@linux.vnet.ibm.com wrote: >> From: "Michael R. Hines" <mrhines@us.ibm.com> > We're building up a LOT of migrate- tunable commands. Maybe it's time > to think about building a more generic migrate-set-parameter, which > takes both the name of the parameter to set and its value, so that a > single command serves all parameters, instead of needing a proliferation > of commands. Of course, for that to be useful, we also need a way to > introspect which parameters can be tuned; whereas with the current > approach of one command per parameter (well, 2 for set vs. get) the > introspection is based on whether the command exists. I asked to have that. My suggestion was that migrate_set_capability auto-throotle on So we could add it to new variables without extra change. And I agree that having a way to read them, and ask what values they have is a good idea. Luiz, any good idea about how to do it through QMP? Having the migration changes is easy, the problem is knowing how we want them. Later, Juan.
On 03/11/2014 04:15 PM, Juan Quintela wrote: > Eric Blake <eblake@redhat.com> wrote: >> On 02/18/2014 01:50 AM, mrhines@linux.vnet.ibm.com wrote: >>> From: "Michael R. Hines" <mrhines@us.ibm.com> > >> We're building up a LOT of migrate- tunable commands. Maybe it's time >> to think about building a more generic migrate-set-parameter, which >> takes both the name of the parameter to set and its value, so that a >> single command serves all parameters, instead of needing a proliferation >> of commands. Of course, for that to be useful, we also need a way to >> introspect which parameters can be tuned; whereas with the current >> approach of one command per parameter (well, 2 for set vs. get) the >> introspection is based on whether the command exists. > > I asked to have that. My suggestion was that > > migrate_set_capability auto-throotle on > > So we could add it to new variables without extra change. > > And I agree that having a way to read them, and ask what values they > have is a good idea. > > Luiz, any good idea about how to do it through QMP? I'm trying to thing of a back-compat method, which exploits the fact that we now have flat unions (something we didn't have when migrate-set-capabilities was first added). Maybe something like: { 'type': 'MigrationCapabilityBase', 'data': { 'capability': 'MigrationCapability' } } { 'type': 'MigrationCapabilityBool', 'data': { 'state': 'bool' } } { 'type': 'Migration CapabilityInt', 'data': { 'value': 'int' } } { 'union': 'MigrationCapabilityStatus', 'base': 'MigrationCapabilityBase', 'discriminator': 'capability', 'data': { 'xbzrle': 'MigrationCapabilityBool', 'auto-converge': 'MigrationCapabilityBool', ... 'mc-delay': 'MigrationCapabilityInt' } } along with a tweak to query-migrate-capabilities for full back-compat: # @query-migrate-capabilities # @extended: #optional defaults to false; set to true to see non-boolean capabilities (since 2.1) { 'command: 'query-migrate-capabilities', 'data': { '*extended': 'bool' }, 'returns': ['MigrationCapabilityStatus'] } Now, observe what happens. If an old client calls { "execute": "query-migrate-capabilities" }, they get a return that lists ONLY the boolean members of the MigrationCapabilityStatus array (good, because if we returned a non-boolean, we would confuse the consumer when they are expecting a 'state' variable that is not present) - what's more, this representation is identical on the wire to the format used in earlier qemu. But new clients can call { "execute": "query-migrate-capabilities", "arguments": { "extended": true } }, and get back: { "capabilities": [ { "capability": "xbzrle", "state": true }, { "capability": "auto-converge", "state": false }, ... { "capability": "mc-delay", "value": 100 } ] } Also, once a new client has learned of non-boolean extended capabilities, they can also set them via the existing command: { "execute": "migrate-set-capabilities", "arguments": [ { "capability": "xbzrle", "state", false }, { "capability": "mc-delay", "value", 200 } ] } So, what do you think? My slick type manipulation means that we need zero new commands, just a new option the the query command, and a new flat union type that replaces the current struct type. The existence (but not the type) of non-boolean parameters is already introspectible to a client new enough to request an 'extended' query, and down the road, if we ever gain full QAPI introspection, then a client also would gain the ability to learn the type of any non-boolean parameter as well. Stability wise, as long as we never change the type of a capability once first exposed, then if a client plans on using a particular parameter when available, it can already hard-code what type that parameter should have without even needing full QAPI introspection (that is, if libvirt is taught to manipulate mc-delay, libvirt will already know to expect mc-delay as an int, and not any other type, and merely needs to probe if qemu supports the 'mc-delay' extended capability). And of course, this new schema idea can retroactively cover all existing migration tunables, such as migrate_set_downtime, migrate_set_speed, migrate-set-cache-size, and so on. > > Having the migration changes is easy, the problem is knowing how we want > them. And maybe my proposal just solved that.
On 03/12/2014 05:49 AM, Eric Blake wrote: > diff --git a/hmp-commands.hx b/hmp-commands.hx > index f3fc514..2066c76 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -888,7 +888,7 @@ ETEXI > "\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)", > Spurious hunk. Oh, I see - you managed to take TAB damage and make it > worse with a space-TAB (I guess this file isn't tab-clean, like the > .json file is). Eww. Ooops. =) >> .mhandler.cmd = hmp_migrate, >> }, >> >> @@ -965,6 +965,20 @@ Set maximum tolerated downtime (in seconds) for migration. >> ETEXI >> >> { >> + .name = "migrate-set-mc-delay", > We're building up a LOT of migrate- tunable commands. Maybe it's time > to think about building a more generic migrate-set-parameter, which > takes both the name of the parameter to set and its value, so that a > single command serves all parameters, instead of needing a proliferation > of commands. Of course, for that to be useful, we also need a way to > introspect which parameters can be tuned; whereas with the current > approach of one command per parameter (well, 2 for set vs. get) the > introspection is based on whether the command exists. > Well, unless there's anymore strong objection, I didn't find it too difficult to add a new command in QEMU, although I did find it quite painful to expose this command in Libvirt - I had to modify something like 5 or 6 (IIRC) different files in libvirt to accomplish the same goal. Could we "merge" the commands into a single command at the libvirt level instead of the QEMU level? Is there any other "pressing" reason to merge them at the QEMU level? >> +++ b/qapi-schema.json >> @@ -2160,6 +2160,19 @@ >> { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} } >> >> ## >> +# @migrate-set-mc-delay >> +# >> +# Set delay (in milliseconds) between micro checkpoints. >> +# >> +# @value: maximum delay in milliseconds >> +# >> +# Returns: nothing on success >> +# >> +# Since: 2.x >> +## >> +{ 'command': 'migrate-set-mc-delay', 'data': {'value': 'int'} } >> + >> +## > I hate write-only interfaces. If I can set the parameter, I _also_ need > a way to query the current value of the parameter. Either an existing > migration statistics output should be modified to include this new > information, or you need to add a counterpart migrate-get-mc-delay command. > Totally forgot about that - will get a 'get' command in there ASAP. - Michael
On 03/12/2014 06:49 AM, Eric Blake wrote: > On 03/11/2014 04:15 PM, Juan Quintela wrote: >> Eric Blake <eblake@redhat.com> wrote: >>> On 02/18/2014 01:50 AM, mrhines@linux.vnet.ibm.com wrote: >>>> From: "Michael R. Hines" <mrhines@us.ibm.com> >>> We're building up a LOT of migrate- tunable commands. Maybe it's time >>> to think about building a more generic migrate-set-parameter, which >>> takes both the name of the parameter to set and its value, so that a >>> single command serves all parameters, instead of needing a proliferation >>> of commands. Of course, for that to be useful, we also need a way to >>> introspect which parameters can be tuned; whereas with the current >>> approach of one command per parameter (well, 2 for set vs. get) the >>> introspection is based on whether the command exists. >> I asked to have that. My suggestion was that >> >> migrate_set_capability auto-throotle on >> >> So we could add it to new variables without extra change. >> >> And I agree that having a way to read them, and ask what values they >> have is a good idea. >> >> Luiz, any good idea about how to do it through QMP? > I'm trying to thing of a back-compat method, which exploits the fact > that we now have flat unions (something we didn't have when > migrate-set-capabilities was first added). Maybe something like: > > { 'type': 'MigrationCapabilityBase', > 'data': { 'capability': 'MigrationCapability' } } > { 'type': 'MigrationCapabilityBool', > 'data': { 'state': 'bool' } } > { 'type': 'Migration CapabilityInt', > 'data': { 'value': 'int' } } > { 'union': 'MigrationCapabilityStatus', > 'base': 'MigrationCapabilityBase', > 'discriminator': 'capability', > 'data': { > 'xbzrle': 'MigrationCapabilityBool', > 'auto-converge': 'MigrationCapabilityBool', > ... > 'mc-delay': 'MigrationCapabilityInt' > } } > > along with a tweak to query-migrate-capabilities for full back-compat: > > # @query-migrate-capabilities > # @extended: #optional defaults to false; set to true to see non-boolean > capabilities (since 2.1) > { 'command: 'query-migrate-capabilities', > 'data': { '*extended': 'bool' }, > 'returns': ['MigrationCapabilityStatus'] } > > Now, observe what happens. If an old client calls { "execute": > "query-migrate-capabilities" }, they get a return that lists ONLY the > boolean members of the MigrationCapabilityStatus array (good, because if > we returned a non-boolean, we would confuse the consumer when they are > expecting a 'state' variable that is not present) - what's more, this > representation is identical on the wire to the format used in earlier > qemu. But new clients can call { "execute": > "query-migrate-capabilities", "arguments": { "extended": true } }, and > get back: > > { "capabilities": [ > { "capability": "xbzrle", "state": true }, > { "capability": "auto-converge", "state": false }, > ... > { "capability": "mc-delay", "value": 100 } > ] } > > Also, once a new client has learned of non-boolean extended > capabilities, they can also set them via the existing command: > { "execute": "migrate-set-capabilities", > "arguments": [ > { "capability": "xbzrle", "state", false }, > { "capability": "mc-delay", "value", 200 } > ] } > > So, what do you think? My slick type manipulation means that we need > zero new commands, just a new option the the query command, and a new > flat union type that replaces the current struct type. The existence > (but not the type) of non-boolean parameters is already introspectible > to a client new enough to request an 'extended' query, and down the > road, if we ever gain full QAPI introspection, then a client also would > gain the ability to learn the type of any non-boolean parameter as well. > Stability wise, as long as we never change the type of a capability > once first exposed, then if a client plans on using a particular > parameter when available, it can already hard-code what type that > parameter should have without even needing full QAPI introspection (that > is, if libvirt is taught to manipulate mc-delay, libvirt will already > know to expect mc-delay as an int, and not any other type, and merely > needs to probe if qemu supports the 'mc-delay' extended capability). > And of course, this new schema idea can retroactively cover all existing > migration tunables, such as migrate_set_downtime, migrate_set_speed, > migrate-set-cache-size, and so on. I like this a lot - it's very complicated, but it is clean, I think. Maybe you should add some "reserved" fields in there as well to the union, in case you want to expand the number of members of the union in the future? - Michael
On 04/03/2014 11:29 PM, Michael R. Hines wrote: >> I'm trying to thing of a back-compat method, which exploits the fact >> that we now have flat unions (something we didn't have when >> migrate-set-capabilities was first added). Maybe something like: >> >> { 'type': 'MigrationCapabilityBase', >> 'data': { 'capability': 'MigrationCapability' } } >> { 'type': 'MigrationCapabilityBool', >> 'data': { 'state': 'bool' } } >> { 'type': 'Migration CapabilityInt', >> 'data': { 'value': 'int' } } >> { 'union': 'MigrationCapabilityStatus', >> 'base': 'MigrationCapabilityBase', >> 'discriminator': 'capability', >> 'data': { >> 'xbzrle': 'MigrationCapabilityBool', >> 'auto-converge': 'MigrationCapabilityBool', >> ... >> 'mc-delay': 'MigrationCapabilityInt' >> } } >> >> along with a tweak to query-migrate-capabilities for full back-compat: >> >> # @query-migrate-capabilities >> # @extended: #optional defaults to false; set to true to see non-boolean >> capabilities (since 2.1) >> { 'command: 'query-migrate-capabilities', >> 'data': { '*extended': 'bool' }, >> 'returns': ['MigrationCapabilityStatus'] } >> > > I like this a lot - it's very complicated, but it is clean, I think. Good - that means I made sense in trying to explain it. And the more I re-read my mail, the more I like the idea - fewer new commands, and make the existing commands both more powerful and more easily extensible, all while still being discoverable by libvirt without waiting for full schema introspection. > > Maybe you should add some "reserved" fields in there as well > to the union, in case you want to expand the number of members > of the union in the future? Adding members to a union is back-compat safe. No need to reserve slots, we just add them when we have a use for them. Besides, how would you reserve a slot? QAPI requires a name (not just a type) - but unless you know what to name your slot, you can't really reserve it. (We are NOT worried about C ABI compatibility, where a union type must be large enough to occupy enough bytes for any future larger structs carved into the union - we are only worried about QAPI API compatibility which requires a name for each branch of the union)
One thing to be a little careful about if we merge these tunables together, is what tunables are allowed to be changed while the migration is running. The 'capabilities' are currently fixed once the migration starts, but I know at least some of the tuneables people want to change while things are going - and some care is needed with it since (as we found with the xbzrle cache size) we get fun due to the use being in a different thread. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 04/04/2014 10:28 AM, Dr. David Alan Gilbert wrote: > One thing to be a little careful about if we merge these tunables > together, is what tunables are allowed to be changed while the migration > is running. The 'capabilities' are currently fixed once the migration > starts, but I know at least some of the tuneables people want to change > while things are going - and some care is needed with it since (as we > found with the xbzrle cache size) we get fun due to the use being in > a different thread. Possible to solve that by adding an annotation in extended query output of which tunables are live, and by making the set command reject any changes for a tunable that is not live if migration is already underway. But yes, worth thinking about.
On 04/04/2014 10:56 PM, Eric Blake wrote: > On 04/03/2014 11:29 PM, Michael R. Hines wrote: >>> I'm trying to thing of a back-compat method, which exploits the fact >>> that we now have flat unions (something we didn't have when >>> migrate-set-capabilities was first added). Maybe something like: >>> >>> { 'type': 'MigrationCapabilityBase', >>> 'data': { 'capability': 'MigrationCapability' } } >>> { 'type': 'MigrationCapabilityBool', >>> 'data': { 'state': 'bool' } } >>> { 'type': 'Migration CapabilityInt', >>> 'data': { 'value': 'int' } } >>> { 'union': 'MigrationCapabilityStatus', >>> 'base': 'MigrationCapabilityBase', >>> 'discriminator': 'capability', >>> 'data': { >>> 'xbzrle': 'MigrationCapabilityBool', >>> 'auto-converge': 'MigrationCapabilityBool', >>> ... >>> 'mc-delay': 'MigrationCapabilityInt' >>> } } >>> >>> along with a tweak to query-migrate-capabilities for full back-compat: >>> >>> # @query-migrate-capabilities >>> # @extended: #optional defaults to false; set to true to see non-boolean >>> capabilities (since 2.1) >>> { 'command: 'query-migrate-capabilities', >>> 'data': { '*extended': 'bool' }, >>> 'returns': ['MigrationCapabilityStatus'] } >>> >> I like this a lot - it's very complicated, but it is clean, I think. > Good - that means I made sense in trying to explain it. And the more I > re-read my mail, the more I like the idea - fewer new commands, and make > the existing commands both more powerful and more easily extensible, all > while still being discoverable by libvirt without waiting for full > schema introspection. Alright, I've saved this proposal on the wiki on the MicroCheckpointing TODO section: http://wiki.qemu.org/Features/MicroCheckpointing#TODO For now, I've got several other issues to address before "someone" gets around to this (I'd assume the maintainer or someone else would want to test the 'extended' feature by itself in isolation with the existing set of migration commands before someone else like me attempts to use it or start adding new features to it.) - Michael
diff --git a/hmp-commands.hx b/hmp-commands.hx index f3fc514..2066c76 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -888,7 +888,7 @@ ETEXI "\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)", .mhandler.cmd = hmp_migrate, }, @@ -965,6 +965,20 @@ Set maximum tolerated downtime (in seconds) for migration. ETEXI { + .name = "migrate-set-mc-delay", + .args_type = "value:i", + .params = "value", + .help = "Set maximum delay (in milliseconds) between micro-checkpoints", + .mhandler.cmd = hmp_migrate_set_mc_delay, + }, + +STEXI +@item migrate-set-mc-delay @var{millisecond} +@findex migrate-set-mc-delay +Set maximum delay (in milliseconds) between micro-checkpoints. +ETEXI + + { .name = "migrate_set_capability", .args_type = "capability:s,state:b", .params = "capability state", diff --git a/hmp.c b/hmp.c index edf062e..9880bc8 100644 --- a/hmp.c +++ b/hmp.c @@ -1029,6 +1029,12 @@ void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict) qmp_migrate_set_downtime(value, NULL); } +void hmp_migrate_set_mc_delay(Monitor *mon, const QDict *qdict) +{ + int64_t value = qdict_get_int(qdict, "value"); + qmp_migrate_set_mc_delay(value, NULL); +} + void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict) { int64_t value = qdict_get_int(qdict, "value"); diff --git a/hmp.h b/hmp.h index ed58f0e..068b2c1 100644 --- a/hmp.h +++ b/hmp.h @@ -60,6 +60,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict); void hmp_drive_backup(Monitor *mon, const QDict *qdict); void hmp_migrate_cancel(Monitor *mon, const QDict *qdict); void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict); +void hmp_migrate_set_mc_delay(Monitor *mon, const QDict *qdict); void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict); void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict); void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict); diff --git a/qapi-schema.json b/qapi-schema.json index 7306adc..98abdac 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2160,6 +2160,19 @@ { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} } ## +# @migrate-set-mc-delay +# +# Set delay (in milliseconds) between micro checkpoints. +# +# @value: maximum delay in milliseconds +# +# Returns: nothing on success +# +# Since: 2.x +## +{ 'command': 'migrate-set-mc-delay', 'data': {'value': 'int'} } + +## # @migrate_set_speed # # Set maximum speed for migration. diff --git a/qmp-commands.hx b/qmp-commands.hx index cce6b81..d8b9c34 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -754,6 +754,29 @@ Example: EQMP { + .name = "migrate-set-mc-delay", + .args_type = "value:i", + .mhandler.cmd_new = qmp_marshal_input_migrate_set_mc_delay, + }, + +SQMP +migrate-set-mc-delay +-------------------- + +Set maximum delay (in milliseconds) between micro-checkpoints. + +Arguments: + +- "value": maximum delay (json-int) + +Example: + +-> { "execute": "migrate-set-mc-delay", "arguments": { "value": 100 } } +<- { "return": {} } + +EQMP + + { .name = "client_migrate_info", .args_type = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?", .params = "protocol hostname port tls-port cert-subject",