[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu mon
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command |
Date: |
Thu, 9 Feb 2023 13:22:30 +0000 |
User-agent: |
Mutt/2.2.9 (2022-11-12) |
On Thu, Feb 09, 2023 at 06:41:41PM +0530, Het Gala wrote:
>
> On 09/02/23 3:59 pm, Daniel P. Berrangé wrote:
> > 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'} }
> > > +
> > > +##
> > > +# @MigrateSocketAddr:
> > > +#
> > > +# To support different type of socket.
> > > +#
> > > +# @socket-type: Different type of socket connections.
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateSocketAddr',
> > > + 'data': {'socket-type': 'SocketAddress' } }
> > I'd really like this struct to go away, but if it must exist,
> > then call this field 'addr', as I think 'socket-type' is overly
> > verbose.
> In v3 patchset, I have already changed from 'socket-type' to 'data'.
> > > +
> > > +##
> > > +# @MigrateExecAddr:
> > > + #
> > > + # Since 8.0
> > > + ##
> > > +{ 'struct': 'MigrateExecAddr',
> > > + 'data' : {'data': ['str'] } }
> > Instead of having the field called 'data' lets me more
> > descriptive, and perhaps rename the struct too:
> >
> > { 'struct': 'MigrateCommand',
> > 'data' : {'args': ['str'] } }
> >
> > Any thoughts on whether we should allow for setting env varibles
> > too ?
>
> Daniel, won't 'MigrateCommand' be too generic ? I am of the opinion that, if
> its related to 'exec' transport, the struct name should reflect that ?
Mostly I'm indicating that it is not really an address that
we're providing, it is a command argv, so felt the struct
could reflect that. We could do MigrateExecCommand.
> I did not get your question allowing setting environment variables. Could
> you explain it in more detail ?
When spawning processes, execvp() lets use provide argv + env. If
env is not provided, we inherit from QEMU. Currently we're only
providing argv, so I was wondering if we should allow env too.
Probably overkill, but at least having the 'MigrateCommand'
struct lets us add it later.
>
> > > +##
> > > +# @MigrateRdmaAddr:
> > > +#
> > > +# Since 8.0
> > > +##
> > > +{ 'struct': 'MigrateRdmaAddr',
> > > + 'data' : {'data': 'InetSocketAddress' } }
> > InetSocketAddress is a plain struct, so I think we can use
> > that directly, no ?
> Yes, we can use it directly. Just to keep consistency with other transport
> mechanisms, I made a separate struct even for rdma.
> > > +
> > > +##
> > > +# @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' } }
> > Ideally this would be
> >
> > 'data' : {
> > 'socket' : 'SocketAddress',
> > 'exec' : 'MigrateCommand',
> > 'rdma': 'InetSocketAddress' } }
> >
> > though the first SocketAddress isn't possible unless it is easy to
> > lift the QAPI limitation.
>
> Yes, I agree with you Daniel. If we can fix the first one - SocketAddress
> one, can we also allow ['str'] to also be directly represented by modifying
> QAPI ?
>
> ex: 'exec': ['str'] ... something like this ?
No, I think it is important to use a struct for 'exec' to allow
future expansion of parameters.
> > > +# -> { "execute": "migrate",
> > > +# "arguments": {
> > > +# "channel": { "channeltype": "main",
> > > +# "addr": { "transport": "socket",
> > > +# "socket-type": { "type': "inet',
> > > +# "host": "10.12.34.9",
> > > +# "port": "1050" } } }
> > > } }
> > > +# <- { "return": {} }
> > > +#
> > > +# -> { "execute": "migrate",
> > > +# "arguments": {
> > > +# "channel": { "channeltype": "main",
> > > +# "addr": { "transport": "exec",
> > > +# "exec": ["/bin/nc", "-U",
> > > +# "/some/sock" ] } } } }
> > > +# <- { "return": {} }
> > > +#
> > > +# -> { "execute": "migrate",
> > > +# "arguments": {
> > > +# "channel": { "channeltype": "main",
> > > +# "addr": { "transport": "rdma",
> > > +# "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', '*channel': 'MigrateChannel', '*blk': 'bool',
> > > + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
> > IIRC, the intention was to allow multiple channels to be set in a
> > follow up to this series. If so that would require adding yet another
> > field as an array of MigrateChannel. Should we just go for the
> > array straight away, and just limit it to 1 element as a runtime
> > check ? eg
> >
> > 'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool',
> > '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
> Yes, I got your point Daniel. But I feel it is better to introduce it in
> follow up series along with introducing different channel types (main, data,
> postcopy). It would then also make sense to introduce a list of
> 'MigrateChannel'.
Right, that means if we release QEMU 8.0.0 with the 'channel' parameter,
and your next series doesn't get merged until 8.1.0, we're stuck
supporting both 'channel' and 'channels'.
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 :|
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, (continued)
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Het Gala, 2023/02/09
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Daniel P . Berrangé, 2023/02/09
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Het Gala, 2023/02/09
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Daniel P . Berrangé, 2023/02/09
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Het Gala, 2023/02/10
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Markus Armbruster, 2023/02/10
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Markus Armbruster, 2023/02/09
- Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Het Gala, 2023/02/10
Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command, Daniel P . Berrangé, 2023/02/09
QAPI unions as branches / unifying struct and union types (was: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command), Markus Armbruster, 2023/02/10
Re: QAPI unions as branches / unifying struct and union types (was: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command), Het Gala, 2023/02/10
Re: QAPI unions as branches / unifying struct and union types, Markus Armbruster, 2023/02/14
Re: QAPI unions as branches / unifying struct and union types, Het Gala, 2023/02/17
Re: QAPI unions as branches / unifying struct and union types, Daniel P . Berrangé, 2023/02/17
Re: QAPI unions as branches / unifying struct and union types, Het Gala, 2023/02/21
[PATCH v2 6/6] migration: Established connection for listener sockets on the dest interface, Het Gala, 2023/02/08