Re: [libvirt] [PATCHv4 24/51] snapshot: prevent stranding snapshot data on domain destruction
On 10/26/2012 09:47 AM, Philipp Hahn wrote:
Hello Eric,
On Friday 02 September 2011 06:25:01 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.
I just noticed a problem with that patch
282fe1f08c89189e36142fc2d12bae0175038bdd:
> index ac71b04..38a21db 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4852,6 +4853,14 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> goto cleanup;
> }
>
> + if (!virDomainObjIsActive(vm) &&
This check restricts the test to only running domains, that is if you undefine
a currently inactive domain, its snapshot metadata is left behind while the
domain is deleted.
I think you read this backwards. This check says that if the domain is
offline, it cannot be undefined, as that would strand the metadata. If
the domain is active, then it is running, and all undefine does on a
running domain is convert it to transient, but the domain is still
running, so the metadata is still accessible (up until the transient
domain halts, at which point it is cleaned up then).
This behaviour is actually documented in
<http://libvirt.org/html/libvirt-libvirt.html#virDomainUndefineFlags> (now
that I know what I have to look at), but I was still surprised that
virDomainUndefineFlags(0) returned success on my inactive domain with
snapshots.
That shouldn't happen - virDomainUndefineFlags(0) on an inactive domain
should fail if the domain has snapshots. Is this something you actually
hit, and can you give me steps to reproduce?
> + (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + _("cannot delete inactive domain with %d
> snapshots"), + nsnapshots);
> + goto cleanup;
> + }
> +
> if (!vm->persistent) {
> qemuReportError(VIR_ERR_OPERATION_INVALID,
> "%s", _("cannot undefine transient
domain"));
Is this restriction to only delete the snapshots for active domains on purpose
or am I missing something?
I would expect the undefine to be refused for all states, not just for active
domains.
The undefine is refused only if it would strand data. In the case of an
active domain, the snapshots are not stranded because the now-transient
domain still exists.
I would prefer the snapshots to be deleted for both active and
inactive
domains, since qemuDomainSnapshotDiscardAllMetadata() is not available
externally. And iterating over all snapshots to just delete them seems to be
wasteful, especially when you use qcow2 with its reference counting issues.
See the attached patch for my proposal.
I'm still not convinced we need your patch - using undefine on an active
domain to convert it to transient is too soon to forcefully discard
snapshots, as I could then redefine the domain to make it persistent
again at which point the snapshots still make sense to keep around.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 617 bytes)