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

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

On 30/05/23 12:43 pm, Markus Armbruster wrote:
> Het Gala <het.gala@nutanix.com> writes:
>
>> 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 "types are things, and names of things are nouns" argument applies
> to all types.
>
> Yes, existing names use verbs in places.  Mildly annoying.
>
> Renaming them would not create compatibility problems, as types are not
> part of the external interface.  Up to migration maintainers.
Yes, I got your point. Will change naming convention for all 'types' as 
nouns other than 'command' which will be a verb. Will kep this in mind 
in future too. Thanks!
>>>> +#
>>>> +# 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!
> Gladly!  It's the only way to make the nicer formatting stick :)
Yes 😅
>
>>>>    #
>>>>    # @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.
> Writing good documentation is hard!
>
>>> 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 document this restriction right here.
Ack. But it is mainly an implementation point that's the reason I did 
not mention it here but have mentioned it in the migration code flow 
path. Will add one more point to note down.
> When you add support for multiple channels, will each channel have a
> unique channel type?

Not every channel will have a unique channel type but mixture like, for 
multifd in future : (main, data, data, data) --> the first connection 
will be migration main connection, other three will be multifd connections.

> [...]
Regards,
Het Gala