Re: [PATCH 19/21] migration: Allow migrate commands to provide the migration config

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jun 09, 2025 at 04:41:06PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Mon, Jun 09, 2025 at 03:02:06PM -0300, Fabiano Rosas wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > On Mon, Jun 09, 2025 at 11:37:02AM -0300, Fabiano Rosas wrote:
>> >> >> Peter Xu <peterx@redhat.com> writes:
>> >> >> 
>> >> >> > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
>> >> >> >> Peter Xu <peterx@redhat.com> writes:
>> >> >> >> 
>> >> >> >> > On Mon, Jun 02, 2025 at 10:38:08PM -0300, Fabiano Rosas wrote:
>> >> >> >> >> Allow the migrate and migrate_incoming commands to pass the migration
>> >> >> >> >> configuration options all at once, dispensing the use of
>> >> >> >> >> migrate-set-parameters and migrate-set-capabilities.
>> >> >> >> >> 
>> >> >> >> >> The motivation of this is to simplify the interface with the
>> >> >> >> >> management layer and avoid the usage of several command invocations to
>> >> >> >> >> configure a migration. It also avoids stale parameters from a previous
>> >> >> >> >> migration to influence the current migration.
>> >> >> >> >> 
>> >> >> >> >> The options that are changed during the migration can still be set
>> >> >> >> >> with the existing commands.
>> >> >> >> >> 
>> >> >> >> >> The order of precedence is:
>> >> >> >> >> 
>> >> >> >> >> 'config' argument > -global cmdline > defaults (migration_properties)
>> >> >> >> >
>> >> >> >> > Could we still keep the QMP migrate-set-parameters values?
>> >> >> >> >
>> >> >> >> >   'config' argument > QMP setups using migrate-set-parameters >
>> >> >> >> >     -global cmdline > defaults (migration_properties)
>> >> >> >> >
>> >> >> >> 
>> >> >> >> That's the case. I failed to mention it in the commit message. IOW it
>> >> >> >> behaves just like today, but the new 'config' way takes precedence over
>> >> >> >> all.
>> >> >> >
>> >> >> > Referring to below chunk of code:
>> >> >> >
>> >> >> > [...]
>> >> >> >
>> >> >> >> >> +bool migrate_params_override(MigrationState *s, MigrationParameters *new,
>> >> >> >> >> +                             Error **errp)
>> >> >> >> >> +{
>> >> >> >> >> +    ERRP_GUARD();
>> >> >> >> >> +
>> >> >> >> >> +    assert(bql_locked());
>> >> >> >> >> +
>> >> >> >> >> +    /* reset to default parameters */
>> >> >> >> >> +    migrate_params_apply(&s->defaults);
>> >> >> >
>> >> >> > IIUC here it'll reset all global parameters using the initial defaults
>> >> >> > first, then apply the "config" specified in "migrate" QMP command?
>> >> >> >
>> >> >> 
>> >> >> Yes, this is so any previously set parameter via migrate-set-parameter
>> >> >> gets erased. I think what we want (but feel free to disagree) is to have
>> >> >> the migrate-set-parameter _eventually_ only handle parameters that need
>> >> >> to be modifed during migration runtime. Anything else can be done via
>> >> >> passing config to qmp_migrate.
>> >> >> 
>> >> >> For -global, I don't have a preference. Having -global take precedence
>> >> >> over all would require a way to know which options were present in the
>> >> >> command-line and which are just the defaults seet in
>> >> >> migration_properties. I currently don't know how to do that. If it is at
>> >> >> all possible (within reason) we could make the change, no worries.
>> >> >> 
>> >> >> > I think there're actually two separate questions to be asked, to make it
>> >> >> > clearer, they are:
>> >> >> 
>> >> >> Here it got ambiguous when you say "global", I've been using -global to
>> >> >> refer to the cmdline -global migration.foo, but others have used global
>> >> >> to mean s->parameters (which has an extended lifetime). Could you
>> >> >> clarify?
>> >> >
>> >> > I meant the -global, and the global setups via migrate-set-parameters.
>> >> >
>> >> > As replied to Dan in the other email, I changed my mind on question (1); I
>> >> > think it makes sense to have it YES.  I left my pure question on (2) there
>> >> > too.
>> >> >
>> >> > Do we really want to disable migrate-set-parameters setting most of the
>> >> > parameters, and only allow it to be set during migration on a few things
>> >> > like bandwidth or so?
>> >> >
>> >> 
>> >> Well, if we decide we have reasons to introduce the "config" concept,
>> >> then I think we should not present two ways of doing the same
>> >> thing. User calls qmp_migrate with its arguments and that's the
>> >> migration. No other ways of setting parameters.
>> >> 
>> >> Since we do have parameters that are set in "runtime" I though of
>> >> keeping migrate-set-parameters around to minimize the interface
>> >> change. Maybe those should have been separate knobs on their own after
>> >> all... But in any case, we can't reject migrate-set-parameters because
>> >> it might happen way earlier than the actual migration command. So I
>> >> don't think anything changes regarding the API.
>> >> 
>> >> > I just don't really see the major benefit of that yet.  I would think it
>> >> > make more sense if we don't need to change any parameters in migration,
>> >> > then provide that in one shot in QMP migrate "config".  Maybe making more
>> >> > sense if migration is not heavily thread-based but having its aiocontext so
>> >> > we could even move to Jobs.
>> >> >
>> >> > Now after all we'll need to allow setting something like bandwidth even
>> >> > during migration alive, and we have all the things ready allowing to set
>> >> > before migration starts, I'm not 100% sure whether we need to bother even
>> >> > if it does look cleaner, because we'll still break mgmt used to be working
>> >> > for years.. I could be over-cautious on breaking things, but I still want
>> >> > to understand better on the benefits.
>> >> >
>> >> 
>> >> Makes sense. We'd say either use the old way or the new way. If both are
>> >> mixed, then the new way takes precedence. That keeps older apps working
>> >> and allows new code to transition into the new way.
>> >
>> > Fair enough.  Yes whenever the new way is chosen it can work in anyway we
>> > define it.
>> >
>> > It's just that if the global list of parameters will still be around then
>> > it seems to have no good reason to not build the migration parameters on
>> > top of the global list of parameters.  After all, anything can be
>> > overwritten in the QMP migrate if needed.
>> >
>> 
>> If we had a way to detect that the user has modified some parameters via
>> the cmdline, then we could merge that with the s->defaults and restore
>> it before applying config, that would achieve what you want. I'm in
>> favor, -global should only be used for debugging, I think it's fine if
>> we let it go through. But anything set by migrate-set-parameters
>> definitely needs to be reset. I just need a way to differentiate between
>> "default parameter" vs. "default parameter that got overwritten by
>> -global". I'll try to figure something out.
>
> I think I see what you meant.  Ignoring -global is ok.  I agree with you
> that should be pure debugging, and feel free to keep it like that if you
> can't find anything to persist it - it may not justify your time spent if
> it grows too much.
>

I think I caused some confusion here. I wrote migrate_params_override()
last thing on a friday and forgot it did the right thing from the
beginning:

    migrate_params_apply(&s->defaults);
    qmp_migrate_set_parameters(new, errp);

This s->defaults is poorly named and is actualy already the merge of
defaults + globals, because qdev does it for us. migrate_params_apply()
will then copy that to s->parameters and qmp_migrate_set_parameters()
will apply the 'new' params from 'config' on top s->parameters. An
example:

Setting multifd-channels (default 2) using various methods and querying
both QMP and HMP:

a) global overrides default:

 $ ./qemu-system-x86_64 -global migration.multifd-channels=4 ...
 => QMP: "multifd-channels": 4, HMP: multifd-channels: 4

b) migrate-set-parameter overrides global:

 { 'execute': 'migrate-set-parameters', 'arguments': { 'multifd-channels': 8 } }
 => QMP: "multifd-channels": 8, HMP: multifd-channels: 8

c) config not touching the parameter, value is reset to global:

 { 'execute': 'migrate', 'arguments': { ..., 'config': { 'multifd': true } } }
 => QMP: "multifd-channels": 4, HMP: multifd-channels: 4

d) config overrides all:

 { 'execute': 'migrate', 'arguments': { ..., 'config': {'multifd-channels': 16 } } }
 => QMP: "multifd-channels": 16, HMP: multifd-channels: 16

Without global:

e) default is set initially

 $ ./qemu-system-x86_64 ...
 => QMP: "multifd-channels": 2, HMP: multifd-channels: 2

f) migrate-set-parameter overrides default:

 { 'execute': 'migrate-set-parameters', 'arguments': { 'multifd-channels': 8 } }
 => QMP: "multifd-channels": 8, HMP: multifd-channels: 8

g) config not touching the parameter, value is reset to default:

 { 'execute': 'migrate', 'arguments': { ..., 'config': { 'multifd': true } } }
 => "multifd-channels": 2, HMP: multifd-channels: 2

h) config overrides all:

 { 'execute': 'migrate', 'arguments': { ..., 'config': {'multifd-channels': 16 } } }
 => QMP: "multifd-channels": 16, HMP: multifd-channels: 16

I'll update the variable names and code comments to be more
precise. Sorry for the noise.