On Fri, Aug 26, 2011 at 08:57:54AM -0600, Eric Blake wrote: > 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. Surely this simply means that the snapshot metadata files shold be recording the orignal domain UUID, so that we can safely ignore/discard them if the guest is re-defined with the same name but new uuid ? > >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. This is all workable, but is it overkill compared to just validating the original VM uuid against the new UUID when loading metdata ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list