Re: [Qemu-devel] [PATCH 13/22] Add migration capabilities

[Orit Wasserman]

On 07/23/2012 09:23 PM, Luiz Capitulino wrote:
> On Fri, 13 Jul 2012 09:23:35 +0200
> Juan Quintela <address@hidden> wrote:
> 
>> From: Orit Wasserman <address@hidden>
>>
>> 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 <address@hidden>
>> Signed-off-by: Juan Quintela <address@hidden>
>> ---
>>  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
>> address@hidden migrate_set_parameter @var{capability} @var{state}
>> address@hidden 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
>> address@hidden 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
>>  -------------
>>
>