| Message ID | 20230606101557.202060-2-het.gala@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration | expand |
On 06/06/23 3:45 pm, Het Gala wrote: > 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/qapi/migration.json b/qapi/migration.json > index 179af0c4d8..e61d25eba2 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1407,6 +1407,51 @@ > ## > { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} } > > +## > +# @MigrationAddressType: > +# > +# The migration stream transport mechanisms. > +# > +# @socket: Migrate via socket. > +# > +# @exec: Direct the migration stream to another process. > +# > +# @rdma: Migrate via RDMA. > +# > +# Since 8.1 > +## > +{ 'enum': 'MigrationAddressType', > + 'data': ['socket', 'exec', 'rdma'] } > + > +## > +# @MigrationExecCommand: > +# > +# @args: list of commands for migraton stream execution to a file. > +# > +# Notes: > +# > +# 1. @args[0] needs to be the path to the new program. > +# > +# Since 8.1 > +## > +{ 'struct': 'MigrationExecCommand', > + 'data': {'args': [ 'str' ] } } > + > +## > +# @MigrationAddress: > +# > +# Migration endpoint configuration. > +# > +# Since 8.1 > +## > +{ 'union': 'MigrationAddress', > + 'base': { 'transport' : 'MigrationAddressType'}, > + 'discriminator': 'transport', > + 'data': { > + 'socket': 'SocketAddress', > + 'exec': 'MigrationExecCommand', > + 'rdma': 'InetSocketAddress' } } > + > ## > # @migrate: > # Hi maintainers, this is just a reminder mail for v6 patchset for modification the QAPI design for migration qapis - 'migrate' and 'incoming-migrate'. From the last discussion, I have modified definitions specifically around QAPIs in patch 1 and 6, and have tried to make it short and consice and meaningful. Please have a look at it and suggest if any changes required. Tagging maintainers who have actively participated in the discussion in last few iterations : Markus, Daniel, Eric, Juan - but regardless other maintainers are also very well welcomed to share their opinions. Regards, Het Gala
On 13/06/23 10:58 am, Het Gala wrote: > > On 06/06/23 3:45 pm, Het Gala wrote: >> 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 45 insertions(+) >> >> diff --git a/qapi/migration.json b/qapi/migration.json >> index 179af0c4d8..e61d25eba2 100644 >> --- a/qapi/migration.json >> +++ b/qapi/migration.json >> @@ -1407,6 +1407,51 @@ >> ## >> { 'command': 'migrate-continue', 'data': {'state': >> 'MigrationStatus'} } >> +## >> +## >> +# @MigrationAddress: >> +# >> +# Migration endpoint configuration. >> +# >> +# Since 8.1 >> +## >> +{ 'union': 'MigrationAddress', >> + 'base': { 'transport' : 'MigrationAddressType'}, >> + 'discriminator': 'transport', >> + 'data': { >> + 'socket': 'SocketAddress', >> + 'exec': 'MigrationExecCommand', >> + 'rdma': 'InetSocketAddress' } } >> + >> ## >> # @migrate: >> # > > Hi maintainers, this is just a reminder mail for v6 patchset for > modification the QAPI design for migration qapis - 'migrate' and > 'incoming-migrate'. From the last discussion, I have modified > definitions specifically around QAPIs in patch 1 and 6, and have tried > to make it short and consice and meaningful. Please have a look at it > and suggest if any changes required. Tagging maintainers who have > actively participated in the discussion in last few iterations : > Markus, Daniel, Eric, Juan - but regardless other maintainers are also > very well welcomed to share their opinions. > This is just a friendly reminder mail for v6 patchset discusiion for modification of QAPI design for migration qapis. I know the maintainers were quite busy with KVM forum during the time I had sent out first reminder. Hope to get some positive reviews on the v6 patches soon. Thanks in advance !! Regards, Het Gala
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 | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/qapi/migration.json b/qapi/migration.json > index 179af0c4d8..e61d25eba2 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1407,6 +1407,51 @@ > ## > { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} } > > +## > +# @MigrationAddressType: > +# > +# The migration stream transport mechanisms. > +# > +# @socket: Migrate via socket. > +# > +# @exec: Direct the migration stream to another process. > +# > +# @rdma: Migrate via RDMA. > +# > +# Since 8.1 > +## > +{ 'enum': 'MigrationAddressType', > + 'data': ['socket', 'exec', 'rdma'] } > + > +## > +# @MigrationExecCommand: > +# > +# @args: list of commands for migraton stream execution to a file. Typo: migration > +# > +# Notes: > +# > +# 1. @args[0] needs to be the path to the new program. @args can't be a "list of commands", as we're spawning just one process. So what is it? Digging through the code with the entire series applied... Member @args is used in two places: 1. qemu_start_incoming_migration() passes it to exec_start_incoming_migration(), which translates it into an array and passes it to qio_channel_command_new_spawn(). 2. qmp_migrate() passes it to exec_start_outgoing_migration(), which does the same. qio_channel_command_new_spawn() passes it to g_spawn_async_with_pipes(). A close read of the latter's documentation leads me to: * args[0] is the excutable's file name. As usual, a relative name is relative to the QEMU process's current working directory. * args[1..] are the arguments. Unlike POSIX interfaces like execv() and posix_spawn(), this doesn't separate the executable's file name and 0-th argument. In short, the head of @args is the executable's filename, and the remainder are the arguments. The fact that the the executable's filename is passed as 0-th argument to the child process is detail. Perhaps this could do: ## # @MigrationExecCommand: # # @args: command and arguments to execute. If we want more detail, perhaps: # @args: command (list head) and arguments (list tail) to execute. Not sure we need it. Thoughts? > +# > +# Since 8.1 > +## > +{ 'struct': 'MigrationExecCommand', > + 'data': {'args': [ 'str' ] } } > + > +## > +# @MigrationAddress: > +# > +# Migration endpoint configuration. > +# > +# Since 8.1 > +## > +{ 'union': 'MigrationAddress', > + 'base': { 'transport' : 'MigrationAddressType'}, > + 'discriminator': 'transport', > + 'data': { > + 'socket': 'SocketAddress', > + 'exec': 'MigrationExecCommand', > + 'rdma': 'InetSocketAddress' } } > + > ## > # @migrate: > #
On 05/07/23 4:51 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 45 insertions(+) >> >> diff --git a/qapi/migration.json b/qapi/migration.json >> index 179af0c4d8..e61d25eba2 100644 >> --- a/qapi/migration.json >> +++ b/qapi/migration.json >> @@ -1407,6 +1407,51 @@ >> ## >> { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} } >> >> +## >> +# @MigrationAddressType: >> +# >> +# The migration stream transport mechanisms. >> +# >> +# @socket: Migrate via socket. >> +# >> +# @exec: Direct the migration stream to another process. >> +# >> +# @rdma: Migrate via RDMA. >> +# >> +# Since 8.1 >> +## >> +{ 'enum': 'MigrationAddressType', >> + 'data': ['socket', 'exec', 'rdma'] } >> + >> +## >> +# @MigrationExecCommand: >> +# >> +# @args: list of commands for migraton stream execution to a file. > Typo: migration > >> +# >> +# Notes: >> +# >> +# 1. @args[0] needs to be the path to the new program. > @args can't be a "list of commands", as we're spawning just one process. > So what is it? > > Digging through the code with the entire series applied... Member @args > is used in two places: > > 1. qemu_start_incoming_migration() passes it to > exec_start_incoming_migration(), which translates it into an array > and passes it to qio_channel_command_new_spawn(). > > 2. qmp_migrate() passes it to exec_start_outgoing_migration(), which > does the same. > > qio_channel_command_new_spawn() passes it to > g_spawn_async_with_pipes(). A close read of the latter's documentation > leads me to: > > * args[0] is the excutable's file name. As usual, a relative name is > relative to the QEMU process's current working directory. > > * args[1..] are the arguments. > > Unlike POSIX interfaces like execv() and posix_spawn(), this doesn't > separate the executable's file name and 0-th argument. > > In short, the head of @args is the executable's filename, and the > remainder are the arguments. The fact that the the executable's > filename is passed as 0-th argument to the child process is detail. > > Perhaps this could do: > > ## > # @MigrationExecCommand: > # > # @args: command and arguments to execute. > > If we want more detail, perhaps: > > # @args: command (list head) and arguments (list tail) to execute. > > Not sure we need it. Thoughts? From a user prespective, I think defining that command / executable's filename is the list head would be good ? something like @args: command (list head) and arguments to execute. >> +# >> +# Since 8.1 >> +## >> +{ 'struct': 'MigrationExecCommand', >> + 'data': {'args': [ 'str' ] } } >> + >> +## >> +# @MigrationAddress: >> +# >> +# Migration endpoint configuration. >> +# >> +# Since 8.1 >> +## >> +{ 'union': 'MigrationAddress', >> + 'base': { 'transport' : 'MigrationAddressType'}, >> + 'discriminator': 'transport', >> + 'data': { >> + 'socket': 'SocketAddress', >> + 'exec': 'MigrationExecCommand', >> + 'rdma': 'InetSocketAddress' } } >> + >> ## >> # @migrate: >> # Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 05/07/23 4:51 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 45 insertions(+) >>> >>> diff --git a/qapi/migration.json b/qapi/migration.json >>> index 179af0c4d8..e61d25eba2 100644 >>> --- a/qapi/migration.json >>> +++ b/qapi/migration.json >>> @@ -1407,6 +1407,51 @@ >>> ## >>> { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} } >>> +## >>> +# @MigrationAddressType: >>> +# >>> +# The migration stream transport mechanisms. >>> +# >>> +# @socket: Migrate via socket. >>> +# >>> +# @exec: Direct the migration stream to another process. >>> +# >>> +# @rdma: Migrate via RDMA. >>> +# >>> +# Since 8.1 >>> +## >>> +{ 'enum': 'MigrationAddressType', >>> + 'data': ['socket', 'exec', 'rdma'] } >>> + >>> +## >>> +# @MigrationExecCommand: >>> +# >>> +# @args: list of commands for migraton stream execution to a file. >> >> Typo: migration >> >>> +# >>> +# Notes: >>> +# >>> +# 1. @args[0] needs to be the path to the new program. >> >> @args can't be a "list of commands", as we're spawning just one process. >> So what is it? >> >> Digging through the code with the entire series applied... Member @args >> is used in two places: >> >> 1. qemu_start_incoming_migration() passes it to >> exec_start_incoming_migration(), which translates it into an array >> and passes it to qio_channel_command_new_spawn(). >> >> 2. qmp_migrate() passes it to exec_start_outgoing_migration(), which >> does the same. >> >> qio_channel_command_new_spawn() passes it to >> g_spawn_async_with_pipes(). A close read of the latter's documentation >> leads me to: >> >> * args[0] is the excutable's file name. As usual, a relative name is >> relative to the QEMU process's current working directory. >> >> * args[1..] are the arguments. >> >> Unlike POSIX interfaces like execv() and posix_spawn(), this doesn't >> separate the executable's file name and 0-th argument. >> >> In short, the head of @args is the executable's filename, and the >> remainder are the arguments. The fact that the the executable's >> filename is passed as 0-th argument to the child process is detail. >> >> Perhaps this could do: >> >> ## >> # @MigrationExecCommand: >> # >> # @args: command and arguments to execute. >> >> If we want more detail, perhaps: >> >> # @args: command (list head) and arguments (list tail) to execute. >> >> Not sure we need it. Thoughts? > > From a user prespective, I think defining that command / executable's filename is the list head would be good ? something like > > @args: command (list head) and arguments to execute. Works for me. [...]
diff --git a/qapi/migration.json b/qapi/migration.json index 179af0c4d8..e61d25eba2 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1407,6 +1407,51 @@ ## { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} } +## +# @MigrationAddressType: +# +# The migration stream transport mechanisms. +# +# @socket: Migrate via socket. +# +# @exec: Direct the migration stream to another process. +# +# @rdma: Migrate via RDMA. +# +# Since 8.1 +## +{ 'enum': 'MigrationAddressType', + 'data': ['socket', 'exec', 'rdma'] } + +## +# @MigrationExecCommand: +# +# @args: list of commands for migraton stream execution to a file. +# +# Notes: +# +# 1. @args[0] needs to be the path to the new program. +# +# Since 8.1 +## +{ 'struct': 'MigrationExecCommand', + 'data': {'args': [ 'str' ] } } + +## +# @MigrationAddress: +# +# Migration endpoint configuration. +# +# Since 8.1 +## +{ 'union': 'MigrationAddress', + 'base': { 'transport' : 'MigrationAddressType'}, + 'discriminator': 'transport', + 'data': { + 'socket': 'SocketAddress', + 'exec': 'MigrationExecCommand', + 'rdma': 'InetSocketAddress' } } + ## # @migrate: #