Fabiano Rosas <faro...@suse.de> writes:
> Markus Armbruster <arm...@redhat.com> writes:
>
>> Fabiano Rosas <faro...@suse.de> writes:
>>
>>> 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.
>>
>> 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....@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.
