10:58 a.m.
On 6/19/19 6:20 AM, Peter Krempa wrote:
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.
Back when I added snapshots, the ability to get back to the
domain/connection was added in symmetry to all of the other opaque
public objects. The code doing so is free (there is no RPC involved), so
it is merely a question of whether we expose the public function.
Looking at the python bindings, we exempt
virDomainSnapshotGet{Connect,Domain} from creation by the generator,
with a comment that "The equivalent should be implemented in pure python
for each class". And there is indeed a python counterpart for getting
at owner objects.
I don't know if dropping these accessor functions would hurt anyone. I
also note that while snapshot APIs were added in 0.8.0,
virDomainSnapshotGet{Connect,Domain} were not added until 0.9.5,
although the mailing list archives don't show strong evidence of a user
needing them so much as adding them for symmetry.
I can't find any evidence of these functions being used directly within
libvirt.git or libvirt-python.git, but that's not conclusive as to how
they might be used externally.
I guess it's also possible to not export them for now, and add them
later if we actually find a use, even though it makes this inconsistent
from other objects that all have a parent accessor, but it feels like
busywork to do so.
> +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)
I already extracted the virDomainCheckpointHasMetadata() API out of my
v8 proposal. I'm working on removing this flag from my initial
implementation (we can add it later if we have a use case, but let's try
to get the initial implementation not relying on it).
> + 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?
Yes, I think that would be possible. In fact, adding a filter flag for
the ListAll api would be an easier way for another hypervisor to return
all checkpoints that are still being actively updated rather than
relying on concatenation with a later checkpoint. I'll give that a shot.
> +
> +/* 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?
Not if the XML exposes it (per your point above).
> +/**
> + * 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?
Probably yes, once we figure out how to integrate snapshots and
checkpoints together. Not in the initial release, though.
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.
And that's precisely the same scenario where checkpoints may have
multiple children (each alternative timeline depends on the same parent
for reconstructing the set of changes from the point in time of the
parent back to the present of that timeline).
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.
> +++ 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.
Copy-and-paste wording; which means the snapshot sentence should also be
improved. Maybe:
If the hypervisor supports checkpoints, then this flag ...
Implying that hypervisors that don't have any checkpoint APIs also don't
support the flag. Then, among hypervisors that do support checkpoints,
it's hypervisor-dependent on whether the flag has an effect (the
hypervisor used libvirt metadata tracking) or is a no-op (the hypervisor
tracks checkpoints without libvirt help).
> 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.
Will do, as a trivial patch (we've tended to use alphabetical sorting in
these sections for past releases, even if it is not chronological for
mid-release additions).
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org