Re: [PATCH] multifd: Updated QAPI format for 'migrate' qemu monitor command

[Juan Quintela <quintela@redhat.com>]

Het Gala <het.gala@nutanix.com> wrote:
> To prevent double data encoding of uris, instead of passing transport
> mechanisms, host address and port all together in form of a single string
> and writing different parsing functions, we intend the user to explicitly
> mention most of the parameters through the medium of qmp command itself.
>
> The current patch is continuation of patchset series
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg901274.html
> and reply to the ongoing discussion for better QAPI design here
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg903753.html.
>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>


Hi

1st of all, I can't see how this is 7.1 material, I guess we need to
move it to 8.0.

> ---
>  qapi/migration.json | 127 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 88ecf86ac8..fd9286ea0f 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1449,12 +1449,101 @@
>  ##
>  { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>  
> +##
> +# @MigrateTransport:
> +#
> +# The supported communication transport mechanisms for migration
> +#
> +# @socket: Supported communication type between two devices for migration.
> +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> +#          'fd' already
> +#
> +# @exec: Supported communication type to redirect migration stream into file.
> +#
> +# Since 7.1
> +##
> +{ 'enum': 'MigrateTransport',
> +  'data': ['socket', 'exec'] }

I haven't looked too much into this, but as Danield told in the past, I
can see where the rdma falls into this scheme.  I guess it is going to
be its own, but who knows.

> +# The supported options for migration channel type requests
> +#
> +# @control: Support request for main outbound migration control channel
> +#
> +# @data: Supported Channel type for multifd data channels
> +#
> +# @async: Supported Channel type for post-copy async requests
> +#
> +# Since 7.1
> +##
> +{ 'enum': 'MigrateChannelType',
> +  'data': ['control', 'data', 'async'] }
> +

'data': ['main', 'data', 'ram-async'] } ???

I don't like the 'control' name because without multifd we still pass
everything through it.

And with multifd, we still pass all devices through it.

About the asynchronous channel, I don't know if calling it postcopy is
better.

> +{ 'struct': 'MigrateChannel',
> +  'data' : {
> +    'channeltype' : 'MigrateChannelType',
> +    '*src-addr' : 'MigrateAddress',
> +    'dest-addr' : 'MigrateAddress',

Why do we want *both* addresses?

> +    '*multifd-count' : 'int' } }

And if we are passing a list, why do we want to pass the real number?

>  # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>  # <- { "return": {} }
>  #
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channels": [ { 'channeltype': 'control',
> +#                          'dest-addr': {'transport': 'socket',
> +#                                        'type': 'inet',
> +#                                        'host': '10.12.34.9', 'port': '1050'}},
> +#                        { 'channeltype': 'data',
> +#                          'src-addr': {'transport': 'socket',
> +#                                        'type': 'inet',
> +#                                        'host': '10.12.34.9',
> +#                                        'port': '4000', 'ipv4': 'true'},
> +#                          'dest-addr': { 'transport': 'socket',
> +#                                          'type': 'inet',
> +#                                          'host': '10.12.34.92',
> +#                                          'port': '1234', 'ipv4': 'true'},
> +#                          'multifd-count': 5 },
> +#                        { 'channeltype': 'data',
> +#                          'src-addr': {'transport': 'socket',
> +#                                        'type': 'inet',
> +#                                        'host': '10.2.3.4', 'port': '1000'},
> +#                          'dest-addr': {'transport': 'socket',
> +#                                         'type': 'inet',
> +#                                         'host': '0.0.0.4', 'port': '3000'},
> +#                          'multifd-count': 3 } ] } }
> +# <- { "return": {} }
> +#
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> -           '*detach': 'bool', '*resume': 'bool' } }
> +  'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool',

I think that "uri" bit should be dropped, right?

> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>  
>  ##
>  # @migrate-incoming:

I can't see how to make the old one to work on top of this one (i.e. we
would have to create strings from lists on QAPI, I think that is just
too much).

So I think that the best way (I know I am contradicting myself) is to
create a new migrate command and just let the old one alone.  That way:
- you can drop blk and blk
- you can do anything that you want with the uris, as assuming that they
  are always sockets.
- I would not care at all about the "exec" protocol, just leave that
  alone in the deprecated command.  Right now:
  * we can't move it to multifd without a lot of PAIN
  * there are patches on the list suggesting that what we really want is
    to create a file that is the size of RAM and just write all the RAM
    at the right place.
  * that would make the way to create snapshots (I don't know if anyones
    still wants them, much easier).
  * I think that the only real use of exec migration was to create
    snapshots, for real migration, using a socket is much, much saner.
  * I.e. what I mean here is that for exec migration, we need to think
    if we want to continue supporting it for normal migration, or only
    as a way to create snapshots.

What do you think?

Later, Juan.