|
From: | Het Gala |
Subject: | Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command |
Date: | Fri, 10 Feb 2023 13:54:11 +0530 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 |
On 09/02/23 6:52 pm, Daniel P. Berrangé wrote:
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.
Yes. 'MigrateExecCommand' seems more appropriate.
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.
Okay, now I get your point. Thanks for the explanation Daniel.
Yes, got your point of exec paramter expansion idea from env variable concept.+## +# @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.
Okay, understood. It might become messy. If we implement 'channels' right from start, we would just need to remove the check in the later series but still supporting 'channels' and unecessary does not need to include anything intermediate.+# -> { "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
Regards, Het Gala
[Prev in Thread] | Current Thread | [Next in Thread] |