Re: [libvirt] [PATCH 0/3 TCK] Some tests for snapshot management

On 08/26/2011 08:16 AM, Daniel P. Berrange wrote: >All these tests fail with current GIT, but are intended to work >with Eric's snapshot series applied. > >The first test in this list, however, does not pass. > >Eric's current tree forbids calling 'Destroy' on a transient >guest which has snapshots present. IMHO this is wrong, because >the guest may itself exit at any time, which leaves us in the >same state as is 'Destroy' had been called wrt snapshots. Hmm, interesting point. With managed save, we don't have that problem - the only way to have a managed save image is with a persistent def, so we can safely forbid undefine of a domain with managed save state. > >IMHO, we should be allowed to call 'virDomainDestroy' on a >transient guest with snapshots, and then later 'virDomainCreateXML' >to re-create the guest and still have access to the snapshots But leaving the snapshot metadata files behind is problematic. If you create a new domain with the same name but a different uuid, then those existing metadata files _will_ cause problems with the domain definition.

>In other words, just because a transient guest is not currently >present, does not mean we should forget about its snapshots as >a management app can re-create it again at any time. How about this compromise? I already documented in my RFC the desire to add a new flag, virDomainSnapshotCreateXML(, VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE), which will allow a management app to recreate a metadata file. I haven't yet implemented it, but think that it will solve this problem, as follows: Libvirt should not ever forbid destroy of a domain with snapshots. However, when a transient guest shuts down, then part of the cleanup that libvirt does for that guest is to remove all existing snapshot metadata (the snapshots themselves continue to exist, but the libvirt hierarchy of creation date, domain xml, and parent relationships is gone). On destroy, either the domain is persistent (so the snapshots will still be useful from the inactive guest), or the domain is transient (so the management app has to be aware of the ramifications). But the management app _already_ has to track domain xml independently if it plans on recreating a new transient guest from the same disk images, so it is not too much of a stretch to assume that the management app can also track snapshot xml. At which point, if the management app wants to recreate a new transient app with the same snapshot metadata, then it simply calls virDomainSnapshotCreateXML(, VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) for each saved snapshot that it wants to revive in libvirt's tracking. Thus, snapshot metadata is never left behind to confuse the creation of a new guest (persistent domains fail to undefine until the snapshots are cleaned up, and transient domains are cleaned up automatically when the last reference to the domain disappears), but can easily be restored and thus tracked externally. However, this means that your first test would need tweaking - the test itself would have to create the snapshot, capture the snapshots xml, destroy the domain, then create a new domain with the same name and uuid, restore the snapshot, then revert to the snapshot, all to prove that redefining a transient domain does not lose the actual snapshot data, just the metadata that tracked the snapshot. There's also some FIXMEs in libvirt about the notion of extracting the name of all internal snapshots directly from qcow2 files, and exposing those to the user with minimal information. qcow2 lacks parent and domain xml information, so the regenerated snapshot xml would be weak and reverting to it would require the use of VIR_DOMAIN_SNAPSHOT_REVERT_FORCE; but making it easier to expose all known internal snapshot names, even when libvirt does not have the metadata behind the origin of those internal snapshots, might be useful via a new flag to virDomainSnapshotListNames. > >Forbidding virDomainUndefine with snapshots is more reasonable, Good, I'll keep it that way, since it is consistent with managed save images. >but I think its still possible to argue that it should be allowed >and that a later virDomainDefine with the same name+uuid should >resurrect any previous snapshots. If resurrecting previous snapshots is important, then it should be done with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE.