Re: [RFC PATCH 05/13] migration: Reduce a bit of duplication in migration.json

[Markus Armbruster <armbru@redhat.com>]

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.