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

[Peter Xu <peterx@redhat.com>]

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?

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.

One step back, on this "allow migrate to specify 'config'" request: I think
we can definitely do that as it still provides some kind of atomicity.  But
frankly speaking I never see it a "real problem" - do we really have report
or use case showing that Libvirt can trigger "migrate" with some global
settings touched by other apps at all?

To me, it was yet an illutionary problem, I never know the answer of that.
If Libvirt is still the owner of QEMU instance via the QMP channel, I
actually don't really see why the atomicity would even help, even though we
can still provide that as it's pretty easy as something optional; like what
this patch does without too much hassle.

Then if to move one step further to remove all global settings, we face
breaking debugging scripts, and breaking of any old libvirt and non-libvirt
mgmt apps.  Frankly I really don't yet know whether it's a good idea.  I
could miss some important reasoning of why we want to do it - it needs to
be something not relevant to "making the code cleaner", IMHO..

-- 
Peter Xu