[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] multifd: Updated QAPI format for 'migrate' qemu monitor comm
From: |
Juan Quintela |
Subject: |
Re: [PATCH] multifd: Updated QAPI format for 'migrate' qemu monitor command |
Date: |
Mon, 21 Nov 2022 13:40:27 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.1 (gnu/linux) |
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.