On Thu, Sep 01, 2022 at 16:51:34 +0200, Peter Krempa wrote:
On Thu, Sep 01, 2022 at 14:47:41 +0200, Jiri Denemark wrote:
We have always considered "migrate_cancel" QMP command to return after successfully cancelling the migration. But this is no longer true (to be honest I'm not sure it ever was) as it just changes the migration state to "cancelling". In most cases the migration is canceled pretty quickly and we don't really notice anything, but sometimes it takes so long we even get to clearing migration capabilities before the migration is actually canceled, which fails as capabilities can only be changed when no migration is running. So to avoid this issue, we can wait for the migration to be really canceled after sending migrate_cancel. The only place where we don't need synchronous behavior is when we're cancelling migration on user's request while it is actively watched by another thread.
https://bugzilla.redhat.com/show_bug.cgi?id=2114866
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_migration.c | 45 +++++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 3 ++- src/qemu/qemu_process.c | 2 +- 4 files changed, 45 insertions(+), 7 deletions(-)
[...]
int qemuMigrationSrcCancel(virDomainObj *vm, - virDomainAsyncJob asyncJob) + virDomainAsyncJob asyncJob, + bool wait) { qemuDomainObjPrivate *priv = vm->privateData;
@@ -4625,6 +4653,15 @@ qemuMigrationSrcCancel(virDomainObj *vm, qemuMonitorMigrateCancel(priv->mon); qemuDomainObjExitMonitor(vm);
+ if (virDomainObjIsActive(vm) && wait) {
Is the call to virDomainObjIsActive() necessary here? IIUC the domain shutdown code is always executed in a way to make sure that waiting threads are always woken.
+ VIR_DEBUG("Waiting for migration to be canceled"); + + while (!qemuMigrationSrcIsCanceled(vm)) { + if (qemuDomainObjWait(vm) < 0)
So here if the VM would crash before we wait we'd report success and if it crashed during our wait we'll report failure, which seems weird too.
Oh right, qemuDomainObjWait already checks for virDomainObjIsActive so we don't have to do it explicitly here. Just if (wait) { ... } is enough. Jirka