Fabiano Rosas <faro...@suse.de> writes: > Fabiano Rosas <faro...@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 <faro...@suse.de> writes: >> >>> Daniel P. Berrangé <berra...@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/zvm5xmsae41wj...@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. >>
Hi all, please disregard my messages on this thread. I've posted a series which has a cover letter that explains the situation better: [RFC PATCH 00/13] migration: Unify capabilities and parameters https://lore.kernel.org/r/20250411191443.22565-1-faro...@suse.de
