| Message ID | 20221226053329.157905-2-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Modified 'migrate' QAPI command for migration | expand |
On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote: > From: Author Het Gala <het.gala@nutanix.com> > > 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. This scheme does seem to have an issue > in it, i.e. double-level encoding of URIs. > > The current patch maps existing QAPI design into a well-defined data > structure - 'MigrateChannel' only from the design perspective. Please note that > the existing 'uri' parameter is kept untouched 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 | 121 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 119 insertions(+), 2 deletions(-) > diff --git a/qapi/migration.json b/qapi/migration.json > index 88ecf86ac8..753e187ce2 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' : {'exec-str': 'str' } } Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command that is passed const char *argv[] = { "/bin/sh", "-c", command, NULL }; I have a strong preference for making it possible to avoid use of shell when spawning commands, since much of thue time it is not required and has the potential to open up vulnerabilities. It would be nice to be able to just take the full argv directly IOW { 'struct': 'MigrateExecAddr', 'data' : {'argv': ['str'] } } If the caller wants to keep life safe and simple now they can use ["/bin/nc", "-U", "/some/sock"] but if they still want to send it via shell, they can also do so ["/bin/sh", "-c", "...arbitrary shell script code...."] > + > +## > +# @MigrateRdmaAddr: > +# > +# Since 8.0 > +## > +{ 'struct': 'MigrateRdmaAddr', > + 'data' : {'rdma-str': 'str' } } Loooking at the RDMA code it takes the str, and treats it as an IPv4 address: addr = g_new(InetSocketAddress, 1); if (!inet_parse(addr, host_port, NULL)) { rdma->port = atoi(addr->port); rdma->host = g_strdup(addr->host); rdma->host_port = g_strdup(host_port); } so we really ought to accept an InetSocketAddress struct directly { 'struct': 'MigrateRdmaAddr', 'data' : {'rdma-str': '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,36 @@ > # 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 mututally exclusive but, atleast Minor typos "mutually" "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-str": "/tmp/exec" } } } } Will need updating given my feedback above > +# <- { "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' } } With regards, Daniel
On 09/01/23 7:37 pm, Daniel P. Berrangé wrote: > On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote: >> From: Author Het Gala <het.gala@nutanix.com> >> >> 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. This scheme does seem to have an issue >> in it, i.e. double-level encoding of URIs. >> >> The current patch maps existing QAPI design into a well-defined data >> structure - 'MigrateChannel' only from the design perspective. Please note that >> the existing 'uri' parameter is kept untouched for backward compatibility. >> >> +## >> +# @MigrateExecAddr: >> + # >> + # Since 8.0 >> + ## >> +{ 'struct': 'MigrateExecAddr', >> + 'data' : {'exec-str': 'str' } } > Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command > that is passed > > const char *argv[] = { "/bin/sh", "-c", command, NULL }; > > I have a strong preference for making it possible to avoid use > of shell when spawning commands, since much of thue time it is > not required and has the potential to open up vulnerabilities. > It would be nice to be able to just take the full argv directly > IOW > > { 'struct': 'MigrateExecAddr', > 'data' : {'argv': ['str'] } } > > If the caller wants to keep life safe and simple now they can > use > ["/bin/nc", "-U", "/some/sock"] > > but if they still want to send it via shell, they can also do > so > > ["/bin/sh", "-c", "...arbitrary shell script code...."] Okay Daniel. I get your point and it makes sense too. This will also have some code changes in exec.c file I assume. >> + >> +## >> +# @MigrateRdmaAddr: >> +# >> +# Since 8.0 >> +## >> +{ 'struct': 'MigrateRdmaAddr', >> + 'data' : {'rdma-str': 'str' } } > Loooking at the RDMA code it takes the str, and treats it > as an IPv4 address: > > > addr = g_new(InetSocketAddress, 1); > if (!inet_parse(addr, host_port, NULL)) { > rdma->port = atoi(addr->port); > rdma->host = g_strdup(addr->host); > rdma->host_port = g_strdup(host_port); > } > > so we really ought to accept an InetSocketAddress struct > directly > > { 'struct': 'MigrateRdmaAddr', > 'data' : {'rdma-str': 'InetSocketAddress' } } > Yes, It resembles to InetSocketAddress. Will make the relevant changes in rdma.c file. With this, I had a small question in mind, do qemu need to develop / leverage some functionality to check the correctness for host or port. So that if the user enters an invalid host address, they get an error message to enter correct address, if trying to migrate via qmp command line interface. Please correct me if I am wrong here. > along with all > +# the details of destination interface required for initiating > +# a migration stream. > # > # @blk: do block migration (full disk copy) > # > @@ -1479,15 +1575,36 @@ > # 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 mututally exclusive but, atleast > > Minor typos "mutually" "at least" Thanks for pointing that out Daniel. 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" } } } } } > +# <- { "return": {} } > +# > +# -> { "execute": "migrate", > +# "arguments": { "channel": { "channeltype": "main", > +# "addr": { "transport": "exec", > +# "exec-str": "/tmp/exec" } } } } > Will need updating given my feedback above Yeah sure.Thanks for the feedback above. > With regards, > Daniel Regards, Het Gala
On Tue, Jan 10, 2023 at 01:09:35PM +0530, Het Gala wrote: > > On 09/01/23 7:37 pm, Daniel P. Berrangé wrote: > > On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote: > > > From: Author Het Gala <het.gala@nutanix.com> > > > > > > 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. This scheme does seem to have an issue > > > in it, i.e. double-level encoding of URIs. > > > > > > The current patch maps existing QAPI design into a well-defined data > > > structure - 'MigrateChannel' only from the design perspective. Please note that > > > the existing 'uri' parameter is kept untouched for backward compatibility. > > > > > > +## > > > +# @MigrateExecAddr: > > > + # > > > + # Since 8.0 > > > + ## > > > +{ 'struct': 'MigrateExecAddr', > > > + 'data' : {'exec-str': 'str' } } > > Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command > > that is passed > > > > const char *argv[] = { "/bin/sh", "-c", command, NULL }; > > > > I have a strong preference for making it possible to avoid use > > of shell when spawning commands, since much of thue time it is > > not required and has the potential to open up vulnerabilities. > > It would be nice to be able to just take the full argv directly > > IOW > > > > { 'struct': 'MigrateExecAddr', > > 'data' : {'argv': ['str'] } } > > > > If the caller wants to keep life safe and simple now they can > > use > > ["/bin/nc", "-U", "/some/sock"] > > > > but if they still want to send it via shell, they can also do > > so > > > > ["/bin/sh", "-c", "...arbitrary shell script code...."] > Okay Daniel. I get your point and it makes sense too. This will also have > some code changes in exec.c file I assume. > > > + > > > +## > > > +# @MigrateRdmaAddr: > > > +# > > > +# Since 8.0 > > > +## > > > +{ 'struct': 'MigrateRdmaAddr', > > > + 'data' : {'rdma-str': 'str' } } > > Loooking at the RDMA code it takes the str, and treats it > > as an IPv4 address: > > > > > > addr = g_new(InetSocketAddress, 1); > > if (!inet_parse(addr, host_port, NULL)) { > > rdma->port = atoi(addr->port); > > rdma->host = g_strdup(addr->host); > > rdma->host_port = g_strdup(host_port); > > } > > > > so we really ought to accept an InetSocketAddress struct > > directly > > > > { 'struct': 'MigrateRdmaAddr', > > 'data' : {'rdma-str': 'InetSocketAddress' } } > > > Yes, It resembles to InetSocketAddress. Will make the relevant changes in > rdma.c file. > > With this, I had a small question in mind, do qemu need to develop / > leverage some functionality to check the correctness for host or port. > So that if the user enters an invalid host address, they get an error > message to enter correct address, if trying to migrate via qmp command line > interface. When the RDMA code uses the host address to resolve the RDMA endpoint, it will fail and report an error back. With regards, Daniel
On 10/01/23 3:02 pm, Daniel P. Berrangé wrote: > On Tue, Jan 10, 2023 at 01:09:35PM +0530, Het Gala wrote: >> On 09/01/23 7:37 pm, Daniel P. Berrangé wrote: >> >>> Loooking at the RDMA code it takes the str, and treats it >>> as an IPv4 address: >>> >>> >>> addr = g_new(InetSocketAddress, 1); >>> if (!inet_parse(addr, host_port, NULL)) { >>> rdma->port = atoi(addr->port); >>> rdma->host = g_strdup(addr->host); >>> rdma->host_port = g_strdup(host_port); >>> } >>> >>> so we really ought to accept an InetSocketAddress struct >>> directly >>> >>> { 'struct': 'MigrateRdmaAddr', >>> 'data' : {'rdma-str': 'InetSocketAddress' } } >>> >> Yes, It resembles to InetSocketAddress. Will make the relevant changes in >> rdma.c file. >> >> With this, I had a small question in mind, do qemu need to develop / >> leverage some functionality to check the correctness for host or port. >> So that if the user enters an invalid host address, they get an error >> message to enter correct address, if trying to migrate via qmp command line >> interface. > When the RDMA code uses the host address to resolve the RDMA > endpoint, it will fail and report an error back. > > > With regards, > Daniel Okay, understood. Thanks for the explanation Daniel Regards, Het Gala
On 09/01/23 7:37 pm, Daniel P. Berrangé wrote: > On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote: >> From: Author Het Gala <het.gala@nutanix.com> >> >> 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. This scheme does seem to have an issue >> in it, i.e. double-level encoding of URIs. >> >> The current patch maps existing QAPI design into a well-defined data >> structure - 'MigrateChannel' only from the design perspective. Please note that >> the existing 'uri' parameter is kept untouched for backward compatibility. >> >> +## >> +# @MigrateRdmaAddr: >> +# >> +# Since 8.0 >> +## >> +{ 'struct': 'MigrateRdmaAddr', >> + 'data' : {'rdma-str': 'str' } } > Loooking at the RDMA code it takes the str, and treats it > as an IPv4 address: > > > addr = g_new(InetSocketAddress, 1); > if (!inet_parse(addr, host_port, NULL)) { > rdma->port = atoi(addr->port); > rdma->host = g_strdup(addr->host); > rdma->host_port = g_strdup(host_port); > } > > so we really ought to accept an InetSocketAddress struct > directly > > { 'struct': 'MigrateRdmaAddr', > 'data' : {'rdma-str': 'InetSocketAddress' } } > Hi Daniel. I was going through the rdma code, and I think we also need 'host_port' for rdma_return_path context https://github.com/qemu/qemu/commit/44bcfd45e9806c78d9d526d69b0590227d215a78. I dont have much understanding but If you have any suggestion or a workaround for this, please suggest :) > >> + >> +## >> +# @MigrateAddress: >> +# >> +# The options available for communication transport mechanisms for migration >> +# >> +# Since 8.0 >> +## >> > With regards, > Daniel Regards, Het Gala
On Fri, Jan 13, 2023 at 01:37:26PM +0530, Het Gala wrote: > > On 09/01/23 7:37 pm, Daniel P. Berrangé wrote: > > On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote: > > > From: Author Het Gala <het.gala@nutanix.com> > > > > > > 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. This scheme does seem to have an issue > > > in it, i.e. double-level encoding of URIs. > > > > > > The current patch maps existing QAPI design into a well-defined data > > > structure - 'MigrateChannel' only from the design perspective. Please note that > > > the existing 'uri' parameter is kept untouched for backward compatibility. > > > > > > +## > > > +# @MigrateRdmaAddr: > > > +# > > > +# Since 8.0 > > > +## > > > +{ 'struct': 'MigrateRdmaAddr', > > > + 'data' : {'rdma-str': 'str' } } > > Loooking at the RDMA code it takes the str, and treats it > > as an IPv4 address: > > > > > > addr = g_new(InetSocketAddress, 1); > > if (!inet_parse(addr, host_port, NULL)) { > > rdma->port = atoi(addr->port); > > rdma->host = g_strdup(addr->host); > > rdma->host_port = g_strdup(host_port); > > } > > > > so we really ought to accept an InetSocketAddress struct > > directly > > > > { 'struct': 'MigrateRdmaAddr', > > 'data' : {'rdma-str': 'InetSocketAddress' } } > > > Hi Daniel. I was going through the rdma code, and I think we also need > 'host_port' for rdma_return_path context > https://github.com/qemu/qemu/commit/44bcfd45e9806c78d9d526d69b0590227d215a78. > I dont have much understanding but If you have any suggestion or a > workaround for this, please suggest :) The return path calls back into qemu_rdma_data_init() which takes host_port, but you'll already be changing that to take InetSocketAddress, so that'll be fine. With regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote: > > From: Author Het Gala <het.gala@nutanix.com> > > > > 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. This scheme does seem to have an issue > > in it, i.e. double-level encoding of URIs. > > > > The current patch maps existing QAPI design into a well-defined data > > structure - 'MigrateChannel' only from the design perspective. Please note that > > the existing 'uri' parameter is kept untouched 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 | 121 +++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 119 insertions(+), 2 deletions(-) > > diff --git a/qapi/migration.json b/qapi/migration.json > > index 88ecf86ac8..753e187ce2 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' : {'exec-str': 'str' } } > > Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command > that is passed > > const char *argv[] = { "/bin/sh", "-c", command, NULL }; > > I have a strong preference for making it possible to avoid use > of shell when spawning commands, since much of thue time it is > not required and has the potential to open up vulnerabilities. > It would be nice to be able to just take the full argv directly > IOW > > { 'struct': 'MigrateExecAddr', > 'data' : {'argv': ['str'] } } > > If the caller wants to keep life safe and simple now they can > use > ["/bin/nc", "-U", "/some/sock"] > > but if they still want to send it via shell, they can also do > so > > ["/bin/sh", "-c", "...arbitrary shell script code...."] > > > + > > +## > > +# @MigrateRdmaAddr: > > +# > > +# Since 8.0 > > +## > > +{ 'struct': 'MigrateRdmaAddr', > > + 'data' : {'rdma-str': 'str' } } > > Loooking at the RDMA code it takes the str, and treats it > as an IPv4 address: > > > addr = g_new(InetSocketAddress, 1); > if (!inet_parse(addr, host_port, NULL)) { > rdma->port = atoi(addr->port); > rdma->host = g_strdup(addr->host); > rdma->host_port = g_strdup(host_port); > } > > so we really ought to accept an InetSocketAddress struct > directly > > { 'struct': 'MigrateRdmaAddr', > 'data' : {'rdma-str': 'InetSocketAddress' } } I think that's probably the right thing to do; there is a native RDMA addressing scheme that people occasionally (once a decade or so) ask about but I don't think we've ever supported it. Dave > > > > + > > +## > > +# @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,36 @@ > > # 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 mututally exclusive but, atleast > > > Minor typos "mutually" "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-str": "/tmp/exec" } } } } > > Will need updating given my feedback above > > > +# <- { "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' } } > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
* Het Gala (het.gala@nutanix.com) wrote: > From: Author Het Gala <het.gala@nutanix.com> > > 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. This scheme does seem to have an issue > in it, i.e. double-level encoding of URIs. > > The current patch maps existing QAPI design into a well-defined data > structure - 'MigrateChannel' only from the design perspective. Please note that > the existing 'uri' parameter is kept untouched 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 | 121 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 119 insertions(+), 2 deletions(-) > > diff --git a/qapi/migration.json b/qapi/migration.json > index 88ecf86ac8..753e187ce2 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' : {'exec-str': 'str' } } > + > +## > +# @MigrateRdmaAddr: > +# > +# Since 8.0 > +## > +{ 'struct': 'MigrateRdmaAddr', > + 'data' : {'rdma-str': 'str' } } > + > +## > +# @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' } } > + The presence of the channeltype field suggests you're going to try to support multiple addresses; that's OK, but can you show an example of how that might look in the migrate command below? Dave > ## > # @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,36 @@ > # 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 mututally exclusive but, atleast > +# 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-str": "/tmp/exec" } } } } > +# <- { "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: > -- > 2.22.3 >
On 17/01/23 4:13 pm, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: >> On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote: >>> From: Author Het Gala <het.gala@nutanix.com> >>> >>> 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. This scheme does seem to have an issue >>> in it, i.e. double-level encoding of URIs. >>> >>> The current patch maps existing QAPI design into a well-defined data >>> structure - 'MigrateChannel' only from the design perspective. Please note that >>> the existing 'uri' parameter is kept untouched 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> >> Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command >> that is passed >> >> const char *argv[] = { "/bin/sh", "-c", command, NULL }; >> >> I have a strong preference for making it possible to avoid use >> of shell when spawning commands, since much of thue time it is >> not required and has the potential to open up vulnerabilities. >> It would be nice to be able to just take the full argv directly >> IOW >> >> { 'struct': 'MigrateExecAddr', >> 'data' : {'argv': ['str'] } } >> >> If the caller wants to keep life safe and simple now they can >> use >> ["/bin/nc", "-U", "/some/sock"] >> >> but if they still want to send it via shell, they can also do >> so >> >> ["/bin/sh", "-c", "...arbitrary shell script code...."] >> >>> + >>> +## >>> +# @MigrateRdmaAddr: >>> +# >>> +# Since 8.0 >>> +## >>> +{ 'struct': 'MigrateRdmaAddr', >>> + 'data' : {'rdma-str': 'str' } } >> Loooking at the RDMA code it takes the str, and treats it >> as an IPv4 address: >> >> >> addr = g_new(InetSocketAddress, 1); >> if (!inet_parse(addr, host_port, NULL)) { >> rdma->port = atoi(addr->port); >> rdma->host = g_strdup(addr->host); >> rdma->host_port = g_strdup(host_port); >> } >> >> so we really ought to accept an InetSocketAddress struct >> directly >> >> { 'struct': 'MigrateRdmaAddr', >> 'data' : {'rdma-str': 'InetSocketAddress' } } > I think that's probably the right thing to do; there is a native RDMA > addressing scheme that people occasionally (once a decade or so) > ask about but I don't think we've ever supported it. > > Dave Yes Dave. I will be implementing Rdma in form of InetSocketAddress only in the upcoming patch. >>> + >>> +## >>> +# @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' } } >>> + >> With regards, >> Daniel >> -- >> |: https://urldefense.proofpoint.com/v2/url?u=https-3A__berrange.com&d=DwIDAw&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=p8peRp4ioaDxoipqUtW15yQdVtCPnDBQv-1wk3r3y41SXWuI5JUUPMATMyMNDI4q&s=hukETUEPKU_01AyhkaMiQFWSRE1kUv84DdpSGvVjr1Q&e= -o- https://urldefense.proofpoint.com/v2/url?u=https-3A__www.flickr.com_photos_dberrange&d=DwIDAw&c=s883GpUCOChKOHiocYtGcg&r=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg&m=p8peRp4ioaDxoipqUtW15yQdVtCPnDBQv- >> Regards, Het Gala
On 17/01/23 4:17 pm, Dr. David Alan Gilbert wrote: > * Het Gala (het.gala@nutanix.com) wrote: >> From: Author Het Gala <het.gala@nutanix.com> >> >> 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. This scheme does seem to have an issue >> in it, i.e. double-level encoding of URIs. >> >> The current patch maps existing QAPI design into a well-defined data >> structure - 'MigrateChannel' only from the design perspective. Please note that >> the existing 'uri' parameter is kept untouched 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 | 121 +++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 119 insertions(+), 2 deletions(-) >> >> diff --git a/qapi/migration.json b/qapi/migration.json >> index 88ecf86ac8..753e187ce2 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' : {'exec-str': 'str' } } >> + >> +## >> +# @MigrateRdmaAddr: >> +# >> +# Since 8.0 >> +## >> +{ 'struct': 'MigrateRdmaAddr', >> + 'data' : {'rdma-str': 'str' } } >> + >> +## >> +# @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' } } >> + > The presence of the channeltype field suggests you're going to try to > support multiple addresses; that's OK, but can you show an example of > how that might look in the migrate command below? > > Dave Yes Dave. Other options will be "data" (for multifd) and "post-copy" (for post-copy migration). I am not very sure how will we represent "post-copy" part, but I will give you an example of how "main" and "data" Channels will be represented by an example below: -> { "execute": "migrate", "arguments": { "channels": [ { 'channeltype': 'main', 'addr': {'transport': 'socket', 'socket-type': { 'type': 'inet', 'host': '10.12.34.9', 'port': '1050'} } { 'channeltype': 'data', 'addr': {'transport': 'socket', 'socket-type': { 'type': 'inet', 'host': '10.12.34.92', 'port': '1234'}, 'multifd-count': 5 }, { 'channeltype': 'data', 'addr': { 'transport': 'socket', 'socket-type': {'type': 'inet', 'host': '0.0.0.4', 'port': '3000'}, 'multifd-count': 3 } ] } } So, 'data' channels will be used for multi-fd conection while 'main' will be used for just normal migration connection. This 'data' channel option has been planned with a whole different patchset series in future. Let me know if you have any more doubts regarding this :) >> # >> # -> { "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-str": "/tmp/exec" } } } } >> +# <- { "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: Regards, Het Gala
diff --git a/qapi/migration.json b/qapi/migration.json index 88ecf86ac8..753e187ce2 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' : {'exec-str': 'str' } } + +## +# @MigrateRdmaAddr: +# +# Since 8.0 +## +{ 'struct': 'MigrateRdmaAddr', + 'data' : {'rdma-str': 'str' } } + +## +# @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,36 @@ # 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 mututally exclusive but, atleast +# 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-str": "/tmp/exec" } } } } +# <- { "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: