Re: [PATCH v4 6/8] migration: modified 'migrate' QAPI to accept 'channels' argument for migration

[Daniel P . Berrangé]

On Fri, May 12, 2023 at 02:32:38PM +0000, Het Gala wrote:
> MigrateChannelList ideally allows to connect accross multiple interfaces.
> 
> Added MigrateChannelList struct as argument to 'migrate' qapi. Introduced
> MigrateChannelList in hmp_migrate() and qmp_migrate() functions.
> 
> Future patchset series plans to include multiple MigrateChannels
> for multiple interfaces to be connected. That is the reason, 
> 'MigrateChannelList'
> is the preferred choice of argument over 'MigrateChannel' and making 'migrate'
> QAPI future proof.
> 
> For current patch, have just limit size of MigrateChannelList to 1 element as
> a runtime check.
> 
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration-hmp-cmds.c | 113 ++++++++++++++++++++++++++++++++-
>  migration/migration.c          |  17 ++++-
>  qapi/migration.json            |  69 +++++++++++++++++++-
>  3 files changed, 192 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 4e9f00e7dc..f098d04542 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -51,6 +51,101 @@ static void migration_global_dump(Monitor *mon)
>                     ms->clear_bitmap_shift);
>  }
>  
> +static bool
> +migrate_channel_from_qdict(MigrateChannel **channel,
> +                           const QDict *qdict, Error **errp)
> +{
> +    Error *err = NULL;
> +    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
> +    const char *transport_str = qdict_get_try_str(qdict, "transport");
> +    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
> +    const char *inet_host = qdict_get_try_str(qdict, "host");
> +    const char *inet_port = qdict_get_try_str(qdict, "port");
> +    const char *unix_path = qdict_get_try_str(qdict, "path");
> +    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
> +    const char *vsock_port = qdict_get_try_str(qdict, "port");
> +    const char *fd = qdict_get_try_str(qdict, "str");
> +    QList *exec = qdict_get_qlist(qdict, "exec");

THis seems to be implicitly defining a huge set of extra parameters
for the migrate 'HMP' command, but none of it is actually defined
at the hmp-commands.hx

Do we even need todo this ?  For HMP it is not unreasonable to just
keep using the URI syntax forever?

> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> +    MigrateChannelType channel_type;
> +    MigrateTransport transport;
> +    MigrateAddress *addr = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new(SocketAddress, 1);
> +    SocketAddressType type;
> +
> +    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
> +                                   channeltype, -1, &err);
> +    if (channel_type < 0) {
> +        goto end;
> +    }
> +
> +    transport = qapi_enum_parse(&MigrateTransport_lookup,
> +                                transport_str, -1, &err);
> +    if (transport < 0) {
> +        goto end;
> +    }
> +
> +    type = qapi_enum_parse(&SocketAddressType_lookup,
> +                           socketaddr_type, -1, &err);
> +    if (type < 0) {
> +        goto end;
> +    }
> +
> +    addr->transport = transport;
> +
> +    switch (transport) {
> +    case MIGRATE_TRANSPORT_SOCKET:
> +        saddr->type = type;
> +
> +        switch (type) {
> +        case SOCKET_ADDRESS_TYPE_INET:
> +            saddr->u.inet.host = (char *)inet_host;
> +            saddr->u.inet.port = (char *)inet_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_UNIX:
> +            saddr->u.q_unix.path = (char *)unix_path;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_VSOCK:
> +            saddr->u.vsock.cid = (char *)vsock_cid;
> +            saddr->u.vsock.port = (char *)vsock_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_FD:
> +            saddr->u.fd.str = (char *)fd;
> +            break;
> +        default:
> +            error_setg(errp, "%s: Unknown socket type %d",
> +                       __func__, saddr->type);
> +            goto end;
> +        }
> +
> +        addr->u.socket = *saddr;
> +        break;
> +    case MIGRATE_TRANSPORT_EXEC:
> +        addr->u.exec.args = (strList *)exec;
> +         break;
> +    case MIGRATE_TRANSPORT_RDMA:
> +        addr->u.rdma.host = (char *)inet_host;
> +        addr->u.rdma.port = (char *)inet_port;
> +        break;
> +    default:
> +        error_setg(errp, "%s: Unknown migrate transport type %d",
> +                   __func__, addr->transport);
> +        goto end;
> +    }
> +
> +    val->channeltype = channel_type;
> +    val->addr = addr;
> +    *channel = val;
> +    return true;
> +
> +end:
> +    error_propagate(errp, err);
> +    qapi_free_MigrateChannel(val);
> +    qapi_free_MigrateAddress(addr);
> +    qapi_free_SocketAddress(saddr);
> +    return false;
> +}
> +
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>  {
>      MigrationInfo *info;
> @@ -702,9 +797,18 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>      bool resume = qdict_get_try_bool(qdict, "resume", false);
>      const char *uri = qdict_get_str(qdict, "uri");
>      Error *err = NULL;
> +    MigrateChannel *channel = g_new0(MigrateChannel, 1);
> +    MigrateChannelList *caps = NULL;
> +
> +    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
> +        error_setg(&err, "error in retrieving channel from qdict");
> +        goto end;
> +    }
>  
> -    qmp_migrate(uri, !!blk, blk, !!inc, inc,
> -                false, false, true, resume, &err);
> +    QAPI_LIST_PREPEND(caps, channel);
> +    qmp_migrate(uri, !!caps, caps, !!blk, blk, !!inc, inc,
> +                 false, false, true, resume, &err);
> +    qapi_free_MigrateChannelList(caps);
>      if (hmp_handle_error(mon, err)) {
>          return;
>      }
> @@ -725,6 +829,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>                                            status);
>          timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
>      }
> +
> +end:
> +    qapi_free_MigrateChannel(channel);
> +    hmp_handle_error(mon, err);
> +    return;
>  }
>  
>  void migrate_set_capability_completion(ReadLineState *rs, int nb_args,
> diff --git a/migration/migration.c b/migration/migration.c
> index 6abd69df8d..78f16e5041 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1694,15 +1694,26 @@ static bool migrate_prepare(MigrationState *s, bool 
> blk, bool blk_inc,
>      return true;
>  }
>  
> -void qmp_migrate(const char *uri, bool has_blk, bool blk,
> -                 bool has_inc, bool inc, bool has_detach, bool detach,
> -                 bool has_resume, bool resume, Error **errp)
> +void qmp_migrate(const char *uri, bool has_channels,
> +                 MigrateChannelList *channels, bool has_blk, bool blk,
> +                 bool has_inc, bool inc, bool has_detach,
> +                 bool detach, bool has_resume, bool resume, Error **errp)
>  {
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      MigrateAddress *channel = g_new0(MigrateAddress, 1);
>      SocketAddress *saddr;
>  
> +    /*
> +     * Having preliminary checks for uri and channel
> +     */
> +    if (uri && has_channels) {
> +        error_setg(errp, "'uri' and 'channels' arguments are mutually "
> +                   "exclusive; exactly one of the two should be present in "
> +                   "'migrate' qmp command ");
> +        return;
> +    }
> +



>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>          goto out;
> diff --git a/qapi/migration.json b/qapi/migration.json
> index bf90bd8fe2..a71af87ffe 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1464,12 +1464,47 @@
>      'exec': 'MigrateExecCommand',
>      'rdma': 'InetSocketAddress' } }
>  
> +##
> +# @MigrateChannelType:
> +#
> +# The supported options for migration channel type requests
> +#
> +# @main: Support request for main outbound migration control channel
> +#
> +# Since 8.0
> +##
> +{ 'enum': 'MigrateChannelType',
> +  'data': [ 'main' ] }
> +
> +##
> +# @MigrateChannel:
> +#
> +# Information regarding migration Channel-type for transferring packets,
> +# source and corresponding destination interface for socket connection
> +# and number of multifd channels over the interface.
> +#
> +# @channeltype: Name of Channel type for transfering packet information
> +#
> +# @addr: Information regarding migration parameters of destination interface
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateChannel',
> +  'data': {
> +       'channeltype': 'MigrateChannelType',
> +       'addr': 'MigrateAddress' } }
> +
>  ##
>  # @migrate:
>  #
>  # Migrates the current running guest to another Virtual Machine.
>  #
>  # @uri: the Uniform Resource Identifier of the destination VM
> +#       for migration thread
> +#
> +# @channels: Struct containing list of migration channel types, with all
> +#            the information regarding destination interfaces required for
> +#            initiating a migration stream.
>  #
>  # @blk: do block migration (full disk copy)
>  #
> @@ -1494,15 +1529,45 @@
>  # 3. The user Monitor's "detach" argument is invalid in QMP and should not
>  #    be used
>  #
> +# 4. The uri argument should have the Uniform Resource Identifier of default
> +#    destination VM. This connection will be bound to default network
> +#
> +# 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
> +#    of the two should be present.
> +#
>  # Example:
>  #
>  # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>  # <- { "return": {} }
>  #
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channeltype": "main",
> +#                          "addr": { "transport": "socket", "type": "inet",
> +#                                    "host": "10.12.34.9",
> +#                                    "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channeltype": "main",
> +#                          "addr": { "transport": "exec",
> +#                                    "args": [ "/bin/nc", "-p", "6000",
> +#                                              "/some/sock" ] } } ] } }
> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { "channeltype": "main",
> +#                          "addr": { "transport": "rdma",
> +#                                    "host": "10.12.34.9",
> +#                                    "port": "1050" } } ] } }
> +# <- { "return": {} }
> +#
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> -           '*detach': 'bool', '*resume': 'bool' } }
> +  'data': {'*uri': 'str', '*channels': [ 'MigrateChannel' ], '*blk': 'bool',
> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>  
>  ##
>  # @migrate-incoming:
> -- 
> 2.22.3
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|