| Message ID | 20230208093600.242665-3-het.gala@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Modified 'migrate' QAPI command for migration | expand |
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'.
On 09/02/23 1:47 am, Eric Blake 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. >> >> +## >> +# @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',... Yes, I wanted a suggestion here actually. Will 'data' instead of 'socket-type' be the right fit ? It will also be consistent with exec and rdma if changed to 'data'. >> + >> +## >> +# @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. Okay, thanks Eric for pointing it out. Will make sure I follow this going forward. >> + >> +## >> +# @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] Ack. In that case, I feel it would be better if I change from 'socket-type' to 'data' to keep consistency among the QAPI. >> + >> +## >> +# @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] Yes Eric. I did search if it is possible for a union inside a branch of union. That's the reason, I had to choose this path for 'socket' and 'rdma', and to keep consistency, I did the same with 'exec' too. >> + >> +## >> +# @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. Ack Eric. Thanks for pointing it out. Will take care about this small issues from next time. >> + >> +## >> +# @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. Yes, infact one of the branches MigrateChannel. Ack. >> +# >> +# 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. Ack. >> +# >> # 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. Yes Eric, excatly. >> +# <- { "return": {} } >> +# >> +# -> { "execute": "migrate", >> +# "arguments": { >> +# "channel": { "channeltype": "main", >> +# "addr": { "transport": "exec", >> +# "exec": ["/bin/nc", "-U", >> +# "/some/sock" ] } } } } > Another lopsided spacing in []. Ack. >> +# <- { "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'. Yes, instead of 'rdma' or 'exec', it should be replaced by 'data' here in the examples. Ack. > >> +# <- { "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'. Yes, 'uri':'str' is kept for backward compatibility right now. This will be depricated in later stages. Regards, Het Gala.
On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake 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'} } > > > > +## > > +# @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] I don't know the backstory on this limitation. Is it something that is very difficult to resolve ? I think it is highly desirable to have 'socket': 'SocketAddress' here. It would be a shame to introduce this better migration API design and then have it complicated by a possibly short term limitation of QAPI. With regards, Daniel
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' } } 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. > + > +## > +# @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 ? > +## > +# @MigrateRdmaAddr: > +# > +# Since 8.0 > +## > +{ 'struct': 'MigrateRdmaAddr', > + 'data' : {'data': 'InetSocketAddress' } } InetSocketAddress is a plain struct, so I think we can use that directly, no ? > + > +## > +# @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. > ## > # @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. > +# > # 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" } } } } } > +# <- { "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' } } With regards, Daniel
On 09/02/23 3:53 pm, Daniel P. Berrangé wrote: > On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake 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(-) >>> >>> + >>> +## >>> +# @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] > I don't know the backstory on this limitation. Is it something that > is very difficult to resolve ? I think it is highly desirable to have > 'socket': 'SocketAddress' here. It would be a shame to introduce this > better migration API design and then have it complicated by a possibly > short term limitation of QAPI. Agree Daniel. Making different struct just because we have a limitation for not able to have union as one of the branch of union, would have chances of increasing complexity. > With regards, > Daniel Regards, Het Gala
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 ? I did not get your question allowing setting environment variables. Could you explain it in more detail ? >> +## >> +# @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 ? >> +# -> { "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'. > With regards, > Daniel Regards, Het Gala
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
On Thu, Feb 09, 2023 at 10:23:43AM +0000, Daniel P. Berrangé wrote: > On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake 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'} } > > > > > > +## > > > +# @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] > > I don't know the backstory on this limitation. Is it something that > is very difficult to resolve ? I think it is highly desirable to have > 'socket': 'SocketAddress' here. It would be a shame to introduce this > better migration API design and then have it complicated by a possibly > short term limitation of QAPI. So to understand this better I did this change on top of Het's patch: diff --git a/qapi/migration.json b/qapi/migration.json index 79acfcfe4e..bbc3e66ad6 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1467,18 +1467,6 @@ { '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' } } - ## # @MigrateExecAddr: # @@ -1487,14 +1475,6 @@ { 'struct': 'MigrateExecAddr', 'data' : {'data': ['str'] } } -## -# @MigrateRdmaAddr: -# -# Since 8.0 -## -{ 'struct': 'MigrateRdmaAddr', - 'data' : {'data': 'InetSocketAddress' } } - ## # @MigrateAddress: # @@ -1506,9 +1486,9 @@ 'base' : { 'transport' : 'MigrateTransport'}, 'discriminator' : 'transport', 'data' : { - 'socket' : 'MigrateSocketAddr', + 'socket' : 'SocketAddress', 'exec' : 'MigrateExecAddr', - 'rdma': 'MigrateRdmaAddr' } } + 'rdma': 'InetSocketAddress' } } ## # @MigrateChannelType: That results in a build error /home/berrange/src/virt/qemu/scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79: ../qapi/migration.json: In union 'MigrateAddress': ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress' To understand what the limitation of QAPI generation is, I blindly disabled this check diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index cd8661125c..cb5c0385bd 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -653,7 +653,7 @@ def check(self, schema, seen): "branch '%s' is not a value of %s" % (v.name, self.tag_member.type.describe())) if (not isinstance(v.type, QAPISchemaObjectType) - or v.type.variants): + or v.type.variants) and False: raise QAPISemError( self.info, "%s cannot use %s" @@ -664,7 +664,8 @@ def check_clash(self, info, seen): for v in self.variants: # Reset seen map for each variant, since qapi names from one # branch do not affect another branch - v.type.check_clash(info, dict(seen)) + #v.type.check_clash(info, dict(seen)) + pass class QAPISchemaMember: After doing that, the QAPI code generated handled the union-inside-union case without any problems that I can see. The generated code looks sane and it compiles correctly. So is this actually just a case of overly strict input validation in the QAPI schema parser ? If so, what's the correct way to adapt the checks to permit this usage. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake wrote: [...] >> 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] > > I don't know the backstory on this limitation. Is it something that > is very difficult to resolve ? I think it is highly desirable to have > 'socket': 'SocketAddress' here. It would be a shame to introduce this > better migration API design and then have it complicated by a possibly > short term limitation of QAPI. We evolve the QAPI language to satisfy concrete use cases. If you could use a language improvement, make a case for it, and we'll see what we can do within a time frame that works for you. Better than ugly work-arounds on the silent assumption the language cannot be adapted.
On 09/02/23 9:56 pm, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake wrote: > [...] > >>> 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] >> I don't know the backstory on this limitation. Is it something that >> is very difficult to resolve ? I think it is highly desirable to have >> 'socket': 'SocketAddress' here. It would be a shame to introduce this >> better migration API design and then have it complicated by a possibly >> short term limitation of QAPI. > We evolve the QAPI language to satisfy concrete use cases. If you could > use a language improvement, make a case for it, and we'll see what we > can do within a time frame that works for you. Better than ugly > work-arounds on the silent assumption the language cannot be adapted. Hi Markus, Daniel has already left some comments on version 2, patch 2. In short, we want to make a union, where some of the branches of that union can call directly to another known union like 'SocketAddress'. Right now, QAPI does not allow to directly call a union inside a branch of union, and need to make a spearate struct, and then call a union again, and increases complexity of QAPI design which is contrary to what we aim for. We can further discuss on the response Markus has given in the latest v2 patch 2, and aim to, how we can evolve QAPI languge and reduce complexity :) Regards, Het Gala
On 09/02/23 7:08 pm, Daniel P. Berrangé wrote: > On Thu, Feb 09, 2023 at 10:23:43AM +0000, Daniel P. Berrangé wrote: >> On Wed, Feb 08, 2023 at 02:17:12PM -0600, Eric Blake 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'} } >>>> >>>> +## >>>> +# @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] >> I don't know the backstory on this limitation. Is it something that >> is very difficult to resolve ? I think it is highly desirable to have >> 'socket': 'SocketAddress' here. It would be a shame to introduce this >> better migration API design and then have it complicated by a possibly >> short term limitation of QAPI. > So to understand this better I did this change on top of Het's > patch: > > diff --git a/qapi/migration.json b/qapi/migration.json > index 79acfcfe4e..bbc3e66ad6 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1467,18 +1467,6 @@ > { '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' } } > - > ## > # @MigrateExecAddr: > # > @@ -1487,14 +1475,6 @@ > { 'struct': 'MigrateExecAddr', > 'data' : {'data': ['str'] } } > > -## > -# @MigrateRdmaAddr: > -# > -# Since 8.0 > -## > -{ 'struct': 'MigrateRdmaAddr', > - 'data' : {'data': 'InetSocketAddress' } } > - > ## > # @MigrateAddress: > # > @@ -1506,9 +1486,9 @@ > 'base' : { 'transport' : 'MigrateTransport'}, > 'discriminator' : 'transport', > 'data' : { > - 'socket' : 'MigrateSocketAddr', > + 'socket' : 'SocketAddress', > 'exec' : 'MigrateExecAddr', > - 'rdma': 'MigrateRdmaAddr' } } > + 'rdma': 'InetSocketAddress' } } > > ## > # @MigrateChannelType: > > > That results in a build error > > /home/berrange/src/virt/qemu/scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79: > ../qapi/migration.json: In union 'MigrateAddress': > ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress' > > To understand what the limitation of QAPI generation is, I blindly > disabled this check > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index cd8661125c..cb5c0385bd 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -653,7 +653,7 @@ def check(self, schema, seen): > "branch '%s' is not a value of %s" > % (v.name, self.tag_member.type.describe())) > if (not isinstance(v.type, QAPISchemaObjectType) > - or v.type.variants): > + or v.type.variants) and False: > raise QAPISemError( > self.info, > "%s cannot use %s" > @@ -664,7 +664,8 @@ def check_clash(self, info, seen): > for v in self.variants: > # Reset seen map for each variant, since qapi names from one > # branch do not affect another branch > - v.type.check_clash(info, dict(seen)) > + #v.type.check_clash(info, dict(seen)) > + pass > > > class QAPISchemaMember: > > > After doing that, the QAPI code generated handled the union-inside-union > case without any problems that I can see. The generated code looks sane > and it compiles correctly. > > So is this actually just a case of overly strict input validation in > the QAPI schema parser ? If so, what's the correct way to adapt the > checks to permit this usage. Could we take advice of Markus and other QAPI maintainers here and improve QAPI language here. > With regards, > Daniel Regards, Het Gala
Daniel P. Berrangé <berrange@redhat.com> writes: [...] >> +## >> +# @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. Context: SocketAddress is a QAPI union, and "the QAPI limitation" is scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79: ../qapi/migration.json: In union 'MigrateAddress': ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress' Emitted by schema.py like this: if (not isinstance(v.type, QAPISchemaObjectType) or v.type.variants): raise QAPISemError( self.info, "%s cannot use %s" % (v.describe(self.info), v.type.describe())) This enforces docs/devel/qapi-code-gen.rst's clause The BRANCH's value defines the branch's properties, in particular its type. The type must a struct type. [...] Next paragraph: In the Client JSON Protocol, a union is represented by an object with the common members (from the base type) and the selected branch's members. The two sets of member names must be disjoint. So, we're splicing in the members of the branch's JSON object. For that to even make sense, the branch type needs to map to a JSON object. This is fundamental. It's the first part of the condition in the code snippet above. We have two kinds of QAPI types that map to a JSON object: struct and union. The second part of the condition restricts to struct. Unless I'm missing something (imperfect memory...), this is *not* fundamental, just a matter of implementing it. But I'd have to try to be sure. Instead of simply allowing unions in addition to structs here, I'd like to go one step further, and fuse the two into "objects". Let me explain. If we abstract from syntax, structs have become almost a special kind of union. Unions have a set of common members and sets of variant members, and a special common member (the tag) selects the set of variant members. Structs are unions with zero variants and no tag. The generator code actually represents both structs and unions as a common QAPISchemaObjectType already. QAPI/QMP introspection does the same: it uses a single meta type 'object' for both. There is another spot where only structs are allowed: a struct or union's base type. That restriction will be awkward to lift, as I made the mistake of baking the assumption "object type has at most one tag member" into QAPI/QMP introspection . [...]
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. >>>> +## >>>> +# @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. Yes, got your point of exec paramter expansion idea from env variable concept. >>>> +# -> { "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'. 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. > With regards, > Daniel Regards, Het Gala
Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Feb 09, 2023 at 10:23:43AM +0000, Daniel P. Berrangé wrote: [...] >> I don't know the backstory on this limitation. Is it something that >> is very difficult to resolve ? I think it is highly desirable to have >> 'socket': 'SocketAddress' here. It would be a shame to introduce this >> better migration API design and then have it complicated by a possibly >> short term limitation of QAPI. > > So to understand this better I did this change on top of Het's > patch: > > diff --git a/qapi/migration.json b/qapi/migration.json > index 79acfcfe4e..bbc3e66ad6 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1467,18 +1467,6 @@ > { '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' } } > - > ## > # @MigrateExecAddr: > # > @@ -1487,14 +1475,6 @@ > { 'struct': 'MigrateExecAddr', > 'data' : {'data': ['str'] } } > > -## > -# @MigrateRdmaAddr: > -# > -# Since 8.0 > -## > -{ 'struct': 'MigrateRdmaAddr', > - 'data' : {'data': 'InetSocketAddress' } } > - > ## > # @MigrateAddress: > # > @@ -1506,9 +1486,9 @@ > 'base' : { 'transport' : 'MigrateTransport'}, > 'discriminator' : 'transport', > 'data' : { > - 'socket' : 'MigrateSocketAddr', > + 'socket' : 'SocketAddress', > 'exec' : 'MigrateExecAddr', > - 'rdma': 'MigrateRdmaAddr' } } > + 'rdma': 'InetSocketAddress' } } > > ## > # @MigrateChannelType: > > > That results in a build error > > /home/berrange/src/virt/qemu/scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79: > ../qapi/migration.json: In union 'MigrateAddress': > ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress' > > To understand what the limitation of QAPI generation is, I blindly > disabled this check > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index cd8661125c..cb5c0385bd 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -653,7 +653,7 @@ def check(self, schema, seen): > "branch '%s' is not a value of %s" > % (v.name, self.tag_member.type.describe())) > if (not isinstance(v.type, QAPISchemaObjectType) > - or v.type.variants): > + or v.type.variants) and False: > raise QAPISemError( > self.info, > "%s cannot use %s" > @@ -664,7 +664,8 @@ def check_clash(self, info, seen): > for v in self.variants: > # Reset seen map for each variant, since qapi names from one > # branch do not affect another branch > - v.type.check_clash(info, dict(seen)) > + #v.type.check_clash(info, dict(seen)) > + pass > > > class QAPISchemaMember: > > > After doing that, the QAPI code generated handled the union-inside-union > case without any problems that I can see. The generated code looks sane > and it compiles correctly. As you discovered, the code was designed to treat structs and unions the same. But there are holes. > So is this actually just a case of overly strict input validation in > the QAPI schema parser ? If so, what's the correct way to adapt the > checks to permit this usage. The check you disabled guards holes. There's at least this one in QAPISchemaObjectType.check_clash(): # Check that the members of this type do not cause duplicate JSON members, # and update seen to track the members seen so far. Report any errors # on behalf of info, which is not necessarily self.info def check_clash(self, info, seen): assert self._checked assert not self.variants # not implemented for m in self.members: m.check_clash(info, seen)
On 10/02/23 12:54 pm, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > [...] > >>> +## >>> +# @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. > Context: SocketAddress is a QAPI union, and "the QAPI limitation" is > > scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79: > ../qapi/migration.json: In union 'MigrateAddress': > ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress' > > Emitted by schema.py like this: > > if (not isinstance(v.type, QAPISchemaObjectType) > or v.type.variants): > raise QAPISemError( > self.info, > "%s cannot use %s" > % (v.describe(self.info), v.type.describe())) > > This enforces docs/devel/qapi-code-gen.rst's clause > > The BRANCH's value defines the branch's properties, in particular its > type. The type must a struct type. [...] > > Next paragraph: > > In the Client JSON Protocol, a union is represented by an object with > the common members (from the base type) and the selected branch's > members. The two sets of member names must be disjoint. > > So, we're splicing in the members of the branch's JSON object. For that > to even make sense, the branch type needs to map to a JSON object. This > is fundamental. It's the first part of the condition in the code > snippet above. > > We have two kinds of QAPI types that map to a JSON object: struct and > union. The second part of the condition restricts to struct. Unless > I'm missing something (imperfect memory...), this is *not* fundamental, > just a matter of implementing it. But I'd have to try to be sure. > > > Instead of simply allowing unions in addition to structs here, I'd like > to go one step further, and fuse the two into "objects". Let me > explain. > > If we abstract from syntax, structs have become almost a special kind of > union. Unions have a set of common members and sets of variant members, > and a special common member (the tag) selects the set of variant > members. Structs are unions with zero variants and no tag. > > The generator code actually represents both structs and unions as a > common QAPISchemaObjectType already. QAPI/QMP introspection does the > same: it uses a single meta type 'object' for both. > > > There is another spot where only structs are allowed: a struct or > union's base type. That restriction will be awkward to lift, as I made > the mistake of baking the assumption "object type has at most one tag > member" into QAPI/QMP introspection . Hi Markus, thankyou for explaning in such detail. I tried to understand of what you explained. So IIUC, you mentioned the QAPI generator treats both structs and unions same, but basically in the schema.py checks is where it tries to distinguish between the two ? and because of the fact that docs/devel/qapi-code-gen.rst states that for a union, it's branches must be 'struct', and that's the reason it gives an error ? If that's the case, can we improve on our checks and allow union as a part of branch of a union ? or something else ? or I may have completely misunderstood most of the part 😅. Please let me know > > [...] Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 10/02/23 12:54 pm, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> [...] >> >>>> +## >>>> +# @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. >> >> Context: SocketAddress is a QAPI union, and "the QAPI limitation" is >> >> scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79: >> ../qapi/migration.json: In union 'MigrateAddress': >> ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress' >> >> Emitted by schema.py like this: >> >> if (not isinstance(v.type, QAPISchemaObjectType) >> or v.type.variants): >> raise QAPISemError( >> self.info, >> "%s cannot use %s" >> % (v.describe(self.info), v.type.describe())) >> >> This enforces docs/devel/qapi-code-gen.rst's clause >> >> The BRANCH's value defines the branch's properties, in particular its >> type. The type must a struct type. [...] >> >> Next paragraph: >> >> In the Client JSON Protocol, a union is represented by an object with >> the common members (from the base type) and the selected branch's >> members. The two sets of member names must be disjoint. >> >> So, we're splicing in the members of the branch's JSON object. For that >> to even make sense, the branch type needs to map to a JSON object. This >> is fundamental. It's the first part of the condition in the code >> snippet above. >> >> We have two kinds of QAPI types that map to a JSON object: struct and >> union. The second part of the condition restricts to struct. Unless >> I'm missing something (imperfect memory...), this is *not* fundamental, >> just a matter of implementing it. But I'd have to try to be sure. >> >> >> Instead of simply allowing unions in addition to structs here, I'd like >> to go one step further, and fuse the two into "objects". Let me >> explain. >> >> If we abstract from syntax, structs have become almost a special kind of >> union. Unions have a set of common members and sets of variant members, >> and a special common member (the tag) selects the set of variant >> members. Structs are unions with zero variants and no tag. >> >> The generator code actually represents both structs and unions as a >> common QAPISchemaObjectType already. QAPI/QMP introspection does the >> same: it uses a single meta type 'object' for both. >> >> >> There is another spot where only structs are allowed: a struct or >> union's base type. That restriction will be awkward to lift, as I made >> the mistake of baking the assumption "object type has at most one tag >> member" into QAPI/QMP introspection . > > Hi Markus, thankyou for explaning in such detail. I tried to understand of what you explained. > > So IIUC, you mentioned the QAPI generator treats both structs and unions same, but basically in the schema.py checks is where it tries to distinguish between the two ? and because of the fact that docs/devel/qapi-code-gen.rst states that for a union, it's branches must be 'struct', and that's the reason it gives an error ? Permit me a brief digression into history. The initial QAPI design language provided product types (structs) and sum types (unions containing exactly one of several types, and a tag member that tells which one). The two are orthogonal. These unions turned out rather awkward. The unions we have today are more general. They have common members, and one of them is the tag member, of enumeration type. For each tag value, they have variant members. Both the common members and each tag value's variant members are given as struct types. What if the tag's enumeration type is empty, i.e. has no values? We get a union with no variant members, only common ones. Isn't that a struct? Not quite. To get a struct, we also have to drop the tag member. It has no possible values anyway. You see, struct types are almost a special case of today's union types. To overcome "almost", we can introduce the notion of "object type": * An object type has common members, one of them can be a tag member, of enumeration type, not empty. For each tag value, it additionally has variant members. * A union type is an object type with a tag member and variant members. * A struct type is an object type without tag member and variant members. The QAPI generator code already made the jump to this object type notion. It transform the special cases into the general case at first opportunity, in QAPISchema._def_struct_type() and ._def_union_type(). *Except* we haven't implemented support for variant members in a few places where they cannot occur now, e.g. as a tag value's variant. This is the restriction you ran into. I'd like to make the jump to object type in the QAPI schema language, too. But that's not a prerequisite to lifting the restriction. > If that's the case, can we improve on our checks and allow union as a part of branch of a union ? or something else ? I believe we can implement the missing parts and relax the checks. But to be sure, we need to try. > or I may have completely misunderstood most of the part 😅. Please let me know More questions?
On 14/02/23 3:46 pm, Markus Armbruster wrote: > Het Gala <het.gala@nutanix.com> writes: > >> On 10/02/23 12:54 pm, Markus Armbruster wrote: >>> Daniel P. Berrangé <berrange@redhat.com> writes: >>> >>> [...] >>> >>>>> +## >>>>> +# @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. >>> Context: SocketAddress is a QAPI union, and "the QAPI limitation" is >>> >>> scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79: >>> ../qapi/migration.json: In union 'MigrateAddress': >>> ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress' >>> >>> Emitted by schema.py like this: >>> >>> if (not isinstance(v.type, QAPISchemaObjectType) >>> or v.type.variants): >>> raise QAPISemError( >>> self.info, >>> "%s cannot use %s" >>> % (v.describe(self.info), v.type.describe())) >>> >>> This enforces docs/devel/qapi-code-gen.rst's clause >>> >>> The BRANCH's value defines the branch's properties, in particular its >>> type. The type must a struct type. [...] >>> >>> Next paragraph: >>> >>> In the Client JSON Protocol, a union is represented by an object with >>> the common members (from the base type) and the selected branch's >>> members. The two sets of member names must be disjoint. >>> >>> So, we're splicing in the members of the branch's JSON object. For that >>> to even make sense, the branch type needs to map to a JSON object. This >>> is fundamental. It's the first part of the condition in the code >>> snippet above. >>> >>> We have two kinds of QAPI types that map to a JSON object: struct and >>> union. The second part of the condition restricts to struct. Unless >>> I'm missing something (imperfect memory...), this is *not* fundamental, >>> just a matter of implementing it. But I'd have to try to be sure. >>> >>> >>> Instead of simply allowing unions in addition to structs here, I'd like >>> to go one step further, and fuse the two into "objects". Let me >>> explain. >>> >>> If we abstract from syntax, structs have become almost a special kind of >>> union. Unions have a set of common members and sets of variant members, >>> and a special common member (the tag) selects the set of variant >>> members. Structs are unions with zero variants and no tag. >>> >>> The generator code actually represents both structs and unions as a >>> common QAPISchemaObjectType already. QAPI/QMP introspection does the >>> same: it uses a single meta type 'object' for both. >>> >>> >>> There is another spot where only structs are allowed: a struct or >>> union's base type. That restriction will be awkward to lift, as I made >>> the mistake of baking the assumption "object type has at most one tag >>> member" into QAPI/QMP introspection . >> Hi Markus, thankyou for explaning in such detail. I tried to understand of what you explained. >> >> So IIUC, you mentioned the QAPI generator treats both structs and unions same, but basically in the schema.py checks is where it tries to distinguish between the two ? and because of the fact that docs/devel/qapi-code-gen.rst states that for a union, it's branches must be 'struct', and that's the reason it gives an error ? > Permit me a brief digression into history. > > The initial QAPI design language provided product types (structs) and > sum types (unions containing exactly one of several types, and a tag > member that tells which one). The two are orthogonal. > > These unions turned out rather awkward. > > The unions we have today are more general. They have common members, > and one of them is the tag member, of enumeration type. For each tag > value, they have variant members. Both the common members and each tag > value's variant members are given as struct types. > > What if the tag's enumeration type is empty, i.e. has no values? We get > a union with no variant members, only common ones. Isn't that a struct? > > Not quite. To get a struct, we also have to drop the tag member. It > has no possible values anyway. > > You see, struct types are almost a special case of today's union types. > To overcome "almost", we can introduce the notion of "object type": > > * An object type has common members, one of them can be a tag member, of > enumeration type, not empty. For each tag value, it additionally has > variant members. > > * A union type is an object type with a tag member and variant members. > > * A struct type is an object type without tag member and variant > members. > > The QAPI generator code already made the jump to this object type > notion. It transform the special cases into the general case at first > opportunity, in QAPISchema._def_struct_type() and ._def_union_type(). > > *Except* we haven't implemented support for variant members in a few > places where they cannot occur now, e.g. as a tag value's variant. This > is the restriction you ran into. > > I'd like to make the jump to object type in the QAPI schema language, > too. But that's not a prerequisite to lifting the restriction. > >> If that's the case, can we improve on our checks and allow union as a part of branch of a union ? or something else ? > I believe we can implement the missing parts and relax the checks. But > to be sure, we need to try. > >> or I may have completely misunderstood most of the part 😅. Please let me know > More questions? Completely understood everything. Thankyou for the wonderful explanation. Looking forward to implement the missing parts in QAPI schema language. Regards, Het Gala
On Fri, Feb 17, 2023 at 04:48:59PM +0530, Het Gala wrote: > > On 14/02/23 3:46 pm, Markus Armbruster wrote: > > Het Gala <het.gala@nutanix.com> writes: > > > > > On 10/02/23 12:54 pm, Markus Armbruster wrote: > > > > Daniel P. Berrangé <berrange@redhat.com> writes: > > > > > > > > [...] > > > > > > > > > > +## > > > > > > +# @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. > > > > Context: SocketAddress is a QAPI union, and "the QAPI limitation" is > > > > > > > > scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79: > > > > ../qapi/migration.json: In union 'MigrateAddress': > > > > ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress' > > > > > > > > Emitted by schema.py like this: > > > > > > > > if (not isinstance(v.type, QAPISchemaObjectType) > > > > or v.type.variants): > > > > raise QAPISemError( > > > > self.info, > > > > "%s cannot use %s" > > > > % (v.describe(self.info), v.type.describe())) > > > > > > > > This enforces docs/devel/qapi-code-gen.rst's clause > > > > > > > > The BRANCH's value defines the branch's properties, in particular its > > > > type. The type must a struct type. [...] > > > > > > > > Next paragraph: > > > > > > > > In the Client JSON Protocol, a union is represented by an object with > > > > the common members (from the base type) and the selected branch's > > > > members. The two sets of member names must be disjoint. > > > > > > > > So, we're splicing in the members of the branch's JSON object. For that > > > > to even make sense, the branch type needs to map to a JSON object. This > > > > is fundamental. It's the first part of the condition in the code > > > > snippet above. > > > > > > > > We have two kinds of QAPI types that map to a JSON object: struct and > > > > union. The second part of the condition restricts to struct. Unless > > > > I'm missing something (imperfect memory...), this is *not* fundamental, > > > > just a matter of implementing it. But I'd have to try to be sure. > > > > > > > > > > > > Instead of simply allowing unions in addition to structs here, I'd like > > > > to go one step further, and fuse the two into "objects". Let me > > > > explain. > > > > > > > > If we abstract from syntax, structs have become almost a special kind of > > > > union. Unions have a set of common members and sets of variant members, > > > > and a special common member (the tag) selects the set of variant > > > > members. Structs are unions with zero variants and no tag. > > > > > > > > The generator code actually represents both structs and unions as a > > > > common QAPISchemaObjectType already. QAPI/QMP introspection does the > > > > same: it uses a single meta type 'object' for both. > > > > > > > > > > > > There is another spot where only structs are allowed: a struct or > > > > union's base type. That restriction will be awkward to lift, as I made > > > > the mistake of baking the assumption "object type has at most one tag > > > > member" into QAPI/QMP introspection . > > > Hi Markus, thankyou for explaning in such detail. I tried to understand of what you explained. > > > > > > So IIUC, you mentioned the QAPI generator treats both structs and unions same, but basically in the schema.py checks is where it tries to distinguish between the two ? and because of the fact that docs/devel/qapi-code-gen.rst states that for a union, it's branches must be 'struct', and that's the reason it gives an error ? > > Permit me a brief digression into history. > > > > The initial QAPI design language provided product types (structs) and > > sum types (unions containing exactly one of several types, and a tag > > member that tells which one). The two are orthogonal. > > > > These unions turned out rather awkward. > > > > The unions we have today are more general. They have common members, > > and one of them is the tag member, of enumeration type. For each tag > > value, they have variant members. Both the common members and each tag > > value's variant members are given as struct types. > > > > What if the tag's enumeration type is empty, i.e. has no values? We get > > a union with no variant members, only common ones. Isn't that a struct? > > > > Not quite. To get a struct, we also have to drop the tag member. It > > has no possible values anyway. > > > > You see, struct types are almost a special case of today's union types. > > To overcome "almost", we can introduce the notion of "object type": > > > > * An object type has common members, one of them can be a tag member, of > > enumeration type, not empty. For each tag value, it additionally has > > variant members. > > > > * A union type is an object type with a tag member and variant members. > > > > * A struct type is an object type without tag member and variant > > members. > > > > The QAPI generator code already made the jump to this object type > > notion. It transform the special cases into the general case at first > > opportunity, in QAPISchema._def_struct_type() and ._def_union_type(). > > > > *Except* we haven't implemented support for variant members in a few > > places where they cannot occur now, e.g. as a tag value's variant. This > > is the restriction you ran into. > > > > I'd like to make the jump to object type in the QAPI schema language, > > too. But that's not a prerequisite to lifting the restriction. > > > > > If that's the case, can we improve on our checks and allow union as a part of branch of a union ? or something else ? > > I believe we can implement the missing parts and relax the checks. But > > to be sure, we need to try. > > > > > or I may have completely misunderstood most of the part 😅. Please let me know > > More questions? > > Completely understood everything. Thankyou for the wonderful explanation. > Looking forward to implement the missing parts in QAPI schema language. I cc'd you on a patch that implements this missing feature a couple of days ago, and its on Markus' review todo list. So we should be able to decide how to move forward sometime next week. With regards, Daniel
On 17/02/23 5:25 pm, Daniel P. Berrangé wrote: > On Fri, Feb 17, 2023 at 04:48:59PM +0530, Het Gala wrote: >> On 14/02/23 3:46 pm, Markus Armbruster wrote: >>> Het Gala<het.gala@nutanix.com> writes: >>> >>>> On 10/02/23 12:54 pm, Markus Armbruster wrote: >>>>> Daniel P. Berrangé<berrange@redhat.com> writes: >>>>> >>>>> [...] >>>>> >>>>>>> +## >>>>>>> +# @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. >>>>> Context: SocketAddress is a QAPI union, and "the QAPI limitation" is >>>>> >>>>> scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:79: >>>>> ../qapi/migration.json: In union 'MigrateAddress': >>>>> ../qapi/migration.json:1505: branch 'socket' cannot use union type 'SocketAddress' >>>>> >>>>> Emitted by schema.py like this: >>>>> >>>>> if (not isinstance(v.type, QAPISchemaObjectType) >>>>> or v.type.variants): >>>>> raise QAPISemError( >>>>> self.info, >>>>> "%s cannot use %s" >>>>> % (v.describe(self.info), v.type.describe())) >>>>> >>>>> This enforces docs/devel/qapi-code-gen.rst's clause >>>>> >>>>> The BRANCH's value defines the branch's properties, in particular its >>>>> type. The type must a struct type. [...] >>>>> >>>>> Next paragraph: >>>>> >>>>> In the Client JSON Protocol, a union is represented by an object with >>>>> the common members (from the base type) and the selected branch's >>>>> members. The two sets of member names must be disjoint. >>>>> >>>>> So, we're splicing in the members of the branch's JSON object. For that >>>>> to even make sense, the branch type needs to map to a JSON object. This >>>>> is fundamental. It's the first part of the condition in the code >>>>> snippet above. >>>>> >>>>> We have two kinds of QAPI types that map to a JSON object: struct and >>>>> union. The second part of the condition restricts to struct. Unless >>>>> I'm missing something (imperfect memory...), this is *not* fundamental, >>>>> just a matter of implementing it. But I'd have to try to be sure. >>>>> >>>>> >>>>> Instead of simply allowing unions in addition to structs here, I'd like >>>>> to go one step further, and fuse the two into "objects". Let me >>>>> explain. >>>>> >>>>> If we abstract from syntax, structs have become almost a special kind of >>>>> union. Unions have a set of common members and sets of variant members, >>>>> and a special common member (the tag) selects the set of variant >>>>> members. Structs are unions with zero variants and no tag. >>>>> >>>>> The generator code actually represents both structs and unions as a >>>>> common QAPISchemaObjectType already. QAPI/QMP introspection does the >>>>> same: it uses a single meta type 'object' for both. >>>>> >>>>> >>>>> There is another spot where only structs are allowed: a struct or >>>>> union's base type. That restriction will be awkward to lift, as I made >>>>> the mistake of baking the assumption "object type has at most one tag >>>>> member" into QAPI/QMP introspection . >>>> Hi Markus, thankyou for explaning in such detail. I tried to understand of what you explained. >>>> >>>> So IIUC, you mentioned the QAPI generator treats both structs and unions same, but basically in the schema.py checks is where it tries to distinguish between the two ? and because of the fact that docs/devel/qapi-code-gen.rst states that for a union, it's branches must be 'struct', and that's the reason it gives an error ? >>> Permit me a brief digression into history. >>> >>> The initial QAPI design language provided product types (structs) and >>> sum types (unions containing exactly one of several types, and a tag >>> member that tells which one). The two are orthogonal. >>> >>> These unions turned out rather awkward. >>> >>> The unions we have today are more general. They have common members, >>> and one of them is the tag member, of enumeration type. For each tag >>> value, they have variant members. Both the common members and each tag >>> value's variant members are given as struct types. >>> >>> What if the tag's enumeration type is empty, i.e. has no values? We get >>> a union with no variant members, only common ones. Isn't that a struct? >>> >>> Not quite. To get a struct, we also have to drop the tag member. It >>> has no possible values anyway. >>> >>> You see, struct types are almost a special case of today's union types. >>> To overcome "almost", we can introduce the notion of "object type": >>> >>> * An object type has common members, one of them can be a tag member, of >>> enumeration type, not empty. For each tag value, it additionally has >>> variant members. >>> >>> * A union type is an object type with a tag member and variant members. >>> >>> * A struct type is an object type without tag member and variant >>> members. >>> >>> The QAPI generator code already made the jump to this object type >>> notion. It transform the special cases into the general case at first >>> opportunity, in QAPISchema._def_struct_type() and ._def_union_type(). >>> >>> *Except* we haven't implemented support for variant members in a few >>> places where they cannot occur now, e.g. as a tag value's variant. This >>> is the restriction you ran into. >>> >>> I'd like to make the jump to object type in the QAPI schema language, >>> too. But that's not a prerequisite to lifting the restriction. >>> >>>> If that's the case, can we improve on our checks and allow union as a part of branch of a union ? or something else ? >>> I believe we can implement the missing parts and relax the checks. But >>> to be sure, we need to try. >>> >>>> or I may have completely misunderstood most of the part 😅. Please let me know >>> More questions? >> Completely understood everything. Thankyou for the wonderful explanation. >> Looking forward to implement the missing parts in QAPI schema language. > I cc'd you on a patch that implements this missing feature a couple > of days ago, and its on Markus' review todo list. So we should be > able to decide how to move forward sometime next week. I did put Daniel's qapi patch and on top, added my patch where I introduce 'MigrateAddress' in 'migrate' QAPI, and assigned 'SocketAddress' union to 'socket' backend transport inside 'MigrateAddress' union. So iow, union inside union worked smoothly, and was able to re-architecture the patchset series accordingly. > With regards, > Daniel Regards, Het Gala
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' } } + +## +# @MigrateExecAddr: + # + # Since 8.0 + ## +{ 'struct': 'MigrateExecAddr', + 'data' : {'data': ['str'] } } + +## +# @MigrateRdmaAddr: +# +# Since 8.0 +## +{ 'struct': 'MigrateRdmaAddr', + 'data' : {'data': 'InetSocketAddress' } } + +## +# @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' } } + +## +# @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'] } + +## +# @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 +# +# 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. +# # 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" } } } } } +# <- { "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' } } ## # @migrate-incoming:
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(-)