Daniel P. Berrangé <berra...@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 <faro...@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
