Re: [libvirt] [PATCH v8 06/21] backup: Parse and output checkpoint XML

On 4/24/19 10:53 AM, Peter Krempa wrote: We have several existing APIs that have not yet been taught flags to perform validation (virDomainManagedSave, for example). Do we really want all new APIs to perform unconditional schema validation, or do we want to finish the ongoing project to add flags to existing APIs and merely insist that new APIs also be given a schema validation flag? So far, my implementation has not wired in any schema validation. This is copying from the snapshot code. If we desire, it may be worth cleaning up the snapshot code to use the modern approaches first, and then the checkpoint code should resemble the cleaned up version (rather than having to clean up both copies at once). Do we want mandatory schema validation for the new API? Then again, a lot of things in the schema are optional - in particular, it is okay for a user to define a new checkpoint without a name (we create a name during post-parse processing); but NOT okay to use the redefine flag without a name - but the schema doesn't really have a way to express a conditional (aspects of the XML that are optional under one use, but mandatory under another - where that use is an external flag rather than something in the XML itself). Yes, bitmaps are how qemu implements checkpoints. But per patch 1/21, the XML being proposed uses: + <dt><code>checkpoint</code></dt> + <dd>An optional attribute; possible values + are <code>no</code> when the disk does not participate + in this checkpoint; or <code>bitmap</code> if the disk + will track all changes since the creation of this + checkpoint via a bitmap.</dd> + <dt><code>bitmap</code></dt> + <dd>The attribute <code>bitmap</code> is only valid + if <code>checkpoint='bitmap'</code>; it describes the + name of the tracking bitmap (defaulting to the + checkpoint name).</dd> It is conceivable that adding checkpoint support for another driver may pick a different (third) checkpoint='MODE', at which point that mode will add a different attribute that ties in with that mode (other than qemu's checkpoint='bitmap' bitmap='NAME'). So parsing and assigning the 'bitmap' name, when checkpoint='bitmap', makes sense here, while generating a default bitmap name starts to be a bit more specific to qemu. Is the question whether this function should be moved into src/qemu/ somewhere, rather than generic to the checkpoint XML parse/format code? XPath wise, you can't match "/domaincheckpoint/disks/disk[N]" to "/domaincheckpoint/domain/devices/disk[N]" unless you have an associated domain definition from the time the checkpoint was created. This code matches what snapshots do, except that in snapshots, the domain definition was optional for back-compat, while for checkpoints, I'm insisting that the domain definition always be present. Copy and paste from the snapshot code. (Personally, I prefer minimalisting typing, and would rather drop == NULL where I can). But if adding explicit == 0 makes this code get merged sooner, I can do that. No, this is the same boat as snapshots. We are correlating the list of "/domainbackup/disks/disk[N]" in the backup with the list of "/domainbackup/domain/devices/disk[N]" elements from the point-in-time domain definition when the checkpoint was created - and that point-in-time definition is unaffected by later hotplugs. You are right that later hotplugs can affect the ability to USE a checkpoint (a disk that is present at time of backup but not at the checkpoint undergoes a full backup, since it was hotplugged in the meantime), but that is a different correlation than what is being determined here. Easy enough to default to no backup on those as well. Mandatory on output, but with sane defaulting on input? Or mandatory on input as well? Copying from what snapshots did. Not pointless - a checkpoint can be created independently from a backup or snapshot, but you HAVE to know which disks were present in the domain at the time of the checkpoint in order to detect which other disks were hotplugged since the checkpoint (and where the hotplugged disks get a full backup, rather than incremental). Correct, we have to figure out which snapshot interactions may invalidate checkpoints. If you create a snapshot without also creating a new checkpoint at that time, then reverting to the snapshot will invalidate the bitmap covering the period between the checkpoint before the snapshot to the next checkpoint after the first branch from the snapshot; otherwise, if you have a checkpoint created in tandem with the snapshot, then reverting to the snapshot can create a new checkpoint child of the same parent that the snapshot's original checkpoint was also child of. Or visually, if we only have: C1 ... Snap ... C2 the bitmap that covers the delta from C1 to C2 is invalidated when reverting to Snap (we don't know how much of the bitmap was from C1 to snap, vs. how much was from Snap to C2); but if we have: C1 ... Snap/C2 ... C3 the bitmap from C2.C3 is a child of the bitmap from C1.C2, and reverting to Snap lets us also create: C1 ... Snap/C4 ... C5 where the bitmap from C4.C5 is also a child of the bitmap from C1.C2 but now tracks the alternate execution. (Checkpoint C2 and C4 cover the same moment in guest execution, but given that implementation names the bitmap after a checkpoint, it means we need two different checkpoint names at the same point in time to track the two different bitmaps covering the divergent guest changes) So in addition to being able to create a checkpoint with a snapshot, we also need to be able to create a checkpoint at the time of reverting to a snapshot. Copied from snapshots, but I can do that. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org