Peter Xu <[email protected]> writes: > 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. >
We can decide that QEMU will not force the mgmt app to do that work and will provide an API that doesn't require setting all parameters. I don't see an argument here. > 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. > We need to think about what the QMP API exposes. If we expose an API that says: QMP_MIGRATE might use a value that was set using MIGRATE-SET-PARAMETERS 6 months ago because QEMU uses global state, that's an API usability issue. If we expose an API that says: QMP_MIGRATE runs the migration with whatever arguments were passed to it via CONFIG, that sounds like something sensible. I don't think the API consumers would be surprised if we allow MIGRATE-SET-PARAMETERS to change runtime values for a migration and on the next migration that value is no longer the same. > 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. :( One calls a function and it uses the arguments passed to it, that's it. New migration, new arguments. As you said, mgmt could just hold the dynamic parameter that their user changed and pass that along with the config for the new migration. But I don't think we fleshed out the usage regarding the dynamic parameters yet. There might be other issues that I'm overlooking. Maybe we'd need a whole new command with slightly different semantics from migrate-set-parameters that adjusts the dynamic options while migration is running, I'm not sure. --- side note: If you think the whole endevour of passing a config to qmp_migrate is a bad idea, please speak up. If mgmt code will require the amount of churn I had to do for our test suite, maybe it's not worth the effort after all. My opinion is that, in general, the "config" changes are going in a good direction. I worry slightly about what the cost would be for the users to adhere to it. Migration has been quirky for a looong time, you make it less quirky people will find it strange =)
