On Thu, Dec 18, 2025 at 04:47:47PM -0300, Fabiano Rosas wrote:
> Peter Xu <[email protected]> writes:
>
> > On Mon, Dec 15, 2025 at 07:00:18PM -0300, Fabiano Rosas wrote:
> >> Adapt the convergence routines migrate_ensure_[non_]converge to set
> >> the convergence parameters in the config dict it instead of using
> >> migrate-set-parameters.
> >>
> >> Some tests need to change the convergence parameters during the
> >> migration. The config object method is specific to configuration prior
> >> to starting a migration, so by design it's not suitable to effect
> >> migration-runtime changes. The existing routines will be kept for this
> >> purpose (renamed with 'ongoing' for clarity).
> >>
> >> Signed-off-by: Fabiano Rosas <[email protected]>
> >> ---
> >> tests/qtest/migration/framework.c | 10 ++++-----
> >> tests/qtest/migration/migration-qmp.c | 32 +++++++++++++++++++++++++--
> >> tests/qtest/migration/migration-qmp.h | 6 +++--
> >> tests/qtest/migration/misc-tests.c | 4 ++--
> >> tests/qtest/migration/precopy-tests.c | 26 +++++++++-------------
> >> 5 files changed, 52 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/tests/qtest/migration/framework.c
> >> b/tests/qtest/migration/framework.c
> >> index fd15bd832e..df42a8a2c6 100644
> >> --- a/tests/qtest/migration/framework.c
> >> +++ b/tests/qtest/migration/framework.c
> >> @@ -583,7 +583,7 @@ static int migrate_postcopy_prepare(QTestState
> >> **from_ptr,
> >> args->postcopy_data = args->start_hook(from, to);
> >> }
> >>
> >> - migrate_ensure_non_converge(from);
> >> + migrate_ensure_non_converge(from, args->start.config);
> >> migrate_prepare_for_dirty_mem(from);
> >> qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
> >> " 'arguments': { "
> >> @@ -872,7 +872,7 @@ int test_precopy_common(MigrateCommon *args)
> >> }
> >>
> >> if (args->live) {
> >> - migrate_ensure_non_converge(from);
> >> + migrate_ensure_non_converge(from, args->start.config);
> >> migrate_prepare_for_dirty_mem(from);
> >> } else {
> >> /*
> >> @@ -884,7 +884,7 @@ int test_precopy_common(MigrateCommon *args)
> >> if (args->result == MIG_TEST_SUCCEED) {
> >> qtest_qmp_assert_success(from, "{ 'execute' : 'stop'}");
> >> wait_for_stop(from, &src_state);
> >> - migrate_ensure_converge(from);
> >> + migrate_ongoing_ensure_converge(from);
> >> }
> >> }
> >>
> >> @@ -942,7 +942,7 @@ int test_precopy_common(MigrateCommon *args)
> >> }
> >> migrate_wait_for_dirty_mem(from, to);
> >>
> >> - migrate_ensure_converge(from);
> >> + migrate_ongoing_ensure_converge(from);
> >>
> >> /*
> >> * We do this first, as it has a timeout to stop us
> >> @@ -1047,7 +1047,7 @@ void test_file_common(MigrateCommon *args, bool
> >> stop_src)
> >> data_hook = args->start_hook(from, to);
> >> }
> >>
> >> - migrate_ensure_converge(from);
> >> + migrate_ensure_converge(from, args->start.config);
> >> wait_for_serial("src_serial");
> >>
> >> if (stop_src) {
> >> diff --git a/tests/qtest/migration/migration-qmp.c
> >> b/tests/qtest/migration/migration-qmp.c
> >> index 5c46ceb3e6..7fe47a5793 100644
> >> --- a/tests/qtest/migration/migration-qmp.c
> >> +++ b/tests/qtest/migration/migration-qmp.c
> >> @@ -499,20 +499,48 @@ void migrate_set_parameter_bool(QTestState *who,
> >> const char *parameter,
> >> migrate_check_parameter_bool(who, parameter, value);
> >> }
> >>
> >> -void migrate_ensure_non_converge(QTestState *who)
> >> +void migrate_ongoing_ensure_non_converge(QTestState *who)
> >> {
> >> /* Can't converge with 1ms downtime + 3 mbs bandwidth limit */
> >> migrate_set_parameter_int(who, "max-bandwidth", 3 * 1000 * 1000);
> >> migrate_set_parameter_int(who, "downtime-limit", 1);
> >> }
> >>
> >> -void migrate_ensure_converge(QTestState *who)
> >> +void migrate_ongoing_ensure_converge(QTestState *who)
> >> {
> >> /* Should converge with 30s downtime + 1 gbs bandwidth limit */
> >> migrate_set_parameter_int(who, "max-bandwidth", 1 * 1000 * 1000 *
> >> 1000);
> >> migrate_set_parameter_int(who, "downtime-limit", 30 * 1000);
> >> }
> >>
> >> +void migrate_ensure_non_converge(QTestState *who, QDict *config)
> >> +{
> >> + config = config_load(config);
> >> + if (config) {
> >> + /* Can't converge with 1ms downtime + 3 mbs bandwidth limit */
> >> + qdict_put_int(config, "max-bandwidth", 3 * 1000 * 1000);
> >> + qdict_put_int(config, "downtime-limit", 1);
> >> + } else {
> >> + assert(who);
> >> + migrate_ongoing_ensure_non_converge(who);
> >> + }
> >> + config_put(config);
> >> +}
> >> +
> >> +void migrate_ensure_converge(QTestState *who, QDict *config)
> >> +{
> >> + config = config_load(config);
> >> + /* Should converge with 30s downtime + 1 gbs bandwidth limit */
> >> + if (config) {
> >> + qdict_put_int(config, "max-bandwidth", 1 * 1000 * 1000 * 1000);
> >> + qdict_put_int(config, "downtime-limit", 30 * 1000);
> >> + } else {
> >> + assert(who);
> >> + migrate_ongoing_ensure_converge(who);
> >> + }
> >> + config_put(config);
> >> +}
> >
> > It's slightly an overkill to me to have these converge helpers to provide
> > two versions. Also a bit confusing on when should we use which.
> >
> > After all, parameters touched on convergence must be able to be dynamically
> > set..
> >
> > Can we always stick with the QMP set-parameters for all these?
> >
>
> Well, QEMU ignores anything set with migrate-set-parameters once it sees
> the config, so we'd need to change that in the code.
>
> Thinking about the design of "config", I think the point was to never
> configure a migration via migrate-set-parameters. Always pass the config
> to the migration commands.
>
> These options are special in that they make sense both before and after
> starting the migration, so it's indeed confusing. I don't know what the
> best approach is.
Hmm, now I start to question whether this is a good idea. That's about
this patch of the series:
migration: Allow migrate commands to provide the migration config
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.
Logically speaking, if mgmt worries about a stale parameter leftover, the
mgmt should always overwrite it in the config of this QMP migrate command..
Now I don't see a real benefit that we need to ignore global setups.
A mgmt should simply query all parameters when QEMU just started up, then
keep it, then whatever user changes should be applied on top, Then when
any QMP migrate happens, it should always set all parameters.. no matter
what is the global.
The problem is exactly here, that when some parameters can be dynamically
changed like max-bw, if it was set and throttled 10Gbps dynamically,
migration failed, someone re-started the migration expecting the 10Gbps was
still applied when QMP migrate didn't set max-bw this time, but it didn't
work like that.
Do you think we should make "config" of migrate / migrate_incoming taking
global setting as base, rather than initial_params? I hope we don't
introduce something for nobody at last, but only to make our lives slightly
harder. :(
--
Peter Xu
