On 30/05/23 12:28 pm, Markus Armbruster wrote:
Het Gala<het.g...@nutanix.com> writes:
On 25/05/23 11:04 pm, Markus Armbruster wrote:
Het Gala<het.g...@nutanix.com> writes:
This patch introduces well defined MigrateAddress struct and its related child
objects.
The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
string type. The current migration flow follows double encoding scheme for
fetching migration parameters such as 'uri' and this is not an ideal design.
Motive for intoducing struct level design is to prevent double encoding of QAPI
arguments, as Qemu should be able to directly use the QAPI arguments without
any level of encoding.
Suggested-by: Aravind Retnakaran<aravind.retnaka...@nutanix.com>
Signed-off-by: Het Gala<het.g...@nutanix.com>
Reviewed-by: Juan Quintela<quint...@redhat.com>
Reviewed-by: Daniel P. Berrangé<berra...@redhat.com>
---
qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/qapi/migration.json b/qapi/migration.json
index 179af0c4d8..c500744bb7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1407,6 +1407,47 @@
##
{ 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
+##
+# @MigrateTransport:
I'd prefer MigrationTransport, because "migration" is a noun, while
migrate is a verb. Verbs are for commands. For types we use nouns.
More of the same below, not noting it again.
Actually, I'd prefer MigrationAddressType, because it's purpose is to
serve as discriminator type in union MigrationAddress.
Okay got it. I kept it Transport as they are different transport mechanisms.
But 'MigrationAddressType' looks cleaner and comaptible with 'MigrateAddress'
union too. Will change that
Transport isn't bad, but I think a type that is only used for a union
discriminator is best named after the union type.
Yes I agree with your approach too. Will change it to 'MigrationAddressType'
in the next patchset.
+#
+# 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
Migration is between hosts, not "two devices".
Here we are just talking about socket communication right ? So I thought
devices might also work.
In QEMU, "devices" are the things you create with device_add.
Sockets connect "endpoints". Also called "peers".
Ack. 'endpoints' sounds very appropriate to me.
Will change that to 'hosts' as this is in context of migration i.e.
MigrattionAddressType
The second sentence confuses me. What are you trying to say?
I am trying to say that socket is a union in itslef right, so it covers
communication transport mechanisms like tcp, unix, vsock and fd already in it.
Also, missing period at the end.
Ack.
+#
+# @exec: Supported communication type to redirect migration stream into file.
+#
+# @rdma: Supported communication type to redirect rdma type migration stream.
What about:
##
# @MigrationTransport:
#
# The migration stream transport mechanisms
#
# @socket: Migrate via socket
#
# @rdma: Migrate via RDMA
#
# @file: Direct the migration stream to a file
Should I change from '@exec' to '@file' ?
Uh, that change happened somewhere between my conscious thought process
and the keyboard ;)
What about
# @exec: Direct the migration stream to another process
No worries Markus. Seems okay.
Other than that, it looks better than what I proposed. Will change it.
+#
+# Since 8.1
+##
+{ 'enum': 'MigrateTransport',
+ 'data': ['socket', 'exec', 'rdma'] }
+
+##
+# @MigrateExecCommand:
Documentation of @args is missing.
Ack. Should the naming '@args' be replaced by '@filepath' or @path' or
something similar ?
Depends on what @args means.
I guess its [program, arg1, arg2, ...].
You could split off the program:
'program: 'str',
'args': [ 'str' ]
Try to write clear documentation for both alternatives. Such an
exercise tends to lead me to the one I prefer.
Hmm, basically here the @args means, for example ['/bin/bash', args1, args2,
..., <command>], where command -> /some/file/path.
Does it even make sense now to break into 3 different parts ?
'program': 'str'
'args': [ 'str' ]
'command': 'str'
This might probably just need to tewak something in the exec file I guess.
+ #
+ # Since 8.1
+ ##
Unwanted indentation.
Not able to see any unwanted indentation here ?
Looks like it got eaten on the way. The last three lines of the doc
comment are indented:
+##
+# @MigrateExecCommand:
+ #
+ # Since 8.1
+ ##
+{ 'struct': 'MigrateExecCommand',
+ 'data': {'args': [ 'str' ] } }
Yes, you are right. I figured out after replying to you and was looking at
the code. Thanks for noticing it out! Will change spurious indentation in
the v6.
+{ 'struct': 'MigrateExecCommand',
+ 'data': {'args': [ 'str' ] } }
+
+##
+# @MigrateAddress:
+#
+# The options available for communication transport mechanisms for migration
Not happy with this sentence (writing good documentation is hard).
Is the address used for the destination only, or for the source as well?
If destination only, could it be used for the source at least in theory?
I'm asking because I need to understand more about intended use to be
able to suggest doc improvements.
This address will be used on both destination and source. In code flow, in
later patches, changes on destination as well as source have been made to
incorporate same definition.
Does @exec work as source?
Maybe:
# Endpoint address for migration
or
# Migration endpoint configuration
I think @exec work as source too, because in exec.c file, there are calls
for souce as well as destination.
I would like to go with "Migration endpoint configuration" because I feel
'migrate' and 'migrate-incoming' QAPIs are defined in context of live
migration.
+#
+# Since 8.1
+##
+{ 'union': 'MigrateAddress',
+ 'base': { 'transport' : 'MigrateTransport'},
+ 'discriminator': 'transport',
+ 'data': {
+ 'socket': 'SocketAddress',
+ 'exec': 'MigrateExecCommand',
+ 'rdma': 'InetSocketAddress' } }
+
Aside: a more powerful type system would let us extend SocketAddress
with additional variants instead of wrapping it in a union.
Markus, what do you mean by additional variants here in context of socket? Can
you give a small example.
As is, we have a nest of two unions:
* The outer union has branches @socket, @exec, @rdma.
* Branch @socket is the inner union, it has branches @inet, @unix, ...
A more powerful type system would let us extend SocketAddress instead,
so MigrateAddress has everything SocketAddress has, plus additional
branches @exec, @rdma. Naturally, the type of the discriminator also
needs to be extended, so that it has everything SocketAddress's
discriminator type has, plus additional members @exec, @rdma.
Okay, so you mean something like :
+# Since 8.1
+##
+{ 'union': 'MigrateAddress',
+ 'base': { 'transport' : 'MigrateTransport'},
+ 'discriminator': 'transport',
+ 'data': {
+ 'inet': 'InetSocketAddress',
+ 'unix': 'UnixSocketAddress',
+ 'vsock': 'VsockSocketAddress',
+ 'fd': 'str',
+ 'exec': 'MigrateExecCommand',
+ 'rdma': 'InetSocketAddress' } }
Even I agree that directly leveraging this is the best option, but then
wouldn't it introduce redundancy ? we would not be able to leverage socket
union right in that case or am I missing something.
