On 3/15/19 5:32 AM, Daniel P. Berrangé wrote:
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 ?
No; it's more of a side note to capture the discussions held on the list recently that led to me writing this patch in the first place. I'll tweak the commit message to make that more obvious.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
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.
It might make it simpler, but it would probably also slow down my goal of getting incremental snapshot in, so I won't be pursuing it, especially since vbox is still fine without <domain> so you are right that tightening just qemu feels awkward.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Thanks; I'll push with the updated commit message. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org