On Mon, Jun 09, 2025 at 11:51:17AM -0400, Peter Xu wrote:
> On Mon, Jun 09, 2025 at 11:37:02AM -0300, Fabiano Rosas wrote:
> > Peter Xu <pet...@redhat.com> writes:
> >
> > > On Fri, Jun 06, 2025 at 05:23:18PM -0300, Fabiano Rosas wrote:
> > >> Peter Xu <pet...@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?
Yes, that's the whole point of the exercise IMHO.
> 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.
The benefit is that it brings 'migrate' into line with all other
QMP commands, such that the data provided with the command is
precisely what controls behaviour, giving predictable behaviour.
> 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?
atomicity is only one of the goals - being free from side effects
of externally set global state is the more important aspect.
> 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.
Even if only a single mgmt app is involved this is still beneficial
because the migration infrastructure is used for distinct use cases
inside QEMU - live migration, CPR, save/restore, and savevm/loadvm.
Any time code any one of those uses cases starts using a new parameter,
apps have to make sure they don't inadvertantly have its effects apply
to the other use cases.
> 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..
Again, it makes the behaviour predictable as the QMP command fully expresses
what action is going to be peformed, free with side effets of any previously
set state.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
