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

Fri Mar 15 10:32:10 UTC 2019

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 at 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.

> diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
> index 947547627a..be9bf71af9 100644
> --- a/src/libvirt-domain-snapshot.c
> +++ b/src/libvirt-domain-snapshot.c
> @@ -117,15 +117,21 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
>   *
>   * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, then this
>   * is a request to reinstate snapshot metadata that was previously
> - * discarded, rather than creating a new snapshot.  This can be used
> - * to recreate a snapshot hierarchy on a destination, then remove it
> - * on the source, in order to allow migration (since migration
> - * normally fails if snapshot metadata still remains on the source
> - * machine).  When redefining snapshot metadata, the current snapshot
> - * will not be altered unless the VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT
> - * flag is also present.  It is an error to request the
> - * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flag without
> - * VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE.  On some hypervisors,
> + * captured from virDomainSnapshotGetXMLDesc() before removing that
> + * metadata, rather than creating a new snapshot.  This can be used to
> + * recreate a snapshot hierarchy on a destination, then remove it on
> + * the source, in order to allow migration (since migration normally
> + * fails if snapshot metadata still remains on the source machine).
> + * Note that while original creation can omit a number of elements
> + * from @xmlDesc (and libvirt will supply sane defaults based on the
> + * domain state at that point in time), a redefinition must supply
> + * more elements (as the domain may have changed in the meantime, so
> + * that libvirt no longer has a way to resupply correct
> + * defaults). When redefining snapshot metadata, the domain's current
> + * snapshot will not be altered unless the
> + * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flag is also present.  It is an
> + * error to request the VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flag
> + * without VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE.  On some hypervisors,
>   * redefining an existing snapshot can be used to alter host-specific
>   * portions of the domain XML to be used during revert (such as
>   * backing filenames associated with disk devices), but must not alter

Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|