Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Eric Blake]

On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
> Existing 'migrate' QAPI design enforces transport mechanism, ip address
> of destination interface and corresponding port number in the form
> of a unified string 'uri' parameter for initiating a migration stream.
> This scheme has a significant flaw in it - double encoding of existing
> URIs to extract migration info.
> 
> The current patch maps QAPI uri design onto well defined MigrateChannel
> struct. This modified QAPI helps in preventing multi-level uri
> encodings ('uri' parameter is kept for backward compatibility).
> 
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  qapi/migration.json | 131 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 129 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index c84fa10e86..79acfcfe4e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1449,12 +1449,108 @@
>  ##
>  { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>  
> +##
> +# @MigrateTransport:
> +#
> +# 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
> +#
> +# @exec: Supported communication type to redirect migration stream into file.
> +#
> +# @rdma: Supported communication type to redirect rdma type migration stream.
> +#
> +# Since 8.0
> +##
> +{ 'enum': 'MigrateTransport',
> +  'data': ['socket', 'exec', 'rdma'] }
> +
> +##
> +# @MigrateSocketAddr:
> +#
> +# To support different type of socket.
> +#
> +# @socket-type: Different type of socket connections.
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateSocketAddr',
> +  'data': {'socket-type': 'SocketAddress' } }

Here, you use 'socket-type',...

> +
> +##
> +# @MigrateExecAddr:
> + #
> + # Since 8.0
> + ##
> +{ 'struct': 'MigrateExecAddr',
> +   'data' : {'data': ['str'] } }

Inconsistent on whether you have a space before :.  Most of our qapi
files prefer the layout:

'key': 'value'

that is, no space before, one space after.  It doesn't affect
correctness, but a consistent visual style is worth striving for.

> +
> +##
> +# @MigrateRdmaAddr:
> +#
> +# Since 8.0
> +##
> +{ 'struct': 'MigrateRdmaAddr',
> +   'data' : {'data': 'InetSocketAddress' } }

...while these branches supply everything else under 'data'. Also,
while you documented @socket-type above, you did not document @data in
either of these two types.  [1]

> +
> +##
> +# @MigrateAddress:
> +#
> +# The options available for communication transport mechanisms for migration
> +#
> +# Since 8.0
> +##
> +{ 'union' : 'MigrateAddress',
> +  'base' : { 'transport' : 'MigrateTransport'},
> +  'discriminator' : 'transport',
> +  'data' : {
> +    'socket' : 'MigrateSocketAddr',
> +    'exec' : 'MigrateExecAddr',
> +    'rdma': 'MigrateRdmaAddr' } }

Another example of inconsistent spacing around :.

I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
is that SocketAddress is itself a discriminated union, and Markus does
not yet have the QAPI generator wired up to support one union as a
branch of another larger union?  It leads to extra nesting on the wire
[2]

> +
> +##
> +# @MigrateChannelType:
> +#
> +# The supported options for migration channel type requests
> +#
> +# @main: Support request for main outbound migration control channel
> +#
> +# Since 8.0
> +##
> +{ 'enum': 'MigrateChannelType',
> +  'data': [ 'main'] }

A different spacing issue: most arrays in QAPI either have spaces at
both ends (as in [ 'string' ]) or neither (as in ['string']).  Here,
it looks lopsided with space at the front but not the back.

> +
> +##
> +# @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
> +#
> +# @addr: SocketAddress of destination interface

More than just a SocketAddress, per the discriminated union type defined above.

> +#
> +# Since 8.0
> +##
> +{ '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
> +#
> +# @channel: Struct containing migration channel type, along with all
> +#           the details of destination interface required for initiating
> +#           a migration stream.
>  #
>  # @blk: do block migration (full disk copy)
>  #
> @@ -1479,15 +1575,46 @@
>  # 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. Both 'uri' and 'channel' arguments, are mutually exclusive but, at least
> +#    one of the two arguments should be present.

Grammar suggestion:

'uri' and 'channel' are mutually exclusive; exactly one of the two
should be present.

> +#
>  # Example:
>  #
>  # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
>  # <- { "return": {} }
>  #
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                        "addr": { "transport": "socket",
> +#                                  "socket-type": { "type': "inet',
> +#                                                   "host": "10.12.34.9",
> +#                                                   "port": "1050" } } } } }

[2] This is the extra nesting that occurs because of the
'socket-type':'MigrateSocketAddr' above; getting rid of the nesting
would require 'socket-type':'SocketAddress', but QAPI would need to
support that first.

> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                       "addr": { "transport": "exec",
> +#                                 "exec": ["/bin/nc", "-U",
> +#                                          "/some/sock" ] } } } }

Another lopsided spacing in [].

> +# <- { "return": {} }
> +#
> +# -> { "execute": "migrate",
> +#      "arguments": {
> +#          "channel": { "channeltype": "main",
> +#                       "addr": { "transport": "rdma",
> +#                                 "rdma": { "host": "10.12.34.9",
> +#                                           "port": "1050" } } } } }

[1] These examples look wrong; above, the schema named the nesting as 'data', 
rather than 'exec' or 'rdma'.

> +# <- { "return": {} }
> +#
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> -           '*detach': 'bool', '*resume': 'bool' } }
> +  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
> +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
>

But overall, I'm a fan of using a more type-accurate description of
the channel than the open-coded 'uri':'str'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org