Re: [libvirt] [RFC v2] external (pull) backup API

On 04/03/2018 07:01 AM, Nikolay Shirokovskiy wrote: This is a first-pass review (making comments as I first encounter something, even if it gets explained later in the email) So, rewriting this to make sure I understand, let's start with a disk with contents A, then take a snapshot, then write B: In the existing libvirt snapshot APIs, the data gets distributed as: base (contents A) <- new active (contents B) where you want the new API: base, remains active (contents B) ~~~ backup (contents A) So the biggest reason for a new libvirt API is that we need management actions to control which NBD images from qemu are exposed and torn down at the appropriate sequences. Just to make sure, we have the existing API of: virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, unsigned int flags); So you are creating a new object (virDomainBlockSnapshotPtr) rather than reusing the existing VirDomainSnapshotPtr, and although the two commands are similar, we get to design a new XML schema from scratch rather than trying to overload yet even more functionality onto the existing API. Should we also have: const char *virDomainBlockSnapshotGetName(virDomainBlockSnapshotPtr snapshot); virDomainPtr virDomainBlockSnapshotGetDomain(virDomainBlockSnapshotPtr snapshot); virConnectPtr virDomainBlockSnapshotGetConnect(virDomainBlockSnapshotPtr snapshot); for symmetry with existing snapshot API? I'm guessing this is the counterpart to virDomainListAllSnapshots() (the modern listing interface), and that we probably don't want counterparts for virDomainSnapshotNum/virDomainSnapshotListNames (the older listing interface, which was inherently racy as the list could change in length between the two calls). Also, the virDomainSnapshotPtr had a number of API to track a tree-like hierarchy between snapshots (that is, you very much want to know if snapshot B is a child of snapshot A), while it looks like your new virDomainBlockSnapshotPtrs are completely independent (no relationships between the snapshots, each can be independently created or torn down, without having to rewrite a relationship tree between them, and there is no need for counterparts to things like virDomainSnapshotNumChildren). Okay, I think that makes sense, and is a good reason for introducing a new object type rather than shoe-horning this into the existing API. So in the XML, the <fleece> element describes the destination file (back to my earlier diagram, it would be the file that is created and will hold content 'A' when the main active image is changed to hold content 'B' after the snapshot was created)? I'm trying to figure out how BlockCheckpoint and BlockSnapshots relate. Maybe it will be more clear when I read the implementation section below. Is the idea that I can't create a BlockSnapshot without first having a checkpoint available? If so, where does that fit in the <domainblocksnapshot> XML? This feels a bit awkward - up to now, attaching a device is something visible to the guest, but you are trying to reuse the interface to attach something tracked by the domain, but which has no impact to the guest. That is, the guest has no clue whether a block export exists pointing to a particular checkpoint, nor does it care. What does the XML look like in these calls? This just reinforces my thoughts above - is the reason it doesn't make sense to assign a device id to the export due to the fact that the export is NOT guest-visible? Does it even belong under the "domain/devices/" xpath of the domain XML, or should it be a new sibling of <devices> with an xpath of "domain/blockexports/"? So this is the XML you pass to virDomainBlockExportStart, with the goal of telling qemu to start or stop an NBD export on the backing chain associated with disk "sda", where the export is serving up data tied to checkpoint "d068765e-8b50-4d74-9b72-1e55c663cbf8", and which will be associated with the destination snapshot file described by the <domainblocksnapshot> named "0044757e-1a2d-4c2c-b92f-bb403309bb17"? Why is it named <domainblockexport> here, but... <blockexport> here? The exportname property is new here compared to the earlier listing - is that something that libvirt generates, or that the user chooses? Which libvirt API triggers this action? virDomainBlockSnapshotCreateXML? Is qemu creating this file, or is libvirt pre-creating it and qemu just opening it? I guess this is a case where libvirt would want to pre-create an empty qcow2 file (either by qemu-img, or by the new x-blockdev-create in qemu 2.12)? Okay, it looks like this file is what you listed in the XML for <domainblocksnapshot>, so libvirt is creating it. Does the new file have a backing image, or does it read as completely zeroes? No trailing comma in JSON {}, but it's not too hard to figure out what you mean. You showed a transaction with only one element; but presumably we are using a transaction because if we want to create a point in time for multiple disks at once, we need two separate blockdev-backup actions joined in the same transaction to cover the two disks. So this command is telling qemu to start using a brand-new qcow2 file as its local storage for tracking that a snapshot is being taken, and that point in time is the checkpoint? Am I correct that you would then tell qemu to export an NBD view of this qcow2 snapshot which a third-party client can connect to and use NBD_CMD_BLOCK_STATUS to learn which portions of the file contain data (that is, which clusters has qemu copied into the backup, because the active image has changed them since the checkpoint, but anything not dirty in this file is still identical to the last backup? Would libvirt ever want to use something other than "sync":"none"? So this is telling qemu to export the temporary qcow2 image created in the point above. An NBD client would see the export getting progressively more blocks with data as the guest continues to write more clusters (as qemu has to copy the data from the checkpoint to the temporary file before updating the main image with the new data). If the NBD client reads a cluster that has not yet been copied by qemu (because the guest has not written to that cluster since the block job started), would it see zeroes, or the same data that the guest still sees? Libvirt is already tracking a tree relationship between internal snapshots (the virDomainSnapshotCreateXML), because qemu does NOT track that (true, internal snapshots don't get as much attention as external snapshots) - but the fact remains that qemu is probably not the best place to track relationship between multiple persistent bitmaps, any more than it tracks relationships between internal snapshots. So having libvirt track relations between persistent bitmaps is just fine. Do we really have to rename bitmaps in the qcow2 file, or can libvirt track it all on its own? Earlier, you said that the new virDomainBlockSnapshotPtr are independent, with no relations between them. But here, you are wanting to keep incremental backups related to one another. Here, the transaction makes sense; you have to create the persistent dirty bitmap to track from the same point in time. The dirty bitmap is tied to the active image, not the backup, so that when you create the NEXT incremental backup, you have an accurate record of which sectors were touched in snapshot-scsi0-0-0-0 between this transaction and the next. Again, you're showing the QMP commands that libvirt is issuing; which libvirt API calls are driving these actions? Do you have measurements on whether having multiple active bitmaps hurts performance? I'm not yet sure that managing a chain of disabled bitmaps (and merging them as needed for restores) is more or less efficient than managing multiple bitmaps all the time. On the other hand, you do have a point that restore is a less frequent operation than backup, so making backup as lean as possible and putting more work on restore is a reasonable tradeoff, even if it adds complexity to the management for doing restores. Adding a bitmap to a server is would would advertise to the NBD client that it can query the "qemu-dirty-bitmap:d068765e-8b50-4d74-9b72-1e55c663cbf8" namespace during NBD_CMD_BLOCK_STATUS, rather than just "base:allocation"? A suggestion on the examples - while UUIDs are nice and handy for management tools, they are a pain to type and for humans to quickly read. Is there any way we can document a sample transaction stream with all the actors involved (someone issues a libvirt API call XYZ, libvirt in turn issues QMP command ABC), and using shorter names that are easier to read as humans? Why can't restore be done while the guest is offline? (Oh right, we still haven't added decent qemu-img support for bitmap manipulation, so we need a qemu process around for any bitmap changes). As I understand it, the point of bitmaps and snapshots is to create an NBD server that a third-party can use to read just the dirty portions of a disk in relation to a known checkpoint, to save off data in whatever form it wants; so you are right that the third party then needs a way to rewrite data from whatever internal form it stored it in back to the view that qemu can consume when rolling back to a given backup, prior to starting the guest on the restored data. Do you need additional libvirt APIs exposed for this, or do the proposed APIs for adding snapshots cover everything already with just an additional flag parameter that says whether the <domainblocksnapshot> is readonly (the third-party is using it for collecting the incremental backup data) or writable (the third-party is actively writing its backup into the file, and when it is done, then perform a block-commit to merge that data back onto the main qcow2 file)? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org