8:59 a.m.
On Wed, May 18, 2022 at 15:40:18 +0200, Jiri Denemark wrote:
On Thu, May 12, 2022 at 14:14:15 +0200, Peter Krempa wrote:
> On Tue, May 10, 2022 at 17:21:15 +0200, Jiri Denemark wrote:
> > Mostly we just need to check whether the domain is in a failed post-copy
> > migration that can be resumed.
> >
> > Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
> > ---
> > src/qemu/qemu_migration.c | 143 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 143 insertions(+)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index c111dd8686..99b1d4b88b 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -2717,6 +2717,143 @@ qemuMigrationSrcBeginPhase(virQEMUDriver *driver,
> > flags);
> > }
> >
> > +
> > +static bool
> > +qemuMigrationAnyCanResume(virDomainObj *vm,
> > + virDomainAsyncJob job,
> > + unsigned long flags,
> > + qemuMigrationJobPhase expectedPhase)
> > +{
> > + qemuDomainObjPrivate *priv = vm->privateData;
> > +
> > + VIR_DEBUG("vm=%p, job=%s, flags=0x%lx, expectedPhase=%s",
> > + vm, virDomainAsyncJobTypeToString(job), flags,
> > + qemuDomainAsyncJobPhaseToString(VIR_ASYNC_JOB_MIGRATION_OUT,
> > + expectedPhase));
> > +
> > + if (!(flags & VIR_MIGRATE_POSTCOPY)) {
> > + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> > + _("resuming failed post-copy migration requires
"
> > + "post-copy to be enabled"));
>
> 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.
Right, the previous migration must have been started as post-copy, but
resuming it is post-copy too. We could have implied the flag based on
POSTCOPY_RESUME (which is ugly) or all relevant places checking POSTCOPY
would need to check POSTCOPY_RESUME too (which is ugly as well). And
with the expected use case (repeating the failed migration command with
all the flags and arguments and adding an extra POSTCOPY_RESUME flag) in
mind requiring POSTCOPY to be used with POSTCOPY_RESUME looks like the
the best option to me.
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.
>
> > + return false;
> > + }
> > +
> > + /* This should never happen since POSTCOPY_RESUME is newer than
> > + * CHANGE_PROTECTION, but let's check it anyway in case we're
talking to
> > + * a weired client.
> > + */
> > + if (job == VIR_ASYNC_JOB_MIGRATION_OUT &&
> > + expectedPhase < QEMU_MIGRATION_PHASE_PERFORM_RESUME &&
> > + !(flags & VIR_MIGRATE_CHANGE_PROTECTION)) {
> > + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> > + _("resuming failed post-copy migration requires
"
> > + "change protection"));
>
> Same here.
>
> > + return NULL;
> > + }
>
>
> [...]
>
> > +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 NULL;
> > + }
> > +
> > + 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.
Which is what happens in qemuMigrationSrcBegin:
if (flags & VIR_MIGRATE_POSTCOPY_RESUME) {
xml = qemuMigrationSrcBeginResumePhase(...);
goto cleanup;
}
...
Or did I miss anything?
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.