Re: [libvirt] [PATCH] snapshots: More clarification about REDEFINE

On Wed, Mar 13, 2019 at 10:13:00PM -0500, Eric Blake wrote: > Based on recent list questions about the proposed addition of > virDomainCheckpointCreateXML(REDEFINE), it is worth adding some > clarification to the existing snapshot redefine documentation that is > serving as the basis for checkpoints. > > Normal snapshot creation requires very few elements from the user XML > (libvirt can pick sane defaults for items that are omitted, and many > fields, including <domain> are documented as readonly output fields > ignored on input). But during REDEFINE, the API wants the complete XML > produced by an earlier virDomainSnapshotGetXMLDesc, which includes a > <domain> sub-element capturing the state a the time the snapshot was > first created. As the domain state has likely changed in the meantime, > omitting <domain> during REDEFINE is extremely risky (to the point > that virDomainSnapshotRevert() requires the use of FORCE flag to use > such a snapshot) - we only support it because of > backwards-compatibility to snapshots created before 0.9.5 started > capturing <domain>. When checkpoints are introduced, the <domain> > element will be mandatory in REDEFINE. > > Note that because <domain> can be potentially huge, we also are unable > to include a bulk listing or redefine of all <domainsnapshot>s for a > single domain in one XML dump (for example, a 1M <domain> XML * 16 > snapshots explodes into 16M in a bulk form, which gets difficult to > send over RPC). Perhaps we could add a flag to request that the > <domain> sub-element be omitted on output, but such output is no > longer suitable for sane REDEFINE input. I'm not understanding why the "bulk listing" comments are relevant here ? We don't have any bulk list support in current code do we ?

> Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > --- > > Question: 0.9.5 is so long ago, is it worth reworking the qemu > snapshot code to declare that REDEFINE will no longer import a > snapshot definition that lacks a <domain> subelement? (Note: the vbox > driver also supports snapshots and the REDEFINE flag, but does not > output <domain> and thus should still allow it to be omitted on > redefine). The current API wording DOES have the disclaimer regarding > REDEFINE that "the hypervisor may validate that reverting to the > snapshot appears to be possible", which we can use as our escape > clause whereby the modern qemu driver is merely adding a validation > that <domain> must be present for the redefine to be viable, even > though it is at odds with our usual efforts to always parse XML that > used to work in earlier versions. Would enforcing it actually make the code any simpler ? If not, then I wouldn't bother changing it, especially because the vbox driver is not enforcing it - I think its undesirable to have different rules for each driver in this respect.

Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt;