Re: [libvirt] [PATCH v2 5/9] backup: Document new XML for backups

On Fri, Oct 12, 2018 at 00:10:07 -0500, Eric Blake wrote: [...] I'd drop the last sentence as anything else will not give the users the ability to do incremental backups. [...] I think that too much of this paragraph was stolen from snapshots. It's not entirely clear to me how the 'current checkpoint' is supposed to work and since this is meant for completely external backups without the way to revert the backup via libvirt the description of the setting of the current checkpoint does not make much sense. I'm not sure even whether we should copy the notion of the 'current' checkpoint at this stage from snapshots. > + be formed by reverting to a checkpoint with a child, then creating > + another checkpoint. > + <p> > + The top-level <code>domaincheckpoint</code> element may contain > + the following elements: > + <dl> > + <dt><code>name</code></dt> > + <dd>The name for this checkpoint. If the name is specified when > + initially creating the checkpoint, then the checkpoint will have > + that particular name. If the name is omitted when initially > + creating the checkpoint, then libvirt will make up a name for > + the checkpoint, based on the time when it was created. > + </dd> > + <dt><code>description</code></dt> > + <dd>A human-readable description of the checkpoint. If the > + description is omitted when initially creating the checkpoint, > + then this field will be empty. > + </dd> > + <dt><code>disks</code></dt> > + <dd>On input, this is an optional listing of specific > + instructions for disk checkpoints; it is needed when making a > + checkpoint on only a subset of the disks associated with a > + domain (in particular, since qemu checkpoints require qcow2 > + disks, this element may be needed on input for excluding guest > + disks that are not in qcow2 format); if the entire element was I'd avoid the part about 'qcow2' and just state that it can be used to control which disks are part of the checkpoint. Please don't allow the <source> part at all. <target> is unique and we don't have to inflict more pain on us. I'm afraid that this will lead to a mess if we allow naming the resource differently than the snapshot. Is this possible by itself? Or only with combination with snapshots? Umm, I don't see a purpose of this. The NBD part is a qemuism. > + a third-party client can then read as needed, and which requires > + the use of temporary storage, typically local, until the backup > + is complete). > + <p> > + The instructions for beginning a backup job are provided as > + attributes and elements of the > + top-level <code>domainbackup</code> element. This element > + includes an optional attribute <code>mode</code> which can be > + either "push" or "pull" (default push). Where elements are > + optional on creation, <code>virDomainBackupGetXMLDesc()</code> > + can be used to see the actual values selected (for example, > + learning which port the NBD server is using in the pull model, > + or what file names libvirt generated when none were supplied). > + The following child elements are supported: > + <dl> > + <dt><code>incremental</code></dt> > + <dd>Optional. If this element is present, it must name an > + existing checkpoint of the domain, which will be used to make > + this backup an incremental one (in the push model, only > + changes since the checkpoint are written to the destination; > + in the pull model, the NBD server uses the > + NBD_OPT_SET_META_CONTEXT extension to advertise to the client > + which portions of the export contain changes since the > + checkpoint). If omitted, a full backup is performed. > + </dd> > + <dt><code>server</code></dt> > + <dd>Present only for a pull mode backup. Contains the same > + attributes as the <code>protocol</code> element of a disk > + attached via NBD in the domain (such as transport, socket, > + name, port, or tls), necessary to set up an NBD server that > + exposes the content of each disk at the time the backup > + started. > + </dd> Is there a possibility that other hypervisors might use a per-disk server? Again. Don't allow this or we will get bitten. qcow2 is a qemuism. The docs should be general. [...] This was not documented. What's the purpose? Shouldn't we use <empty/> ? Split this out similarly to name='backupDisksPush'. I see that the disk portion is removed but the server specification should be as well. Yes The above needs to be deleted. The above should be deleted. [...] [...] Nope.