Hi everyone, I did a cleanup (if it can be called that) of the user
input validation for capabilities and parameters and turned the two
concepts into a single 'options' to be stored in a MigrationConfig
object.
RFC mostly because this idea exposes (pre-existing) issues around how
to validate capabilities that are mutually excludent and options that
need to be enabled together.
I'd also like some feedback on what approach to take regarding
compatibility.
Let me know what you think. I know it's a lot to look at, any comments
are welcomed and don't worry on trying to cover everything. This is
long-term work. Thank you.
The reasons for this work are:
------------------------------
Capabilities are just boolean parameters that can only be set before
migration. For the majority of the code there's no distinction between
the two.
Having a single structure allows the qmp_migrate command to receive
the migration configuration all at once:
{ 'command': 'migrate',
'data': {'*uri': 'str',
'*channels': [ 'MigrationChannel' ],
+ '*config': 'MigrationConfig',
'*detach': 'bool', '*resume': 'bool' } }
+{ 'struct': 'MigrationConfig',
+ 'data': { '*announce-initial': 'size',
+ '*announce-max': 'size',
+ ... <-- all parameters and capabilities as optional
+} }
(optionally fold 'detach' and 'resume' into MigrationConfig)
Other benefits of a single configuration struct:
- allows the removal of two commands:
migrate-set-capabilities/query-capabilities which simplifies the
user interface (migrate-set-parameters, or similar, is still
required for options that need to be adjusted during migration);
- removes (some of) the triplication in migration.json;
- simplifies the (future) work of implementing a handshake feature for
migration: only one structure to negotiate over and less reliance on
commands other than 'migrate'.
The major changes in this series are:
-------------------------------------
- Add a (QAPI) type hierarchy:
MigrationConfigBase
(most config options)
|
+-------------------------|-------------------------+
| | |
MigrationConfig MigrationParameters MigrateSetParameters
(internal use, s->config, (compat with (compat with
new query/set-config) query-migrate-parameters) migrate-set-parameters)
- Remove migrate_params_test_apply. This function duplicates a lot of code;
- Add compatibility routines to convert from the existing QMP user
input into the new MigrationConfig for internal use;
- Merge capabilities and parameters validation into one function;
- Convert qmp_migrate and qmp_migrate_incoming to use the new structure.
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.
- HMP?
Can we convert the strings passed via hmp_set_parameters without
having an enum of parameters? Duplication problem again.
- incoming defer?
It seems we cannot do the final step of removing
migrate-set-capabilites before we have a form of handshake
implemented. That would take the config from qmp_migrate on source and
send it to the destination for negotiation.
- last but definitely not least:
Changing caps from a list of bools into struct members complicates the
validation because some caps must be checked against every other cap
already set. And the user could be playing games with switching caps
on and off, so there's a lot of redundant checking the must be
made. The current code already has a bunch of gaps in that regard.
For this series I opted to not check has_* fields for capabilities,
i.e. to validate them all every time migrate_config_check() is called.
Fabiano Rosas (12):
migration: Normalize tls arguments
migration: Run a post update routine after setting parameters
migration: Fix parameter validation
migration: Reduce a bit of duplication in migration.json
migration: Remove the parameters copy during validation
migration: Introduce new MigrationConfig structure
migration: Replace s->parameters with s->config
migration: Do away with usage of QERR_INVALID_PARAMETER_VALUE
migration: Replace s->capabilities with s->config
migration: Merge parameters and capability checks
[PoC] migration: Add query/set commands for MigrationConfig
[PoC] migration: Allow migrate commands to provide the migration
config
Markus Armbruster (1):
migration: Fix latent bug in migrate_params_test_apply()
migration/migration-hmp-cmds.c | 5 +-
migration/migration.c | 52 +-
migration/migration.h | 5 +-
migration/options.c | 1386 ++++++++++++++++----------------
migration/options.h | 25 +-
migration/page_cache.c | 6 +-
migration/ram.c | 9 +-
migration/savevm.c | 8 +-
migration/tls.c | 2 +-
qapi/migration.json | 571 +++++++------
system/vl.c | 3 +-
11 files changed, 1079 insertions(+), 993 deletions(-)
--
2.35.3
