Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

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

On 09/02/23 3:59 pm, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
>> Existing 'migrate' QAPI design enforces transport mechanism, ip address
>> of destination interface and corresponding port number in the form
>> of a unified string 'uri' parameter for initiating a migration stream.
>> This scheme has a significant flaw in it - double encoding of existing
>> URIs to extract migration info.
>>
>> The current patch maps QAPI uri design onto well defined MigrateChannel
>> struct. This modified QAPI helps in preventing multi-level uri
>> encodings ('uri' parameter is kept for backward compatibility).
>>
>> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>>   qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 129 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index c84fa10e86..79acfcfe4e 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1449,12 +1449,108 @@
>>   ##
>>   { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>   
>> +
>> +##
>> +# @MigrateSocketAddr:
>> +#
>> +# To support different type of socket.
>> +#
>> +# @socket-type: Different type of socket connections.
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateSocketAddr',
>> +  'data': {'socket-type': 'SocketAddress' } }
> I'd really like this struct to go away, but if it must exist,
> then call this field 'addr', as I think 'socket-type' is overly
> verbose.
In v3 patchset, I have already changed from 'socket-type' to 'data'.
>> +
>> +##
>> +# @MigrateExecAddr:
>> + #
>> + # Since 8.0
>> + ##
>> +{ 'struct': 'MigrateExecAddr',
>> +   'data' : {'data': ['str'] } }
> Instead of having the field called 'data' lets me more
> descriptive, and perhaps rename the struct too:
>
>   { 'struct': 'MigrateCommand',
>      'data' : {'args': ['str'] } }
>
> Any thoughts on whether we should allow for setting env varibles
> too ?

Daniel, won't 'MigrateCommand' be too generic ? I am of the opinion 
that, if its related to 'exec' transport, the struct name should reflect 
that ?

I did not get your question allowing setting environment variables. 
Could you explain it in more detail ?

>> +##
>> +# @MigrateRdmaAddr:
>> +#
>> +# Since 8.0
>> +##
>> +{ 'struct': 'MigrateRdmaAddr',
>> +   'data' : {'data': 'InetSocketAddress' } }
> InetSocketAddress is a plain struct, so I think we can use
> that directly, no ?
Yes, we can use it directly. Just to keep consistency with other 
transport mechanisms, I made a separate struct even for rdma.
>> +
>> +##
>> +# @MigrateAddress:
>> +#
>> +# The options available for communication transport mechanisms for migration
>> +#
>> +# Since 8.0
>> +##
>> +{ 'union' : 'MigrateAddress',
>> +  'base' : { 'transport' : 'MigrateTransport'},
>> +  'discriminator' : 'transport',
>> +  'data' : {
>> +    'socket' : 'MigrateSocketAddr',
>> +    'exec' : 'MigrateExecAddr',
>> +    'rdma': 'MigrateRdmaAddr' } }
> Ideally this would be
>
>     'data' : {
>       'socket' : 'SocketAddress',
>       'exec' : 'MigrateCommand',
>       'rdma': 'InetSocketAddress' } }
>
> though the first SocketAddress isn't possible unless it is easy to
> lift the QAPI limitation.

Yes, I agree with you Daniel. If we can fix the first one - 
SocketAddress one, can we also allow ['str'] to also be directly 
represented by modifying QAPI ?

ex: 'exec': ['str'] ... something like this ?

>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                        "addr": { "transport": "socket",
>> +#                                  "socket-type": { "type': "inet',
>> +#                                                   "host": "10.12.34.9",
>> +#                                                   "port": "1050" } } } } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                       "addr": { "transport": "exec",
>> +#                                 "exec": ["/bin/nc", "-U",
>> +#                                          "/some/sock" ] } } } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channel": { "channeltype": "main",
>> +#                       "addr": { "transport": "rdma",
>> +#                                 "rdma": { "host": "10.12.34.9",
>> +#                                           "port": "1050" } } } } }
>> +# <- { "return": {} }
>> +#
>>   ##
>>   { 'command': 'migrate',
>> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
>> -           '*detach': 'bool', '*resume': 'bool' } }
>> +  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
> IIRC, the intention was to allow multiple channels to be set in a
> follow up to this series. If so that would require adding yet another
> field as an array of MigrateChannel.  Should we just go for the
> array straight away, and just limit it to 1 element  as a runtime
> check ? eg
>
>    'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool',
>             '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
Yes, I got your point Daniel. But I feel it is better to introduce it in 
follow up series along with introducing different channel types (main, 
data, postcopy). It would then also make sense to introduce a list of 
'MigrateChannel'.
> With regards,
> Daniel
Regards,
Het Gala