[libvirt] [PATCH 4/6] qemu: snapshot: Refactor snapshot rollback on failure

[Peter Krempa]

On Sat, Jan 07, 2017 at 13:37:12 -0500, John Ferlan wrote:
> 
> 
> On 12/16/2016 11:24 AM, Peter Krempa wrote:
> > The code at first changed the definition and then rolled it back in case
> > of failure. This was ridiculous. Refactor the code so that the image in
> > the definition is changed only when the snapshot is successful.
> > 
> > The refactor will also simplify further fix of image locking when doing
> > snapshots.
> > ---
> >  src/qemu/qemu_driver.c | 352 ++++++++++++++++++++++++++++---------------------
> >  1 file changed, 203 insertions(+), 149 deletions(-)

[...]

> > +            if (diskdata[i].prepared)
> > +                qemuDomainDiskChainElementRevoke(driver, vm, diskdata[i].src);
> > +
> > +            if (diskdata[i].created &&
> > +                virStorageFileUnlink(diskdata[i].src) < 0)
> > +                VIR_WARN("Unable to remove just-created %s", diskdata[i].src->path);
> >          }
> >      }
> > 
> > -    if (ret == 0 || !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) {
> > +    if (ret == 0 || !actions) {

Note that the above change does not change logic in any way. It just
saves a call to virQEMUCapsGet since it's checked earlier when 'actions'
is allocated. 'actions' is allocated only if the capability is present.

> 
> Wouldn't we want to save the status based on transactions? Should this
> be "if ((!actions && do_transaction) || (actions && ret == 0))".

Not sure if this is relevant for this patch. This refactor tries to
remove the rollback of the disk image data. I don't think this piece of
code should be touched in this patch.

> 
>     /* Either a partial update has happened without transaction
>      * support or a successful usage of actions to perform all
>      * updates occurred, thus we need to save our state/config */
>     if (do_transaction && (!actions || (actions && ret == 0))) {
>     }
> 
> Trying to avoid a possible write of the file if no transactions were
> completed that could cause an erroneous failure since we did nothing.

That is a very unlikely corner case. You are right that at this point
only the memory snapshot was taken which currently does not modify the
status or config XML in any way so it would be valid at this point to do
the suggested change.

I just don't think it's worth at all.

> 
> >          if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0 ||
> >              (persist && virDomainSaveConfig(cfg->configDir, driver->caps,
> >                                              vm->newDef) < 0))
> >              ret = -1;
> 
> I know this just follows the previous code, but at this point there's no
> more undoing and returning -1 would indicate something went wrong.
> Nothing quick comes to mind on the best way to handle that, so we can
> continue with this. Still we have an inconsistency between actual,
> Status, and possibly Config.  Which is worse?

Well, losing the status information will result into libvirt thinking
that the backing chain is different than qemu thinks and thus block jobs
will fail to work properly.

When we lose config changes you are in for a data loss. A new
start of the VM will basically revert the snapshot disk images back to
the state at the snapshot and you lose all data since the snapshot.

Both cases happen only if libvirtd is restarted since otherwise it does
not re-read the files.

I think that both should be fatal errors anyways due to somewhat
catastrophic implications.
> 
> ACK with at least the leak fixed... It'd be good to handle saving things
> better.  Whether there's anything than be done if one of the save's fail
> - is perhaps a challenge for another day.
> 
> John

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170110/7bcd7c5b/attachment-0001.sig>