[libvirt] [PATCH 3/5] snapshot: Support deleting external disk snapshots when deleting

[Peter Krempa]

On Sun, Oct 21, 2018 at 19:38:50 +0300, Povilas Kanapickas wrote:
> Signed-off-by: Povilas Kanapickas <povilas at radix.lt>
> ---
>  src/qemu/qemu_domain.c | 45 ++++++++++++++++++++++++++++++++++++++----
>  src/qemu/qemu_driver.c |  7 ++++---
>  2 files changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ba3fff607a..73c41788f8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8244,6 +8244,38 @@ qemuDomainSnapshotForEachQcow2(virQEMUDriverPtr driver,
>                                               op, try_all, def->ndisks);
>  }
>  
> +static int
> +qemuDomainSnapshotDiscardExternal(virDomainSnapshotObjPtr snap)
> +{
> +    int i;
> +    virDomainSnapshotDiskDefPtr snap_disk;
> +
> +    // FIXME: libvirt stores the VM definition and the list of disks that
> +    // snapshot has been taken of since 0.9.5. Before that we must assume that
> +    // all disks from within the VM definition have been snapshotted and that
> +    // they all were internal disks. Did libvirt support external snapshots
> +    // before 0.9.5? If so, if we ever end up in this code path we may incur
> +    // data loss as we'll delete whole QEMU file thinking it's an external
> +    // snapshot.

No any external snapshot does have the state, so the condition below is
dead code.

> +    if (!snap->def->dom) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       _("Snapshots created using libvirt 9.5 and containing "
> +                         "external disk snapshots cannot be safely "
> +                         "discarded."));
> +        return -1;
> +    }
> +
> +    for (i = 0; i < snap->def->ndisks; ++i) {
> +        snap_disk = &(snap->def->disks[i]);
> +        if (snap_disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> +            continue;
> +        if (unlink(snap_disk->src->path) < 0)
> +            VIR_WARN("Failed to unlink snapshot disk '%s'",
> +                     snap_disk->src->path);

So this will ever only work properly for leaf snapshots. If you try to
delete a snapshot which has children you'll destroy any of it's
children. Since they are not deleted here we can't do it without that
since reverting to them would be broken.

Ideally we want to merge the changes to all relevant snapshots so that
the intermediate files and snapshot metadata can be deleted without data
loss even in the middle of the chain/tree.

Also one other note, snapshots are possible on networked storage where
unlink will not work, so this needs to be made more general.
-------------- 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/20181107/547daf5a/attachment-0001.sig>