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

[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.
> > > +{ '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