| Message ID | 1342164224-32709-14-git-send-email-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 13 Jul 2012 09:23:35 +0200 Juan Quintela <quintela@redhat.com> wrote: > From: Orit Wasserman <owasserm@redhat.com> > > Add migration capabilities that can be queried by the management. > The management can query the source QEMU and the destination QEMU in order to > verify both support some migration capability (currently only XBZRLE). > The management can enable a capability for the next migration by using > migrate_set_parameter command. Please, split this into one command per-patch. Otherwise it's difficult to review. Have libvirt folks acked this approach btw? It looks fine to me, but we need their ack too. More comments below. > > Signed-off-by: Orit Wasserman <owasserm@redhat.com> > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > hmp-commands.hx | 16 ++++++++++++ > hmp.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ > hmp.h | 2 ++ > migration.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- > migration.h | 2 ++ > monitor.c | 7 ++++++ > qapi-schema.json | 53 ++++++++++++++++++++++++++++++++++++++-- > qmp-commands.hx | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++--- > 8 files changed, 280 insertions(+), 7 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index f5d9d91..9245bef 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration. > ETEXI > > { > + .name = "migrate_set_parameter", > + .args_type = "capability:s,state:b", > + .params = "", Please, fill in params. > + .help = "Enable/Disable the usage of a capability for migration", > + .mhandler.cmd = hmp_migrate_set_parameter, > + }, > + > +STEXI > +@item migrate_set_parameter @var{capability} @var{state} > +@findex migrate_set_parameter > +Enable/Disable the usage of a capability @var{capability} for migration. > +ETEXI > + > + { > .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", > @@ -1419,6 +1433,8 @@ show CPU statistics > show user network stack connection states > @item info migrate > show migration status > +@item info migration_capabilities > +show migration capabilities > @item info balloon > show balloon information > @item info qtree > diff --git a/hmp.c b/hmp.c > index 4c6d4ae..b0440e6 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon) > void hmp_info_migrate(Monitor *mon) > { > MigrationInfo *info; > + MigrationCapabilityInfoList *cap; > > info = qmp_query_migrate(NULL); > > + if (info->has_capabilities && info->capabilities) { > + monitor_printf(mon, "capabilities: "); > + for (cap = info->capabilities; cap; cap = cap->next) { > + monitor_printf(mon, "%s: %s ", > + MigrationCapability_lookup[cap->value->capability], > + cap->value->state ? "on" : "off"); > + } > + monitor_printf(mon, "\n"); Why is this is needed? Isn't info migration-capabilities good enough? Besides, info migrate should only contain information about current migration process... > + } > if (info->has_status) { > monitor_printf(mon, "Migration status: %s\n", info->status); > } > @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon) > qapi_free_MigrationInfo(info); > } > > +void hmp_info_migration_capabilities(Monitor *mon) > +{ > + MigrationCapabilityInfoList *caps_list, *cap; > + > + caps_list = qmp_query_migration_capabilities(NULL); > + if (!caps_list) { > + monitor_printf(mon, "No migration capabilities found\n"); > + return; > + } > + > + for (cap = caps_list; cap; cap = cap->next) { > + monitor_printf(mon, "%s: %s ", > + MigrationCapability_lookup[cap->value->capability], > + cap->value->state ? "on" : "off"); > + } > + > + qapi_free_MigrationCapabilityInfoList(caps_list); > +} > + > void hmp_info_cpus(Monitor *mon) > { > CpuInfoList *cpu_list, *cpu; > @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict) > qmp_migrate_set_speed(value, NULL); > } > > +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > +{ > + const char *cap = qdict_get_str(qdict, "capability"); > + bool state = qdict_get_bool(qdict, "state"); > + Error *err = NULL; > + MigrationCapabilityInfoList *params = NULL; > + int i; > + > + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > + if (strcmp(cap, MigrationCapability_lookup[i]) == 0) { > + if (!params) { > + params = g_malloc0(sizeof(*params)); > + } > + params->value = g_malloc0(sizeof(*params->value)); > + params->value->capability = i; > + params->value->state = state; > + params->next = NULL; > + qmp_migrate_set_parameters(params, &err); > + break; > + } > + } > + > + if (i == MIGRATION_CAPABILITY_MAX) { > + error_set(&err, QERR_INVALID_PARAMETER, cap); > + } > + > + qapi_free_MigrationCapabilityInfoList(params); > + > + if (err) { > + monitor_printf(mon, "migrate_set_parameter: %s\n", > + error_get_pretty(err)); > + error_free(err); > + } > +} > + > void hmp_set_password(Monitor *mon, const QDict *qdict) > { > const char *protocol = qdict_get_str(qdict, "protocol"); > diff --git a/hmp.h b/hmp.h > index 79d138d..09ba198 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon); > void hmp_info_chardev(Monitor *mon); > void hmp_info_mice(Monitor *mon); > void hmp_info_migrate(Monitor *mon); > +void hmp_info_migration_capabilities(Monitor *mon); > void hmp_info_cpus(Monitor *mon); > void hmp_info_block(Monitor *mon); > void hmp_info_blockstats(Monitor *mon); > @@ -51,6 +52,7 @@ void hmp_snapshot_blkdev(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_speed(Monitor *mon, const QDict *qdict); > +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict); > void hmp_set_password(Monitor *mon, const QDict *qdict); > void hmp_expire_password(Monitor *mon, const QDict *qdict); > void hmp_eject(Monitor *mon, const QDict *qdict); > diff --git a/migration.c b/migration.c > index 8db1b43..fd004d7 100644 > --- a/migration.c > +++ b/migration.c > @@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp) > { > MigrationInfo *info = g_malloc0(sizeof(*info)); > MigrationState *s = migrate_get_current(); > + int i; > > switch (s->state) { > case MIG_STATE_SETUP: > - /* no migration has happened ever */ > + /* no migration has ever happened; show enabled capabilities */ > + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > + if (!info->has_capabilities) { > + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > + info->has_capabilities = true; > + } > + info->capabilities->value = > + g_malloc(sizeof(*info->capabilities->value)); > + info->capabilities->value->capability = i; > + info->capabilities->value->state = s->enabled_capabilities[i]; > + info->capabilities->next = NULL; > + } > break; > case MIG_STATE_ACTIVE: > + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > + if (!info->has_capabilities) { > + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > + info->has_capabilities = true; > + } > + info->capabilities->value = > + g_malloc(sizeof(*info->capabilities->value)); > + info->capabilities->value->capability = i; > + info->capabilities->value->state = s->enabled_capabilities[i]; > + info->capabilities->next = NULL; > + } > + > info->has_status = true; > info->status = g_strdup("active"); > > @@ -143,6 +167,18 @@ MigrationInfo *qmp_query_migrate(Error **errp) > } > break; > case MIG_STATE_COMPLETED: > + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > + if (!info->has_capabilities) { > + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > + info->has_capabilities = true; > + } > + info->capabilities->value = > + g_malloc(sizeof(*info->capabilities->value)); > + info->capabilities->value->capability = i; > + info->capabilities->value->state = s->enabled_capabilities[i]; > + info->capabilities->next = NULL; > + } Code triplication :) Why is this is needed? Isn't query-migration-capabilities good enough? Besides, query-migrate should only contain information about current migration process... > + > info->has_status = true; > info->status = g_strdup("completed"); > > @@ -166,6 +202,33 @@ MigrationInfo *qmp_query_migrate(Error **errp) > return info; > } > > +MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp) > +{ > + MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list)); > + > + caps_list->value = g_malloc(sizeof(*caps_list->value)); > + caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE; > + caps_list->next = NULL; Shouldn't this get the capabilities array from migrate_get_current()? I mean, this makes query-migration-capabilities always return true for xbzrle, even if we set it to off. > + > + return caps_list; > +} > + > +void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params, > + Error **errp) > +{ > + MigrationState *s = migrate_get_current(); > + MigrationCapabilityInfoList *cap; > + > + if (s->state == MIG_STATE_ACTIVE) { > + error_set(errp, QERR_MIGRATION_ACTIVE); > + return; > + } > + > + for (cap = params; cap; cap = cap->next) { > + s->enabled_capabilities[cap->value->capability] = cap->value->state; > + } > +} > + > /* shared migration helpers */ > > static int migrate_fd_cleanup(MigrationState *s) > @@ -375,12 +438,17 @@ static MigrationState *migrate_init(const MigrationParams *params) > { > MigrationState *s = migrate_get_current(); > int64_t bandwidth_limit = s->bandwidth_limit; > + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; > + > + memcpy(enabled_capabilities, s->enabled_capabilities, > + sizeof(enabled_capabilities)); > > memset(s, 0, sizeof(*s)); > s->bandwidth_limit = bandwidth_limit; > s->params = *params; > + memcpy(s->enabled_capabilities, enabled_capabilities, > + sizeof(enabled_capabilities)); > > - s->bandwidth_limit = bandwidth_limit; > s->state = MIG_STATE_SETUP; > s->total_time = qemu_get_clock_ms(rt_clock); > > diff --git a/migration.h b/migration.h > index 57572a6..713aae0 100644 > --- a/migration.h > +++ b/migration.h > @@ -19,6 +19,7 @@ > #include "notify.h" > #include "error.h" > #include "vmstate.h" > +#include "qapi-types.h" > > struct MigrationParams { > bool blk; > @@ -39,6 +40,7 @@ struct MigrationState > void *opaque; > MigrationParams params; > int64_t total_time; > + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; > }; > > void process_incoming_migration(QEMUFile *f); > diff --git a/monitor.c b/monitor.c > index f6107ba..e2be6cd 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = { > .mhandler.info = hmp_info_migrate, > }, > { > + .name = "migration_capabilities", migration-capabilities is better. > + .args_type = "", > + .params = "", > + .help = "show migration capabilities", > + .mhandler.info = hmp_info_migration_capabilities, > + }, > + { > .name = "balloon", > .args_type = "", > .params = "", > diff --git a/qapi-schema.json b/qapi-schema.json > index 1ab5dbd..a8408fd 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -288,11 +288,15 @@ > # status, only returned if status is 'active' and it is a block > # migration > # > -# Since: 0.14.0 > +# @capabilities: #optional a list describing all the migration capabilities > +# state I don't think this is needed, as I've said above. > +# > +# Since: 0.14.0, 'capabilities' since 1.2 > ## > { 'type': 'MigrationInfo', > 'data': {'*status': 'str', '*ram': 'MigrationStats', > - '*disk': 'MigrationStats'} } > + '*disk': 'MigrationStats', > + '*capabilities': ['MigrationCapabilityInfo']} } > > ## > # @query-migrate > @@ -306,6 +310,51 @@ > { 'command': 'query-migrate', 'returns': 'MigrationInfo' } > > ## > +# @MigrationCapability > +# > +# Migration capabilities enumeration > +# > +# @xbzrle: current migration supports xbzrle You should explain what xbzrle is. > +# > +# Since: 1.2 > +## > +{ 'enum': 'MigrationCapability', > + 'data': ['xbzrle'] } > + > +## > +# @MigrationCapabilityInfo MigrationCapabilityStatus? > +# > +# Migration capability information > +# > +# @capability: capability enum Please, document state too. > +# > +# Since: 1.2 > +## > +{ 'type': 'MigrationCapabilityInfo', > + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } } > + > +## > +# @query-migration-capabilities > +# > +# Returns information about current migration process capabilties. > +# > +# Returns: @MigrationCapabilityInfo list > +# > +# Since: 1.2 > +## > +{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] } > + > +## > +# @migrate_set_parameters > +# > +# Enable/Disable the following migration capabilities (like xbzrle) > +# > +# Since: 1.2 > +## > +{ 'command': 'migrate-set-parameters', > + 'data': { 'capabilities': ['MigrationCapabilityInfo'] } } > + > +## > # @MouseInfo: > # > # Information about a mouse device. > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 2e1a38e..3ee6e00 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -2075,18 +2075,31 @@ The main json-object contains the following: > - "transferred": amount transferred (json-int) > - "remaining": amount remaining (json-int) > - "total": total (json-int) > - > +- "capabilities": migration capabilities state > + - "xbzrle" : XBZRLE state (json-bool) > Examples: > > 1. Before the first migration > > -> { "execute": "query-migrate" } > -<- { "return": {} } > +<- { "return": { > + "capabilities" : [ { "capability" : "xbzrle", "state" : false } ] > + } > + } > > 2. Migration is done and has succeeded > > -> { "execute": "query-migrate" } > -<- { "return": { "status": "completed" } } > +<- { "return": { > + "status": "completed", > + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > + "ram":{ > + "transferred":123, > + "remaining":123, > + "total":246 > + } > + } > + } > > 3. Migration is done and has failed > > @@ -2099,6 +2112,7 @@ Examples: > <- { > "return":{ > "status":"active", > + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > "ram":{ > "transferred":123, > "remaining":123, > @@ -2113,6 +2127,7 @@ Examples: > <- { > "return":{ > "status":"active", > + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > "ram":{ > "total":1057024, > "remaining":1053304, > @@ -2135,6 +2150,56 @@ EQMP > }, > > SQMP > +query-migration-capabilities > +------- > + > +Query migration capabilities > + > +- "xbzrle": xbzrle support > + > +Arguments: > + > +Example: > + > +-> { "execute": "query-migration-capabilities"} > +<- { "return": [ { "capability": "xbzrle", "state": true }, > + { "capability": "foobar", "state": false } ] } > + > +EQMP > + > + { > + .name = "query-migration-capabilities", > + .args_type = "", > + .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities, > + }, > + > +SQMP > +migrate_set_parameters > +------- > + > +Enable/Disable migration capabilities > + > +- "xbzrle": xbzrle support > + > +Arguments: > + > +Example: > + > +-> { "execute": "migrate_set_parameters" , "arguments": > + { "parameters": [ { "capability": "xbzrle", "state": true } ] } } > + > +EQMP > + > + { > + .name = "migrate_set_parameters", > + .args_type = "parameters:O", > + .params = "capability:s,state:b", > + .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, > + }, > + > + > + > +SQMP > query-balloon > ------------- >
On 07/23/2012 12:23 PM, Luiz Capitulino wrote: > On Fri, 13 Jul 2012 09:23:35 +0200 > Juan Quintela <quintela@redhat.com> wrote: > >> From: Orit Wasserman <owasserm@redhat.com> >> >> Add migration capabilities that can be queried by the management. >> The management can query the source QEMU and the destination QEMU in order to >> verify both support some migration capability (currently only XBZRLE). >> The management can enable a capability for the next migration by using >> migrate_set_parameter command. > > Please, split this into one command per-patch. Otherwise it's difficult to > review. > > Have libvirt folks acked this approach btw? It looks fine to me, but we need > their ack too. Yes, I've been reviewing versions of this series, and am okay with the libvirt impact with the current proposed set of new QMP commands.
On 07/23/2012 09:23 PM, Luiz Capitulino wrote: > On Fri, 13 Jul 2012 09:23:35 +0200 > Juan Quintela <quintela@redhat.com> wrote: > >> From: Orit Wasserman <owasserm@redhat.com> >> >> Add migration capabilities that can be queried by the management. >> The management can query the source QEMU and the destination QEMU in order to >> verify both support some migration capability (currently only XBZRLE). >> The management can enable a capability for the next migration by using >> migrate_set_parameter command. > > Please, split this into one command per-patch. Otherwise it's difficult to > review. > Sure. > Have libvirt folks acked this approach btw? It looks fine to me, but we need > their ack too. > > More comments below. > >> >> Signed-off-by: Orit Wasserman <owasserm@redhat.com> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> hmp-commands.hx | 16 ++++++++++++ >> hmp.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ >> hmp.h | 2 ++ >> migration.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- >> migration.h | 2 ++ >> monitor.c | 7 ++++++ >> qapi-schema.json | 53 ++++++++++++++++++++++++++++++++++++++-- >> qmp-commands.hx | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++--- >> 8 files changed, 280 insertions(+), 7 deletions(-) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index f5d9d91..9245bef 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration. >> ETEXI >> >> { >> + .name = "migrate_set_parameter", >> + .args_type = "capability:s,state:b", >> + .params = "", > > Please, fill in params. ok > >> + .help = "Enable/Disable the usage of a capability for migration", >> + .mhandler.cmd = hmp_migrate_set_parameter, >> + }, >> + >> +STEXI >> +@item migrate_set_parameter @var{capability} @var{state} >> +@findex migrate_set_parameter >> +Enable/Disable the usage of a capability @var{capability} for migration. >> +ETEXI >> + >> + { >> .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", >> @@ -1419,6 +1433,8 @@ show CPU statistics >> show user network stack connection states >> @item info migrate >> show migration status >> +@item info migration_capabilities >> +show migration capabilities >> @item info balloon >> show balloon information >> @item info qtree >> diff --git a/hmp.c b/hmp.c >> index 4c6d4ae..b0440e6 100644 >> --- a/hmp.c >> +++ b/hmp.c >> @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon) >> void hmp_info_migrate(Monitor *mon) >> { >> MigrationInfo *info; >> + MigrationCapabilityInfoList *cap; >> >> info = qmp_query_migrate(NULL); >> >> + if (info->has_capabilities && info->capabilities) { >> + monitor_printf(mon, "capabilities: "); >> + for (cap = info->capabilities; cap; cap = cap->next) { >> + monitor_printf(mon, "%s: %s ", >> + MigrationCapability_lookup[cap->value->capability], >> + cap->value->state ? "on" : "off"); >> + } >> + monitor_printf(mon, "\n"); > > Why is this is needed? Isn't info migration-capabilities good enough? > Besides, info migrate should only contain information about current migration > process... The reason we introduced capabilities is that xbzrle needs for both source and destination QEMU to be able to handle it. Even if both side support xbzrle the user may decide not to use it. User that wants to use xbzrle needs to check that both sides have support for it (using info capabilities) , than enable it in both sides (using migrate-set-parameter/s commands). This is a parameter for the current migration. So the user needs to know if xbzrle was enabled or disabled for the current migration, this code displays it. some day when there will be better migration protocol, feature negotiations can be part of it ... > >> + } >> if (info->has_status) { >> monitor_printf(mon, "Migration status: %s\n", info->status); >> } >> @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon) >> qapi_free_MigrationInfo(info); >> } >> >> +void hmp_info_migration_capabilities(Monitor *mon) >> +{ >> + MigrationCapabilityInfoList *caps_list, *cap; >> + >> + caps_list = qmp_query_migration_capabilities(NULL); >> + if (!caps_list) { >> + monitor_printf(mon, "No migration capabilities found\n"); >> + return; >> + } >> + >> + for (cap = caps_list; cap; cap = cap->next) { >> + monitor_printf(mon, "%s: %s ", >> + MigrationCapability_lookup[cap->value->capability], >> + cap->value->state ? "on" : "off"); >> + } >> + >> + qapi_free_MigrationCapabilityInfoList(caps_list); >> +} >> + >> void hmp_info_cpus(Monitor *mon) >> { >> CpuInfoList *cpu_list, *cpu; >> @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict) >> qmp_migrate_set_speed(value, NULL); >> } >> >> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) >> +{ >> + const char *cap = qdict_get_str(qdict, "capability"); >> + bool state = qdict_get_bool(qdict, "state"); >> + Error *err = NULL; >> + MigrationCapabilityInfoList *params = NULL; >> + int i; >> + >> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >> + if (strcmp(cap, MigrationCapability_lookup[i]) == 0) { >> + if (!params) { >> + params = g_malloc0(sizeof(*params)); >> + } >> + params->value = g_malloc0(sizeof(*params->value)); >> + params->value->capability = i; >> + params->value->state = state; >> + params->next = NULL; >> + qmp_migrate_set_parameters(params, &err); >> + break; >> + } >> + } >> + >> + if (i == MIGRATION_CAPABILITY_MAX) { >> + error_set(&err, QERR_INVALID_PARAMETER, cap); >> + } >> + >> + qapi_free_MigrationCapabilityInfoList(params); >> + >> + if (err) { >> + monitor_printf(mon, "migrate_set_parameter: %s\n", >> + error_get_pretty(err)); >> + error_free(err); >> + } >> +} >> + >> void hmp_set_password(Monitor *mon, const QDict *qdict) >> { >> const char *protocol = qdict_get_str(qdict, "protocol"); >> diff --git a/hmp.h b/hmp.h >> index 79d138d..09ba198 100644 >> --- a/hmp.h >> +++ b/hmp.h >> @@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon); >> void hmp_info_chardev(Monitor *mon); >> void hmp_info_mice(Monitor *mon); >> void hmp_info_migrate(Monitor *mon); >> +void hmp_info_migration_capabilities(Monitor *mon); >> void hmp_info_cpus(Monitor *mon); >> void hmp_info_block(Monitor *mon); >> void hmp_info_blockstats(Monitor *mon); >> @@ -51,6 +52,7 @@ void hmp_snapshot_blkdev(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_speed(Monitor *mon, const QDict *qdict); >> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict); >> void hmp_set_password(Monitor *mon, const QDict *qdict); >> void hmp_expire_password(Monitor *mon, const QDict *qdict); >> void hmp_eject(Monitor *mon, const QDict *qdict); >> diff --git a/migration.c b/migration.c >> index 8db1b43..fd004d7 100644 >> --- a/migration.c >> +++ b/migration.c >> @@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp) >> { >> MigrationInfo *info = g_malloc0(sizeof(*info)); >> MigrationState *s = migrate_get_current(); >> + int i; >> >> switch (s->state) { >> case MIG_STATE_SETUP: >> - /* no migration has happened ever */ >> + /* no migration has ever happened; show enabled capabilities */ >> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >> + if (!info->has_capabilities) { >> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >> + info->has_capabilities = true; >> + } >> + info->capabilities->value = >> + g_malloc(sizeof(*info->capabilities->value)); >> + info->capabilities->value->capability = i; >> + info->capabilities->value->state = s->enabled_capabilities[i]; >> + info->capabilities->next = NULL; >> + } >> break; >> case MIG_STATE_ACTIVE: >> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >> + if (!info->has_capabilities) { >> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >> + info->has_capabilities = true; >> + } >> + info->capabilities->value = >> + g_malloc(sizeof(*info->capabilities->value)); >> + info->capabilities->value->capability = i; >> + info->capabilities->value->state = s->enabled_capabilities[i]; >> + info->capabilities->next = NULL; >> + } >> + >> info->has_status = true; >> info->status = g_strdup("active"); >> >> @@ -143,6 +167,18 @@ MigrationInfo *qmp_query_migrate(Error **errp) >> } >> break; >> case MIG_STATE_COMPLETED: >> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >> + if (!info->has_capabilities) { >> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >> + info->has_capabilities = true; >> + } >> + info->capabilities->value = >> + g_malloc(sizeof(*info->capabilities->value)); >> + info->capabilities->value->capability = i; >> + info->capabilities->value->state = s->enabled_capabilities[i]; >> + info->capabilities->next = NULL; >> + } > > Code triplication :) > > Why is this is needed? Isn't query-migration-capabilities good enough? > Besides, query-migrate should only contain information about current migration > process... > see above >> + >> info->has_status = true; >> info->status = g_strdup("completed"); >> >> @@ -166,6 +202,33 @@ MigrationInfo *qmp_query_migrate(Error **errp) >> return info; >> } >> >> +MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp) >> +{ >> + MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list)); >> + >> + caps_list->value = g_malloc(sizeof(*caps_list->value)); >> + caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE; >> + caps_list->next = NULL; > > Shouldn't this get the capabilities array from migrate_get_current()? > > I mean, this makes query-migration-capabilities always return true for > xbzrle, even if we set it to off. Those are the general capabilities , if I want to use xbzrle ,does this QEMU support it ? this is required because we need both the destination and the source to be able to handle it. If one QEMU doesn't (older version of qemu, maybe it will be disbaled for certain architectures) we can't use the feature. This is for management and the user to check what migration capabilities this QEMU supports. > >> + >> + return caps_list; >> +} >> + >> +void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params, >> + Error **errp) >> +{ >> + MigrationState *s = migrate_get_current(); >> + MigrationCapabilityInfoList *cap; >> + >> + if (s->state == MIG_STATE_ACTIVE) { >> + error_set(errp, QERR_MIGRATION_ACTIVE); >> + return; >> + } >> + >> + for (cap = params; cap; cap = cap->next) { >> + s->enabled_capabilities[cap->value->capability] = cap->value->state; >> + } >> +} >> + >> /* shared migration helpers */ >> >> static int migrate_fd_cleanup(MigrationState *s) >> @@ -375,12 +438,17 @@ static MigrationState *migrate_init(const MigrationParams *params) >> { >> MigrationState *s = migrate_get_current(); >> int64_t bandwidth_limit = s->bandwidth_limit; >> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; >> + >> + memcpy(enabled_capabilities, s->enabled_capabilities, >> + sizeof(enabled_capabilities)); >> >> memset(s, 0, sizeof(*s)); >> s->bandwidth_limit = bandwidth_limit; >> s->params = *params; >> + memcpy(s->enabled_capabilities, enabled_capabilities, >> + sizeof(enabled_capabilities)); >> >> - s->bandwidth_limit = bandwidth_limit; >> s->state = MIG_STATE_SETUP; >> s->total_time = qemu_get_clock_ms(rt_clock); >> >> diff --git a/migration.h b/migration.h >> index 57572a6..713aae0 100644 >> --- a/migration.h >> +++ b/migration.h >> @@ -19,6 +19,7 @@ >> #include "notify.h" >> #include "error.h" >> #include "vmstate.h" >> +#include "qapi-types.h" >> >> struct MigrationParams { >> bool blk; >> @@ -39,6 +40,7 @@ struct MigrationState >> void *opaque; >> MigrationParams params; >> int64_t total_time; >> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; >> }; >> >> void process_incoming_migration(QEMUFile *f); >> diff --git a/monitor.c b/monitor.c >> index f6107ba..e2be6cd 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = { >> .mhandler.info = hmp_info_migrate, >> }, >> { >> + .name = "migration_capabilities", > > migration-capabilities is better. ok > >> + .args_type = "", >> + .params = "", >> + .help = "show migration capabilities", >> + .mhandler.info = hmp_info_migration_capabilities, >> + }, >> + { >> .name = "balloon", >> .args_type = "", >> .params = "", >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 1ab5dbd..a8408fd 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -288,11 +288,15 @@ >> # status, only returned if status is 'active' and it is a block >> # migration >> # >> -# Since: 0.14.0 >> +# @capabilities: #optional a list describing all the migration capabilities >> +# state > > I don't think this is needed, as I've said above. see above > >> +# >> +# Since: 0.14.0, 'capabilities' since 1.2 >> ## >> { 'type': 'MigrationInfo', >> 'data': {'*status': 'str', '*ram': 'MigrationStats', >> - '*disk': 'MigrationStats'} } >> + '*disk': 'MigrationStats', >> + '*capabilities': ['MigrationCapabilityInfo']} } >> >> ## >> # @query-migrate >> @@ -306,6 +310,51 @@ >> { 'command': 'query-migrate', 'returns': 'MigrationInfo' } >> >> ## >> +# @MigrationCapability >> +# >> +# Migration capabilities enumeration >> +# >> +# @xbzrle: current migration supports xbzrle > > You should explain what xbzrle is. ok > >> +# >> +# Since: 1.2 >> +## >> +{ 'enum': 'MigrationCapability', >> + 'data': ['xbzrle'] } >> + >> +## >> +# @MigrationCapabilityInfo > > MigrationCapabilityStatus? > >> +# >> +# Migration capability information >> +# >> +# @capability: capability enum > > Please, document state too. ok Orit > >> +# >> +# Since: 1.2 >> +## >> +{ 'type': 'MigrationCapabilityInfo', >> + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } } >> + >> +## >> +# @query-migration-capabilities >> +# >> +# Returns information about current migration process capabilties. >> +# >> +# Returns: @MigrationCapabilityInfo list >> +# >> +# Since: 1.2 >> +## >> +{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] } >> + >> +## >> +# @migrate_set_parameters >> +# >> +# Enable/Disable the following migration capabilities (like xbzrle) >> +# >> +# Since: 1.2 >> +## >> +{ 'command': 'migrate-set-parameters', >> + 'data': { 'capabilities': ['MigrationCapabilityInfo'] } } >> + >> +## >> # @MouseInfo: >> # >> # Information about a mouse device. >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 2e1a38e..3ee6e00 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -2075,18 +2075,31 @@ The main json-object contains the following: >> - "transferred": amount transferred (json-int) >> - "remaining": amount remaining (json-int) >> - "total": total (json-int) >> - >> +- "capabilities": migration capabilities state >> + - "xbzrle" : XBZRLE state (json-bool) >> Examples: >> >> 1. Before the first migration >> >> -> { "execute": "query-migrate" } >> -<- { "return": {} } >> +<- { "return": { >> + "capabilities" : [ { "capability" : "xbzrle", "state" : false } ] >> + } >> + } >> >> 2. Migration is done and has succeeded >> >> -> { "execute": "query-migrate" } >> -<- { "return": { "status": "completed" } } >> +<- { "return": { >> + "status": "completed", >> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >> + "ram":{ >> + "transferred":123, >> + "remaining":123, >> + "total":246 >> + } >> + } >> + } >> >> 3. Migration is done and has failed >> >> @@ -2099,6 +2112,7 @@ Examples: >> <- { >> "return":{ >> "status":"active", >> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >> "ram":{ >> "transferred":123, >> "remaining":123, >> @@ -2113,6 +2127,7 @@ Examples: >> <- { >> "return":{ >> "status":"active", >> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >> "ram":{ >> "total":1057024, >> "remaining":1053304, >> @@ -2135,6 +2150,56 @@ EQMP >> }, >> >> SQMP >> +query-migration-capabilities >> +------- >> + >> +Query migration capabilities >> + >> +- "xbzrle": xbzrle support >> + >> +Arguments: >> + >> +Example: >> + >> +-> { "execute": "query-migration-capabilities"} >> +<- { "return": [ { "capability": "xbzrle", "state": true }, >> + { "capability": "foobar", "state": false } ] } >> + >> +EQMP >> + >> + { >> + .name = "query-migration-capabilities", >> + .args_type = "", >> + .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities, >> + }, >> + >> +SQMP >> +migrate_set_parameters >> +------- >> + >> +Enable/Disable migration capabilities >> + >> +- "xbzrle": xbzrle support >> + >> +Arguments: >> + >> +Example: >> + >> +-> { "execute": "migrate_set_parameters" , "arguments": >> + { "parameters": [ { "capability": "xbzrle", "state": true } ] } } >> + >> +EQMP >> + >> + { >> + .name = "migrate_set_parameters", >> + .args_type = "parameters:O", >> + .params = "capability:s,state:b", >> + .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, >> + }, >> + >> + >> + >> +SQMP >> query-balloon >> ------------- >> >
On Tue, 24 Jul 2012 09:25:11 +0300 Orit Wasserman <owasserm@redhat.com> wrote: > On 07/23/2012 09:23 PM, Luiz Capitulino wrote: > > On Fri, 13 Jul 2012 09:23:35 +0200 > > Juan Quintela <quintela@redhat.com> wrote: > > > >> From: Orit Wasserman <owasserm@redhat.com> > >> > >> Add migration capabilities that can be queried by the management. > >> The management can query the source QEMU and the destination QEMU in order to > >> verify both support some migration capability (currently only XBZRLE). > >> The management can enable a capability for the next migration by using > >> migrate_set_parameter command. > > > > Please, split this into one command per-patch. Otherwise it's difficult to > > review. > > > Sure. > > Have libvirt folks acked this approach btw? It looks fine to me, but we need > > their ack too. > > > > More comments below. > > > >> > >> Signed-off-by: Orit Wasserman <owasserm@redhat.com> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> hmp-commands.hx | 16 ++++++++++++ > >> hmp.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ > >> hmp.h | 2 ++ > >> migration.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- > >> migration.h | 2 ++ > >> monitor.c | 7 ++++++ > >> qapi-schema.json | 53 ++++++++++++++++++++++++++++++++++++++-- > >> qmp-commands.hx | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++--- > >> 8 files changed, 280 insertions(+), 7 deletions(-) > >> > >> diff --git a/hmp-commands.hx b/hmp-commands.hx > >> index f5d9d91..9245bef 100644 > >> --- a/hmp-commands.hx > >> +++ b/hmp-commands.hx > >> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration. > >> ETEXI > >> > >> { > >> + .name = "migrate_set_parameter", > >> + .args_type = "capability:s,state:b", > >> + .params = "", > > > > Please, fill in params. > ok > > > >> + .help = "Enable/Disable the usage of a capability for migration", > >> + .mhandler.cmd = hmp_migrate_set_parameter, > >> + }, > >> + > >> +STEXI > >> +@item migrate_set_parameter @var{capability} @var{state} > >> +@findex migrate_set_parameter > >> +Enable/Disable the usage of a capability @var{capability} for migration. > >> +ETEXI > >> + > >> + { > >> .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", > >> @@ -1419,6 +1433,8 @@ show CPU statistics > >> show user network stack connection states > >> @item info migrate > >> show migration status > >> +@item info migration_capabilities > >> +show migration capabilities > >> @item info balloon > >> show balloon information > >> @item info qtree > >> diff --git a/hmp.c b/hmp.c > >> index 4c6d4ae..b0440e6 100644 > >> --- a/hmp.c > >> +++ b/hmp.c > >> @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon) > >> void hmp_info_migrate(Monitor *mon) > >> { > >> MigrationInfo *info; > >> + MigrationCapabilityInfoList *cap; > >> > >> info = qmp_query_migrate(NULL); > >> > >> + if (info->has_capabilities && info->capabilities) { > >> + monitor_printf(mon, "capabilities: "); > >> + for (cap = info->capabilities; cap; cap = cap->next) { > >> + monitor_printf(mon, "%s: %s ", > >> + MigrationCapability_lookup[cap->value->capability], > >> + cap->value->state ? "on" : "off"); > >> + } > >> + monitor_printf(mon, "\n"); > > > > Why is this is needed? Isn't info migration-capabilities good enough? > > Besides, info migrate should only contain information about current migration > > process... > The reason we introduced capabilities is that xbzrle needs for both source and destination QEMU > to be able to handle it. Even if both side support xbzrle the user may decide not to use it. > User that wants to use xbzrle needs to check that both sides have support for it (using info capabilities) , than > enable it in both sides (using migrate-set-parameter/s commands). This is a parameter for the current migration. > So the user needs to know if xbzrle was enabled or disabled for the current migration, this code displays it. But this is being returned even when there is no migration taking place. I'm answering this here in the 'info migrate' hunk, but my main concern is query-migrate. query-migrate shouldn't return this info in MIG_STATE_SETUP, and for MIG_STATE_COMPLETED it should only be returned if there was a migration. > some day when there will be better migration protocol, feature negotiations can be part of it ... > > > >> + } > >> if (info->has_status) { > >> monitor_printf(mon, "Migration status: %s\n", info->status); > >> } > >> @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon) > >> qapi_free_MigrationInfo(info); > >> } > >> > >> +void hmp_info_migration_capabilities(Monitor *mon) > >> +{ > >> + MigrationCapabilityInfoList *caps_list, *cap; > >> + > >> + caps_list = qmp_query_migration_capabilities(NULL); > >> + if (!caps_list) { > >> + monitor_printf(mon, "No migration capabilities found\n"); > >> + return; > >> + } > >> + > >> + for (cap = caps_list; cap; cap = cap->next) { > >> + monitor_printf(mon, "%s: %s ", > >> + MigrationCapability_lookup[cap->value->capability], > >> + cap->value->state ? "on" : "off"); > >> + } > >> + > >> + qapi_free_MigrationCapabilityInfoList(caps_list); > >> +} > >> + > >> void hmp_info_cpus(Monitor *mon) > >> { > >> CpuInfoList *cpu_list, *cpu; > >> @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict) > >> qmp_migrate_set_speed(value, NULL); > >> } > >> > >> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > >> +{ > >> + const char *cap = qdict_get_str(qdict, "capability"); > >> + bool state = qdict_get_bool(qdict, "state"); > >> + Error *err = NULL; > >> + MigrationCapabilityInfoList *params = NULL; > >> + int i; > >> + > >> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >> + if (strcmp(cap, MigrationCapability_lookup[i]) == 0) { > >> + if (!params) { > >> + params = g_malloc0(sizeof(*params)); > >> + } > >> + params->value = g_malloc0(sizeof(*params->value)); > >> + params->value->capability = i; > >> + params->value->state = state; > >> + params->next = NULL; > >> + qmp_migrate_set_parameters(params, &err); > >> + break; > >> + } > >> + } > >> + > >> + if (i == MIGRATION_CAPABILITY_MAX) { > >> + error_set(&err, QERR_INVALID_PARAMETER, cap); > >> + } > >> + > >> + qapi_free_MigrationCapabilityInfoList(params); > >> + > >> + if (err) { > >> + monitor_printf(mon, "migrate_set_parameter: %s\n", > >> + error_get_pretty(err)); > >> + error_free(err); > >> + } > >> +} > >> + > >> void hmp_set_password(Monitor *mon, const QDict *qdict) > >> { > >> const char *protocol = qdict_get_str(qdict, "protocol"); > >> diff --git a/hmp.h b/hmp.h > >> index 79d138d..09ba198 100644 > >> --- a/hmp.h > >> +++ b/hmp.h > >> @@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon); > >> void hmp_info_chardev(Monitor *mon); > >> void hmp_info_mice(Monitor *mon); > >> void hmp_info_migrate(Monitor *mon); > >> +void hmp_info_migration_capabilities(Monitor *mon); > >> void hmp_info_cpus(Monitor *mon); > >> void hmp_info_block(Monitor *mon); > >> void hmp_info_blockstats(Monitor *mon); > >> @@ -51,6 +52,7 @@ void hmp_snapshot_blkdev(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_speed(Monitor *mon, const QDict *qdict); > >> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict); > >> void hmp_set_password(Monitor *mon, const QDict *qdict); > >> void hmp_expire_password(Monitor *mon, const QDict *qdict); > >> void hmp_eject(Monitor *mon, const QDict *qdict); > >> diff --git a/migration.c b/migration.c > >> index 8db1b43..fd004d7 100644 > >> --- a/migration.c > >> +++ b/migration.c > >> @@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp) > >> { > >> MigrationInfo *info = g_malloc0(sizeof(*info)); > >> MigrationState *s = migrate_get_current(); > >> + int i; > >> > >> switch (s->state) { > >> case MIG_STATE_SETUP: > >> - /* no migration has happened ever */ > >> + /* no migration has ever happened; show enabled capabilities */ > >> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >> + if (!info->has_capabilities) { > >> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > >> + info->has_capabilities = true; > >> + } > >> + info->capabilities->value = > >> + g_malloc(sizeof(*info->capabilities->value)); > >> + info->capabilities->value->capability = i; > >> + info->capabilities->value->state = s->enabled_capabilities[i]; > >> + info->capabilities->next = NULL; > >> + } > >> break; > >> case MIG_STATE_ACTIVE: > >> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >> + if (!info->has_capabilities) { > >> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > >> + info->has_capabilities = true; > >> + } > >> + info->capabilities->value = > >> + g_malloc(sizeof(*info->capabilities->value)); > >> + info->capabilities->value->capability = i; > >> + info->capabilities->value->state = s->enabled_capabilities[i]; > >> + info->capabilities->next = NULL; > >> + } > >> + > >> info->has_status = true; > >> info->status = g_strdup("active"); > >> > >> @@ -143,6 +167,18 @@ MigrationInfo *qmp_query_migrate(Error **errp) > >> } > >> break; > >> case MIG_STATE_COMPLETED: > >> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >> + if (!info->has_capabilities) { > >> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > >> + info->has_capabilities = true; > >> + } > >> + info->capabilities->value = > >> + g_malloc(sizeof(*info->capabilities->value)); > >> + info->capabilities->value->capability = i; > >> + info->capabilities->value->state = s->enabled_capabilities[i]; > >> + info->capabilities->next = NULL; > >> + } > > > > Code triplication :) > > > > Why is this is needed? Isn't query-migration-capabilities good enough? > > Besides, query-migrate should only contain information about current migration > > process... > > > see above > >> + > >> info->has_status = true; > >> info->status = g_strdup("completed"); > >> > >> @@ -166,6 +202,33 @@ MigrationInfo *qmp_query_migrate(Error **errp) > >> return info; > >> } > >> > >> +MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp) > >> +{ > >> + MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list)); > >> + > >> + caps_list->value = g_malloc(sizeof(*caps_list->value)); > >> + caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE; > >> + caps_list->next = NULL; > > > > Shouldn't this get the capabilities array from migrate_get_current()? > > > > I mean, this makes query-migration-capabilities always return true for > > xbzrle, even if we set it to off. > Those are the general capabilities , if I want to use xbzrle ,does this QEMU support it ? > this is required because we need both the destination and the source to be able to handle it. > If one QEMU doesn't (older version of qemu, maybe it will be disbaled for certain architectures) we can't use the feature. > This is for management and the user to check what migration capabilities this QEMU supports. My point was that query-migration-capabilities will always return state=true for all existing capabilities. Don't we want users/mngt app to know which states have been turned off? > > > > >> + > >> + return caps_list; > >> +} > >> + > >> +void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params, > >> + Error **errp) > >> +{ > >> + MigrationState *s = migrate_get_current(); > >> + MigrationCapabilityInfoList *cap; > >> + > >> + if (s->state == MIG_STATE_ACTIVE) { > >> + error_set(errp, QERR_MIGRATION_ACTIVE); > >> + return; > >> + } > >> + > >> + for (cap = params; cap; cap = cap->next) { > >> + s->enabled_capabilities[cap->value->capability] = cap->value->state; > >> + } > >> +} > >> + > >> /* shared migration helpers */ > >> > >> static int migrate_fd_cleanup(MigrationState *s) > >> @@ -375,12 +438,17 @@ static MigrationState *migrate_init(const MigrationParams *params) > >> { > >> MigrationState *s = migrate_get_current(); > >> int64_t bandwidth_limit = s->bandwidth_limit; > >> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; > >> + > >> + memcpy(enabled_capabilities, s->enabled_capabilities, > >> + sizeof(enabled_capabilities)); > >> > >> memset(s, 0, sizeof(*s)); > >> s->bandwidth_limit = bandwidth_limit; > >> s->params = *params; > >> + memcpy(s->enabled_capabilities, enabled_capabilities, > >> + sizeof(enabled_capabilities)); > >> > >> - s->bandwidth_limit = bandwidth_limit; > >> s->state = MIG_STATE_SETUP; > >> s->total_time = qemu_get_clock_ms(rt_clock); > >> > >> diff --git a/migration.h b/migration.h > >> index 57572a6..713aae0 100644 > >> --- a/migration.h > >> +++ b/migration.h > >> @@ -19,6 +19,7 @@ > >> #include "notify.h" > >> #include "error.h" > >> #include "vmstate.h" > >> +#include "qapi-types.h" > >> > >> struct MigrationParams { > >> bool blk; > >> @@ -39,6 +40,7 @@ struct MigrationState > >> void *opaque; > >> MigrationParams params; > >> int64_t total_time; > >> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; > >> }; > >> > >> void process_incoming_migration(QEMUFile *f); > >> diff --git a/monitor.c b/monitor.c > >> index f6107ba..e2be6cd 100644 > >> --- a/monitor.c > >> +++ b/monitor.c > >> @@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = { > >> .mhandler.info = hmp_info_migrate, > >> }, > >> { > >> + .name = "migration_capabilities", > > > > migration-capabilities is better. > ok > > > >> + .args_type = "", > >> + .params = "", > >> + .help = "show migration capabilities", > >> + .mhandler.info = hmp_info_migration_capabilities, > >> + }, > >> + { > >> .name = "balloon", > >> .args_type = "", > >> .params = "", > >> diff --git a/qapi-schema.json b/qapi-schema.json > >> index 1ab5dbd..a8408fd 100644 > >> --- a/qapi-schema.json > >> +++ b/qapi-schema.json > >> @@ -288,11 +288,15 @@ > >> # status, only returned if status is 'active' and it is a block > >> # migration > >> # > >> -# Since: 0.14.0 > >> +# @capabilities: #optional a list describing all the migration capabilities > >> +# state > > > > I don't think this is needed, as I've said above. > see above > > > >> +# > >> +# Since: 0.14.0, 'capabilities' since 1.2 > >> ## > >> { 'type': 'MigrationInfo', > >> 'data': {'*status': 'str', '*ram': 'MigrationStats', > >> - '*disk': 'MigrationStats'} } > >> + '*disk': 'MigrationStats', > >> + '*capabilities': ['MigrationCapabilityInfo']} } > >> > >> ## > >> # @query-migrate > >> @@ -306,6 +310,51 @@ > >> { 'command': 'query-migrate', 'returns': 'MigrationInfo' } > >> > >> ## > >> +# @MigrationCapability > >> +# > >> +# Migration capabilities enumeration > >> +# > >> +# @xbzrle: current migration supports xbzrle > > > > You should explain what xbzrle is. > ok > > > >> +# > >> +# Since: 1.2 > >> +## > >> +{ 'enum': 'MigrationCapability', > >> + 'data': ['xbzrle'] } > >> + > >> +## > >> +# @MigrationCapabilityInfo > > > > MigrationCapabilityStatus? > > > >> +# > >> +# Migration capability information > >> +# > >> +# @capability: capability enum > > > > Please, document state too. > ok > > Orit > > > >> +# > >> +# Since: 1.2 > >> +## > >> +{ 'type': 'MigrationCapabilityInfo', > >> + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } } > >> + > >> +## > >> +# @query-migration-capabilities > >> +# > >> +# Returns information about current migration process capabilties. > >> +# > >> +# Returns: @MigrationCapabilityInfo list > >> +# > >> +# Since: 1.2 > >> +## > >> +{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] } > >> + > >> +## > >> +# @migrate_set_parameters > >> +# > >> +# Enable/Disable the following migration capabilities (like xbzrle) > >> +# > >> +# Since: 1.2 > >> +## > >> +{ 'command': 'migrate-set-parameters', > >> + 'data': { 'capabilities': ['MigrationCapabilityInfo'] } } > >> + > >> +## > >> # @MouseInfo: > >> # > >> # Information about a mouse device. > >> diff --git a/qmp-commands.hx b/qmp-commands.hx > >> index 2e1a38e..3ee6e00 100644 > >> --- a/qmp-commands.hx > >> +++ b/qmp-commands.hx > >> @@ -2075,18 +2075,31 @@ The main json-object contains the following: > >> - "transferred": amount transferred (json-int) > >> - "remaining": amount remaining (json-int) > >> - "total": total (json-int) > >> - > >> +- "capabilities": migration capabilities state > >> + - "xbzrle" : XBZRLE state (json-bool) > >> Examples: > >> > >> 1. Before the first migration > >> > >> -> { "execute": "query-migrate" } > >> -<- { "return": {} } > >> +<- { "return": { > >> + "capabilities" : [ { "capability" : "xbzrle", "state" : false } ] > >> + } > >> + } > >> > >> 2. Migration is done and has succeeded > >> > >> -> { "execute": "query-migrate" } > >> -<- { "return": { "status": "completed" } } > >> +<- { "return": { > >> + "status": "completed", > >> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > >> + "ram":{ > >> + "transferred":123, > >> + "remaining":123, > >> + "total":246 > >> + } > >> + } > >> + } > >> > >> 3. Migration is done and has failed > >> > >> @@ -2099,6 +2112,7 @@ Examples: > >> <- { > >> "return":{ > >> "status":"active", > >> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > >> "ram":{ > >> "transferred":123, > >> "remaining":123, > >> @@ -2113,6 +2127,7 @@ Examples: > >> <- { > >> "return":{ > >> "status":"active", > >> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > >> "ram":{ > >> "total":1057024, > >> "remaining":1053304, > >> @@ -2135,6 +2150,56 @@ EQMP > >> }, > >> > >> SQMP > >> +query-migration-capabilities > >> +------- > >> + > >> +Query migration capabilities > >> + > >> +- "xbzrle": xbzrle support > >> + > >> +Arguments: > >> + > >> +Example: > >> + > >> +-> { "execute": "query-migration-capabilities"} > >> +<- { "return": [ { "capability": "xbzrle", "state": true }, > >> + { "capability": "foobar", "state": false } ] } > >> + > >> +EQMP > >> + > >> + { > >> + .name = "query-migration-capabilities", > >> + .args_type = "", > >> + .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities, > >> + }, > >> + > >> +SQMP > >> +migrate_set_parameters > >> +------- > >> + > >> +Enable/Disable migration capabilities > >> + > >> +- "xbzrle": xbzrle support > >> + > >> +Arguments: > >> + > >> +Example: > >> + > >> +-> { "execute": "migrate_set_parameters" , "arguments": > >> + { "parameters": [ { "capability": "xbzrle", "state": true } ] } } > >> + > >> +EQMP > >> + > >> + { > >> + .name = "migrate_set_parameters", > >> + .args_type = "parameters:O", > >> + .params = "capability:s,state:b", > >> + .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, > >> + }, > >> + > >> + > >> + > >> +SQMP > >> query-balloon > >> ------------- > >> > > > >
On 07/24/2012 03:50 PM, Luiz Capitulino wrote: > On Tue, 24 Jul 2012 09:25:11 +0300 > Orit Wasserman <owasserm@redhat.com> wrote: > >> On 07/23/2012 09:23 PM, Luiz Capitulino wrote: >>> On Fri, 13 Jul 2012 09:23:35 +0200 >>> Juan Quintela <quintela@redhat.com> wrote: >>> >>>> From: Orit Wasserman <owasserm@redhat.com> >>>> >>>> Add migration capabilities that can be queried by the management. >>>> The management can query the source QEMU and the destination QEMU in order to >>>> verify both support some migration capability (currently only XBZRLE). >>>> The management can enable a capability for the next migration by using >>>> migrate_set_parameter command. >>> >>> Please, split this into one command per-patch. Otherwise it's difficult to >>> review. >>> >> Sure. >>> Have libvirt folks acked this approach btw? It looks fine to me, but we need >>> their ack too. >>> >>> More comments below. >>> >>>> >>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com> >>>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>>> --- >>>> hmp-commands.hx | 16 ++++++++++++ >>>> hmp.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>> hmp.h | 2 ++ >>>> migration.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- >>>> migration.h | 2 ++ >>>> monitor.c | 7 ++++++ >>>> qapi-schema.json | 53 ++++++++++++++++++++++++++++++++++++++-- >>>> qmp-commands.hx | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++--- >>>> 8 files changed, 280 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/hmp-commands.hx b/hmp-commands.hx >>>> index f5d9d91..9245bef 100644 >>>> --- a/hmp-commands.hx >>>> +++ b/hmp-commands.hx >>>> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration. >>>> ETEXI >>>> >>>> { >>>> + .name = "migrate_set_parameter", >>>> + .args_type = "capability:s,state:b", >>>> + .params = "", >>> >>> Please, fill in params. >> ok >>> >>>> + .help = "Enable/Disable the usage of a capability for migration", >>>> + .mhandler.cmd = hmp_migrate_set_parameter, >>>> + }, >>>> + >>>> +STEXI >>>> +@item migrate_set_parameter @var{capability} @var{state} >>>> +@findex migrate_set_parameter >>>> +Enable/Disable the usage of a capability @var{capability} for migration. >>>> +ETEXI >>>> + >>>> + { >>>> .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", >>>> @@ -1419,6 +1433,8 @@ show CPU statistics >>>> show user network stack connection states >>>> @item info migrate >>>> show migration status >>>> +@item info migration_capabilities >>>> +show migration capabilities >>>> @item info balloon >>>> show balloon information >>>> @item info qtree >>>> diff --git a/hmp.c b/hmp.c >>>> index 4c6d4ae..b0440e6 100644 >>>> --- a/hmp.c >>>> +++ b/hmp.c >>>> @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon) >>>> void hmp_info_migrate(Monitor *mon) >>>> { >>>> MigrationInfo *info; >>>> + MigrationCapabilityInfoList *cap; >>>> >>>> info = qmp_query_migrate(NULL); >>>> >>>> + if (info->has_capabilities && info->capabilities) { >>>> + monitor_printf(mon, "capabilities: "); >>>> + for (cap = info->capabilities; cap; cap = cap->next) { >>>> + monitor_printf(mon, "%s: %s ", >>>> + MigrationCapability_lookup[cap->value->capability], >>>> + cap->value->state ? "on" : "off"); >>>> + } >>>> + monitor_printf(mon, "\n"); >>> >>> Why is this is needed? Isn't info migration-capabilities good enough? >>> Besides, info migrate should only contain information about current migration >>> process... >> The reason we introduced capabilities is that xbzrle needs for both source and destination QEMU >> to be able to handle it. Even if both side support xbzrle the user may decide not to use it. >> User that wants to use xbzrle needs to check that both sides have support for it (using info capabilities) , than >> enable it in both sides (using migrate-set-parameter/s commands). This is a parameter for the current migration. >> So the user needs to know if xbzrle was enabled or disabled for the current migration, this code displays it. > > But this is being returned even when there is no migration taking place. I'm > answering this here in the 'info migrate' hunk, but my main concern is > query-migrate. I think migration configuration is part of migration information and should be available to the user in 'info migration'. Especially before he starts running it to see if he need to change the configuration. Also it is very helpful for the user during the migration and also after it's completes. There is no way today to query the downtime and migration speed at all . you can always set them again but there is no way to know how it is configured. I was actually thinking to add them to info migration (also for SETUP). > > query-migrate shouldn't return this info in MIG_STATE_SETUP, and for > MIG_STATE_COMPLETED it should only be returned if there was a migration. I think is case of MIG_SETUP we should return the configuration and params for the next migration. MIG_STATE_COMPLETED happens only if there was a successful migration other wise there will be another state. > >> some day when there will be better migration protocol, feature negotiations can be part of it ... >>> >>>> + } >>>> if (info->has_status) { >>>> monitor_printf(mon, "Migration status: %s\n", info->status); >>>> } >>>> @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon) >>>> qapi_free_MigrationInfo(info); >>>> } >>>> >>>> +void hmp_info_migration_capabilities(Monitor *mon) >>>> +{ >>>> + MigrationCapabilityInfoList *caps_list, *cap; >>>> + >>>> + caps_list = qmp_query_migration_capabilities(NULL); >>>> + if (!caps_list) { >>>> + monitor_printf(mon, "No migration capabilities found\n"); >>>> + return; >>>> + } >>>> + >>>> + for (cap = caps_list; cap; cap = cap->next) { >>>> + monitor_printf(mon, "%s: %s ", >>>> + MigrationCapability_lookup[cap->value->capability], >>>> + cap->value->state ? "on" : "off"); >>>> + } >>>> + >>>> + qapi_free_MigrationCapabilityInfoList(caps_list); >>>> +} >>>> + >>>> void hmp_info_cpus(Monitor *mon) >>>> { >>>> CpuInfoList *cpu_list, *cpu; >>>> @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict) >>>> qmp_migrate_set_speed(value, NULL); >>>> } >>>> >>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) >>>> +{ >>>> + const char *cap = qdict_get_str(qdict, "capability"); >>>> + bool state = qdict_get_bool(qdict, "state"); >>>> + Error *err = NULL; >>>> + MigrationCapabilityInfoList *params = NULL; >>>> + int i; >>>> + >>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>> + if (strcmp(cap, MigrationCapability_lookup[i]) == 0) { >>>> + if (!params) { >>>> + params = g_malloc0(sizeof(*params)); >>>> + } >>>> + params->value = g_malloc0(sizeof(*params->value)); >>>> + params->value->capability = i; >>>> + params->value->state = state; >>>> + params->next = NULL; >>>> + qmp_migrate_set_parameters(params, &err); >>>> + break; >>>> + } >>>> + } >>>> + >>>> + if (i == MIGRATION_CAPABILITY_MAX) { >>>> + error_set(&err, QERR_INVALID_PARAMETER, cap); >>>> + } >>>> + >>>> + qapi_free_MigrationCapabilityInfoList(params); >>>> + >>>> + if (err) { >>>> + monitor_printf(mon, "migrate_set_parameter: %s\n", >>>> + error_get_pretty(err)); >>>> + error_free(err); >>>> + } >>>> +} >>>> + >>>> void hmp_set_password(Monitor *mon, const QDict *qdict) >>>> { >>>> const char *protocol = qdict_get_str(qdict, "protocol"); >>>> diff --git a/hmp.h b/hmp.h >>>> index 79d138d..09ba198 100644 >>>> --- a/hmp.h >>>> +++ b/hmp.h >>>> @@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon); >>>> void hmp_info_chardev(Monitor *mon); >>>> void hmp_info_mice(Monitor *mon); >>>> void hmp_info_migrate(Monitor *mon); >>>> +void hmp_info_migration_capabilities(Monitor *mon); >>>> void hmp_info_cpus(Monitor *mon); >>>> void hmp_info_block(Monitor *mon); >>>> void hmp_info_blockstats(Monitor *mon); >>>> @@ -51,6 +52,7 @@ void hmp_snapshot_blkdev(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_speed(Monitor *mon, const QDict *qdict); >>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict); >>>> void hmp_set_password(Monitor *mon, const QDict *qdict); >>>> void hmp_expire_password(Monitor *mon, const QDict *qdict); >>>> void hmp_eject(Monitor *mon, const QDict *qdict); >>>> diff --git a/migration.c b/migration.c >>>> index 8db1b43..fd004d7 100644 >>>> --- a/migration.c >>>> +++ b/migration.c >>>> @@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp) >>>> { >>>> MigrationInfo *info = g_malloc0(sizeof(*info)); >>>> MigrationState *s = migrate_get_current(); >>>> + int i; >>>> >>>> switch (s->state) { >>>> case MIG_STATE_SETUP: >>>> - /* no migration has happened ever */ >>>> + /* no migration has ever happened; show enabled capabilities */ >>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>> + if (!info->has_capabilities) { >>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >>>> + info->has_capabilities = true; >>>> + } >>>> + info->capabilities->value = >>>> + g_malloc(sizeof(*info->capabilities->value)); >>>> + info->capabilities->value->capability = i; >>>> + info->capabilities->value->state = s->enabled_capabilities[i]; >>>> + info->capabilities->next = NULL; >>>> + } >>>> break; >>>> case MIG_STATE_ACTIVE: >>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>> + if (!info->has_capabilities) { >>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >>>> + info->has_capabilities = true; >>>> + } >>>> + info->capabilities->value = >>>> + g_malloc(sizeof(*info->capabilities->value)); >>>> + info->capabilities->value->capability = i; >>>> + info->capabilities->value->state = s->enabled_capabilities[i]; >>>> + info->capabilities->next = NULL; >>>> + } >>>> + >>>> info->has_status = true; >>>> info->status = g_strdup("active"); >>>> >>>> @@ -143,6 +167,18 @@ MigrationInfo *qmp_query_migrate(Error **errp) >>>> } >>>> break; >>>> case MIG_STATE_COMPLETED: >>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>> + if (!info->has_capabilities) { >>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >>>> + info->has_capabilities = true; >>>> + } >>>> + info->capabilities->value = >>>> + g_malloc(sizeof(*info->capabilities->value)); >>>> + info->capabilities->value->capability = i; >>>> + info->capabilities->value->state = s->enabled_capabilities[i]; >>>> + info->capabilities->next = NULL; >>>> + } >>> >>> Code triplication :) >>> >>> Why is this is needed? Isn't query-migration-capabilities good enough? >>> Besides, query-migrate should only contain information about current migration >>> process... >>> >> see above >>>> + >>>> info->has_status = true; >>>> info->status = g_strdup("completed"); >>>> >>>> @@ -166,6 +202,33 @@ MigrationInfo *qmp_query_migrate(Error **errp) >>>> return info; >>>> } >>>> >>>> +MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp) >>>> +{ >>>> + MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list)); >>>> + >>>> + caps_list->value = g_malloc(sizeof(*caps_list->value)); >>>> + caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE; >>>> + caps_list->next = NULL; >>> >>> Shouldn't this get the capabilities array from migrate_get_current()? >>> >>> I mean, this makes query-migration-capabilities always return true for >>> xbzrle, even if we set it to off. >> Those are the general capabilities , if I want to use xbzrle ,does this QEMU support it ? >> this is required because we need both the destination and the source to be able to handle it. >> If one QEMU doesn't (older version of qemu, maybe it will be disbaled for certain architectures) we can't use the feature. >> This is for management and the user to check what migration capabilities this QEMU supports. > > My point was that query-migration-capabilities will always return state=true > for all existing capabilities. Don't we want users/mngt app to know which > states have been turned off? You need to separate the ability to use a capability and actually using it for migration. Also I don't want the user to be able to set some capability on, it depends on the code not user configuration. And for xbzrle we always return true for some version on QEMU , let say we decide we want to remove the code that implement it for some reason in some future version than it will return false. There is a reason the command that enables a capability for a migration was called 'migrate-set-parameter' and not set-capability to distinguish between them one is per QEMU code base and one is per migration. Regards, Orit > >> >>> >>>> + >>>> + return caps_list; >>>> +} >>>> + >>>> +void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params, >>>> + Error **errp) >>>> +{ >>>> + MigrationState *s = migrate_get_current(); >>>> + MigrationCapabilityInfoList *cap; >>>> + >>>> + if (s->state == MIG_STATE_ACTIVE) { >>>> + error_set(errp, QERR_MIGRATION_ACTIVE); >>>> + return; >>>> + } >>>> + >>>> + for (cap = params; cap; cap = cap->next) { >>>> + s->enabled_capabilities[cap->value->capability] = cap->value->state; >>>> + } >>>> +} >>>> + >>>> /* shared migration helpers */ >>>> >>>> static int migrate_fd_cleanup(MigrationState *s) >>>> @@ -375,12 +438,17 @@ static MigrationState *migrate_init(const MigrationParams *params) >>>> { >>>> MigrationState *s = migrate_get_current(); >>>> int64_t bandwidth_limit = s->bandwidth_limit; >>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; >>>> + >>>> + memcpy(enabled_capabilities, s->enabled_capabilities, >>>> + sizeof(enabled_capabilities)); >>>> >>>> memset(s, 0, sizeof(*s)); >>>> s->bandwidth_limit = bandwidth_limit; >>>> s->params = *params; >>>> + memcpy(s->enabled_capabilities, enabled_capabilities, >>>> + sizeof(enabled_capabilities)); >>>> >>>> - s->bandwidth_limit = bandwidth_limit; >>>> s->state = MIG_STATE_SETUP; >>>> s->total_time = qemu_get_clock_ms(rt_clock); >>>> >>>> diff --git a/migration.h b/migration.h >>>> index 57572a6..713aae0 100644 >>>> --- a/migration.h >>>> +++ b/migration.h >>>> @@ -19,6 +19,7 @@ >>>> #include "notify.h" >>>> #include "error.h" >>>> #include "vmstate.h" >>>> +#include "qapi-types.h" >>>> >>>> struct MigrationParams { >>>> bool blk; >>>> @@ -39,6 +40,7 @@ struct MigrationState >>>> void *opaque; >>>> MigrationParams params; >>>> int64_t total_time; >>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; >>>> }; >>>> >>>> void process_incoming_migration(QEMUFile *f); >>>> diff --git a/monitor.c b/monitor.c >>>> index f6107ba..e2be6cd 100644 >>>> --- a/monitor.c >>>> +++ b/monitor.c >>>> @@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = { >>>> .mhandler.info = hmp_info_migrate, >>>> }, >>>> { >>>> + .name = "migration_capabilities", >>> >>> migration-capabilities is better. >> ok >>> >>>> + .args_type = "", >>>> + .params = "", >>>> + .help = "show migration capabilities", >>>> + .mhandler.info = hmp_info_migration_capabilities, >>>> + }, >>>> + { >>>> .name = "balloon", >>>> .args_type = "", >>>> .params = "", >>>> diff --git a/qapi-schema.json b/qapi-schema.json >>>> index 1ab5dbd..a8408fd 100644 >>>> --- a/qapi-schema.json >>>> +++ b/qapi-schema.json >>>> @@ -288,11 +288,15 @@ >>>> # status, only returned if status is 'active' and it is a block >>>> # migration >>>> # >>>> -# Since: 0.14.0 >>>> +# @capabilities: #optional a list describing all the migration capabilities >>>> +# state >>> >>> I don't think this is needed, as I've said above. >> see above >>> >>>> +# >>>> +# Since: 0.14.0, 'capabilities' since 1.2 >>>> ## >>>> { 'type': 'MigrationInfo', >>>> 'data': {'*status': 'str', '*ram': 'MigrationStats', >>>> - '*disk': 'MigrationStats'} } >>>> + '*disk': 'MigrationStats', >>>> + '*capabilities': ['MigrationCapabilityInfo']} } >>>> >>>> ## >>>> # @query-migrate >>>> @@ -306,6 +310,51 @@ >>>> { 'command': 'query-migrate', 'returns': 'MigrationInfo' } >>>> >>>> ## >>>> +# @MigrationCapability >>>> +# >>>> +# Migration capabilities enumeration >>>> +# >>>> +# @xbzrle: current migration supports xbzrle >>> >>> You should explain what xbzrle is. >> ok >>> >>>> +# >>>> +# Since: 1.2 >>>> +## >>>> +{ 'enum': 'MigrationCapability', >>>> + 'data': ['xbzrle'] } >>>> + >>>> +## >>>> +# @MigrationCapabilityInfo >>> >>> MigrationCapabilityStatus? >>> >>>> +# >>>> +# Migration capability information >>>> +# >>>> +# @capability: capability enum >>> >>> Please, document state too. >> ok >> >> Orit >>> >>>> +# >>>> +# Since: 1.2 >>>> +## >>>> +{ 'type': 'MigrationCapabilityInfo', >>>> + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } } >>>> + >>>> +## >>>> +# @query-migration-capabilities >>>> +# >>>> +# Returns information about current migration process capabilties. >>>> +# >>>> +# Returns: @MigrationCapabilityInfo list >>>> +# >>>> +# Since: 1.2 >>>> +## >>>> +{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] } >>>> + >>>> +## >>>> +# @migrate_set_parameters >>>> +# >>>> +# Enable/Disable the following migration capabilities (like xbzrle) >>>> +# >>>> +# Since: 1.2 >>>> +## >>>> +{ 'command': 'migrate-set-parameters', >>>> + 'data': { 'capabilities': ['MigrationCapabilityInfo'] } } >>>> + >>>> +## >>>> # @MouseInfo: >>>> # >>>> # Information about a mouse device. >>>> diff --git a/qmp-commands.hx b/qmp-commands.hx >>>> index 2e1a38e..3ee6e00 100644 >>>> --- a/qmp-commands.hx >>>> +++ b/qmp-commands.hx >>>> @@ -2075,18 +2075,31 @@ The main json-object contains the following: >>>> - "transferred": amount transferred (json-int) >>>> - "remaining": amount remaining (json-int) >>>> - "total": total (json-int) >>>> - >>>> +- "capabilities": migration capabilities state >>>> + - "xbzrle" : XBZRLE state (json-bool) >>>> Examples: >>>> >>>> 1. Before the first migration >>>> >>>> -> { "execute": "query-migrate" } >>>> -<- { "return": {} } >>>> +<- { "return": { >>>> + "capabilities" : [ { "capability" : "xbzrle", "state" : false } ] >>>> + } >>>> + } >>>> >>>> 2. Migration is done and has succeeded >>>> >>>> -> { "execute": "query-migrate" } >>>> -<- { "return": { "status": "completed" } } >>>> +<- { "return": { >>>> + "status": "completed", >>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >>>> + "ram":{ >>>> + "transferred":123, >>>> + "remaining":123, >>>> + "total":246 >>>> + } >>>> + } >>>> + } >>>> >>>> 3. Migration is done and has failed >>>> >>>> @@ -2099,6 +2112,7 @@ Examples: >>>> <- { >>>> "return":{ >>>> "status":"active", >>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >>>> "ram":{ >>>> "transferred":123, >>>> "remaining":123, >>>> @@ -2113,6 +2127,7 @@ Examples: >>>> <- { >>>> "return":{ >>>> "status":"active", >>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >>>> "ram":{ >>>> "total":1057024, >>>> "remaining":1053304, >>>> @@ -2135,6 +2150,56 @@ EQMP >>>> }, >>>> >>>> SQMP >>>> +query-migration-capabilities >>>> +------- >>>> + >>>> +Query migration capabilities >>>> + >>>> +- "xbzrle": xbzrle support >>>> + >>>> +Arguments: >>>> + >>>> +Example: >>>> + >>>> +-> { "execute": "query-migration-capabilities"} >>>> +<- { "return": [ { "capability": "xbzrle", "state": true }, >>>> + { "capability": "foobar", "state": false } ] } >>>> + >>>> +EQMP >>>> + >>>> + { >>>> + .name = "query-migration-capabilities", >>>> + .args_type = "", >>>> + .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities, >>>> + }, >>>> + >>>> +SQMP >>>> +migrate_set_parameters >>>> +------- >>>> + >>>> +Enable/Disable migration capabilities >>>> + >>>> +- "xbzrle": xbzrle support >>>> + >>>> +Arguments: >>>> + >>>> +Example: >>>> + >>>> +-> { "execute": "migrate_set_parameters" , "arguments": >>>> + { "parameters": [ { "capability": "xbzrle", "state": true } ] } } >>>> + >>>> +EQMP >>>> + >>>> + { >>>> + .name = "migrate_set_parameters", >>>> + .args_type = "parameters:O", >>>> + .params = "capability:s,state:b", >>>> + .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, >>>> + }, >>>> + >>>> + >>>> + >>>> +SQMP >>>> query-balloon >>>> ------------- >>>> >>> >> >> >
On Tue, 24 Jul 2012 20:06:06 +0300 Orit Wasserman <owasserm@redhat.com> wrote: > On 07/24/2012 03:50 PM, Luiz Capitulino wrote: > > On Tue, 24 Jul 2012 09:25:11 +0300 > > Orit Wasserman <owasserm@redhat.com> wrote: > > > >> On 07/23/2012 09:23 PM, Luiz Capitulino wrote: > >>> On Fri, 13 Jul 2012 09:23:35 +0200 > >>> Juan Quintela <quintela@redhat.com> wrote: > >>> > >>>> From: Orit Wasserman <owasserm@redhat.com> > >>>> > >>>> Add migration capabilities that can be queried by the management. > >>>> The management can query the source QEMU and the destination QEMU in order to > >>>> verify both support some migration capability (currently only XBZRLE). > >>>> The management can enable a capability for the next migration by using > >>>> migrate_set_parameter command. > >>> > >>> Please, split this into one command per-patch. Otherwise it's difficult to > >>> review. > >>> > >> Sure. > >>> Have libvirt folks acked this approach btw? It looks fine to me, but we need > >>> their ack too. > >>> > >>> More comments below. > >>> > >>>> > >>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com> > >>>> Signed-off-by: Juan Quintela <quintela@redhat.com> > >>>> --- > >>>> hmp-commands.hx | 16 ++++++++++++ > >>>> hmp.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ > >>>> hmp.h | 2 ++ > >>>> migration.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- > >>>> migration.h | 2 ++ > >>>> monitor.c | 7 ++++++ > >>>> qapi-schema.json | 53 ++++++++++++++++++++++++++++++++++++++-- > >>>> qmp-commands.hx | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++--- > >>>> 8 files changed, 280 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/hmp-commands.hx b/hmp-commands.hx > >>>> index f5d9d91..9245bef 100644 > >>>> --- a/hmp-commands.hx > >>>> +++ b/hmp-commands.hx > >>>> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration. > >>>> ETEXI > >>>> > >>>> { > >>>> + .name = "migrate_set_parameter", > >>>> + .args_type = "capability:s,state:b", > >>>> + .params = "", > >>> > >>> Please, fill in params. > >> ok > >>> > >>>> + .help = "Enable/Disable the usage of a capability for migration", > >>>> + .mhandler.cmd = hmp_migrate_set_parameter, > >>>> + }, > >>>> + > >>>> +STEXI > >>>> +@item migrate_set_parameter @var{capability} @var{state} > >>>> +@findex migrate_set_parameter > >>>> +Enable/Disable the usage of a capability @var{capability} for migration. > >>>> +ETEXI > >>>> + > >>>> + { > >>>> .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", > >>>> @@ -1419,6 +1433,8 @@ show CPU statistics > >>>> show user network stack connection states > >>>> @item info migrate > >>>> show migration status > >>>> +@item info migration_capabilities > >>>> +show migration capabilities > >>>> @item info balloon > >>>> show balloon information > >>>> @item info qtree > >>>> diff --git a/hmp.c b/hmp.c > >>>> index 4c6d4ae..b0440e6 100644 > >>>> --- a/hmp.c > >>>> +++ b/hmp.c > >>>> @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon) > >>>> void hmp_info_migrate(Monitor *mon) > >>>> { > >>>> MigrationInfo *info; > >>>> + MigrationCapabilityInfoList *cap; > >>>> > >>>> info = qmp_query_migrate(NULL); > >>>> > >>>> + if (info->has_capabilities && info->capabilities) { > >>>> + monitor_printf(mon, "capabilities: "); > >>>> + for (cap = info->capabilities; cap; cap = cap->next) { > >>>> + monitor_printf(mon, "%s: %s ", > >>>> + MigrationCapability_lookup[cap->value->capability], > >>>> + cap->value->state ? "on" : "off"); > >>>> + } > >>>> + monitor_printf(mon, "\n"); > >>> > >>> Why is this is needed? Isn't info migration-capabilities good enough? > >>> Besides, info migrate should only contain information about current migration > >>> process... > >> The reason we introduced capabilities is that xbzrle needs for both source and destination QEMU > >> to be able to handle it. Even if both side support xbzrle the user may decide not to use it. > >> User that wants to use xbzrle needs to check that both sides have support for it (using info capabilities) , than > >> enable it in both sides (using migrate-set-parameter/s commands). This is a parameter for the current migration. > >> So the user needs to know if xbzrle was enabled or disabled for the current migration, this code displays it. > > > > But this is being returned even when there is no migration taking place. I'm > > answering this here in the 'info migrate' hunk, but my main concern is > > query-migrate. > I think migration configuration is part of migration information and should be available to the user in 'info migration'. Especially before he starts running it to see if he need to change the configuration. Also it is very helpful for the user during the migration and also after it's completes. I don't mind having this in 'info migrate' because we can change it later case we regret, so it's fine with me having that info there. But query-migrate is different, once we add anything there it's there forever, so we have to be careful. > There is no way today to query the downtime and migration speed at all . you can always set them again > but there is no way to know how it is configured. Good point, but I think it's better to concentrate on the needs of xbzrle for now. > I was actually thinking to add them to info migration (also for SETUP). But we're in SETUP state only once, what happens if the settings are changed after a migration completed? How is mngt going to be able to query it? My advice to not delay the inclusion of this series is to drop the information from SETUP state and rely on query-migration-capabilities in 'info migrate'. We can extend later if needed (by means of new commands or adding info to query-migrate). > > > > query-migrate shouldn't return this info in MIG_STATE_SETUP, and for > > MIG_STATE_COMPLETED it should only be returned if there was a migration. > I think is case of MIG_SETUP we should return the configuration and params for the next migration. > MIG_STATE_COMPLETED happens only if there was a successful migration other wise there will be another state. > > > >> some day when there will be better migration protocol, feature negotiations can be part of it ... > >>> > >>>> + } > >>>> if (info->has_status) { > >>>> monitor_printf(mon, "Migration status: %s\n", info->status); > >>>> } > >>>> @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon) > >>>> qapi_free_MigrationInfo(info); > >>>> } > >>>> > >>>> +void hmp_info_migration_capabilities(Monitor *mon) > >>>> +{ > >>>> + MigrationCapabilityInfoList *caps_list, *cap; > >>>> + > >>>> + caps_list = qmp_query_migration_capabilities(NULL); > >>>> + if (!caps_list) { > >>>> + monitor_printf(mon, "No migration capabilities found\n"); > >>>> + return; > >>>> + } > >>>> + > >>>> + for (cap = caps_list; cap; cap = cap->next) { > >>>> + monitor_printf(mon, "%s: %s ", > >>>> + MigrationCapability_lookup[cap->value->capability], > >>>> + cap->value->state ? "on" : "off"); > >>>> + } > >>>> + > >>>> + qapi_free_MigrationCapabilityInfoList(caps_list); > >>>> +} > >>>> + > >>>> void hmp_info_cpus(Monitor *mon) > >>>> { > >>>> CpuInfoList *cpu_list, *cpu; > >>>> @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict) > >>>> qmp_migrate_set_speed(value, NULL); > >>>> } > >>>> > >>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > >>>> +{ > >>>> + const char *cap = qdict_get_str(qdict, "capability"); > >>>> + bool state = qdict_get_bool(qdict, "state"); > >>>> + Error *err = NULL; > >>>> + MigrationCapabilityInfoList *params = NULL; > >>>> + int i; > >>>> + > >>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >>>> + if (strcmp(cap, MigrationCapability_lookup[i]) == 0) { > >>>> + if (!params) { > >>>> + params = g_malloc0(sizeof(*params)); > >>>> + } > >>>> + params->value = g_malloc0(sizeof(*params->value)); > >>>> + params->value->capability = i; > >>>> + params->value->state = state; > >>>> + params->next = NULL; > >>>> + qmp_migrate_set_parameters(params, &err); > >>>> + break; > >>>> + } > >>>> + } > >>>> + > >>>> + if (i == MIGRATION_CAPABILITY_MAX) { > >>>> + error_set(&err, QERR_INVALID_PARAMETER, cap); > >>>> + } > >>>> + > >>>> + qapi_free_MigrationCapabilityInfoList(params); > >>>> + > >>>> + if (err) { > >>>> + monitor_printf(mon, "migrate_set_parameter: %s\n", > >>>> + error_get_pretty(err)); > >>>> + error_free(err); > >>>> + } > >>>> +} > >>>> + > >>>> void hmp_set_password(Monitor *mon, const QDict *qdict) > >>>> { > >>>> const char *protocol = qdict_get_str(qdict, "protocol"); > >>>> diff --git a/hmp.h b/hmp.h > >>>> index 79d138d..09ba198 100644 > >>>> --- a/hmp.h > >>>> +++ b/hmp.h > >>>> @@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon); > >>>> void hmp_info_chardev(Monitor *mon); > >>>> void hmp_info_mice(Monitor *mon); > >>>> void hmp_info_migrate(Monitor *mon); > >>>> +void hmp_info_migration_capabilities(Monitor *mon); > >>>> void hmp_info_cpus(Monitor *mon); > >>>> void hmp_info_block(Monitor *mon); > >>>> void hmp_info_blockstats(Monitor *mon); > >>>> @@ -51,6 +52,7 @@ void hmp_snapshot_blkdev(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_speed(Monitor *mon, const QDict *qdict); > >>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict); > >>>> void hmp_set_password(Monitor *mon, const QDict *qdict); > >>>> void hmp_expire_password(Monitor *mon, const QDict *qdict); > >>>> void hmp_eject(Monitor *mon, const QDict *qdict); > >>>> diff --git a/migration.c b/migration.c > >>>> index 8db1b43..fd004d7 100644 > >>>> --- a/migration.c > >>>> +++ b/migration.c > >>>> @@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp) > >>>> { > >>>> MigrationInfo *info = g_malloc0(sizeof(*info)); > >>>> MigrationState *s = migrate_get_current(); > >>>> + int i; > >>>> > >>>> switch (s->state) { > >>>> case MIG_STATE_SETUP: > >>>> - /* no migration has happened ever */ > >>>> + /* no migration has ever happened; show enabled capabilities */ > >>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >>>> + if (!info->has_capabilities) { > >>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > >>>> + info->has_capabilities = true; > >>>> + } > >>>> + info->capabilities->value = > >>>> + g_malloc(sizeof(*info->capabilities->value)); > >>>> + info->capabilities->value->capability = i; > >>>> + info->capabilities->value->state = s->enabled_capabilities[i]; > >>>> + info->capabilities->next = NULL; > >>>> + } > >>>> break; > >>>> case MIG_STATE_ACTIVE: > >>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >>>> + if (!info->has_capabilities) { > >>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > >>>> + info->has_capabilities = true; > >>>> + } > >>>> + info->capabilities->value = > >>>> + g_malloc(sizeof(*info->capabilities->value)); > >>>> + info->capabilities->value->capability = i; > >>>> + info->capabilities->value->state = s->enabled_capabilities[i]; > >>>> + info->capabilities->next = NULL; > >>>> + } > >>>> + > >>>> info->has_status = true; > >>>> info->status = g_strdup("active"); > >>>> > >>>> @@ -143,6 +167,18 @@ MigrationInfo *qmp_query_migrate(Error **errp) > >>>> } > >>>> break; > >>>> case MIG_STATE_COMPLETED: > >>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >>>> + if (!info->has_capabilities) { > >>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > >>>> + info->has_capabilities = true; > >>>> + } > >>>> + info->capabilities->value = > >>>> + g_malloc(sizeof(*info->capabilities->value)); > >>>> + info->capabilities->value->capability = i; > >>>> + info->capabilities->value->state = s->enabled_capabilities[i]; > >>>> + info->capabilities->next = NULL; > >>>> + } > >>> > >>> Code triplication :) > >>> > >>> Why is this is needed? Isn't query-migration-capabilities good enough? > >>> Besides, query-migrate should only contain information about current migration > >>> process... > >>> > >> see above > >>>> + > >>>> info->has_status = true; > >>>> info->status = g_strdup("completed"); > >>>> > >>>> @@ -166,6 +202,33 @@ MigrationInfo *qmp_query_migrate(Error **errp) > >>>> return info; > >>>> } > >>>> > >>>> +MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp) > >>>> +{ > >>>> + MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list)); > >>>> + > >>>> + caps_list->value = g_malloc(sizeof(*caps_list->value)); > >>>> + caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE; > >>>> + caps_list->next = NULL; > >>> > >>> Shouldn't this get the capabilities array from migrate_get_current()? > >>> > >>> I mean, this makes query-migration-capabilities always return true for > >>> xbzrle, even if we set it to off. > >> Those are the general capabilities , if I want to use xbzrle ,does this QEMU support it ? > >> this is required because we need both the destination and the source to be able to handle it. > >> If one QEMU doesn't (older version of qemu, maybe it will be disbaled for certain architectures) we can't use the feature. > >> This is for management and the user to check what migration capabilities this QEMU supports. > > > > My point was that query-migration-capabilities will always return state=true > > for all existing capabilities. Don't we want users/mngt app to know which > > states have been turned off? > You need to separate the ability to use a capability and actually using it for migration. Also I don't want the user to be able to set some capability on, it depends on the code not user configuration. Not sure I can follow, a user can set xbzrle capability to off, no? > And for xbzrle we always return true for some version on QEMU , let say we decide we want to remove the code that implement it for some reason in some future version than it will return false. That's fine (I mean, I'm not sure we will ever do it, but shouldn't be a problem if we do). > There is a reason the command that enables a capability for a migration was called 'migrate-set-parameter' and not set-capability to distinguish between them one is per QEMU code base and one is per migration. > > Regards, > Orit > > > >> > >>> > >>>> + > >>>> + return caps_list; > >>>> +} > >>>> + > >>>> +void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params, > >>>> + Error **errp) > >>>> +{ > >>>> + MigrationState *s = migrate_get_current(); > >>>> + MigrationCapabilityInfoList *cap; > >>>> + > >>>> + if (s->state == MIG_STATE_ACTIVE) { > >>>> + error_set(errp, QERR_MIGRATION_ACTIVE); > >>>> + return; > >>>> + } > >>>> + > >>>> + for (cap = params; cap; cap = cap->next) { > >>>> + s->enabled_capabilities[cap->value->capability] = cap->value->state; > >>>> + } > >>>> +} > >>>> + > >>>> /* shared migration helpers */ > >>>> > >>>> static int migrate_fd_cleanup(MigrationState *s) > >>>> @@ -375,12 +438,17 @@ static MigrationState *migrate_init(const MigrationParams *params) > >>>> { > >>>> MigrationState *s = migrate_get_current(); > >>>> int64_t bandwidth_limit = s->bandwidth_limit; > >>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; > >>>> + > >>>> + memcpy(enabled_capabilities, s->enabled_capabilities, > >>>> + sizeof(enabled_capabilities)); > >>>> > >>>> memset(s, 0, sizeof(*s)); > >>>> s->bandwidth_limit = bandwidth_limit; > >>>> s->params = *params; > >>>> + memcpy(s->enabled_capabilities, enabled_capabilities, > >>>> + sizeof(enabled_capabilities)); > >>>> > >>>> - s->bandwidth_limit = bandwidth_limit; > >>>> s->state = MIG_STATE_SETUP; > >>>> s->total_time = qemu_get_clock_ms(rt_clock); > >>>> > >>>> diff --git a/migration.h b/migration.h > >>>> index 57572a6..713aae0 100644 > >>>> --- a/migration.h > >>>> +++ b/migration.h > >>>> @@ -19,6 +19,7 @@ > >>>> #include "notify.h" > >>>> #include "error.h" > >>>> #include "vmstate.h" > >>>> +#include "qapi-types.h" > >>>> > >>>> struct MigrationParams { > >>>> bool blk; > >>>> @@ -39,6 +40,7 @@ struct MigrationState > >>>> void *opaque; > >>>> MigrationParams params; > >>>> int64_t total_time; > >>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; > >>>> }; > >>>> > >>>> void process_incoming_migration(QEMUFile *f); > >>>> diff --git a/monitor.c b/monitor.c > >>>> index f6107ba..e2be6cd 100644 > >>>> --- a/monitor.c > >>>> +++ b/monitor.c > >>>> @@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = { > >>>> .mhandler.info = hmp_info_migrate, > >>>> }, > >>>> { > >>>> + .name = "migration_capabilities", > >>> > >>> migration-capabilities is better. > >> ok > >>> > >>>> + .args_type = "", > >>>> + .params = "", > >>>> + .help = "show migration capabilities", > >>>> + .mhandler.info = hmp_info_migration_capabilities, > >>>> + }, > >>>> + { > >>>> .name = "balloon", > >>>> .args_type = "", > >>>> .params = "", > >>>> diff --git a/qapi-schema.json b/qapi-schema.json > >>>> index 1ab5dbd..a8408fd 100644 > >>>> --- a/qapi-schema.json > >>>> +++ b/qapi-schema.json > >>>> @@ -288,11 +288,15 @@ > >>>> # status, only returned if status is 'active' and it is a block > >>>> # migration > >>>> # > >>>> -# Since: 0.14.0 > >>>> +# @capabilities: #optional a list describing all the migration capabilities > >>>> +# state > >>> > >>> I don't think this is needed, as I've said above. > >> see above > >>> > >>>> +# > >>>> +# Since: 0.14.0, 'capabilities' since 1.2 > >>>> ## > >>>> { 'type': 'MigrationInfo', > >>>> 'data': {'*status': 'str', '*ram': 'MigrationStats', > >>>> - '*disk': 'MigrationStats'} } > >>>> + '*disk': 'MigrationStats', > >>>> + '*capabilities': ['MigrationCapabilityInfo']} } > >>>> > >>>> ## > >>>> # @query-migrate > >>>> @@ -306,6 +310,51 @@ > >>>> { 'command': 'query-migrate', 'returns': 'MigrationInfo' } > >>>> > >>>> ## > >>>> +# @MigrationCapability > >>>> +# > >>>> +# Migration capabilities enumeration > >>>> +# > >>>> +# @xbzrle: current migration supports xbzrle > >>> > >>> You should explain what xbzrle is. > >> ok > >>> > >>>> +# > >>>> +# Since: 1.2 > >>>> +## > >>>> +{ 'enum': 'MigrationCapability', > >>>> + 'data': ['xbzrle'] } > >>>> + > >>>> +## > >>>> +# @MigrationCapabilityInfo > >>> > >>> MigrationCapabilityStatus? > >>> > >>>> +# > >>>> +# Migration capability information > >>>> +# > >>>> +# @capability: capability enum > >>> > >>> Please, document state too. > >> ok > >> > >> Orit > >>> > >>>> +# > >>>> +# Since: 1.2 > >>>> +## > >>>> +{ 'type': 'MigrationCapabilityInfo', > >>>> + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } } > >>>> + > >>>> +## > >>>> +# @query-migration-capabilities > >>>> +# > >>>> +# Returns information about current migration process capabilties. > >>>> +# > >>>> +# Returns: @MigrationCapabilityInfo list > >>>> +# > >>>> +# Since: 1.2 > >>>> +## > >>>> +{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] } > >>>> + > >>>> +## > >>>> +# @migrate_set_parameters > >>>> +# > >>>> +# Enable/Disable the following migration capabilities (like xbzrle) > >>>> +# > >>>> +# Since: 1.2 > >>>> +## > >>>> +{ 'command': 'migrate-set-parameters', > >>>> + 'data': { 'capabilities': ['MigrationCapabilityInfo'] } } > >>>> + > >>>> +## > >>>> # @MouseInfo: > >>>> # > >>>> # Information about a mouse device. > >>>> diff --git a/qmp-commands.hx b/qmp-commands.hx > >>>> index 2e1a38e..3ee6e00 100644 > >>>> --- a/qmp-commands.hx > >>>> +++ b/qmp-commands.hx > >>>> @@ -2075,18 +2075,31 @@ The main json-object contains the following: > >>>> - "transferred": amount transferred (json-int) > >>>> - "remaining": amount remaining (json-int) > >>>> - "total": total (json-int) > >>>> - > >>>> +- "capabilities": migration capabilities state > >>>> + - "xbzrle" : XBZRLE state (json-bool) > >>>> Examples: > >>>> > >>>> 1. Before the first migration > >>>> > >>>> -> { "execute": "query-migrate" } > >>>> -<- { "return": {} } > >>>> +<- { "return": { > >>>> + "capabilities" : [ { "capability" : "xbzrle", "state" : false } ] > >>>> + } > >>>> + } > >>>> > >>>> 2. Migration is done and has succeeded > >>>> > >>>> -> { "execute": "query-migrate" } > >>>> -<- { "return": { "status": "completed" } } > >>>> +<- { "return": { > >>>> + "status": "completed", > >>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > >>>> + "ram":{ > >>>> + "transferred":123, > >>>> + "remaining":123, > >>>> + "total":246 > >>>> + } > >>>> + } > >>>> + } > >>>> > >>>> 3. Migration is done and has failed > >>>> > >>>> @@ -2099,6 +2112,7 @@ Examples: > >>>> <- { > >>>> "return":{ > >>>> "status":"active", > >>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > >>>> "ram":{ > >>>> "transferred":123, > >>>> "remaining":123, > >>>> @@ -2113,6 +2127,7 @@ Examples: > >>>> <- { > >>>> "return":{ > >>>> "status":"active", > >>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > >>>> "ram":{ > >>>> "total":1057024, > >>>> "remaining":1053304, > >>>> @@ -2135,6 +2150,56 @@ EQMP > >>>> }, > >>>> > >>>> SQMP > >>>> +query-migration-capabilities > >>>> +------- > >>>> + > >>>> +Query migration capabilities > >>>> + > >>>> +- "xbzrle": xbzrle support > >>>> + > >>>> +Arguments: > >>>> + > >>>> +Example: > >>>> + > >>>> +-> { "execute": "query-migration-capabilities"} > >>>> +<- { "return": [ { "capability": "xbzrle", "state": true }, > >>>> + { "capability": "foobar", "state": false } ] } > >>>> + > >>>> +EQMP > >>>> + > >>>> + { > >>>> + .name = "query-migration-capabilities", > >>>> + .args_type = "", > >>>> + .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities, > >>>> + }, > >>>> + > >>>> +SQMP > >>>> +migrate_set_parameters > >>>> +------- > >>>> + > >>>> +Enable/Disable migration capabilities > >>>> + > >>>> +- "xbzrle": xbzrle support > >>>> + > >>>> +Arguments: > >>>> + > >>>> +Example: > >>>> + > >>>> +-> { "execute": "migrate_set_parameters" , "arguments": > >>>> + { "parameters": [ { "capability": "xbzrle", "state": true } ] } } > >>>> + > >>>> +EQMP > >>>> + > >>>> + { > >>>> + .name = "migrate_set_parameters", > >>>> + .args_type = "parameters:O", > >>>> + .params = "capability:s,state:b", > >>>> + .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, > >>>> + }, > >>>> + > >>>> + > >>>> + > >>>> +SQMP > >>>> query-balloon > >>>> ------------- > >>>> > >>> > >> > >> > > > >
On 07/24/2012 09:17 PM, Luiz Capitulino wrote: > On Tue, 24 Jul 2012 20:06:06 +0300 > Orit Wasserman <owasserm@redhat.com> wrote: > >> On 07/24/2012 03:50 PM, Luiz Capitulino wrote: >>> On Tue, 24 Jul 2012 09:25:11 +0300 >>> Orit Wasserman <owasserm@redhat.com> wrote: >>> >>>> On 07/23/2012 09:23 PM, Luiz Capitulino wrote: >>>>> On Fri, 13 Jul 2012 09:23:35 +0200 >>>>> Juan Quintela <quintela@redhat.com> wrote: >>>>> >>>>>> From: Orit Wasserman <owasserm@redhat.com> >>>>>> >>>>>> Add migration capabilities that can be queried by the management. >>>>>> The management can query the source QEMU and the destination QEMU in order to >>>>>> verify both support some migration capability (currently only XBZRLE). >>>>>> The management can enable a capability for the next migration by using >>>>>> migrate_set_parameter command. >>>>> >>>>> Please, split this into one command per-patch. Otherwise it's difficult to >>>>> review. >>>>> >>>> Sure. >>>>> Have libvirt folks acked this approach btw? It looks fine to me, but we need >>>>> their ack too. >>>>> >>>>> More comments below. >>>>> >>>>>> >>>>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com> >>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>>>>> --- >>>>>> hmp-commands.hx | 16 ++++++++++++ >>>>>> hmp.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> hmp.h | 2 ++ >>>>>> migration.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- >>>>>> migration.h | 2 ++ >>>>>> monitor.c | 7 ++++++ >>>>>> qapi-schema.json | 53 ++++++++++++++++++++++++++++++++++++++-- >>>>>> qmp-commands.hx | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++--- >>>>>> 8 files changed, 280 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/hmp-commands.hx b/hmp-commands.hx >>>>>> index f5d9d91..9245bef 100644 >>>>>> --- a/hmp-commands.hx >>>>>> +++ b/hmp-commands.hx >>>>>> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration. >>>>>> ETEXI >>>>>> >>>>>> { >>>>>> + .name = "migrate_set_parameter", >>>>>> + .args_type = "capability:s,state:b", >>>>>> + .params = "", >>>>> >>>>> Please, fill in params. >>>> ok >>>>> >>>>>> + .help = "Enable/Disable the usage of a capability for migration", >>>>>> + .mhandler.cmd = hmp_migrate_set_parameter, >>>>>> + }, >>>>>> + >>>>>> +STEXI >>>>>> +@item migrate_set_parameter @var{capability} @var{state} >>>>>> +@findex migrate_set_parameter >>>>>> +Enable/Disable the usage of a capability @var{capability} for migration. >>>>>> +ETEXI >>>>>> + >>>>>> + { >>>>>> .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", >>>>>> @@ -1419,6 +1433,8 @@ show CPU statistics >>>>>> show user network stack connection states >>>>>> @item info migrate >>>>>> show migration status >>>>>> +@item info migration_capabilities >>>>>> +show migration capabilities >>>>>> @item info balloon >>>>>> show balloon information >>>>>> @item info qtree >>>>>> diff --git a/hmp.c b/hmp.c >>>>>> index 4c6d4ae..b0440e6 100644 >>>>>> --- a/hmp.c >>>>>> +++ b/hmp.c >>>>>> @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon) >>>>>> void hmp_info_migrate(Monitor *mon) >>>>>> { >>>>>> MigrationInfo *info; >>>>>> + MigrationCapabilityInfoList *cap; >>>>>> >>>>>> info = qmp_query_migrate(NULL); >>>>>> >>>>>> + if (info->has_capabilities && info->capabilities) { >>>>>> + monitor_printf(mon, "capabilities: "); >>>>>> + for (cap = info->capabilities; cap; cap = cap->next) { >>>>>> + monitor_printf(mon, "%s: %s ", >>>>>> + MigrationCapability_lookup[cap->value->capability], >>>>>> + cap->value->state ? "on" : "off"); >>>>>> + } >>>>>> + monitor_printf(mon, "\n"); >>>>> >>>>> Why is this is needed? Isn't info migration-capabilities good enough? >>>>> Besides, info migrate should only contain information about current migration >>>>> process... >>>> The reason we introduced capabilities is that xbzrle needs for both source and destination QEMU >>>> to be able to handle it. Even if both side support xbzrle the user may decide not to use it. >>>> User that wants to use xbzrle needs to check that both sides have support for it (using info capabilities) , than >>>> enable it in both sides (using migrate-set-parameter/s commands). This is a parameter for the current migration. >>>> So the user needs to know if xbzrle was enabled or disabled for the current migration, this code displays it. >>> >>> But this is being returned even when there is no migration taking place. I'm >>> answering this here in the 'info migrate' hunk, but my main concern is >>> query-migrate. >> I think migration configuration is part of migration information and should be available to the user in 'info migration'. Especially before he starts running it to see if he need to change the configuration. Also it is very helpful for the user during the migration and also after it's completes. > > I don't mind having this in 'info migrate' because we can change it later case > we regret, so it's fine with me having that info there. > > But query-migrate is different, once we add anything there it's there forever, > so we have to be careful. ok I will remove them from query-migrate. > >> There is no way today to query the downtime and migration speed at all . you can always set them again >> but there is no way to know how it is configured. > > Good point, but I think it's better to concentrate on the needs of xbzrle > for now. > >> I was actually thinking to add them to info migration (also for SETUP). > > But we're in SETUP state only once, what happens if the settings are changed > after a migration completed? How is mngt going to be able to query it? After migration completes you can't change those setting , you should get an error. > > My advice to not delay the inclusion of this series is to drop the > information from SETUP state and rely on query-migration-capabilities in > 'info migrate'. We can extend later if needed (by means of new commands or > adding info to query-migrate). It needs to be another command, I can add query-migration-parameters. You can look at this as 64bit support - you have 64bit support in the processor (this is what query-capabilities) but the OS can decide not to work in 64bit mode (do nothing) or using 64 bit (migrate-set-parameter). Orit > >>> >>> query-migrate shouldn't return this info in MIG_STATE_SETUP, and for >>> MIG_STATE_COMPLETED it should only be returned if there was a migration. >> I think is case of MIG_SETUP we should return the configuration and params for the next migration. >> MIG_STATE_COMPLETED happens only if there was a successful migration other wise there will be another state. >>> >>>> some day when there will be better migration protocol, feature negotiations can be part of it ... >>>>> >>>>>> + } >>>>>> if (info->has_status) { >>>>>> monitor_printf(mon, "Migration status: %s\n", info->status); >>>>>> } >>>>>> @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon) >>>>>> qapi_free_MigrationInfo(info); >>>>>> } >>>>>> >>>>>> +void hmp_info_migration_capabilities(Monitor *mon) >>>>>> +{ >>>>>> + MigrationCapabilityInfoList *caps_list, *cap; >>>>>> + >>>>>> + caps_list = qmp_query_migration_capabilities(NULL); >>>>>> + if (!caps_list) { >>>>>> + monitor_printf(mon, "No migration capabilities found\n"); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + for (cap = caps_list; cap; cap = cap->next) { >>>>>> + monitor_printf(mon, "%s: %s ", >>>>>> + MigrationCapability_lookup[cap->value->capability], >>>>>> + cap->value->state ? "on" : "off"); >>>>>> + } >>>>>> + >>>>>> + qapi_free_MigrationCapabilityInfoList(caps_list); >>>>>> +} >>>>>> + >>>>>> void hmp_info_cpus(Monitor *mon) >>>>>> { >>>>>> CpuInfoList *cpu_list, *cpu; >>>>>> @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict) >>>>>> qmp_migrate_set_speed(value, NULL); >>>>>> } >>>>>> >>>>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) >>>>>> +{ >>>>>> + const char *cap = qdict_get_str(qdict, "capability"); >>>>>> + bool state = qdict_get_bool(qdict, "state"); >>>>>> + Error *err = NULL; >>>>>> + MigrationCapabilityInfoList *params = NULL; >>>>>> + int i; >>>>>> + >>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>>>> + if (strcmp(cap, MigrationCapability_lookup[i]) == 0) { >>>>>> + if (!params) { >>>>>> + params = g_malloc0(sizeof(*params)); >>>>>> + } >>>>>> + params->value = g_malloc0(sizeof(*params->value)); >>>>>> + params->value->capability = i; >>>>>> + params->value->state = state; >>>>>> + params->next = NULL; >>>>>> + qmp_migrate_set_parameters(params, &err); >>>>>> + break; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + if (i == MIGRATION_CAPABILITY_MAX) { >>>>>> + error_set(&err, QERR_INVALID_PARAMETER, cap); >>>>>> + } >>>>>> + >>>>>> + qapi_free_MigrationCapabilityInfoList(params); >>>>>> + >>>>>> + if (err) { >>>>>> + monitor_printf(mon, "migrate_set_parameter: %s\n", >>>>>> + error_get_pretty(err)); >>>>>> + error_free(err); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> void hmp_set_password(Monitor *mon, const QDict *qdict) >>>>>> { >>>>>> const char *protocol = qdict_get_str(qdict, "protocol"); >>>>>> diff --git a/hmp.h b/hmp.h >>>>>> index 79d138d..09ba198 100644 >>>>>> --- a/hmp.h >>>>>> +++ b/hmp.h >>>>>> @@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon); >>>>>> void hmp_info_chardev(Monitor *mon); >>>>>> void hmp_info_mice(Monitor *mon); >>>>>> void hmp_info_migrate(Monitor *mon); >>>>>> +void hmp_info_migration_capabilities(Monitor *mon); >>>>>> void hmp_info_cpus(Monitor *mon); >>>>>> void hmp_info_block(Monitor *mon); >>>>>> void hmp_info_blockstats(Monitor *mon); >>>>>> @@ -51,6 +52,7 @@ void hmp_snapshot_blkdev(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_speed(Monitor *mon, const QDict *qdict); >>>>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict); >>>>>> void hmp_set_password(Monitor *mon, const QDict *qdict); >>>>>> void hmp_expire_password(Monitor *mon, const QDict *qdict); >>>>>> void hmp_eject(Monitor *mon, const QDict *qdict); >>>>>> diff --git a/migration.c b/migration.c >>>>>> index 8db1b43..fd004d7 100644 >>>>>> --- a/migration.c >>>>>> +++ b/migration.c >>>>>> @@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp) >>>>>> { >>>>>> MigrationInfo *info = g_malloc0(sizeof(*info)); >>>>>> MigrationState *s = migrate_get_current(); >>>>>> + int i; >>>>>> >>>>>> switch (s->state) { >>>>>> case MIG_STATE_SETUP: >>>>>> - /* no migration has happened ever */ >>>>>> + /* no migration has ever happened; show enabled capabilities */ >>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>>>> + if (!info->has_capabilities) { >>>>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >>>>>> + info->has_capabilities = true; >>>>>> + } >>>>>> + info->capabilities->value = >>>>>> + g_malloc(sizeof(*info->capabilities->value)); >>>>>> + info->capabilities->value->capability = i; >>>>>> + info->capabilities->value->state = s->enabled_capabilities[i]; >>>>>> + info->capabilities->next = NULL; >>>>>> + } >>>>>> break; >>>>>> case MIG_STATE_ACTIVE: >>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>>>> + if (!info->has_capabilities) { >>>>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >>>>>> + info->has_capabilities = true; >>>>>> + } >>>>>> + info->capabilities->value = >>>>>> + g_malloc(sizeof(*info->capabilities->value)); >>>>>> + info->capabilities->value->capability = i; >>>>>> + info->capabilities->value->state = s->enabled_capabilities[i]; >>>>>> + info->capabilities->next = NULL; >>>>>> + } >>>>>> + >>>>>> info->has_status = true; >>>>>> info->status = g_strdup("active"); >>>>>> >>>>>> @@ -143,6 +167,18 @@ MigrationInfo *qmp_query_migrate(Error **errp) >>>>>> } >>>>>> break; >>>>>> case MIG_STATE_COMPLETED: >>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>>>> + if (!info->has_capabilities) { >>>>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >>>>>> + info->has_capabilities = true; >>>>>> + } >>>>>> + info->capabilities->value = >>>>>> + g_malloc(sizeof(*info->capabilities->value)); >>>>>> + info->capabilities->value->capability = i; >>>>>> + info->capabilities->value->state = s->enabled_capabilities[i]; >>>>>> + info->capabilities->next = NULL; >>>>>> + } >>>>> >>>>> Code triplication :) >>>>> >>>>> Why is this is needed? Isn't query-migration-capabilities good enough? >>>>> Besides, query-migrate should only contain information about current migration >>>>> process... >>>>> >>>> see above >>>>>> + >>>>>> info->has_status = true; >>>>>> info->status = g_strdup("completed"); >>>>>> >>>>>> @@ -166,6 +202,33 @@ MigrationInfo *qmp_query_migrate(Error **errp) >>>>>> return info; >>>>>> } >>>>>> >>>>>> +MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp) >>>>>> +{ >>>>>> + MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list)); >>>>>> + >>>>>> + caps_list->value = g_malloc(sizeof(*caps_list->value)); >>>>>> + caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE; >>>>>> + caps_list->next = NULL; >>>>> >>>>> Shouldn't this get the capabilities array from migrate_get_current()? >>>>> >>>>> I mean, this makes query-migration-capabilities always return true for >>>>> xbzrle, even if we set it to off. >>>> Those are the general capabilities , if I want to use xbzrle ,does this QEMU support it ? >>>> this is required because we need both the destination and the source to be able to handle it. >>>> If one QEMU doesn't (older version of qemu, maybe it will be disbaled for certain architectures) we can't use the feature. >>>> This is for management and the user to check what migration capabilities this QEMU supports. >>> >>> My point was that query-migration-capabilities will always return state=true >>> for all existing capabilities. Don't we want users/mngt app to know which >>> states have been turned off? >> You need to separate the ability to use a capability and actually using it for migration. Also I don't want the user to be able to set some capability on, it depends on the code not user configuration. > > Not sure I can follow, a user can set xbzrle capability to off, no? > >> And for xbzrle we always return true for some version on QEMU , let say we decide we want to remove the code that implement it for some reason in some future version than it will return false. > > That's fine (I mean, I'm not sure we will ever do it, but shouldn't be a problem > if we do). > >> There is a reason the command that enables a capability for a migration was called 'migrate-set-parameter' and not set-capability to distinguish between them one is per QEMU code base and one is per migration. >> >> Regards, >> Orit >>> >>>> >>>>> >>>>>> + >>>>>> + return caps_list; >>>>>> +} >>>>>> + >>>>>> +void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params, >>>>>> + Error **errp) >>>>>> +{ >>>>>> + MigrationState *s = migrate_get_current(); >>>>>> + MigrationCapabilityInfoList *cap; >>>>>> + >>>>>> + if (s->state == MIG_STATE_ACTIVE) { >>>>>> + error_set(errp, QERR_MIGRATION_ACTIVE); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + for (cap = params; cap; cap = cap->next) { >>>>>> + s->enabled_capabilities[cap->value->capability] = cap->value->state; >>>>>> + } >>>>>> +} >>>>>> + >>>>>> /* shared migration helpers */ >>>>>> >>>>>> static int migrate_fd_cleanup(MigrationState *s) >>>>>> @@ -375,12 +438,17 @@ static MigrationState *migrate_init(const MigrationParams *params) >>>>>> { >>>>>> MigrationState *s = migrate_get_current(); >>>>>> int64_t bandwidth_limit = s->bandwidth_limit; >>>>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; >>>>>> + >>>>>> + memcpy(enabled_capabilities, s->enabled_capabilities, >>>>>> + sizeof(enabled_capabilities)); >>>>>> >>>>>> memset(s, 0, sizeof(*s)); >>>>>> s->bandwidth_limit = bandwidth_limit; >>>>>> s->params = *params; >>>>>> + memcpy(s->enabled_capabilities, enabled_capabilities, >>>>>> + sizeof(enabled_capabilities)); >>>>>> >>>>>> - s->bandwidth_limit = bandwidth_limit; >>>>>> s->state = MIG_STATE_SETUP; >>>>>> s->total_time = qemu_get_clock_ms(rt_clock); >>>>>> >>>>>> diff --git a/migration.h b/migration.h >>>>>> index 57572a6..713aae0 100644 >>>>>> --- a/migration.h >>>>>> +++ b/migration.h >>>>>> @@ -19,6 +19,7 @@ >>>>>> #include "notify.h" >>>>>> #include "error.h" >>>>>> #include "vmstate.h" >>>>>> +#include "qapi-types.h" >>>>>> >>>>>> struct MigrationParams { >>>>>> bool blk; >>>>>> @@ -39,6 +40,7 @@ struct MigrationState >>>>>> void *opaque; >>>>>> MigrationParams params; >>>>>> int64_t total_time; >>>>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; >>>>>> }; >>>>>> >>>>>> void process_incoming_migration(QEMUFile *f); >>>>>> diff --git a/monitor.c b/monitor.c >>>>>> index f6107ba..e2be6cd 100644 >>>>>> --- a/monitor.c >>>>>> +++ b/monitor.c >>>>>> @@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = { >>>>>> .mhandler.info = hmp_info_migrate, >>>>>> }, >>>>>> { >>>>>> + .name = "migration_capabilities", >>>>> >>>>> migration-capabilities is better. >>>> ok >>>>> >>>>>> + .args_type = "", >>>>>> + .params = "", >>>>>> + .help = "show migration capabilities", >>>>>> + .mhandler.info = hmp_info_migration_capabilities, >>>>>> + }, >>>>>> + { >>>>>> .name = "balloon", >>>>>> .args_type = "", >>>>>> .params = "", >>>>>> diff --git a/qapi-schema.json b/qapi-schema.json >>>>>> index 1ab5dbd..a8408fd 100644 >>>>>> --- a/qapi-schema.json >>>>>> +++ b/qapi-schema.json >>>>>> @@ -288,11 +288,15 @@ >>>>>> # status, only returned if status is 'active' and it is a block >>>>>> # migration >>>>>> # >>>>>> -# Since: 0.14.0 >>>>>> +# @capabilities: #optional a list describing all the migration capabilities >>>>>> +# state >>>>> >>>>> I don't think this is needed, as I've said above. >>>> see above >>>>> >>>>>> +# >>>>>> +# Since: 0.14.0, 'capabilities' since 1.2 >>>>>> ## >>>>>> { 'type': 'MigrationInfo', >>>>>> 'data': {'*status': 'str', '*ram': 'MigrationStats', >>>>>> - '*disk': 'MigrationStats'} } >>>>>> + '*disk': 'MigrationStats', >>>>>> + '*capabilities': ['MigrationCapabilityInfo']} } >>>>>> >>>>>> ## >>>>>> # @query-migrate >>>>>> @@ -306,6 +310,51 @@ >>>>>> { 'command': 'query-migrate', 'returns': 'MigrationInfo' } >>>>>> >>>>>> ## >>>>>> +# @MigrationCapability >>>>>> +# >>>>>> +# Migration capabilities enumeration >>>>>> +# >>>>>> +# @xbzrle: current migration supports xbzrle >>>>> >>>>> You should explain what xbzrle is. >>>> ok >>>>> >>>>>> +# >>>>>> +# Since: 1.2 >>>>>> +## >>>>>> +{ 'enum': 'MigrationCapability', >>>>>> + 'data': ['xbzrle'] } >>>>>> + >>>>>> +## >>>>>> +# @MigrationCapabilityInfo >>>>> >>>>> MigrationCapabilityStatus? >>>>> >>>>>> +# >>>>>> +# Migration capability information >>>>>> +# >>>>>> +# @capability: capability enum >>>>> >>>>> Please, document state too. >>>> ok >>>> >>>> Orit >>>>> >>>>>> +# >>>>>> +# Since: 1.2 >>>>>> +## >>>>>> +{ 'type': 'MigrationCapabilityInfo', >>>>>> + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } } >>>>>> + >>>>>> +## >>>>>> +# @query-migration-capabilities >>>>>> +# >>>>>> +# Returns information about current migration process capabilties. >>>>>> +# >>>>>> +# Returns: @MigrationCapabilityInfo list >>>>>> +# >>>>>> +# Since: 1.2 >>>>>> +## >>>>>> +{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] } >>>>>> + >>>>>> +## >>>>>> +# @migrate_set_parameters >>>>>> +# >>>>>> +# Enable/Disable the following migration capabilities (like xbzrle) >>>>>> +# >>>>>> +# Since: 1.2 >>>>>> +## >>>>>> +{ 'command': 'migrate-set-parameters', >>>>>> + 'data': { 'capabilities': ['MigrationCapabilityInfo'] } } >>>>>> + >>>>>> +## >>>>>> # @MouseInfo: >>>>>> # >>>>>> # Information about a mouse device. >>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx >>>>>> index 2e1a38e..3ee6e00 100644 >>>>>> --- a/qmp-commands.hx >>>>>> +++ b/qmp-commands.hx >>>>>> @@ -2075,18 +2075,31 @@ The main json-object contains the following: >>>>>> - "transferred": amount transferred (json-int) >>>>>> - "remaining": amount remaining (json-int) >>>>>> - "total": total (json-int) >>>>>> - >>>>>> +- "capabilities": migration capabilities state >>>>>> + - "xbzrle" : XBZRLE state (json-bool) >>>>>> Examples: >>>>>> >>>>>> 1. Before the first migration >>>>>> >>>>>> -> { "execute": "query-migrate" } >>>>>> -<- { "return": {} } >>>>>> +<- { "return": { >>>>>> + "capabilities" : [ { "capability" : "xbzrle", "state" : false } ] >>>>>> + } >>>>>> + } >>>>>> >>>>>> 2. Migration is done and has succeeded >>>>>> >>>>>> -> { "execute": "query-migrate" } >>>>>> -<- { "return": { "status": "completed" } } >>>>>> +<- { "return": { >>>>>> + "status": "completed", >>>>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >>>>>> + "ram":{ >>>>>> + "transferred":123, >>>>>> + "remaining":123, >>>>>> + "total":246 >>>>>> + } >>>>>> + } >>>>>> + } >>>>>> >>>>>> 3. Migration is done and has failed >>>>>> >>>>>> @@ -2099,6 +2112,7 @@ Examples: >>>>>> <- { >>>>>> "return":{ >>>>>> "status":"active", >>>>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >>>>>> "ram":{ >>>>>> "transferred":123, >>>>>> "remaining":123, >>>>>> @@ -2113,6 +2127,7 @@ Examples: >>>>>> <- { >>>>>> "return":{ >>>>>> "status":"active", >>>>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >>>>>> "ram":{ >>>>>> "total":1057024, >>>>>> "remaining":1053304, >>>>>> @@ -2135,6 +2150,56 @@ EQMP >>>>>> }, >>>>>> >>>>>> SQMP >>>>>> +query-migration-capabilities >>>>>> +------- >>>>>> + >>>>>> +Query migration capabilities >>>>>> + >>>>>> +- "xbzrle": xbzrle support >>>>>> + >>>>>> +Arguments: >>>>>> + >>>>>> +Example: >>>>>> + >>>>>> +-> { "execute": "query-migration-capabilities"} >>>>>> +<- { "return": [ { "capability": "xbzrle", "state": true }, >>>>>> + { "capability": "foobar", "state": false } ] } >>>>>> + >>>>>> +EQMP >>>>>> + >>>>>> + { >>>>>> + .name = "query-migration-capabilities", >>>>>> + .args_type = "", >>>>>> + .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities, >>>>>> + }, >>>>>> + >>>>>> +SQMP >>>>>> +migrate_set_parameters >>>>>> +------- >>>>>> + >>>>>> +Enable/Disable migration capabilities >>>>>> + >>>>>> +- "xbzrle": xbzrle support >>>>>> + >>>>>> +Arguments: >>>>>> + >>>>>> +Example: >>>>>> + >>>>>> +-> { "execute": "migrate_set_parameters" , "arguments": >>>>>> + { "parameters": [ { "capability": "xbzrle", "state": true } ] } } >>>>>> + >>>>>> +EQMP >>>>>> + >>>>>> + { >>>>>> + .name = "migrate_set_parameters", >>>>>> + .args_type = "parameters:O", >>>>>> + .params = "capability:s,state:b", >>>>>> + .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, >>>>>> + }, >>>>>> + >>>>>> + >>>>>> + >>>>>> +SQMP >>>>>> query-balloon >>>>>> ------------- >>>>>> >>>>> >>>> >>>> >>> >> >> >
On Wed, 25 Jul 2012 16:05:10 +0300 Orit Wasserman <owasserm@redhat.com> wrote: > On 07/24/2012 09:17 PM, Luiz Capitulino wrote: > > On Tue, 24 Jul 2012 20:06:06 +0300 > > Orit Wasserman <owasserm@redhat.com> wrote: > > > >> On 07/24/2012 03:50 PM, Luiz Capitulino wrote: > >>> On Tue, 24 Jul 2012 09:25:11 +0300 > >>> Orit Wasserman <owasserm@redhat.com> wrote: > >>> > >>>> On 07/23/2012 09:23 PM, Luiz Capitulino wrote: > >>>>> On Fri, 13 Jul 2012 09:23:35 +0200 > >>>>> Juan Quintela <quintela@redhat.com> wrote: > >>>>> > >>>>>> From: Orit Wasserman <owasserm@redhat.com> > >>>>>> > >>>>>> Add migration capabilities that can be queried by the management. > >>>>>> The management can query the source QEMU and the destination QEMU in order to > >>>>>> verify both support some migration capability (currently only XBZRLE). > >>>>>> The management can enable a capability for the next migration by using > >>>>>> migrate_set_parameter command. > >>>>> > >>>>> Please, split this into one command per-patch. Otherwise it's difficult to > >>>>> review. > >>>>> > >>>> Sure. > >>>>> Have libvirt folks acked this approach btw? It looks fine to me, but we need > >>>>> their ack too. > >>>>> > >>>>> More comments below. > >>>>> > >>>>>> > >>>>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com> > >>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com> > >>>>>> --- > >>>>>> hmp-commands.hx | 16 ++++++++++++ > >>>>>> hmp.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ > >>>>>> hmp.h | 2 ++ > >>>>>> migration.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- > >>>>>> migration.h | 2 ++ > >>>>>> monitor.c | 7 ++++++ > >>>>>> qapi-schema.json | 53 ++++++++++++++++++++++++++++++++++++++-- > >>>>>> qmp-commands.hx | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++--- > >>>>>> 8 files changed, 280 insertions(+), 7 deletions(-) > >>>>>> > >>>>>> diff --git a/hmp-commands.hx b/hmp-commands.hx > >>>>>> index f5d9d91..9245bef 100644 > >>>>>> --- a/hmp-commands.hx > >>>>>> +++ b/hmp-commands.hx > >>>>>> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration. > >>>>>> ETEXI > >>>>>> > >>>>>> { > >>>>>> + .name = "migrate_set_parameter", > >>>>>> + .args_type = "capability:s,state:b", > >>>>>> + .params = "", > >>>>> > >>>>> Please, fill in params. > >>>> ok > >>>>> > >>>>>> + .help = "Enable/Disable the usage of a capability for migration", > >>>>>> + .mhandler.cmd = hmp_migrate_set_parameter, > >>>>>> + }, > >>>>>> + > >>>>>> +STEXI > >>>>>> +@item migrate_set_parameter @var{capability} @var{state} > >>>>>> +@findex migrate_set_parameter > >>>>>> +Enable/Disable the usage of a capability @var{capability} for migration. > >>>>>> +ETEXI > >>>>>> + > >>>>>> + { > >>>>>> .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", > >>>>>> @@ -1419,6 +1433,8 @@ show CPU statistics > >>>>>> show user network stack connection states > >>>>>> @item info migrate > >>>>>> show migration status > >>>>>> +@item info migration_capabilities > >>>>>> +show migration capabilities > >>>>>> @item info balloon > >>>>>> show balloon information > >>>>>> @item info qtree > >>>>>> diff --git a/hmp.c b/hmp.c > >>>>>> index 4c6d4ae..b0440e6 100644 > >>>>>> --- a/hmp.c > >>>>>> +++ b/hmp.c > >>>>>> @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon) > >>>>>> void hmp_info_migrate(Monitor *mon) > >>>>>> { > >>>>>> MigrationInfo *info; > >>>>>> + MigrationCapabilityInfoList *cap; > >>>>>> > >>>>>> info = qmp_query_migrate(NULL); > >>>>>> > >>>>>> + if (info->has_capabilities && info->capabilities) { > >>>>>> + monitor_printf(mon, "capabilities: "); > >>>>>> + for (cap = info->capabilities; cap; cap = cap->next) { > >>>>>> + monitor_printf(mon, "%s: %s ", > >>>>>> + MigrationCapability_lookup[cap->value->capability], > >>>>>> + cap->value->state ? "on" : "off"); > >>>>>> + } > >>>>>> + monitor_printf(mon, "\n"); > >>>>> > >>>>> Why is this is needed? Isn't info migration-capabilities good enough? > >>>>> Besides, info migrate should only contain information about current migration > >>>>> process... > >>>> The reason we introduced capabilities is that xbzrle needs for both source and destination QEMU > >>>> to be able to handle it. Even if both side support xbzrle the user may decide not to use it. > >>>> User that wants to use xbzrle needs to check that both sides have support for it (using info capabilities) , than > >>>> enable it in both sides (using migrate-set-parameter/s commands). This is a parameter for the current migration. > >>>> So the user needs to know if xbzrle was enabled or disabled for the current migration, this code displays it. > >>> > >>> But this is being returned even when there is no migration taking place. I'm > >>> answering this here in the 'info migrate' hunk, but my main concern is > >>> query-migrate. > >> I think migration configuration is part of migration information and should be available to the user in 'info migration'. Especially before he starts running it to see if he need to change the configuration. Also it is very helpful for the user during the migration and also after it's completes. > > > > I don't mind having this in 'info migrate' because we can change it later case > > we regret, so it's fine with me having that info there. > > > > But query-migrate is different, once we add anything there it's there forever, > > so we have to be careful. > ok I will remove them from query-migrate. Note that I'm asking to remove it from MIG_SETUP, the others states are fine. > > > >> There is no way today to query the downtime and migration speed at all . you can always set them again > >> but there is no way to know how it is configured. > > > > Good point, but I think it's better to concentrate on the needs of xbzrle > > for now. > > > >> I was actually thinking to add them to info migration (also for SETUP). > > > > But we're in SETUP state only once, what happens if the settings are changed > > after a migration completed? How is mngt going to be able to query it? > After migration completes you can't change those setting , you should get an error. Really, why? I thought it would be fine to change them at any time (except during migration, of course). > > > > > My advice to not delay the inclusion of this series is to drop the > > information from SETUP state and rely on query-migration-capabilities in > > 'info migrate'. We can extend later if needed (by means of new commands or > > adding info to query-migrate). > It needs to be another command, I can add query-migration-parameters. > You can look at this as 64bit support - you have 64bit support in the processor (this is what query-capabilities) but the OS can decide not to work in 64bit mode (do nothing) or using 64 bit (migrate-set-parameter). > > Orit > > > >>> > >>> query-migrate shouldn't return this info in MIG_STATE_SETUP, and for > >>> MIG_STATE_COMPLETED it should only be returned if there was a migration. > >> I think is case of MIG_SETUP we should return the configuration and params for the next migration. > >> MIG_STATE_COMPLETED happens only if there was a successful migration other wise there will be another state. > >>> > >>>> some day when there will be better migration protocol, feature negotiations can be part of it ... > >>>>> > >>>>>> + } > >>>>>> if (info->has_status) { > >>>>>> monitor_printf(mon, "Migration status: %s\n", info->status); > >>>>>> } > >>>>>> @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon) > >>>>>> qapi_free_MigrationInfo(info); > >>>>>> } > >>>>>> > >>>>>> +void hmp_info_migration_capabilities(Monitor *mon) > >>>>>> +{ > >>>>>> + MigrationCapabilityInfoList *caps_list, *cap; > >>>>>> + > >>>>>> + caps_list = qmp_query_migration_capabilities(NULL); > >>>>>> + if (!caps_list) { > >>>>>> + monitor_printf(mon, "No migration capabilities found\n"); > >>>>>> + return; > >>>>>> + } > >>>>>> + > >>>>>> + for (cap = caps_list; cap; cap = cap->next) { > >>>>>> + monitor_printf(mon, "%s: %s ", > >>>>>> + MigrationCapability_lookup[cap->value->capability], > >>>>>> + cap->value->state ? "on" : "off"); > >>>>>> + } > >>>>>> + > >>>>>> + qapi_free_MigrationCapabilityInfoList(caps_list); > >>>>>> +} > >>>>>> + > >>>>>> void hmp_info_cpus(Monitor *mon) > >>>>>> { > >>>>>> CpuInfoList *cpu_list, *cpu; > >>>>>> @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict) > >>>>>> qmp_migrate_set_speed(value, NULL); > >>>>>> } > >>>>>> > >>>>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > >>>>>> +{ > >>>>>> + const char *cap = qdict_get_str(qdict, "capability"); > >>>>>> + bool state = qdict_get_bool(qdict, "state"); > >>>>>> + Error *err = NULL; > >>>>>> + MigrationCapabilityInfoList *params = NULL; > >>>>>> + int i; > >>>>>> + > >>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >>>>>> + if (strcmp(cap, MigrationCapability_lookup[i]) == 0) { > >>>>>> + if (!params) { > >>>>>> + params = g_malloc0(sizeof(*params)); > >>>>>> + } > >>>>>> + params->value = g_malloc0(sizeof(*params->value)); > >>>>>> + params->value->capability = i; > >>>>>> + params->value->state = state; > >>>>>> + params->next = NULL; > >>>>>> + qmp_migrate_set_parameters(params, &err); > >>>>>> + break; > >>>>>> + } > >>>>>> + } > >>>>>> + > >>>>>> + if (i == MIGRATION_CAPABILITY_MAX) { > >>>>>> + error_set(&err, QERR_INVALID_PARAMETER, cap); > >>>>>> + } > >>>>>> + > >>>>>> + qapi_free_MigrationCapabilityInfoList(params); > >>>>>> + > >>>>>> + if (err) { > >>>>>> + monitor_printf(mon, "migrate_set_parameter: %s\n", > >>>>>> + error_get_pretty(err)); > >>>>>> + error_free(err); > >>>>>> + } > >>>>>> +} > >>>>>> + > >>>>>> void hmp_set_password(Monitor *mon, const QDict *qdict) > >>>>>> { > >>>>>> const char *protocol = qdict_get_str(qdict, "protocol"); > >>>>>> diff --git a/hmp.h b/hmp.h > >>>>>> index 79d138d..09ba198 100644 > >>>>>> --- a/hmp.h > >>>>>> +++ b/hmp.h > >>>>>> @@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon); > >>>>>> void hmp_info_chardev(Monitor *mon); > >>>>>> void hmp_info_mice(Monitor *mon); > >>>>>> void hmp_info_migrate(Monitor *mon); > >>>>>> +void hmp_info_migration_capabilities(Monitor *mon); > >>>>>> void hmp_info_cpus(Monitor *mon); > >>>>>> void hmp_info_block(Monitor *mon); > >>>>>> void hmp_info_blockstats(Monitor *mon); > >>>>>> @@ -51,6 +52,7 @@ void hmp_snapshot_blkdev(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_speed(Monitor *mon, const QDict *qdict); > >>>>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict); > >>>>>> void hmp_set_password(Monitor *mon, const QDict *qdict); > >>>>>> void hmp_expire_password(Monitor *mon, const QDict *qdict); > >>>>>> void hmp_eject(Monitor *mon, const QDict *qdict); > >>>>>> diff --git a/migration.c b/migration.c > >>>>>> index 8db1b43..fd004d7 100644 > >>>>>> --- a/migration.c > >>>>>> +++ b/migration.c > >>>>>> @@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp) > >>>>>> { > >>>>>> MigrationInfo *info = g_malloc0(sizeof(*info)); > >>>>>> MigrationState *s = migrate_get_current(); > >>>>>> + int i; > >>>>>> > >>>>>> switch (s->state) { > >>>>>> case MIG_STATE_SETUP: > >>>>>> - /* no migration has happened ever */ > >>>>>> + /* no migration has ever happened; show enabled capabilities */ > >>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >>>>>> + if (!info->has_capabilities) { > >>>>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > >>>>>> + info->has_capabilities = true; > >>>>>> + } > >>>>>> + info->capabilities->value = > >>>>>> + g_malloc(sizeof(*info->capabilities->value)); > >>>>>> + info->capabilities->value->capability = i; > >>>>>> + info->capabilities->value->state = s->enabled_capabilities[i]; > >>>>>> + info->capabilities->next = NULL; > >>>>>> + } > >>>>>> break; > >>>>>> case MIG_STATE_ACTIVE: > >>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >>>>>> + if (!info->has_capabilities) { > >>>>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > >>>>>> + info->has_capabilities = true; > >>>>>> + } > >>>>>> + info->capabilities->value = > >>>>>> + g_malloc(sizeof(*info->capabilities->value)); > >>>>>> + info->capabilities->value->capability = i; > >>>>>> + info->capabilities->value->state = s->enabled_capabilities[i]; > >>>>>> + info->capabilities->next = NULL; > >>>>>> + } > >>>>>> + > >>>>>> info->has_status = true; > >>>>>> info->status = g_strdup("active"); > >>>>>> > >>>>>> @@ -143,6 +167,18 @@ MigrationInfo *qmp_query_migrate(Error **errp) > >>>>>> } > >>>>>> break; > >>>>>> case MIG_STATE_COMPLETED: > >>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { > >>>>>> + if (!info->has_capabilities) { > >>>>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); > >>>>>> + info->has_capabilities = true; > >>>>>> + } > >>>>>> + info->capabilities->value = > >>>>>> + g_malloc(sizeof(*info->capabilities->value)); > >>>>>> + info->capabilities->value->capability = i; > >>>>>> + info->capabilities->value->state = s->enabled_capabilities[i]; > >>>>>> + info->capabilities->next = NULL; > >>>>>> + } > >>>>> > >>>>> Code triplication :) > >>>>> > >>>>> Why is this is needed? Isn't query-migration-capabilities good enough? > >>>>> Besides, query-migrate should only contain information about current migration > >>>>> process... > >>>>> > >>>> see above > >>>>>> + > >>>>>> info->has_status = true; > >>>>>> info->status = g_strdup("completed"); > >>>>>> > >>>>>> @@ -166,6 +202,33 @@ MigrationInfo *qmp_query_migrate(Error **errp) > >>>>>> return info; > >>>>>> } > >>>>>> > >>>>>> +MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp) > >>>>>> +{ > >>>>>> + MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list)); > >>>>>> + > >>>>>> + caps_list->value = g_malloc(sizeof(*caps_list->value)); > >>>>>> + caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE; > >>>>>> + caps_list->next = NULL; > >>>>> > >>>>> Shouldn't this get the capabilities array from migrate_get_current()? > >>>>> > >>>>> I mean, this makes query-migration-capabilities always return true for > >>>>> xbzrle, even if we set it to off. > >>>> Those are the general capabilities , if I want to use xbzrle ,does this QEMU support it ? > >>>> this is required because we need both the destination and the source to be able to handle it. > >>>> If one QEMU doesn't (older version of qemu, maybe it will be disbaled for certain architectures) we can't use the feature. > >>>> This is for management and the user to check what migration capabilities this QEMU supports. > >>> > >>> My point was that query-migration-capabilities will always return state=true > >>> for all existing capabilities. Don't we want users/mngt app to know which > >>> states have been turned off? > >> You need to separate the ability to use a capability and actually using it for migration. Also I don't want the user to be able to set some capability on, it depends on the code not user configuration. > > > > Not sure I can follow, a user can set xbzrle capability to off, no? > > > >> And for xbzrle we always return true for some version on QEMU , let say we decide we want to remove the code that implement it for some reason in some future version than it will return false. > > > > That's fine (I mean, I'm not sure we will ever do it, but shouldn't be a problem > > if we do). > > > >> There is a reason the command that enables a capability for a migration was called 'migrate-set-parameter' and not set-capability to distinguish between them one is per QEMU code base and one is per migration. > >> > >> Regards, > >> Orit > >>> > >>>> > >>>>> > >>>>>> + > >>>>>> + return caps_list; > >>>>>> +} > >>>>>> + > >>>>>> +void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params, > >>>>>> + Error **errp) > >>>>>> +{ > >>>>>> + MigrationState *s = migrate_get_current(); > >>>>>> + MigrationCapabilityInfoList *cap; > >>>>>> + > >>>>>> + if (s->state == MIG_STATE_ACTIVE) { > >>>>>> + error_set(errp, QERR_MIGRATION_ACTIVE); > >>>>>> + return; > >>>>>> + } > >>>>>> + > >>>>>> + for (cap = params; cap; cap = cap->next) { > >>>>>> + s->enabled_capabilities[cap->value->capability] = cap->value->state; > >>>>>> + } > >>>>>> +} > >>>>>> + > >>>>>> /* shared migration helpers */ > >>>>>> > >>>>>> static int migrate_fd_cleanup(MigrationState *s) > >>>>>> @@ -375,12 +438,17 @@ static MigrationState *migrate_init(const MigrationParams *params) > >>>>>> { > >>>>>> MigrationState *s = migrate_get_current(); > >>>>>> int64_t bandwidth_limit = s->bandwidth_limit; > >>>>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; > >>>>>> + > >>>>>> + memcpy(enabled_capabilities, s->enabled_capabilities, > >>>>>> + sizeof(enabled_capabilities)); > >>>>>> > >>>>>> memset(s, 0, sizeof(*s)); > >>>>>> s->bandwidth_limit = bandwidth_limit; > >>>>>> s->params = *params; > >>>>>> + memcpy(s->enabled_capabilities, enabled_capabilities, > >>>>>> + sizeof(enabled_capabilities)); > >>>>>> > >>>>>> - s->bandwidth_limit = bandwidth_limit; > >>>>>> s->state = MIG_STATE_SETUP; > >>>>>> s->total_time = qemu_get_clock_ms(rt_clock); > >>>>>> > >>>>>> diff --git a/migration.h b/migration.h > >>>>>> index 57572a6..713aae0 100644 > >>>>>> --- a/migration.h > >>>>>> +++ b/migration.h > >>>>>> @@ -19,6 +19,7 @@ > >>>>>> #include "notify.h" > >>>>>> #include "error.h" > >>>>>> #include "vmstate.h" > >>>>>> +#include "qapi-types.h" > >>>>>> > >>>>>> struct MigrationParams { > >>>>>> bool blk; > >>>>>> @@ -39,6 +40,7 @@ struct MigrationState > >>>>>> void *opaque; > >>>>>> MigrationParams params; > >>>>>> int64_t total_time; > >>>>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; > >>>>>> }; > >>>>>> > >>>>>> void process_incoming_migration(QEMUFile *f); > >>>>>> diff --git a/monitor.c b/monitor.c > >>>>>> index f6107ba..e2be6cd 100644 > >>>>>> --- a/monitor.c > >>>>>> +++ b/monitor.c > >>>>>> @@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = { > >>>>>> .mhandler.info = hmp_info_migrate, > >>>>>> }, > >>>>>> { > >>>>>> + .name = "migration_capabilities", > >>>>> > >>>>> migration-capabilities is better. > >>>> ok > >>>>> > >>>>>> + .args_type = "", > >>>>>> + .params = "", > >>>>>> + .help = "show migration capabilities", > >>>>>> + .mhandler.info = hmp_info_migration_capabilities, > >>>>>> + }, > >>>>>> + { > >>>>>> .name = "balloon", > >>>>>> .args_type = "", > >>>>>> .params = "", > >>>>>> diff --git a/qapi-schema.json b/qapi-schema.json > >>>>>> index 1ab5dbd..a8408fd 100644 > >>>>>> --- a/qapi-schema.json > >>>>>> +++ b/qapi-schema.json > >>>>>> @@ -288,11 +288,15 @@ > >>>>>> # status, only returned if status is 'active' and it is a block > >>>>>> # migration > >>>>>> # > >>>>>> -# Since: 0.14.0 > >>>>>> +# @capabilities: #optional a list describing all the migration capabilities > >>>>>> +# state > >>>>> > >>>>> I don't think this is needed, as I've said above. > >>>> see above > >>>>> > >>>>>> +# > >>>>>> +# Since: 0.14.0, 'capabilities' since 1.2 > >>>>>> ## > >>>>>> { 'type': 'MigrationInfo', > >>>>>> 'data': {'*status': 'str', '*ram': 'MigrationStats', > >>>>>> - '*disk': 'MigrationStats'} } > >>>>>> + '*disk': 'MigrationStats', > >>>>>> + '*capabilities': ['MigrationCapabilityInfo']} } > >>>>>> > >>>>>> ## > >>>>>> # @query-migrate > >>>>>> @@ -306,6 +310,51 @@ > >>>>>> { 'command': 'query-migrate', 'returns': 'MigrationInfo' } > >>>>>> > >>>>>> ## > >>>>>> +# @MigrationCapability > >>>>>> +# > >>>>>> +# Migration capabilities enumeration > >>>>>> +# > >>>>>> +# @xbzrle: current migration supports xbzrle > >>>>> > >>>>> You should explain what xbzrle is. > >>>> ok > >>>>> > >>>>>> +# > >>>>>> +# Since: 1.2 > >>>>>> +## > >>>>>> +{ 'enum': 'MigrationCapability', > >>>>>> + 'data': ['xbzrle'] } > >>>>>> + > >>>>>> +## > >>>>>> +# @MigrationCapabilityInfo > >>>>> > >>>>> MigrationCapabilityStatus? > >>>>> > >>>>>> +# > >>>>>> +# Migration capability information > >>>>>> +# > >>>>>> +# @capability: capability enum > >>>>> > >>>>> Please, document state too. > >>>> ok > >>>> > >>>> Orit > >>>>> > >>>>>> +# > >>>>>> +# Since: 1.2 > >>>>>> +## > >>>>>> +{ 'type': 'MigrationCapabilityInfo', > >>>>>> + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } } > >>>>>> + > >>>>>> +## > >>>>>> +# @query-migration-capabilities > >>>>>> +# > >>>>>> +# Returns information about current migration process capabilties. > >>>>>> +# > >>>>>> +# Returns: @MigrationCapabilityInfo list > >>>>>> +# > >>>>>> +# Since: 1.2 > >>>>>> +## > >>>>>> +{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] } > >>>>>> + > >>>>>> +## > >>>>>> +# @migrate_set_parameters > >>>>>> +# > >>>>>> +# Enable/Disable the following migration capabilities (like xbzrle) > >>>>>> +# > >>>>>> +# Since: 1.2 > >>>>>> +## > >>>>>> +{ 'command': 'migrate-set-parameters', > >>>>>> + 'data': { 'capabilities': ['MigrationCapabilityInfo'] } } > >>>>>> + > >>>>>> +## > >>>>>> # @MouseInfo: > >>>>>> # > >>>>>> # Information about a mouse device. > >>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx > >>>>>> index 2e1a38e..3ee6e00 100644 > >>>>>> --- a/qmp-commands.hx > >>>>>> +++ b/qmp-commands.hx > >>>>>> @@ -2075,18 +2075,31 @@ The main json-object contains the following: > >>>>>> - "transferred": amount transferred (json-int) > >>>>>> - "remaining": amount remaining (json-int) > >>>>>> - "total": total (json-int) > >>>>>> - > >>>>>> +- "capabilities": migration capabilities state > >>>>>> + - "xbzrle" : XBZRLE state (json-bool) > >>>>>> Examples: > >>>>>> > >>>>>> 1. Before the first migration > >>>>>> > >>>>>> -> { "execute": "query-migrate" } > >>>>>> -<- { "return": {} } > >>>>>> +<- { "return": { > >>>>>> + "capabilities" : [ { "capability" : "xbzrle", "state" : false } ] > >>>>>> + } > >>>>>> + } > >>>>>> > >>>>>> 2. Migration is done and has succeeded > >>>>>> > >>>>>> -> { "execute": "query-migrate" } > >>>>>> -<- { "return": { "status": "completed" } } > >>>>>> +<- { "return": { > >>>>>> + "status": "completed", > >>>>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > >>>>>> + "ram":{ > >>>>>> + "transferred":123, > >>>>>> + "remaining":123, > >>>>>> + "total":246 > >>>>>> + } > >>>>>> + } > >>>>>> + } > >>>>>> > >>>>>> 3. Migration is done and has failed > >>>>>> > >>>>>> @@ -2099,6 +2112,7 @@ Examples: > >>>>>> <- { > >>>>>> "return":{ > >>>>>> "status":"active", > >>>>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > >>>>>> "ram":{ > >>>>>> "transferred":123, > >>>>>> "remaining":123, > >>>>>> @@ -2113,6 +2127,7 @@ Examples: > >>>>>> <- { > >>>>>> "return":{ > >>>>>> "status":"active", > >>>>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], > >>>>>> "ram":{ > >>>>>> "total":1057024, > >>>>>> "remaining":1053304, > >>>>>> @@ -2135,6 +2150,56 @@ EQMP > >>>>>> }, > >>>>>> > >>>>>> SQMP > >>>>>> +query-migration-capabilities > >>>>>> +------- > >>>>>> + > >>>>>> +Query migration capabilities > >>>>>> + > >>>>>> +- "xbzrle": xbzrle support > >>>>>> + > >>>>>> +Arguments: > >>>>>> + > >>>>>> +Example: > >>>>>> + > >>>>>> +-> { "execute": "query-migration-capabilities"} > >>>>>> +<- { "return": [ { "capability": "xbzrle", "state": true }, > >>>>>> + { "capability": "foobar", "state": false } ] } > >>>>>> + > >>>>>> +EQMP > >>>>>> + > >>>>>> + { > >>>>>> + .name = "query-migration-capabilities", > >>>>>> + .args_type = "", > >>>>>> + .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities, > >>>>>> + }, > >>>>>> + > >>>>>> +SQMP > >>>>>> +migrate_set_parameters > >>>>>> +------- > >>>>>> + > >>>>>> +Enable/Disable migration capabilities > >>>>>> + > >>>>>> +- "xbzrle": xbzrle support > >>>>>> + > >>>>>> +Arguments: > >>>>>> + > >>>>>> +Example: > >>>>>> + > >>>>>> +-> { "execute": "migrate_set_parameters" , "arguments": > >>>>>> + { "parameters": [ { "capability": "xbzrle", "state": true } ] } } > >>>>>> + > >>>>>> +EQMP > >>>>>> + > >>>>>> + { > >>>>>> + .name = "migrate_set_parameters", > >>>>>> + .args_type = "parameters:O", > >>>>>> + .params = "capability:s,state:b", > >>>>>> + .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, > >>>>>> + }, > >>>>>> + > >>>>>> + > >>>>>> + > >>>>>> +SQMP > >>>>>> query-balloon > >>>>>> ------------- > >>>>>> > >>>>> > >>>> > >>>> > >>> > >> > >> > > >
On 07/25/2012 04:11 PM, Luiz Capitulino wrote: > On Wed, 25 Jul 2012 16:05:10 +0300 > Orit Wasserman <owasserm@redhat.com> wrote: > >> On 07/24/2012 09:17 PM, Luiz Capitulino wrote: >>> On Tue, 24 Jul 2012 20:06:06 +0300 >>> Orit Wasserman <owasserm@redhat.com> wrote: >>> >>>> On 07/24/2012 03:50 PM, Luiz Capitulino wrote: >>>>> On Tue, 24 Jul 2012 09:25:11 +0300 >>>>> Orit Wasserman <owasserm@redhat.com> wrote: >>>>> >>>>>> On 07/23/2012 09:23 PM, Luiz Capitulino wrote: >>>>>>> On Fri, 13 Jul 2012 09:23:35 +0200 >>>>>>> Juan Quintela <quintela@redhat.com> wrote: >>>>>>> >>>>>>>> From: Orit Wasserman <owasserm@redhat.com> >>>>>>>> >>>>>>>> Add migration capabilities that can be queried by the management. >>>>>>>> The management can query the source QEMU and the destination QEMU in order to >>>>>>>> verify both support some migration capability (currently only XBZRLE). >>>>>>>> The management can enable a capability for the next migration by using >>>>>>>> migrate_set_parameter command. >>>>>>> >>>>>>> Please, split this into one command per-patch. Otherwise it's difficult to >>>>>>> review. >>>>>>> >>>>>> Sure. >>>>>>> Have libvirt folks acked this approach btw? It looks fine to me, but we need >>>>>>> their ack too. >>>>>>> >>>>>>> More comments below. >>>>>>> >>>>>>>> >>>>>>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com> >>>>>>>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>>>>>>> --- >>>>>>>> hmp-commands.hx | 16 ++++++++++++ >>>>>>>> hmp.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> hmp.h | 2 ++ >>>>>>>> migration.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- >>>>>>>> migration.h | 2 ++ >>>>>>>> monitor.c | 7 ++++++ >>>>>>>> qapi-schema.json | 53 ++++++++++++++++++++++++++++++++++++++-- >>>>>>>> qmp-commands.hx | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++--- >>>>>>>> 8 files changed, 280 insertions(+), 7 deletions(-) >>>>>>>> >>>>>>>> diff --git a/hmp-commands.hx b/hmp-commands.hx >>>>>>>> index f5d9d91..9245bef 100644 >>>>>>>> --- a/hmp-commands.hx >>>>>>>> +++ b/hmp-commands.hx >>>>>>>> @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration. >>>>>>>> ETEXI >>>>>>>> >>>>>>>> { >>>>>>>> + .name = "migrate_set_parameter", >>>>>>>> + .args_type = "capability:s,state:b", >>>>>>>> + .params = "", >>>>>>> >>>>>>> Please, fill in params. >>>>>> ok >>>>>>> >>>>>>>> + .help = "Enable/Disable the usage of a capability for migration", >>>>>>>> + .mhandler.cmd = hmp_migrate_set_parameter, >>>>>>>> + }, >>>>>>>> + >>>>>>>> +STEXI >>>>>>>> +@item migrate_set_parameter @var{capability} @var{state} >>>>>>>> +@findex migrate_set_parameter >>>>>>>> +Enable/Disable the usage of a capability @var{capability} for migration. >>>>>>>> +ETEXI >>>>>>>> + >>>>>>>> + { >>>>>>>> .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", >>>>>>>> @@ -1419,6 +1433,8 @@ show CPU statistics >>>>>>>> show user network stack connection states >>>>>>>> @item info migrate >>>>>>>> show migration status >>>>>>>> +@item info migration_capabilities >>>>>>>> +show migration capabilities >>>>>>>> @item info balloon >>>>>>>> show balloon information >>>>>>>> @item info qtree >>>>>>>> diff --git a/hmp.c b/hmp.c >>>>>>>> index 4c6d4ae..b0440e6 100644 >>>>>>>> --- a/hmp.c >>>>>>>> +++ b/hmp.c >>>>>>>> @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon) >>>>>>>> void hmp_info_migrate(Monitor *mon) >>>>>>>> { >>>>>>>> MigrationInfo *info; >>>>>>>> + MigrationCapabilityInfoList *cap; >>>>>>>> >>>>>>>> info = qmp_query_migrate(NULL); >>>>>>>> >>>>>>>> + if (info->has_capabilities && info->capabilities) { >>>>>>>> + monitor_printf(mon, "capabilities: "); >>>>>>>> + for (cap = info->capabilities; cap; cap = cap->next) { >>>>>>>> + monitor_printf(mon, "%s: %s ", >>>>>>>> + MigrationCapability_lookup[cap->value->capability], >>>>>>>> + cap->value->state ? "on" : "off"); >>>>>>>> + } >>>>>>>> + monitor_printf(mon, "\n"); >>>>>>> >>>>>>> Why is this is needed? Isn't info migration-capabilities good enough? >>>>>>> Besides, info migrate should only contain information about current migration >>>>>>> process... >>>>>> The reason we introduced capabilities is that xbzrle needs for both source and destination QEMU >>>>>> to be able to handle it. Even if both side support xbzrle the user may decide not to use it. >>>>>> User that wants to use xbzrle needs to check that both sides have support for it (using info capabilities) , than >>>>>> enable it in both sides (using migrate-set-parameter/s commands). This is a parameter for the current migration. >>>>>> So the user needs to know if xbzrle was enabled or disabled for the current migration, this code displays it. >>>>> >>>>> But this is being returned even when there is no migration taking place. I'm >>>>> answering this here in the 'info migrate' hunk, but my main concern is >>>>> query-migrate. >>>> I think migration configuration is part of migration information and should be available to the user in 'info migration'. Especially before he starts running it to see if he need to change the configuration. Also it is very helpful for the user during the migration and also after it's completes. >>> >>> I don't mind having this in 'info migrate' because we can change it later case >>> we regret, so it's fine with me having that info there. >>> >>> But query-migrate is different, once we add anything there it's there forever, >>> so we have to be careful. >> ok I will remove them from query-migrate. > > Note that I'm asking to remove it from MIG_SETUP, the others states are fine. ok > >>> >>>> There is no way today to query the downtime and migration speed at all . you can always set them again >>>> but there is no way to know how it is configured. >>> >>> Good point, but I think it's better to concentrate on the needs of xbzrle >>> for now. >>> >>>> I was actually thinking to add them to info migration (also for SETUP). >>> >>> But we're in SETUP state only once, what happens if the settings are changed >>> after a migration completed? How is mngt going to be able to query it? >> After migration completes you can't change those setting , you should get an error. > > Really, why? I thought it would be fine to change them at any time > (except during migration, of course). you are right . Orit > >> >>> >>> My advice to not delay the inclusion of this series is to drop the >>> information from SETUP state and rely on query-migration-capabilities in >>> 'info migrate'. We can extend later if needed (by means of new commands or >>> adding info to query-migrate). >> It needs to be another command, I can add query-migration-parameters. >> You can look at this as 64bit support - you have 64bit support in the processor (this is what query-capabilities) but the OS can decide not to work in 64bit mode (do nothing) or using 64 bit (migrate-set-parameter). >> >> Orit >>> >>>>> >>>>> query-migrate shouldn't return this info in MIG_STATE_SETUP, and for >>>>> MIG_STATE_COMPLETED it should only be returned if there was a migration. >>>> I think is case of MIG_SETUP we should return the configuration and params for the next migration. >>>> MIG_STATE_COMPLETED happens only if there was a successful migration other wise there will be another state. >>>>> >>>>>> some day when there will be better migration protocol, feature negotiations can be part of it ... >>>>>>> >>>>>>>> + } >>>>>>>> if (info->has_status) { >>>>>>>> monitor_printf(mon, "Migration status: %s\n", info->status); >>>>>>>> } >>>>>>>> @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon) >>>>>>>> qapi_free_MigrationInfo(info); >>>>>>>> } >>>>>>>> >>>>>>>> +void hmp_info_migration_capabilities(Monitor *mon) >>>>>>>> +{ >>>>>>>> + MigrationCapabilityInfoList *caps_list, *cap; >>>>>>>> + >>>>>>>> + caps_list = qmp_query_migration_capabilities(NULL); >>>>>>>> + if (!caps_list) { >>>>>>>> + monitor_printf(mon, "No migration capabilities found\n"); >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + >>>>>>>> + for (cap = caps_list; cap; cap = cap->next) { >>>>>>>> + monitor_printf(mon, "%s: %s ", >>>>>>>> + MigrationCapability_lookup[cap->value->capability], >>>>>>>> + cap->value->state ? "on" : "off"); >>>>>>>> + } >>>>>>>> + >>>>>>>> + qapi_free_MigrationCapabilityInfoList(caps_list); >>>>>>>> +} >>>>>>>> + >>>>>>>> void hmp_info_cpus(Monitor *mon) >>>>>>>> { >>>>>>>> CpuInfoList *cpu_list, *cpu; >>>>>>>> @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict) >>>>>>>> qmp_migrate_set_speed(value, NULL); >>>>>>>> } >>>>>>>> >>>>>>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) >>>>>>>> +{ >>>>>>>> + const char *cap = qdict_get_str(qdict, "capability"); >>>>>>>> + bool state = qdict_get_bool(qdict, "state"); >>>>>>>> + Error *err = NULL; >>>>>>>> + MigrationCapabilityInfoList *params = NULL; >>>>>>>> + int i; >>>>>>>> + >>>>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>>>>>> + if (strcmp(cap, MigrationCapability_lookup[i]) == 0) { >>>>>>>> + if (!params) { >>>>>>>> + params = g_malloc0(sizeof(*params)); >>>>>>>> + } >>>>>>>> + params->value = g_malloc0(sizeof(*params->value)); >>>>>>>> + params->value->capability = i; >>>>>>>> + params->value->state = state; >>>>>>>> + params->next = NULL; >>>>>>>> + qmp_migrate_set_parameters(params, &err); >>>>>>>> + break; >>>>>>>> + } >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (i == MIGRATION_CAPABILITY_MAX) { >>>>>>>> + error_set(&err, QERR_INVALID_PARAMETER, cap); >>>>>>>> + } >>>>>>>> + >>>>>>>> + qapi_free_MigrationCapabilityInfoList(params); >>>>>>>> + >>>>>>>> + if (err) { >>>>>>>> + monitor_printf(mon, "migrate_set_parameter: %s\n", >>>>>>>> + error_get_pretty(err)); >>>>>>>> + error_free(err); >>>>>>>> + } >>>>>>>> +} >>>>>>>> + >>>>>>>> void hmp_set_password(Monitor *mon, const QDict *qdict) >>>>>>>> { >>>>>>>> const char *protocol = qdict_get_str(qdict, "protocol"); >>>>>>>> diff --git a/hmp.h b/hmp.h >>>>>>>> index 79d138d..09ba198 100644 >>>>>>>> --- a/hmp.h >>>>>>>> +++ b/hmp.h >>>>>>>> @@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon); >>>>>>>> void hmp_info_chardev(Monitor *mon); >>>>>>>> void hmp_info_mice(Monitor *mon); >>>>>>>> void hmp_info_migrate(Monitor *mon); >>>>>>>> +void hmp_info_migration_capabilities(Monitor *mon); >>>>>>>> void hmp_info_cpus(Monitor *mon); >>>>>>>> void hmp_info_block(Monitor *mon); >>>>>>>> void hmp_info_blockstats(Monitor *mon); >>>>>>>> @@ -51,6 +52,7 @@ void hmp_snapshot_blkdev(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_speed(Monitor *mon, const QDict *qdict); >>>>>>>> +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict); >>>>>>>> void hmp_set_password(Monitor *mon, const QDict *qdict); >>>>>>>> void hmp_expire_password(Monitor *mon, const QDict *qdict); >>>>>>>> void hmp_eject(Monitor *mon, const QDict *qdict); >>>>>>>> diff --git a/migration.c b/migration.c >>>>>>>> index 8db1b43..fd004d7 100644 >>>>>>>> --- a/migration.c >>>>>>>> +++ b/migration.c >>>>>>>> @@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp) >>>>>>>> { >>>>>>>> MigrationInfo *info = g_malloc0(sizeof(*info)); >>>>>>>> MigrationState *s = migrate_get_current(); >>>>>>>> + int i; >>>>>>>> >>>>>>>> switch (s->state) { >>>>>>>> case MIG_STATE_SETUP: >>>>>>>> - /* no migration has happened ever */ >>>>>>>> + /* no migration has ever happened; show enabled capabilities */ >>>>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>>>>>> + if (!info->has_capabilities) { >>>>>>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >>>>>>>> + info->has_capabilities = true; >>>>>>>> + } >>>>>>>> + info->capabilities->value = >>>>>>>> + g_malloc(sizeof(*info->capabilities->value)); >>>>>>>> + info->capabilities->value->capability = i; >>>>>>>> + info->capabilities->value->state = s->enabled_capabilities[i]; >>>>>>>> + info->capabilities->next = NULL; >>>>>>>> + } >>>>>>>> break; >>>>>>>> case MIG_STATE_ACTIVE: >>>>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>>>>>> + if (!info->has_capabilities) { >>>>>>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >>>>>>>> + info->has_capabilities = true; >>>>>>>> + } >>>>>>>> + info->capabilities->value = >>>>>>>> + g_malloc(sizeof(*info->capabilities->value)); >>>>>>>> + info->capabilities->value->capability = i; >>>>>>>> + info->capabilities->value->state = s->enabled_capabilities[i]; >>>>>>>> + info->capabilities->next = NULL; >>>>>>>> + } >>>>>>>> + >>>>>>>> info->has_status = true; >>>>>>>> info->status = g_strdup("active"); >>>>>>>> >>>>>>>> @@ -143,6 +167,18 @@ MigrationInfo *qmp_query_migrate(Error **errp) >>>>>>>> } >>>>>>>> break; >>>>>>>> case MIG_STATE_COMPLETED: >>>>>>>> + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >>>>>>>> + if (!info->has_capabilities) { >>>>>>>> + info->capabilities = g_malloc0(sizeof(*info->capabilities)); >>>>>>>> + info->has_capabilities = true; >>>>>>>> + } >>>>>>>> + info->capabilities->value = >>>>>>>> + g_malloc(sizeof(*info->capabilities->value)); >>>>>>>> + info->capabilities->value->capability = i; >>>>>>>> + info->capabilities->value->state = s->enabled_capabilities[i]; >>>>>>>> + info->capabilities->next = NULL; >>>>>>>> + } >>>>>>> >>>>>>> Code triplication :) >>>>>>> >>>>>>> Why is this is needed? Isn't query-migration-capabilities good enough? >>>>>>> Besides, query-migrate should only contain information about current migration >>>>>>> process... >>>>>>> >>>>>> see above >>>>>>>> + >>>>>>>> info->has_status = true; >>>>>>>> info->status = g_strdup("completed"); >>>>>>>> >>>>>>>> @@ -166,6 +202,33 @@ MigrationInfo *qmp_query_migrate(Error **errp) >>>>>>>> return info; >>>>>>>> } >>>>>>>> >>>>>>>> +MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp) >>>>>>>> +{ >>>>>>>> + MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list)); >>>>>>>> + >>>>>>>> + caps_list->value = g_malloc(sizeof(*caps_list->value)); >>>>>>>> + caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE; >>>>>>>> + caps_list->next = NULL; >>>>>>> >>>>>>> Shouldn't this get the capabilities array from migrate_get_current()? >>>>>>> >>>>>>> I mean, this makes query-migration-capabilities always return true for >>>>>>> xbzrle, even if we set it to off. >>>>>> Those are the general capabilities , if I want to use xbzrle ,does this QEMU support it ? >>>>>> this is required because we need both the destination and the source to be able to handle it. >>>>>> If one QEMU doesn't (older version of qemu, maybe it will be disbaled for certain architectures) we can't use the feature. >>>>>> This is for management and the user to check what migration capabilities this QEMU supports. >>>>> >>>>> My point was that query-migration-capabilities will always return state=true >>>>> for all existing capabilities. Don't we want users/mngt app to know which >>>>> states have been turned off? >>>> You need to separate the ability to use a capability and actually using it for migration. Also I don't want the user to be able to set some capability on, it depends on the code not user configuration. >>> >>> Not sure I can follow, a user can set xbzrle capability to off, no? >>> >>>> And for xbzrle we always return true for some version on QEMU , let say we decide we want to remove the code that implement it for some reason in some future version than it will return false. >>> >>> That's fine (I mean, I'm not sure we will ever do it, but shouldn't be a problem >>> if we do). >>> >>>> There is a reason the command that enables a capability for a migration was called 'migrate-set-parameter' and not set-capability to distinguish between them one is per QEMU code base and one is per migration. >>>> >>>> Regards, >>>> Orit >>>>> >>>>>> >>>>>>> >>>>>>>> + >>>>>>>> + return caps_list; >>>>>>>> +} >>>>>>>> + >>>>>>>> +void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params, >>>>>>>> + Error **errp) >>>>>>>> +{ >>>>>>>> + MigrationState *s = migrate_get_current(); >>>>>>>> + MigrationCapabilityInfoList *cap; >>>>>>>> + >>>>>>>> + if (s->state == MIG_STATE_ACTIVE) { >>>>>>>> + error_set(errp, QERR_MIGRATION_ACTIVE); >>>>>>>> + return; >>>>>>>> + } >>>>>>>> + >>>>>>>> + for (cap = params; cap; cap = cap->next) { >>>>>>>> + s->enabled_capabilities[cap->value->capability] = cap->value->state; >>>>>>>> + } >>>>>>>> +} >>>>>>>> + >>>>>>>> /* shared migration helpers */ >>>>>>>> >>>>>>>> static int migrate_fd_cleanup(MigrationState *s) >>>>>>>> @@ -375,12 +438,17 @@ static MigrationState *migrate_init(const MigrationParams *params) >>>>>>>> { >>>>>>>> MigrationState *s = migrate_get_current(); >>>>>>>> int64_t bandwidth_limit = s->bandwidth_limit; >>>>>>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; >>>>>>>> + >>>>>>>> + memcpy(enabled_capabilities, s->enabled_capabilities, >>>>>>>> + sizeof(enabled_capabilities)); >>>>>>>> >>>>>>>> memset(s, 0, sizeof(*s)); >>>>>>>> s->bandwidth_limit = bandwidth_limit; >>>>>>>> s->params = *params; >>>>>>>> + memcpy(s->enabled_capabilities, enabled_capabilities, >>>>>>>> + sizeof(enabled_capabilities)); >>>>>>>> >>>>>>>> - s->bandwidth_limit = bandwidth_limit; >>>>>>>> s->state = MIG_STATE_SETUP; >>>>>>>> s->total_time = qemu_get_clock_ms(rt_clock); >>>>>>>> >>>>>>>> diff --git a/migration.h b/migration.h >>>>>>>> index 57572a6..713aae0 100644 >>>>>>>> --- a/migration.h >>>>>>>> +++ b/migration.h >>>>>>>> @@ -19,6 +19,7 @@ >>>>>>>> #include "notify.h" >>>>>>>> #include "error.h" >>>>>>>> #include "vmstate.h" >>>>>>>> +#include "qapi-types.h" >>>>>>>> >>>>>>>> struct MigrationParams { >>>>>>>> bool blk; >>>>>>>> @@ -39,6 +40,7 @@ struct MigrationState >>>>>>>> void *opaque; >>>>>>>> MigrationParams params; >>>>>>>> int64_t total_time; >>>>>>>> + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; >>>>>>>> }; >>>>>>>> >>>>>>>> void process_incoming_migration(QEMUFile *f); >>>>>>>> diff --git a/monitor.c b/monitor.c >>>>>>>> index f6107ba..e2be6cd 100644 >>>>>>>> --- a/monitor.c >>>>>>>> +++ b/monitor.c >>>>>>>> @@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = { >>>>>>>> .mhandler.info = hmp_info_migrate, >>>>>>>> }, >>>>>>>> { >>>>>>>> + .name = "migration_capabilities", >>>>>>> >>>>>>> migration-capabilities is better. >>>>>> ok >>>>>>> >>>>>>>> + .args_type = "", >>>>>>>> + .params = "", >>>>>>>> + .help = "show migration capabilities", >>>>>>>> + .mhandler.info = hmp_info_migration_capabilities, >>>>>>>> + }, >>>>>>>> + { >>>>>>>> .name = "balloon", >>>>>>>> .args_type = "", >>>>>>>> .params = "", >>>>>>>> diff --git a/qapi-schema.json b/qapi-schema.json >>>>>>>> index 1ab5dbd..a8408fd 100644 >>>>>>>> --- a/qapi-schema.json >>>>>>>> +++ b/qapi-schema.json >>>>>>>> @@ -288,11 +288,15 @@ >>>>>>>> # status, only returned if status is 'active' and it is a block >>>>>>>> # migration >>>>>>>> # >>>>>>>> -# Since: 0.14.0 >>>>>>>> +# @capabilities: #optional a list describing all the migration capabilities >>>>>>>> +# state >>>>>>> >>>>>>> I don't think this is needed, as I've said above. >>>>>> see above >>>>>>> >>>>>>>> +# >>>>>>>> +# Since: 0.14.0, 'capabilities' since 1.2 >>>>>>>> ## >>>>>>>> { 'type': 'MigrationInfo', >>>>>>>> 'data': {'*status': 'str', '*ram': 'MigrationStats', >>>>>>>> - '*disk': 'MigrationStats'} } >>>>>>>> + '*disk': 'MigrationStats', >>>>>>>> + '*capabilities': ['MigrationCapabilityInfo']} } >>>>>>>> >>>>>>>> ## >>>>>>>> # @query-migrate >>>>>>>> @@ -306,6 +310,51 @@ >>>>>>>> { 'command': 'query-migrate', 'returns': 'MigrationInfo' } >>>>>>>> >>>>>>>> ## >>>>>>>> +# @MigrationCapability >>>>>>>> +# >>>>>>>> +# Migration capabilities enumeration >>>>>>>> +# >>>>>>>> +# @xbzrle: current migration supports xbzrle >>>>>>> >>>>>>> You should explain what xbzrle is. >>>>>> ok >>>>>>> >>>>>>>> +# >>>>>>>> +# Since: 1.2 >>>>>>>> +## >>>>>>>> +{ 'enum': 'MigrationCapability', >>>>>>>> + 'data': ['xbzrle'] } >>>>>>>> + >>>>>>>> +## >>>>>>>> +# @MigrationCapabilityInfo >>>>>>> >>>>>>> MigrationCapabilityStatus? >>>>>>> >>>>>>>> +# >>>>>>>> +# Migration capability information >>>>>>>> +# >>>>>>>> +# @capability: capability enum >>>>>>> >>>>>>> Please, document state too. >>>>>> ok >>>>>> >>>>>> Orit >>>>>>> >>>>>>>> +# >>>>>>>> +# Since: 1.2 >>>>>>>> +## >>>>>>>> +{ 'type': 'MigrationCapabilityInfo', >>>>>>>> + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } } >>>>>>>> + >>>>>>>> +## >>>>>>>> +# @query-migration-capabilities >>>>>>>> +# >>>>>>>> +# Returns information about current migration process capabilties. >>>>>>>> +# >>>>>>>> +# Returns: @MigrationCapabilityInfo list >>>>>>>> +# >>>>>>>> +# Since: 1.2 >>>>>>>> +## >>>>>>>> +{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] } >>>>>>>> + >>>>>>>> +## >>>>>>>> +# @migrate_set_parameters >>>>>>>> +# >>>>>>>> +# Enable/Disable the following migration capabilities (like xbzrle) >>>>>>>> +# >>>>>>>> +# Since: 1.2 >>>>>>>> +## >>>>>>>> +{ 'command': 'migrate-set-parameters', >>>>>>>> + 'data': { 'capabilities': ['MigrationCapabilityInfo'] } } >>>>>>>> + >>>>>>>> +## >>>>>>>> # @MouseInfo: >>>>>>>> # >>>>>>>> # Information about a mouse device. >>>>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx >>>>>>>> index 2e1a38e..3ee6e00 100644 >>>>>>>> --- a/qmp-commands.hx >>>>>>>> +++ b/qmp-commands.hx >>>>>>>> @@ -2075,18 +2075,31 @@ The main json-object contains the following: >>>>>>>> - "transferred": amount transferred (json-int) >>>>>>>> - "remaining": amount remaining (json-int) >>>>>>>> - "total": total (json-int) >>>>>>>> - >>>>>>>> +- "capabilities": migration capabilities state >>>>>>>> + - "xbzrle" : XBZRLE state (json-bool) >>>>>>>> Examples: >>>>>>>> >>>>>>>> 1. Before the first migration >>>>>>>> >>>>>>>> -> { "execute": "query-migrate" } >>>>>>>> -<- { "return": {} } >>>>>>>> +<- { "return": { >>>>>>>> + "capabilities" : [ { "capability" : "xbzrle", "state" : false } ] >>>>>>>> + } >>>>>>>> + } >>>>>>>> >>>>>>>> 2. Migration is done and has succeeded >>>>>>>> >>>>>>>> -> { "execute": "query-migrate" } >>>>>>>> -<- { "return": { "status": "completed" } } >>>>>>>> +<- { "return": { >>>>>>>> + "status": "completed", >>>>>>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >>>>>>>> + "ram":{ >>>>>>>> + "transferred":123, >>>>>>>> + "remaining":123, >>>>>>>> + "total":246 >>>>>>>> + } >>>>>>>> + } >>>>>>>> + } >>>>>>>> >>>>>>>> 3. Migration is done and has failed >>>>>>>> >>>>>>>> @@ -2099,6 +2112,7 @@ Examples: >>>>>>>> <- { >>>>>>>> "return":{ >>>>>>>> "status":"active", >>>>>>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >>>>>>>> "ram":{ >>>>>>>> "transferred":123, >>>>>>>> "remaining":123, >>>>>>>> @@ -2113,6 +2127,7 @@ Examples: >>>>>>>> <- { >>>>>>>> "return":{ >>>>>>>> "status":"active", >>>>>>>> + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], >>>>>>>> "ram":{ >>>>>>>> "total":1057024, >>>>>>>> "remaining":1053304, >>>>>>>> @@ -2135,6 +2150,56 @@ EQMP >>>>>>>> }, >>>>>>>> >>>>>>>> SQMP >>>>>>>> +query-migration-capabilities >>>>>>>> +------- >>>>>>>> + >>>>>>>> +Query migration capabilities >>>>>>>> + >>>>>>>> +- "xbzrle": xbzrle support >>>>>>>> + >>>>>>>> +Arguments: >>>>>>>> + >>>>>>>> +Example: >>>>>>>> + >>>>>>>> +-> { "execute": "query-migration-capabilities"} >>>>>>>> +<- { "return": [ { "capability": "xbzrle", "state": true }, >>>>>>>> + { "capability": "foobar", "state": false } ] } >>>>>>>> + >>>>>>>> +EQMP >>>>>>>> + >>>>>>>> + { >>>>>>>> + .name = "query-migration-capabilities", >>>>>>>> + .args_type = "", >>>>>>>> + .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities, >>>>>>>> + }, >>>>>>>> + >>>>>>>> +SQMP >>>>>>>> +migrate_set_parameters >>>>>>>> +------- >>>>>>>> + >>>>>>>> +Enable/Disable migration capabilities >>>>>>>> + >>>>>>>> +- "xbzrle": xbzrle support >>>>>>>> + >>>>>>>> +Arguments: >>>>>>>> + >>>>>>>> +Example: >>>>>>>> + >>>>>>>> +-> { "execute": "migrate_set_parameters" , "arguments": >>>>>>>> + { "parameters": [ { "capability": "xbzrle", "state": true } ] } } >>>>>>>> + >>>>>>>> +EQMP >>>>>>>> + >>>>>>>> + { >>>>>>>> + .name = "migrate_set_parameters", >>>>>>>> + .args_type = "parameters:O", >>>>>>>> + .params = "capability:s,state:b", >>>>>>>> + .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, >>>>>>>> + }, >>>>>>>> + >>>>>>>> + >>>>>>>> + >>>>>>>> +SQMP >>>>>>>> query-balloon >>>>>>>> ------------- >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>> >> >
diff --git a/hmp-commands.hx b/hmp-commands.hx index f5d9d91..9245bef 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -861,6 +861,20 @@ Set maximum tolerated downtime (in seconds) for migration. ETEXI { + .name = "migrate_set_parameter", + .args_type = "capability:s,state:b", + .params = "", + .help = "Enable/Disable the usage of a capability for migration", + .mhandler.cmd = hmp_migrate_set_parameter, + }, + +STEXI +@item migrate_set_parameter @var{capability} @var{state} +@findex migrate_set_parameter +Enable/Disable the usage of a capability @var{capability} for migration. +ETEXI + + { .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", @@ -1419,6 +1433,8 @@ show CPU statistics show user network stack connection states @item info migrate show migration status +@item info migration_capabilities +show migration capabilities @item info balloon show balloon information @item info qtree diff --git a/hmp.c b/hmp.c index 4c6d4ae..b0440e6 100644 --- a/hmp.c +++ b/hmp.c @@ -131,9 +131,19 @@ void hmp_info_mice(Monitor *mon) void hmp_info_migrate(Monitor *mon) { MigrationInfo *info; + MigrationCapabilityInfoList *cap; info = qmp_query_migrate(NULL); + if (info->has_capabilities && info->capabilities) { + monitor_printf(mon, "capabilities: "); + for (cap = info->capabilities; cap; cap = cap->next) { + monitor_printf(mon, "%s: %s ", + MigrationCapability_lookup[cap->value->capability], + cap->value->state ? "on" : "off"); + } + monitor_printf(mon, "\n"); + } if (info->has_status) { monitor_printf(mon, "Migration status: %s\n", info->status); } @@ -161,6 +171,25 @@ void hmp_info_migrate(Monitor *mon) qapi_free_MigrationInfo(info); } +void hmp_info_migration_capabilities(Monitor *mon) +{ + MigrationCapabilityInfoList *caps_list, *cap; + + caps_list = qmp_query_migration_capabilities(NULL); + if (!caps_list) { + monitor_printf(mon, "No migration capabilities found\n"); + return; + } + + for (cap = caps_list; cap; cap = cap->next) { + monitor_printf(mon, "%s: %s ", + MigrationCapability_lookup[cap->value->capability], + cap->value->state ? "on" : "off"); + } + + qapi_free_MigrationCapabilityInfoList(caps_list); +} + void hmp_info_cpus(Monitor *mon) { CpuInfoList *cpu_list, *cpu; @@ -735,6 +764,41 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict) qmp_migrate_set_speed(value, NULL); } +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) +{ + const char *cap = qdict_get_str(qdict, "capability"); + bool state = qdict_get_bool(qdict, "state"); + Error *err = NULL; + MigrationCapabilityInfoList *params = NULL; + int i; + + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { + if (strcmp(cap, MigrationCapability_lookup[i]) == 0) { + if (!params) { + params = g_malloc0(sizeof(*params)); + } + params->value = g_malloc0(sizeof(*params->value)); + params->value->capability = i; + params->value->state = state; + params->next = NULL; + qmp_migrate_set_parameters(params, &err); + break; + } + } + + if (i == MIGRATION_CAPABILITY_MAX) { + error_set(&err, QERR_INVALID_PARAMETER, cap); + } + + qapi_free_MigrationCapabilityInfoList(params); + + if (err) { + monitor_printf(mon, "migrate_set_parameter: %s\n", + error_get_pretty(err)); + error_free(err); + } +} + void hmp_set_password(Monitor *mon, const QDict *qdict) { const char *protocol = qdict_get_str(qdict, "protocol"); diff --git a/hmp.h b/hmp.h index 79d138d..09ba198 100644 --- a/hmp.h +++ b/hmp.h @@ -25,6 +25,7 @@ void hmp_info_uuid(Monitor *mon); void hmp_info_chardev(Monitor *mon); void hmp_info_mice(Monitor *mon); void hmp_info_migrate(Monitor *mon); +void hmp_info_migration_capabilities(Monitor *mon); void hmp_info_cpus(Monitor *mon); void hmp_info_block(Monitor *mon); void hmp_info_blockstats(Monitor *mon); @@ -51,6 +52,7 @@ void hmp_snapshot_blkdev(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_speed(Monitor *mon, const QDict *qdict); +void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict); void hmp_set_password(Monitor *mon, const QDict *qdict); void hmp_expire_password(Monitor *mon, const QDict *qdict); void hmp_eject(Monitor *mon, const QDict *qdict); diff --git a/migration.c b/migration.c index 8db1b43..fd004d7 100644 --- a/migration.c +++ b/migration.c @@ -117,12 +117,36 @@ MigrationInfo *qmp_query_migrate(Error **errp) { MigrationInfo *info = g_malloc0(sizeof(*info)); MigrationState *s = migrate_get_current(); + int i; switch (s->state) { case MIG_STATE_SETUP: - /* no migration has happened ever */ + /* no migration has ever happened; show enabled capabilities */ + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { + if (!info->has_capabilities) { + info->capabilities = g_malloc0(sizeof(*info->capabilities)); + info->has_capabilities = true; + } + info->capabilities->value = + g_malloc(sizeof(*info->capabilities->value)); + info->capabilities->value->capability = i; + info->capabilities->value->state = s->enabled_capabilities[i]; + info->capabilities->next = NULL; + } break; case MIG_STATE_ACTIVE: + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { + if (!info->has_capabilities) { + info->capabilities = g_malloc0(sizeof(*info->capabilities)); + info->has_capabilities = true; + } + info->capabilities->value = + g_malloc(sizeof(*info->capabilities->value)); + info->capabilities->value->capability = i; + info->capabilities->value->state = s->enabled_capabilities[i]; + info->capabilities->next = NULL; + } + info->has_status = true; info->status = g_strdup("active"); @@ -143,6 +167,18 @@ MigrationInfo *qmp_query_migrate(Error **errp) } break; case MIG_STATE_COMPLETED: + for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { + if (!info->has_capabilities) { + info->capabilities = g_malloc0(sizeof(*info->capabilities)); + info->has_capabilities = true; + } + info->capabilities->value = + g_malloc(sizeof(*info->capabilities->value)); + info->capabilities->value->capability = i; + info->capabilities->value->state = s->enabled_capabilities[i]; + info->capabilities->next = NULL; + } + info->has_status = true; info->status = g_strdup("completed"); @@ -166,6 +202,33 @@ MigrationInfo *qmp_query_migrate(Error **errp) return info; } +MigrationCapabilityInfoList *qmp_query_migration_capabilities(Error **errp) +{ + MigrationCapabilityInfoList *caps_list = g_malloc0(sizeof(*caps_list)); + + caps_list->value = g_malloc(sizeof(*caps_list->value)); + caps_list->value->capability = MIGRATION_CAPABILITY_XBZRLE; + caps_list->next = NULL; + + return caps_list; +} + +void qmp_migrate_set_parameters(MigrationCapabilityInfoList *params, + Error **errp) +{ + MigrationState *s = migrate_get_current(); + MigrationCapabilityInfoList *cap; + + if (s->state == MIG_STATE_ACTIVE) { + error_set(errp, QERR_MIGRATION_ACTIVE); + return; + } + + for (cap = params; cap; cap = cap->next) { + s->enabled_capabilities[cap->value->capability] = cap->value->state; + } +} + /* shared migration helpers */ static int migrate_fd_cleanup(MigrationState *s) @@ -375,12 +438,17 @@ static MigrationState *migrate_init(const MigrationParams *params) { MigrationState *s = migrate_get_current(); int64_t bandwidth_limit = s->bandwidth_limit; + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; + + memcpy(enabled_capabilities, s->enabled_capabilities, + sizeof(enabled_capabilities)); memset(s, 0, sizeof(*s)); s->bandwidth_limit = bandwidth_limit; s->params = *params; + memcpy(s->enabled_capabilities, enabled_capabilities, + sizeof(enabled_capabilities)); - s->bandwidth_limit = bandwidth_limit; s->state = MIG_STATE_SETUP; s->total_time = qemu_get_clock_ms(rt_clock); diff --git a/migration.h b/migration.h index 57572a6..713aae0 100644 --- a/migration.h +++ b/migration.h @@ -19,6 +19,7 @@ #include "notify.h" #include "error.h" #include "vmstate.h" +#include "qapi-types.h" struct MigrationParams { bool blk; @@ -39,6 +40,7 @@ struct MigrationState void *opaque; MigrationParams params; int64_t total_time; + bool enabled_capabilities[MIGRATION_CAPABILITY_MAX]; }; void process_incoming_migration(QEMUFile *f); diff --git a/monitor.c b/monitor.c index f6107ba..e2be6cd 100644 --- a/monitor.c +++ b/monitor.c @@ -2687,6 +2687,13 @@ static mon_cmd_t info_cmds[] = { .mhandler.info = hmp_info_migrate, }, { + .name = "migration_capabilities", + .args_type = "", + .params = "", + .help = "show migration capabilities", + .mhandler.info = hmp_info_migration_capabilities, + }, + { .name = "balloon", .args_type = "", .params = "", diff --git a/qapi-schema.json b/qapi-schema.json index 1ab5dbd..a8408fd 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -288,11 +288,15 @@ # status, only returned if status is 'active' and it is a block # migration # -# Since: 0.14.0 +# @capabilities: #optional a list describing all the migration capabilities +# state +# +# Since: 0.14.0, 'capabilities' since 1.2 ## { 'type': 'MigrationInfo', 'data': {'*status': 'str', '*ram': 'MigrationStats', - '*disk': 'MigrationStats'} } + '*disk': 'MigrationStats', + '*capabilities': ['MigrationCapabilityInfo']} } ## # @query-migrate @@ -306,6 +310,51 @@ { 'command': 'query-migrate', 'returns': 'MigrationInfo' } ## +# @MigrationCapability +# +# Migration capabilities enumeration +# +# @xbzrle: current migration supports xbzrle +# +# Since: 1.2 +## +{ 'enum': 'MigrationCapability', + 'data': ['xbzrle'] } + +## +# @MigrationCapabilityInfo +# +# Migration capability information +# +# @capability: capability enum +# +# Since: 1.2 +## +{ 'type': 'MigrationCapabilityInfo', + 'data': { 'capability' : 'MigrationCapability', 'state' : 'bool' } } + +## +# @query-migration-capabilities +# +# Returns information about current migration process capabilties. +# +# Returns: @MigrationCapabilityInfo list +# +# Since: 1.2 +## +{ 'command': 'query-migration-capabilities', 'returns': ['MigrationCapabilityInfo'] } + +## +# @migrate_set_parameters +# +# Enable/Disable the following migration capabilities (like xbzrle) +# +# Since: 1.2 +## +{ 'command': 'migrate-set-parameters', + 'data': { 'capabilities': ['MigrationCapabilityInfo'] } } + +## # @MouseInfo: # # Information about a mouse device. diff --git a/qmp-commands.hx b/qmp-commands.hx index 2e1a38e..3ee6e00 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -2075,18 +2075,31 @@ The main json-object contains the following: - "transferred": amount transferred (json-int) - "remaining": amount remaining (json-int) - "total": total (json-int) - +- "capabilities": migration capabilities state + - "xbzrle" : XBZRLE state (json-bool) Examples: 1. Before the first migration -> { "execute": "query-migrate" } -<- { "return": {} } +<- { "return": { + "capabilities" : [ { "capability" : "xbzrle", "state" : false } ] + } + } 2. Migration is done and has succeeded -> { "execute": "query-migrate" } -<- { "return": { "status": "completed" } } +<- { "return": { + "status": "completed", + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], + "ram":{ + "transferred":123, + "remaining":123, + "total":246 + } + } + } 3. Migration is done and has failed @@ -2099,6 +2112,7 @@ Examples: <- { "return":{ "status":"active", + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], "ram":{ "transferred":123, "remaining":123, @@ -2113,6 +2127,7 @@ Examples: <- { "return":{ "status":"active", + "capabilities": [ { "capability" : "xbzrle", "state" : false } ], "ram":{ "total":1057024, "remaining":1053304, @@ -2135,6 +2150,56 @@ EQMP }, SQMP +query-migration-capabilities +------- + +Query migration capabilities + +- "xbzrle": xbzrle support + +Arguments: + +Example: + +-> { "execute": "query-migration-capabilities"} +<- { "return": [ { "capability": "xbzrle", "state": true }, + { "capability": "foobar", "state": false } ] } + +EQMP + + { + .name = "query-migration-capabilities", + .args_type = "", + .mhandler.cmd_new = qmp_marshal_input_query_migration_capabilities, + }, + +SQMP +migrate_set_parameters +------- + +Enable/Disable migration capabilities + +- "xbzrle": xbzrle support + +Arguments: + +Example: + +-> { "execute": "migrate_set_parameters" , "arguments": + { "parameters": [ { "capability": "xbzrle", "state": true } ] } } + +EQMP + + { + .name = "migrate_set_parameters", + .args_type = "parameters:O", + .params = "capability:s,state:b", + .mhandler.cmd_new = qmp_marshal_input_migrate_set_parameters, + }, + + + +SQMP query-balloon -------------