Introduce a new MigrationConfigBase, to allow most of the duplication
in migration.json to be eliminated.
The reason we need MigrationParameters and MigrationSetParameters is
that the internal parameter representation in the migration code, as
well as the user-facing return of query-migrate-parameters use one
type for the TLS options (tls-creds, tls-hostname, tls-authz), while
the user-facing input from migrate-set-parameters uses another.
The difference is in whether the NULL values is accepted. The former
considers NULL as invalid, while the latter doesn't.
Move all other (non-TLS) options into the new type and make it a base
type for the two diverging types so that each child type can declare
the TLS options in its own way.
Nothing changes in the user API, nothing changes in the internal
representation, but we save several lines of duplication in
migration.json.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
qapi/migration.json | 358 +++++++++++++-------------------------------
1 file changed, 108 insertions(+), 250 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index 8b9c53595c..5a4d5a2d3e 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -914,202 +914,6 @@
'zero-page-detection',
'direct-io'] }
-##
-# @MigrateSetParameters:
-#
-# @announce-initial: Initial delay (in milliseconds) before sending
-# the first announce (Since 4.0)
-#
-# @announce-max: Maximum delay (in milliseconds) between packets in
-# the announcement (Since 4.0)
-#
-# @announce-rounds: Number of self-announce packets sent after
-# migration (Since 4.0)
-#
-# @announce-step: Increase in delay (in milliseconds) between
-# subsequent packets in the announcement (Since 4.0)
-#
-# @throttle-trigger-threshold: The ratio of bytes_dirty_period and
-# bytes_xfer_period to trigger throttling. It is expressed as
-# percentage. The default value is 50. (Since 5.0)
-#
-# @cpu-throttle-initial: Initial percentage of time guest cpus are
-# throttled when migration auto-converge is activated. The
-# default value is 20. (Since 2.7)
-#
-# @cpu-throttle-increment: throttle percentage increase each time
-# auto-converge detects that migration is not making progress.
-# The default value is 10. (Since 2.7)
-#
-# @cpu-throttle-tailslow: Make CPU throttling slower at tail stage At
-# the tail stage of throttling, the Guest is very sensitive to CPU
-# percentage while the @cpu-throttle -increment is excessive
-# usually at tail stage. If this parameter is true, we will
-# compute the ideal CPU percentage used by the Guest, which may
-# exactly make the dirty rate match the dirty rate threshold.
-# Then we will choose a smaller throttle increment between the one
-# specified by @cpu-throttle-increment and the one generated by
-# ideal CPU percentage. Therefore, it is compatible to
-# traditional throttling, meanwhile the throttle increment won't
-# be excessive at tail stage. The default value is false. (Since
-# 5.1)
-#
-# @tls-creds: ID of the 'tls-creds' object that provides credentials
-# for establishing a TLS connection over the migration data
-# channel. On the outgoing side of the migration, the credentials
-# must be for a 'client' endpoint, while for the incoming side the
-# credentials must be for a 'server' endpoint. Setting this to a
-# non-empty string enables TLS for all migrations. An empty
-# string means that QEMU will use plain text mode for migration,
-# rather than TLS. This is the default. (Since 2.7)
-#
-# @tls-hostname: migration target's hostname for validating the
-# server's x509 certificate identity. If empty, QEMU will use the
-# hostname from the migration URI, if any. A non-empty value is
-# required when using x509 based TLS credentials and the migration
-# URI does not include a hostname, such as fd: or exec: based
-# migration. (Since 2.7)
-#
-# Note: empty value works only since 2.9.
-#
-# @tls-authz: ID of the 'authz' object subclass that provides access
-# control checking of the TLS x509 certificate distinguished name.
-# This object is only resolved at time of use, so can be deleted
-# and recreated on the fly while the migration server is active.
-# If missing, it will default to denying access (Since 4.0)
-#
-# @max-bandwidth: maximum speed for migration, in bytes per second.
-# (Since 2.8)
-#
-# @avail-switchover-bandwidth: to set the available bandwidth that
-# migration can use during switchover phase. NOTE! This does not
-# limit the bandwidth during switchover, but only for calculations
-# when making decisions to switchover. By default, this value is
-# zero, which means QEMU will estimate the bandwidth
-# automatically. This can be set when the estimated value is not
-# accurate, while the user is able to guarantee such bandwidth is
-# available when switching over. When specified correctly, this
-# can make the switchover decision much more accurate.
-# (Since 8.2)
-#
-# @downtime-limit: set maximum tolerated downtime for migration.
-# maximum downtime in milliseconds (Since 2.8)
-#
-# @x-checkpoint-delay: The delay time (in ms) between two COLO
-# checkpoints in periodic mode. (Since 2.8)
-#
-# @multifd-channels: Number of channels used to migrate data in
-# parallel. This is the same number that the number of sockets
-# used for migration. The default value is 2 (since 4.0)
-#
-# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
-# needs to be a multiple of the target page size and a power of 2
-# (Since 2.11)
-#
-# @max-postcopy-bandwidth: Background transfer bandwidth during
-# postcopy. Defaults to 0 (unlimited). In bytes per second.
-# (Since 3.0)
-#
-# @max-cpu-throttle: maximum cpu throttle percentage. Defaults to 99.
-# (Since 3.1)
-#
-# @multifd-compression: Which compression method to use. Defaults to
-# none. (Since 5.0)
-#
-# @multifd-zlib-level: Set the compression level to be used in live
-# migration, the compression level is an integer between 0 and 9,
-# where 0 means no compression, 1 means the best compression
-# speed, and 9 means best compression ratio which will consume
-# more CPU. Defaults to 1. (Since 5.0)
-#
-# @multifd-qatzip-level: Set the compression level to be used in live
-# migration. The level is an integer between 1 and 9, where 1 means
-# the best compression speed, and 9 means the best compression
-# ratio which will consume more CPU. Defaults to 1. (Since 9.2)
-#
-# @multifd-zstd-level: Set the compression level to be used in live
-# migration, the compression level is an integer between 0 and 20,
-# where 0 means no compression, 1 means the best compression
-# speed, and 20 means best compression ratio which will consume
-# more CPU. Defaults to 1. (Since 5.0)
-#
-# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
-# aliases for the purpose of dirty bitmap migration. Such aliases
-# may for example be the corresponding names on the opposite site.
-# The mapping must be one-to-one, but not necessarily complete: On
-# the source, unmapped bitmaps and all bitmaps on unmapped nodes
-# will be ignored. On the destination, encountering an unmapped
-# alias in the incoming migration stream will result in a report,
-# and all further bitmap migration data will then be discarded.
-# Note that the destination does not know about bitmaps it does
-# not receive, so there is no limitation or requirement regarding
-# the number of bitmaps received, or how they are named, or on
-# which nodes they are placed. By default (when this parameter
-# has never been set), bitmap names are mapped to themselves.
-# Nodes are mapped to their block device name if there is one, and
-# to their node name otherwise. (Since 5.2)
-#
-# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
-# limit during live migration. Should be in the range 1 to
-# 1000ms. Defaults to 1000ms. (Since 8.1)
-#
-# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration.
-# Defaults to 1. (Since 8.1)
-#
-# @mode: Migration mode. See description in @MigMode. Default is
-# 'normal'. (Since 8.2)
-#
-# @zero-page-detection: Whether and how to detect zero pages.
-# See description in @ZeroPageDetection. Default is 'multifd'.
-# (since 9.0)
-#
-# @direct-io: Open migration files with O_DIRECT when possible. This
-# only has effect if the @mapped-ram capability is enabled.
-# (Since 9.1)
-#
-# Features:
-#
-# @unstable: Members @x-checkpoint-delay and
-# @x-vcpu-dirty-limit-period are experimental.
-#
-# TODO: either fuse back into MigrationParameters, or make
-# MigrationParameters members mandatory
-#
-# Since: 2.4
-##
-{ 'struct': 'MigrateSetParameters',
- 'data': { '*announce-initial': 'size',
- '*announce-max': 'size',
- '*announce-rounds': 'size',
- '*announce-step': 'size',
- '*throttle-trigger-threshold': 'uint8',
- '*cpu-throttle-initial': 'uint8',
- '*cpu-throttle-increment': 'uint8',
- '*cpu-throttle-tailslow': 'bool',
- '*tls-creds': 'StrOrNull',
- '*tls-hostname': 'StrOrNull',
- '*tls-authz': 'StrOrNull',
- '*max-bandwidth': 'size',
- '*avail-switchover-bandwidth': 'size',
- '*downtime-limit': 'uint64',
- '*x-checkpoint-delay': { 'type': 'uint32',
- 'features': [ 'unstable' ] },
- '*multifd-channels': 'uint8',
- '*xbzrle-cache-size': 'size',
- '*max-postcopy-bandwidth': 'size',
- '*max-cpu-throttle': 'uint8',
- '*multifd-compression': 'MultiFDCompression',
- '*multifd-zlib-level': 'uint8',
- '*multifd-qatzip-level': 'uint8',
- '*multifd-zstd-level': 'uint8',
- '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
- '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
- 'features': [ 'unstable' ] },
- '*vcpu-dirty-limit': 'uint64',
- '*mode': 'MigMode',
- '*zero-page-detection': 'ZeroPageDetection',
- '*direct-io': 'bool' } }
-
##
# @migrate-set-parameters:
#
@@ -1127,9 +931,7 @@
'data': 'MigrateSetParameters' }
##
-# @MigrationParameters:
-#
-# The optional members aren't actually optional.
+# @MigrationConfigBase:
#
# @announce-initial: Initial delay (in milliseconds) before sending
# the first announce (Since 4.0)
@@ -1168,26 +970,6 @@
# be excessive at tail stage. The default value is false. (Since
# 5.1)
#
-# @tls-creds: ID of the 'tls-creds' object that provides credentials
-# for establishing a TLS connection over the migration data
-# channel. On the outgoing side of the migration, the credentials
-# must be for a 'client' endpoint, while for the incoming side the
-# credentials must be for a 'server' endpoint. An empty string
-# means that QEMU will use plain text mode for migration, rather
-# than TLS. (Since 2.7)
-#
-# Note: 2.8 omits empty @tls-creds instead.
-#
-# @tls-hostname: migration target's hostname for validating the
-# server's x509 certificate identity. If empty, QEMU will use the
-# hostname from the migration URI, if any. (Since 2.7)
-#
-# Note: 2.8 omits empty @tls-hostname instead.
-#
-# @tls-authz: ID of the 'authz' object subclass that provides access
-# control checking of the TLS x509 certificate distinguished name.
-# (Since 4.0)
-#
# @max-bandwidth: maximum speed for migration, in bytes per second.
# (Since 2.8)
#
@@ -1277,45 +1059,121 @@
# only has effect if the @mapped-ram capability is enabled.
# (Since 9.1)
#
+# @tls: Whether to use TLS. If this is set the options @tls-authz,
+# @tls-creds, @tls-hostname are mandatory and a valid string is
+# expected. (Since 10.1)
+#
# Features:
#
# @unstable: Members @x-checkpoint-delay and
# @x-vcpu-dirty-limit-period are experimental.
#
+# Since: 10.1
+##
+{ 'struct': 'MigrationConfigBase',
+ 'data': { '*announce-initial': 'size',
+ '*announce-max': 'size',
+ '*announce-rounds': 'size',
+ '*announce-step': 'size',
+ '*throttle-trigger-threshold': 'uint8',
+ '*cpu-throttle-initial': 'uint8',
+ '*cpu-throttle-increment': 'uint8',
+ '*cpu-throttle-tailslow': 'bool',
+ '*max-bandwidth': 'size',
+ '*avail-switchover-bandwidth': 'size',
+ '*downtime-limit': 'uint64',
+ '*x-checkpoint-delay': { 'type': 'uint32',
+ 'features': [ 'unstable' ] },
+ '*multifd-channels': 'uint8',
+ '*xbzrle-cache-size': 'size',
+ '*max-postcopy-bandwidth': 'size',
+ '*max-cpu-throttle': 'uint8',
+ '*multifd-compression': 'MultiFDCompression',
+ '*multifd-zlib-level': 'uint8',
+ '*multifd-qatzip-level': 'uint8',
+ '*multifd-zstd-level': 'uint8',
+ '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
+ '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
+ 'features': [ 'unstable' ] },
+ '*vcpu-dirty-limit': 'uint64',
+ '*mode': 'MigMode',
+ '*zero-page-detection': 'ZeroPageDetection',
+ '*direct-io': 'bool',
+ '*tls': 'bool' } }
+
+##
+# @MigrationParameters:
+#
+# The optional members of the base type aren't actually optional.
+#
+# @tls-creds: ID of the 'tls-creds' object that provides credentials
+# for establishing a TLS connection over the migration data
+# channel. On the outgoing side of the migration, the credentials
+# must be for a 'client' endpoint, while for the incoming side the
+# credentials must be for a 'server' endpoint. Setting this to a
+# non-empty string enables TLS for all migrations. An empty
+# string means that QEMU will use plain text mode for migration,
+# rather than TLS. (Since 2.7)
+#
+# @tls-hostname: migration target's hostname for validating the
+# server's x509 certificate identity. If empty, QEMU will use the
+# hostname from the migration URI, if any. A non-empty value is
+# required when using x509 based TLS credentials and the migration
+# URI does not include a hostname, such as fd: or exec: based
+# migration. (Since 2.7)
+#
+# Note: empty value works only since 2.9.
+#
+# @tls-authz: ID of the 'authz' object subclass that provides access
+# control checking of the TLS x509 certificate distinguished name.
+# This object is only resolved at time of use, so can be deleted
+# and recreated on the fly while the migration server is active.
+# If missing, it will default to denying access (Since 4.0)
+#
# Since: 2.4
##
{ 'struct': 'MigrationParameters',
- 'data': { '*announce-initial': 'size',
- '*announce-max': 'size',
- '*announce-rounds': 'size',
- '*announce-step': 'size',
- '*throttle-trigger-threshold': 'uint8',
- '*cpu-throttle-initial': 'uint8',
- '*cpu-throttle-increment': 'uint8',
- '*cpu-throttle-tailslow': 'bool',
- '*tls-creds': 'str',
- '*tls-hostname': 'str',
- '*tls-authz': 'str',
- '*max-bandwidth': 'size',
- '*avail-switchover-bandwidth': 'size',
- '*downtime-limit': 'uint64',
- '*x-checkpoint-delay': { 'type': 'uint32',
- 'features': [ 'unstable' ] },
- '*multifd-channels': 'uint8',
- '*xbzrle-cache-size': 'size',
- '*max-postcopy-bandwidth': 'size',
- '*max-cpu-throttle': 'uint8',
- '*multifd-compression': 'MultiFDCompression',
- '*multifd-zlib-level': 'uint8',
- '*multifd-qatzip-level': 'uint8',
- '*multifd-zstd-level': 'uint8',
- '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
- '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
- 'features': [ 'unstable' ] },
- '*vcpu-dirty-limit': 'uint64',
- '*mode': 'MigMode',
- '*zero-page-detection': 'ZeroPageDetection',
- '*direct-io': 'bool' } }
+ 'base': 'MigrationConfigBase',
+ 'data': { 'tls-creds': 'str',
+ 'tls-hostname': 'str',
+ 'tls-authz': 'str' } }
+
+##
+# @MigrateSetParameters:
+#
+# Compatibility layer to accept null values for the TLS options.
+#
+# @tls-creds: ID of the 'tls-creds' object that provides credentials
+# for establishing a TLS connection over the migration data
+# channel. On the outgoing side of the migration, the credentials
+# must be for a 'client' endpoint, while for the incoming side the
+# credentials must be for a 'server' endpoint. Setting this to a
+# non-empty string enables TLS for all migrations. An empty
+# string means that QEMU will use plain text mode for migration,
+# rather than TLS. This is the default. (Since 2.7)
+#
+# @tls-hostname: migration target's hostname for validating the
+# server's x509 certificate identity. If empty, QEMU will use the
+# hostname from the migration URI, if any. A non-empty value is
+# required when using x509 based TLS credentials and the migration
+# URI does not include a hostname, such as fd: or exec: based
+# migration. (Since 2.7)
+#
+# Note: empty value works only since 2.9.
+#
+# @tls-authz: ID of the 'authz' object subclass that provides access
+# control checking of the TLS x509 certificate distinguished name.
+# This object is only resolved at time of use, so can be deleted
+# and recreated on the fly while the migration server is active.
+# If missing, it will default to denying access (Since 4.0)
+#
+# Since: 2.4
+##
+{ 'struct': 'MigrateSetParameters',
+ 'base': 'MigrationConfigBase',
+ 'data': { '*tls-creds': 'StrOrNull',
+ '*tls-hostname': 'StrOrNull',
+ '*tls-authz': 'StrOrNull' } }
##
# @query-migrate-parameters:
--
2.35.3Fabiano Rosas <farosas@suse.de> writes:
> Introduce a new MigrationConfigBase, to allow most of the duplication
> in migration.json to be eliminated.
>
> The reason we need MigrationParameters and MigrationSetParameters is
> that the internal parameter representation in the migration code, as
> well as the user-facing return of query-migrate-parameters use one
> type for the TLS options (tls-creds, tls-hostname, tls-authz), while
> the user-facing input from migrate-set-parameters uses another.
>
> The difference is in whether the NULL values is accepted. The former
> considers NULL as invalid, while the latter doesn't.
>
> Move all other (non-TLS) options into the new type and make it a base
> type for the two diverging types so that each child type can declare
> the TLS options in its own way.
>
> Nothing changes in the user API, nothing changes in the internal
> representation, but we save several lines of duplication in
> migration.json.
I double-checked the external interface. There's a new member @tls, but
you already told us it's an accident.
Documentation does change. More on that below.
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> qapi/migration.json | 358 +++++++++++++-------------------------------
> 1 file changed, 108 insertions(+), 250 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 8b9c53595c..5a4d5a2d3e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -914,202 +914,6 @@
> 'zero-page-detection',
> 'direct-io'] }
>
> -##
> -# @MigrateSetParameters:
> -#
> -# @announce-initial: Initial delay (in milliseconds) before sending
> -# the first announce (Since 4.0)
> -#
> -# @announce-max: Maximum delay (in milliseconds) between packets in
> -# the announcement (Since 4.0)
> -#
> -# @announce-rounds: Number of self-announce packets sent after
> -# migration (Since 4.0)
> -#
> -# @announce-step: Increase in delay (in milliseconds) between
> -# subsequent packets in the announcement (Since 4.0)
> -#
> -# @throttle-trigger-threshold: The ratio of bytes_dirty_period and
> -# bytes_xfer_period to trigger throttling. It is expressed as
> -# percentage. The default value is 50. (Since 5.0)
> -#
> -# @cpu-throttle-initial: Initial percentage of time guest cpus are
> -# throttled when migration auto-converge is activated. The
> -# default value is 20. (Since 2.7)
> -#
> -# @cpu-throttle-increment: throttle percentage increase each time
> -# auto-converge detects that migration is not making progress.
> -# The default value is 10. (Since 2.7)
> -#
> -# @cpu-throttle-tailslow: Make CPU throttling slower at tail stage At
> -# the tail stage of throttling, the Guest is very sensitive to CPU
> -# percentage while the @cpu-throttle -increment is excessive
> -# usually at tail stage. If this parameter is true, we will
> -# compute the ideal CPU percentage used by the Guest, which may
> -# exactly make the dirty rate match the dirty rate threshold.
> -# Then we will choose a smaller throttle increment between the one
> -# specified by @cpu-throttle-increment and the one generated by
> -# ideal CPU percentage. Therefore, it is compatible to
> -# traditional throttling, meanwhile the throttle increment won't
> -# be excessive at tail stage. The default value is false. (Since
> -# 5.1)
> -#
> -# @tls-creds: ID of the 'tls-creds' object that provides credentials
> -# for establishing a TLS connection over the migration data
> -# channel. On the outgoing side of the migration, the credentials
> -# must be for a 'client' endpoint, while for the incoming side the
> -# credentials must be for a 'server' endpoint. Setting this to a
> -# non-empty string enables TLS for all migrations. An empty
> -# string means that QEMU will use plain text mode for migration,
> -# rather than TLS. This is the default. (Since 2.7)
> -#
> -# @tls-hostname: migration target's hostname for validating the
> -# server's x509 certificate identity. If empty, QEMU will use the
> -# hostname from the migration URI, if any. A non-empty value is
> -# required when using x509 based TLS credentials and the migration
> -# URI does not include a hostname, such as fd: or exec: based
> -# migration. (Since 2.7)
> -#
> -# Note: empty value works only since 2.9.
> -#
> -# @tls-authz: ID of the 'authz' object subclass that provides access
> -# control checking of the TLS x509 certificate distinguished name.
> -# This object is only resolved at time of use, so can be deleted
> -# and recreated on the fly while the migration server is active.
> -# If missing, it will default to denying access (Since 4.0)
> -#
> -# @max-bandwidth: maximum speed for migration, in bytes per second.
> -# (Since 2.8)
> -#
> -# @avail-switchover-bandwidth: to set the available bandwidth that
> -# migration can use during switchover phase. NOTE! This does not
> -# limit the bandwidth during switchover, but only for calculations
> -# when making decisions to switchover. By default, this value is
> -# zero, which means QEMU will estimate the bandwidth
> -# automatically. This can be set when the estimated value is not
> -# accurate, while the user is able to guarantee such bandwidth is
> -# available when switching over. When specified correctly, this
> -# can make the switchover decision much more accurate.
> -# (Since 8.2)
> -#
> -# @downtime-limit: set maximum tolerated downtime for migration.
> -# maximum downtime in milliseconds (Since 2.8)
> -#
> -# @x-checkpoint-delay: The delay time (in ms) between two COLO
> -# checkpoints in periodic mode. (Since 2.8)
> -#
> -# @multifd-channels: Number of channels used to migrate data in
> -# parallel. This is the same number that the number of sockets
> -# used for migration. The default value is 2 (since 4.0)
> -#
> -# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
> -# needs to be a multiple of the target page size and a power of 2
> -# (Since 2.11)
> -#
> -# @max-postcopy-bandwidth: Background transfer bandwidth during
> -# postcopy. Defaults to 0 (unlimited). In bytes per second.
> -# (Since 3.0)
> -#
> -# @max-cpu-throttle: maximum cpu throttle percentage. Defaults to 99.
> -# (Since 3.1)
> -#
> -# @multifd-compression: Which compression method to use. Defaults to
> -# none. (Since 5.0)
> -#
> -# @multifd-zlib-level: Set the compression level to be used in live
> -# migration, the compression level is an integer between 0 and 9,
> -# where 0 means no compression, 1 means the best compression
> -# speed, and 9 means best compression ratio which will consume
> -# more CPU. Defaults to 1. (Since 5.0)
> -#
> -# @multifd-qatzip-level: Set the compression level to be used in live
> -# migration. The level is an integer between 1 and 9, where 1 means
> -# the best compression speed, and 9 means the best compression
> -# ratio which will consume more CPU. Defaults to 1. (Since 9.2)
> -#
> -# @multifd-zstd-level: Set the compression level to be used in live
> -# migration, the compression level is an integer between 0 and 20,
> -# where 0 means no compression, 1 means the best compression
> -# speed, and 20 means best compression ratio which will consume
> -# more CPU. Defaults to 1. (Since 5.0)
> -#
> -# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
> -# aliases for the purpose of dirty bitmap migration. Such aliases
> -# may for example be the corresponding names on the opposite site.
> -# The mapping must be one-to-one, but not necessarily complete: On
> -# the source, unmapped bitmaps and all bitmaps on unmapped nodes
> -# will be ignored. On the destination, encountering an unmapped
> -# alias in the incoming migration stream will result in a report,
> -# and all further bitmap migration data will then be discarded.
> -# Note that the destination does not know about bitmaps it does
> -# not receive, so there is no limitation or requirement regarding
> -# the number of bitmaps received, or how they are named, or on
> -# which nodes they are placed. By default (when this parameter
> -# has never been set), bitmap names are mapped to themselves.
> -# Nodes are mapped to their block device name if there is one, and
> -# to their node name otherwise. (Since 5.2)
> -#
> -# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
> -# limit during live migration. Should be in the range 1 to
> -# 1000ms. Defaults to 1000ms. (Since 8.1)
> -#
> -# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration.
> -# Defaults to 1. (Since 8.1)
> -#
> -# @mode: Migration mode. See description in @MigMode. Default is
> -# 'normal'. (Since 8.2)
> -#
> -# @zero-page-detection: Whether and how to detect zero pages.
> -# See description in @ZeroPageDetection. Default is 'multifd'.
> -# (since 9.0)
> -#
> -# @direct-io: Open migration files with O_DIRECT when possible. This
> -# only has effect if the @mapped-ram capability is enabled.
> -# (Since 9.1)
> -#
> -# Features:
> -#
> -# @unstable: Members @x-checkpoint-delay and
> -# @x-vcpu-dirty-limit-period are experimental.
> -#
> -# TODO: either fuse back into MigrationParameters, or make
> -# MigrationParameters members mandatory
> -#
> -# Since: 2.4
> -##
> -{ 'struct': 'MigrateSetParameters',
> - 'data': { '*announce-initial': 'size',
> - '*announce-max': 'size',
> - '*announce-rounds': 'size',
> - '*announce-step': 'size',
> - '*throttle-trigger-threshold': 'uint8',
> - '*cpu-throttle-initial': 'uint8',
> - '*cpu-throttle-increment': 'uint8',
> - '*cpu-throttle-tailslow': 'bool',
> - '*tls-creds': 'StrOrNull',
> - '*tls-hostname': 'StrOrNull',
> - '*tls-authz': 'StrOrNull',
> - '*max-bandwidth': 'size',
> - '*avail-switchover-bandwidth': 'size',
> - '*downtime-limit': 'uint64',
> - '*x-checkpoint-delay': { 'type': 'uint32',
> - 'features': [ 'unstable' ] },
> - '*multifd-channels': 'uint8',
> - '*xbzrle-cache-size': 'size',
> - '*max-postcopy-bandwidth': 'size',
> - '*max-cpu-throttle': 'uint8',
> - '*multifd-compression': 'MultiFDCompression',
> - '*multifd-zlib-level': 'uint8',
> - '*multifd-qatzip-level': 'uint8',
> - '*multifd-zstd-level': 'uint8',
> - '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> - '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
> - 'features': [ 'unstable' ] },
> - '*vcpu-dirty-limit': 'uint64',
> - '*mode': 'MigMode',
> - '*zero-page-detection': 'ZeroPageDetection',
> - '*direct-io': 'bool' } }
> -
> ##
> # @migrate-set-parameters:
> #
> @@ -1127,9 +931,7 @@
> 'data': 'MigrateSetParameters' }
>
> ##
> -# @MigrationParameters:
> -#
> -# The optional members aren't actually optional.
> +# @MigrationConfigBase:
> #
> # @announce-initial: Initial delay (in milliseconds) before sending
> # the first announce (Since 4.0)
> @@ -1168,26 +970,6 @@
> # be excessive at tail stage. The default value is false. (Since
> # 5.1)
> #
> -# @tls-creds: ID of the 'tls-creds' object that provides credentials
> -# for establishing a TLS connection over the migration data
> -# channel. On the outgoing side of the migration, the credentials
> -# must be for a 'client' endpoint, while for the incoming side the
> -# credentials must be for a 'server' endpoint. An empty string
> -# means that QEMU will use plain text mode for migration, rather
> -# than TLS. (Since 2.7)
> -#
> -# Note: 2.8 omits empty @tls-creds instead.
> -#
> -# @tls-hostname: migration target's hostname for validating the
> -# server's x509 certificate identity. If empty, QEMU will use the
> -# hostname from the migration URI, if any. (Since 2.7)
> -#
> -# Note: 2.8 omits empty @tls-hostname instead.
> -#
> -# @tls-authz: ID of the 'authz' object subclass that provides access
> -# control checking of the TLS x509 certificate distinguished name.
> -# (Since 4.0)
> -#
> # @max-bandwidth: maximum speed for migration, in bytes per second.
> # (Since 2.8)
> #
> @@ -1277,45 +1059,121 @@
> # only has effect if the @mapped-ram capability is enabled.
> # (Since 9.1)
> #
> +# @tls: Whether to use TLS. If this is set the options @tls-authz,
> +# @tls-creds, @tls-hostname are mandatory and a valid string is
> +# expected. (Since 10.1)
> +#
> # Features:
> #
> # @unstable: Members @x-checkpoint-delay and
> # @x-vcpu-dirty-limit-period are experimental.
> #
> +# Since: 10.1
> +##
> +{ 'struct': 'MigrationConfigBase',
> + 'data': { '*announce-initial': 'size',
> + '*announce-max': 'size',
> + '*announce-rounds': 'size',
> + '*announce-step': 'size',
> + '*throttle-trigger-threshold': 'uint8',
> + '*cpu-throttle-initial': 'uint8',
> + '*cpu-throttle-increment': 'uint8',
> + '*cpu-throttle-tailslow': 'bool',
> + '*max-bandwidth': 'size',
> + '*avail-switchover-bandwidth': 'size',
> + '*downtime-limit': 'uint64',
> + '*x-checkpoint-delay': { 'type': 'uint32',
> + 'features': [ 'unstable' ] },
> + '*multifd-channels': 'uint8',
> + '*xbzrle-cache-size': 'size',
> + '*max-postcopy-bandwidth': 'size',
> + '*max-cpu-throttle': 'uint8',
> + '*multifd-compression': 'MultiFDCompression',
> + '*multifd-zlib-level': 'uint8',
> + '*multifd-qatzip-level': 'uint8',
> + '*multifd-zstd-level': 'uint8',
> + '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> + '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
> + 'features': [ 'unstable' ] },
> + '*vcpu-dirty-limit': 'uint64',
> + '*mode': 'MigMode',
> + '*zero-page-detection': 'ZeroPageDetection',
> + '*direct-io': 'bool',
> + '*tls': 'bool' } }
> +
> +##
> +# @MigrationParameters:
> +#
> +# The optional members of the base type aren't actually optional.
> +#
> +# @tls-creds: ID of the 'tls-creds' object that provides credentials
> +# for establishing a TLS connection over the migration data
> +# channel. On the outgoing side of the migration, the credentials
> +# must be for a 'client' endpoint, while for the incoming side the
> +# credentials must be for a 'server' endpoint. Setting this to a
> +# non-empty string enables TLS for all migrations. An empty
> +# string means that QEMU will use plain text mode for migration,
> +# rather than TLS. (Since 2.7)
> +#
> +# @tls-hostname: migration target's hostname for validating the
> +# server's x509 certificate identity. If empty, QEMU will use the
> +# hostname from the migration URI, if any. A non-empty value is
> +# required when using x509 based TLS credentials and the migration
> +# URI does not include a hostname, such as fd: or exec: based
> +# migration. (Since 2.7)
> +#
> +# Note: empty value works only since 2.9.
> +#
> +# @tls-authz: ID of the 'authz' object subclass that provides access
> +# control checking of the TLS x509 certificate distinguished name.
> +# This object is only resolved at time of use, so can be deleted
> +# and recreated on the fly while the migration server is active.
> +# If missing, it will default to denying access (Since 4.0)
> +#
> # Since: 2.4
> ##
> { 'struct': 'MigrationParameters',
> - 'data': { '*announce-initial': 'size',
> - '*announce-max': 'size',
> - '*announce-rounds': 'size',
> - '*announce-step': 'size',
> - '*throttle-trigger-threshold': 'uint8',
> - '*cpu-throttle-initial': 'uint8',
> - '*cpu-throttle-increment': 'uint8',
> - '*cpu-throttle-tailslow': 'bool',
> - '*tls-creds': 'str',
> - '*tls-hostname': 'str',
> - '*tls-authz': 'str',
> - '*max-bandwidth': 'size',
> - '*avail-switchover-bandwidth': 'size',
> - '*downtime-limit': 'uint64',
> - '*x-checkpoint-delay': { 'type': 'uint32',
> - 'features': [ 'unstable' ] },
> - '*multifd-channels': 'uint8',
> - '*xbzrle-cache-size': 'size',
> - '*max-postcopy-bandwidth': 'size',
> - '*max-cpu-throttle': 'uint8',
> - '*multifd-compression': 'MultiFDCompression',
> - '*multifd-zlib-level': 'uint8',
> - '*multifd-qatzip-level': 'uint8',
> - '*multifd-zstd-level': 'uint8',
> - '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> - '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
> - 'features': [ 'unstable' ] },
> - '*vcpu-dirty-limit': 'uint64',
> - '*mode': 'MigMode',
> - '*zero-page-detection': 'ZeroPageDetection',
> - '*direct-io': 'bool' } }
> + 'base': 'MigrationConfigBase',
> + 'data': { 'tls-creds': 'str',
> + 'tls-hostname': 'str',
> + 'tls-authz': 'str' } }
> +
> +##
> +# @MigrateSetParameters:
> +#
> +# Compatibility layer to accept null values for the TLS options.
> +#
> +# @tls-creds: ID of the 'tls-creds' object that provides credentials
> +# for establishing a TLS connection over the migration data
> +# channel. On the outgoing side of the migration, the credentials
> +# must be for a 'client' endpoint, while for the incoming side the
> +# credentials must be for a 'server' endpoint. Setting this to a
> +# non-empty string enables TLS for all migrations. An empty
> +# string means that QEMU will use plain text mode for migration,
> +# rather than TLS. This is the default. (Since 2.7)
> +#
> +# @tls-hostname: migration target's hostname for validating the
> +# server's x509 certificate identity. If empty, QEMU will use the
> +# hostname from the migration URI, if any. A non-empty value is
> +# required when using x509 based TLS credentials and the migration
> +# URI does not include a hostname, such as fd: or exec: based
> +# migration. (Since 2.7)
> +#
> +# Note: empty value works only since 2.9.
> +#
> +# @tls-authz: ID of the 'authz' object subclass that provides access
> +# control checking of the TLS x509 certificate distinguished name.
> +# This object is only resolved at time of use, so can be deleted
> +# and recreated on the fly while the migration server is active.
> +# If missing, it will default to denying access (Since 4.0)
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'MigrateSetParameters',
> + 'base': 'MigrationConfigBase',
> + 'data': { '*tls-creds': 'StrOrNull',
> + '*tls-hostname': 'StrOrNull',
> + '*tls-authz': 'StrOrNull' } }
>
> ##
> # @query-migrate-parameters:
Your change is fairly simple, but that's not obvious from the diff.
1. Move everything but the TLS stuff from MigrationParameters to its new
base type MigrationConfigBase.
No change to the interface, obviously.
Introspection hides the base type, and shows members of
MigrationParameters in a different order, but order doesn't matter.
The doc generator isn't smart enough (yet) to hide the base type.
Apart from that, documentation is unchanged.
2. Replace everything but the TLS stuff from MigrateSetParameters by
base type MigrateSetParameters.
Because the base type's members are identical to the members it
replaces, no change to the interface.
Introspection hides the base type, and shows members of
MigrateSetParameters in a different order, but order doesn't matter.
The base type's member documentation is *not* identical to the member
documentation it replaces. To see the differences, I ran
sphinx-build with -b text before and after, and fed its output to
diff:
* **cpu-throttle-initial** ("int", *optional*) -- Initial
percentage of time guest cpus are throttled when migration
- auto-converge is activated. The default value is 20. (Since
- 2.7)
+ auto-converge is activated. (Since 2.7)
We no longer document the default value. This is wrong.
The difference before the patch makes some sense. The members of
MigrateSetParameters are actually optional command arguments, and
their defaults must be documented. The members of
MigrationParameters aren't actually optional, and talking about
defaults is misleading there. We do it anyway in a few places.
Factoring out MigrationConfigBase leads to a conflict: in its role as
base of MigrationParameters, it shouldn't talk about defaults, and in
its role as base of MigrateSetParameters, it should document the
defaults supplied by migrate-set-parameters.
There is no perfectly satisfactory solution with our current
infrastructure.
We can elect to ignore the problem for now. Point it out in a TODO
comment.
We can try to explain in the doc text that default values apply only
to migrate-set-parameters. I expect this to be awkward. More
awkward once the doc generator inlines stuff.
* **cpu-throttle-increment** ("int", *optional*) -- throttle
percentage increase each time auto-converge detects that
- migration is not making progress. The default value is 10.
- (Since 2.7)
+ migration is not making progress. (Since 2.7)
Likewise.
- * **x-checkpoint-delay** ("int", *optional*) -- The delay time
- (in ms) between two COLO checkpoints in periodic mode. (Since
- 2.8)
+ * **x-checkpoint-delay** ("int", *optional*) -- the delay time
+ between two COLO checkpoints. (Since 2.8)
Inconsistent before the patch. The deleted version is better. Easy
enough to fix.
+ * **tls** ("boolean", *optional*) -- Whether to use TLS. If this
+ is set the options "tls-authz", "tls-creds", "tls-hostname"
+ are mandatory and a valid string is expected. (Since 10.1)
+
Accident.
Markus Armbruster <armbru@redhat.com> writes:
> Fabiano Rosas <farosas@suse.de> writes:
>
>> Introduce a new MigrationConfigBase, to allow most of the duplication
>> in migration.json to be eliminated.
>>
>> The reason we need MigrationParameters and MigrationSetParameters is
>> that the internal parameter representation in the migration code, as
>> well as the user-facing return of query-migrate-parameters use one
>> type for the TLS options (tls-creds, tls-hostname, tls-authz), while
>> the user-facing input from migrate-set-parameters uses another.
>>
>> The difference is in whether the NULL values is accepted. The former
>> considers NULL as invalid, while the latter doesn't.
>>
>> Move all other (non-TLS) options into the new type and make it a base
>> type for the two diverging types so that each child type can declare
>> the TLS options in its own way.
>>
>> Nothing changes in the user API, nothing changes in the internal
>> representation, but we save several lines of duplication in
>> migration.json.
>
> I double-checked the external interface. There's a new member @tls, but
> you already told us it's an accident.
>
> Documentation does change. More on that below.
>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> qapi/migration.json | 358 +++++++++++++-------------------------------
>> 1 file changed, 108 insertions(+), 250 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 8b9c53595c..5a4d5a2d3e 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -914,202 +914,6 @@
>> 'zero-page-detection',
>> 'direct-io'] }
>>
>> -##
>> -# @MigrateSetParameters:
>> -#
>> -# @announce-initial: Initial delay (in milliseconds) before sending
>> -# the first announce (Since 4.0)
>> -#
>> -# @announce-max: Maximum delay (in milliseconds) between packets in
>> -# the announcement (Since 4.0)
>> -#
>> -# @announce-rounds: Number of self-announce packets sent after
>> -# migration (Since 4.0)
>> -#
>> -# @announce-step: Increase in delay (in milliseconds) between
>> -# subsequent packets in the announcement (Since 4.0)
>> -#
>> -# @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>> -# bytes_xfer_period to trigger throttling. It is expressed as
>> -# percentage. The default value is 50. (Since 5.0)
>> -#
>> -# @cpu-throttle-initial: Initial percentage of time guest cpus are
>> -# throttled when migration auto-converge is activated. The
>> -# default value is 20. (Since 2.7)
>> -#
>> -# @cpu-throttle-increment: throttle percentage increase each time
>> -# auto-converge detects that migration is not making progress.
>> -# The default value is 10. (Since 2.7)
>> -#
>> -# @cpu-throttle-tailslow: Make CPU throttling slower at tail stage At
>> -# the tail stage of throttling, the Guest is very sensitive to CPU
>> -# percentage while the @cpu-throttle -increment is excessive
>> -# usually at tail stage. If this parameter is true, we will
>> -# compute the ideal CPU percentage used by the Guest, which may
>> -# exactly make the dirty rate match the dirty rate threshold.
>> -# Then we will choose a smaller throttle increment between the one
>> -# specified by @cpu-throttle-increment and the one generated by
>> -# ideal CPU percentage. Therefore, it is compatible to
>> -# traditional throttling, meanwhile the throttle increment won't
>> -# be excessive at tail stage. The default value is false. (Since
>> -# 5.1)
>> -#
>> -# @tls-creds: ID of the 'tls-creds' object that provides credentials
>> -# for establishing a TLS connection over the migration data
>> -# channel. On the outgoing side of the migration, the credentials
>> -# must be for a 'client' endpoint, while for the incoming side the
>> -# credentials must be for a 'server' endpoint. Setting this to a
>> -# non-empty string enables TLS for all migrations. An empty
>> -# string means that QEMU will use plain text mode for migration,
>> -# rather than TLS. This is the default. (Since 2.7)
>> -#
>> -# @tls-hostname: migration target's hostname for validating the
>> -# server's x509 certificate identity. If empty, QEMU will use the
>> -# hostname from the migration URI, if any. A non-empty value is
>> -# required when using x509 based TLS credentials and the migration
>> -# URI does not include a hostname, such as fd: or exec: based
>> -# migration. (Since 2.7)
>> -#
>> -# Note: empty value works only since 2.9.
>> -#
>> -# @tls-authz: ID of the 'authz' object subclass that provides access
>> -# control checking of the TLS x509 certificate distinguished name.
>> -# This object is only resolved at time of use, so can be deleted
>> -# and recreated on the fly while the migration server is active.
>> -# If missing, it will default to denying access (Since 4.0)
>> -#
>> -# @max-bandwidth: maximum speed for migration, in bytes per second.
>> -# (Since 2.8)
>> -#
>> -# @avail-switchover-bandwidth: to set the available bandwidth that
>> -# migration can use during switchover phase. NOTE! This does not
>> -# limit the bandwidth during switchover, but only for calculations
>> -# when making decisions to switchover. By default, this value is
>> -# zero, which means QEMU will estimate the bandwidth
>> -# automatically. This can be set when the estimated value is not
>> -# accurate, while the user is able to guarantee such bandwidth is
>> -# available when switching over. When specified correctly, this
>> -# can make the switchover decision much more accurate.
>> -# (Since 8.2)
>> -#
>> -# @downtime-limit: set maximum tolerated downtime for migration.
>> -# maximum downtime in milliseconds (Since 2.8)
>> -#
>> -# @x-checkpoint-delay: The delay time (in ms) between two COLO
>> -# checkpoints in periodic mode. (Since 2.8)
>> -#
>> -# @multifd-channels: Number of channels used to migrate data in
>> -# parallel. This is the same number that the number of sockets
>> -# used for migration. The default value is 2 (since 4.0)
>> -#
>> -# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It
>> -# needs to be a multiple of the target page size and a power of 2
>> -# (Since 2.11)
>> -#
>> -# @max-postcopy-bandwidth: Background transfer bandwidth during
>> -# postcopy. Defaults to 0 (unlimited). In bytes per second.
>> -# (Since 3.0)
>> -#
>> -# @max-cpu-throttle: maximum cpu throttle percentage. Defaults to 99.
>> -# (Since 3.1)
>> -#
>> -# @multifd-compression: Which compression method to use. Defaults to
>> -# none. (Since 5.0)
>> -#
>> -# @multifd-zlib-level: Set the compression level to be used in live
>> -# migration, the compression level is an integer between 0 and 9,
>> -# where 0 means no compression, 1 means the best compression
>> -# speed, and 9 means best compression ratio which will consume
>> -# more CPU. Defaults to 1. (Since 5.0)
>> -#
>> -# @multifd-qatzip-level: Set the compression level to be used in live
>> -# migration. The level is an integer between 1 and 9, where 1 means
>> -# the best compression speed, and 9 means the best compression
>> -# ratio which will consume more CPU. Defaults to 1. (Since 9.2)
>> -#
>> -# @multifd-zstd-level: Set the compression level to be used in live
>> -# migration, the compression level is an integer between 0 and 20,
>> -# where 0 means no compression, 1 means the best compression
>> -# speed, and 20 means best compression ratio which will consume
>> -# more CPU. Defaults to 1. (Since 5.0)
>> -#
>> -# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>> -# aliases for the purpose of dirty bitmap migration. Such aliases
>> -# may for example be the corresponding names on the opposite site.
>> -# The mapping must be one-to-one, but not necessarily complete: On
>> -# the source, unmapped bitmaps and all bitmaps on unmapped nodes
>> -# will be ignored. On the destination, encountering an unmapped
>> -# alias in the incoming migration stream will result in a report,
>> -# and all further bitmap migration data will then be discarded.
>> -# Note that the destination does not know about bitmaps it does
>> -# not receive, so there is no limitation or requirement regarding
>> -# the number of bitmaps received, or how they are named, or on
>> -# which nodes they are placed. By default (when this parameter
>> -# has never been set), bitmap names are mapped to themselves.
>> -# Nodes are mapped to their block device name if there is one, and
>> -# to their node name otherwise. (Since 5.2)
>> -#
>> -# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty
>> -# limit during live migration. Should be in the range 1 to
>> -# 1000ms. Defaults to 1000ms. (Since 8.1)
>> -#
>> -# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration.
>> -# Defaults to 1. (Since 8.1)
>> -#
>> -# @mode: Migration mode. See description in @MigMode. Default is
>> -# 'normal'. (Since 8.2)
>> -#
>> -# @zero-page-detection: Whether and how to detect zero pages.
>> -# See description in @ZeroPageDetection. Default is 'multifd'.
>> -# (since 9.0)
>> -#
>> -# @direct-io: Open migration files with O_DIRECT when possible. This
>> -# only has effect if the @mapped-ram capability is enabled.
>> -# (Since 9.1)
>> -#
>> -# Features:
>> -#
>> -# @unstable: Members @x-checkpoint-delay and
>> -# @x-vcpu-dirty-limit-period are experimental.
>> -#
>> -# TODO: either fuse back into MigrationParameters, or make
>> -# MigrationParameters members mandatory
>> -#
>> -# Since: 2.4
>> -##
>> -{ 'struct': 'MigrateSetParameters',
>> - 'data': { '*announce-initial': 'size',
>> - '*announce-max': 'size',
>> - '*announce-rounds': 'size',
>> - '*announce-step': 'size',
>> - '*throttle-trigger-threshold': 'uint8',
>> - '*cpu-throttle-initial': 'uint8',
>> - '*cpu-throttle-increment': 'uint8',
>> - '*cpu-throttle-tailslow': 'bool',
>> - '*tls-creds': 'StrOrNull',
>> - '*tls-hostname': 'StrOrNull',
>> - '*tls-authz': 'StrOrNull',
>> - '*max-bandwidth': 'size',
>> - '*avail-switchover-bandwidth': 'size',
>> - '*downtime-limit': 'uint64',
>> - '*x-checkpoint-delay': { 'type': 'uint32',
>> - 'features': [ 'unstable' ] },
>> - '*multifd-channels': 'uint8',
>> - '*xbzrle-cache-size': 'size',
>> - '*max-postcopy-bandwidth': 'size',
>> - '*max-cpu-throttle': 'uint8',
>> - '*multifd-compression': 'MultiFDCompression',
>> - '*multifd-zlib-level': 'uint8',
>> - '*multifd-qatzip-level': 'uint8',
>> - '*multifd-zstd-level': 'uint8',
>> - '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
>> - '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
>> - 'features': [ 'unstable' ] },
>> - '*vcpu-dirty-limit': 'uint64',
>> - '*mode': 'MigMode',
>> - '*zero-page-detection': 'ZeroPageDetection',
>> - '*direct-io': 'bool' } }
>> -
>> ##
>> # @migrate-set-parameters:
>> #
>> @@ -1127,9 +931,7 @@
>> 'data': 'MigrateSetParameters' }
>>
>> ##
>> -# @MigrationParameters:
>> -#
>> -# The optional members aren't actually optional.
>> +# @MigrationConfigBase:
>> #
>> # @announce-initial: Initial delay (in milliseconds) before sending
>> # the first announce (Since 4.0)
>> @@ -1168,26 +970,6 @@
>> # be excessive at tail stage. The default value is false. (Since
>> # 5.1)
>> #
>> -# @tls-creds: ID of the 'tls-creds' object that provides credentials
>> -# for establishing a TLS connection over the migration data
>> -# channel. On the outgoing side of the migration, the credentials
>> -# must be for a 'client' endpoint, while for the incoming side the
>> -# credentials must be for a 'server' endpoint. An empty string
>> -# means that QEMU will use plain text mode for migration, rather
>> -# than TLS. (Since 2.7)
>> -#
>> -# Note: 2.8 omits empty @tls-creds instead.
>> -#
>> -# @tls-hostname: migration target's hostname for validating the
>> -# server's x509 certificate identity. If empty, QEMU will use the
>> -# hostname from the migration URI, if any. (Since 2.7)
>> -#
>> -# Note: 2.8 omits empty @tls-hostname instead.
>> -#
>> -# @tls-authz: ID of the 'authz' object subclass that provides access
>> -# control checking of the TLS x509 certificate distinguished name.
>> -# (Since 4.0)
>> -#
>> # @max-bandwidth: maximum speed for migration, in bytes per second.
>> # (Since 2.8)
>> #
>> @@ -1277,45 +1059,121 @@
>> # only has effect if the @mapped-ram capability is enabled.
>> # (Since 9.1)
>> #
>> +# @tls: Whether to use TLS. If this is set the options @tls-authz,
>> +# @tls-creds, @tls-hostname are mandatory and a valid string is
>> +# expected. (Since 10.1)
>> +#
>> # Features:
>> #
>> # @unstable: Members @x-checkpoint-delay and
>> # @x-vcpu-dirty-limit-period are experimental.
>> #
>> +# Since: 10.1
>> +##
>> +{ 'struct': 'MigrationConfigBase',
>> + 'data': { '*announce-initial': 'size',
>> + '*announce-max': 'size',
>> + '*announce-rounds': 'size',
>> + '*announce-step': 'size',
>> + '*throttle-trigger-threshold': 'uint8',
>> + '*cpu-throttle-initial': 'uint8',
>> + '*cpu-throttle-increment': 'uint8',
>> + '*cpu-throttle-tailslow': 'bool',
>> + '*max-bandwidth': 'size',
>> + '*avail-switchover-bandwidth': 'size',
>> + '*downtime-limit': 'uint64',
>> + '*x-checkpoint-delay': { 'type': 'uint32',
>> + 'features': [ 'unstable' ] },
>> + '*multifd-channels': 'uint8',
>> + '*xbzrle-cache-size': 'size',
>> + '*max-postcopy-bandwidth': 'size',
>> + '*max-cpu-throttle': 'uint8',
>> + '*multifd-compression': 'MultiFDCompression',
>> + '*multifd-zlib-level': 'uint8',
>> + '*multifd-qatzip-level': 'uint8',
>> + '*multifd-zstd-level': 'uint8',
>> + '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
>> + '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
>> + 'features': [ 'unstable' ] },
>> + '*vcpu-dirty-limit': 'uint64',
>> + '*mode': 'MigMode',
>> + '*zero-page-detection': 'ZeroPageDetection',
>> + '*direct-io': 'bool',
>> + '*tls': 'bool' } }
>> +
>> +##
>> +# @MigrationParameters:
>> +#
>> +# The optional members of the base type aren't actually optional.
>> +#
>> +# @tls-creds: ID of the 'tls-creds' object that provides credentials
>> +# for establishing a TLS connection over the migration data
>> +# channel. On the outgoing side of the migration, the credentials
>> +# must be for a 'client' endpoint, while for the incoming side the
>> +# credentials must be for a 'server' endpoint. Setting this to a
>> +# non-empty string enables TLS for all migrations. An empty
>> +# string means that QEMU will use plain text mode for migration,
>> +# rather than TLS. (Since 2.7)
>> +#
>> +# @tls-hostname: migration target's hostname for validating the
>> +# server's x509 certificate identity. If empty, QEMU will use the
>> +# hostname from the migration URI, if any. A non-empty value is
>> +# required when using x509 based TLS credentials and the migration
>> +# URI does not include a hostname, such as fd: or exec: based
>> +# migration. (Since 2.7)
>> +#
>> +# Note: empty value works only since 2.9.
>> +#
>> +# @tls-authz: ID of the 'authz' object subclass that provides access
>> +# control checking of the TLS x509 certificate distinguished name.
>> +# This object is only resolved at time of use, so can be deleted
>> +# and recreated on the fly while the migration server is active.
>> +# If missing, it will default to denying access (Since 4.0)
>> +#
>> # Since: 2.4
>> ##
>> { 'struct': 'MigrationParameters',
>> - 'data': { '*announce-initial': 'size',
>> - '*announce-max': 'size',
>> - '*announce-rounds': 'size',
>> - '*announce-step': 'size',
>> - '*throttle-trigger-threshold': 'uint8',
>> - '*cpu-throttle-initial': 'uint8',
>> - '*cpu-throttle-increment': 'uint8',
>> - '*cpu-throttle-tailslow': 'bool',
>> - '*tls-creds': 'str',
>> - '*tls-hostname': 'str',
>> - '*tls-authz': 'str',
>> - '*max-bandwidth': 'size',
>> - '*avail-switchover-bandwidth': 'size',
>> - '*downtime-limit': 'uint64',
>> - '*x-checkpoint-delay': { 'type': 'uint32',
>> - 'features': [ 'unstable' ] },
>> - '*multifd-channels': 'uint8',
>> - '*xbzrle-cache-size': 'size',
>> - '*max-postcopy-bandwidth': 'size',
>> - '*max-cpu-throttle': 'uint8',
>> - '*multifd-compression': 'MultiFDCompression',
>> - '*multifd-zlib-level': 'uint8',
>> - '*multifd-qatzip-level': 'uint8',
>> - '*multifd-zstd-level': 'uint8',
>> - '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
>> - '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
>> - 'features': [ 'unstable' ] },
>> - '*vcpu-dirty-limit': 'uint64',
>> - '*mode': 'MigMode',
>> - '*zero-page-detection': 'ZeroPageDetection',
>> - '*direct-io': 'bool' } }
>> + 'base': 'MigrationConfigBase',
>> + 'data': { 'tls-creds': 'str',
>> + 'tls-hostname': 'str',
>> + 'tls-authz': 'str' } }
>> +
>> +##
>> +# @MigrateSetParameters:
>> +#
>> +# Compatibility layer to accept null values for the TLS options.
>> +#
>> +# @tls-creds: ID of the 'tls-creds' object that provides credentials
>> +# for establishing a TLS connection over the migration data
>> +# channel. On the outgoing side of the migration, the credentials
>> +# must be for a 'client' endpoint, while for the incoming side the
>> +# credentials must be for a 'server' endpoint. Setting this to a
>> +# non-empty string enables TLS for all migrations. An empty
>> +# string means that QEMU will use plain text mode for migration,
>> +# rather than TLS. This is the default. (Since 2.7)
>> +#
>> +# @tls-hostname: migration target's hostname for validating the
>> +# server's x509 certificate identity. If empty, QEMU will use the
>> +# hostname from the migration URI, if any. A non-empty value is
>> +# required when using x509 based TLS credentials and the migration
>> +# URI does not include a hostname, such as fd: or exec: based
>> +# migration. (Since 2.7)
>> +#
>> +# Note: empty value works only since 2.9.
>> +#
>> +# @tls-authz: ID of the 'authz' object subclass that provides access
>> +# control checking of the TLS x509 certificate distinguished name.
>> +# This object is only resolved at time of use, so can be deleted
>> +# and recreated on the fly while the migration server is active.
>> +# If missing, it will default to denying access (Since 4.0)
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'struct': 'MigrateSetParameters',
>> + 'base': 'MigrationConfigBase',
>> + 'data': { '*tls-creds': 'StrOrNull',
>> + '*tls-hostname': 'StrOrNull',
>> + '*tls-authz': 'StrOrNull' } }
>>
>> ##
>> # @query-migrate-parameters:
>
> Your change is fairly simple, but that's not obvious from the diff.
>
> 1. Move everything but the TLS stuff from MigrationParameters to its new
> base type MigrationConfigBase.
>
> No change to the interface, obviously.
>
> Introspection hides the base type, and shows members of
> MigrationParameters in a different order, but order doesn't matter.
>
> The doc generator isn't smart enough (yet) to hide the base type.
> Apart from that, documentation is unchanged.
>
> 2. Replace everything but the TLS stuff from MigrateSetParameters by
> base type MigrateSetParameters.
>
> Because the base type's members are identical to the members it
> replaces, no change to the interface.
>
> Introspection hides the base type, and shows members of
> MigrateSetParameters in a different order, but order doesn't matter.
>
> The base type's member documentation is *not* identical to the member
> documentation it replaces. To see the differences, I ran
> sphinx-build with -b text before and after, and fed its output to
> diff:
>
> * **cpu-throttle-initial** ("int", *optional*) -- Initial
> percentage of time guest cpus are throttled when migration
> - auto-converge is activated. The default value is 20. (Since
> - 2.7)
> + auto-converge is activated. (Since 2.7)
>
> We no longer document the default value. This is wrong.
>
> The difference before the patch makes some sense. The members of
> MigrateSetParameters are actually optional command arguments, and
> their defaults must be documented. The members of
> MigrationParameters aren't actually optional, and talking about
> defaults is misleading there. We do it anyway in a few places.
>
> Factoring out MigrationConfigBase leads to a conflict: in its role as
> base of MigrationParameters, it shouldn't talk about defaults, and in
> its role as base of MigrateSetParameters, it should document the
> defaults supplied by migrate-set-parameters.
>
> There is no perfectly satisfactory solution with our current
> infrastructure.
>
> We can elect to ignore the problem for now. Point it out in a TODO
> comment.
>
> We can try to explain in the doc text that default values apply only
> to migrate-set-parameters. I expect this to be awkward. More
> awkward once the doc generator inlines stuff.
>
> * **cpu-throttle-increment** ("int", *optional*) -- throttle
> percentage increase each time auto-converge detects that
> - migration is not making progress. The default value is 10.
> - (Since 2.7)
> + migration is not making progress. (Since 2.7)
>
> Likewise.
>
> - * **x-checkpoint-delay** ("int", *optional*) -- The delay time
> - (in ms) between two COLO checkpoints in periodic mode. (Since
> - 2.8)
> + * **x-checkpoint-delay** ("int", *optional*) -- the delay time
> + between two COLO checkpoints. (Since 2.8)
>
> Inconsistent before the patch. The deleted version is better. Easy
> enough to fix.
>
> + * **tls** ("boolean", *optional*) -- Whether to use TLS. If this
> + is set the options "tls-authz", "tls-creds", "tls-hostname"
> + are mandatory and a valid string is expected. (Since 10.1)
> +
>
> Accident.
I missed
> base type MigrateSetParameters.
>
> Because the base type's members are identical to the members it
> replaces, no change to the interface.
>
> Introspection hides the base type, and shows members of
> MigrateSetParameters in a different order, but order doesn't matter.
>
> The base type's member documentation is *not* identical to the member
> documentation it replaces. To see the differences, I ran
> sphinx-build with -b text before and after, and fed its output to
> diff:
>
> * **cpu-throttle-initial** ("int", *optional*) -- Initial
> percentage of time guest cpus are throttled when migration
> - auto-converge is activated. The default value is 20. (Since
> - 2.7)
> + auto-converge is activated. (Since 2.7)
>
> We no longer document the default value. This is wrong.
>
> The difference before the patch makes some sense. The members of
> MigrateSetParameters are actually optional command arguments, and
> their defaults must be documented. The members of
> MigrationParameters aren't actually optional, and talking about
> defaults is misleading there. We do it anyway in a few places.
>
> Factoring out MigrationConfigBase leads to a conflict: in its role as
> base of MigrationParameters, it shouldn't talk about defaults, and in
> its role as base of MigrateSetParameters, it should document the
> defaults supplied by migrate-set-parameters.
>
> There is no perfectly satisfactory solution with our current
> infrastructure.
>
> We can elect to ignore the problem for now. Point it out in a TODO
> comment.
>
> We can try to explain in the doc text that default values apply only
> to migrate-set-parameters. I expect this to be awkward. More
> awkward once the doc generator inlines stuff.
>
> * **cpu-throttle-increment** ("int", *optional*) -- throttle
> percentage increase each time auto-converge detects that
> - migration is not making progress. The default value is 10.
> - (Since 2.7)
> + migration is not making progress. (Since 2.7)
>
> Likewise.
>
> - * **x-checkpoint-delay** ("int", *optional*) -- The delay time
> - (in ms) between two COLO checkpoints in periodic mode. (Since
> - 2.8)
> + * **x-checkpoint-delay** ("int", *optional*) -- the delay time
> + between two COLO checkpoints. (Since 2.8)
>
> Inconsistent before the patch. The deleted version is better. Easy
> enough to fix.
>
> + * **tls** ("boolean", *optional*) -- Whether to use TLS. If this
> + is set the options "tls-authz", "tls-creds", "tls-hostname"
> + are mandatory and a valid string is expected. (Since 10.1)
> +
>
> Accident.
I missed
3. Make @tls-creds, @tls-hostname, @tls-authz non-optional in
MigrationParameters.
Compatible change to the interface.
When my description of what a patch does goes 1. 2. 3., then splitting
it up could be in order. Especially when I miss 3. initially :)
On Fri, Apr 11, 2025 at 04:14:35PM -0300, Fabiano Rosas wrote:
> Introduce a new MigrationConfigBase, to allow most of the duplication
> in migration.json to be eliminated.
>
> The reason we need MigrationParameters and MigrationSetParameters is
> that the internal parameter representation in the migration code, as
> well as the user-facing return of query-migrate-parameters use one
> type for the TLS options (tls-creds, tls-hostname, tls-authz), while
> the user-facing input from migrate-set-parameters uses another.
>
> The difference is in whether the NULL values is accepted. The former
> considers NULL as invalid, while the latter doesn't.
>
> Move all other (non-TLS) options into the new type and make it a base
> type for the two diverging types so that each child type can declare
> the TLS options in its own way.
>
> Nothing changes in the user API, nothing changes in the internal
> representation, but we save several lines of duplication in
> migration.json.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> qapi/migration.json | 358 +++++++++++++-------------------------------
> 1 file changed, 108 insertions(+), 250 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 8b9c53595c..5a4d5a2d3e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -914,202 +914,6 @@
> @@ -1277,45 +1059,121 @@
> # only has effect if the @mapped-ram capability is enabled.
> # (Since 9.1)
> #
> +# @tls: Whether to use TLS. If this is set the options @tls-authz,
> +# @tls-creds, @tls-hostname are mandatory and a valid string is
> +# expected. (Since 10.1)
> +#
I'm not really finding it compelling to add a bool @tls as it
is just a denormalization of !!@tls-creds.
Incidentally the docs here are wrong - TLS can be used by
only setting @tls-creds. The @tls-authz & @tls-hostname
params are always optional.
> # Features:
> #
> # @unstable: Members @x-checkpoint-delay and
> # @x-vcpu-dirty-limit-period are experimental.
> #
> +# Since: 10.1
> +##
> +{ 'struct': 'MigrationConfigBase',
> + 'data': { '*announce-initial': 'size',
> + '*announce-max': 'size',
> + '*announce-rounds': 'size',
> + '*announce-step': 'size',
> + '*throttle-trigger-threshold': 'uint8',
> + '*cpu-throttle-initial': 'uint8',
> + '*cpu-throttle-increment': 'uint8',
> + '*cpu-throttle-tailslow': 'bool',
> + '*max-bandwidth': 'size',
> + '*avail-switchover-bandwidth': 'size',
> + '*downtime-limit': 'uint64',
> + '*x-checkpoint-delay': { 'type': 'uint32',
> + 'features': [ 'unstable' ] },
> + '*multifd-channels': 'uint8',
> + '*xbzrle-cache-size': 'size',
> + '*max-postcopy-bandwidth': 'size',
> + '*max-cpu-throttle': 'uint8',
> + '*multifd-compression': 'MultiFDCompression',
> + '*multifd-zlib-level': 'uint8',
> + '*multifd-qatzip-level': 'uint8',
> + '*multifd-zstd-level': 'uint8',
> + '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
> + '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
> + 'features': [ 'unstable' ] },
> + '*vcpu-dirty-limit': 'uint64',
> + '*mode': 'MigMode',
> + '*zero-page-detection': 'ZeroPageDetection',
> + '*direct-io': 'bool',
> + '*tls': 'bool' } }
> { 'struct': 'MigrationParameters',
> + 'base': 'MigrationConfigBase',
> + 'data': { 'tls-creds': 'str',
> + 'tls-hostname': 'str',
> + 'tls-authz': 'str' } }
snip
> +{ 'struct': 'MigrateSetParameters',
> + 'base': 'MigrationConfigBase',
> + 'data': { '*tls-creds': 'StrOrNull',
> + '*tls-hostname': 'StrOrNull',
> + '*tls-authz': 'StrOrNull' } }
I recall we discussed this difference a year or two ago, but can't
recall what the outcome was.
Making the TLS params optional is a back compatible change for
MigrationParameters. I would think replacing 'str' with 'StrOrNull'
is also back compatible. So I'm wondering if we can't just unify
the sttructs fully for TLS, even if one usage scenario never actually
needs the "OrNull" bit nor needs the optionality
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Fri, Apr 11, 2025 at 04:14:35PM -0300, Fabiano Rosas wrote:
>> Introduce a new MigrationConfigBase, to allow most of the duplication
>> in migration.json to be eliminated.
>>
>> The reason we need MigrationParameters and MigrationSetParameters is
>> that the internal parameter representation in the migration code, as
>> well as the user-facing return of query-migrate-parameters use one
>> type for the TLS options (tls-creds, tls-hostname, tls-authz), while
>> the user-facing input from migrate-set-parameters uses another.
>>
>> The difference is in whether the NULL values is accepted. The former
>> considers NULL as invalid, while the latter doesn't.
>>
>> Move all other (non-TLS) options into the new type and make it a base
>> type for the two diverging types so that each child type can declare
>> the TLS options in its own way.
>>
>> Nothing changes in the user API, nothing changes in the internal
>> representation, but we save several lines of duplication in
>> migration.json.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> qapi/migration.json | 358 +++++++++++++-------------------------------
>> 1 file changed, 108 insertions(+), 250 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index 8b9c53595c..5a4d5a2d3e 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -914,202 +914,6 @@
>
>> @@ -1277,45 +1059,121 @@
>> # only has effect if the @mapped-ram capability is enabled.
>> # (Since 9.1)
>> #
>> +# @tls: Whether to use TLS. If this is set the options @tls-authz,
>> +# @tls-creds, @tls-hostname are mandatory and a valid string is
>> +# expected. (Since 10.1)
>> +#
>
> I'm not really finding it compelling to add a bool @tls as it
> is just a denormalization of !!@tls-creds.
>
This is here by mistake.
I remember Markus mentioning that implying TLS usage from tls-creds was
undesirable. I was prototyping a way of requiring an explicit opt-in.
> Incidentally the docs here are wrong - TLS can be used by
> only setting @tls-creds. The @tls-authz & @tls-hostname
> params are always optional.
>
>> # Features:
>> #
>> # @unstable: Members @x-checkpoint-delay and
>> # @x-vcpu-dirty-limit-period are experimental.
>> #
>> +# Since: 10.1
>> +##
>> +{ 'struct': 'MigrationConfigBase',
>> + 'data': { '*announce-initial': 'size',
>> + '*announce-max': 'size',
>> + '*announce-rounds': 'size',
>> + '*announce-step': 'size',
>> + '*throttle-trigger-threshold': 'uint8',
>> + '*cpu-throttle-initial': 'uint8',
>> + '*cpu-throttle-increment': 'uint8',
>> + '*cpu-throttle-tailslow': 'bool',
>> + '*max-bandwidth': 'size',
>> + '*avail-switchover-bandwidth': 'size',
>> + '*downtime-limit': 'uint64',
>> + '*x-checkpoint-delay': { 'type': 'uint32',
>> + 'features': [ 'unstable' ] },
>> + '*multifd-channels': 'uint8',
>> + '*xbzrle-cache-size': 'size',
>> + '*max-postcopy-bandwidth': 'size',
>> + '*max-cpu-throttle': 'uint8',
>> + '*multifd-compression': 'MultiFDCompression',
>> + '*multifd-zlib-level': 'uint8',
>> + '*multifd-qatzip-level': 'uint8',
>> + '*multifd-zstd-level': 'uint8',
>> + '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
>> + '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
>> + 'features': [ 'unstable' ] },
>> + '*vcpu-dirty-limit': 'uint64',
>> + '*mode': 'MigMode',
>> + '*zero-page-detection': 'ZeroPageDetection',
>> + '*direct-io': 'bool',
>> + '*tls': 'bool' } }
>
>
>> { 'struct': 'MigrationParameters',
>> + 'base': 'MigrationConfigBase',
>> + 'data': { 'tls-creds': 'str',
>> + 'tls-hostname': 'str',
>> + 'tls-authz': 'str' } }
>
> snip
>
>> +{ 'struct': 'MigrateSetParameters',
>> + 'base': 'MigrationConfigBase',
>> + 'data': { '*tls-creds': 'StrOrNull',
>> + '*tls-hostname': 'StrOrNull',
>> + '*tls-authz': 'StrOrNull' } }
>
> I recall we discussed this difference a year or two ago, but can't
> recall what the outcome was.
>
> Making the TLS params optional is a back compatible change for
> MigrationParameters. I would think replacing 'str' with 'StrOrNull'
> is also back compatible. So I'm wondering if we can't just unify
> the sttructs fully for TLS, even if one usage scenario never actually
> needs the "OrNull" bit nor needs the optionality
>
MigrationParameters is the output type for query-migrate-parameters. I
belive it must be all non-optional to keep compatibility. The docs on
master say: "The optional members aren't really optional"
For MigrateSetParameters they've always been optional and continue to
be. There we need to keep StrOrNull for compat.
>
> With regards,
> Daniel
Fabiano Rosas <farosas@suse.de> writes:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> On Fri, Apr 11, 2025 at 04:14:35PM -0300, Fabiano Rosas wrote:
>>> Introduce a new MigrationConfigBase, to allow most of the duplication
>>> in migration.json to be eliminated.
>>>
>>> The reason we need MigrationParameters and MigrationSetParameters is
>>> that the internal parameter representation in the migration code, as
>>> well as the user-facing return of query-migrate-parameters use one
>>> type for the TLS options (tls-creds, tls-hostname, tls-authz), while
>>> the user-facing input from migrate-set-parameters uses another.
>>>
>>> The difference is in whether the NULL values is accepted. The former
>>> considers NULL as invalid, while the latter doesn't.
>>>
>>> Move all other (non-TLS) options into the new type and make it a base
>>> type for the two diverging types so that each child type can declare
>>> the TLS options in its own way.
>>>
>>> Nothing changes in the user API, nothing changes in the internal
>>> representation, but we save several lines of duplication in
>>> migration.json.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>> qapi/migration.json | 358 +++++++++++++-------------------------------
>>> 1 file changed, 108 insertions(+), 250 deletions(-)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 8b9c53595c..5a4d5a2d3e 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -914,202 +914,6 @@
>>
>>> @@ -1277,45 +1059,121 @@
>>> # only has effect if the @mapped-ram capability is enabled.
>>> # (Since 9.1)
>>> #
>>> +# @tls: Whether to use TLS. If this is set the options @tls-authz,
>>> +# @tls-creds, @tls-hostname are mandatory and a valid string is
>>> +# expected. (Since 10.1)
>>> +#
>>
>> I'm not really finding it compelling to add a bool @tls as it
>> is just a denormalization of !!@tls-creds.
>>
>
> This is here by mistake.
>
> I remember Markus mentioning that implying TLS usage from tls-creds was
> undesirable. I was prototyping a way of requiring an explicit opt-in.
I don't remember a thing :)
>> Incidentally the docs here are wrong - TLS can be used by
>> only setting @tls-creds. The @tls-authz & @tls-hostname
>> params are always optional.
>>
>>> # Features:
>>> #
>>> # @unstable: Members @x-checkpoint-delay and
>>> # @x-vcpu-dirty-limit-period are experimental.
>>> #
>>> +# Since: 10.1
>>> +##
>>> +{ 'struct': 'MigrationConfigBase',
>>> + 'data': { '*announce-initial': 'size',
>>> + '*announce-max': 'size',
>>> + '*announce-rounds': 'size',
>>> + '*announce-step': 'size',
>>> + '*throttle-trigger-threshold': 'uint8',
>>> + '*cpu-throttle-initial': 'uint8',
>>> + '*cpu-throttle-increment': 'uint8',
>>> + '*cpu-throttle-tailslow': 'bool',
>>> + '*max-bandwidth': 'size',
>>> + '*avail-switchover-bandwidth': 'size',
>>> + '*downtime-limit': 'uint64',
>>> + '*x-checkpoint-delay': { 'type': 'uint32',
>>> + 'features': [ 'unstable' ] },
>>> + '*multifd-channels': 'uint8',
>>> + '*xbzrle-cache-size': 'size',
>>> + '*max-postcopy-bandwidth': 'size',
>>> + '*max-cpu-throttle': 'uint8',
>>> + '*multifd-compression': 'MultiFDCompression',
>>> + '*multifd-zlib-level': 'uint8',
>>> + '*multifd-qatzip-level': 'uint8',
>>> + '*multifd-zstd-level': 'uint8',
>>> + '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
>>> + '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
>>> + 'features': [ 'unstable' ] },
>>> + '*vcpu-dirty-limit': 'uint64',
>>> + '*mode': 'MigMode',
>>> + '*zero-page-detection': 'ZeroPageDetection',
>>> + '*direct-io': 'bool',
>>> + '*tls': 'bool' } }
>>
>>
>>> { 'struct': 'MigrationParameters',
>>> + 'base': 'MigrationConfigBase',
>>> + 'data': { 'tls-creds': 'str',
>>> + 'tls-hostname': 'str',
>>> + 'tls-authz': 'str' } }
>>
>> snip
>>
>>> +{ 'struct': 'MigrateSetParameters',
>>> + 'base': 'MigrationConfigBase',
>>> + 'data': { '*tls-creds': 'StrOrNull',
>>> + '*tls-hostname': 'StrOrNull',
>>> + '*tls-authz': 'StrOrNull' } }
>>
>> I recall we discussed this difference a year or two ago, but can't
>> recall what the outcome was.
>>
>> Making the TLS params optional is a back compatible change for
>> MigrationParameters. I would think replacing 'str' with 'StrOrNull'
>> is also back compatible. So I'm wondering if we can't just unify
>> the sttructs fully for TLS, even if one usage scenario never actually
>> needs the "OrNull" bit nor needs the optionality
>>
>
> MigrationParameters is the output type for query-migrate-parameters. I
> belive it must be all non-optional to keep compatibility. The docs on
> master say: "The optional members aren't really optional"
To be precise: even though the members are declared optional, they must
always be present.
The members became optional when commit de63ab61241 (migrate: Share
common MigrationParameters struct) reused MigrationParameters as
migrate-set-parameters argument type.
Making mandatory members of some type optional is compatible as long as
the type occurs only in command arguments. But MigrationParameters is a
command return type. Making its members optional was not kosher.
Because the optional members are always present, QMP didn't actually
change, except for introspection.
> For MigrateSetParameters they've always been optional and continue to
> be. There we need to keep StrOrNull for compat.
StrOrNull goes back to commit 01fa5598269 (migration: Use JSON null
instead of "" to reset parameter to default).
Similar argument: adding values to a member is compatible as long as the
type occurs only in command arguments. But MigrationParameters is a
command return type. Adding value null is won't be kosher. However, as
long as as the value is never actually used there, QMP won't actually
change, except for introspection.
If this was a clean and obvious interface, I'd argue against such
trickery and for keeping it clean and obvious. But the migration
configuration interface isn't.
Markus Armbruster <armbru@redhat.com> writes:
> Fabiano Rosas <farosas@suse.de> writes:
>
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>>> On Fri, Apr 11, 2025 at 04:14:35PM -0300, Fabiano Rosas wrote:
>>>> Introduce a new MigrationConfigBase, to allow most of the duplication
>>>> in migration.json to be eliminated.
>>>>
>>>> The reason we need MigrationParameters and MigrationSetParameters is
>>>> that the internal parameter representation in the migration code, as
>>>> well as the user-facing return of query-migrate-parameters use one
>>>> type for the TLS options (tls-creds, tls-hostname, tls-authz), while
>>>> the user-facing input from migrate-set-parameters uses another.
>>>>
>>>> The difference is in whether the NULL values is accepted. The former
>>>> considers NULL as invalid, while the latter doesn't.
>>>>
>>>> Move all other (non-TLS) options into the new type and make it a base
>>>> type for the two diverging types so that each child type can declare
>>>> the TLS options in its own way.
>>>>
>>>> Nothing changes in the user API, nothing changes in the internal
>>>> representation, but we save several lines of duplication in
>>>> migration.json.
>>>>
>>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>>> ---
>>>> qapi/migration.json | 358 +++++++++++++-------------------------------
>>>> 1 file changed, 108 insertions(+), 250 deletions(-)
>>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index 8b9c53595c..5a4d5a2d3e 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -914,202 +914,6 @@
>>>
>>>> @@ -1277,45 +1059,121 @@
>>>> # only has effect if the @mapped-ram capability is enabled.
>>>> # (Since 9.1)
>>>> #
>>>> +# @tls: Whether to use TLS. If this is set the options @tls-authz,
>>>> +# @tls-creds, @tls-hostname are mandatory and a valid string is
>>>> +# expected. (Since 10.1)
>>>> +#
>>>
>>> I'm not really finding it compelling to add a bool @tls as it
>>> is just a denormalization of !!@tls-creds.
>>>
>>
>> This is here by mistake.
>>
>> I remember Markus mentioning that implying TLS usage from tls-creds was
>> undesirable. I was prototyping a way of requiring an explicit opt-in.
>
> I don't remember a thing :)
>
I hope I interpreted you correctly:
"We have three syntactically independent parameters: @tls-creds,
@tls-hostname, @tls-authz. The latter two are meaningless (and silently
ignored) unless the first one is given. I hate that.
TLS is off by default. To enable it, set @tls-creds. Except setting it
to the otherwise invalid value "" disables it. That's because all we
have for configuration is the "set migration parameter to value"
interface. Only works because we have an invalid value we can abuse. I
hate that." -- https://lore.kernel.org/all/877cnrjd71.fsf@pond.sub.org/
I still think it might be an improvement to define a new interface that
requires explicitly enabling TLS. IOW, make tls a capability. But I
really don't want to get caught on that right now, one mess at a time.
>>> Incidentally the docs here are wrong - TLS can be used by
>>> only setting @tls-creds. The @tls-authz & @tls-hostname
>>> params are always optional.
>>>
>>>> # Features:
>>>> #
>>>> # @unstable: Members @x-checkpoint-delay and
>>>> # @x-vcpu-dirty-limit-period are experimental.
>>>> #
>>>> +# Since: 10.1
>>>> +##
>>>> +{ 'struct': 'MigrationConfigBase',
>>>> + 'data': { '*announce-initial': 'size',
>>>> + '*announce-max': 'size',
>>>> + '*announce-rounds': 'size',
>>>> + '*announce-step': 'size',
>>>> + '*throttle-trigger-threshold': 'uint8',
>>>> + '*cpu-throttle-initial': 'uint8',
>>>> + '*cpu-throttle-increment': 'uint8',
>>>> + '*cpu-throttle-tailslow': 'bool',
>>>> + '*max-bandwidth': 'size',
>>>> + '*avail-switchover-bandwidth': 'size',
>>>> + '*downtime-limit': 'uint64',
>>>> + '*x-checkpoint-delay': { 'type': 'uint32',
>>>> + 'features': [ 'unstable' ] },
>>>> + '*multifd-channels': 'uint8',
>>>> + '*xbzrle-cache-size': 'size',
>>>> + '*max-postcopy-bandwidth': 'size',
>>>> + '*max-cpu-throttle': 'uint8',
>>>> + '*multifd-compression': 'MultiFDCompression',
>>>> + '*multifd-zlib-level': 'uint8',
>>>> + '*multifd-qatzip-level': 'uint8',
>>>> + '*multifd-zstd-level': 'uint8',
>>>> + '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
>>>> + '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
>>>> + 'features': [ 'unstable' ] },
>>>> + '*vcpu-dirty-limit': 'uint64',
>>>> + '*mode': 'MigMode',
>>>> + '*zero-page-detection': 'ZeroPageDetection',
>>>> + '*direct-io': 'bool',
>>>> + '*tls': 'bool' } }
>>>
>>>
>>>> { 'struct': 'MigrationParameters',
>>>> + 'base': 'MigrationConfigBase',
>>>> + 'data': { 'tls-creds': 'str',
>>>> + 'tls-hostname': 'str',
>>>> + 'tls-authz': 'str' } }
>>>
>>> snip
>>>
>>>> +{ 'struct': 'MigrateSetParameters',
>>>> + 'base': 'MigrationConfigBase',
>>>> + 'data': { '*tls-creds': 'StrOrNull',
>>>> + '*tls-hostname': 'StrOrNull',
>>>> + '*tls-authz': 'StrOrNull' } }
>>>
>>> I recall we discussed this difference a year or two ago, but can't
>>> recall what the outcome was.
>>>
>>> Making the TLS params optional is a back compatible change for
>>> MigrationParameters. I would think replacing 'str' with 'StrOrNull'
>>> is also back compatible. So I'm wondering if we can't just unify
>>> the sttructs fully for TLS, even if one usage scenario never actually
>>> needs the "OrNull" bit nor needs the optionality
>>>
>>
>> MigrationParameters is the output type for query-migrate-parameters. I
>> belive it must be all non-optional to keep compatibility. The docs on
>> master say: "The optional members aren't really optional"
>
> To be precise: even though the members are declared optional, they must
> always be present.
>
> The members became optional when commit de63ab61241 (migrate: Share
> common MigrationParameters struct) reused MigrationParameters as
> migrate-set-parameters argument type.
>
Then, commit 1bda8b3c69 (migration: Unshare MigrationParameters struct
for now, 2017-07-18) partially undid that change and introduced
MigrateSetParameters.
At that point we might have been able to make the MigrationParameters'
members mandatory, but it was chosen not too, probably with good
reason. I can't tell from the commit message.
Strictly speaking, we cannot make them mandatory now because commit
31e4c354b3 (migration: Add block-bitmap-mapping parameter, 2020-08-20)
added the block-bitmap parameter, but made its output in
query-migrate-parameters truly optional:
if (s->parameters.has_block_bitmap_mapping) {
params->has_block_bitmap_mapping = true;
params->block_bitmap_mapping =
QAPI_CLONE(BitmapMigrationNodeAliasList,
s->parameters.block_bitmap_mapping);
}
> Making mandatory members of some type optional is compatible as long as
> the type occurs only in command arguments. But MigrationParameters is a
> command return type. Making its members optional was not kosher.
> Because the optional members are always present, QMP didn't actually
> change, except for introspection.
>
>> For MigrateSetParameters they've always been optional and continue to
>> be. There we need to keep StrOrNull for compat.
>
> StrOrNull goes back to commit 01fa5598269 (migration: Use JSON null
> instead of "" to reset parameter to default).
>
> Similar argument: adding values to a member is compatible as long as the
> type occurs only in command arguments. But MigrationParameters is a
> command return type. Adding value null is won't be kosher. However, as
> long as as the value is never actually used there, QMP won't actually
> change, except for introspection.
>
> If this was a clean and obvious interface, I'd argue against such
> trickery and for keeping it clean and obvious. But the migration
> configuration interface isn't.
Fabiano Rosas <farosas@suse.de> writes:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>
>>>> On Fri, Apr 11, 2025 at 04:14:35PM -0300, Fabiano Rosas wrote:
>>>>> Introduce a new MigrationConfigBase, to allow most of the duplication
>>>>> in migration.json to be eliminated.
>>>>>
>>>>> The reason we need MigrationParameters and MigrationSetParameters is
>>>>> that the internal parameter representation in the migration code, as
>>>>> well as the user-facing return of query-migrate-parameters use one
>>>>> type for the TLS options (tls-creds, tls-hostname, tls-authz), while
>>>>> the user-facing input from migrate-set-parameters uses another.
>>>>>
>>>>> The difference is in whether the NULL values is accepted. The former
>>>>> considers NULL as invalid, while the latter doesn't.
>>>>>
>>>>> Move all other (non-TLS) options into the new type and make it a base
>>>>> type for the two diverging types so that each child type can declare
>>>>> the TLS options in its own way.
>>>>>
>>>>> Nothing changes in the user API, nothing changes in the internal
>>>>> representation, but we save several lines of duplication in
>>>>> migration.json.
>>>>>
>>>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>>>> ---
>>>>> qapi/migration.json | 358 +++++++++++++-------------------------------
>>>>> 1 file changed, 108 insertions(+), 250 deletions(-)
>>>>>
>>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>>> index 8b9c53595c..5a4d5a2d3e 100644
>>>>> --- a/qapi/migration.json
>>>>> +++ b/qapi/migration.json
>>>>> @@ -914,202 +914,6 @@
>>>>
>>>>> @@ -1277,45 +1059,121 @@
>>>>> # only has effect if the @mapped-ram capability is enabled.
>>>>> # (Since 9.1)
>>>>> #
>>>>> +# @tls: Whether to use TLS. If this is set the options @tls-authz,
>>>>> +# @tls-creds, @tls-hostname are mandatory and a valid string is
>>>>> +# expected. (Since 10.1)
>>>>> +#
>>>>
>>>> I'm not really finding it compelling to add a bool @tls as it
>>>> is just a denormalization of !!@tls-creds.
>>>>
>>>
>>> This is here by mistake.
>>>
>>> I remember Markus mentioning that implying TLS usage from tls-creds was
>>> undesirable. I was prototyping a way of requiring an explicit opt-in.
>>
>> I don't remember a thing :)
>>
>
> I hope I interpreted you correctly:
>
> "We have three syntactically independent parameters: @tls-creds,
> @tls-hostname, @tls-authz. The latter two are meaningless (and silently
> ignored) unless the first one is given. I hate that.
>
> TLS is off by default. To enable it, set @tls-creds. Except setting it
> to the otherwise invalid value "" disables it. That's because all we
> have for configuration is the "set migration parameter to value"
> interface. Only works because we have an invalid value we can abuse. I
> hate that." -- https://lore.kernel.org/all/877cnrjd71.fsf@pond.sub.org/
Ah, now I remember!
Possible minimally invasive improvement here:
* Reject @tls-hostname and @tls-authz when the value of @tls-creds means
"TLS is off".
* When @tls-set-creds get set to a value that means "TLS is off",
@tls-hostname and @tls-authz get unset.
This could break applications. I'm not at all sure the slight reduction
in ugliness would be worth the risk.
I do not mean to discourage pursuing your RFC! The TLS configuration is
ugly and confusing to use internally. Getting that streamlined would be
nice.
> I still think it might be an improvement to define a new interface that
> requires explicitly enabling TLS. IOW, make tls a capability. But I
> really don't want to get caught on that right now, one mess at a time.
First: one mess at a time is commonly the sensible approach.
Second: I'm not sure attempts to make the existing migration
configuration interfaces somewhat less ugly and confusing are
worthwhile.
I rarely advocate starting over just because something is ugly.
However, migration configuration is not only ugly; its use of global
state feels fundamentally misguided to me. Evidence: the surprising
side effects on savevm Daniel mentioned. I'll add: when a management
application connects to an existing QEMU, it needs to laboriously set
the entire global migration configuration state, or else rely on unsound
assumptions. Complete non-problem when the entire configuration is
passed to the command, like we do elsewhere.
>>>> Incidentally the docs here are wrong - TLS can be used by
>>>> only setting @tls-creds. The @tls-authz & @tls-hostname
>>>> params are always optional.
>>>>
>>>>> # Features:
>>>>> #
>>>>> # @unstable: Members @x-checkpoint-delay and
>>>>> # @x-vcpu-dirty-limit-period are experimental.
>>>>> #
>>>>> +# Since: 10.1
>>>>> +##
>>>>> +{ 'struct': 'MigrationConfigBase',
>>>>> + 'data': { '*announce-initial': 'size',
>>>>> + '*announce-max': 'size',
>>>>> + '*announce-rounds': 'size',
>>>>> + '*announce-step': 'size',
>>>>> + '*throttle-trigger-threshold': 'uint8',
>>>>> + '*cpu-throttle-initial': 'uint8',
>>>>> + '*cpu-throttle-increment': 'uint8',
>>>>> + '*cpu-throttle-tailslow': 'bool',
>>>>> + '*max-bandwidth': 'size',
>>>>> + '*avail-switchover-bandwidth': 'size',
>>>>> + '*downtime-limit': 'uint64',
>>>>> + '*x-checkpoint-delay': { 'type': 'uint32',
>>>>> + 'features': [ 'unstable' ] },
>>>>> + '*multifd-channels': 'uint8',
>>>>> + '*xbzrle-cache-size': 'size',
>>>>> + '*max-postcopy-bandwidth': 'size',
>>>>> + '*max-cpu-throttle': 'uint8',
>>>>> + '*multifd-compression': 'MultiFDCompression',
>>>>> + '*multifd-zlib-level': 'uint8',
>>>>> + '*multifd-qatzip-level': 'uint8',
>>>>> + '*multifd-zstd-level': 'uint8',
>>>>> + '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ],
>>>>> + '*x-vcpu-dirty-limit-period': { 'type': 'uint64',
>>>>> + 'features': [ 'unstable' ] },
>>>>> + '*vcpu-dirty-limit': 'uint64',
>>>>> + '*mode': 'MigMode',
>>>>> + '*zero-page-detection': 'ZeroPageDetection',
>>>>> + '*direct-io': 'bool',
>>>>> + '*tls': 'bool' } }
>>>>
>>>>
>>>>> { 'struct': 'MigrationParameters',
>>>>> + 'base': 'MigrationConfigBase',
>>>>> + 'data': { 'tls-creds': 'str',
>>>>> + 'tls-hostname': 'str',
>>>>> + 'tls-authz': 'str' } }
>>>>
>>>> snip
>>>>
>>>>> +{ 'struct': 'MigrateSetParameters',
>>>>> + 'base': 'MigrationConfigBase',
>>>>> + 'data': { '*tls-creds': 'StrOrNull',
>>>>> + '*tls-hostname': 'StrOrNull',
>>>>> + '*tls-authz': 'StrOrNull' } }
>>>>
>>>> I recall we discussed this difference a year or two ago, but can't
>>>> recall what the outcome was.
>>>>
>>>> Making the TLS params optional is a back compatible change for
>>>> MigrationParameters. I would think replacing 'str' with 'StrOrNull'
>>>> is also back compatible. So I'm wondering if we can't just unify
>>>> the sttructs fully for TLS, even if one usage scenario never actually
>>>> needs the "OrNull" bit nor needs the optionality
>>>>
>>>
>>> MigrationParameters is the output type for query-migrate-parameters. I
>>> belive it must be all non-optional to keep compatibility. The docs on
>>> master say: "The optional members aren't really optional"
>>
>> To be precise: even though the members are declared optional, they must
>> always be present.
>>
>> The members became optional when commit de63ab61241 (migrate: Share
>> common MigrationParameters struct) reused MigrationParameters as
>> migrate-set-parameters argument type.
>>
>
> Then, commit 1bda8b3c69 (migration: Unshare MigrationParameters struct
> for now, 2017-07-18) partially undid that change and introduced
> MigrateSetParameters.
>
> At that point we might have been able to make the MigrationParameters'
> members mandatory, but it was chosen not too, probably with good
> reason. I can't tell from the commit message.
As far as I remember: to minimize last minute churn, and to not
complicate a future re-sharing.
> Strictly speaking, we cannot make them mandatory now because commit
> 31e4c354b3 (migration: Add block-bitmap-mapping parameter, 2020-08-20)
> added the block-bitmap parameter, but made its output in
> query-migrate-parameters truly optional:
>
> if (s->parameters.has_block_bitmap_mapping) {
> params->has_block_bitmap_mapping = true;
> params->block_bitmap_mapping =
> QAPI_CLONE(BitmapMigrationNodeAliasList,
> s->parameters.block_bitmap_mapping);
> }
We're consistently inconsistent ;)
>> Making mandatory members of some type optional is compatible as long as
>> the type occurs only in command arguments. But MigrationParameters is a
>> command return type. Making its members optional was not kosher.
>> Because the optional members are always present, QMP didn't actually
>> change, except for introspection.
>>
>>> For MigrateSetParameters they've always been optional and continue to
>>> be. There we need to keep StrOrNull for compat.
>>
>> StrOrNull goes back to commit 01fa5598269 (migration: Use JSON null
>> instead of "" to reset parameter to default).
>>
>> Similar argument: adding values to a member is compatible as long as the
>> type occurs only in command arguments. But MigrationParameters is a
>> command return type. Adding value null is won't be kosher. However, as
>> long as as the value is never actually used there, QMP won't actually
>> change, except for introspection.
>>
>> If this was a clean and obvious interface, I'd argue against such
>> trickery and for keeping it clean and obvious. But the migration
>> configuration interface isn't.
© 2016 - 2025 Red Hat, Inc.