[libvirt] [PATCHv4 24/51] snapshot: prevent stranding snapshot data on domain destruction

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121030/82e20ec4/attachment-0001.sig>