On Mon, Nov 21, 2022 at 01:40:27PM +0100, Juan Quintela wrote: > Het Gala <het.g...@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 <berra...@redhat.com> > > Suggested-by: Aravind Retnakaran <aravind.retnaka...@nutanix.com> > > Suggested-by: Manish Mishra <manish.mis...@nutanix.com> > > Signed-off-by: Het Gala <het.g...@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 -- |: 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 :|