Re: [PATCH 1/4] migration/savevm: Add a compatibility check for capabilities

[Fabiano Rosas <farosas@suse.de>]

Fabiano Rosas <farosas@suse.de> writes:

+Cc Markus

context:
This series was trying to stop savevm from crashing when arbitrary
migration capabilities are enabled. Daniel brought up the previous
discussion around unifying capabilities + parameters and passing it all
via the migrate (or snapshot in this case) command arguments. I'm
looking into that now.

> Fabiano Rosas <farosas@suse.de> writes:
>
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>>> On Thu, Mar 27, 2025 at 11:39:31AM -0300, Fabiano Rosas wrote:
>>>> It has always been possible to enable arbitrary migration capabilities
>>>> and attempt to take a snapshot of the VM with the savevm/loadvm
>>>> commands as well as their QMP counterparts
>>>> snapshot-save/snapshot-load.
>>>> 
>>>> Most migration capabilities are not meant to be used with snapshots
>>>> and there's a risk of crashing QEMU or producing incorrect
>>>> behavior. Ideally, every migration capability would either be
>>>> implemented for savevm or explicitly rejected.
>>>
>>> IMHO, this a prime example of why migration config shouldn't be held
>>> as global state, and instead passed as parameters to the commands
>>> that need them.  The snapshot-save/load commands would then only
>>> be able to accept what few settings are actually relevant, instead
>>> of inheriting any/all global migration state.
>>>
>>
>> Right, I remember we got caught around the fact that some migration
>> options are needed during runtime as well... but I don't remember the
>> details, let try to find that thread.
>>
>
> Found it: https://lore.kernel.org/r/ZVM5xmsaE41WJYgb@redhat.com
>
> I don't think it's *too* hard to start passing the configuration to
> qmp_migrate & friends. We just need to figure out a path for the
> compatibility.
>
> I'm thiking of:
>
> 1) Unifying capabilities and parameters in a MigrationConfig
> structure. We take the opportunity to fix the tls options to 'str'
> instead of StrOrNull.
>
> 2) Deprecate migrate-set-capabilities. There are no capabilities
> anymore.
>
> 3) Deprecate migate-set-parameters. There are no parameters
> anymore. Alternatively, reuse the existing command, but have it take the
> additional capabilities as optional (everything else is already
> optional).
>
> 4) Depending on what we do on (3), add a new migrate-set-config command
> that sets every option. All as optional. This would be nice because we
> wouldn't need to worry about breaking compat on the tls options, we just
> define the new command in the correct way.
>
> 5) Add a {'*config': MigrationConfig} entry to qmp_migrate and
> migrate_set_{config|parameters}. Here is where I have questions, because
> ideally we'd have a way to limit the migrate_set_config command to only
> the options that can be set at runtime. But I can't see a way of doing
> that without the duplication of the options in the QAPI .json file. I'm
> inclined to allow the whole set of options and do some tracking on the
> side in options.c in the migration code.
>
> (same issue for savevm really. To allow it to (say) work with
> mapped-ram, we'd need a duplicate mapped-ram entry in migration.json)
>
> About (2) and (3). If we use this unified MigrationConfig, I can keep
> the old commands working (along with the query_* variants), by defining
> a compat function that converts from those commands specific format into
> the new format. But then there's the question of what do we do when a
> new capability/parameter comes along? Can we declare that the old
> commands will not see the new data and that's it? If there's no
> distinction between caps and params anymore, there isn't even a way to
> decide which command to use.
>