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

[Het Gala]

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