| Message ID | 20230519094617.7078-2-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration | expand |
Het Gala <het.gala@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.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> > Reviewed-by: Juan Quintela <quintela@redhat.com> > Reviewed-by: Daniel P. Berrangé <berrange@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. > +# > +# 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". The second sentence confuses me. What are you trying to say? Also, missing period at the end. > +# > +# @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 > +# > +# Since 8.1 > +## > +{ 'enum': 'MigrateTransport', > + 'data': ['socket', 'exec', 'rdma'] } > + > +## > +# @MigrateExecCommand: Documentation of @args is missing. > + # > + # Since 8.1 > + ## Unwanted indentation. > +{ '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. > +# > +# 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. > ## > # @migrate: > #
On 25/05/23 11:04 pm, Markus Armbruster wrote: > Het Gala <het.gala@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.retnakaran@nutanix.com> >> Signed-off-by: Het Gala <het.gala@nutanix.com> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> Reviewed-by: Daniel P. Berrangé <berrange@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 >> +# >> +# 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. 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' ? 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 ? >> + # >> + # Since 8.1 >> + ## > Unwanted indentation. Not able to see any unwanted indentation here ? >> +{ '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. >> +# >> +# 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. >> ## >> # @migrate: >> # Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 25/05/23 11:04 pm, Markus Armbruster wrote: >> Het Gala <het.gala@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.retnakaran@nutanix.com> >>> Signed-off-by: Het Gala <het.gala@nutanix.com> >>> Reviewed-by: Juan Quintela <quintela@redhat.com> >>> Reviewed-by: Daniel P. Berrangé <berrange@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. >>> +# >>> +# 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". > 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 > 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. >>> + # >>> + # 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' ] } } >>> +{ '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 >>> +# >>> +# 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.
On 30/05/23 12:28 pm, Markus Armbruster wrote: > Het Gala<het.gala@nutanix.com> writes: > >> On 25/05/23 11:04 pm, Markus Armbruster wrote: >>> Het Gala<het.gala@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.retnakaran@nutanix.com> >>>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>>> Reviewed-by: Juan Quintela<quintela@redhat.com> >>>> Reviewed-by: Daniel P. Berrangé<berrange@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. Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 30/05/23 12:28 pm, Markus Armbruster wrote: >> Het Gala<het.gala@nutanix.com> writes: >> >>> On 25/05/23 11:04 pm, Markus Armbruster wrote: [...] >>>> 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. Yes, there's redundancy, due to QAPI's insufficient expressive power. Is the cleaner external interface worth the (internal) redundancy, and possibly coding complications that go with it?
On 30/05/23 1:26 pm, Markus Armbruster wrote: > Het Gala <het.gala@nutanix.com> writes: > >> On 30/05/23 12:28 pm, Markus Armbruster wrote: >>> Het Gala<het.gala@nutanix.com> writes: >>> >>>> On 25/05/23 11:04 pm, Markus Armbruster wrote: > [...] > >>>>> 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. > Yes, there's redundancy, due to QAPI's insufficient expressive power. > > Is the cleaner external interface worth the (internal) redundancy, and > possibly coding complications that go with it? Honestly, I would like to have it this way, where the user is aware of all the transport mechanisms available. But I guess for external interface problem statement, the migration code flow has similar path for SocketAddress and non-socketAddress (exec and rdma). So even if on the QAPI side we express explicitly all the transport mechanisms, while implementing it we either would need to brinf them under a single umbrella. For now, I would keep the implementation as it is (union inside a union) but would want to have a more powerful and better appraoch out there if possible. I would like to have Migration maintainers - Juan, Peter Xu and others to comment on what approach from the above two is a better one. Regards, Het Gala
On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote: > > On 30/05/23 12:28 pm, Markus Armbruster wrote: > > Het Gala<het.gala@nutanix.com> writes: > > > > > On 25/05/23 11:04 pm, Markus Armbruster wrote: > > > > Het Gala<het.gala@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.retnakaran@nutanix.com> > > > > > Signed-off-by: Het Gala<het.gala@nutanix.com> > > > > > Reviewed-by: Juan Quintela<quintela@redhat.com> > > > > > Reviewed-by: Daniel P. Berrangé<berrange@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. The first four are going to have to be packed back into a SocketAddress struct immediately, as the internal migration APIs all work in terms of a SocketAddress for the inet/unix/vsock/fd case. With regards, Daniel
On 30/05/23 2:27 pm, Daniel P. Berrangé wrote: > On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote: >> On 30/05/23 12:28 pm, Markus Armbruster wrote: >>> Het Gala<het.gala@nutanix.com> writes: >>> >>>> On 25/05/23 11:04 pm, Markus Armbruster wrote: >>>>> Het Gala<het.gala@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.retnakaran@nutanix.com> >>>>>> Signed-off-by: Het Gala<het.gala@nutanix.com> >>>>>> Reviewed-by: Juan Quintela<quintela@redhat.com> >>>>>> Reviewed-by: Daniel P. Berrangé<berrange@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. > The first four are going to have to be packed back into a SocketAddress > struct immediately, as the internal migration APIs all work in terms of > a SocketAddress for the inet/unix/vsock/fd case. Concur, that's what I mentioned in just earlier reply, Daniel. > With regards, > Daniel Regards, Het Gala
On Tue, May 30, 2023 at 02:26:23PM +0530, Het Gala wrote: > > On 30/05/23 1:26 pm, Markus Armbruster wrote: > > Het Gala <het.gala@nutanix.com> writes: > > > > > On 30/05/23 12:28 pm, Markus Armbruster wrote: > > > > Het Gala<het.gala@nutanix.com> writes: > > > > > > > > > On 25/05/23 11:04 pm, Markus Armbruster wrote: > > [...] > > > > > > > > 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. > > Yes, there's redundancy, due to QAPI's insufficient expressive power. > > > > Is the cleaner external interface worth the (internal) redundancy, and > > possibly coding complications that go with it? > > Honestly, I would like to have it this way, where the user is aware of all > the transport mechanisms available. But I guess for external interface > problem statement, the migration code flow has similar path for > SocketAddress and non-socketAddress (exec and rdma). So even if on the QAPI > side we express explicitly all the transport mechanisms, while implementing > it we either would need to brinf them under a single umbrella. For now, I > would keep the implementation as it is (union inside a union) but would want > to have a more powerful and better appraoch out there if possible. IMHO, unpacking the SocketAddress types into the two level is not something we need to do. The main (perhaps even only) reason it arises as a design question is because historically the migration public interface has NOT used SocketAddress, and exposed unix/inet/vsock as distinct top level options, and this taints our thoughts. If the "migrate" command didn't already exist and we were adding it now IMHO we would just expose 'socket': 'SocketAdress' as a top level type, in parallel with 'exec' and 'rdma', and not even discuss the idea of unpacking 'SocketAddress'. IMHO the compelling reason to NOT unpack 'SocketAddress' is that it lets the migration code seemlessly benefit from any new 'SocketAddress' types that gets implemented. eg consider when 'vsock' was added to QEMU. Any command that took a 'SocketAddress' parameter needed no QAPI changes and usually no code changes either. With regards, Daniel
On 30/05/23 1:02 pm, Het Gala wrote: > > > On 30/05/23 12:28 pm, Markus Armbruster wrote: >> Het Gala<het.gala@nutanix.com> writes: >> >>> On 25/05/23 11:04 pm, Markus Armbruster wrote: >>>> Het Gala<het.gala@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. >>>>> >>>>> 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. > Markus, Daniel - any comments on this ? >>>>> + # >>>>> + # 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' ] } } [...] Regards, Het Gala
On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote: > > On 30/05/23 12:28 pm, Markus Armbruster wrote: > > Het Gala<het.gala@nutanix.com> writes: > > > > > On 25/05/23 11:04 pm, Markus Armbruster wrote: > > > > Het Gala<het.gala@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.retnakaran@nutanix.com> > > > > > Signed-off-by: Het Gala<het.gala@nutanix.com> > > > > > Reviewed-by: Juan Quintela<quintela@redhat.com> > > > > > Reviewed-by: Daniel P. Berrangé<berrange@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, ...]. Yes, that is correct. Essentially this ends up calling execve(@args[0], @args) > > You could split off the program: > > > > 'program: 'str', > > 'args': [ 'str' ] Now you also have to declare whether '@args' includes or excludes 'program' as @args[0]. execve() style would be to have '@args[0]' duplicate 'program'. Personally I think that's overkill for QEMU's needs. If we don't include 'program' in @args, then we have to document this, as it isn't discoverable from the QAPI design. Not separating 'program' and 'args' in QAPI makes it unambiguous that 'args' must include everything. > > 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' Definitely not. This encodes an assumption that we're spawning via a shell. The intent with the new design is that it lets mgmt apps fully eliminate use of shell, and directly invoke the program, thus eliminating potential (security) pitfalls with shell metacharacters. With regards, Daniel
On 30/05/23 5:40 pm, Daniel P. Berrangé wrote: > On Tue, May 30, 2023 at 01:02:27PM +0530, Het Gala wrote: >> On 30/05/23 12:28 pm, Markus Armbruster wrote: >>> Het Gala<het.gala@nutanix.com> writes: >>> >>> [...] >>>>>> +## >>>>>> +{ '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, ...]. > Yes, that is correct. Essentially this ends up calling > > execve(@args[0], @args) > >>> You could split off the program: >>> >>> 'program: 'str', >>> 'args': [ 'str' ] > Now you also have to declare whether '@args' includes or excludes > 'program' as @args[0]. execve() style would be to have '@args[0]' > duplicate 'program'. Personally I think that's overkill for QEMU's > needs. If we don't include 'program' in @args, then we have to > document this, as it isn't discoverable from the QAPI design. > > Not separating 'program' and 'args' in QAPI makes it unambiguous > that 'args' must include everything. Yes, we need to make user aware of args[0], so keeping it @args along with adding a note that @args[0] - path to the new program ? is the best alternative here ? - Markus, Daniel >>> 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' > Definitely not. This encodes an assumption that we're spawning via a > shell. The intent with the new design is that it lets mgmt apps fully > eliminate use of shell, and directly invoke the program, thus eliminating > potential (security) pitfalls with shell metacharacters. Got your point Daniel. Thanks. > With regards, > Daniel Regards, Het Gala
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: +# +# 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.1 +## +{ 'enum': 'MigrateTransport', + 'data': ['socket', 'exec', 'rdma'] } + +## +# @MigrateExecCommand: + # + # Since 8.1 + ## +{ 'struct': 'MigrateExecCommand', + 'data': {'args': [ 'str' ] } } + +## +# @MigrateAddress: +# +# The options available for communication transport mechanisms for migration +# +# Since 8.1 +## +{ 'union': 'MigrateAddress', + 'base': { 'transport' : 'MigrateTransport'}, + 'discriminator': 'transport', + 'data': { + 'socket': 'SocketAddress', + 'exec': 'MigrateExecCommand', + 'rdma': 'InetSocketAddress' } } + ## # @migrate: #