[libvirt PATCH 0/3] qemu: Properly reset migration parameters on reconnect
https://bugzilla.redhat.com/show_bug.cgi?id=2107892 Jiri Denemark (3): qemu_migration: Store original migration params in status XML qemu_migration_params: Refactor qemuMigrationParamsApply qemu_migration_params: Refactor qemuMigrationParamsReset src/qemu/qemu_migration.c | 6 +++ src/qemu/qemu_migration_params.c | 69 +++++++++++++++++++++++--------- 2 files changed, 55 insertions(+), 20 deletions(-) -- 2.35.1
We keep original values of migration parameters so that we can restore them at the end of migration to make sure later migration does not use some random values. However, this does not really work when libvirt daemon is restarted on the source host because we failed to explicitly save the status XML after getting the migration parameters from QEMU. Actually it might work if the status XML is written later for some other reason such as domain state change, but that's not how it should work. https://bugzilla.redhat.com/show_bug.cgi?id=2107892 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 20dc91f1ce..769b818f66 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3245,6 +3245,9 @@ qemuMigrationDstPrepareActive(virQEMUDriver *driver, migParams, mig->caps->automatic) < 0) goto error; + /* Save original migration parameters */ + qemuDomainSaveStatus(vm); + /* Migrations using TLS need to add the "tls-creds-x509" object and * set the migration TLS parameters */ if (flags & VIR_MIGRATE_TLS) { @@ -4821,6 +4824,9 @@ qemuMigrationSrcRun(virQEMUDriver *driver, migParams, mig->caps->automatic) < 0) goto error; + /* Save original migration parameters */ + qemuDomainSaveStatus(vm); + if (flags & VIR_MIGRATE_TLS) { const char *hostname = NULL; -- 2.35.1
qemuMigrationParamsApply restricts when capabilities can be set, but this is not useful in all cases. Let's create new helpers for setting migration capabilities and parameters which can be reused in more places without the restriction. https://bugzilla.redhat.com/show_bug.cgi?id=2107892 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration_params.c | 55 +++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 1fd0c55905..dcfb30b56f 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -864,6 +864,43 @@ qemuMigrationCapsToJSON(virBitmap *caps, } +static int +qemuMigrationParamsApplyCaps(virDomainObj *vm, + virBitmap *states) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virJSONValue) json = NULL; + + if (!(json = qemuMigrationCapsToJSON(priv->migrationCaps, states))) + return -1; + + if (virJSONValueArraySize(json) > 0 && + qemuMonitorSetMigrationCapabilities(priv->mon, &json) < 0) + return -1; + + return 0; +} + + +static int +qemuMigrationParamsApplyValues(virDomainObj *vm, + qemuMigrationParams *params, + bool postcopyResume) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virJSONValue) json = NULL; + + if (!(json = qemuMigrationParamsToJSON(params, postcopyResume))) + return -1; + + if (virJSONValueObjectKeysNumber(json) > 0 && + qemuMonitorSetMigrationParams(priv->mon, &json) < 0) + return -1; + + return 0; +} + + /** * qemuMigrationParamsApply * @driver: qemu driver @@ -885,9 +922,6 @@ qemuMigrationParamsApply(virQEMUDriver *driver, qemuMigrationParams *migParams, unsigned long apiFlags) { - qemuDomainObjPrivate *priv = vm->privateData; - g_autoptr(virJSONValue) params = NULL; - g_autoptr(virJSONValue) caps = NULL; bool postcopyResume = !!(apiFlags & VIR_MIGRATE_POSTCOPY_RESUME); int ret = -1; @@ -905,21 +939,12 @@ qemuMigrationParamsApply(virQEMUDriver *driver, "a migration job")); goto cleanup; } - } else { - if (!(caps = qemuMigrationCapsToJSON(priv->migrationCaps, migParams->caps))) - goto cleanup; - - if (virJSONValueArraySize(caps) > 0 && - qemuMonitorSetMigrationCapabilities(priv->mon, &caps) < 0) - goto cleanup; + } else if (qemuMigrationParamsApplyCaps(vm, migParams->caps) < 0) { + goto cleanup; } } - if (!(params = qemuMigrationParamsToJSON(migParams, postcopyResume))) - goto cleanup; - - if (virJSONValueObjectKeysNumber(params) > 0 && - qemuMonitorSetMigrationParams(priv->mon, ¶ms) < 0) + if (qemuMigrationParamsApplyValues(vm, migParams, postcopyResume) < 0) goto cleanup; ret = 0; -- 2.35.1
Because qemuMigrationParamsReset used to call qemuMigrationParamsApply for resetting migration capabilities and parameters, it did not work well since commit v5.1.0-83-ga1dec315c9 which only allowed capabilities to be set from an async job. However, when reconnecting to running domains after daemon restart we do not have an async job. Thus the capabilities were not properly reset in case the daemon was restarted during an ongoing migration. We need to avoid calling qemuMigrationParamsApply to make sure both parameters and capabilities can be reset by a normal job. https://bugzilla.redhat.com/show_bug.cgi?id=2107892 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration_params.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index dcfb30b56f..be7966a18a 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -1290,6 +1290,7 @@ qemuMigrationParamsReset(virQEMUDriver *driver, unsigned long apiFlags) { virErrorPtr err; + g_autoptr(virBitmap) clearCaps = NULL; virErrorPreserveLast(&err); @@ -1299,13 +1300,16 @@ qemuMigrationParamsReset(virQEMUDriver *driver, if (!virDomainObjIsActive(vm) || !origParams) goto cleanup; - /* Do not pass apiFlags to qemuMigrationParamsApply here to make sure all - * parameters and capabilities are reset. */ - if (qemuMigrationParamsApply(driver, vm, asyncJob, origParams, 0) < 0) + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; - qemuMigrationParamsResetTLS(driver, vm, asyncJob, origParams, apiFlags); - /* We don't reset 'block-bitmap-mapping' as it can't be unset */ + clearCaps = virBitmapNew(0); + + if (qemuMigrationParamsApplyCaps(vm, clearCaps) == 0 && + qemuMigrationParamsApplyValues(vm, origParams, false) == 0) + qemuMigrationParamsResetTLS(driver, vm, asyncJob, origParams, apiFlags); + + qemuDomainObjExitMonitor(vm); cleanup: virErrorRestore(&err); -- 2.35.1
On 7/22/22 17:08, Jiri Denemark wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=2107892
Jiri Denemark (3): qemu_migration: Store original migration params in status XML qemu_migration_params: Refactor qemuMigrationParamsApply qemu_migration_params: Refactor qemuMigrationParamsReset
src/qemu/qemu_migration.c | 6 +++ src/qemu/qemu_migration_params.c | 69 +++++++++++++++++++++++--------- 2 files changed, 55 insertions(+), 20 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Jiri Denemark -
Michal Prívozník