Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.

[Markus Armbruster]

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

> On 25/05/23 11:04 pm, Markus Armbruster wrote:
>> Het Gala <het.gala@nutanix.com> writes:
>>
>>> This patch introduces well defined MigrateAddress struct and its related 
>>> child
>>> objects.
>>>
>>> The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
>>> string type. The current migration flow follows double encoding scheme for
>>> fetching migration parameters such as 'uri' and this is not an ideal design.
>>>
>>> Motive for intoducing struct level design is to prevent double encoding of 
>>> QAPI
>>> arguments, as Qemu should be able to directly use the QAPI arguments without
>>> any level of encoding.
>>>
>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>   qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 41 insertions(+)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 179af0c4d8..c500744bb7 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1407,6 +1407,47 @@
>>>   ##
>>>   { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>>   +##
>>> +# @MigrateTransport:
>>
>> I'd prefer MigrationTransport, because "migration" is a noun, while
>> migrate is a verb.  Verbs are for commands.  For types we use nouns.
>> More of the same below, not noting it again.
>>
>> Actually, I'd prefer MigrationAddressType, because it's purpose is to
>> serve as discriminator type in union MigrationAddress.
>>
> Okay got it. I kept it Transport as they are different transport mechanisms. 
> But 'MigrationAddressType' looks cleaner and comaptible with 'MigrateAddress' 
> union too. Will change that

Transport isn't bad, but I think a type that is only used for a union
discriminator is best named after the union type.

>>> +#
>>> +# The supported communication transport mechanisms for migration
>>> +#
>>> +# @socket: Supported communication type between two devices for migration.
>>> +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
>>> +#          'fd' already
>>
>> Migration is between hosts, not "two devices".
>
> Here we are just talking about socket communication right ? So I thought 
> devices might also work.

In QEMU, "devices" are the things you create with device_add.

Sockets connect "endpoints".  Also called "peers".

> Will change that to 'hosts' as this is in context of migration i.e. 
> MigrattionAddressType
>
>> The second sentence confuses me.  What are you trying to say?
>
> I am trying to say that socket is a union in itslef right, so it covers 
> communication transport mechanisms like tcp, unix, vsock and fd already in it.
>
>> Also, missing period at the end.
>
> Ack.
>
>>> +#
>>> +# @exec: Supported communication type to redirect migration stream into 
>>> file.
>>> +#
>>> +# @rdma: Supported communication type to redirect rdma type migration 
>>> stream.
>> What about:
>>
>>     ##
>>     # @MigrationTransport:
>>     #
>>     # The migration stream transport mechanisms
>>     #
>>     # @socket: Migrate via socket
>>     #
>>     # @rdma: Migrate via RDMA
>>     #
>>     # @file: Direct the migration stream to a file
>
> Should I change from '@exec' to '@file' ?

Uh, that change happened somewhere between my conscious thought process
and the keyboard ;)

What about

       # @exec: Direct the migration stream to another process

> Other than that, it looks better than what I proposed. Will change it.
>
>>> +#
>>> +# Since 8.1
>>> +##
>>> +{ 'enum': 'MigrateTransport',
>>> +  'data': ['socket', 'exec', 'rdma'] }
>>> +
>>> +##
>>> +# @MigrateExecCommand:
>
>> Documentation of @args is missing.
>
> Ack. Should the naming '@args' be replaced by '@filepath' or @path' or 
> something similar ?

Depends on what @args means.

I guess its [program, arg1, arg2, ...].

You could split off the program:

    'program: 'str',
    'args': [ 'str' ]

Try to write clear documentation for both alternatives.  Such an
exercise tends to lead me to the one I prefer.

>>> + #
>>> + # Since 8.1
>>> + ##
>>
>> Unwanted indentation.
>
> Not able to see any unwanted indentation here ?

Looks like it got eaten on the way.  The last three lines of the doc
comment are indented:

    +##
    +# @MigrateExecCommand:
    + #
    + # Since 8.1
    + ##
    +{ 'struct': 'MigrateExecCommand',
    +   'data': {'args': [ 'str' ] } }

>>> +{ 'struct': 'MigrateExecCommand',
>>> +   'data': {'args': [ 'str' ] } }
>>> +
>>> +##
>>> +# @MigrateAddress:
>>> +#
>>> +# The options available for communication transport mechanisms for 
>>> migration
>> Not happy with this sentence (writing good documentation is hard).
>>
>> Is the address used for the destination only, or for the source as well?
>>
>> If destination only, could it be used for the source at least in theory?
>>
>> I'm asking because I need to understand more about intended use to be
>> able to suggest doc improvements.
>
> This address will be used on both destination and source. In code flow, in 
> later patches, changes on destination as well as source have been made to 
> incorporate same definition.

Does @exec work as source?

Maybe:

     # Endpoint address for migration

or

     # Migration endpoint configuration

>>> +#
>>> +# Since 8.1
>>> +##
>>> +{ 'union': 'MigrateAddress',
>>> +  'base': { 'transport' : 'MigrateTransport'},
>>> +  'discriminator': 'transport',
>>> +  'data': {
>>> +    'socket': 'SocketAddress',
>>> +    'exec': 'MigrateExecCommand',
>>> +    'rdma': 'InetSocketAddress' } }
>>> +
>> Aside: a more powerful type system would let us extend SocketAddress
>> with additional variants instead of wrapping it in a union.
>
> Markus, what do you mean by additional variants here in context of socket? 
> Can you give a small example.

As is, we have a nest of two unions:

* The outer union has branches @socket, @exec, @rdma.

* Branch @socket is the inner union, it has branches @inet, @unix, ...

A more powerful type system would let us extend SocketAddress instead,
so MigrateAddress has everything SocketAddress has, plus additional
branches @exec, @rdma.  Naturally, the type of the discriminator also
needs to be extended, so that it has everything SocketAddress's
discriminator type has, plus additional members @exec, @rdma.