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

[Markus Armbruster <armbru@redhat.com>]

Het Gala <het.gala@nutanix.com> writes:

> 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>

[...]

>>>>> @@ -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.

Documenting temporary restrictions is work that's useful only
temporarily.

When a restriction goes away within the same patch series, not doing
that work can make sense.  I'd still want it mentioned in the commit
message.

It's tempting treat restrictions expected to go away before we release
the same.  But when lifting the restriction misses the release, it's
easy to forget we still have a restriction to document.  Better document
it right away.

>> 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.

Got it, thanks!