On Tue, Jun 18, 2019 at 22:47:43 -0500, Eric Blake wrote:
[...]
> diff --git a/include/libvirt/libvirt-domain-checkpoint.h b/include/libvirt/libvirt-domain-checkpoint.h
> new file mode 100644
> index 0000000000..21b89cd190
> --- /dev/null
> +++ b/include/libvirt/libvirt-domain-checkpoint.h
> @@ -0,0 +1,149 @@
[...]
> +const char *virDomainCheckpointGetName(virDomainCheckpointPtr checkpoint);
> +virDomainPtr virDomainCheckpointGetDomain(virDomainCheckpointPtr checkpoint);
> +virConnectPtr virDomainCheckpointGetConnect(virDomainCheckpointPtr checkpoint);
Can't we just get the connection from the domain? Is this API of any
use? The same is kind of true about virDomainCheckpointGetDomain as
well. You already needed that API to get the checkpoint object in the
first place. I know these are the same as for the snapshots but I didn't
really undestand why they were needed in the first place.
> +typedef enum {
> + VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE = (1 << 0), /* Restore or alter
> + metadata */
> + VIR_DOMAIN_CHECKPOINT_CREATE_CURRENT = (1 << 1), /* With redefine, make
> + checkpoint current */
> + VIR_DOMAIN_CHECKPOINT_CREATE_NO_METADATA = (1 << 2), /* Make checkpoint without
> + remembering it */
This gets abused by higher level management tools in the snapshot world
to always create metadata-less snapshots. I don't think we should
implement this now so that there's more motivation to use this feature
properly.
This flag can stay here, but really the impl should just not bother with
that. (I'll raise this comment in the appropriate patch again).
(on the other hand we should still allow deleting the metadata only via
the CheckpointDelete API)
> + VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE = (1 << 3), /* use guest agent to
> + quiesce all mounted
> + file systems within
> + the domain */
> +} virDomainCheckpointCreateFlags;
[...]
> +/* Get a handle to a named checkpoint */
> +virDomainCheckpointPtr virDomainCheckpointLookupByName(virDomainPtr domain,
> + const char *name,
> + unsigned int flags);
> +
> +/* Get a handle to the current checkpoint */
> +virDomainCheckpointPtr virDomainCheckpointCurrent(virDomainPtr domain,
> + unsigned int flags);
So you described this as the checkpoint which gets updates, but can't
other hypervisors update all the checkpoints at once? In that case this
API would not be able to return all of that.
Can't we return that fact in a different way? e.g. in the XML?
> +
> +/* Get a handle to the parent checkpoint, if one exists */
> +virDomainCheckpointPtr virDomainCheckpointGetParent(virDomainCheckpointPtr checkpoint,
> + unsigned int flags);
> +
> +/* Determine if a checkpoint is the current checkpoint of its domain. */
> +int virDomainCheckpointIsCurrent(virDomainCheckpointPtr checkpoint,
> + unsigned int flags);
Is this necessary?
> +
> +/* Delete a checkpoint */
> +typedef enum {
> + VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN = (1 << 0), /* Also delete children */
> + VIR_DOMAIN_CHECKPOINT_DELETE_METADATA_ONLY = (1 << 1), /* Delete just metadata */
> + VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN_ONLY = (1 << 2), /* Delete just children */
> +} virDomainCheckpointDeleteFlags;
> +
> +int virDomainCheckpointDelete(virDomainCheckpointPtr checkpoint,
> + unsigned int flags);
> +
> +int virDomainCheckpointRef(virDomainCheckpointPtr checkpoint);
> +int virDomainCheckpointFree(virDomainCheckpointPtr checkpoint);
> +
> +#endif /* LIBVIRT_DOMAIN_CHECKPOINT_H */
[...]
> diff --git a/src/libvirt-domain-checkpoint.c b/src/libvirt-domain-checkpoint.c
> new file mode 100644
> index 0000000000..7485c414a4
> --- /dev/null
> +++ b/src/libvirt-domain-checkpoint.c
> @@ -0,0 +1,670 @@
[...]
> +/**
> + * virDomainCheckpointGetXMLDesc:
> + * @checkpoint: a domain checkpoint object
> + * @flags: bitwise-OR of supported virDomainCheckpointXMLFlags
> + *
> + * Provide an XML description of the domain checkpoint.
> + *
> + * No security-sensitive data will be included unless @flags contains
> + * VIR_DOMAIN_CHECKPOINT_XML_SECURE; this flag is rejected on read-only
> + * connections.
> + *
> + * Normally, the XML description includes an element giving a full
> + * description of the domain at the time the snapshot was created; to
s/snapshot/checkpoint/
> + * reduce parsing time, it will be suppressed when @flags contains
> + * VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN.
> + *
> + * By default, the XML description contains only static information that
> + * does not change over time. However, when @flags contains
> + * VIR_DOMAIN_CHECKPOINT_XML_SIZE, each <disk> listing adds an additional
> + * attribute that shows an estimate of the current size in bytes that
> + * have been dirtied between the time the checkpoint was created and the
> + * current point in time.
> + *
> + * Returns a 0 terminated UTF-8 encoded XML instance or NULL in case
> + * of error. The caller must free() the returned value.
> + */
[...]
> +/**
> + * virDomainCheckpointDelete:
> + * @checkpoint: the checkpoint to remove
> + * @flags: bitwise-OR of supported virDomainCheckpointDeleteFlags
> + *
> + * Removes a checkpoint from the domain.
> + *
> + * When removing a checkpoint, the record of which portions of the
> + * disk were dirtied after the checkpoint will be merged into the
> + * record tracked by the parent checkpoint, if any. Likewise, if the
> + * checkpoint being deleted was the current checkpoint, the parent
> + * checkpoint becomes the new current checkpoint.
Is there any situation where a checkpoint could have multiple children?
In that case semantics of the above may be ... hard. We are going to hit
that with snapshots when reverting to an earlier point and then starting
an alternate future.
IIRC from our previous discussions you did not want to hide checkpoints
which are not relevant in a given timeline, so that might also cause
problems.
The problem is that we didn't address this yet with external snapshots
where all this fun will need to be dealt whith too.
> + *
> + * If @flags includes VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN, then any
> + * descendant checkpoints are also deleted. If @flags includes
> + * VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN_ONLY, then any descendant
> + * checkepoints are deleted, but this checkpoint remains. These two
> + * flags are mutually exclusive.
> + *
> + * If @flags includes VIR_DOMAIN_CHECKPOINT_DELETE_METADATA_ONLY, then
> + * any checkpoint metadata tracked by libvirt is removed while keeping
> + * the checkpoint contents intact; if a hypervisor does not require
> + * any libvirt metadata to track checkpoints, then this flag is
> + * silently ignored.
> + *
> + * Returns 0 on success, -1 on error.
> + */
> +int
> +virDomainCheckpointDelete(virDomainCheckpointPtr checkpoint,
> + unsigned int flags)
> +{
> + virConnectPtr conn;
> +
> + VIR_DEBUG("checkpoint=%p, flags=0x%x", checkpoint, flags);
> +
> + virResetLastError();
> +
> + virCheckDomainCheckpointReturn(checkpoint, -1);
> + conn = checkpoint->domain->conn;
> +
> + virCheckReadOnlyGoto(conn->flags, error);
> +
> + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN,
> + VIR_DOMAIN_CHECKPOINT_DELETE_CHILDREN_ONLY,
> + error);
> +
> + if (conn->driver->domainCheckpointDelete) {
> + int ret = conn->driver->domainCheckpointDelete(checkpoint, flags);
> + if (ret < 0)
> + goto error;
> + return ret;
> + }
> +
> + virReportUnsupportedError();
> + error:
> + virDispatchError(conn);
> + return -1;
> +}
[...]
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 509ce5ac8b..c6e322b211 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
[...]
> @@ -6282,6 +6283,15 @@ virDomainUndefine(virDomainPtr domain)
> * whether this flag is present. On hypervisors where snapshots do
> * not use libvirt metadata, this flag has no effect.
> *
> + * If the domain is inactive and has any checkpoint metadata (see
> + * virDomainListAllCheckpoints()), then including
> + * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA in @flags will also remove
> + * that metadata. Omitting the flag will cause the undefine of an
> + * inactive domain to fail. Active checkpoints will retain checkpoint
Active domains will ...
> + * metadata until the (now-transient) domain halts, regardless of
> + * whether this flag is present. On hypervisors where checkpoints do
> + * not use libvirt metadata, this flag has no effect.
This will not be true if you don't implement that flag with
virCheckFlags. In that case it will have the efect of returning error
rather than no effect.
Also this series does not seem to fix all hypervisors.
> + *
> * If the domain has any nvram specified, the undefine process will fail
> * unless VIR_DOMAIN_UNDEFINE_KEEP_NVRAM is specified, or if
> * VIR_DOMAIN_UNDEFINE_NVRAM is specified to remove the nvram file.
[...]
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 46c32a081b..231c2bc0f0 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -820,17 +820,32 @@ LIBVIRT_5.2.0 {
> } LIBVIRT_4.10.0;
>
> LIBVIRT_5.5.0 {
> + global:
> + virDomainCheckpointCreateXML;
> + virDomainCheckpointCurrent;
> + virDomainCheckpointDelete;
> + virDomainCheckpointFree;
> + virDomainCheckpointGetConnect;
> + virDomainCheckpointGetDomain;
> + virDomainCheckpointGetName;
> + virDomainCheckpointGetParent;
> + virDomainCheckpointGetXMLDesc;
> + virDomainCheckpointIsCurrent;
> + virDomainCheckpointListAllChildren;
> + virDomainCheckpointLookupByName;
> + virDomainCheckpointRef;
> + virDomainListAllCheckpoints;
> virNetworkListAllPorts;
> - virNetworkPortLookupByUUID;
> - virNetworkPortLookupByUUIDString;
Fixing of the ordering here should really be in a separate commit.
ACK if the above hunk consists only of line additions and you make sure
(as a follow up possibly) to fix all implementations of
virDomainUndefine to support the new flag so that the above semantics is
adhered to.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
