| Message ID | 20221121110427.198431-1-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | multifd: Updated QAPI format for 'migrate' qemu monitor command | expand |
Het Gala <het.gala@nutanix.com> wrote: > To prevent double data encoding of uris, instead of passing transport > mechanisms, host address and port all together in form of a single string > and writing different parsing functions, we intend the user to explicitly > mention most of the parameters through the medium of qmp command itself. > > The current patch is continuation of patchset series > https://www.mail-archive.com/qemu-devel@nongnu.org/msg901274.html > and reply to the ongoing discussion for better QAPI design here > https://www.mail-archive.com/qemu-devel@nongnu.org/msg903753.html. > > Suggested-by: Daniel P. Berrange <berrange@redhat.com> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > Suggested-by: Manish Mishra <manish.mishra@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> Hi 1st of all, I can't see how this is 7.1 material, I guess we need to move it to 8.0. > --- > qapi/migration.json | 127 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 125 insertions(+), 2 deletions(-) > > diff --git a/qapi/migration.json b/qapi/migration.json > index 88ecf86ac8..fd9286ea0f 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1449,12 +1449,101 @@ > ## > { '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. > +# > +# Since 7.1 > +## > +{ 'enum': 'MigrateTransport', > + 'data': ['socket', 'exec'] } I haven't looked too much into this, but as Danield told in the past, I can see where the rdma falls into this scheme. I guess it is going to be its own, but who knows. > +# The supported options for migration channel type requests > +# > +# @control: Support request for main outbound migration control channel > +# > +# @data: Supported Channel type for multifd data channels > +# > +# @async: Supported Channel type for post-copy async requests > +# > +# Since 7.1 > +## > +{ 'enum': 'MigrateChannelType', > + 'data': ['control', 'data', 'async'] } > + 'data': ['main', 'data', 'ram-async'] } ??? I don't like the 'control' name because without multifd we still pass everything through it. And with multifd, we still pass all devices through it. About the asynchronous channel, I don't know if calling it postcopy is better. > +{ 'struct': 'MigrateChannel', > + 'data' : { > + 'channeltype' : 'MigrateChannelType', > + '*src-addr' : 'MigrateAddress', > + 'dest-addr' : 'MigrateAddress', Why do we want *both* addresses? > + '*multifd-count' : 'int' } } And if we are passing a list, why do we want to pass the real number? > # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } > # <- { "return": {} } > # > +# -> { "execute": "migrate", > +# "arguments": { > +# "channels": [ { 'channeltype': 'control', > +# 'dest-addr': {'transport': 'socket', > +# 'type': 'inet', > +# 'host': '10.12.34.9', 'port': '1050'}}, > +# { 'channeltype': 'data', > +# 'src-addr': {'transport': 'socket', > +# 'type': 'inet', > +# 'host': '10.12.34.9', > +# 'port': '4000', 'ipv4': 'true'}, > +# 'dest-addr': { 'transport': 'socket', > +# 'type': 'inet', > +# 'host': '10.12.34.92', > +# 'port': '1234', 'ipv4': 'true'}, > +# 'multifd-count': 5 }, > +# { 'channeltype': 'data', > +# 'src-addr': {'transport': 'socket', > +# 'type': 'inet', > +# 'host': '10.2.3.4', 'port': '1000'}, > +# 'dest-addr': {'transport': 'socket', > +# 'type': 'inet', > +# 'host': '0.0.0.4', 'port': '3000'}, > +# 'multifd-count': 3 } ] } } > +# <- { "return": {} } > +# > ## > { 'command': 'migrate', > - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', > - '*detach': 'bool', '*resume': 'bool' } } > + 'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool', I think that "uri" bit should be dropped, right? > + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } } > > ## > # @migrate-incoming: I can't see how to make the old one to work on top of this one (i.e. we would have to create strings from lists on QAPI, I think that is just too much). So I think that the best way (I know I am contradicting myself) is to create a new migrate command and just let the old one alone. That way: - you can drop blk and blk - you can do anything that you want with the uris, as assuming that they are always sockets. - I would not care at all about the "exec" protocol, just leave that alone in the deprecated command. Right now: * we can't move it to multifd without a lot of PAIN * there are patches on the list suggesting that what we really want is to create a file that is the size of RAM and just write all the RAM at the right place. * that would make the way to create snapshots (I don't know if anyones still wants them, much easier). * I think that the only real use of exec migration was to create snapshots, for real migration, using a socket is much, much saner. * I.e. what I mean here is that for exec migration, we need to think if we want to continue supporting it for normal migration, or only as a way to create snapshots. What do you think? Later, Juan.
On Mon, Nov 21, 2022 at 01:40:27PM +0100, Juan Quintela wrote: > Het Gala <het.gala@nutanix.com> wrote: > > To prevent double data encoding of uris, instead of passing transport > > mechanisms, host address and port all together in form of a single string > > and writing different parsing functions, we intend the user to explicitly > > mention most of the parameters through the medium of qmp command itself. > > > > The current patch is continuation of patchset series > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg901274.html > > and reply to the ongoing discussion for better QAPI design here > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg903753.html. > > > > Suggested-by: Daniel P. Berrange <berrange@redhat.com> > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > > Suggested-by: Manish Mishra <manish.mishra@nutanix.com> > > Signed-off-by: Het Gala <het.gala@nutanix.com> > > > +{ 'struct': 'MigrateChannel', > > + 'data' : { > > + 'channeltype' : 'MigrateChannelType', > > + '*src-addr' : 'MigrateAddress', > > + 'dest-addr' : 'MigrateAddress', > > Why do we want *both* addresses? This is part of their goal to have source based routing, so that traffic exits the src host via a particular NIC. I think this patch would be better if we split it into two parts. First introduce "MigrateChannel" struct with *only* the 'dest-addr' field, and only allow one element in the list, making 'uri' optional. Basically the first patch would *only* be about switching the 'migrate' command from using a plain string to a MigrateAddress structure. A second patch would then extend it to allow multiple MigrateChannel elements, to support different destinations for each channel. A third patch would then extend it to add src-addr to attain the source based routing. A fourth patch can then deprecate the existing 'uri' field. > > > + '*multifd-count' : 'int' } } > > And if we are passing a list, why do we want to pass the real number? Yeah, technically I think this field is redundant, as you can just add many entires to the 'channels' list, using the same addresses. All this field does is allow a more compact JSON arg list. I'm not sure this is neccessary, unless we're expecting huge numbers of 'channels', and even then this isn't likely to be a performance issue. > > > # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } > > # <- { "return": {} } > > # > > +# -> { "execute": "migrate", > > +# "arguments": { > > +# "channels": [ { 'channeltype': 'control', > > +# 'dest-addr': {'transport': 'socket', > > +# 'type': 'inet', > > +# 'host': '10.12.34.9', 'port': '1050'}}, > > +# { 'channeltype': 'data', > > +# 'src-addr': {'transport': 'socket', > > +# 'type': 'inet', > > +# 'host': '10.12.34.9', > > +# 'port': '4000', 'ipv4': 'true'}, > > +# 'dest-addr': { 'transport': 'socket', > > +# 'type': 'inet', > > +# 'host': '10.12.34.92', > > +# 'port': '1234', 'ipv4': 'true'}, > > +# 'multifd-count': 5 }, > > +# { 'channeltype': 'data', > > +# 'src-addr': {'transport': 'socket', > > +# 'type': 'inet', > > +# 'host': '10.2.3.4', 'port': '1000'}, > > +# 'dest-addr': {'transport': 'socket', > > +# 'type': 'inet', > > +# 'host': '0.0.0.4', 'port': '3000'}, > > +# 'multifd-count': 3 } ] } } > > +# <- { "return": {} } > > +# > > ## > > { 'command': 'migrate', > > - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', > > - '*detach': 'bool', '*resume': 'bool' } } > > + 'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool', > > I think that "uri" bit should be dropped, right? No, it is required for back compatibility with existing clients. It should be marked deprecated, and removed several releases later, at which point 'chanels' can become mandatory instead of optional. > > > + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } } > > > > ## > > # @migrate-incoming: > > I can't see how to make the old one to work on top of this one (i.e. we > would have to create strings from lists on QAPI, I think that is just > too much). All we need is a piece of code that parses the 'uri' parameter and creates a MigrateAddress address from it. This can be called in the qmp_migrate() handler, and thereafter, everything else in the migration code can work with the MigrateAddress structs. SHould be pretty easy. > > So I think that the best way (I know I am contradicting myself) is to > create a new migrate command and just let the old one alone. That way: > - you can drop blk and blk blk & inc > - you can do anything that you want with the uris, as assuming that they > are always sockets. Regardless of whether we use the existing or new QMP command, we still have to convert the code to use the MigrateAddress struct, so I don't think it makes any difference. Both cases will require that we write code to convert from the legacy 'string' URI, to the new MigrateAddress struct. > - I would not care at all about the "exec" protocol, just leave that > alone in the deprecated command. Right now: > * we can't move it to multifd without a lot of PAIN > * there are patches on the list suggesting that what we really want is > to create a file that is the size of RAM and just write all the RAM > at the right place. > * that would make the way to create snapshots (I don't know if anyones > still wants them, much easier). > * I think that the only real use of exec migration was to create > snapshots, for real migration, using a socket is much, much saner. > * I.e. what I mean here is that for exec migration, we need to think > if we want to continue supporting it for normal migration, or only > as a way to create snapshots. The main challenge with 'exec' protocol is that it only sets up a uni-directional data channel. The main migration channel protocol has always been unidirectional, and that's responsible for alot of our pain in migration. There's no way todo a protocol handshake to negotiate features between client&server during connection. As such we've invented a ridiculous number of migration parameters that libvirt and other mgmt apps need to set on both sides - basically we have punted negotiation out of band to the mgmt app which is crazy. For our future sanity I think we need to define a brand new migration protocol which is bidirectional from the start. A large number of the MigrateParameters would become obsolete immediately, as QEMU could negotiate them automatically. This would let QEMU introduce new migration features without requiring any work in libvirt in many cases. Libvirt should only be required when there are performance tunables that need exposing, never for protocol feature negotiation. I think introducing a new QMP command, without introducing a fully new protocol would be a big mistake as the QMP command is not the problem we have. I've written much more detail about this here: https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03655.html I don't think this should be a dependancy for this patch proposal though. This is purely about making the 'migrate' command follow QMP best practice, by using a struct instead of encoding data in a string URI, and can be retrofitted to the existing command without difficulty. With regards, Daniel
On 22/11/22 2:53 pm, Daniel P. Berrangé wrote: > On Mon, Nov 21, 2022 at 01:40:27PM +0100, Juan Quintela wrote: >> Het Gala <het.gala@nutanix.com> wrote: >>> To prevent double data encoding of uris, instead of passing transport >>> mechanisms, host address and port all together in form of a single string >>> and writing different parsing functions, we intend the user to explicitly >>> mention most of the parameters through the medium of qmp command itself. >>> >>> The current patch is continuation of patchset series >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_qemu-2Ddevel-40nongnu.org_msg901274.html&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=c4KON2DiMd-szjwjggQcuUvTsPWblztAL0gVzaHnNmc&m=htOVZmDRCu27EhvYDu-28snN6GV0a6-ffX34joSwBkELLLVGHzRqD0LicZed3Xtw&s=xvzrWRBN4S5l3orPqu6di0MRq-gSGWZZU6-e7wpn8w4&e= >>> and reply to the ongoing discussion for better QAPI design here >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_qemu-2Ddevel-40nongnu.org_msg903753.html&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=c4KON2DiMd-szjwjggQcuUvTsPWblztAL0gVzaHnNmc&m=htOVZmDRCu27EhvYDu-28snN6GV0a6-ffX34joSwBkELLLVGHzRqD0LicZed3Xtw&s=anLqZbhqa73P9SyPUaGk5q8R0SYR6IUtH_FWFML3lZY&e= . >>> >>> Suggested-by: Daniel P. Berrange <berrange@redhat.com> >>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >>> Suggested-by: Manish Mishra <manish.mishra@nutanix.com> >>> Signed-off-by: Het Gala <het.gala@nutanix.com> > > > >>> +{ 'struct': 'MigrateChannel', >>> + 'data' : { >>> + 'channeltype' : 'MigrateChannelType', >>> + '*src-addr' : 'MigrateAddress', >>> + 'dest-addr' : 'MigrateAddress', >> Why do we want *both* addresses? > This is part of their goal to have source based routing, so that > traffic exits the src host via a particular NIC. > > I think this patch would be better if we split it into two parts. > > First introduce "MigrateChannel" struct with *only* the 'dest-addr' > field, and only allow one element in the list, making 'uri' optional. > > Basically the first patch would *only* be about switching the > 'migrate' command from using a plain string to a MigrateAddress > structure. > > A second patch would then extend it to allow multiple MigrateChannel > elements, to support different destinations for each channel. > > A third patch would then extend it to add src-addr to attain the > source based routing. > > A fourth patch can then deprecate the existing 'uri' field. > >>> + '*multifd-count' : 'int' } } >> And if we are passing a list, why do we want to pass the real number? > Yeah, technically I think this field is redundant, as you can just > add many entires to the 'channels' list, using the same addresses. > All this field does is allow a more compact JSON arg list. I'm > not sure this is neccessary, unless we're expecting huge numbers > of 'channels', and even then this isn't likely to be a performance > issue. > >>> # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } >>> # <- { "return": {} } >>> # >>> +# -> { "execute": "migrate", >>> +# "arguments": { >>> +# "channels": [ { 'channeltype': 'control', >>> +# 'dest-addr': {'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '10.12.34.9', 'port': '1050'}}, >>> +# { 'channeltype': 'data', >>> +# 'src-addr': {'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '10.12.34.9', >>> +# 'port': '4000', 'ipv4': 'true'}, >>> +# 'dest-addr': { 'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '10.12.34.92', >>> +# 'port': '1234', 'ipv4': 'true'}, >>> +# 'multifd-count': 5 }, >>> +# { 'channeltype': 'data', >>> +# 'src-addr': {'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '10.2.3.4', 'port': '1000'}, >>> +# 'dest-addr': {'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '0.0.0.4', 'port': '3000'}, >>> +# 'multifd-count': 3 } ] } } >>> +# <- { "return": {} } >>> +# >>> ## >>> { 'command': 'migrate', >>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', >>> - '*detach': 'bool', '*resume': 'bool' } } >>> + 'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool', >> I think that "uri" bit should be dropped, right? > No, it is required for back compatibility with existing clients. > It should be marked deprecated, and removed several releases later, > at which point 'chanels' can become mandatory instead of optional. > >>> + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } } >>> >>> ## >>> # @migrate-incoming: >> I can't see how to make the old one to work on top of this one (i.e. we >> would have to create strings from lists on QAPI, I think that is just >> too much). > All we need is a piece of code that parses the 'uri' parameter and > creates a MigrateAddress address from it. This can be called in > the qmp_migrate() handler, and thereafter, everything else in the > migration code can work with the MigrateAddress structs. SHould > be pretty easy. > >> So I think that the best way (I know I am contradicting myself) is to >> create a new migrate command and just let the old one alone. That way: >> - you can drop blk and blk > blk & inc > >> - you can do anything that you want with the uris, as assuming that they >> are always sockets. > Regardless of whether we use the existing or new QMP command, we still > have to convert the code to use the MigrateAddress struct, so I don't > think it makes any difference. Both cases will require that we write > code to convert from the legacy 'string' URI, to the new MigrateAddress > struct. > >> - I would not care at all about the "exec" protocol, just leave that >> alone in the deprecated command. Right now: >> * we can't move it to multifd without a lot of PAIN >> * there are patches on the list suggesting that what we really want is >> to create a file that is the size of RAM and just write all the RAM >> at the right place. >> * that would make the way to create snapshots (I don't know if anyones >> still wants them, much easier). >> * I think that the only real use of exec migration was to create >> snapshots, for real migration, using a socket is much, much saner. >> * I.e. what I mean here is that for exec migration, we need to think >> if we want to continue supporting it for normal migration, or only >> as a way to create snapshots. > The main challenge with 'exec' protocol is that it only sets up a > uni-directional data channel. The main migration channel protocol > has always been unidirectional, and that's responsible for alot of > our pain in migration. There's no way todo a protocol handshake to > negotiate features between client&server during connection. As such > we've invented a ridiculous number of migration parameters that > libvirt and other mgmt apps need to set on both sides - basically > we have punted negotiation out of band to the mgmt app which is > crazy. > > For our future sanity I think we need to define a brand new migration > protocol which is bidirectional from the start. A large number of the > MigrateParameters would become obsolete immediately, as QEMU could > negotiate them automatically. This would let QEMU introduce new > migration features without requiring any work in libvirt in many > cases. Libvirt should only be required when there are performance > tunables that need exposing, never for protocol feature negotiation. > > I think introducing a new QMP command, without introducing a fully > new protocol would be a big mistake as the QMP command is not the > problem we have. > > I've written much more detail about this here: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Ddevel_2022-2D03_msg03655.html&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=c4KON2DiMd-szjwjggQcuUvTsPWblztAL0gVzaHnNmc&m=htOVZmDRCu27EhvYDu-28snN6GV0a6-ffX34joSwBkELLLVGHzRqD0LicZed3Xtw&s=56T0bqIyHaHqZRDImrEfhvQHlsiPZXX-dCpX0Bm98wE&e= Daniel, Totally agree on above mentioned discussion. Does this account for verifying other things too along with migration capabilities e.g. if qemu machine type, vm config or cpu features are excatly same of both side. Currently libvirt takes that responsibility, but as you mentioned libvirt may take some time to get to level where qemu is so causing potential issues till then. Similar to this discussion we had on libvirt list https://www.mail-archive.com/libvir-list@redhat.com/msg233725.html. If these things too were managed by qemu indepenedelty things could have been much better. I mean those too are kind of related to live migration. :) Thanks Manish Mishra > I don't think this should be a dependancy for this patch proposal > though. This is purely about making the 'migrate' command follow > QMP best practice, by using a struct instead of encoding data in > a string URI, and can be retrofitted to the existing command without > difficulty. > > > > With regards, > Daniel
On Tue, Nov 22, 2022 at 09:27:52PM +0530, manish.mishra wrote: > > On 22/11/22 2:53 pm, Daniel P. Berrangé wrote: > > For our future sanity I think we need to define a brand new migration > > protocol which is bidirectional from the start. A large number of the > > MigrateParameters would become obsolete immediately, as QEMU could > > negotiate them automatically. This would let QEMU introduce new > > migration features without requiring any work in libvirt in many > > cases. Libvirt should only be required when there are performance > > tunables that need exposing, never for protocol feature negotiation. > > > > I think introducing a new QMP command, without introducing a fully > > new protocol would be a big mistake as the QMP command is not the > > problem we have. > > > > Daniel, Totally agree on above mentioned discussion. Does this > account for verifying other things too along with migration > capabilities e.g. if qemu machine type, vm config or cpu > features are excatly same of both side. Currently libvirt > takes that responsibility, but as you mentioned libvirt may > take some time to get to level where qemu is so causing > potential issues till then. Similar to this discussion we > had on libvirt list https://www.mail-archive.com/libvir-list@redhat.com/msg233725.html. > If these things too were managed by qemu indepenedelty > things could have been much better. I mean those too are > kind of related to live migration. :) Today libvirt has no choice, because if there's an ABI compat mistake in the dest QEMU config, vs the src QEMU config, then when loading VMstate on the target, the dst QEMU will often be unable to de-serialize the migration device stream and end up printing an error on stderr and exiting. Getting that info back to the src QEMU is impossible. If QEMU had a bi-directional migration stream, then the dst QEMU could simply send an error message back to the src QEMU and 'query-migrate' could actually display this. There may still be validation libvirt wants to do as well, but at least there would be the possiblity of offloading to QEMU without sacrificing error reporting. With regards, Daniel
On 21/11/22 6:10 pm, Juan Quintela wrote: > Het Gala<het.gala@nutanix.com> wrote: >> To prevent double data encoding of uris, instead of passing transport >> mechanisms, host address and port all together in form of a single string >> and writing different parsing functions, we intend the user to explicitly >> mention most of the parameters through the medium of qmp command itself. > Hi > > 1st of all, I can't see how this is 7.1 material, I guess we need to > move it to 8.0. > > Thankyou for informing. I will change it to 8.0 >> +## >> +# @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. >> +# >> +# Since 7.1 >> +## >> +{ 'enum': 'MigrateTransport', >> + 'data': ['socket', 'exec'] } > I haven't looked too much into this, but as Danield told in the past, I > can see where the rdma falls into this scheme. I guess it is going to > be its own, but who knows. >So do you mean, 'data' : ['socket', 'exec', 'rdma'] ? or it will be separately represented >> +# The supported options for migration channel type requests >> +# >> +# @control: Support request for main outbound migration control channel >> +# >> +# @data: Supported Channel type for multifd data channels >> +# >> +# @async: Supported Channel type for post-copy async requests >> +# >> +# Since 7.1 >> +## >> +{ 'enum': 'MigrateChannelType', >> + 'data': ['control', 'data', 'async'] } >> + > 'data': ['main', 'data', 'ram-async'] } ??? > > I don't like the 'control' name because without multifd we still pass > everything through it. > > And with multifd, we still pass all devices through it. > I agree with you Juan. 'main' seams a better name here. Thanks for the suggestion :) > About the asynchronous channel, I don't know if calling it postcopy is > better. > Okay, noted. Will change in the next patchset >> +{ 'struct': 'MigrateChannel', >> + 'data' : { >> + 'channeltype' : 'MigrateChannelType', >> + '*src-addr' : 'MigrateAddress', >> + 'dest-addr' : 'MigrateAddress', > Why do we want *both* addresses? > As mentioned by Daniel, multifd right now is supported for tcp socket, and we want to make multifd migration as bi-directional migration stream. >> + '*multifd-count' : 'int' } } > And if we are passing a list, why do we want to pass the real number? > I think multifd-count variable would be handy. Just to take a used case here, we have heavy workloads to migrate, and in our system we have a 25 Gig X 2 NIC cards available, and we want to ensure that migration does not fail due to less available network (because IO, and other processes might also consume network). By experiments, we know that per multifd channel - on an average we get around 8.5-10 Gbps. Writing the same channel atleast 3 times to cover one NIC seems again redundant. So to prevent this, we think inclusion of multifd-count would be useful there. And anyways it is an optional, so if you don't mention it, it will take into account as a single thread by default. Let me know if this is convincing or we can discuss more on this ? >> +# -> { "execute": "migrate", >> +# "arguments": { >> +# "channels": [ { 'channeltype': 'control', >> +# 'dest-addr': {'transport': 'socket', >> +# 'type': 'inet', >> +# 'host': '10.12.34.9', 'port': '1050'}}, >> +# { 'channeltype': 'data', >> +# 'src-addr': {'transport': 'socket', >> +# 'type': 'inet', >> +# 'host': '10.12.34.9', >> +# 'port': '4000', 'ipv4': 'true'}, >> +# 'dest-addr': { 'transport': 'socket', >> +# 'type': 'inet', >> +# 'host': '10.12.34.92', >> +# 'port': '1234', 'ipv4': 'true'}, >> +# 'multifd-count': 5 }, >> +# { 'channeltype': 'data', >> +# 'src-addr': {'transport': 'socket', >> +# 'type': 'inet', >> +# 'host': '10.2.3.4', 'port': '1000'}, >> +# 'dest-addr': {'transport': 'socket', >> +# 'type': 'inet', >> +# 'host': '0.0.0.4', 'port': '3000'}, >> +# 'multifd-count': 3 } ] } } >> +# <- { "return": {} } >> +# >> ## >> { 'command': 'migrate', >> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', >> - '*detach': 'bool', '*resume': 'bool' } } >> + 'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool', > I think that "uri" bit should be dropped, right? > yes, it will be dropped at a later point, right now we are keeping it for backward compatibility. Daniel has also responded to the same query. >> + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } } >> >> ## >> # @migrate-incoming: > We want to know your views on a suggestion we had in your mind. Like we know that for now, multifd feature is available only for tcp/inet. So should we frame our QAPI design in such a way that instead of 'socket' including 'tcp', 'fd', 'vsock', 'unix'; we should keep the transport union as ['tcp','fd','vsock','unix','exec'] and we would have 'MigrateAddress' struct only for 'tcp' (i.e. bi-directional migration stream only for tcp). For other transoprt protocols (uni-directional), we can either have struct which would include 'path' or can directly link to 'VsockSocketAddres',....... Juan, Daniel, Markus and other migration maintainers too, would love to hear your inputs on this :) Regards, Het Gala
On 22/11/22 2:53 pm, Daniel P. Berrangé wrote: > On Mon, Nov 21, 2022 at 01:40:27PM +0100, Juan Quintela wrote: >> Het Gala <het.gala@nutanix.com> wrote: >>> To prevent double data encoding of uris, instead of passing transport >>> mechanisms, host address and port all together in form of a single string >>> and writing different parsing functions, we intend the user to explicitly >>> mention most of the parameters through the medium of qmp command itself. >>> +{ 'struct': 'MigrateChannel', >>> + 'data' : { >>> + 'channeltype' : 'MigrateChannelType', >>> + '*src-addr' : 'MigrateAddress', >>> + 'dest-addr' : 'MigrateAddress', >> Why do we want *both* addresses? > This is part of their goal to have source based routing, so that > traffic exits the src host via a particular NIC. > > I think this patch would be better if we split it into two parts. > > First introduce "MigrateChannel" struct with *only* the 'dest-addr' > field, and only allow one element in the list, making 'uri' optional. > > Basically the first patch would *only* be about switching the > 'migrate' command from using a plain string to a MigrateAddress > structure. > > A second patch would then extend it to allow multiple MigrateChannel > elements, to support different destinations for each channel. > > A third patch would then extend it to add src-addr to attain the > source based routing. > > A fourth patch can then deprecate the existing 'uri' field. > Thanks Daniel. This is a nice idea. I will break this patch into 4 different patches, so it would be clear to see how the QAPI design is evolving. >>> + '*multifd-count' : 'int' } } >> And if we are passing a list, why do we want to pass the real number? > Yeah, technically I think this field is redundant, as you can just > add many entires to the 'channels' list, using the same addresses. > All this field does is allow a more compact JSON arg list. I'm > not sure this is neccessary, unless we're expecting huge numbers > of 'channels', and even then this isn't likely to be a performance > issue. > I have tried to explain this to Juan. The main purpose is, if we want to add 3 channels to one pair of interface and 4 pair of channels to another pair of interface, instead of writing the same interface 3 or 4 times, this field saves that redundancy, and I personally feel, it gives one clear representation of multifd capability. >>> +# -> { "execute": "migrate", >>> +# "arguments": { >>> +# "channels": [ { 'channeltype': 'control', >>> +# 'dest-addr': {'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '10.12.34.9', 'port': '1050'}}, >>> +# { 'channeltype': 'data', >>> +# 'src-addr': {'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '10.12.34.9', >>> +# 'port': '4000', 'ipv4': 'true'}, >>> +# 'dest-addr': { 'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '10.12.34.92', >>> +# 'port': '1234', 'ipv4': 'true'}, >>> +# 'multifd-count': 5 }, >>> +# { 'channeltype': 'data', >>> +# 'src-addr': {'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '10.2.3.4', 'port': '1000'}, >>> +# 'dest-addr': {'transport': 'socket', >>> +# 'type': 'inet', >>> +# 'host': '0.0.0.4', 'port': '3000'}, >>> +# 'multifd-count': 3 } ] } } >>> +# <- { "return": {} } >>> +# >>> ## >>> { 'command': 'migrate', >>> - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', >>> - '*detach': 'bool', '*resume': 'bool' } } >>> + 'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool', >>> >> - I would not care at all about the "exec" protocol, just leave that >> alone in the deprecated command. Right now: >> * we can't move it to multifd without a lot of PAIN >> * there are patches on the list suggesting that what we really want is >> to create a file that is the size of RAM and just write all the RAM >> at the right place. >> * that would make the way to create snapshots (I don't know if anyones >> still wants them, much easier). >> * I think that the only real use of exec migration was to create >> snapshots, for real migration, using a socket is much, much saner. >> * I.e. what I mean here is that for exec migration, we need to think >> if we want to continue supporting it for normal migration, or only >> as a way to create snapshots. > > With regards, > Daniel Thanks and regards, Het Gala
On Wed, Nov 23, 2022 at 01:35:22AM +0530, Het Gala wrote: > > On 21/11/22 6:10 pm, Juan Quintela wrote: > > Het Gala<het.gala@nutanix.com> wrote: > > > To prevent double data encoding of uris, instead of passing transport > > > mechanisms, host address and port all together in form of a single string > > > and writing different parsing functions, we intend the user to explicitly > > > mention most of the parameters through the medium of qmp command itself. > > Hi > > > > 1st of all, I can't see how this is 7.1 material, I guess we need to > > move it to 8.0. > > > > Thankyou for informing. I will change it to 8.0 > > > +## > > > +# @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. > > > +# > > > +# Since 7.1 > > > +## > > > +{ 'enum': 'MigrateTransport', > > > + 'data': ['socket', 'exec'] } > > I haven't looked too much into this, but as Danield told in the past, I > > can see where the rdma falls into this scheme. I guess it is going to > > be its own, but who knows. > So do you mean, 'data' : ['socket', 'exec', 'rdma'] ? or it will be > separately represented Yes, you'll need an rdma entry there. If you follow the patch split I suggested in the other mail, you'll naturally have to represent 'rdma' in the very first patch, as you introduce the new syntax for addresses and parse the existing URIs to map onto the new address struct. With regards, Daniel
diff --git a/qapi/migration.json b/qapi/migration.json index 88ecf86ac8..fd9286ea0f 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1449,12 +1449,101 @@ ## { '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. +# +# Since 7.1 +## +{ 'enum': 'MigrateTransport', + 'data': ['socket', 'exec'] } + +## +# @MigrateSocketAddrType: +# +# To support different type of socket. +# +# @socket-type: Different type of socket connections. +# +# Since (7.1) +## +{ 'struct': 'MigrateSocketAddrType', + 'data': {'socket-type': 'SocketAddress' } } + +## +# @MigrateAddress: +# +# The options available for communication transport mechanisms for migration +# +# Since 7.1 +## +{ 'union' : 'MigrateAddress', + 'base' : { 'transport' : 'MigrateTransport'}, + 'discriminator' : 'transport', + 'data' : { + 'socket' : 'MigrateSocketAddrType', + 'exec' : 'String' } } + +## +# @MigrateChannelType: +# +# The supported options for migration channel type requests +# +# @control: Support request for main outbound migration control channel +# +# @data: Supported Channel type for multifd data channels +# +# @async: Supported Channel type for post-copy async requests +# +# Since 7.1 +## +{ 'enum': 'MigrateChannelType', + 'data': ['control', 'data', 'async'] } + +## +# @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 +# +# @src-addr: SocketAddress of source interface +# +# @dest-addr: SocketAddress of destination interface +# +# @multifd-count: number of multifd channels used to migrate +# data between a specific source and destination +# interface. Default value in this case is 2. +# +# Since 7.1 +## +{ 'struct': 'MigrateChannel', + 'data' : { + 'channeltype' : 'MigrateChannelType', + '*src-addr' : 'MigrateAddress', + 'dest-addr' : 'MigrateAddress', + '*multifd-count' : 'int' } } + ## # @migrate: # # Migrates the current running guest to another Virtual Machine. # # @uri: the Uniform Resource Identifier of the destination VM +# for migration thread +# +# @channels: list of migration channel type, pair of source and +# destination interface with number of multifd-channels +# for each pair # # @blk: do block migration (full disk copy) # @@ -1479,15 +1568,49 @@ # 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 'channels' arguments, are mututally exclusive. +# +# 6. The 'channels' argument should contain atleast one control channel for +# main outbound migration. +# # Example: # # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } # <- { "return": {} } # +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { 'channeltype': 'control', +# 'dest-addr': {'transport': 'socket', +# 'type': 'inet', +# 'host': '10.12.34.9', 'port': '1050'}}, +# { 'channeltype': 'data', +# 'src-addr': {'transport': 'socket', +# 'type': 'inet', +# 'host': '10.12.34.9', +# 'port': '4000', 'ipv4': 'true'}, +# 'dest-addr': { 'transport': 'socket', +# 'type': 'inet', +# 'host': '10.12.34.92', +# 'port': '1234', 'ipv4': 'true'}, +# 'multifd-count': 5 }, +# { 'channeltype': 'data', +# 'src-addr': {'transport': 'socket', +# 'type': 'inet', +# 'host': '10.2.3.4', 'port': '1000'}, +# 'dest-addr': {'transport': 'socket', +# 'type': 'inet', +# 'host': '0.0.0.4', 'port': '3000'}, +# 'multifd-count': 3 } ] } } +# <- { "return": {} } +# ## { 'command': 'migrate', - 'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', - '*detach': 'bool', '*resume': 'bool' } } + 'data': {'*uri': 'str', '*channels': ['MigrateChannel'], '*blk': 'bool', + '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } } ## # @migrate-incoming:
To prevent double data encoding of uris, instead of passing transport mechanisms, host address and port all together in form of a single string and writing different parsing functions, we intend the user to explicitly mention most of the parameters through the medium of qmp command itself. The current patch is continuation of patchset series https://www.mail-archive.com/qemu-devel@nongnu.org/msg901274.html and reply to the ongoing discussion for better QAPI design here https://www.mail-archive.com/qemu-devel@nongnu.org/msg903753.html. Suggested-by: Daniel P. Berrange <berrange@redhat.com> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> Suggested-by: Manish Mishra <manish.mishra@nutanix.com> Signed-off-by: Het Gala <het.gala@nutanix.com> --- qapi/migration.json | 127 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 125 insertions(+), 2 deletions(-)