Re: [PATCH] qemu: keep pre-migration domain state after failed migration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 06, 2014 at 06:23:43PM +0100, Jiri Denemark wrote:
> On Thu, Feb 06, 2014 at 17:33:14 +0100, Martin Kletzander wrote:
> > Couple of codepaths shared the same code which can be moved out to a
> > function and on one of such places, qemuMigrationConfirmPhase(), the
> > domain was resumed even if it wasn't running before the migration
> > started.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1057407
> >
> > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> > ---
> >  src/qemu/qemu_domain.h    |   3 +-
> >  src/qemu/qemu_migration.c | 117 +++++++++++++++++++++++++---------------------
> >  2 files changed, 66 insertions(+), 54 deletions(-)
> >
> > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> > index 6a92351..84624f9 100644
> > --- a/src/qemu/qemu_domain.h
> > +++ b/src/qemu/qemu_domain.h
> > @@ -1,7 +1,7 @@
> >  /*
> >   * qemu_domain.h: QEMU domain private state
> >   *
> > - * Copyright (C) 2006-2013 Red Hat, Inc.
> > + * Copyright (C) 2006-2014 Red Hat, Inc.
> >   * Copyright (C) 2006 Daniel P. Berrange
> >   *
> >   * This library is free software; you can redistribute it and/or
> > @@ -161,6 +161,7 @@ struct _qemuDomainObjPrivate {
> >      char *origname;
> >      int nbdPort; /* Port used for migration with NBD */
> >      unsigned short migrationPort;
> > +    int preMigrationState;
> >
> >      virChrdevsPtr devs;
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 407fb70..1a29efa 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -1075,6 +1075,53 @@ error:
> >      return NULL;
> >  }
> >
> > +static void
> > +qemuDomainObjStorePreMigrationState(virDomainObjPtr vm)
> > +{
> > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > +    priv->preMigrationState = virDomainObjGetState(vm, NULL);
> > +
> > +    VIR_DEBUG("Storing pre-migration state=%d domain=%p",
> > +              priv->preMigrationState, vm);
> > +}
>
> We are in qemu_migration.c file so this function and the one below
> should use qemuMigration prefix.
>

I've found one exception now, but I'm not basing it on that.  I
changed these to qemuMigration{S,Res}toreDomainState().

> > +
> > +/* Returns true if the domain was resumed, false otherwise */
> > +static bool
> > +qemuDomainObjRestorePreMigrationState(virConnectPtr conn, virDomainObjPtr vm)
> > +{
> > +    virQEMUDriverPtr driver = conn->privateData;
> > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > +    int state = virDomainObjGetState(vm, NULL);
> > +    bool ret = false;
> > +
> > +    VIR_DEBUG("driver=%p, vm=%p, pre-mig-state=%d, state=%d",
> > +              driver, vm, priv->preMigrationState, state);
> > +
> > +    if (state == VIR_DOMAIN_PAUSED &&
> > +        priv->preMigrationState == VIR_DOMAIN_RUNNING) {
> > +        /* This is basically the only restore possibility that's safe
> > +         * and we should attemt to do */
> > +

Changed this to attempt.

> > +        VIR_DEBUG("Restoring pre-migration state due to migration error");
> > +
> > +        /* we got here through some sort of failure; start the domain again */
> > +        if (qemuProcessStartCPUs(driver, vm, conn,
> > +                                 VIR_DOMAIN_RUNNING_MIGRATION_CANCELED,
> > +                                 QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) {
> > +            /* Hm, we already know we are in error here.  We don't want to
> > +             * overwrite the previous error, though, so we just throw something
> > +             * to the logs and hope for the best */
> > +            VIR_ERROR(_("Failed to resume guest %s after failure"), vm->def->name);
> > +            goto cleanup;
> > +        }
> > +        ret = true;
> > +    }
> > +
> > + cleanup:
> > +    priv->preMigrationState = VIR_DOMAIN_NOSTATE;
> > +    return ret;
> > +}
> > +
> >  /**
> >   * qemuMigrationStartNBDServer:
> >   * @driver: qemu driver
> > @@ -1952,8 +1999,8 @@ cleanup:
> >
> >
> >  /* The caller is supposed to lock the vm and start a migration job. */
> > -static char
> > -*qemuMigrationBeginPhase(virQEMUDriverPtr driver,
> > +static char *
> > +qemuMigrationBeginPhase(virQEMUDriverPtr driver,
>
> Heh, it took me a while to see what you did :-) However, the following
> lines are not indented correctly now...
>

It shouldn't be in this patch and there are more cleanups to do, so I
left it out of this patch and will post it with some other cleanups
soon.

> >                           virDomainObjPtr vm,
> >                           const char *xmlin,
> >                           const char *dname,
> > @@ -2075,6 +2122,8 @@ qemuMigrationBegin(virConnectPtr conn,
> >          asyncJob = QEMU_ASYNC_JOB_NONE;
> >      }
> >
> > +    qemuDomainObjStorePreMigrationState(vm);
> > +
> >      if (!virDomainObjIsActive(vm) && !(flags & VIR_MIGRATE_OFFLINE)) {
> >          virReportError(VIR_ERR_OPERATION_INVALID,
> >                         "%s", _("domain is not running"));
> > @@ -2744,22 +2793,12 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver,
> >          /* cancel any outstanding NBD jobs */
> >          qemuMigrationCancelDriveMirror(mig, driver, vm);
> >
> > -        /* 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, conn,
> > -                                 VIR_DOMAIN_RUNNING_MIGRATED,
> > -                                 QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) {
> > -            if (virGetLastError() == NULL)
> > -                virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                               "%s", _("resume operation failed"));
> > -            goto cleanup;
> > +        if (qemuDomainObjRestorePreMigrationState(conn, vm)) {
> > +            event = virDomainEventLifecycleNewFromObj(vm,
> > +                                                      VIR_DOMAIN_EVENT_RESUMED,
> > +                                                      VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> >          }
> >
> > -        event = virDomainEventLifecycleNewFromObj(vm,
> > -                                         VIR_DOMAIN_EVENT_RESUMED,
> > -                                         VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> >          if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) {
> >              VIR_WARN("Failed to save status on vm %s", vm->def->name);
> >              goto cleanup;
> > @@ -4065,7 +4104,6 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
> >  {
> >      virObjectEventPtr event = NULL;
> >      int ret = -1;
> > -    int resume = 0;
> >      virErrorPtr orig_err = NULL;
> >      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> >      bool abort_on_error = !!(flags & VIR_MIGRATE_ABORT_ON_ERROR);
> > @@ -4085,7 +4123,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
> >      if (!(flags & VIR_MIGRATE_UNSAFE) && !qemuMigrationIsSafe(vm->def))
> >          goto endjob;
> >
> > -    resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING;
> > +    qemuDomainObjStorePreMigrationState(vm);
> >
> >      if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) {
> >          ret = doPeer2PeerMigrate(driver, conn, vm, xmlin,
> > @@ -4112,25 +4150,13 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
> >                                           VIR_DOMAIN_EVENT_STOPPED,
> >                                           VIR_DOMAIN_EVENT_STOPPED_MIGRATED);
> >      }
> > -    resume = 0;
> >
> >  endjob:
> >      if (ret < 0)
> >          orig_err = virSaveLastError();
> >
> > -    if (resume && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
> > -        /* we got here through some sort of failure; start the domain again */
> > -        if (qemuProcessStartCPUs(driver, vm, conn,
> > -                                 VIR_DOMAIN_RUNNING_MIGRATION_CANCELED,
> > -                                 QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) {
> > -            /* Hm, we already know we are in error here.  We don't want to
> > -             * overwrite the previous error, though, so we just throw something
> > -             * to the logs and hope for the best
> > -             */
> > -            VIR_ERROR(_("Failed to resume guest %s after failure"),
> > -                      vm->def->name);
> > -        }
> > -
> > +    if (!v3proto &&
> > +        qemuDomainObjRestorePreMigrationState(conn, vm)) {
>
> You could also call qemuDomainObjRestorePreMigrationState
> unconditionally since you always reset preMigrationState which makes it
> safe to call qemuDomainObjRestorePreMigrationState multiple times but it
> does not really matter.
>

I changed it to unconditional call, I wanted to do that at first
(hence the carefulness inside that function), but rather called it
conditionally because I didn't want to explain that :)

> >          event = virDomainEventLifecycleNewFromObj(vm,
> >                                           VIR_DOMAIN_EVENT_RESUMED,
> >                                           VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> > @@ -4179,7 +4205,6 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver,
> >  {
> >      virObjectEventPtr event = NULL;
> >      int ret = -1;
> > -    bool resume;
> >      bool hasrefs;
> >
> >      /* If we didn't start the job in the begin phase, start it now. */
> > @@ -4194,32 +4219,18 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver,
> >      virCloseCallbacksUnset(driver->closeCallbacks, vm,
> >                             qemuMigrationCleanup);
> >
> > -    resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING;
> >      ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen,
> >                            cookieout, cookieoutlen,
> >                            flags, resource, NULL, graphicsuri);
> >
> > -    if (ret < 0 && resume &&
> > -        virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
> > -        /* we got here through some sort of failure; start the domain again */
> > -        if (qemuProcessStartCPUs(driver, vm, conn,
> > -                                 VIR_DOMAIN_RUNNING_MIGRATION_CANCELED,
> > -                                 QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) {
> > -            /* Hm, we already know we are in error here.  We don't want to
> > -             * overwrite the previous error, though, so we just throw something
> > -             * to the logs and hope for the best
> > -             */
> > -            VIR_ERROR(_("Failed to resume guest %s after failure"),
> > -                      vm->def->name);
> > +    if (ret < 0) {
> > +        if (qemuDomainObjRestorePreMigrationState(conn, vm)) {
> > +            event = virDomainEventLifecycleNewFromObj(vm,
> > +                                                      VIR_DOMAIN_EVENT_RESUMED,
> > +                                                      VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> >          }
> > -
> > -        event = virDomainEventLifecycleNewFromObj(vm,
> > -                                         VIR_DOMAIN_EVENT_RESUMED,
> > -                                         VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
> > -    }
> > -
> > -    if (ret < 0)
> >          goto endjob;
> > +    }
> >
> >      qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM3_DONE);
> >
>
> ACK with the nits addressed.
>

Pushed with the nits fixed.

> Jirka

Martin

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]