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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote:
> 
> On 30/05/23 12:28 pm, Markus Armbruster wrote:
> > 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.
> Yes I agree with your approach too. Will change it to 'MigrationAddressType'
> in the next patchset.
> > > > > +#
> > > > > +# 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".
> Ack. 'endpoints' sounds very appropriate to me.
> > > 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
> No worries Markus. Seems okay.
> > > 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.
> 
> Hmm, basically here the @args means, for example ['/bin/bash', args1, args2,
> ..., <command>], where command -> /some/file/path.
> 
> Does it even make sense now to break into 3 different parts ?
> 
> 	'program': 'str'
> 	'args': [ 'str' ]
> 	'command': 'str'
> 
> This might probably just need to tewak something in the exec file I guess.
> 
> > > > > + #
> > > > > + # 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' ] } }
> Yes, you are right. I figured out after replying to you and was looking at
> the code. Thanks for noticing it out! Will change spurious indentation in
> the v6.
> > > > > +{ '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
> 
> I think @exec work as source too, because in exec.c file, there are calls
> for souce as well as destination.
> 
> I would like to go with "Migration endpoint configuration" because I feel
> 'migrate' and 'migrate-incoming' QAPIs are defined in context of live
> migration.
> 
> > > > > +#
> > > > > +# 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.
> > 
> Okay, so you mean something like :
> 
> +# Since 8.1
> +##
> +{ 'union': 'MigrateAddress',
> +  'base': { 'transport' : 'MigrateTransport'},
> +  'discriminator': 'transport',
> +  'data': {
> +    'inet': 'InetSocketAddress',
> +    'unix': 'UnixSocketAddress',
> +    'vsock': 'VsockSocketAddress',
> +    'fd': 'str',
> +    'exec': 'MigrateExecCommand',
> +    'rdma': 'InetSocketAddress' } }
> 
> Even I agree that directly leveraging this is the best option, but then
> wouldn't it introduce redundancy ? we would not be able to leverage socket
> union right in that case or am I missing something.

The first four are going to have to be packed back into a SocketAddress
struct immediately, as the internal migration APIs all work in terms of
a SocketAddress for the inet/unix/vsock/fd case.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|