Re: [libvirt] [PATCHv3 13/43] snapshot: prevent stranding snapshot data on domain destruction
On 08/24/2011 10:49 AM, Daniel P. Berrange wrote:
On Wed, Aug 24, 2011 at 09:22:30AM -0600, Eric Blake wrote:
> Just as leaving managed save metadata behind can cause problems
> when creating a new domain that happens to collide with the name
> of the just-deleted domain, the same is true of leaving any
> snapshot metadata behind. For safety sake, extend the semantic
> change of commit b26a9fa9 to also cover snapshot metadata as a
> reason to reject losing the last reference to a domain (undefine
> on an inactive, or shutdown/destroy on a transient). The caller
> must first take care of snapshots, possible via the existing
> virDomainSnapshotDelete.
>
> This also documents some new flags that hypervisors can choose to
> support to shortcuts taking care of the metadata as part of the
> shutdown process; however, nontrivial driver support for these flags
> will be deferred to future patches.
>
> +/* Counterparts to virDomainUndefineFlagsValues, but note that
running
> + * domains have no managed save data, so no flag is provided for that.
> */
> +typedef enum {
> + /* VIR_DOMAIN_DESTROY_MANAGED_SAVE = (1<< 0), */ /* Reserved */
> + VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA = (1<< 1), /* If last use of
domain,
> + then also remove any
> + snapshot metadata */
This one is always safe - the only things deleted are libvirt files,
leaving all the qcow2 disks and external drives intact.
> + VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL = (1<< 2), /*
If last use of domain,
> + then also remove any
> + snapshot data */
But I am starting to be convinced by your assertion that this is
dangerous - there is no way to cleanly report partial failure with a
flag like this, which means that in the failure case, it did not add any
convenience, but made things worse.
This feels a little bit wrong to me, for the same reasons we
discussed wrt deleteing disks on undefine, particularly once
we start storing snapshots in external files across arbitrary
storage, instead of just inside qcow2 images
https://www.redhat.com/archives/libvir-list/2011-July/msg01548.html
How about a compromise? Are you willing to agree that deleting snapshot
metadata is always safe (the user's disk images are still intact,
whether qcow2 or external, and only the hidden files in /var/lib/libvirt
are removed so that they cannot interfere with later creation of a new
domain by the same name)? That is, should I rework this patch for just
the metadata flag, or drop it entirely?
And there's still the O(n) vs. O(n^2) consideration; if we drop this
patch (or even if we keep the metadata portion of this patch but drop
the full deletion aspect), I have to wonder if there is any way we can
improve existing virDomainSnapshotDelete (or create a new API) that
allows deletion of multiple snapshots without inherent quadratic
behavior caused by deleting one snapshot at a time forcing all other
snapshots to be checked for reparenting.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org