| Message ID | 20230519094617.7078-7-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: > MigrateChannelList allows to connect accross multiple interfaces. Added > MigrateChannelList struct as argument to migration QAPIs. > > Future patchset series plans to include multiple MigrateChannels > for multiple interfaces to be connected. That is the reason, 'MigrateChannelList' > is the preferred choice of argument over 'MigrateChannel' and making > migration QAPIs future proof. > > For current patchset series, have limited the size of the list to single > element (single interface) as runtime check. > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> > --- [...] > diff --git a/qapi/migration.json b/qapi/migration.json > index c500744bb7..86bbc916d1 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1448,12 +1448,47 @@ > 'exec': 'MigrateExecCommand', > 'rdma': 'InetSocketAddress' } } > > +## > +# @MigrateChannelType: As mentioned in my review of PATCH 1, I prefer nouns to verbs for types, i.e. Migration, not Migrate. More of the same below, not flagging it again. > +# > +# The supported options for migration channel type requests > +# > +# @main: Support request for main outbound migration control channel > +# > +# Since 8.1 > +## > +{ 'enum': 'MigrateChannelType', > + 'data': [ 'main' ] } > + > +## > +# @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 @channel-type, because "channeltype" is not a word. > +# > +# @addr: Information regarding migration parameters of destination interface > +# > +# Since 8.1 > +## > +{ 'struct': 'MigrateChannel', > + 'data': { > + 'channeltype': 'MigrateChannelType', > + 'addr': 'MigrateAddress' } } > + > ## > # @migrate: > # > # Migrates the current running guest to another Virtual Machine. > # > # @uri: the Uniform Resource Identifier of the destination VM > +# for migration thread > +# > +# @channels: Struct containing list of migration channel types, with all > +# the information regarding destination interfaces required for > +# initiating a migration stream. Please format like # @uri: the Uniform Resource Identifier of the destination VM for # migration thread # # @channels: Struct containing list of migration channel types, with # all the information regarding destination interfaces required # for initiating a migration stream. to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments to conform to current conventions). > # > # @blk: do block migration (full disk copy) > # > @@ -1479,14 +1514,44 @@ > # 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. The 'uri' and 'channel' arguments are mutually exclusive; exactly one > +# of the two should be present. > +# Long lines. Better: # 4. The uri argument should have the Uniform Resource Identifier of # default destination VM. This connection will be bound to default # network # # 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly # one of the two should be present. > # Example: > # > # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } > # <- { "return": {} } > +# -> { "execute": "migrate", > +# "arguments": { > +# "channels": [ { "channeltype": "main", > +# "addr": { "transport": "socket", "type": "inet", > +# "host": "10.12.34.9", > +# "port": "1050" } } ] } } > +# <- { "return": {} } > +# > +# -> { "execute": "migrate", > +# "arguments": { > +# "channels": [ { "channeltype": "main", > +# "addr": { "transport": "exec", > +# "args": [ "/bin/nc", "-p", "6000", > +# "/some/sock" ] } } ] } } > +# <- { "return": {} } > +# > +# -> { "execute": "migrate", > +# "arguments": { > +# "channels": [ { "channeltype": "main", > +# "addr": { "transport": "rdma", > +# "host": "10.12.34.9", > +# "port": "1050" } } ] } } > +# <- { "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: > @@ -1497,6 +1562,10 @@ > # @uri: The Uniform Resource Identifier identifying the source or > # address to listen on > # > +# @channels: Struct containing list of migration channel types, with all > +# the information regarding destination interfaces required for > +# initiating a migration stream. > +# The list doesn't contain migration channel types, it contains migration channels. I'm not sure what you're trying to say by "with all the information ..." What does it mean to have multiple channels? Please format like # @channels: Struct containing list of migration channel types, with # all the information regarding destination interfaces required # for initiating a migration stream. > # Returns: nothing on success > # > # Since: 2.3 > @@ -1512,13 +1581,42 @@ > # > # 3. The uri format is the same as for -incoming > # > +# 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly one > +# of the two should be present. > +# Long line. Better: # 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly # one of the two should be present. > # Example: > # > # -> { "execute": "migrate-incoming", > # "arguments": { "uri": "tcp::4446" } } > # <- { "return": {} } > +# > +# -> { "execute": "migrate", > +# "arguments": { > +# "channels": [ { "channeltype": "main", > +# "addr": { "transport": "socket", "type": "inet", > +# "host": "10.12.34.9", > +# "port": "1050" } } ] } } > +# <- { "return": {} } > +# > +# -> { "execute": "migrate", > +# "arguments": { > +# "channels": [ { "channeltype": "main", > +# "addr": { "transport": "exec", > +# "args": [ "/bin/nc", "-p", "6000", > +# "/some/sock" ] } } ] } } > +# <- { "return": {} } > +# > +# -> { "execute": "migrate", > +# "arguments": { > +# "channels": [ { "channeltype": "main", > +# "addr": { "transport": "rdma", > +# "host": "10.12.34.9", > +# "port": "1050" } } ] } } > +# <- { "return": {} } > ## > -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } } > +{ 'command': 'migrate-incoming', > + 'data': {'*uri': 'str', > + '*channels': [ 'MigrateChannel' ] } } > > ## > # @xen-save-devices-state: The text feels cumbersome. Writing good prose is hard, especially when you're not a native speaker. Eric, would you like to try your hand at polishing this? [...]
On 25/05/23 11:20 pm, Markus Armbruster wrote: > Het Gala <het.gala@nutanix.com> writes: > >> MigrateChannelList allows to connect accross multiple interfaces. Added >> MigrateChannelList struct as argument to migration QAPIs. >> >> Future patchset series plans to include multiple MigrateChannels >> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList' >> is the preferred choice of argument over 'MigrateChannel' and making >> migration QAPIs future proof. >> >> For current patchset series, have limited the size of the list to single >> element (single interface) as runtime check. >> >> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >> Signed-off-by: Het Gala <het.gala@nutanix.com> >> --- > [...] > >> diff --git a/qapi/migration.json b/qapi/migration.json >> index c500744bb7..86bbc916d1 100644 >> --- a/qapi/migration.json >> +++ b/qapi/migration.json >> @@ -1448,12 +1448,47 @@ >> 'exec': 'MigrateExecCommand', >> 'rdma': 'InetSocketAddress' } } >> >> +## >> +# @MigrateChannelType: > As mentioned in my review of PATCH 1, I prefer nouns to verbs for types, > i.e. Migration, not Migrate. More of the same below, not flagging it > again. > Ack. Also, I forgot ot mention in the 1st patch - even for union and struct namings - nouns are preffered over verbs ? or its just for enum types ? We use structs like - MigrateExecCommand, MigrateChannel --> MigrationExecCommand, MigrationChannel ? and union like - MigrateAddress --> MigrationAddress ? >> +# >> +# The supported options for migration channel type requests >> +# >> +# @main: Support request for main outbound migration control channel >> +# >> +# Since 8.1 >> +## >> +{ 'enum': 'MigrateChannelType', >> + 'data': [ 'main' ] } >> + >> +## >> +# @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 > @channel-type, because "channeltype" is not a word. Ack. >> +# >> +# @addr: Information regarding migration parameters of destination interface >> +# >> +# Since 8.1 >> +## >> +{ 'struct': 'MigrateChannel', >> + 'data': { >> + 'channeltype': 'MigrateChannelType', >> + 'addr': 'MigrateAddress' } } >> + >> ## >> # @migrate: >> # >> # Migrates the current running guest to another Virtual Machine. >> # >> # @uri: the Uniform Resource Identifier of the destination VM >> +# for migration thread >> +# >> +# @channels: Struct containing list of migration channel types, with all >> +# the information regarding destination interfaces required for >> +# initiating a migration stream. > Please format like > > # @uri: the Uniform Resource Identifier of the destination VM for > # migration thread > # > # @channels: Struct containing list of migration channel types, with > # all the information regarding destination interfaces required > # for initiating a migration stream. > > to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments > to conform to current conventions). Ack. Will change that in the previous patch and will take care in future patches too. Thanks for informing regarding qapi documentation changes! >> # >> # @blk: do block migration (full disk copy) >> # >> @@ -1479,14 +1514,44 @@ >> # 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. The 'uri' and 'channel' arguments are mutually exclusive; exactly one >> +# of the two should be present. >> +# > Long lines. Better: > > # 4. The uri argument should have the Uniform Resource Identifier of > # default destination VM. This connection will be bound to default > # network > # > # 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly > # one of the two should be present. Ack. >> # Example: >> # >> # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } >> # <- { "return": {} } >> +# -> { "execute": "migrate", >> +# "arguments": { >> +# "channels": [ { "channeltype": "main", >> +# "addr": { "transport": "socket", "type": "inet", >> +# "host": "10.12.34.9", >> +# "port": "1050" } } ] } } >> +# <- { "return": {} } >> +# >> +# -> { "execute": "migrate", >> +# "arguments": { >> +# "channels": [ { "channeltype": "main", >> +# "addr": { "transport": "exec", >> +# "args": [ "/bin/nc", "-p", "6000", >> +# "/some/sock" ] } } ] } } >> +# <- { "return": {} } >> +# >> +# -> { "execute": "migrate", >> +# "arguments": { >> +# "channels": [ { "channeltype": "main", >> +# "addr": { "transport": "rdma", >> +# "host": "10.12.34.9", >> +# "port": "1050" } } ] } } >> +# <- { "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: >> @@ -1497,6 +1562,10 @@ >> # @uri: The Uniform Resource Identifier identifying the source or >> # address to listen on >> # >> +# @channels: Struct containing list of migration channel types, with all >> +# the information regarding destination interfaces required for >> +# initiating a migration stream. >> +# > The list doesn't contain migration channel types, it contains migration > channels. Yes, my bad. Will update it. > I'm not sure what you're trying to say by "with all the information ..." > > What does it mean to have multiple channels? In future patchset series, we will be introducing channels over different interfaces (src-dest pair), with each channel having multiple multifd channels. For now we will restrict the size of the list to 1. > Please format like > > # @channels: Struct containing list of migration channel types, with > # all the information regarding destination interfaces required > # for initiating a migration stream. Ack. >> # Returns: nothing on success >> # >> # Since: 2.3 >> @@ -1512,13 +1581,42 @@ >> # >> # 3. The uri format is the same as for -incoming >> # >> +# 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly one >> +# of the two should be present. >> +# > Long line. Better: > > # 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly > # one of the two should be present. Ack. >> # Example: >> # >> # -> { "execute": "migrate-incoming", >> # "arguments": { "uri": "tcp::4446" } } >> # <- { "return": {} } >> +# >> +# -> { "execute": "migrate", >> +# "arguments": { >> +# "channels": [ { "channeltype": "main", >> +# "addr": { "transport": "socket", "type": "inet", >> +# "host": "10.12.34.9", >> +# "port": "1050" } } ] } } >> +# <- { "return": {} } >> +# >> +# -> { "execute": "migrate", >> +# "arguments": { >> +# "channels": [ { "channeltype": "main", >> +# "addr": { "transport": "exec", >> +# "args": [ "/bin/nc", "-p", "6000", >> +# "/some/sock" ] } } ] } } >> +# <- { "return": {} } >> +# >> +# -> { "execute": "migrate", >> +# "arguments": { >> +# "channels": [ { "channeltype": "main", >> +# "addr": { "transport": "rdma", >> +# "host": "10.12.34.9", >> +# "port": "1050" } } ] } } >> +# <- { "return": {} } >> ## >> -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } } >> +{ 'command': 'migrate-incoming', >> + 'data': {'*uri': 'str', >> + '*channels': [ 'MigrateChannel' ] } } >> >> ## >> # @xen-save-devices-state: > The text feels cumbersome. Writing good prose is hard, especially when > you're not a native speaker. Eric, would you like to try your hand at > polishing this? > [...] Regards, Het Gala
Het Gala <het.gala@nutanix.com> writes: > On 25/05/23 11:20 pm, Markus Armbruster wrote: >> Het Gala <het.gala@nutanix.com> writes: >> >>> MigrateChannelList allows to connect accross multiple interfaces. Added >>> MigrateChannelList struct as argument to migration QAPIs. >>> >>> Future patchset series plans to include multiple MigrateChannels >>> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList' >>> is the preferred choice of argument over 'MigrateChannel' and making >>> migration QAPIs future proof. >>> >>> For current patchset series, have limited the size of the list to single >>> element (single interface) as runtime check. >>> >>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >>> Signed-off-by: Het Gala <het.gala@nutanix.com> >>> --- >> [...] >> >>> diff --git a/qapi/migration.json b/qapi/migration.json >>> index c500744bb7..86bbc916d1 100644 >>> --- a/qapi/migration.json >>> +++ b/qapi/migration.json >>> @@ -1448,12 +1448,47 @@ >>> 'exec': 'MigrateExecCommand', >>> 'rdma': 'InetSocketAddress' } } >>> +## >>> +# @MigrateChannelType: >> >> As mentioned in my review of PATCH 1, I prefer nouns to verbs for types, >> i.e. Migration, not Migrate. More of the same below, not flagging it >> again. >> > Ack. > > Also, I forgot ot mention in the 1st patch - even for union and struct namings - nouns are preffered over verbs ? or its just for enum types ? > We use structs like - MigrateExecCommand, MigrateChannel --> MigrationExecCommand, MigrationChannel ? > and union like - MigrateAddress --> MigrationAddress ? The "types are things, and names of things are nouns" argument applies to all types. Yes, existing names use verbs in places. Mildly annoying. Renaming them would not create compatibility problems, as types are not part of the external interface. Up to migration maintainers. >>> +# >>> +# The supported options for migration channel type requests >>> +# >>> +# @main: Support request for main outbound migration control channel >>> +# >>> +# Since 8.1 >>> +## >>> +{ 'enum': 'MigrateChannelType', >>> + 'data': [ 'main' ] } >>> + >>> +## >>> +# @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 >> >> @channel-type, because "channeltype" is not a word. > > Ack. > >>> +# >>> +# @addr: Information regarding migration parameters of destination interface >>> +# >>> +# Since 8.1 >>> +## >>> +{ 'struct': 'MigrateChannel', >>> + 'data': { >>> + 'channeltype': 'MigrateChannelType', >>> + 'addr': 'MigrateAddress' } } >>> + >>> ## >>> # @migrate: >>> # >>> # Migrates the current running guest to another Virtual Machine. >>> # >>> # @uri: the Uniform Resource Identifier of the destination VM >>> +# for migration thread >>> +# >>> +# @channels: Struct containing list of migration channel types, with all >>> +# the information regarding destination interfaces required for >>> +# initiating a migration stream. >> >> Please format like >> >> # @uri: the Uniform Resource Identifier of the destination VM for >> # migration thread >> # >> # @channels: Struct containing list of migration channel types, with >> # all the information regarding destination interfaces required >> # for initiating a migration stream. >> >> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments >> to conform to current conventions). > > Ack. Will change that in the previous patch and will take care in future patches too. Thanks for informing regarding qapi documentation changes! Gladly! It's the only way to make the nicer formatting stick :) >>> # >>> # @blk: do block migration (full disk copy) >>> # >>> @@ -1479,14 +1514,44 @@ >>> # 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. The 'uri' and 'channel' arguments are mutually exclusive; exactly one >>> +# of the two should be present. >>> +# >> Long lines. Better: >> >> # 4. The uri argument should have the Uniform Resource Identifier of >> # default destination VM. This connection will be bound to default >> # network >> # >> # 5. The 'uri' and 'channel' arguments are mutually exclusive; exactly >> # one of the two should be present. > Ack. >>> # Example: >>> # >>> # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } >>> # <- { "return": {} } >>> +# -> { "execute": "migrate", >>> +# "arguments": { >>> +# "channels": [ { "channeltype": "main", >>> +# "addr": { "transport": "socket", "type": "inet", >>> +# "host": "10.12.34.9", >>> +# "port": "1050" } } ] } } >>> +# <- { "return": {} } >>> +# >>> +# -> { "execute": "migrate", >>> +# "arguments": { >>> +# "channels": [ { "channeltype": "main", >>> +# "addr": { "transport": "exec", >>> +# "args": [ "/bin/nc", "-p", "6000", >>> +# "/some/sock" ] } } ] } } >>> +# <- { "return": {} } >>> +# >>> +# -> { "execute": "migrate", >>> +# "arguments": { >>> +# "channels": [ { "channeltype": "main", >>> +# "addr": { "transport": "rdma", >>> +# "host": "10.12.34.9", >>> +# "port": "1050" } } ] } } >>> +# <- { "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: >>> @@ -1497,6 +1562,10 @@ >>> # @uri: The Uniform Resource Identifier identifying the source or >>> # address to listen on >>> # >>> +# @channels: Struct containing list of migration channel types, with all >>> +# the information regarding destination interfaces required for >>> +# initiating a migration stream. >>> +# >> >> The list doesn't contain migration channel types, it contains migration >> channels. > > Yes, my bad. Will update it. Writing good documentation is hard! >> I'm not sure what you're trying to say by "with all the information ..." >> >> What does it mean to have multiple channels? > > In future patchset series, we will be introducing channels over different interfaces (src-dest pair), with each channel having multiple multifd channels. For now we will restrict the size of the list to 1. Please document this restriction right here. When you add support for multiple channels, will each channel have a unique channel type? [...]
On 30/05/23 12:43 pm, Markus Armbruster wrote: > Het Gala <het.gala@nutanix.com> writes: > >> On 25/05/23 11:20 pm, Markus Armbruster wrote: >>> Het Gala <het.gala@nutanix.com> writes: >>> >>>> MigrateChannelList allows to connect accross multiple interfaces. Added >>>> MigrateChannelList struct as argument to migration QAPIs. >>>> >>>> Future patchset series plans to include multiple MigrateChannels >>>> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList' >>>> is the preferred choice of argument over 'MigrateChannel' and making >>>> migration QAPIs future proof. >>>> >>>> For current patchset series, have limited the size of the list to single >>>> element (single interface) as runtime check. >>>> >>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >>>> Signed-off-by: Het Gala <het.gala@nutanix.com> >>>> --- >>> [...] >>> >>>> diff --git a/qapi/migration.json b/qapi/migration.json >>>> index c500744bb7..86bbc916d1 100644 >>>> --- a/qapi/migration.json >>>> +++ b/qapi/migration.json >>>> @@ -1448,12 +1448,47 @@ >>>> 'exec': 'MigrateExecCommand', >>>> 'rdma': 'InetSocketAddress' } } >>>> +## >>>> +# @MigrateChannelType: >>> As mentioned in my review of PATCH 1, I prefer nouns to verbs for types, >>> i.e. Migration, not Migrate. More of the same below, not flagging it >>> again. >>> >> Ack. >> >> Also, I forgot ot mention in the 1st patch - even for union and struct namings - nouns are preffered over verbs ? or its just for enum types ? >> We use structs like - MigrateExecCommand, MigrateChannel --> MigrationExecCommand, MigrationChannel ? >> and union like - MigrateAddress --> MigrationAddress ? > The "types are things, and names of things are nouns" argument applies > to all types. > > Yes, existing names use verbs in places. Mildly annoying. > > Renaming them would not create compatibility problems, as types are not > part of the external interface. Up to migration maintainers. Yes, I got your point. Will change naming convention for all 'types' as nouns other than 'command' which will be a verb. Will kep this in mind in future too. Thanks! >>>> +# >>>> +# The supported options for migration channel type requests >>>> +# >>>> +# @main: Support request for main outbound migration control channel >>>> +# >>>> +# Since 8.1 >>>> +## >>>> +{ 'enum': 'MigrateChannelType', >>>> + 'data': [ 'main' ] } >>>> + >>>> +## >>>> +# @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 >>> @channel-type, because "channeltype" is not a word. >> Ack. >> >>>> +# >>>> +# @addr: Information regarding migration parameters of destination interface >>>> +# >>>> +# Since 8.1 >>>> +## >>>> +{ 'struct': 'MigrateChannel', >>>> + 'data': { >>>> + 'channeltype': 'MigrateChannelType', >>>> + 'addr': 'MigrateAddress' } } >>>> + >>>> ## >>>> # @migrate: >>>> # >>>> # Migrates the current running guest to another Virtual Machine. >>>> # >>>> # @uri: the Uniform Resource Identifier of the destination VM >>>> +# for migration thread >>>> +# >>>> +# @channels: Struct containing list of migration channel types, with all >>>> +# the information regarding destination interfaces required for >>>> +# initiating a migration stream. >>> Please format like >>> >>> # @uri: the Uniform Resource Identifier of the destination VM for >>> # migration thread >>> # >>> # @channels: Struct containing list of migration channel types, with >>> # all the information regarding destination interfaces required >>> # for initiating a migration stream. >>> >>> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments >>> to conform to current conventions). >> Ack. Will change that in the previous patch and will take care in future patches too. Thanks for informing regarding qapi documentation changes! > Gladly! It's the only way to make the nicer formatting stick :) Yes
Het Gala <het.gala@nutanix.com> writes: > On 30/05/23 12:43 pm, Markus Armbruster wrote: >> Het Gala <het.gala@nutanix.com> writes: >> >>> On 25/05/23 11:20 pm, Markus Armbruster wrote: >>>> Het Gala <het.gala@nutanix.com> writes: >>>> >>>>> MigrateChannelList allows to connect accross multiple interfaces. Added >>>>> MigrateChannelList struct as argument to migration QAPIs. >>>>> >>>>> Future patchset series plans to include multiple MigrateChannels >>>>> for multiple interfaces to be connected. That is the reason, 'MigrateChannelList' >>>>> is the preferred choice of argument over 'MigrateChannel' and making >>>>> migration QAPIs future proof. >>>>> >>>>> For current patchset series, have limited the size of the list to single >>>>> element (single interface) as runtime check. >>>>> >>>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> >>>>> Signed-off-by: Het Gala <het.gala@nutanix.com> [...] >>>>> @@ -1497,6 +1562,10 @@ >>>>> # @uri: The Uniform Resource Identifier identifying the source or >>>>> # address to listen on >>>>> # >>>>> +# @channels: Struct containing list of migration channel types, with all >>>>> +# the information regarding destination interfaces required for >>>>> +# initiating a migration stream. >>>>> +# >>>> The list doesn't contain migration channel types, it contains migration >>>> channels. >>> >>> Yes, my bad. Will update it. >> >> Writing good documentation is hard! >> >>>> I'm not sure what you're trying to say by "with all the information ..." >>>> >>>> What does it mean to have multiple channels? >>> >>> In future patchset series, we will be introducing channels over different interfaces (src-dest pair), with each channel having multiple multifd channels. For now we will restrict the size of the list to 1. >> >> Please document this restriction right here. > > Ack. But it is mainly an implementation point that's the reason I did not mention it here but have mentioned it in the migration code flow path. Will add one more point to note down. Documenting temporary restrictions is work that's useful only temporarily. When a restriction goes away within the same patch series, not doing that work can make sense. I'd still want it mentioned in the commit message. It's tempting treat restrictions expected to go away before we release the same. But when lifting the restriction misses the release, it's easy to forget we still have a restriction to document. Better document it right away. >> When you add support for multiple channels, will each channel have a >> unique channel type? > > Not every channel will have a unique channel type but mixture like, for multifd in future : (main, data, data, data) --> the first connection will be migration main connection, other three will be multifd connections. Got it, thanks!
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 9885d7c9f7..8ddfa258ad 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -423,10 +423,12 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict) { Error *err = NULL; const char *uri = qdict_get_str(qdict, "uri"); + MigrateChannelList *caps = NULL; + g_autoptr(MigrateChannel) channel = g_new0(MigrateChannel, 1); - qmp_migrate_incoming(uri, &err); - - hmp_handle_error(mon, err); + QAPI_LIST_PREPEND(caps, channel); + qmp_migrate_incoming(uri, false, caps, &err); + qapi_free_MigrateChannelList(caps); } void hmp_migrate_recover(Monitor *mon, const QDict *qdict) @@ -704,9 +706,13 @@ void hmp_migrate(Monitor *mon, const QDict *qdict) bool resume = qdict_get_try_bool(qdict, "resume", false); const char *uri = qdict_get_str(qdict, "uri"); Error *err = NULL; + MigrateChannelList *caps = NULL; + g_autoptr(MigrateChannel) channel = g_new0(MigrateChannel, 1); - qmp_migrate(uri, !!blk, blk, !!inc, inc, - false, false, true, resume, &err); + QAPI_LIST_PREPEND(caps, channel); + qmp_migrate(uri, false, caps, !!blk, blk, !!inc, inc, + false, false, true, resume, &err); + qapi_free_MigrateChannelList(caps); if (hmp_handle_error(mon, err)) { return; } diff --git a/migration/migration.c b/migration/migration.c index 0a6ab9229b..abccc6bf26 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -463,10 +463,22 @@ static bool migrate_uri_parse(const char *uri, MigrateAddress **channel, return true; } -static void qemu_start_incoming_migration(const char *uri, Error **errp) +static void qemu_start_incoming_migration(const char *uri, bool has_channels, + MigrateChannelList *channels, + Error **errp) { g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1); + /* + * Having preliminary checks for uri and channel + */ + if (uri && has_channels) { + error_setg(errp, "'uri' and 'channels' arguments are mutually " + "exclusive; exactly one of the two should be present in " + "'migrate-incoming' qmp command "); + return; + } + /* URI is not suitable for migration? */ if (!migration_channels_and_uri_compatible(uri, errp)) { return; @@ -1488,7 +1500,8 @@ void migrate_del_blocker(Error *reason) migration_blockers = g_slist_remove(migration_blockers, reason); } -void qmp_migrate_incoming(const char *uri, Error **errp) +void qmp_migrate_incoming(const char *uri, bool has_channels, + MigrateChannelList *channels, Error **errp) { Error *local_err = NULL; static bool once = true; @@ -1506,7 +1519,7 @@ void qmp_migrate_incoming(const char *uri, Error **errp) return; } - qemu_start_incoming_migration(uri, &local_err); + qemu_start_incoming_migration(uri, has_channels, channels, &local_err); if (local_err) { yank_unregister_instance(MIGRATION_YANK_INSTANCE); @@ -1542,7 +1555,7 @@ void qmp_migrate_recover(const char *uri, Error **errp) * only re-setup the migration stream and poke existing migration * to continue using that newly established channel. */ - qemu_start_incoming_migration(uri, errp); + qemu_start_incoming_migration(uri, false, NULL, errp); } void qmp_migrate_pause(Error **errp) @@ -1675,14 +1688,25 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc, return true; } -void qmp_migrate(const char *uri, bool has_blk, bool blk, - bool has_inc, bool inc, bool has_detach, bool detach, - bool has_resume, bool resume, Error **errp) +void qmp_migrate(const char *uri, bool has_channels, + MigrateChannelList *channels, bool has_blk, bool blk, + bool has_inc, bool inc, bool has_detach, + bool detach, bool has_resume, bool resume, Error **errp) { Error *local_err = NULL; MigrationState *s = migrate_get_current(); g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1); + /* + * Having preliminary checks for uri and channel + */ + if (uri && has_channels) { + error_setg(errp, "'uri' and 'channels' arguments are mutually " + "exclusive; exactly one of the two should be present in " + "'migrate' qmp command "); + return; + } + /* URI is not suitable for migration? */ if (!migration_channels_and_uri_compatible(uri, errp)) { return; diff --git a/qapi/migration.json b/qapi/migration.json index c500744bb7..86bbc916d1 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1448,12 +1448,47 @@ 'exec': 'MigrateExecCommand', 'rdma': 'InetSocketAddress' } } +## +# @MigrateChannelType: +# +# The supported options for migration channel type requests +# +# @main: Support request for main outbound migration control channel +# +# Since 8.1 +## +{ 'enum': 'MigrateChannelType', + 'data': [ 'main' ] } + +## +# @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 +# +# @addr: Information regarding migration parameters of destination interface +# +# Since 8.1 +## +{ 'struct': 'MigrateChannel', + 'data': { + 'channeltype': 'MigrateChannelType', + 'addr': 'MigrateAddress' } } + ## # @migrate: # # Migrates the current running guest to another Virtual Machine. # # @uri: the Uniform Resource Identifier of the destination VM +# for migration thread +# +# @channels: Struct containing list of migration channel types, with all +# the information regarding destination interfaces required for +# initiating a migration stream. # # @blk: do block migration (full disk copy) # @@ -1479,14 +1514,44 @@ # 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. The 'uri' and 'channel' arguments are mutually exclusive; exactly one +# of the two should be present. +# # Example: # # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } } # <- { "return": {} } +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channeltype": "main", +# "addr": { "transport": "socket", "type": "inet", +# "host": "10.12.34.9", +# "port": "1050" } } ] } } +# <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channeltype": "main", +# "addr": { "transport": "exec", +# "args": [ "/bin/nc", "-p", "6000", +# "/some/sock" ] } } ] } } +# <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channeltype": "main", +# "addr": { "transport": "rdma", +# "host": "10.12.34.9", +# "port": "1050" } } ] } } +# <- { "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: @@ -1497,6 +1562,10 @@ # @uri: The Uniform Resource Identifier identifying the source or # address to listen on # +# @channels: Struct containing list of migration channel types, with all +# the information regarding destination interfaces required for +# initiating a migration stream. +# # Returns: nothing on success # # Since: 2.3 @@ -1512,13 +1581,42 @@ # # 3. The uri format is the same as for -incoming # +# 4. The 'uri' and 'channel' arguments are mutually exclusive; exactly one +# of the two should be present. +# # Example: # # -> { "execute": "migrate-incoming", # "arguments": { "uri": "tcp::4446" } } # <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channeltype": "main", +# "addr": { "transport": "socket", "type": "inet", +# "host": "10.12.34.9", +# "port": "1050" } } ] } } +# <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channeltype": "main", +# "addr": { "transport": "exec", +# "args": [ "/bin/nc", "-p", "6000", +# "/some/sock" ] } } ] } } +# <- { "return": {} } +# +# -> { "execute": "migrate", +# "arguments": { +# "channels": [ { "channeltype": "main", +# "addr": { "transport": "rdma", +# "host": "10.12.34.9", +# "port": "1050" } } ] } } +# <- { "return": {} } ## -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } } +{ 'command': 'migrate-incoming', + 'data': {'*uri': 'str', + '*channels': [ 'MigrateChannel' ] } } ## # @xen-save-devices-state: diff --git a/softmmu/vl.c b/softmmu/vl.c index 6c2427262b..ade411eb4f 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2633,7 +2633,7 @@ void qmp_x_exit_preconfig(Error **errp) if (incoming) { Error *local_err = NULL; if (strcmp(incoming, "defer") != 0) { - qmp_migrate_incoming(incoming, &local_err); + qmp_migrate_incoming(incoming, false, NULL, &local_err); if (local_err) { error_reportf_err(local_err, "-incoming %s: ", incoming); exit(1);
MigrateChannelList allows to connect accross multiple interfaces. Added MigrateChannelList struct as argument to migration QAPIs. Future patchset series plans to include multiple MigrateChannels for multiple interfaces to be connected. That is the reason, 'MigrateChannelList' is the preferred choice of argument over 'MigrateChannel' and making migration QAPIs future proof. For current patchset series, have limited the size of the list to single element (single interface) as runtime check. Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> Signed-off-by: Het Gala <het.gala@nutanix.com> --- migration/migration-hmp-cmds.c | 16 +++-- migration/migration.c | 38 +++++++++--- qapi/migration.json | 104 ++++++++++++++++++++++++++++++++- softmmu/vl.c | 2 +- 4 files changed, 144 insertions(+), 16 deletions(-)