Re: [libvirt] [PATCH] qemu: Allow migration to be cancelled at any phase

On Wed, Nov 07, 2012 at 12:03:39 +0100, Michal Privoznik wrote: > Currently, if user calls virDomainAbortJob we just issue > 'migrate_cancel' and hope for the best. However, if user calls > the API in wrong phase when migration hasn't been started yet > (perform phase) the cancel request is just ignored. With this > patch, the request is remembered and as soon as perform phase > starts, migration is cancelled. > --- > src/qemu/qemu_domain.c | 26 ++++++++++++++++++++++++++ > src/qemu/qemu_domain.h | 4 ++++ > src/qemu/qemu_driver.c | 31 +++++++++++++++++++++++++++---- > src/qemu/qemu_migration.c | 43 +++++++++++++++++++++++++++++++++++++++++-- > 4 files changed, 98 insertions(+), 6 deletions(-) > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index a5592b9..031be5f 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -160,6 +160,7 @@ qemuDomainObjResetAsyncJob(qemuDomainObjPrivatePtr priv) > job->mask = DEFAULT_JOB_MASK; > job->start = 0; > job->dump_memory_only = false; > + job->asyncAbort = false; > memset(&job->info, 0, sizeof(job->info)); > } > > @@ -959,6 +960,31 @@ qemuDomainObjEndAsyncJob(struct qemud_driver *driver, virDomainObjPtr obj) > return virObjectUnref(obj); > } > > +void > +qemuDomainObjAbortAsyncJob(virDomainObjPtr obj) > +{ > + qemuDomainObjPrivatePtr priv = obj->privateData; > + > + VIR_DEBUG("Requesting abort of async job: %s", > + qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); > + > + priv->job.asyncAbort = true; > +} > + > +/** > + * qemuDomainObjAbortAsyncJobRequested: > + * @obj: domain object > + * > + * Was abort requested? @obj MUST be locked when calling this. > + */ > +bool > +qemuDomainObjAbortAsyncJobRequested(virDomainObjPtr obj) > +{ > + qemuDomainObjPrivatePtr priv = obj->privateData; > + > + return priv->job.asyncAbort; > +} > + I don't think we need this qemuDomainObjAbortAsyncJobRequested function. It's much easier and shorter if the caller just checks priv->job.asyncAbort directly. The current code uses functions only for changing the values or more complicated reads. > static int > qemuDomainObjEnterMonitorInternal(struct qemud_driver *driver, > bool driver_locked, > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h > index 9c2f67c..9a31bbe 100644 > --- a/src/qemu/qemu_domain.h > +++ b/src/qemu/qemu_domain.h > @@ -111,6 +111,7 @@ struct qemuDomainJobObj { > unsigned long long start; /* When the async job started */ > bool dump_memory_only; /* use dump-guest-memory to do dump */ > virDomainJobInfo info; /* Async job progress data */ > + bool asyncAbort; /* abort of async job requested */ > }; > > typedef struct _qemuDomainPCIAddressSet qemuDomainPCIAddressSet; > @@ -204,6 +205,9 @@ bool qemuDomainObjEndJob(struct qemud_driver *driver, > bool qemuDomainObjEndAsyncJob(struct qemud_driver *driver, > virDomainObjPtr obj) > ATTRIBUTE_RETURN_CHECK; > +void qemuDomainObjAbortAsyncJob(virDomainObjPtr obj); > +bool qemuDomainObjAbortAsyncJobRequested(virDomainObjPtr obj); > + > void qemuDomainObjSetJobPhase(struct qemud_driver *driver, > virDomainObjPtr obj, > int phase); > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 7b8eec6..009c2c8 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -10331,6 +10331,8 @@ static int qemuDomainAbortJob(virDomainPtr dom) { > virDomainObjPtr vm; > int ret = -1; > qemuDomainObjPrivatePtr priv; > + /* Poll every 50ms for job termination */ > + struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull }; > > qemuDriverLock(driver); > vm = virDomainFindByUUID(&driver->domains, dom->uuid); > @@ -10365,10 +10367,31 @@ static int qemuDomainAbortJob(virDomainPtr dom) { > goto endjob; > } > > - VIR_DEBUG("Cancelling job at client request"); > - qemuDomainObjEnterMonitor(driver, vm); > - ret = qemuMonitorMigrateCancel(priv->mon); > - qemuDomainObjExitMonitor(driver, vm); > + qemuDomainObjAbortAsyncJob(vm); > + VIR_DEBUG("Waiting for async job '%s' to finish", > + qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); > + while (priv->job.asyncJob) { > + if (qemuDomainObjEndJob(driver, vm) == 0) { > + vm = NULL; > + goto cleanup; > + } > + virDomainObjUnlock(vm); > + > + nanosleep(&ts, NULL); > + > + virDomainObjLock(vm); > + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_ABORT) < 0) > + goto cleanup; > + > + if (!virDomainObjIsActive(vm)) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + "%s", _("domain is not running")); > + goto endjob; > + } > + > + } > + > + ret = 0; > > endjob: > if (qemuDomainObjEndJob(driver, vm) == 0) AbortJob API was never a synchronous one and I don't think we need to change it. Not to mention that this code unlocks and locks vm without holding an extra reference to it. And even if it did so, it cannot guarantee that the job it's checking in the loop is the same one it tried to cancel. It's quite unlikely but the original job could have finished and another one could have been started while this code was sleeping.

In other words, I'd just change to do: VIR_DEBUG("Cancelling job at client request"); + qemuDomainObjAbortAsyncJob(vm); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorMigrateCancel(priv->mon); qemuDomainObjExitMonitor(driver, vm); > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index 5f8a9c5..c840686 100644 > --- a/src/qemu/qemu_migration.c > +++ b/src/qemu/qemu_migration.c > @@ -1172,6 +1172,12 @@ qemuMigrationUpdateJobStatus(struct qemud_driver *driver, > } > priv->job.info.timeElapsed -= priv->job.start; > > + if (qemuDomainObjAbortAsyncJobRequested(vm)) { > + VIR_DEBUG("Migration abort requested. Translating " > + "status to MIGRATION_STATUS_CANCELLED"); > + status = QEMU_MONITOR_MIGRATION_STATUS_CANCELLED; > + } > + This check seems to be to late and that complicates the code later. If we keep qemuDomainAbortJob calling qemuMonitorMigrateCancel in qemuDomainAbortJob, we may count on that and check priv.job.asyncAbort before entering the monitor to tell QEMU to start migrating in qemuMigrationRun. If the abort is not requested by then, it may only happen after we call qemuMonitorMigrateTo* and in that case the migration will be properly aborted by qemuMonitorMigrateCancel.

> ret = -1; > switch (status) { > case QEMU_MONITOR_MIGRATION_STATUS_INACTIVE: > @@ -1214,6 +1220,35 @@ qemuMigrationUpdateJobStatus(struct qemud_driver *driver, > return ret; > } > > +static int > +qemuMigrationCancel(struct qemud_driver *driver, virDomainObjPtr vm) > +{ > + qemuDomainObjPrivatePtr priv = vm->privateData; > + int ret = -1; > + > + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_ABORT) < 0) > + goto cleanup; > + > + if (!virDomainObjIsActive(vm)) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + "%s", _("domain is not running")); > + goto endjob; > + } > + > + qemuDomainObjEnterMonitor(driver, vm); > + ret = qemuMonitorMigrateCancel(priv->mon); > + qemuDomainObjExitMonitor(driver, vm); > + > +endjob: > + if (qemuDomainObjEndJob(driver, vm) == 0) { > + virReportError(VIR_ERR_OPEN_FAILED, "%s", > + _("domain unexpectedly died")); > + ret = -1; > + } > + > +cleanup: > + return ret; > +} > > static int > qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm, > @@ -1262,10 +1297,14 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm, > } > > cleanup: > - if (priv->job.info.type == VIR_DOMAIN_JOB_COMPLETED) > + if (priv->job.info.type == VIR_DOMAIN_JOB_COMPLETED) { > return 0; > - else > + } else { > + if (priv->job.info.type == VIR_DOMAIN_JOB_CANCELLED && > + qemuMigrationCancel(driver, vm) < 0) > + VIR_DEBUG("Cancelling job at client request"); > return -1; > + } > } This hack is unnecessary when we do what I suggested above. But in any case, this patch does not actually allow the migration to be cancelled at any phase. It just allow the migration to be cancelled during Perform phase or before. And the cancellation would actually happen only during Perform phase thus possibly doing unnecessary Prepare phase in case someone tried to cancel the migration at the very beginning. However, since even such improvement in this area is a nice step forward and this particular case of aborting migration before it gets to Perform phase is the most visible issue, we can address the rest of the issues later. I'm looking forward to a simplified v2 :-) Jirka