On Tue, May 10, 2022 at 17:20:24 +0200, Jiri Denemark wrote: [...] I'm not exactly a fan of such loose correlation of data. Asking you to return a list of stucts would probably cause too much conflicts though, so as an alternative please document the relation between the 'capabilities' array and the positions in the 'state' bitmap in the comment for this function (that you'll need to add). Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

When connecting to a QEMU monitor, we always try to enable migration events, but this is an invalid operation during migration. Thus reconnecting to a domain with active migration would fail. Let's check the state of migration events capability and only try to enable it when it is disabled. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration_params.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 34416f89be..ed2d9ac103 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -1391,13 +1391,16 @@ qemuMigrationCapsCheck(virQEMUDriver *driver, g_autoptr(virBitmap) migEvent = NULL; g_autoptr(virJSONValue) json = NULL; g_auto(GStrv) caps = NULL; + g_autoptr(virBitmap) capState = NULL; + g_autoptr(virBitmap) enabled = virBitmapNew(QEMU_MIGRATION_CAP_LAST); char **capStr; + size_t i; int rc; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; - rc = qemuMonitorGetMigrationCapabilities(priv->mon, &caps, NULL); + rc = qemuMonitorGetMigrationCapabilities(priv->mon, &caps, &capState); qemuDomainObjExitMonitor(vm); if (rc < 0) @@ -1408,7 +1411,7 @@ qemuMigrationCapsCheck(virQEMUDriver *driver, priv->migrationCaps = virBitmapNew(QEMU_MIGRATION_CAP_LAST); - for (capStr = caps; *capStr; capStr++) { + for (i = 0, capStr = caps; *capStr; capStr++, i++) { int cap = qemuMigrationCapabilityTypeFromString(*capStr); if (cap < 0) { @@ -1416,10 +1419,20 @@ qemuMigrationCapsCheck(virQEMUDriver *driver, } else { ignore_value(virBitmapSetBit(priv->migrationCaps, cap)); VIR_DEBUG("Found migration capability: '%s'", *capStr); + + if (virBitmapIsBitSet(capState, i)) + ignore_value(virBitmapSetBit(enabled, cap)); } } - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) { + if (virBitmapIsBitSet(enabled, QEMU_MIGRATION_CAP_EVENTS)) { + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) { + VIR_DEBUG("Migration events already enabled"); + } else { + VIR_DEBUG("Migration events enabled; setting capability"); + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT); + } + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) { migEvent = virBitmapNew(QEMU_MIGRATION_CAP_LAST); ignore_value(virBitmapSetBit(migEvent, QEMU_MIGRATION_CAP_EVENTS)); -- 2.35.1

On Wed, May 11, 2022 at 10:37:25 +0200, Peter Krempa wrote: Hmm, it looks like migration events are indeed supported by all QEMU releases we support so we could drop some parts of this and most likely even the previous patch :-) Jirka

This new "post-copy failed" reason for the running state will be used on the destination host when post-copy migration fails while the domain is already running there. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- examples/c/misc/event-test.c | 3 +++ include/libvirt/libvirt-domain.h | 2 ++ src/conf/domain_conf.c | 1 + src/libvirt-domain.c | 15 ++++++++++----- src/qemu/qemu_domain.c | 3 +++ tools/virsh-domain-event.c | 3 ++- tools/virsh-domain-monitor.c | 1 + 7 files changed, 22 insertions(+), 6 deletions(-) diff --git a/examples/c/misc/event-test.c b/examples/c/misc/event-test.c index 1165469a65..64652b0153 100644 --- a/examples/c/misc/event-test.c +++ b/examples/c/misc/event-test.c @@ -196,6 +196,9 @@ eventDetailToString(int event, case VIR_DOMAIN_EVENT_RESUMED_POSTCOPY: return "Post-copy"; + case VIR_DOMAIN_EVENT_RESUMED_POSTCOPY_FAILED: + return "Post-copy Error"; + case VIR_DOMAIN_EVENT_RESUMED_LAST: break; } diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index cf9d9efd51..34b8adc2bf 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -108,6 +108,7 @@ typedef enum { wakeup event (Since: 0.9.11) */ VIR_DOMAIN_RUNNING_CRASHED = 9, /* resumed from crashed (Since: 1.1.1) */ VIR_DOMAIN_RUNNING_POSTCOPY = 10, /* running in post-copy migration mode (Since: 1.3.3) */ + VIR_DOMAIN_RUNNING_POSTCOPY_FAILED = 11, /* running in failed post-copy migration (Since: 8.4.0) */ # ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_RUNNING_LAST /* (Since: 0.9.10) */ @@ -3801,6 +3802,7 @@ typedef enum { VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT = 2, /* Resumed from snapshot (Since: 0.9.5) */ VIR_DOMAIN_EVENT_RESUMED_POSTCOPY = 3, /* Resumed, but migration is still running in post-copy mode (Since: 1.3.3) */ + VIR_DOMAIN_EVENT_RESUMED_POSTCOPY_FAILED = 4, /* Running, but migration failed in post-copy (Since: 8.4.0) */ # ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_RESUMED_LAST /* (Since: 0.9.10) */ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd2884088c..f8c6b78c92 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1102,6 +1102,7 @@ VIR_ENUM_IMPL(virDomainRunningReason, "wakeup", "crashed", "post-copy", + "post-copy failed", ); VIR_ENUM_IMPL(virDomainBlockedReason, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index d1d62daa71..2e39687e27 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -9753,10 +9753,14 @@ virDomainMigrateGetMaxSpeed(virDomainPtr domain, * at most once no matter how fast it changes. On the other hand once the * guest is running on the destination host, the migration can no longer be * rolled back because none of the hosts has complete state. If this happens, - * libvirt will leave the domain paused on both hosts with - * VIR_DOMAIN_PAUSED_POSTCOPY_FAILED reason. It's up to the upper layer to - * decide what to do in such case. Because of this, libvirt will refuse to - * cancel post-copy migration via virDomainAbortJob. + * libvirt will leave the domain paused on the source host with + * VIR_DOMAIN_PAUSED_POSTCOPY_FAILED reason. The domain on the destination host + * will remain running, but the reason will change to + * VIR_DOMAIN_RUNNING_POSTCOPY_FAILED. Even though the virtual CPUs remain + * running, individual tasks in the guest may be blocked waiting for a page that + * has not been migrated yet. It's up to the upper layer to decide what to do + * in such case. Because of this, libvirt will refuse to cancel post-copy + * migration via virDomainAbortJob. * * The following domain life cycle events are emitted during post-copy * migration: @@ -9770,7 +9774,8 @@ virDomainMigrateGetMaxSpeed(virDomainPtr domain, * VIR_DOMAIN_EVENT_RESUMED_MIGRATED (on the destination), * VIR_DOMAIN_EVENT_STOPPED_MIGRATED (on the source) -- migration finished * successfully and the destination host holds a complete guest state. - * VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY_FAILED (on the destination) -- emitted + * VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY_FAILED (on the source), + * VIR_DOMAIN_EVENT_RESUMED_POSTCOPY_FAILED (on the destination) -- emitted * when migration fails in post-copy mode and it's unclear whether any * of the hosts has a complete guest state. * diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7974cdb00b..51236b19f2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11105,6 +11105,9 @@ qemuDomainRunningReasonToResumeEvent(virDomainRunningReason reason) case VIR_DOMAIN_RUNNING_POSTCOPY: return VIR_DOMAIN_EVENT_RESUMED_POSTCOPY; + case VIR_DOMAIN_RUNNING_POSTCOPY_FAILED: + return VIR_DOMAIN_EVENT_RESUMED_POSTCOPY_FAILED; + case VIR_DOMAIN_RUNNING_UNKNOWN: case VIR_DOMAIN_RUNNING_SAVE_CANCELED: case VIR_DOMAIN_RUNNING_BOOTED: diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index 6dbb64a655..2d6db5f881 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -93,7 +93,8 @@ VIR_ENUM_IMPL(virshDomainEventResumed, N_("Unpaused"), N_("Migrated"), N_("Snapshot"), - N_("Post-copy")); + N_("Post-copy"), + N_("Post-copy Error")); VIR_ENUM_DECL(virshDomainEventStopped); VIR_ENUM_IMPL(virshDomainEventStopped, diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 246e8a16c0..dc5fe13e49 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -166,6 +166,7 @@ VIR_ENUM_IMPL(virshDomainRunningReason, N_("event wakeup"), N_("crashed"), N_("post-copy"), + N_("post-copy failed"), ); VIR_ENUM_DECL(virshDomainBlockedReason); -- 2.35.1

There's no need to artificially pause a domain when post-copy fails. The virtual CPUs may continue running, only the guest tasks that decide to read a page which has not been migrated yet will get blocked. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 37 +++++++++++++++++++++++++++++++++---- src/qemu/qemu_migration.h | 6 ++++-- src/qemu/qemu_process.c | 8 ++++---- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b735bdb391..a5c7a27124 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1577,14 +1577,19 @@ qemuMigrationSrcIsSafe(virDomainDef *def, void -qemuMigrationAnyPostcopyFailed(virQEMUDriver *driver, - virDomainObj *vm) +qemuMigrationSrcPostcopyFailed(virDomainObj *vm) { + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; virDomainState state; int reason; state = virDomainObjGetState(vm, &reason); + VIR_DEBUG("%s/%s", + virDomainStateTypeToString(state), + virDomainStateReasonToString(state, reason)); + if (state != VIR_DOMAIN_PAUSED && state != VIR_DOMAIN_RUNNING) return; @@ -1608,6 +1613,30 @@ qemuMigrationAnyPostcopyFailed(virQEMUDriver *driver, } +void +qemuMigrationDstPostcopyFailed(virDomainObj *vm) +{ + virDomainState state; + int reason; + + state = virDomainObjGetState(vm, &reason); + + VIR_DEBUG("%s/%s", + virDomainStateTypeToString(state), + virDomainStateReasonToString(state, reason)); + + if (state != VIR_DOMAIN_RUNNING || + reason == VIR_DOMAIN_RUNNING_POSTCOPY_FAILED) + return; + + VIR_WARN("Incoming migration of domain %s failed during post-copy; " + "leaving the domain running in a degraded mode", vm->def->name); + + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, + VIR_DOMAIN_RUNNING_POSTCOPY_FAILED); +} + + static int qemuMigrationSrcWaitForSpice(virDomainObj *vm) { @@ -3470,7 +3499,7 @@ qemuMigrationSrcConfirmPhase(virQEMUDriver *driver, if (virDomainObjGetState(vm, &reason) == VIR_DOMAIN_PAUSED && reason == VIR_DOMAIN_PAUSED_POSTCOPY) - qemuMigrationAnyPostcopyFailed(driver, vm); + qemuMigrationSrcPostcopyFailed(vm); else qemuMigrationSrcRestoreDomainState(driver, vm); @@ -5847,7 +5876,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, VIR_DOMAIN_EVENT_STOPPED_FAILED); virObjectEventStateQueue(driver->domainEventState, event); } else { - qemuMigrationAnyPostcopyFailed(driver, vm); + qemuMigrationDstPostcopyFailed(vm); } } diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index a8afa66119..c4e4228282 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -251,8 +251,10 @@ qemuMigrationDstRun(virQEMUDriver *driver, virDomainAsyncJob asyncJob); void -qemuMigrationAnyPostcopyFailed(virQEMUDriver *driver, - virDomainObj *vm); +qemuMigrationSrcPostcopyFailed(virDomainObj *vm); + +void +qemuMigrationDstPostcopyFailed(virDomainObj *vm); int qemuMigrationSrcFetchMirrorStats(virQEMUDriver *driver, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1925559fad..a3192a7196 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3482,7 +3482,7 @@ qemuProcessRecoverMigrationIn(virQEMUDriver *driver, * confirm success or failure yet; killing it seems safest unless * we already started guest CPUs or we were in post-copy mode */ if (postcopy) { - qemuMigrationAnyPostcopyFailed(driver, vm); + qemuMigrationDstPostcopyFailed(vm); } else if (state != VIR_DOMAIN_RUNNING) { VIR_DEBUG("Killing migrated domain %s", vm->def->name); return -1; @@ -3533,7 +3533,7 @@ qemuProcessRecoverMigrationOut(virQEMUDriver *driver, * post-copy mode */ if (postcopy) { - qemuMigrationAnyPostcopyFailed(driver, vm); + qemuMigrationSrcPostcopyFailed(vm); } else { VIR_DEBUG("Cancelling unfinished migration of domain %s", vm->def->name); @@ -3551,7 +3551,7 @@ qemuProcessRecoverMigrationOut(virQEMUDriver *driver, * post-copy mode we can use PAUSED_POSTCOPY_FAILED state for this */ if (postcopy) - qemuMigrationAnyPostcopyFailed(driver, vm); + qemuMigrationSrcPostcopyFailed(vm); break; case QEMU_MIGRATION_PHASE_CONFIRM3_CANCELLED: @@ -3560,7 +3560,7 @@ qemuProcessRecoverMigrationOut(virQEMUDriver *driver, * as broken in that case */ if (postcopy) { - qemuMigrationAnyPostcopyFailed(driver, vm); + qemuMigrationSrcPostcopyFailed(vm); } else { VIR_DEBUG("Resuming domain %s after failed migration", vm->def->name); -- 2.35.1

On Tue, May 10, 2022 at 05:20:34PM +0200, Jiri Denemark wrote: s/priv->job./job->/

On Tue, May 10, 2022 at 17:20:35 +0200, Jiri Denemark wrote: This logic change is a bit obscure and IMO would benefit from a comment stating that we want to continue all post-copy migration jobs and all successful other migrations. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

When migration fails after it already switched to post-copy phase on the source, but early enough that we haven't called Finish on the destination yet, we know the vCPUs were not started on the destination and the source host still has a complete state of the domain. Thus we can just ignore the fact post-copy phase started and normally abort the migration and resume vCPUs on the source. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 9 +++++++++ src/qemu/qemu_process.c | 20 ++++++++------------ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 532a9300b6..e892a09885 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4442,6 +4442,15 @@ qemuMigrationSrcRun(virQEMUDriver *driver, virErrorPreserveLast(&orig_err); if (virDomainObjIsActive(vm)) { + int reason; + virDomainState state = virDomainObjGetState(vm, &reason); + + if (state == VIR_DOMAIN_PAUSED && reason == VIR_DOMAIN_PAUSED_POSTCOPY) { + VIR_DEBUG("Aborting failed post-copy migration as the destination " + "is not running yet"); + virDomainObjSetState(vm, state, VIR_DOMAIN_PAUSED_MIGRATION); + } + if (cancel && priv->job.current->status != VIR_DOMAIN_JOB_STATUS_HYPERVISOR_COMPLETED && qemuDomainObjEnterMonitorAsync(driver, vm, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 590e989126..e83668e088 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3554,20 +3554,16 @@ qemuProcessRecoverMigrationOut(virQEMUDriver *driver, case QEMU_MIGRATION_PHASE_PERFORM2: case QEMU_MIGRATION_PHASE_PERFORM3: /* migration is still in progress, let's cancel it and resume the - * domain; however we can only do that before migration enters - * post-copy mode + * domain; we can do so even in post-copy phase as the domain was not + * resumed on the destination host yet */ - if (postcopy) { - qemuMigrationSrcPostcopyFailed(vm); - } else { - VIR_DEBUG("Cancelling unfinished migration of domain %s", - vm->def->name); - if (qemuMigrationSrcCancel(driver, vm) < 0) { - VIR_WARN("Could not cancel ongoing migration of domain %s", - vm->def->name); - } - resume = true; + VIR_DEBUG("Cancelling unfinished migration of domain %s", + vm->def->name); + if (qemuMigrationSrcCancel(driver, vm) < 0) { + VIR_WARN("Could not cancel ongoing migration of domain %s", + vm->def->name); } + resume = true; break; case QEMU_MIGRATION_PHASE_PERFORM3_DONE: -- 2.35.1

Since we keep the migration job active when post-copy migration fails, we need to restore it when reconnecting to running domains. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_process.c | 128 ++++++++++++++++++++++++++++++---------- 1 file changed, 96 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e83668e088..3d73c716f1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3456,20 +3456,48 @@ qemuProcessCleanupMigrationJob(virQEMUDriver *driver, } +static void +qemuProcessRestoreMigrationJob(virDomainObj *vm, + qemuDomainJobObj *job) +{ + qemuDomainObjPrivate *priv = vm->privateData; + qemuDomainJobPrivate *jobPriv = job->privateData; + virDomainJobOperation op; + unsigned long long allowedJobs; + + if (job->asyncJob == VIR_ASYNC_JOB_MIGRATION_IN) { + op = VIR_DOMAIN_JOB_OPERATION_MIGRATION_IN; + allowedJobs = VIR_JOB_NONE; + } else { + op = VIR_DOMAIN_JOB_OPERATION_MIGRATION_OUT; + allowedJobs = VIR_JOB_DEFAULT_MASK | JOB_MASK(VIR_JOB_MIGRATION_OP); + } + + qemuDomainObjRestoreAsyncJob(vm, job->asyncJob, job->phase, op, + QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION, + VIR_DOMAIN_JOB_STATUS_PAUSED, + allowedJobs); + + job->privateData = g_steal_pointer(&priv->job.privateData); + priv->job.privateData = jobPriv; + priv->job.apiFlags = job->apiFlags; + + qemuDomainCleanupAdd(vm, qemuProcessCleanupMigrationJob); +} + + +/* + * Returns + * -1 on error, the domain will be killed, + * 0 the domain should remain running with the migration job discarded, + * 1 the daemon was restarted during post-copy phase + */ static int qemuProcessRecoverMigrationIn(virQEMUDriver *driver, virDomainObj *vm, - const qemuDomainJobObj *job, - virDomainState state, - int reason) + qemuDomainJobObj *job, + virDomainState state) { - - qemuDomainJobPrivate *jobPriv = job->privateData; - bool postcopy = (state == VIR_DOMAIN_PAUSED && - reason == VIR_DOMAIN_PAUSED_POSTCOPY_FAILED) || - (state == VIR_DOMAIN_RUNNING && - reason == VIR_DOMAIN_RUNNING_POSTCOPY); - VIR_DEBUG("Active incoming migration in phase %s", qemuMigrationJobPhaseTypeToString(job->phase)); @@ -3506,32 +3534,37 @@ qemuProcessRecoverMigrationIn(virQEMUDriver *driver, /* migration finished, we started resuming the domain but didn't * confirm success or failure yet; killing it seems safest unless * we already started guest CPUs or we were in post-copy mode */ - if (postcopy) { + if (virDomainObjIsPostcopy(vm, VIR_DOMAIN_JOB_OPERATION_MIGRATION_IN)) { qemuMigrationDstPostcopyFailed(vm); - } else if (state != VIR_DOMAIN_RUNNING) { + return 1; + } + + if (state != VIR_DOMAIN_RUNNING) { VIR_DEBUG("Killing migrated domain %s", vm->def->name); return -1; } break; } - qemuMigrationParamsReset(driver, vm, VIR_ASYNC_JOB_NONE, - jobPriv->migParams, job->apiFlags); return 0; } + +/* + * Returns + * -1 on error, the domain will be killed, + * 0 the domain should remain running with the migration job discarded, + * 1 the daemon was restarted during post-copy phase + */ static int qemuProcessRecoverMigrationOut(virQEMUDriver *driver, virDomainObj *vm, - const qemuDomainJobObj *job, + qemuDomainJobObj *job, virDomainState state, int reason, unsigned int *stopFlags) { - qemuDomainJobPrivate *jobPriv = job->privateData; - bool postcopy = state == VIR_DOMAIN_PAUSED && - (reason == VIR_DOMAIN_PAUSED_POSTCOPY || - reason == VIR_DOMAIN_PAUSED_POSTCOPY_FAILED); + bool postcopy = virDomainObjIsPostcopy(vm, VIR_DOMAIN_JOB_OPERATION_MIGRATION_OUT); bool resume = false; VIR_DEBUG("Active outgoing migration in phase %s", @@ -3571,8 +3604,10 @@ qemuProcessRecoverMigrationOut(virQEMUDriver *driver, * of Finish3 step; third party needs to check what to do next; in * post-copy mode we can use PAUSED_POSTCOPY_FAILED state for this */ - if (postcopy) + if (postcopy) { qemuMigrationSrcPostcopyFailed(vm); + return 1; + } break; case QEMU_MIGRATION_PHASE_CONFIRM3_CANCELLED: @@ -3582,11 +3617,12 @@ qemuProcessRecoverMigrationOut(virQEMUDriver *driver, */ if (postcopy) { qemuMigrationSrcPostcopyFailed(vm); - } else { - VIR_DEBUG("Resuming domain %s after failed migration", - vm->def->name); - resume = true; + return 1; } + + VIR_DEBUG("Resuming domain %s after failed migration", + vm->def->name); + resume = true; break; case QEMU_MIGRATION_PHASE_CONFIRM3: @@ -3610,15 +3646,49 @@ qemuProcessRecoverMigrationOut(virQEMUDriver *driver, } } + return 0; +} + + +static int +qemuProcessRecoverMigration(virQEMUDriver *driver, + virDomainObj *vm, + qemuDomainJobObj *job, + unsigned int *stopFlags) +{ + qemuDomainJobPrivate *jobPriv = job->privateData; + virDomainState state; + int reason; + int rc; + + state = virDomainObjGetState(vm, &reason); + + if (job->asyncJob == VIR_ASYNC_JOB_MIGRATION_OUT) { + rc = qemuProcessRecoverMigrationOut(driver, vm, job, + state, reason, stopFlags); + } else { + rc = qemuProcessRecoverMigrationIn(driver, vm, job, state); + } + + if (rc < 0) + return -1; + + if (rc > 0) { + qemuProcessRestoreMigrationJob(vm, job); + return 0; + } + qemuMigrationParamsReset(driver, vm, VIR_ASYNC_JOB_NONE, jobPriv->migParams, job->apiFlags); + return 0; } + static int qemuProcessRecoverJob(virQEMUDriver *driver, virDomainObj *vm, - const qemuDomainJobObj *job, + qemuDomainJobObj *job, unsigned int *stopFlags) { qemuDomainObjPrivate *priv = vm->privateData; @@ -3636,14 +3706,8 @@ qemuProcessRecoverJob(virQEMUDriver *driver, switch (job->asyncJob) { case VIR_ASYNC_JOB_MIGRATION_OUT: - if (qemuProcessRecoverMigrationOut(driver, vm, job, - state, reason, stopFlags) < 0) - return -1; - break; - case VIR_ASYNC_JOB_MIGRATION_IN: - if (qemuProcessRecoverMigrationIn(driver, vm, job, - state, reason) < 0) + if (qemuProcessRecoverMigration(driver, vm, job, stopFlags) < 0) return -1; break; -- 2.35.1

Jobs that are supposed to remain active even when libvirt daemon restarts were reported as started at the time the daemon was restarted. This is not very helpful, we should restore the original timestamp. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_domainjob.c | 20 +++++++++++++------ src/qemu/qemu_domainjob.h | 1 + src/qemu/qemu_process.c | 4 +++- .../migration-in-params-in.xml | 2 +- .../migration-out-nbd-bitmaps-in.xml | 2 +- .../migration-out-nbd-out.xml | 2 +- .../migration-out-nbd-tls-out.xml | 2 +- .../migration-out-params-in.xml | 2 +- 8 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index 1f82457bd4..8e8d229afe 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -235,6 +235,7 @@ qemuDomainObjPreserveJob(virDomainObj *obj, job->owner = priv->job.owner; job->asyncJob = priv->job.asyncJob; job->asyncOwner = priv->job.asyncOwner; + job->asyncStarted = priv->job.asyncStarted; job->phase = priv->job.phase; job->privateData = g_steal_pointer(&priv->job.privateData); job->apiFlags = priv->job.apiFlags; @@ -254,6 +255,7 @@ void qemuDomainObjRestoreAsyncJob(virDomainObj *vm, virDomainAsyncJob asyncJob, int phase, + unsigned long long started, virDomainJobOperation operation, qemuDomainJobStatsType statsType, virDomainJobStatus status, @@ -261,18 +263,15 @@ qemuDomainObjRestoreAsyncJob(virDomainObj *vm, { qemuDomainObjPrivate *priv = vm->privateData; qemuDomainJobObj *job = &priv->job; - unsigned long long now; VIR_DEBUG("Restoring %s async job for domain %s", virDomainAsyncJobTypeToString(asyncJob), vm->def->name); - ignore_value(virTimeMillisNow(&now)); - priv->job.jobsQueued++; job->asyncJob = asyncJob; job->phase = phase; job->asyncOwnerAPI = g_strdup(virThreadJobGet()); - job->asyncStarted = now; + job->asyncStarted = started; qemuDomainObjSetAsyncJobMask(vm, allowedJobs); @@ -280,7 +279,7 @@ qemuDomainObjRestoreAsyncJob(virDomainObj *vm, qemuDomainJobSetStatsType(priv->job.current, statsType); job->current->operation = operation; job->current->status = status; - job->current->started = now; + job->current->started = started; } @@ -1250,8 +1249,10 @@ qemuDomainObjPrivateXMLFormatJob(virBuffer *buf, priv->job.phase)); } - if (priv->job.asyncJob != VIR_ASYNC_JOB_NONE) + if (priv->job.asyncJob != VIR_ASYNC_JOB_NONE) { virBufferAsprintf(&attrBuf, " flags='0x%lx'", priv->job.apiFlags); + virBufferAsprintf(&attrBuf, " asyncStarted='%llu'", priv->job.asyncStarted); + } if (priv->job.cb && priv->job.cb->formatJob(&childBuf, &priv->job, vm) < 0) @@ -1307,6 +1308,13 @@ qemuDomainObjPrivateXMLParseJob(virDomainObj *vm, } VIR_FREE(tmp); } + + if (virXPathULongLong("string(@asyncStarted)", ctxt, + &priv->job.asyncStarted) == -2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid async job start")); + return -1; + } } if (virXPathULongHex("string(@flags)", ctxt, &priv->job.apiFlags) == -2) { diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h index 069bb9f8cb..707d4e91ed 100644 --- a/src/qemu/qemu_domainjob.h +++ b/src/qemu/qemu_domainjob.h @@ -164,6 +164,7 @@ void qemuDomainObjRestoreAsyncJob(virDomainObj *vm, virDomainAsyncJob asyncJob, int phase, + unsigned long long started, virDomainJobOperation operation, qemuDomainJobStatsType statsType, virDomainJobStatus status, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3d73c716f1..53221ea9ff 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3473,7 +3473,8 @@ qemuProcessRestoreMigrationJob(virDomainObj *vm, allowedJobs = VIR_JOB_DEFAULT_MASK | JOB_MASK(VIR_JOB_MIGRATION_OP); } - qemuDomainObjRestoreAsyncJob(vm, job->asyncJob, job->phase, op, + qemuDomainObjRestoreAsyncJob(vm, job->asyncJob, job->phase, + job->asyncStarted, op, QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION, VIR_DOMAIN_JOB_STATUS_PAUSED, allowedJobs); @@ -3746,6 +3747,7 @@ qemuProcessRecoverJob(virQEMUDriver *driver, case VIR_ASYNC_JOB_BACKUP: /* Restore the config of the async job which is not persisted */ qemuDomainObjRestoreAsyncJob(vm, VIR_ASYNC_JOB_BACKUP, 0, + job->asyncStarted, VIR_DOMAIN_JOB_OPERATION_BACKUP, QEMU_DOMAIN_JOB_STATS_TYPE_BACKUP, VIR_DOMAIN_JOB_STATUS_ACTIVE, diff --git a/tests/qemustatusxml2xmldata/migration-in-params-in.xml b/tests/qemustatusxml2xmldata/migration-in-params-in.xml index f4bc5753c4..9e9c2deac6 100644 --- a/tests/qemustatusxml2xmldata/migration-in-params-in.xml +++ b/tests/qemustatusxml2xmldata/migration-in-params-in.xml @@ -238,7 +238,7 @@ <flag name='dump-completed'/> <flag name='hda-output'/> </qemuCaps> - <job type='none' async='migration in' phase='prepare' flags='0x900'> + <job type='none' async='migration in' phase='prepare' flags='0x900' asyncStarted='0'> <migParams> <param name='compress-level' value='1'/> <param name='compress-threads' value='8'/> diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-bitmaps-in.xml b/tests/qemustatusxml2xmldata/migration-out-nbd-bitmaps-in.xml index c88996f923..1e14efe79a 100644 --- a/tests/qemustatusxml2xmldata/migration-out-nbd-bitmaps-in.xml +++ b/tests/qemustatusxml2xmldata/migration-out-nbd-bitmaps-in.xml @@ -262,7 +262,7 @@ <flag name='cpu-max'/> <flag name='migration-param.block-bitmap-mapping'/> </qemuCaps> - <job type='none' async='migration out' phase='perform3' flags='0x42'> + <job type='none' async='migration out' phase='perform3' flags='0x42' asyncStarted='0'> <disk dev='hda' migrating='no'/> <disk dev='vda' migrating='yes'> <migrationSource type='network' format='raw'> diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-out.xml b/tests/qemustatusxml2xmldata/migration-out-nbd-out.xml index 039dcacc58..12abd0c81a 100644 --- a/tests/qemustatusxml2xmldata/migration-out-nbd-out.xml +++ b/tests/qemustatusxml2xmldata/migration-out-nbd-out.xml @@ -231,7 +231,7 @@ <flag name='dump-completed'/> <flag name='hda-output'/> </qemuCaps> - <job type='none' async='migration out' phase='perform3' flags='0x0'> + <job type='none' async='migration out' phase='perform3' flags='0x0' asyncStarted='0'> <disk dev='vdb' migrating='yes'/> <disk dev='hda' migrating='no'/> </job> diff --git a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml index 3d1ddd5771..75c656d6c4 100644 --- a/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml +++ b/tests/qemustatusxml2xmldata/migration-out-nbd-tls-out.xml @@ -235,7 +235,7 @@ <flag name='nbd-tls'/> <flag name='blockdev-del'/> </qemuCaps> - <job type='none' async='migration out' phase='perform3' flags='0x0'> + <job type='none' async='migration out' phase='perform3' flags='0x0' asyncStarted='0'> <disk dev='vdb' migrating='yes'> <migrationSource type='network' format='raw'> <source protocol='nbd' name='drive-virtio-disk1' tlsFromConfig='0'> diff --git a/tests/qemustatusxml2xmldata/migration-out-params-in.xml b/tests/qemustatusxml2xmldata/migration-out-params-in.xml index cd9dbccd3a..c23c3c3083 100644 --- a/tests/qemustatusxml2xmldata/migration-out-params-in.xml +++ b/tests/qemustatusxml2xmldata/migration-out-params-in.xml @@ -238,7 +238,7 @@ <flag name='dump-completed'/> <flag name='hda-output'/> </qemuCaps> - <job type='none' async='migration out' phase='perform3' flags='0x802'> + <job type='none' async='migration out' phase='perform3' flags='0x802' asyncStarted='0'> <disk dev='vda' migrating='no'/> <migParams> <param name='compress-level' value='1'/> -- 2.35.1

On Wed, May 11, 2022 at 15:04:53 +0200, Peter Krempa wrote: We can keep this fallback in qemuDomainObjRestoreAsyncJob instead of putting it in the parser to avoid test issues. Jirka

Even though a migration is paused, we still want to see the amount of data transferred so far and that the migration is indeed not progressing any further. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 85ccc446d7..9e6cf26001 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12513,6 +12513,7 @@ qemuDomainGetJobInfoMigrationStats(virQEMUDriver *driver, case VIR_DOMAIN_JOB_STATUS_MIGRATING: case VIR_DOMAIN_JOB_STATUS_HYPERVISOR_COMPLETED: case VIR_DOMAIN_JOB_STATUS_POSTCOPY: + case VIR_DOMAIN_JOB_STATUS_PAUSED: if (events && jobData->status != VIR_DOMAIN_JOB_STATUS_ACTIVE && qemuMigrationAnyFetchStats(driver, vm, VIR_ASYNC_JOB_NONE, @@ -12531,7 +12532,6 @@ qemuDomainGetJobInfoMigrationStats(virQEMUDriver *driver, break; case VIR_DOMAIN_JOB_STATUS_NONE: - case VIR_DOMAIN_JOB_STATUS_PAUSED: case VIR_DOMAIN_JOB_STATUS_COMPLETED: case VIR_DOMAIN_JOB_STATUS_FAILED: case VIR_DOMAIN_JOB_STATUS_CANCELED: -- 2.35.1

When connection breaks during post-copy migration, QEMU enters 'postcopy-paused' state. We need to handle this state and make the situation visible to upper layers. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/hypervisor/domain_job.c | 1 + src/hypervisor/domain_job.h | 1 + src/qemu/qemu_driver.c | 1 + src/qemu/qemu_migration.c | 11 +++++++++++ src/qemu/qemu_monitor.c | 1 + src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 1 + src/qemu/qemu_process.c | 33 ++++++++++++++++++++------------- 8 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/hypervisor/domain_job.c b/src/hypervisor/domain_job.c index ff4e008cb5..49867c3982 100644 --- a/src/hypervisor/domain_job.c +++ b/src/hypervisor/domain_job.c @@ -93,6 +93,7 @@ virDomainJobStatusToType(virDomainJobStatus status) case VIR_DOMAIN_JOB_STATUS_MIGRATING: case VIR_DOMAIN_JOB_STATUS_HYPERVISOR_COMPLETED: case VIR_DOMAIN_JOB_STATUS_POSTCOPY: + case VIR_DOMAIN_JOB_STATUS_POSTCOPY_PAUSED: case VIR_DOMAIN_JOB_STATUS_PAUSED: return VIR_DOMAIN_JOB_UNBOUNDED; diff --git a/src/hypervisor/domain_job.h b/src/hypervisor/domain_job.h index db8b8b1390..fce35ffbf5 100644 --- a/src/hypervisor/domain_job.h +++ b/src/hypervisor/domain_job.h @@ -78,6 +78,7 @@ typedef enum { VIR_DOMAIN_JOB_STATUS_HYPERVISOR_COMPLETED, VIR_DOMAIN_JOB_STATUS_PAUSED, VIR_DOMAIN_JOB_STATUS_POSTCOPY, + VIR_DOMAIN_JOB_STATUS_POSTCOPY_PAUSED, VIR_DOMAIN_JOB_STATUS_COMPLETED, VIR_DOMAIN_JOB_STATUS_FAILED, VIR_DOMAIN_JOB_STATUS_CANCELED, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9e6cf26001..d19115473a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12514,6 +12514,7 @@ qemuDomainGetJobInfoMigrationStats(virQEMUDriver *driver, case VIR_DOMAIN_JOB_STATUS_HYPERVISOR_COMPLETED: case VIR_DOMAIN_JOB_STATUS_POSTCOPY: case VIR_DOMAIN_JOB_STATUS_PAUSED: + case VIR_DOMAIN_JOB_STATUS_POSTCOPY_PAUSED: if (events && jobData->status != VIR_DOMAIN_JOB_STATUS_ACTIVE && qemuMigrationAnyFetchStats(driver, vm, VIR_ASYNC_JOB_NONE, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 72a2846d1f..e5e33556e3 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1757,6 +1757,10 @@ qemuMigrationUpdateJobType(virDomainJobData *jobData) jobData->status = VIR_DOMAIN_JOB_STATUS_POSTCOPY; break; + case QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY_PAUSED: + jobData->status = VIR_DOMAIN_JOB_STATUS_POSTCOPY_PAUSED; + break; + case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED: jobData->status = VIR_DOMAIN_JOB_STATUS_HYPERVISOR_COMPLETED; break; @@ -1883,6 +1887,12 @@ qemuMigrationJobCheckStatus(virQEMUDriver *driver, qemuMigrationJobName(vm), _("canceled by client")); return -1; + case VIR_DOMAIN_JOB_STATUS_POSTCOPY_PAUSED: + virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), + qemuMigrationJobName(vm), + _("post-copy phase failed")); + return -1; + case VIR_DOMAIN_JOB_STATUS_COMPLETED: case VIR_DOMAIN_JOB_STATUS_ACTIVE: case VIR_DOMAIN_JOB_STATUS_MIGRATING: @@ -1985,6 +1995,7 @@ qemuMigrationAnyCompleted(virQEMUDriver *driver, case VIR_DOMAIN_JOB_STATUS_FAILED: case VIR_DOMAIN_JOB_STATUS_CANCELED: + case VIR_DOMAIN_JOB_STATUS_POSTCOPY_PAUSED: /* QEMU aborted the migration. */ return -1; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 98cf1c949e..36cf4e57b5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -149,6 +149,7 @@ VIR_ENUM_IMPL(qemuMonitorMigrationStatus, "inactive", "setup", "active", "pre-switchover", "device", "postcopy-active", + "postcopy-paused", "completed", "failed", "cancelling", "cancelled", "wait-unplug", diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index abc29eaf4c..46cdc04925 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -831,6 +831,7 @@ typedef enum { QEMU_MONITOR_MIGRATION_STATUS_PRE_SWITCHOVER, QEMU_MONITOR_MIGRATION_STATUS_DEVICE, QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY, + QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY_PAUSED, QEMU_MONITOR_MIGRATION_STATUS_COMPLETED, QEMU_MONITOR_MIGRATION_STATUS_ERROR, QEMU_MONITOR_MIGRATION_STATUS_CANCELLING, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 532aad348e..ac3ec42fdd 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3242,6 +3242,7 @@ qemuMonitorJSONGetMigrationStatsReply(virJSONValue *reply, case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE: case QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY: + case QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY_PAUSED: case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED: case QEMU_MONITOR_MIGRATION_STATUS_CANCELLING: case QEMU_MONITOR_MIGRATION_STATUS_PRE_SWITCHOVER: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 53221ea9ff..fc61aa71a0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1568,19 +1568,26 @@ qemuProcessHandleMigrationStatus(qemuMonitor *mon G_GNUC_UNUSED, privJob->stats.mig.status = status; virDomainObjBroadcast(vm); - if (status == QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY && - priv->job.asyncJob == VIR_ASYNC_JOB_MIGRATION_OUT && - virDomainObjGetState(vm, &reason) == VIR_DOMAIN_PAUSED && - reason == VIR_DOMAIN_PAUSED_MIGRATION) { - VIR_DEBUG("Correcting paused state reason for domain %s to %s", - vm->def->name, - virDomainPausedReasonTypeToString(VIR_DOMAIN_PAUSED_POSTCOPY)); - - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_POSTCOPY); - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY); - qemuDomainSaveStatus(vm); + if (priv->job.asyncJob == VIR_ASYNC_JOB_MIGRATION_OUT && + virDomainObjGetState(vm, &reason) == VIR_DOMAIN_PAUSED) { + if (status == QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY_PAUSED) { + /* At this point no thread is watching the migration progress on + * the source as it is just waiting for the Finish phase to end. + * Thus we need to handle the event here. */ + qemuMigrationSrcPostcopyFailed(vm); + qemuDomainSaveStatus(vm); + } else if (status == QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY && + reason == VIR_DOMAIN_PAUSED_MIGRATION) { + VIR_DEBUG("Correcting paused state reason for domain %s to %s", + vm->def->name, + virDomainPausedReasonTypeToString(VIR_DOMAIN_PAUSED_POSTCOPY)); + + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_POSTCOPY); + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_POSTCOPY); + qemuDomainSaveStatus(vm); + } } cleanup: -- 2.35.1

On Wed, May 11, 2022 at 15:27:53 +0200, Peter Krempa wrote: Yeah. I'll make a follow up patch to fix this. Jirka

On Tue, May 10, 2022 at 17:20:44 +0200, Jiri Denemark wrote: [...] Preferrably conform to the error message coding style here: - no linebreaks - all substitutions should be somehow enclosed e.g. in '%s' Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:20:46 +0200, Jiri Denemark wrote: IMO virGetDomain should be treated as that it can't fail ... ... so this check seems pointless. broken alignment E.g. here you don't check 'dom' because it's not yet allocated. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

Most of the code in "endjob" label is executed only on failure. Let's duplicate the rest so that the label can be used only in error path making the success path easier to follow and refactor. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 45 ++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index cb17cbd189..0c6d2c041b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5854,7 +5854,9 @@ qemuMigrationDstFinish(virQEMUDriver *driver, QEMU_MIGRATION_COOKIE_STATS) < 0) VIR_WARN("Unable to encode migration cookie"); } - goto endjob; + + qemuMigrationJobFinish(vm); + goto cleanup; } if (retcode != 0) { @@ -6035,12 +6037,31 @@ qemuMigrationDstFinish(virQEMUDriver *driver, if (inPostCopy) g_clear_pointer(&priv->job.completed, virDomainJobDataFree); + qemuMigrationParamsReset(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, + jobPriv->migParams, priv->job.apiFlags); + dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, vm->def->id); + qemuMigrationJobFinish(vm); + + cleanup: + g_clear_pointer(&jobData, virDomainJobDataFree); + virPortAllocatorRelease(port); + if (priv->mon) + qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL); + VIR_FREE(priv->origname); + virDomainObjEndAPI(&vm); + virErrorRestore(&orig_err); + + /* Set a special error if Finish is expected to return NULL as a result of + * successful call with retcode != 0 + */ + if (retcode != 0 && !dom && virGetLastErrorCode() == VIR_ERR_OK) + virReportError(VIR_ERR_MIGRATE_FINISH_OK, NULL); + return dom; + endjob: - if (!dom && - !(flags & VIR_MIGRATE_OFFLINE) && - virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) { if (doKill) { qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, VIR_ASYNC_JOB_MIGRATION_IN, @@ -6069,21 +6090,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, if (!virDomainObjIsActive(vm)) qemuDomainRemoveInactive(driver, vm); - cleanup: - g_clear_pointer(&jobData, virDomainJobDataFree); - virPortAllocatorRelease(port); - if (priv->mon) - qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL); - VIR_FREE(priv->origname); - virDomainObjEndAPI(&vm); - virErrorRestore(&orig_err); - - /* Set a special error if Finish is expected to return NULL as a result of - * successful call with retcode != 0 - */ - if (retcode != 0 && !dom && virGetLastErrorCode() == VIR_ERR_OK) - virReportError(VIR_ERR_MIGRATE_FINISH_OK, NULL); - return dom; + goto cleanup; } -- 2.35.1

Let's call it "error" so that it's clear the label is only used in failure path. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0c6d2c041b..4096aba0a6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5840,7 +5840,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, if (!(mig = qemuMigrationCookieParse(driver, vm->def, priv->origname, priv, cookiein, cookieinlen, cookie_flags))) - goto endjob; + goto error; if (flags & VIR_MIGRATE_OFFLINE) { if (retcode == 0 && @@ -5864,31 +5864,31 @@ qemuMigrationDstFinish(virQEMUDriver *driver, * earlier than monitor EOF handler got a chance to process the error */ qemuDomainCheckMonitor(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN); - goto endjob; + goto error; } if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("guest unexpectedly quit")); qemuMigrationDstErrorReport(driver, vm->def->name); - goto endjob; + goto error; } if (qemuMigrationDstVPAssociatePortProfiles(vm->def) < 0) - goto endjob; + goto error; if (mig->network && qemuMigrationDstOPDRelocate(driver, vm, mig) < 0) VIR_WARN("unable to provide network data for relocation"); if (qemuMigrationDstStopNBDServer(driver, vm, mig) < 0) - goto endjob; + goto error; if (qemuRefreshVirtioChannelState(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN) < 0) - goto endjob; + goto error; if (qemuConnectAgent(driver, vm) < 0) - goto endjob; + goto error; if (flags & VIR_MIGRATE_PERSIST_DEST) { if (qemuMigrationDstPersist(driver, vm, mig, !v3proto) < 0) { @@ -5905,7 +5905,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, * to restart during confirm() step, so we kill it off now. */ if (v3proto) - goto endjob; + goto error; } } @@ -5919,7 +5919,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, * original domain on the source host is already gone. */ if (v3proto) - goto endjob; + goto error; } /* Now that the state data was transferred we can refresh the actual state @@ -5928,7 +5928,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, /* Similarly to the case above v2 protocol will not be able to recover * from this. Let's ignore this and perhaps stuff will not break. */ if (v3proto) - goto endjob; + goto error; } if (priv->job.current->status == VIR_DOMAIN_JOB_STATUS_POSTCOPY) @@ -5960,7 +5960,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, * things up. */ if (v3proto) - goto endjob; + goto error; } if (inPostCopy) @@ -5983,7 +5983,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, if (qemuMigrationDstWaitForCompletion(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, false) < 0) { - goto endjob; + goto error; } if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { virDomainObjSetState(vm, @@ -6060,7 +6060,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, virReportError(VIR_ERR_MIGRATE_FINISH_OK, NULL); return dom; - endjob: + error: if (virDomainObjIsActive(vm)) { if (doKill) { qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, -- 2.35.1

The final part of Finish phase will be refactored into a dedicated function and we don't want to generate the cookie there. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4096aba0a6..85ab4257d4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5979,12 +5979,27 @@ qemuMigrationDstFinish(virQEMUDriver *driver, qemuDomainJobDataUpdateDowntime(jobData); } + if (inPostCopy && + qemuMigrationDstWaitForCompletion(driver, vm, + VIR_ASYNC_JOB_MIGRATION_IN, + false) < 0) { + goto error; + } + + if (jobData) { + priv->job.completed = g_steal_pointer(&jobData); + priv->job.completed->status = VIR_DOMAIN_JOB_STATUS_COMPLETED; + qemuDomainJobSetStatsType(priv->job.completed, + QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION); + } + + if (qemuMigrationCookieFormat(mig, driver, vm, + QEMU_MIGRATION_DESTINATION, + cookieout, cookieoutlen, + QEMU_MIGRATION_COOKIE_STATS) < 0) + VIR_WARN("Unable to encode migration cookie"); + if (inPostCopy) { - if (qemuMigrationDstWaitForCompletion(driver, vm, - VIR_ASYNC_JOB_MIGRATION_IN, - false) < 0) { - goto error; - } if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, @@ -6018,19 +6033,6 @@ qemuMigrationDstFinish(virQEMUDriver *driver, /* Guest is successfully running, so cancel previous auto destroy */ qemuProcessAutoDestroyRemove(driver, vm); - if (jobData) { - priv->job.completed = g_steal_pointer(&jobData); - priv->job.completed->status = VIR_DOMAIN_JOB_STATUS_COMPLETED; - qemuDomainJobSetStatsType(priv->job.completed, - QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION); - } - - if (qemuMigrationCookieFormat(mig, driver, vm, - QEMU_MIGRATION_DESTINATION, - cookieout, cookieoutlen, - QEMU_MIGRATION_COOKIE_STATS) < 0) - VIR_WARN("Unable to encode migration cookie"); - /* Remove completed stats for post-copy, everything but timing fields * is obsolete anyway. */ -- 2.35.1

By separating it into a dedicated qemuMigrationDstComplete function which can be later called in other places. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 99 ++++++++++++++++++++++----------------- src/qemu/qemu_migration.h | 6 +++ 2 files changed, 63 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 85ab4257d4..a3d55c8e8b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5784,6 +5784,61 @@ qemuMigrationDstPersist(virQEMUDriver *driver, } +void +qemuMigrationDstComplete(virQEMUDriver *driver, + virDomainObj *vm, + bool inPostCopy, + virDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivate *priv = vm->privateData; + qemuDomainJobPrivate *jobPriv = priv->job.privateData; + virObjectEvent *event; + + if (inPostCopy) { + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + virDomainObjSetState(vm, + VIR_DOMAIN_RUNNING, + VIR_DOMAIN_RUNNING_MIGRATED); + } + + /* The only RESUME event during post-copy migration is triggered by + * QEMU when the running domain moves from the source to the + * destination host, but then the migration keeps running until all + * modified memory is transferred from the source host. This will + * result in VIR_DOMAIN_EVENT_RESUMED with RESUMED_POSTCOPY detail. + * However, our API documentation says we need to fire another RESUMED + * event at the very end of migration with RESUMED_MIGRATED detail. + */ + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_RESUMED, + VIR_DOMAIN_EVENT_RESUMED_MIGRATED); + virObjectEventStateQueue(driver->domainEventState, event); + } + + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER); + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); + virObjectEventStateQueue(driver->domainEventState, event); + } + + qemuDomainSaveStatus(vm); + + /* Guest is successfully running, so cancel previous auto destroy */ + qemuProcessAutoDestroyRemove(driver, vm); + + /* Remove completed stats for post-copy, everything but timing fields + * is obsolete anyway. + */ + if (inPostCopy) + g_clear_pointer(&priv->job.completed, virDomainJobDataFree); + + qemuMigrationParamsReset(driver, vm, asyncJob, jobPriv->migParams, + priv->job.apiFlags); +} + + virDomainPtr qemuMigrationDstFinish(virQEMUDriver *driver, virConnectPtr dconn, @@ -5999,48 +6054,8 @@ qemuMigrationDstFinish(virQEMUDriver *driver, QEMU_MIGRATION_COOKIE_STATS) < 0) VIR_WARN("Unable to encode migration cookie"); - if (inPostCopy) { - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - virDomainObjSetState(vm, - VIR_DOMAIN_RUNNING, - VIR_DOMAIN_RUNNING_MIGRATED); - } - - /* The only RESUME event during post-copy migration is triggered by - * QEMU when the running domain moves from the source to the - * destination host, but then the migration keeps running until all - * modified memory is transferred from the source host. This will - * result in VIR_DOMAIN_EVENT_RESUMED with RESUMED_POSTCOPY detail. - * However, our API documentation says we need to fire another RESUMED - * event at the very end of migration with RESUMED_MIGRATED detail. - */ - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_RESUMED, - VIR_DOMAIN_EVENT_RESUMED_MIGRATED); - virObjectEventStateQueue(driver->domainEventState, event); - } - - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER); - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); - virObjectEventStateQueue(driver->domainEventState, event); - } - - qemuDomainSaveStatus(vm); - - /* Guest is successfully running, so cancel previous auto destroy */ - qemuProcessAutoDestroyRemove(driver, vm); - - /* Remove completed stats for post-copy, everything but timing fields - * is obsolete anyway. - */ - if (inPostCopy) - g_clear_pointer(&priv->job.completed, virDomainJobDataFree); - - qemuMigrationParamsReset(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, - jobPriv->migParams, priv->job.apiFlags); + qemuMigrationDstComplete(driver, vm, inPostCopy, + VIR_ASYNC_JOB_MIGRATION_IN); dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, vm->def->id); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index c4e4228282..ecb0ad6302 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -191,6 +191,12 @@ qemuMigrationDstFinish(virQEMUDriver *driver, int retcode, bool v3proto); +void +qemuMigrationDstComplete(virQEMUDriver *driver, + virDomainObj *vm, + bool inPostCopy, + virDomainAsyncJob asyncJob); + int qemuMigrationSrcConfirm(virQEMUDriver *driver, virDomainObj *vm, -- 2.35.1

The comment about QEMU < 0.10.6 has been irrelevant for years. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a3d55c8e8b..a6c8e53ac5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5990,10 +5990,6 @@ qemuMigrationDstFinish(virQEMUDriver *driver, inPostCopy = true; if (!(flags & VIR_MIGRATE_PAUSED)) { - /* run 'cont' on the destination, which allows migration on qemu - * >= 0.10.6 to work properly. This isn't strictly necessary on - * older qemu's, but it also doesn't hurt anything there - */ if (qemuProcessStartCPUs(driver, vm, inPostCopy ? VIR_DOMAIN_RUNNING_POSTCOPY : VIR_DOMAIN_RUNNING_MIGRATED, -- 2.35.1

We want to prevent our error path that can potentially kill the domain on the destination host from overwriting an error reported earlier, but we were only doing so in one specific path when starting vCPUs fails. Let's do it in all paths. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a6c8e53ac5..bbea2f6e0e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5997,11 +5997,6 @@ qemuMigrationDstFinish(virQEMUDriver *driver, if (virGetLastErrorCode() == VIR_ERR_OK) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); - /* Need to save the current error, in case shutting - * down the process overwrites it - */ - virErrorPreserveLast(&orig_err); - /* * In v3 protocol, the source VM is still available to * restart during confirm() step, so we kill it off @@ -6074,6 +6069,10 @@ qemuMigrationDstFinish(virQEMUDriver *driver, return dom; error: + /* Need to save the current error, in case shutting down the process + * overwrites it. */ + virErrorPreserveLast(&orig_err); + if (virDomainObjIsActive(vm)) { if (doKill) { qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, -- 2.35.1

Refactors qemuMigrationDstFinish by moving some parts to a dedicated function for easier introduction of postcopy resume code without duplicating common parts of the Finish phase. The goal is to have the following call graph: - qemuMigrationDstFinish - qemuMigrationDstFinishOffline - qemuMigrationDstFinishActive - qemuMigrationDstFinishFresh - qemuMigrationDstFinishResume Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 234 +++++++++++++++++++++----------------- 1 file changed, 129 insertions(+), 105 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index bbea2f6e0e..385bd91a6b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5839,111 +5839,38 @@ qemuMigrationDstComplete(virQEMUDriver *driver, } -virDomainPtr -qemuMigrationDstFinish(virQEMUDriver *driver, - virConnectPtr dconn, - virDomainObj *vm, - const char *cookiein, - int cookieinlen, - char **cookieout, - int *cookieoutlen, - unsigned long flags, - int retcode, - bool v3proto) +/* + * Perform Finish phase of a fresh (i.e., not recovery) migration of an active + * domain. + */ +static int +qemuMigrationDstFinishFresh(virQEMUDriver *driver, + virDomainObj *vm, + qemuMigrationCookie *mig, + unsigned long flags, + bool v3proto, + unsigned long long timeReceived, + bool *doKill, + bool *inPostCopy) { - virDomainPtr dom = NULL; - g_autoptr(qemuMigrationCookie) mig = NULL; - virErrorPtr orig_err = NULL; - int cookie_flags = 0; qemuDomainObjPrivate *priv = vm->privateData; - qemuDomainJobPrivate *jobPriv = priv->job.privateData; - unsigned short port; - unsigned long long timeReceived = 0; - virObjectEvent *event; - virDomainJobData *jobData = NULL; - bool inPostCopy = false; - bool doKill = true; - - VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " - "cookieout=%p, cookieoutlen=%p, flags=0x%lx, retcode=%d", - driver, dconn, vm, NULLSTR(cookiein), cookieinlen, - cookieout, cookieoutlen, flags, retcode); - - port = priv->migrationPort; - priv->migrationPort = 0; - - if (!qemuMigrationJobIsActive(vm, VIR_ASYNC_JOB_MIGRATION_IN)) { - qemuMigrationDstErrorReport(driver, vm->def->name); - goto cleanup; - } - - ignore_value(virTimeMillisNow(&timeReceived)); - - qemuMigrationJobStartPhase(vm, - v3proto ? QEMU_MIGRATION_PHASE_FINISH3 - : QEMU_MIGRATION_PHASE_FINISH2); - - qemuDomainCleanupRemove(vm, qemuMigrationDstPrepareCleanup); - g_clear_pointer(&priv->job.completed, virDomainJobDataFree); - - cookie_flags = QEMU_MIGRATION_COOKIE_NETWORK | - QEMU_MIGRATION_COOKIE_STATS | - QEMU_MIGRATION_COOKIE_NBD; - /* Some older versions of libvirt always send persistent XML in the cookie - * even though VIR_MIGRATE_PERSIST_DEST was not used. */ - cookie_flags |= QEMU_MIGRATION_COOKIE_PERSISTENT; - - if (!(mig = qemuMigrationCookieParse(driver, vm->def, priv->origname, priv, - cookiein, cookieinlen, cookie_flags))) - goto error; - - if (flags & VIR_MIGRATE_OFFLINE) { - if (retcode == 0 && - qemuMigrationDstPersist(driver, vm, mig, false) == 0) { - dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, -1); - - if (dom && - qemuMigrationCookieFormat(mig, driver, vm, - QEMU_MIGRATION_DESTINATION, - cookieout, cookieoutlen, - QEMU_MIGRATION_COOKIE_STATS) < 0) - VIR_WARN("Unable to encode migration cookie"); - } - - qemuMigrationJobFinish(vm); - goto cleanup; - } - - if (retcode != 0) { - /* Check for a possible error on the monitor in case Finish was called - * earlier than monitor EOF handler got a chance to process the error - */ - qemuDomainCheckMonitor(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN); - goto error; - } - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - qemuMigrationDstErrorReport(driver, vm->def->name); - goto error; - } + g_autoptr(virDomainJobData) jobData = NULL; if (qemuMigrationDstVPAssociatePortProfiles(vm->def) < 0) - goto error; + return -1; if (mig->network && qemuMigrationDstOPDRelocate(driver, vm, mig) < 0) VIR_WARN("unable to provide network data for relocation"); if (qemuMigrationDstStopNBDServer(driver, vm, mig) < 0) - goto error; + return -1; if (qemuRefreshVirtioChannelState(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN) < 0) - goto error; + return -1; if (qemuConnectAgent(driver, vm) < 0) - goto error; + return -1; if (flags & VIR_MIGRATE_PERSIST_DEST) { if (qemuMigrationDstPersist(driver, vm, mig, !v3proto) < 0) { @@ -5960,7 +5887,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, * to restart during confirm() step, so we kill it off now. */ if (v3proto) - goto error; + return -1; } } @@ -5974,7 +5901,7 @@ qemuMigrationDstFinish(virQEMUDriver *driver, * original domain on the source host is already gone. */ if (v3proto) - goto error; + return -1; } /* Now that the state data was transferred we can refresh the actual state @@ -5983,16 +5910,16 @@ qemuMigrationDstFinish(virQEMUDriver *driver, /* Similarly to the case above v2 protocol will not be able to recover * from this. Let's ignore this and perhaps stuff will not break. */ if (v3proto) - goto error; + return -1; } if (priv->job.current->status == VIR_DOMAIN_JOB_STATUS_POSTCOPY) - inPostCopy = true; + *inPostCopy = true; if (!(flags & VIR_MIGRATE_PAUSED)) { if (qemuProcessStartCPUs(driver, vm, - inPostCopy ? VIR_DOMAIN_RUNNING_POSTCOPY - : VIR_DOMAIN_RUNNING_MIGRATED, + *inPostCopy ? VIR_DOMAIN_RUNNING_POSTCOPY + : VIR_DOMAIN_RUNNING_MIGRATED, VIR_ASYNC_JOB_MIGRATION_IN) < 0) { if (virGetLastErrorCode() == VIR_ERR_OK) virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6006,11 +5933,11 @@ qemuMigrationDstFinish(virQEMUDriver *driver, * things up. */ if (v3proto) - goto error; + return -1; } - if (inPostCopy) - doKill = false; + if (*inPostCopy) + *doKill = false; } if (mig->jobData) { @@ -6025,11 +5952,11 @@ qemuMigrationDstFinish(virQEMUDriver *driver, qemuDomainJobDataUpdateDowntime(jobData); } - if (inPostCopy && + if (*inPostCopy && qemuMigrationDstWaitForCompletion(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, false) < 0) { - goto error; + return -1; } if (jobData) { @@ -6039,6 +5966,106 @@ qemuMigrationDstFinish(virQEMUDriver *driver, QEMU_DOMAIN_JOB_STATS_TYPE_MIGRATION); } + return 0; +} + + +virDomainPtr +qemuMigrationDstFinish(virQEMUDriver *driver, + virConnectPtr dconn, + virDomainObj *vm, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + unsigned long flags, + int retcode, + bool v3proto) +{ + virDomainPtr dom = NULL; + g_autoptr(qemuMigrationCookie) mig = NULL; + virErrorPtr orig_err = NULL; + int cookie_flags = 0; + qemuDomainObjPrivate *priv = vm->privateData; + qemuDomainJobPrivate *jobPriv = priv->job.privateData; + unsigned short port; + unsigned long long timeReceived = 0; + virObjectEvent *event; + bool inPostCopy = false; + bool doKill = true; + int rc; + + VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, flags=0x%lx, retcode=%d", + driver, dconn, vm, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, retcode); + + port = priv->migrationPort; + priv->migrationPort = 0; + + if (!qemuMigrationJobIsActive(vm, VIR_ASYNC_JOB_MIGRATION_IN)) { + qemuMigrationDstErrorReport(driver, vm->def->name); + goto cleanup; + } + + ignore_value(virTimeMillisNow(&timeReceived)); + + qemuMigrationJobStartPhase(vm, + v3proto ? QEMU_MIGRATION_PHASE_FINISH3 + : QEMU_MIGRATION_PHASE_FINISH2); + + qemuDomainCleanupRemove(vm, qemuMigrationDstPrepareCleanup); + g_clear_pointer(&priv->job.completed, virDomainJobDataFree); + + cookie_flags = QEMU_MIGRATION_COOKIE_NETWORK | + QEMU_MIGRATION_COOKIE_STATS | + QEMU_MIGRATION_COOKIE_NBD; + /* Some older versions of libvirt always send persistent XML in the cookie + * even though VIR_MIGRATE_PERSIST_DEST was not used. */ + cookie_flags |= QEMU_MIGRATION_COOKIE_PERSISTENT; + + if (!(mig = qemuMigrationCookieParse(driver, vm->def, priv->origname, priv, + cookiein, cookieinlen, cookie_flags))) + goto error; + + if (flags & VIR_MIGRATE_OFFLINE) { + if (retcode == 0 && + qemuMigrationDstPersist(driver, vm, mig, false) == 0) { + dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, -1); + + if (dom && + qemuMigrationCookieFormat(mig, driver, vm, + QEMU_MIGRATION_DESTINATION, + cookieout, cookieoutlen, + QEMU_MIGRATION_COOKIE_STATS) < 0) + VIR_WARN("Unable to encode migration cookie"); + } + + qemuMigrationJobFinish(vm); + goto cleanup; + } + + if (retcode != 0) { + /* Check for a possible error on the monitor in case Finish was called + * earlier than monitor EOF handler got a chance to process the error + */ + qemuDomainCheckMonitor(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN); + goto error; + } + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + qemuMigrationDstErrorReport(driver, vm->def->name); + goto error; + } + + rc = qemuMigrationDstFinishFresh(driver, vm, mig, flags, v3proto, + timeReceived, &doKill, &inPostCopy); + if (rc < 0 || + !(dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, vm->def->id))) + goto error; + if (qemuMigrationCookieFormat(mig, driver, vm, QEMU_MIGRATION_DESTINATION, cookieout, cookieoutlen, @@ -6048,12 +6075,9 @@ qemuMigrationDstFinish(virQEMUDriver *driver, qemuMigrationDstComplete(driver, vm, inPostCopy, VIR_ASYNC_JOB_MIGRATION_IN); - dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, vm->def->id); - qemuMigrationJobFinish(vm); cleanup: - g_clear_pointer(&jobData, virDomainJobDataFree); virPortAllocatorRelease(port); if (priv->mon) qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL); -- 2.35.1

Refactors qemuMigrationDstFinish by moving some parts to a dedicated function for easier introduction of postcopy resume code without duplicating common parts of the Finish phase. The goal is to have the following call graph: - qemuMigrationDstFinish - qemuMigrationDstFinishOffline - qemuMigrationDstFinishActive - qemuMigrationDstFinishFresh - qemuMigrationDstFinishResume Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 385bd91a6b..dcd7ff3597 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5839,6 +5839,32 @@ qemuMigrationDstComplete(virQEMUDriver *driver, } +static virDomainPtr +qemuMigrationDstFinishOffline(virQEMUDriver *driver, + virConnectPtr dconn, + virDomainObj *vm, + qemuMigrationCookie *mig, + char **cookieout, + int *cookieoutlen) +{ + virDomainPtr dom = NULL; + + if (qemuMigrationDstPersist(driver, vm, mig, false) < 0) + return NULL; + + dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, -1); + + if (dom && + qemuMigrationCookieFormat(mig, driver, vm, + QEMU_MIGRATION_DESTINATION, + cookieout, cookieoutlen, + QEMU_MIGRATION_COOKIE_STATS) < 0) + VIR_WARN("Unable to encode migration cookie"); + + return dom; +} + + /* * Perform Finish phase of a fresh (i.e., not recovery) migration of an active * domain. @@ -6029,16 +6055,9 @@ qemuMigrationDstFinish(virQEMUDriver *driver, goto error; if (flags & VIR_MIGRATE_OFFLINE) { - if (retcode == 0 && - qemuMigrationDstPersist(driver, vm, mig, false) == 0) { - dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, -1); - - if (dom && - qemuMigrationCookieFormat(mig, driver, vm, - QEMU_MIGRATION_DESTINATION, - cookieout, cookieoutlen, - QEMU_MIGRATION_COOKIE_STATS) < 0) - VIR_WARN("Unable to encode migration cookie"); + if (retcode == 0) { + dom = qemuMigrationDstFinishOffline(driver, dconn, vm, mig, + cookieout, cookieoutlen); } qemuMigrationJobFinish(vm); -- 2.35.1

On Wed, May 11, 2022 at 16:53:48 +0200, Peter Krempa wrote: That's because the cookie is formatted before calling virGetDomain there. We could swap it here too, or drop the check, but we can as well keep it as is and avoid formatting the cookie when virGetDomain fails (even though the failure is mostly theoretical). Jirka

On Tue, May 10, 2022 at 17:20:56 +0200, Jiri Denemark wrote: The argument list is getting a bit riddiculous. Especially weird is that you pass 'orig_err' in. The only error that could overwrite it is from virPortAllocatorRelease and that one IMNSHO should not report any errrors or at least have a quiet version. Please don't pass the error out, but re-set it and deal with the caller separately. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:20:57 +0200, Jiri Denemark wrote: [1] Come on! Really?!? [1] [....] If I overlook the fact that you have to remember what qemuMigrationDstFinishActive you used probably the least easy variant of seeing what is happening with that extra jump. Do it without that jump: if (finishJob) qemuMigrationJobFinish(vm); else qemuMigrationJobContinue(vm); With that I'll maybe buy the argument from the commit message, thus: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:20:58 +0200, Jiri Denemark wrote: Fix the alignment here. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:02 +0200, Jiri Denemark wrote: Please fix error messages to conform with coding style since you are touching them. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:03 +0200, Jiri Denemark wrote: Diagnostics on a single line please [...] This one too. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:04 +0200, Jiri Denemark wrote: I'm aware that you are doing pure code movement, but remove the linebreaks in the error messages. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:05 +0200, Jiri Denemark wrote: The name of this function is a bit too generic IMO, and doesn't imply that it's actually formatting the XML. Also note that 'driver' can be fetched from 'priv->driver' since you are refactoring the code. Preferrably remove the 'driver' argument. I can live with the name. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:06 +0200, Jiri Denemark wrote: [...] I'd consider this case to be success ... ... so this technically violates the return values as you've previously declared that: * -1 on error, the domain will be killed, Please add a comment that makes the reader aware that this is success actually or modify the comment at the top of the function. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:08 +0200, Jiri Denemark wrote: This bit seems to belongs to the previous commit actually, but since it's not used anywhere else ... [...] Any reason why you want to ignore this before the migration was performed? I could somehwat understand it here after the migration is done, but a bug could be also in this code. Same here.

We will want to update migration phase without affecting job ownership. Either in the thread that already owns the job or from an event handler which only changes the phase (of a job no-one owns) without assuming it. Let's move the ownership change to a new qemuDomainObjStartJobPhase helper and let qemuDomainObjSetJobPhase set the phase without touching ownership. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_domainjob.c | 44 +++++++++++++++++++++++++++++++++++---- src/qemu/qemu_domainjob.h | 3 +++ src/qemu/qemu_migration.c | 2 +- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index 8e8d229afe..10381755a9 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -714,6 +714,10 @@ qemuDomainJobDataToParams(virDomainJobData *jobData, } +/* + * Sets the job phase without changing the job owner. The owner is supposed to + * be 0 or the current thread, a warning is issued otherwise. + */ void qemuDomainObjSetJobPhase(virDomainObj *obj, int phase) @@ -728,19 +732,51 @@ qemuDomainObjSetJobPhase(virDomainObj *obj, virDomainAsyncJobTypeToString(priv->job.asyncJob), qemuDomainAsyncJobPhaseToString(priv->job.asyncJob, phase)); + if (priv->job.asyncOwner != 0 && + priv->job.asyncOwner != me) { + VIR_WARN("'%s' async job is owned by thread %llu, API '%s'", + virDomainAsyncJobTypeToString(priv->job.asyncJob), + priv->job.asyncOwner, + NULLSTR(priv->job.asyncOwnerAPI)); + } + + priv->job.phase = phase; + qemuDomainSaveStatus(obj); +} + + +/* + * Changes the job owner and sets the job phase. The current owner is supposed + * to be 0 or the current thread, a warning is issued otherwise. + */ +void +qemuDomainObjStartJobPhase(virDomainObj *obj, + int phase) +{ + qemuDomainObjPrivate *priv = obj->privateData; + unsigned long long me = virThreadSelfID(); + + if (!priv->job.asyncJob) + return; + + VIR_DEBUG("Starting phase '%s' of '%s' job", + qemuDomainAsyncJobPhaseToString(priv->job.asyncJob, phase), + virDomainAsyncJobTypeToString(priv->job.asyncJob)); + if (priv->job.asyncOwner == 0) { priv->job.asyncOwnerAPI = g_strdup(virThreadJobGet()); } else if (me != priv->job.asyncOwner) { - VIR_WARN("'%s' async job is owned by thread %llu", + VIR_WARN("'%s' async job is owned by thread %llu, API '%s'", virDomainAsyncJobTypeToString(priv->job.asyncJob), - priv->job.asyncOwner); + priv->job.asyncOwner, + NULLSTR(priv->job.asyncOwnerAPI)); } - priv->job.phase = phase; priv->job.asyncOwner = me; - qemuDomainSaveStatus(obj); + qemuDomainObjSetJobPhase(obj, phase); } + void qemuDomainObjSetAsyncJobMask(virDomainObj *obj, unsigned long long allowedJobs) diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h index 707d4e91ed..e8021a7f04 100644 --- a/src/qemu/qemu_domainjob.h +++ b/src/qemu/qemu_domainjob.h @@ -156,6 +156,9 @@ void qemuDomainObjEndAsyncJob(virDomainObj *obj); void qemuDomainObjAbortAsyncJob(virDomainObj *obj); void qemuDomainObjSetJobPhase(virDomainObj *obj, int phase); +void +qemuDomainObjStartJobPhase(virDomainObj *obj, + int phase); void qemuDomainObjSetAsyncJobMask(virDomainObj *obj, unsigned long long allowedJobs); int qemuDomainObjPreserveJob(virDomainObj *obj, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1c5dd9b391..e4c0c1c6f4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -165,7 +165,7 @@ qemuMigrationJobSetPhase(virDomainObj *vm, if (qemuMigrationCheckPhase(vm, phase) < 0) return -1; - qemuDomainObjSetJobPhase(vm, phase); + qemuDomainObjStartJobPhase(vm, phase); return 0; } -- 2.35.1

Both qemuMigrationJobSetPhase and qemuMigrationJobStartPhase were doing the same thing, which made little sense. Let's follow the difference between qemuDomainObjSetJobPhase and qemuDomainObjStartJobPhase and change job owner only in qemuMigrationJobStartPhase. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e4c0c1c6f4..3f6921b4b2 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -165,7 +165,7 @@ qemuMigrationJobSetPhase(virDomainObj *vm, if (qemuMigrationCheckPhase(vm, phase) < 0) return -1; - qemuDomainObjStartJobPhase(vm, phase); + qemuDomainObjSetJobPhase(vm, phase); return 0; } @@ -174,7 +174,11 @@ static int G_GNUC_WARN_UNUSED_RESULT qemuMigrationJobStartPhase(virDomainObj *vm, qemuMigrationJobPhase phase) { - return qemuMigrationJobSetPhase(vm, phase); + if (qemuMigrationCheckPhase(vm, phase) < 0) + return -1; + + qemuDomainObjStartJobPhase(vm, phase); + return 0; } @@ -2599,7 +2603,7 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver, * change protection. */ if (priv->job.asyncJob == VIR_ASYNC_JOB_MIGRATION_OUT && - qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_BEGIN3) < 0) + qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_BEGIN3) < 0) return NULL; if (!qemuMigrationSrcIsAllowed(driver, vm, true, flags)) @@ -3152,7 +3156,7 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, flags) < 0) goto cleanup; - if (qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_PREPARE) < 0) + if (qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_PREPARE) < 0) goto stopjob; /* Domain starts inactive, even if the domain XML had an id field. */ @@ -3673,7 +3677,7 @@ qemuMigrationSrcConfirmPhase(virQEMUDriver *driver, else phase = QEMU_MIGRATION_PHASE_CONFIRM3_CANCELLED; - if (qemuMigrationJobSetPhase(vm, phase) < 0) + if (qemuMigrationJobStartPhase(vm, phase) < 0) return -1; if (!(mig = qemuMigrationCookieParse(driver, vm->def, priv->origname, priv, @@ -4928,7 +4932,7 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriver *driver, * until the migration is complete. */ VIR_DEBUG("Perform %p", sconn); - ignore_value(qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_PERFORM2)); + ignore_value(qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_PERFORM2)); if (flags & VIR_MIGRATE_TUNNELLED) ret = qemuMigrationSrcPerformTunnel(driver, vm, st, NULL, NULL, 0, NULL, NULL, @@ -5573,7 +5577,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, migParams, flags, dname, resource, &v3proto); } else { - if (qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_PERFORM2) < 0) + if (qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_PERFORM2) < 0) goto endjob; ret = qemuMigrationSrcPerformNative(driver, vm, persist_xml, uri, cookiein, cookieinlen, -- 2.35.1

On Tue, May 10, 2022 at 17:21:11 +0200, Jiri Denemark wrote: [...] This hunk seems to be misplaced or at least doesn't really seem to be related to anything this patch is claiming to do. [..] This bit also doesn't seem to be justified by what this patch is supposed to do. The rest looks good so if you explain what's going on in those two cases you can use: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Wed, May 18, 2022 at 14:53:50 +0200, Jiri Denemark wrote: Fair enough on both points. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:15 +0200, Jiri Denemark wrote: Error messages shall not have line breaks. Also ... requiring this flag seems to be a bit pointless and ... cosmetic at best. Since the previous migration would need to be started with the post-copy flag already which is checked below. Same here. [...] > +static char * > +qemuMigrationSrcBeginResume(virQEMUDriver *driver, > + virDomainObj *vm, > + const char *xmlin, > + char **cookieout, > + int *cookieoutlen, > + unsigned long flags) > +{ > + virDomainJobStatus status; > + > + if (qemuMigrationAnyRefreshStatus(driver, vm, VIR_ASYNC_JOB_MIGRATION_OUT, > + &status) < 0) > + return NULL; > + > + if (status != VIR_DOMAIN_JOB_STATUS_POSTCOPY_PAUSED) { > + virReportError(VIR_ERR_OPERATION_INVALID, "%s", > + _("QEMU reports migration is still running")); > + > + return qemuMigrationSrcBeginXML(driver, vm, xmlin, > + cookieout, cookieoutlen, 0, NULL, 0, flags); Certain steps here around the non-shared-storage migration are redundant at this point and IMO should be skipped. Could you please add an explanation why we are removing the callbacks ... Just to add them here?

On Wed, May 18, 2022 at 15:40:18 +0200, Jiri Denemark wrote: I was kind of referring to the fact that it's implied that postcopy is being resumed with a POSTCOPY_RESUME flag and you need to check anyways that the migration is indeed postcopy, so this feels redundant, but I think it's okay. In 'qemuMigrationSrcBeginXML' qemuMigrationSrcBeginPhaseBlockDirtyBitmaps is called which does a bunch of setup for bitmap migration which is pointless when recovering as storage is copied already if the CPUs are running on the destination.

On Tue, May 10, 2022 at 05:21:15PM +0200, Jiri Denemark wrote: s/return NULL/return false/

On Tue, May 10, 2022 at 17:21:16 +0200, Jiri Denemark wrote: So this failure now becomes an error from previously being technically ignored, and ... ... this call will not be skipped in such case, thus restoring the VM, but since we fail anyways the migration will be cancelled. So I think this can be considered a "bugfix" for a extremely unlikely/impossible bug. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:17 +0200, Jiri Denemark wrote: driver should not be needed. you can get it in priv->driver. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:18 +0200, Jiri Denemark wrote: [...] Okay, resume is supported in qemu-3.1 already so we don't need "B:...". Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:19 +0200, Jiri Denemark wrote: 'driver' is redundant. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

Moves most of the code from qemuMigrationDstPrepareAny to a new qemuMigrationDstPrepareFresh so that qemuMigrationDstPrepareAny can be shared with post-copy resume. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 165 ++++++++++++++++++++++++-------------- 1 file changed, 103 insertions(+), 62 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8ed1c2c2b6..f1e3774034 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3119,27 +3119,26 @@ qemuMigrationDstPrepareAnyBlockDirtyBitmaps(virDomainObj *vm, static int -qemuMigrationDstPrepareAny(virQEMUDriver *driver, - virConnectPtr dconn, - const char *cookiein, - int cookieinlen, - char **cookieout, - int *cookieoutlen, - virDomainDef **def, - const char *origname, - virStreamPtr st, - const char *protocol, - unsigned short port, - bool autoPort, - const char *listenAddress, - size_t nmigrate_disks, - const char **migrate_disks, - int nbdPort, - const char *nbdURI, - qemuMigrationParams *migParams, - unsigned long flags) +qemuMigrationDstPrepareFresh(virQEMUDriver *driver, + virConnectPtr dconn, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + virDomainDef **def, + const char *origname, + virStreamPtr st, + const char *protocol, + unsigned short port, + bool autoPort, + const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, + int nbdPort, + const char *nbdURI, + qemuMigrationParams *migParams, + unsigned long flags) { - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virDomainObj *vm = NULL; virObjectEvent *event = NULL; virErrorPtr origErr; @@ -3159,55 +3158,18 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, int rv; g_autofree char *tlsAlias = NULL; + VIR_DEBUG("name=%s, origname=%s, protocol=%s, port=%hu, " + "listenAddress=%s, nbdPort=%d, nbdURI=%s, flags=0x%lx", + (*def)->name, NULLSTR(origname), protocol, port, + listenAddress, nbdPort, NULLSTR(nbdURI), flags); + if (flags & VIR_MIGRATE_OFFLINE) { - if (flags & (VIR_MIGRATE_NON_SHARED_DISK | - VIR_MIGRATE_NON_SHARED_INC)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("offline migration cannot handle " - "non-shared storage")); - goto cleanup; - } - if (!(flags & VIR_MIGRATE_PERSIST_DEST)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("offline migration must be specified with " - "the persistent flag set")); - goto cleanup; - } - if (tunnel) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("tunnelled offline migration does not " - "make sense")); - goto cleanup; - } cookieFlags = 0; } else { cookieFlags = QEMU_MIGRATION_COOKIE_GRAPHICS | QEMU_MIGRATION_COOKIE_CAPS; } - if (flags & VIR_MIGRATE_POSTCOPY && - (!(flags & VIR_MIGRATE_LIVE) || - flags & VIR_MIGRATE_PAUSED)) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("post-copy migration is not supported with non-live " - "or paused migration")); - goto cleanup; - } - - if (flags & VIR_MIGRATE_POSTCOPY && flags & VIR_MIGRATE_TUNNELLED) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("post-copy is not supported with tunnelled migration")); - goto cleanup; - } - - if (cfg->migrateTLSForce && - !(flags & VIR_MIGRATE_TUNNELLED) && - !(flags & VIR_MIGRATE_TLS)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("this libvirtd instance allows migration only with VIR_MIGRATE_TLS flag")); - goto cleanup; - } - if (!qemuMigrationSrcIsAllowedHostdev(*def)) goto cleanup; @@ -3499,6 +3461,85 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, } +static int +qemuMigrationDstPrepareAny(virQEMUDriver *driver, + virConnectPtr dconn, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + virDomainDef **def, + const char *origname, + virStreamPtr st, + const char *protocol, + unsigned short port, + bool autoPort, + const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, + int nbdPort, + const char *nbdURI, + qemuMigrationParams *migParams, + unsigned long flags) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + if (flags & VIR_MIGRATE_OFFLINE) { + if (flags & (VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("offline migration cannot handle " + "non-shared storage")); + return -1; + } + if (!(flags & VIR_MIGRATE_PERSIST_DEST)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("offline migration must be specified with " + "the persistent flag set")); + return -1; + } + if (st) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("tunnelled offline migration does not " + "make sense")); + return -1; + } + } + + if (flags & VIR_MIGRATE_POSTCOPY && + (!(flags & VIR_MIGRATE_LIVE) || + flags & VIR_MIGRATE_PAUSED)) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("post-copy migration is not supported with non-live " + "or paused migration")); + return -1; + } + + if (flags & VIR_MIGRATE_POSTCOPY && flags & VIR_MIGRATE_TUNNELLED) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("post-copy is not supported with tunnelled migration")); + return -1; + } + + if (cfg->migrateTLSForce && + !(flags & VIR_MIGRATE_TUNNELLED) && + !(flags & VIR_MIGRATE_TLS)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("this libvirtd instance allows migration only with VIR_MIGRATE_TLS flag")); + return -1; + } + + return qemuMigrationDstPrepareFresh(driver, dconn, + cookiein, cookieinlen, + cookieout, cookieoutlen, + def, origname, st, protocol, + port, autoPort, listenAddress, + nmigrate_disks, migrate_disks, + nbdPort, nbdURI, + migParams, flags); +} + + /* * This version starts an empty VM listening on a localhost TCP port, and * sets up the corresponding virStream to handle the incoming data. -- 2.35.1

Offline migration jumps over a big part of qemuMigrationDstPrepareFresh. Let's move that part into a new qemuMigrationDstPrepareActive function to make the code easier to follow. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 374 +++++++++++++++++++++----------------- 1 file changed, 206 insertions(+), 168 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f1e3774034..dc608fb8a4 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3118,6 +3118,200 @@ qemuMigrationDstPrepareAnyBlockDirtyBitmaps(virDomainObj *vm, } +static int +qemuMigrationDstPrepareActive(virQEMUDriver *driver, + virDomainObj *vm, + virConnectPtr dconn, + qemuMigrationCookie *mig, + virStreamPtr st, + const char *protocol, + unsigned short port, + const char *listenAddress, + size_t nmigrate_disks, + const char **migrate_disks, + int nbdPort, + const char *nbdURI, + qemuMigrationParams *migParams, + unsigned long flags) +{ + qemuDomainObjPrivate *priv = vm->privateData; + qemuDomainJobPrivate *jobPriv = priv->job.privateData; + qemuProcessIncomingDef *incoming = NULL; + g_autofree char *tlsAlias = NULL; + virObjectEvent *event = NULL; + virErrorPtr origErr = NULL; + int dataFD[2] = { -1, -1 }; + bool stopProcess = false; + unsigned int startFlags; + bool relabel = false; + bool tunnel = !!st; + int ret = -1; + int rv; + + if (STREQ_NULLABLE(protocol, "rdma") && + !virMemoryLimitIsSet(vm->def->mem.hard_limit)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot start RDMA migration with no memory hard " + "limit set")); + goto error; + } + + if (qemuMigrationDstPrecreateStorage(vm, mig->nbd, + nmigrate_disks, migrate_disks, + !!(flags & VIR_MIGRATE_NON_SHARED_INC)) < 0) + goto error; + + if (tunnel && + virPipe(dataFD) < 0) + goto error; + + startFlags = VIR_QEMU_PROCESS_START_AUTODESTROY; + + if (qemuProcessInit(driver, vm, mig->cpu, VIR_ASYNC_JOB_MIGRATION_IN, + true, startFlags) < 0) + goto error; + stopProcess = true; + + if (!(incoming = qemuMigrationDstPrepare(vm, tunnel, protocol, + listenAddress, port, + dataFD[0]))) + goto error; + + if (qemuProcessPrepareDomain(driver, vm, startFlags) < 0) + goto error; + + if (qemuProcessPrepareHost(driver, vm, startFlags) < 0) + goto error; + + rv = qemuProcessLaunch(dconn, driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, + incoming, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START, + startFlags); + if (rv < 0) { + if (rv == -2) + relabel = true; + goto error; + } + relabel = true; + + if (tunnel) { + if (virFDStreamOpen(st, dataFD[1]) < 0) { + virReportSystemError(errno, "%s", + _("cannot pass pipe for tunnelled migration")); + goto error; + } + dataFD[1] = -1; /* 'st' owns the FD now & will close it */ + } + + if (STREQ_NULLABLE(protocol, "rdma") && + vm->def->mem.hard_limit > 0 && + virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) { + goto error; + } + + if (qemuMigrationDstPrepareAnyBlockDirtyBitmaps(vm, mig, migParams, flags) < 0) + goto error; + + if (qemuMigrationParamsCheck(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, + migParams, mig->caps->automatic) < 0) + goto error; + + /* Migrations using TLS need to add the "tls-creds-x509" object and + * set the migration TLS parameters */ + if (flags & VIR_MIGRATE_TLS) { + if (qemuMigrationParamsEnableTLS(driver, vm, true, + VIR_ASYNC_JOB_MIGRATION_IN, + &tlsAlias, NULL, + migParams) < 0) + goto error; + } else { + if (qemuMigrationParamsDisableTLS(vm, migParams) < 0) + goto error; + } + + if (qemuMigrationParamsApply(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, + migParams) < 0) + goto error; + + if (mig->nbd && + flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { + const char *nbdTLSAlias = NULL; + + if (flags & VIR_MIGRATE_TLS) { + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_TLS)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("QEMU NBD server does not support TLS transport")); + goto error; + } + + nbdTLSAlias = tlsAlias; + } + + if (qemuMigrationDstStartNBDServer(driver, vm, incoming->address, + nmigrate_disks, migrate_disks, + nbdPort, nbdURI, + nbdTLSAlias) < 0) { + goto error; + } + } + + if (mig->lockState) { + VIR_DEBUG("Received lockstate %s", mig->lockState); + VIR_FREE(priv->lockState); + priv->lockState = g_steal_pointer(&mig->lockState); + } else { + VIR_DEBUG("Received no lockstate"); + } + + if (qemuMigrationDstRun(driver, vm, incoming->uri, + VIR_ASYNC_JOB_MIGRATION_IN) < 0) + goto error; + + if (qemuProcessFinishStartup(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, + false, VIR_DOMAIN_PAUSED_MIGRATION) < 0) + goto error; + + if (!(flags & VIR_MIGRATE_OFFLINE)) { + virDomainAuditStart(vm, "migrated", true); + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_MIGRATED); + } + + ret = 0; + + cleanup: + qemuProcessIncomingDefFree(incoming); + VIR_FORCE_CLOSE(dataFD[0]); + VIR_FORCE_CLOSE(dataFD[1]); + virObjectEventStateQueue(driver->domainEventState, event); + virErrorRestore(&origErr); + return ret; + + error: + virErrorPreserveLast(&origErr); + qemuMigrationParamsReset(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, + jobPriv->migParams, priv->job.apiFlags); + + if (stopProcess) { + unsigned int stopFlags = VIR_QEMU_PROCESS_STOP_MIGRATED; + if (!relabel) + stopFlags |= VIR_QEMU_PROCESS_STOP_NO_RELABEL; + virDomainAuditStart(vm, "migrated", false); + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, + VIR_ASYNC_JOB_MIGRATION_IN, stopFlags); + /* release if port is auto selected which is not the case if + * it is given in parameters + */ + if (nbdPort == 0) + virPortAllocatorRelease(priv->nbdPort); + priv->nbdPort = 0; + } + goto cleanup; +} + + static int qemuMigrationDstPrepareFresh(virQEMUDriver *driver, virConnectPtr dconn, @@ -3140,32 +3334,20 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, unsigned long flags) { virDomainObj *vm = NULL; - virObjectEvent *event = NULL; virErrorPtr origErr; int ret = -1; - int dataFD[2] = { -1, -1 }; qemuDomainObjPrivate *priv = NULL; qemuMigrationCookie *mig = NULL; - qemuDomainJobPrivate *jobPriv = NULL; - bool tunnel = !!st; g_autofree char *xmlout = NULL; - unsigned int cookieFlags; - unsigned int startFlags; - qemuProcessIncomingDef *incoming = NULL; + unsigned int cookieFlags = 0; bool taint_hook = false; - bool stopProcess = false; - bool relabel = false; - int rv; - g_autofree char *tlsAlias = NULL; VIR_DEBUG("name=%s, origname=%s, protocol=%s, port=%hu, " "listenAddress=%s, nbdPort=%d, nbdURI=%s, flags=0x%lx", (*def)->name, NULLSTR(origname), protocol, port, listenAddress, nbdPort, NULLSTR(nbdURI), flags); - if (flags & VIR_MIGRATE_OFFLINE) { - cookieFlags = 0; - } else { + if (!(flags & VIR_MIGRATE_OFFLINE)) { cookieFlags = QEMU_MIGRATION_COOKIE_GRAPHICS | QEMU_MIGRATION_COOKIE_CAPS; } @@ -3238,7 +3420,6 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, goto cleanup; priv = vm->privateData; - jobPriv = priv->job.privateData; priv->origname = g_strdup(origname); if (taint_hook) { @@ -3246,19 +3427,6 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, priv->hookRun = true; } - if (STREQ_NULLABLE(protocol, "rdma") && - !virMemoryLimitIsSet(vm->def->mem.hard_limit)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot start RDMA migration with no memory hard " - "limit set")); - goto cleanup; - } - - if (qemuMigrationDstPrecreateStorage(vm, mig->nbd, - nmigrate_disks, migrate_disks, - !!(flags & VIR_MIGRATE_NON_SHARED_INC)) < 0) - goto cleanup; - if (qemuMigrationJobStart(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, flags) < 0) goto cleanup; @@ -3269,122 +3437,21 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, /* Domain starts inactive, even if the domain XML had an id field. */ vm->def->id = -1; - if (flags & VIR_MIGRATE_OFFLINE) - goto done; - - if (tunnel && - virPipe(dataFD) < 0) - goto stopjob; - - startFlags = VIR_QEMU_PROCESS_START_AUTODESTROY; - - if (qemuProcessInit(driver, vm, mig->cpu, VIR_ASYNC_JOB_MIGRATION_IN, - true, startFlags) < 0) - goto stopjob; - stopProcess = true; - - if (!(incoming = qemuMigrationDstPrepare(vm, tunnel, protocol, - listenAddress, port, - dataFD[0]))) - goto stopjob; - - if (qemuProcessPrepareDomain(driver, vm, startFlags) < 0) - goto stopjob; - - if (qemuProcessPrepareHost(driver, vm, startFlags) < 0) - goto stopjob; - - rv = qemuProcessLaunch(dconn, driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, - incoming, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_START, - startFlags); - if (rv < 0) { - if (rv == -2) - relabel = true; - goto stopjob; - } - relabel = true; - - if (tunnel) { - if (virFDStreamOpen(st, dataFD[1]) < 0) { - virReportSystemError(errno, "%s", - _("cannot pass pipe for tunnelled migration")); - goto stopjob; - } - dataFD[1] = -1; /* 'st' owns the FD now & will close it */ - } - - if (STREQ_NULLABLE(protocol, "rdma") && - vm->def->mem.hard_limit > 0 && - virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit << 10) < 0) { - goto stopjob; - } - - if (qemuMigrationDstPrepareAnyBlockDirtyBitmaps(vm, mig, migParams, flags) < 0) - goto stopjob; - - if (qemuMigrationParamsCheck(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, - migParams, mig->caps->automatic) < 0) - goto stopjob; - - /* Migrations using TLS need to add the "tls-creds-x509" object and - * set the migration TLS parameters */ - if (flags & VIR_MIGRATE_TLS) { - if (qemuMigrationParamsEnableTLS(driver, vm, true, - VIR_ASYNC_JOB_MIGRATION_IN, - &tlsAlias, NULL, - migParams) < 0) - goto stopjob; - } else { - if (qemuMigrationParamsDisableTLS(vm, migParams) < 0) - goto stopjob; - } - - if (qemuMigrationParamsApply(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, - migParams) < 0) - goto stopjob; - - if (mig->nbd && - flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) { - const char *nbdTLSAlias = NULL; - - if (flags & VIR_MIGRATE_TLS) { - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_TLS)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("QEMU NBD server does not support TLS transport")); - goto stopjob; - } - - nbdTLSAlias = tlsAlias; - } - - if (qemuMigrationDstStartNBDServer(driver, vm, incoming->address, - nmigrate_disks, migrate_disks, - nbdPort, nbdURI, - nbdTLSAlias) < 0) { + if (!(flags & VIR_MIGRATE_OFFLINE)) { + if (qemuMigrationDstPrepareActive(driver, vm, dconn, mig, st, + protocol, port, listenAddress, + nmigrate_disks, migrate_disks, + nbdPort, nbdURI, + migParams, flags) < 0) { goto stopjob; } - cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; - } - if (mig->lockState) { - VIR_DEBUG("Received lockstate %s", mig->lockState); - VIR_FREE(priv->lockState); - priv->lockState = g_steal_pointer(&mig->lockState); - } else { - VIR_DEBUG("Received no lockstate"); + if (mig->nbd && + flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD; } - if (qemuMigrationDstRun(driver, vm, incoming->uri, - VIR_ASYNC_JOB_MIGRATION_IN) < 0) - goto stopjob; - - if (qemuProcessFinishStartup(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, - false, VIR_DOMAIN_PAUSED_MIGRATION) < 0) - goto stopjob; - - done: if (qemuMigrationCookieFormat(mig, driver, vm, QEMU_MIGRATION_DESTINATION, cookieout, cookieoutlen, cookieFlags) < 0) { @@ -3397,13 +3464,6 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, qemuDomainCleanupAdd(vm, qemuMigrationDstPrepareCleanup); - if (!(flags & VIR_MIGRATE_OFFLINE)) { - virDomainAuditStart(vm, "migrated", true); - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_MIGRATED); - } - /* We keep the job active across API calls until the finish() call. * This prevents any other APIs being invoked while incoming * migration is taking place. @@ -3421,41 +3481,19 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, cleanup: virErrorPreserveLast(&origErr); - qemuProcessIncomingDefFree(incoming); - VIR_FORCE_CLOSE(dataFD[0]); - VIR_FORCE_CLOSE(dataFD[1]); if (ret < 0 && priv) { /* priv is set right after vm is added to the list of domains * and there is no 'goto cleanup;' in the middle of those */ VIR_FREE(priv->origname); - /* release if port is auto selected which is not the case if - * it is given in parameters - */ - if (nbdPort == 0) - virPortAllocatorRelease(priv->nbdPort); - priv->nbdPort = 0; virDomainObjRemoveTransientDef(vm); qemuDomainRemoveInactive(driver, vm); } virDomainObjEndAPI(&vm); - virObjectEventStateQueue(driver->domainEventState, event); qemuMigrationCookieFree(mig); virErrorRestore(&origErr); return ret; stopjob: - qemuMigrationParamsReset(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, - jobPriv->migParams, priv->job.apiFlags); - - if (stopProcess) { - unsigned int stopFlags = VIR_QEMU_PROCESS_STOP_MIGRATED; - if (!relabel) - stopFlags |= VIR_QEMU_PROCESS_STOP_NO_RELABEL; - virDomainAuditStart(vm, "migrated", false); - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, - VIR_ASYNC_JOB_MIGRATION_IN, stopFlags); - } - qemuMigrationJobFinish(vm); goto cleanup; } -- 2.35.1

On Tue, May 10, 2022 at 05:21:22PM +0200, Jiri Denemark wrote: [...] No need for this check because the function call is guarded by the same check. [...] [...]

On Tue, May 10, 2022 at 17:21:23 +0200, Jiri Denemark wrote: [...] I'm not sure how much we care, but qemuDomainRemoveInactive has theoretical possibility to overwrite the error in at least the snapshot/checkpoint removal code and also via qemuExtDevicesCleanupHost. If you add justification to the commit message outlining why it's okay not to preserve the error or drop that part: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, May 10, 2022 at 17:21:27 +0200, Jiri Denemark wrote: What was the point of moving these to a separate section and then moving them back, where they were before? Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

Non-postcopy case talks to QEMU monitor and thus needs to create a nested job. Since qemuMigrationAnyConnectionClosed is called in case there's no thread processing a migration API, we need to make the current thread a temporary owner of the migration job to avoid "This thread doesn't seem to be the async job owner: 0". This is done by starting a migration phase. While no monitor interaction happens in postcopy case and just setting the phase (to indicate a broken postcopy migration) would be enough, being consistent and setting the owner does not hurt anything. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c1a60c90ef..2e9235e1d5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2345,6 +2345,7 @@ qemuMigrationAnyConnectionClosed(virDomainObj *vm, qemuDomainObjPrivate *priv = vm->privateData; qemuDomainJobPrivate *jobPriv = priv->job.privateData; bool postcopy = false; + int phase; VIR_DEBUG("vm=%s, conn=%p, asyncJob=%s, phase=%s", vm->def->name, conn, @@ -2404,12 +2405,17 @@ qemuMigrationAnyConnectionClosed(virDomainObj *vm, return; } + if (postcopy) + phase = QEMU_MIGRATION_PHASE_POSTCOPY_FAILED; + else + phase = QEMU_MIGRATION_PHASE_CONFIRM3_CANCELLED; + ignore_value(qemuMigrationJobStartPhase(vm, phase)); + if (postcopy) { if (priv->job.asyncJob == VIR_ASYNC_JOB_MIGRATION_OUT) qemuMigrationSrcPostcopyFailed(vm); else qemuMigrationDstPostcopyFailed(vm); - ignore_value(qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_POSTCOPY_FAILED)); qemuDomainCleanupAdd(vm, qemuProcessCleanupMigrationJob); qemuMigrationJobContinue(vm); } else { -- 2.35.1

The QEMU process is already running, all we need to do is to call migrate-recover QMP command. Except for some checks and cookie handling, of course. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 99 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2e9235e1d5..a8481f7515 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3509,6 +3509,98 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, } +static int +qemuMigrationDstPrepareResume(virQEMUDriver *driver, + virConnectPtr conn, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + virDomainDef *def, + const char *origname, + const char *protocol, + unsigned short port, + const char *listenAddress, + unsigned long flags) +{ + g_autoptr(qemuMigrationCookie) mig = NULL; + qemuProcessIncomingDef *incoming = NULL; + qemuDomainObjPrivate *priv; + virDomainJobStatus status; + virDomainObj *vm; + int ret = -1; + + VIR_DEBUG("name=%s, origname=%s, protocol=%s, port=%hu, " + "listenAddress=%s, flags=0x%lx", + def->name, NULLSTR(origname), protocol, port, + NULLSTR(listenAddress), flags); + + vm = virDomainObjListFindByName(driver->domains, def->name); + if (!vm) { + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching name '%s'"), def->name); + qemuMigrationDstErrorReport(driver, def->name); + return -1; + } + priv = vm->privateData; + + if (!qemuMigrationAnyCanResume(vm, VIR_ASYNC_JOB_MIGRATION_IN, flags, + QEMU_MIGRATION_PHASE_POSTCOPY_FAILED)) + goto cleanup; + + if (qemuMigrationJobStartPhase(vm, QEMU_MIGRATION_PHASE_PREPARE_RESUME) < 0) + goto cleanup; + + qemuDomainCleanupRemove(vm, qemuProcessCleanupMigrationJob); + + if (qemuMigrationAnyRefreshStatus(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN, + &status) < 0) + goto cleanup; + + if (status != VIR_DOMAIN_JOB_STATUS_POSTCOPY_PAUSED) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("QEMU reports migration is still running")); + goto cleanup; + } + + if (!(mig = qemuMigrationCookieParse(driver, def, origname, NULL, + cookiein, cookieinlen, + QEMU_MIGRATION_COOKIE_CAPS))) + goto cleanup; + + if (!(incoming = qemuMigrationDstPrepare(vm, false, protocol, + listenAddress, port, -1))) + goto cleanup; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, VIR_ASYNC_JOB_MIGRATION_IN) < 0) + goto cleanup; + + ret = qemuMonitorMigrateRecover(priv->mon, incoming->uri); + qemuDomainObjExitMonitor(vm); + + if (ret < 0) + goto cleanup; + + if (qemuMigrationCookieFormat(mig, driver, vm, + QEMU_MIGRATION_DESTINATION, + cookieout, cookieoutlen, + QEMU_MIGRATION_COOKIE_CAPS) < 0) + VIR_WARN("Unable to encode migration cookie"); + + virCloseCallbacksSet(driver->closeCallbacks, vm, conn, + qemuMigrationAnyConnectionClosed); + + cleanup: + qemuProcessIncomingDefFree(incoming); + if (ret < 0) + ignore_value(qemuMigrationJobSetPhase(vm, QEMU_MIGRATION_PHASE_POSTCOPY_FAILED)); + qemuDomainCleanupAdd(vm, qemuProcessCleanupMigrationJob); + qemuMigrationJobContinue(vm); + virDomainObjEndAPI(&vm); + return ret; +} + + static int qemuMigrationDstPrepareAny(virQEMUDriver *driver, virConnectPtr dconn, @@ -3577,6 +3669,13 @@ qemuMigrationDstPrepareAny(virQEMUDriver *driver, return -1; } + if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { + return qemuMigrationDstPrepareResume(driver, dconn, cookiein, cookieinlen, + cookieout, cookieoutlen, + *def, origname, protocol, port, + listenAddress, flags); + } + return qemuMigrationDstPrepareFresh(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, -- 2.35.1

On Tue, May 10, 2022 at 17:21:29 +0200, Jiri Denemark wrote: [...] Is there any reason not to allow adopting a running unattended migration with a recovery API?

Everything was already done in the normal Finish phase and vCPUs are running. We just need to wait for all remaining data to be transferred. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 46 ++++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a8481f7515..430dfb1abb 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -6600,6 +6600,22 @@ qemuMigrationDstFinishFresh(virQEMUDriver *driver, } +static int +qemuMigrationDstFinishResume(virQEMUDriver *driver, + virDomainObj *vm) +{ + VIR_DEBUG("vm=%p", vm); + + if (qemuMigrationDstWaitForCompletion(driver, vm, + VIR_ASYNC_JOB_MIGRATION_IN, + false) < 0) { + return -1; + } + + return 0; +} + + static virDomainPtr qemuMigrationDstFinishActive(virQEMUDriver *driver, virConnectPtr dconn, @@ -6647,8 +6663,14 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, goto error; } - rc = qemuMigrationDstFinishFresh(driver, vm, mig, flags, v3proto, - timeReceived, &doKill, &inPostCopy); + if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { + rc = qemuMigrationDstFinishResume(driver, vm); + inPostCopy = true; + } else { + rc = qemuMigrationDstFinishFresh(driver, vm, mig, flags, v3proto, + timeReceived, &doKill, &inPostCopy); + } + if (rc < 0 || !(dom = virGetDomain(dconn, vm->def->name, vm->def->uuid, vm->def->id))) goto error; @@ -6719,6 +6741,8 @@ qemuMigrationDstFinish(virQEMUDriver *driver, qemuDomainObjPrivate *priv = vm->privateData; unsigned short port; unsigned long long timeReceived = 0; + int phase = v3proto ? QEMU_MIGRATION_PHASE_FINISH3 + : QEMU_MIGRATION_PHASE_FINISH2; VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=0x%lx, retcode=%d", @@ -6733,14 +6757,24 @@ qemuMigrationDstFinish(virQEMUDriver *driver, goto cleanup; } + if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { + if (!qemuMigrationAnyCanResume(vm, VIR_ASYNC_JOB_MIGRATION_IN, flags, + QEMU_MIGRATION_PHASE_PREPARE_RESUME)) + goto cleanup; + phase = QEMU_MIGRATION_PHASE_FINISH_RESUME; + } ignore_value(virTimeMillisNow(&timeReceived)); - if (qemuMigrationJobStartPhase(vm, - v3proto ? QEMU_MIGRATION_PHASE_FINISH3 - : QEMU_MIGRATION_PHASE_FINISH2) < 0) + if (qemuMigrationJobStartPhase(vm, phase) < 0) goto cleanup; - qemuDomainCleanupRemove(vm, qemuMigrationDstPrepareCleanup); + if (flags & VIR_MIGRATE_POSTCOPY_RESUME) { + virCloseCallbacksUnset(driver->closeCallbacks, vm, + qemuMigrationAnyConnectionClosed); + qemuDomainCleanupRemove(vm, qemuProcessCleanupMigrationJob); + } else { + qemuDomainCleanupRemove(vm, qemuMigrationDstPrepareCleanup); + } g_clear_pointer(&priv->job.completed, virDomainJobDataFree); cookie_flags = QEMU_MIGRATION_COOKIE_NETWORK | -- 2.35.1

On Thu, May 12, 2022 at 17:20:34 +0200, Peter Krempa wrote: I think event processing in mgmt apps is inevitable anyway as the migration may finish before an app even tries to recover the migration in case only libvirt side of migration was broken. And it may still be broken to make recovery impossible while migration is progressing just fine (esp. if migration streams use separate network). Even plain post-copy which never fails requires event processing since the management has to explicitly switch migration to post-copy mode. Also events are often the only way to get statistics about the completed migration. That said, I think it would be nice to have this functionality anyway so I'll try to look at it as a follow up series. Jirka

Since all parts of post-copy recovery have been implemented now, it's time to enable the corresponding flag. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_migration.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 7eb0d4fe02..fbc0549b34 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -60,6 +60,7 @@ VIR_MIGRATE_TLS | \ VIR_MIGRATE_PARALLEL | \ VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES | \ + VIR_MIGRATE_POSTCOPY_RESUME | \ 0) /* All supported migration parameters and their types. */ -- 2.35.1

The original virDomainAbortJob did not support flags. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- include/libvirt/libvirt-domain.h | 3 +++ src/driver-hypervisor.h | 5 ++++ src/libvirt-domain.c | 45 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 14 +++++++++- src/remote_protocol-structs | 5 ++++ 7 files changed, 73 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index aee2b49114..af4406cb61 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -4099,6 +4099,9 @@ int virDomainGetJobStats(virDomainPtr domain, unsigned int flags); int virDomainAbortJob(virDomainPtr dom); +int virDomainAbortJobFlags(virDomainPtr dom, + unsigned int flags); + /** * virDomainJobOperation: * diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 69516e8fea..016d5cec7c 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -746,6 +746,10 @@ typedef int typedef int (*virDrvDomainAbortJob)(virDomainPtr domain); +typedef int +(*virDrvDomainAbortJobFlags)(virDomainPtr domain, + unsigned int flags); + typedef int (*virDrvDomainMigrateGetMaxDowntime)(virDomainPtr domain, unsigned long long *downtime, @@ -1590,6 +1594,7 @@ struct _virHypervisorDriver { virDrvDomainGetJobInfo domainGetJobInfo; virDrvDomainGetJobStats domainGetJobStats; virDrvDomainAbortJob domainAbortJob; + virDrvDomainAbortJobFlags domainAbortJobFlags; virDrvDomainMigrateGetMaxDowntime domainMigrateGetMaxDowntime; virDrvDomainMigrateSetMaxDowntime domainMigrateSetMaxDowntime; virDrvDomainMigrateGetCompressionCache domainMigrateGetCompressionCache; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 07fb8119d9..1e1b378361 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -9459,6 +9459,51 @@ virDomainAbortJob(virDomainPtr domain) } +/** + * virDomainAbortJobFlags: + * @domain: a domain object + * @flags: extra flags; not used yet, callers should always pass 0 + * + * Requests that the current background job be aborted at the + * soonest opportunity. In case the job is a migration in a post-copy mode, + * this function will report an error (see virDomainMigrateStartPostCopy for + * more details). + * + * Returns 0 in case of success and -1 in case of failure. + * + * Since: 8.4.0 + */ +int +virDomainAbortJobFlags(virDomainPtr domain, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + conn = domain->conn; + + virCheckReadOnlyGoto(conn->flags, error); + + if (conn->driver->domainAbortJobFlags) { + int ret; + ret = conn->driver->domainAbortJobFlags(domain, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(conn); + return -1; +} + + /** * virDomainMigrateSetMaxDowntime: * @domain: a domain object diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 9f58b52924..c68bbf3680 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -918,6 +918,7 @@ LIBVIRT_8.0.0 { LIBVIRT_8.4.0 { global: + virDomainAbortJobFlags; virDomainSaveParams; virDomainRestoreParams; } LIBVIRT_8.0.0; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 423f5f9fb9..5ad517a387 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8534,6 +8534,7 @@ static virHypervisorDriver hypervisor_driver = { .domainGetJobInfo = remoteDomainGetJobInfo, /* 0.7.7 */ .domainGetJobStats = remoteDomainGetJobStats, /* 1.0.3 */ .domainAbortJob = remoteDomainAbortJob, /* 0.7.7 */ + .domainAbortJobFlags = remoteDomainAbortJobFlags, /* 8.2.0 */ .domainMigrateGetMaxDowntime = remoteDomainMigrateGetMaxDowntime, /* 3.7.0 */ .domainMigrateSetMaxDowntime = remoteDomainMigrateSetMaxDowntime, /* 0.8.0 */ .domainMigrateGetCompressionCache = remoteDomainMigrateGetCompressionCache, /* 1.0.3 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 085631c11b..79ffc63f03 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2492,6 +2492,12 @@ struct remote_domain_abort_job_args { }; +struct remote_domain_abort_job_flags_args { + remote_nonnull_domain dom; + unsigned int flags; +}; + + struct remote_domain_migrate_get_max_downtime_args { remote_nonnull_domain dom; unsigned int flags; @@ -6947,5 +6953,11 @@ enum remote_procedure { * @acl: domain:start * @acl: domain:write */ - REMOTE_PROC_DOMAIN_RESTORE_PARAMS = 441 + REMOTE_PROC_DOMAIN_RESTORE_PARAMS = 441, + + /** + * @generate: both + * @acl: domain:write + */ + REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 4ffdce5679..ca5222439d 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1869,6 +1869,10 @@ struct remote_domain_get_job_stats_ret { struct remote_domain_abort_job_args { remote_nonnull_domain dom; }; +struct remote_domain_abort_job_flags_args { + remote_nonnull_domain dom; + u_int flags; +}; struct remote_domain_migrate_get_max_downtime_args { remote_nonnull_domain dom; u_int flags; @@ -3706,4 +3710,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_LAUNCH_SECURITY_STATE = 439, REMOTE_PROC_DOMAIN_SAVE_PARAMS = 440, REMOTE_PROC_DOMAIN_RESTORE_PARAMS = 441, + REMOTE_PROC_DOMAIN_ABORT_JOB_FLAGS = 442, }; -- 2.35.1

Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e268d3a478..efa6cbe14d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12770,7 +12770,9 @@ qemuDomainAbortJobMigration(virDomainObj *vm) } -static int qemuDomainAbortJob(virDomainPtr dom) +static int +qemuDomainAbortJobFlags(virDomainPtr dom, + unsigned int flags) { virQEMUDriver *driver = dom->conn->privateData; virDomainObj *vm; @@ -12778,10 +12780,12 @@ static int qemuDomainAbortJob(virDomainPtr dom) qemuDomainObjPrivate *priv; int reason; + virCheckFlags(0, -1); + if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; - if (virDomainAbortJobEnsureACL(dom->conn, vm->def) < 0) + if (virDomainAbortJobFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; if (qemuDomainObjBeginJob(driver, vm, VIR_JOB_ABORT) < 0) @@ -12860,6 +12864,13 @@ static int qemuDomainAbortJob(virDomainPtr dom) } +static int +qemuDomainAbortJob(virDomainPtr dom) +{ + return qemuDomainAbortJobFlags(dom, 0); +} + + static int qemuDomainMigrateSetMaxDowntime(virDomainPtr dom, unsigned long long downtime, @@ -21018,6 +21029,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetJobInfo = qemuDomainGetJobInfo, /* 0.7.7 */ .domainGetJobStats = qemuDomainGetJobStats, /* 1.0.3 */ .domainAbortJob = qemuDomainAbortJob, /* 0.7.7 */ + .domainAbortJobFlags = qemuDomainAbortJobFlags, /* 8.4.0 */ .domainMigrateGetMaxDowntime = qemuDomainMigrateGetMaxDowntime, /* 3.7.0 */ .domainMigrateSetMaxDowntime = qemuDomainMigrateSetMaxDowntime, /* 0.8.0 */ .domainMigrateGetCompressionCache = qemuDomainMigrateGetCompressionCache, /* 1.0.3 */ -- 2.35.1

Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- include/libvirt/libvirt-domain.h | 16 ++++++++++++++++ src/libvirt-domain.c | 10 ++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index af4406cb61..bcd377b74f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -4099,6 +4099,22 @@ int virDomainGetJobStats(virDomainPtr domain, unsigned int flags); int virDomainAbortJob(virDomainPtr dom); +/** + * virDomainAbortJobFlagsValues: + * + * Flags OR'ed together to provide specific behavior when aborting a domain job. + * + * Since: 8.4.0 + */ +typedef enum { + /* Interrupt post-copy migration. Since migration in a post-copy phase + * cannot be aborted without losing the domain (none of the hosts involved + * in migration has a complete state of the domain), the migration will be + * suspended and it can later be resumed using virDomainMigrate* APIs with + * VIR_MIGRATE_POSTCOPY_RESUME flag. (Since: 8.4.0) */ + VIR_DOMAIN_ABORT_JOB_POSTCOPY = 1 << 0, +} virDomainAbortJobFlagsValues; + int virDomainAbortJobFlags(virDomainPtr dom, unsigned int flags); diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 1e1b378361..5f0f60781a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -9462,12 +9462,12 @@ virDomainAbortJob(virDomainPtr domain) /** * virDomainAbortJobFlags: * @domain: a domain object - * @flags: extra flags; not used yet, callers should always pass 0 + * @flags: bitwise-OR of virDomainAbortJobFlagsValues * * Requests that the current background job be aborted at the * soonest opportunity. In case the job is a migration in a post-copy mode, - * this function will report an error (see virDomainMigrateStartPostCopy for - * more details). + * this function will report an error unless VIR_DOMAIN_ABORT_JOB_POSTCOPY + * flag is used (see virDomainMigrateStartPostCopy for more details). * * Returns 0 in case of success and -1 in case of failure. * @@ -9807,7 +9807,9 @@ virDomainMigrateGetMaxSpeed(virDomainPtr domain, * running, individual tasks in the guest may be blocked waiting for a page that * has not been migrated yet. It's up to the upper layer to decide what to do * in such case. Because of this, libvirt will refuse to cancel post-copy - * migration via virDomainAbortJob. + * migration via virDomainAbortJobFlags unless it is called with + * VIR_DOMAIN_ABORT_JOB_POSTCOPY, in which case the post-copy migration will be + * paused. * * Failed post-copy migration can be recovered once the cause for the failure * (e.g., a network issue) is resolved by repeating the migration with an -- 2.35.1

Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 43 +++++++++++++++++++++++++++++------- src/qemu/qemu_monitor.c | 9 ++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 22 ++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 +++ 5 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index efa6cbe14d..344bbe2196 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12770,6 +12770,36 @@ qemuDomainAbortJobMigration(virDomainObj *vm) } +static int +qemuDomainAbortJobPostcopy(virDomainObj *vm, + unsigned int flags) +{ + qemuDomainObjPrivate *priv = vm->privateData; + int rc; + + if (!(flags & VIR_DOMAIN_ABORT_JOB_POSTCOPY)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot abort migration in post-copy mode")); + return -1; + } + + VIR_DEBUG("Suspending post-copy migration at client request"); + + qemuDomainObjAbortAsyncJob(vm); + qemuDomainObjEnterMonitor(priv->driver, vm); + rc = qemuMonitorMigratePause(priv->mon); + qemuDomainObjExitMonitor(vm); + + if (rc == -2) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot suspend post-copy migration")); + return -1; + } + + return rc; +} + + static int qemuDomainAbortJobFlags(virDomainPtr dom, unsigned int flags) @@ -12780,7 +12810,7 @@ qemuDomainAbortJobFlags(virDomainPtr dom, qemuDomainObjPrivate *priv; int reason; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_ABORT_JOB_POSTCOPY, -1); if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -12817,13 +12847,10 @@ qemuDomainAbortJobFlags(virDomainPtr dom, case VIR_ASYNC_JOB_MIGRATION_OUT: if ((priv->job.current->status == VIR_DOMAIN_JOB_STATUS_POSTCOPY || (virDomainObjGetState(vm, &reason) == VIR_DOMAIN_PAUSED && - reason == VIR_DOMAIN_PAUSED_POSTCOPY))) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot abort migration in post-copy mode")); - goto endjob; - } - - ret = qemuDomainAbortJobMigration(vm); + reason == VIR_DOMAIN_PAUSED_POSTCOPY))) + ret = qemuDomainAbortJobPostcopy(vm, flags); + else + ret = qemuDomainAbortJobMigration(vm); break; case VIR_ASYNC_JOB_SAVE: diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5424fdc4fd..af37ce7d19 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2429,6 +2429,15 @@ qemuMonitorMigrateCancel(qemuMonitor *mon) } +int +qemuMonitorMigratePause(qemuMonitor *mon) +{ + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONMigratePause(mon); +} + + int qemuMonitorQueryDump(qemuMonitor *mon, qemuMonitorDumpStats *stats) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 812f8f4dc2..e36ce8b4eb 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -927,6 +927,9 @@ int qemuMonitorMigrateToSocket(qemuMonitor *mon, int qemuMonitorMigrateCancel(qemuMonitor *mon); +int +qemuMonitorMigratePause(qemuMonitor *mon); + int qemuMonitorGetDumpGuestMemoryCapability(qemuMonitor *mon, const char *capability); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 44361ae003..66e0b870b4 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3458,6 +3458,28 @@ int qemuMonitorJSONMigrateCancel(qemuMonitor *mon) } +int +qemuMonitorJSONMigratePause(qemuMonitor *mon) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("migrate-pause", NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) + return -2; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +} + + /* qemuMonitorJSONQueryDump: * @mon: Monitor pointer * @stats: Monitor dump stats diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index a60c0bfac4..20c3f63f93 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -208,6 +208,9 @@ qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitor *mon, int qemuMonitorJSONMigrateCancel(qemuMonitor *mon); +int +qemuMonitorJSONMigratePause(qemuMonitor *mon); + int qemuMonitorJSONQueryDump(qemuMonitor *mon, qemuMonitorDumpStats *stats); -- 2.35.1

On Tue, May 10, 2022 at 17:21:41 +0200, Jiri Denemark wrote: ``VIR_MIGRATE_POSTCOPY_RESUME`` and ``virsh migrate --postcopy-resume`` s/fecovering/recovering/ Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;