Re: [RFC PATCH 00/13] migration: Unify capabilities and parameters

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>>> On Fri, Apr 11, 2025 at 04:14:30PM -0300, Fabiano Rosas wrote:
>>>> Open questions:
>>>> ---------------
>>>> 
>>>> - Deprecations/compat?
>>>> 
>>>> I think we should deprecate migrate-set/query-capabilities and everything to do
>>>> with capabilities (specifically the validation in the JSON at the end of the
>>>> stream).
>>>> 
>>>> For migrate-set/query-parameters, we could probably keep it around indefinitely,
>>>> but it'd be convenient to introduce new commands so we can give them new
>>>> semantics.
>>>> 
>>>> - How to restrict the options that should not be set when the migration is in
>>>> progress?
>>>> 
>>>> i.e.:
>>>>   all options can be set before migration (initial config)
>>>>   some options can be set during migration (runtime)
>>>> 
>>>> I thought of adding another type at the top of the hierarchy, with
>>>> just the options allowed to change at runtime, but that doesn't really
>>>> stop the others being also set at runtime. I'd need a way to have a
>>>> set of options that are rejected 'if migration_is_running()', without
>>>> adding more duplication all around.
>>>> 
>>>> - What about savevm?
>>>> 
>>>> None of this solves the issue of random caps/params being set before
>>>> calling savevm. We still need to special-case savevm and reject
>>>> everything. Unless we entirely deprecate setting initial options via
>>>> set-parameters (or set-config) and require all options to be set as
>>>> savevm (and migrate) arguments.
>>>
>>> I'd suggest we aim for a world where the commands take all options
>>> as direct args and try to remove the global state eventually.
>>
>> Yes.
>>
>> Even better: make it a job.
>
> What do we gain from that in relation to being able to pass ~50
> parameters to a command? I don't see it. I think that's the actual crux
> here, too many options and how to get them from the QAPI into the
> migration core for consumption.

Two separate interface design problems:

1. Query and manipulate complex configuration

2. Control and monitor long-running jobs

Let's start with 1.

Migration is not the only instance of complex configuration.  Block
devices are arguably even more complex.  A difference: we can have many
block devices, but just one migration.

Here's the usual way to configure things: commands creating a thing take
the thing's configuration as arguments, other commands query and
manipulate a thing's configuration.  Works fine both for simple and
complex configuration.  Block devices work this way.

Migration doesn't.  Instead, we have global configuration state, and
commands to query and manipulate it.  Also works.

Instead of passing the entire configuration to the "create" command, you
can pass it piecemeal to "manipulate global configuration" commands.
You pay for the convenience by having to reset the entire global
configuration to a known state in certain situations.  Since use of the
global configuration state is implicit, it can be surprising, as Daniel
pointed out for savevm.

Overall, this isn't simpler or more convenient than the usual way, just
different.

Both ways use QAPI objects to hold configuration.  Query commands can
return such an object.  Commands that deal with partial configuration
can only take the same object when its members are optional.

In places where configuration is complete (internal configuration state,
the query command that returns it), optional makes no sense, and is
annoying.  We either accept that, or duplicate the configuration object.
Duplicating violates DRY.  It does result in better command
documentation, though.

On to 2.

Migration is not the only kind of long-running job.  There's also
block-commit, block-stream, drive-mirror, drive-backup, blockdev-create,
x-blockdev-amend, snapshot-load, snapshot-save, snapshot-delete.

These all create a "job" object that represents the long-running job.
We have commands and events to control and monitor such jobs: job-pause,
job-resume, job-cancel, job-complete, job-dismiss, job-finalize,
query-jobs, JOB_STATUS_CHANGE.

Migration does its own thing instead.

> The current usage of MigrationParameters as both the return type for
> query-set-parameters and the global parameter store for the migration
> state is really dissonant. What do the has_* fields even mean when
> accessed via MigrationState::parameters?

Yes, it's ugly, and the alternative is differently ugly, as discussed
above.

>                                          This series is not doing any
> better in that regard, mind you. I'm almost tempted to ask that we
> expose the QDict from the marshaling function directly to the migration
> code, at least that's a data type that makes sense internally.

Based on experience in the block layer, I predict you'd regret this :)

Use of native C data types is just so much easier on the eyes than
messing with QObjects.  Moreover, the compiler can't help you as much
with QObjects.