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

[Peter Xu <peterx@redhat.com>]

On Mon, Jun 09, 2025 at 04:03:01PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 06, 2025 at 04:50:54PM -0400, Peter Xu wrote:
> > 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?
> > 
> > I think there're actually two separate questions to be asked, to make it
> > clearer, they are:
> > 
> >   (1) Whether we should allow QMP "migrate" 'config' parameter to overwrite
> >       global setup?
> > 
> >   (2) Whether we should allow previous QMP global setup to be used even if
> >       QMP "migrate" provided 'config' parameter?
> > 
> > So IIUC the patch does (1) YES (2) NO, while what I think might be more
> > intuitive is (1) NO (2) YES.
> 
> The point of the 'config' parameter to the 'migrate' command is to
> enable the mgmt app to fully specify what it wants the configuration
> to be, such that there is no previously set state will will cause
> it surprises. Allowing -global to have an effect undermines the
> predictibility in the same way that migrate-set-parameter undermines
> the predictibility.

Now I think I know part of what I've missed: I used to think the "config"
of per-QMP-migrate-command can be totally temporary for a specific
migration request, but then we need another MigrationState.parameters_2 to
cache the old or vice versa; that's probably not necessary.  Now I think it
makes sense to overwrite any settings directly, hence I think I changed my
mind on question (1), YES is fine here.

For (2), why it would introduce any uncertainty for mgmt?

If the mgmt app can both: (1) query from qapi schema knowing all the
parameters supported, then (2) specify all the parameters in QMP migrate's
"option" parameter.  Then it's literally overwritting all the parameters,
so it's predictable with or without completely removing global settings as
an idea?  To me, the "option" is the key to make QMP migrate command and
parameter/cap setup in one "atomic-like" operation, and that provides the
predictability if the command succeeded and if all the parameters are
specified (otherwise it'll fail saying migration in progress, internally
protected by BQL or whatever lock QMP monitor holds).

Thanks,

-- 
Peter Xu