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

[Daniel P . Berrangé]

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 :|