Peter Xu <pet...@redhat.com> writes:
> 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?
>
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.
> 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?
>
I don't think other apps is the problem, but libvirt itself maybe
attempting two migrations in sequence after one of them fails.
There always the possibility that the user is poking around, which of
course is not advisable, but if a weird migration bug shows up it's
difficult to confirm that other app/user hasn't changed the parameters.
> 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.
>
We can provide it, but I'd rather not unless we agree that is the way
forward. We don't need another way of doing the same as existing
commands.
> 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..
I don't see it as breaking the old stuff. Because any old users would
still be using migrate-set-parameters as usual. So I think your concern
is about calling migrate the new way and also keeping -global
working. As I said, personally I don't mind if put some ifs around to
keep -global working.
Could we add another parameter that says allow-globals (or w/e) and make
everyone happy?
