On Fri, 13 Jul 2012 09:23:35 +0200
Juan Quintela <quint...@redhat.com> wrote:
> From: Orit Wasserman <owass...@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 <owass...@redhat.com>
> Signed-off-by: Juan Quintela <quint...@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
> -------------
>
