Re: [PATCH v5 6/9] migration: modified migration QAPIs to accept 'channels' argument for migration

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

On 25/05/23 11:20 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> MigrateChannelList allows to connect accross multiple interfaces. Added
>> MigrateChannelList struct as argument to migration QAPIs.
>>
>> Future patchset series plans to include multiple MigrateChannels
>> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList'
>> is the preferred choice of argument over 'MigrateChannel' and making
>> migration QAPIs future proof.
>>
>> For current patchset series, have limited the size of the list to single
>> element (single interface) as runtime check.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
> [...]
>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index c500744bb7..86bbc916d1 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1448,12 +1448,47 @@
>>       'exec': 'MigrateExecCommand',
>>       'rdma': 'InetSocketAddress' } }
>>   
>> +##
>> +# @MigrateChannelType:
> As mentioned in my review of PATCH 1, I prefer nouns to verbs for types,
> i.e.  Migration, not Migrate.  More of the same below, not flagging it
> again.
>
Ack.

Also, I forgot ot mention in the 1st patch - even for union and struct 
namings - nouns are preffered over verbs ? or its just for enum types ?
We use structs like - MigrateExecCommand, MigrateChannel --> 
MigrationExecCommand, MigrationChannel ?
and union like - MigrateAddress --> MigrationAddress ?
>> +#
>> +# The supported options for migration channel type requests
>> +#
>> +# @main: Support request for main outbound migration control channel
>> +#
>> +# Since 8.1
>> +##
>> +{ 'enum': 'MigrateChannelType',
>> +  'data': [ 'main' ] }
>> +
>> +##
>> +# @MigrateChannel:
>> +#
>> +# Information regarding migration Channel-type for transferring packets,
>> +# source and corresponding destination interface for socket connection
>> +# and number of multifd channels over the interface.
>> +#
>> +# @channeltype: Name of Channel type for transfering packet information
> @channel-type, because "channeltype" is not a word.
Ack.
>> +#
>> +# @addr: Information regarding migration parameters of destination interface
>> +#
>> +# Since 8.1
>> +##
>> +{ 'struct': 'MigrateChannel',
>> +  'data': {
>> +       'channeltype': 'MigrateChannelType',
>> +       'addr': 'MigrateAddress' } }
>> +
>>   ##
>>   # @migrate:
>>   #
>>   # Migrates the current running guest to another Virtual Machine.
>>   #
>>   # @uri: the Uniform Resource Identifier of the destination VM
>> +#       for migration thread
>> +#
>> +# @channels: Struct containing list of migration channel types, with all
>> +#            the information regarding destination interfaces required for
>> +#            initiating a migration stream.
> Please format like
>
>     # @uri: the Uniform Resource Identifier of the destination VM for
>     #     migration thread
>     #
>     # @channels: Struct containing list of migration channel types, with
>     #     all the information regarding destination interfaces required
>     #     for initiating a migration stream.
>
> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
> to conform to current conventions).
Ack. Will change that in the previous patch and will take care in future 
patches too. Thanks for informing regarding qapi documentation changes!
>>   #
>>   # @blk: do block migration (full disk copy)
>>   #
>> @@ -1479,14 +1514,44 @@
>>   # 3. The user Monitor's "detach" argument is invalid in QMP and should
>>   #    not be used
>>   #
>> +# 4. The uri argument should have the Uniform Resource Identifier of default
>> +#    destination VM. This connection will be bound to default network
>> +#
>> +# 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
>> +#    of the two should be present.
>> +#
> Long lines.  Better:
>
>     # 4. The uri argument should have the Uniform Resource Identifier of
>     #    default destination VM.  This connection will be bound to default
>     #    network
>     #
>     # 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly
>     #    one of the two should be present.
Ack.
>>   # Example:
>>   #
>>   # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>>   # <- { "return": {} }
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channels": [ { "channeltype": "main",
>> +#                          "addr": { "transport": "socket", "type": "inet",
>> +#                                    "host": "10.12.34.9",
>> +#                                    "port": "1050" } } ] } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channels": [ { "channeltype": "main",
>> +#                          "addr": { "transport": "exec",
>> +#                                    "args": [ "/bin/nc", "-p", "6000",
>> +#                                              "/some/sock" ] } } ] } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channels": [ { "channeltype": "main",
>> +#                          "addr": { "transport": "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', '*channels': [ 'MigrateChannel' ], '*blk': 'bool',
>> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>>   
>>   ##
>>   # @migrate-incoming:
>> @@ -1497,6 +1562,10 @@
>>   # @uri: The Uniform Resource Identifier identifying the source or
>>   #     address to listen on
>>   #
>> +# @channels: Struct containing list of migration channel types, with all
>> +#            the information regarding destination interfaces required for
>> +#            initiating a migration stream.
>> +#
> The list doesn't contain migration channel types, it contains migration
> channels.
Yes, my bad. Will update it.
> I'm not sure what you're trying to say by "with all the information ..."
>
> What does it mean to have multiple channels?
In future patchset series, we will be introducing channels over 
different interfaces (src-dest pair), with each channel having multiple 
multifd channels. For now we will restrict the size of the list to 1.
> Please format like
>
>     # @channels: Struct containing list of migration channel types, with
>     #     all the information regarding destination interfaces required
>     #     for initiating a migration stream.
Ack.
>>   # Returns: nothing on success
>>   #
>>   # Since: 2.3
>> @@ -1512,13 +1581,42 @@
>>   #
>>   # 3. The uri format is the same as for -incoming
>>   #
>> +# 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly one
>> +#    of the two should be present.
>> +#
> Long line.  Better:
>
>     # 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly
>     #    one of the two should be present.
Ack.
>>   # Example:
>>   #
>>   # -> { "execute": "migrate-incoming",
>>   #      "arguments": { "uri": "tcp::4446" } }
>>   # <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channels": [ { "channeltype": "main",
>> +#                          "addr": { "transport": "socket", "type": "inet",
>> +#                                    "host": "10.12.34.9",
>> +#                                    "port": "1050" } } ] } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channels": [ { "channeltype": "main",
>> +#                          "addr": { "transport": "exec",
>> +#                                    "args": [ "/bin/nc", "-p", "6000",
>> +#                                              "/some/sock" ] } } ] } }
>> +# <- { "return": {} }
>> +#
>> +# -> { "execute": "migrate",
>> +#      "arguments": {
>> +#          "channels": [ { "channeltype": "main",
>> +#                          "addr": { "transport": "rdma",
>> +#                                    "host": "10.12.34.9",
>> +#                                    "port": "1050" } } ] } }
>> +# <- { "return": {} }
>>   ##
>> -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
>> +{ 'command': 'migrate-incoming',
>> +             'data': {'*uri': 'str',
>> +                      '*channels': [ 'MigrateChannel' ] } }
>>   
>>   ##
>>   # @xen-save-devices-state:
> The text feels cumbersome.  Writing good prose is hard, especially when
> you're not a native speaker.  Eric, would you like to try your hand at
> polishing this?
> [...]
Regards,
Het Gala