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

[Het Gala <het.gala@nutanix.com>]

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