qapi/migration.json | 127 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 2 deletions(-)
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>
---
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'] }
+
+##
+# @MigrateSocketAddrType:
+#
+# To support different type of socket.
+#
+# @socket-type: Different type of socket connections.
+#
+# Since (7.1)
+##
+{ 'struct': 'MigrateSocketAddrType',
+ 'data': {'socket-type': 'SocketAddress' } }
+
+##
+# @MigrateAddress:
+#
+# The options available for communication transport mechanisms for migration
+#
+# Since 7.1
+##
+{ 'union' : 'MigrateAddress',
+ 'base' : { 'transport' : 'MigrateTransport'},
+ 'discriminator' : 'transport',
+ 'data' : {
+ 'socket' : 'MigrateSocketAddrType',
+ 'exec' : 'String' } }
+
+##
+# @MigrateChannelType:
+#
+# 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'] }
+
+##
+# @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
+#
+# @src-addr: SocketAddress of source interface
+#
+# @dest-addr: SocketAddress of destination interface
+#
+# @multifd-count: number of multifd channels used to migrate
+# data between a specific source and destination
+# interface. Default value in this case is 2.
+#
+# Since 7.1
+##
+{ 'struct': 'MigrateChannel',
+ 'data' : {
+ 'channeltype' : 'MigrateChannelType',
+ '*src-addr' : 'MigrateAddress',
+ 'dest-addr' : 'MigrateAddress',
+ '*multifd-count' : 'int' } }
+
##
# @migrate:
#
# Migrates the current running guest to another Virtual Machine.
#
# @uri: the Uniform Resource Identifier of the destination VM
+# for migration thread
+#
+# @channels: list of migration channel type, pair of source and
+# destination interface with number of multifd-channels
+# for each pair
#
# @blk: do block migration (full disk copy)
#
@@ -1479,15 +1568,49 @@
# 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. Both 'uri' and 'channels' arguments, are mututally exclusive.
+#
+# 6. The 'channels' argument should contain atleast one control channel for
+# main outbound migration.
+#
# Example:
#
# -> { "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',
+ '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
##
# @migrate-incoming:
--
2.31.1Het 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.
On 21/11/22 6:10 pm, Juan Quintela wrote:
> 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.
> Hi
>
> 1st of all, I can't see how this is 7.1 material, I guess we need to
> move it to 8.0.
>
> Thankyou for informing. I will change it to 8.0
>> +##
>> +# @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.
>So do you mean, 'data' : ['socket', 'exec', 'rdma'] ? or it will be
separately represented
>> +# 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.
> I agree with you Juan. 'main' seams a better name here. Thanks for
the suggestion :)
> About the asynchronous channel, I don't know if calling it postcopy is
> better.
> Okay, noted. Will change in the next patchset
>> +{ 'struct': 'MigrateChannel',
>> + 'data' : {
>> + 'channeltype' : 'MigrateChannelType',
>> + '*src-addr' : 'MigrateAddress',
>> + 'dest-addr' : 'MigrateAddress',
> Why do we want *both* addresses?
> As mentioned by Daniel, multifd right now is supported for tcp
socket, and we want to make multifd migration as bi-directional
migration stream.
>> + '*multifd-count' : 'int' } }
> And if we are passing a list, why do we want to pass the real number?
> I think multifd-count variable would be handy. Just to take a used
case here, we have heavy workloads to migrate, and in our system we have
a 25 Gig X 2 NIC cards available, and we want to ensure that migration
does not fail due to less available network (because IO, and other
processes might also consume network). By experiments, we know that per
multifd channel - on an average we get around 8.5-10 Gbps. Writing the
same channel atleast 3 times to cover one NIC seems again redundant. So
to prevent this, we think inclusion of multifd-count would be useful
there. And anyways it is an optional, so if you don't mention it, it
will take into account as a single thread by default. Let me know if
this is convincing or we can discuss more on this ?
>> +# -> { "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?
> yes, it will be dropped at a later point, right now we are keeping it
for backward compatibility. Daniel has also responded to the same query.
>> + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>
>> ##
>> # @migrate-incoming:
> We want to know your views on a suggestion we had in your mind. Like
we know that for now, multifd feature is available only for tcp/inet. So
should we frame our QAPI design in such a way that instead of 'socket'
including 'tcp', 'fd', 'vsock', 'unix'; we should keep the transport
union as ['tcp','fd','vsock','unix','exec'] and we would have
'MigrateAddress' struct only for 'tcp' (i.e. bi-directional migration
stream only for tcp). For other transoprt protocols (uni-directional),
we can either have struct which would include 'path' or can directly
link to 'VsockSocketAddres',.......
Juan, Daniel, Markus and other migration maintainers too, would love to
hear your inputs on this :)
Regards,
Het Gala
On Wed, Nov 23, 2022 at 01:35:22AM +0530, Het Gala wrote:
>
> On 21/11/22 6:10 pm, Juan Quintela wrote:
> > 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.
> > Hi
> >
> > 1st of all, I can't see how this is 7.1 material, I guess we need to
> > move it to 8.0.
> >
> > Thankyou for informing. I will change it to 8.0
> > > +##
> > > +# @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.
> So do you mean, 'data' : ['socket', 'exec', 'rdma'] ? or it will be
> separately represented
Yes, you'll need an rdma entry there. If you follow the patch split
I suggested in the other mail, you'll naturally have to represent
'rdma' in the very first patch, as you introduce the new syntax
for addresses and parse the existing URIs to map onto the new
address struct.
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 :|
On Mon, Nov 21, 2022 at 01:40:27PM +0100, Juan Quintela wrote:
> 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>
>
> > +{ 'struct': 'MigrateChannel',
> > + 'data' : {
> > + 'channeltype' : 'MigrateChannelType',
> > + '*src-addr' : 'MigrateAddress',
> > + 'dest-addr' : 'MigrateAddress',
>
> Why do we want *both* addresses?
This is part of their goal to have source based routing, so that
traffic exits the src host via a particular NIC.
I think this patch would be better if we split it into two parts.
First introduce "MigrateChannel" struct with *only* the 'dest-addr'
field, and only allow one element in the list, making 'uri' optional.
Basically the first patch would *only* be about switching the
'migrate' command from using a plain string to a MigrateAddress
structure.
A second patch would then extend it to allow multiple MigrateChannel
elements, to support different destinations for each channel.
A third patch would then extend it to add src-addr to attain the
source based routing.
A fourth patch can then deprecate the existing 'uri' field.
>
> > + '*multifd-count' : 'int' } }
>
> And if we are passing a list, why do we want to pass the real number?
Yeah, technically I think this field is redundant, as you can just
add many entires to the 'channels' list, using the same addresses.
All this field does is allow a more compact JSON arg list. I'm
not sure this is neccessary, unless we're expecting huge numbers
of 'channels', and even then this isn't likely to be a performance
issue.
>
> > # -> { "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?
No, it is required for back compatibility with existing clients.
It should be marked deprecated, and removed several releases later,
at which point 'chanels' can become mandatory instead of optional.
>
> > + '*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).
All we need is a piece of code that parses the 'uri' parameter and
creates a MigrateAddress address from it. This can be called in
the qmp_migrate() handler, and thereafter, everything else in the
migration code can work with the MigrateAddress structs. SHould
be pretty easy.
>
> 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
blk & inc
> - you can do anything that you want with the uris, as assuming that they
> are always sockets.
Regardless of whether we use the existing or new QMP command, we still
have to convert the code to use the MigrateAddress struct, so I don't
think it makes any difference. Both cases will require that we write
code to convert from the legacy 'string' URI, to the new MigrateAddress
struct.
> - 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.
The main challenge with 'exec' protocol is that it only sets up a
uni-directional data channel. The main migration channel protocol
has always been unidirectional, and that's responsible for alot of
our pain in migration. There's no way todo a protocol handshake to
negotiate features between client&server during connection. As such
we've invented a ridiculous number of migration parameters that
libvirt and other mgmt apps need to set on both sides - basically
we have punted negotiation out of band to the mgmt app which is
crazy.
For our future sanity I think we need to define a brand new migration
protocol which is bidirectional from the start. A large number of the
MigrateParameters would become obsolete immediately, as QEMU could
negotiate them automatically. This would let QEMU introduce new
migration features without requiring any work in libvirt in many
cases. Libvirt should only be required when there are performance
tunables that need exposing, never for protocol feature negotiation.
I think introducing a new QMP command, without introducing a fully
new protocol would be a big mistake as the QMP command is not the
problem we have.
I've written much more detail about this here:
https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03655.html
I don't think this should be a dependancy for this patch proposal
though. This is purely about making the 'migrate' command follow
QMP best practice, by using a struct instead of encoding data in
a string URI, and can be retrofitted to the existing command without
difficulty.
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 :|
On 22/11/22 2:53 pm, Daniel P. Berrangé wrote:
> On Mon, Nov 21, 2022 at 01:40:27PM +0100, Juan Quintela wrote:
>> 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.
>>> +{ 'struct': 'MigrateChannel',
>>> + 'data' : {
>>> + 'channeltype' : 'MigrateChannelType',
>>> + '*src-addr' : 'MigrateAddress',
>>> + 'dest-addr' : 'MigrateAddress',
>> Why do we want *both* addresses?
> This is part of their goal to have source based routing, so that
> traffic exits the src host via a particular NIC.
>
> I think this patch would be better if we split it into two parts.
>
> First introduce "MigrateChannel" struct with *only* the 'dest-addr'
> field, and only allow one element in the list, making 'uri' optional.
>
> Basically the first patch would *only* be about switching the
> 'migrate' command from using a plain string to a MigrateAddress
> structure.
>
> A second patch would then extend it to allow multiple MigrateChannel
> elements, to support different destinations for each channel.
>
> A third patch would then extend it to add src-addr to attain the
> source based routing.
>
> A fourth patch can then deprecate the existing 'uri' field.
> Thanks Daniel. This is a nice idea. I will break this patch into 4
different patches, so it would be clear to see how the QAPI design is
evolving.
>>> + '*multifd-count' : 'int' } }
>> And if we are passing a list, why do we want to pass the real number?
> Yeah, technically I think this field is redundant, as you can just
> add many entires to the 'channels' list, using the same addresses.
> All this field does is allow a more compact JSON arg list. I'm
> not sure this is neccessary, unless we're expecting huge numbers
> of 'channels', and even then this isn't likely to be a performance
> issue.
> I have tried to explain this to Juan. The main purpose is, if we want
to add 3 channels to one pair of interface and 4 pair of channels to
another pair of interface, instead of writing the same interface 3 or 4
times, this field saves that redundancy, and I personally feel, it gives
one clear representation of multifd capability.
>>> +# -> { "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 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.
>
> With regards,
> Daniel
Thanks and regards,
Het Gala
On 22/11/22 2:53 pm, Daniel P. Berrangé wrote:
> On Mon, Nov 21, 2022 at 01:40:27PM +0100, Juan Quintela wrote:
>> 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://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_qemu-2Ddevel-40nongnu.org_msg901274.html&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=c4KON2DiMd-szjwjggQcuUvTsPWblztAL0gVzaHnNmc&m=htOVZmDRCu27EhvYDu-28snN6GV0a6-ffX34joSwBkELLLVGHzRqD0LicZed3Xtw&s=xvzrWRBN4S5l3orPqu6di0MRq-gSGWZZU6-e7wpn8w4&e=
>>> and reply to the ongoing discussion for better QAPI design here
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_qemu-2Ddevel-40nongnu.org_msg903753.html&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=c4KON2DiMd-szjwjggQcuUvTsPWblztAL0gVzaHnNmc&m=htOVZmDRCu27EhvYDu-28snN6GV0a6-ffX34joSwBkELLLVGHzRqD0LicZed3Xtw&s=anLqZbhqa73P9SyPUaGk5q8R0SYR6IUtH_FWFML3lZY&e= .
>>>
>>> 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>
>
>
>
>>> +{ 'struct': 'MigrateChannel',
>>> + 'data' : {
>>> + 'channeltype' : 'MigrateChannelType',
>>> + '*src-addr' : 'MigrateAddress',
>>> + 'dest-addr' : 'MigrateAddress',
>> Why do we want *both* addresses?
> This is part of their goal to have source based routing, so that
> traffic exits the src host via a particular NIC.
>
> I think this patch would be better if we split it into two parts.
>
> First introduce "MigrateChannel" struct with *only* the 'dest-addr'
> field, and only allow one element in the list, making 'uri' optional.
>
> Basically the first patch would *only* be about switching the
> 'migrate' command from using a plain string to a MigrateAddress
> structure.
>
> A second patch would then extend it to allow multiple MigrateChannel
> elements, to support different destinations for each channel.
>
> A third patch would then extend it to add src-addr to attain the
> source based routing.
>
> A fourth patch can then deprecate the existing 'uri' field.
>
>>> + '*multifd-count' : 'int' } }
>> And if we are passing a list, why do we want to pass the real number?
> Yeah, technically I think this field is redundant, as you can just
> add many entires to the 'channels' list, using the same addresses.
> All this field does is allow a more compact JSON arg list. I'm
> not sure this is neccessary, unless we're expecting huge numbers
> of 'channels', and even then this isn't likely to be a performance
> issue.
>
>>> # -> { "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?
> No, it is required for back compatibility with existing clients.
> It should be marked deprecated, and removed several releases later,
> at which point 'chanels' can become mandatory instead of optional.
>
>>> + '*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).
> All we need is a piece of code that parses the 'uri' parameter and
> creates a MigrateAddress address from it. This can be called in
> the qmp_migrate() handler, and thereafter, everything else in the
> migration code can work with the MigrateAddress structs. SHould
> be pretty easy.
>
>> 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
> blk & inc
>
>> - you can do anything that you want with the uris, as assuming that they
>> are always sockets.
> Regardless of whether we use the existing or new QMP command, we still
> have to convert the code to use the MigrateAddress struct, so I don't
> think it makes any difference. Both cases will require that we write
> code to convert from the legacy 'string' URI, to the new MigrateAddress
> struct.
>
>> - 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.
> The main challenge with 'exec' protocol is that it only sets up a
> uni-directional data channel. The main migration channel protocol
> has always been unidirectional, and that's responsible for alot of
> our pain in migration. There's no way todo a protocol handshake to
> negotiate features between client&server during connection. As such
> we've invented a ridiculous number of migration parameters that
> libvirt and other mgmt apps need to set on both sides - basically
> we have punted negotiation out of band to the mgmt app which is
> crazy.
>
> For our future sanity I think we need to define a brand new migration
> protocol which is bidirectional from the start. A large number of the
> MigrateParameters would become obsolete immediately, as QEMU could
> negotiate them automatically. This would let QEMU introduce new
> migration features without requiring any work in libvirt in many
> cases. Libvirt should only be required when there are performance
> tunables that need exposing, never for protocol feature negotiation.
>
> I think introducing a new QMP command, without introducing a fully
> new protocol would be a big mistake as the QMP command is not the
> problem we have.
>
> I've written much more detail about this here:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2022-2D03_msg03655.html&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=c4KON2DiMd-szjwjggQcuUvTsPWblztAL0gVzaHnNmc&m=htOVZmDRCu27EhvYDu-28snN6GV0a6-ffX34joSwBkELLLVGHzRqD0LicZed3Xtw&s=56T0bqIyHaHqZRDImrEfhvQHlsiPZXX-dCpX0Bm98wE&e=
Daniel, Totally agree on above mentioned discussion. Does this account for verifying other things too along with migration capabilities e.g. if qemu machine type, vm config or cpu features are excatly same of both side. Currently libvirt takes that responsibility, but as you mentioned libvirt may take some time to get to level where qemu is so causing potential issues till then. Similar to this discussion we had on libvirt list https://www.mail-archive.com/libvir-list@redhat.com/msg233725.html. If these things too were managed by qemu indepenedelty things could have been much better. I mean those too are kind of related to live migration. :)
Thanks
Manish Mishra
> I don't think this should be a dependancy for this patch proposal
> though. This is purely about making the 'migrate' command follow
> QMP best practice, by using a struct instead of encoding data in
> a string URI, and can be retrofitted to the existing command without
> difficulty.
>
>
>
> With regards,
> Daniel
On Tue, Nov 22, 2022 at 09:27:52PM +0530, manish.mishra wrote: > > On 22/11/22 2:53 pm, Daniel P. Berrangé wrote: > > For our future sanity I think we need to define a brand new migration > > protocol which is bidirectional from the start. A large number of the > > MigrateParameters would become obsolete immediately, as QEMU could > > negotiate them automatically. This would let QEMU introduce new > > migration features without requiring any work in libvirt in many > > cases. Libvirt should only be required when there are performance > > tunables that need exposing, never for protocol feature negotiation. > > > > I think introducing a new QMP command, without introducing a fully > > new protocol would be a big mistake as the QMP command is not the > > problem we have. > > > > Daniel, Totally agree on above mentioned discussion. Does this > account for verifying other things too along with migration > capabilities e.g. if qemu machine type, vm config or cpu > features are excatly same of both side. Currently libvirt > takes that responsibility, but as you mentioned libvirt may > take some time to get to level where qemu is so causing > potential issues till then. Similar to this discussion we > had on libvirt list https://www.mail-archive.com/libvir-list@redhat.com/msg233725.html. > If these things too were managed by qemu indepenedelty > things could have been much better. I mean those too are > kind of related to live migration. :) Today libvirt has no choice, because if there's an ABI compat mistake in the dest QEMU config, vs the src QEMU config, then when loading VMstate on the target, the dst QEMU will often be unable to de-serialize the migration device stream and end up printing an error on stderr and exiting. Getting that info back to the src QEMU is impossible. If QEMU had a bi-directional migration stream, then the dst QEMU could simply send an error message back to the src QEMU and 'query-migrate' could actually display this. There may still be validation libvirt wants to do as well, but at least there would be the possiblity of offloading to QEMU without sacrificing error reporting. 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 :|
© 2016 - 2024 Red Hat, Inc.