Re: [libvirt] [PATCH v4 11/20] wip: backup: Parse and output checkpoint XML

On 2/12/19 11:23 AM, John Ferlan wrote: I understand. The 'wip' tags should be gone for v5. New file, but so heavily copied from snapshot_conf.h that I felt safer preserving all existing copyrights. Copy-and-paste from virDomainSnapshotDef, but my recent work there has made me want to reconsider where the current object is tracked. The problem is that the current way we store things on disk is via a separate <domainsnapshot> XML per snapshot, with no other obvious place for the internal representation of a <domain> to track which snapshot is current. BUT, as I have just posted patches to add a bulk dump/redefine with VIR_DOMAIN_XML_SNAPSHOTS, we COULD just ditch our current separate-file storage of snapshots, and instead store the snapshots directly with the <domain> (for all new saves; when loading libvirtd, we'd still have to keep the code to load from older separate snapshot files for back-compat reasons, even if we no longer track separate files after the one-time conversion). And if I make snapshots cleaner like that, then checkpoints can just start life with the saner approach, rather than being stuck with copying the older one-file-per-checkpoint approach that I implemented using copy-and-paste. Again, heavy copy-and-paste from snapshots, which DO lend themselves to non-linear DAGs of related snapshots. (If you create snapshot A, then run the guest, then create B, then revert to A, then run the guest and create C, then both B and C are children of A). I don't readily know if checkpoints lend themselves as easily to non-linear relations, or if my copy-and-paste is an instance of YAGNI. And it also makes me wonder if I would be better served by factoring out the snapshot non-linear relationships into some more reusable helper functions rather than just open-coding things in both files. The short answer is that there is a hash table (for O(1) name lookup) AND a linked-list relationship listing (for tracking DAG relationships, where one child is common, but more than one child is possible); and that the domain tracks a metaroot (a static instance with name==NULL) so that all other snapshots/checkpoints have nice properties of always having a parent (either another real snapshot as the parent, or the metaroot as the parent when the presentation to the user is that the snapshot is the root of a DAG with no user-created parent). If I improve anything here with better comments, I should also apply similar improvements to the snapshot code. Indeed, since there is such heavy copy-and-paste between the two, first refactoring virDomainSnapshot to be Object-ified with better helper routines, and then making virDomainCheckpoint share a common parent class with virDomainSnapshot to reuse those helper routines, rather than open-coded copy-and-paste is probably worth the effort. (I didn't do it initially in order to get a working demo out the door, but I also admit that adds technical debt, and I should really be trying to reduce it rather than adding to it). Yeah, I'll have to double-check what improvements have been made to snapshot in the meantime, and make sure I'm not regressing back to things the way they were before cleanups (since my copy-and-paste point is mostly from sometime around Sep-Oct 2018). Yeah, I know it's coming. From what I've seen, I'll like it, too. Same question for snapshots. So far, all modification of snapshots vs. current_snapshot have been under an appropriate API lock, but you do raise the point that I should be careful about it. In fact, I'm half-tempted to first fix virDomainSnapshotObjListPtr to subsume the current_snapshot designation rather than letting virDomainPtr track it independently, in which case this would need the same treatment. All the more reason to put the current object as part of the ObjListPtr, so that locking for adding/removing members from the list is guaranteed to be the same as locking for updating which member is current. Possibly not. I'll play the trimming game. No, statically allocated right here. It's a trick that let's me convert a user's possible multi-root forest: A - B - C D - E into a single-root DAG: metarooot - A - B - C - D - E and various operations that need to quickly find all the user-visible roots can just traverse the children of the metaroot. Again, heavily copy-pasted from snapshots, and probably worth first refactoring it into a common parent-class object with better comments, so that both snapshots and checkpoints can reuse the same parent, rather than open-coding things twice. I'll have to double-check if memset(,0) as a final step in clear would make any difference. Intentional. Size is not persistent, but dynamically computed every time you pass the VIR_DOMAIN_CHECKPOINT_XML_SIZE flag (and potentially constantly changing, for a running domain). In other words, it is an output-only flag, and not tied to the checkpoint itself, so much as the checkpoint XML being the easiest way to query that particular piece of per-disk runtime information in bulk. Yeah, snapshot should get it first, and then I should make sure my code here gets the same improvements. The user's XML for creating the checkpoint can do: <domaincheckpoint> <disks> <disk name='vdb'/> <disk name='vda'/> </disk> </domaincheckpoint> even though the corresponding <domain> at the same time declared things in the other order (vda before vdb); also, like snapshots, you may only want to take checkpoints on a subset of your disks (for example, if you don't care about changes to the OS disk because you can recreate the OS configuration, but DO care about changes to the user's database disk); if the <domaincheckpoint> lists only a subset of all disk names, this function exists to fill in the remaining array elements with sane defaults, so that the rest of the code can blindly process an entire array of disks (and just skip the disks that are not participating in the snapshot), rather than having to repeat checks for which disks are being manipulated. It's also a drawback of copying heavily from snapshots, which has the same sort of align function. So, back to that argument of having a common base class rather than open-coding duplicated efforts :) Yes (and same for snapshots), but a comment wouldn't hurt. Users wanting a checkpoint on only a subset of their disks, or a user messing with XML output and then feeding it back in through the REDEFINE flag. That's what the bitmap is supposed to track. The first pass sees which disk names the user passed, and figures out which index value those disks are in the corresponding domain definition (or errors out if the same disk is specified more than once or is not present in the domain); the second pass fills in array entries for all other disks that the user omitted. You are right that things might not be correlated until after the qsort, but after this point, iterating through def->disks[] should visit disks in the same order as iterating through def->dom->disks[]. Which I hope I addressed. Copy-paste from <domainsnapshot>. I don't know what else we might add to <parent>, but as snapshot left it open like that, it's easier to do the same here (and then things like virsh can reuse the same XPath queries for more code reuse when converting a listing from array form into tree form). Here, I've already improved the situation somewhat for snapshots, and so intend to flow those improvements into checkpoints for v5. But you've already got me wondering earlier in the series if we still want one file per checkpoint, or if I'm better off using VIR_DOMAIN_XML_CHECKPOINTS (where I don't need an internal <active> field after all) for how libvirt stores checkpoints persistently. Yeah, especially if I refactor the object code to make virDomainSnapshotObj and virDomainCheckpointObj have a common parent class, since the two are very similar (both are tied to a point-in-time <domain> capture, have a notion of one current object amid a set of more objects, and have a DAG relationship tracking parent/child pointers, with a possibility of more than one child depending on how reverts play with checkpoints in the future). Indeed, improving virDomainSnapshot first may be a wiser use of my time, while still getting this checkpoint code ready for inclusion. I blame the lack of comments in snapshot code. (Again, anything I fix here as a result of your comments, I plan to do to snapshots as well). Or two snapshots. The metaroot will ALWAYS be an empty placeholder object. It allows the rest of the code to have more than one independent user-visible root (depending on how reverting to an older snapshot ends up interplaying with checkpoints). virDomainCheckpointLookupByName(objlist, NULL) returns the metaroot, for callers that directly manipulate its first_child/nchildren fields. But as you point out elsewhere, refactoring that into common helper code for both snapshots and checkpoints to reuse via accessor functions (rather than direct field access) may be even smarter. The same XXX exists for snapshots (if we were to ever reconstruct qcow2 snapshots without relying on libvirt metadata). I don't see it as a showstopper for the initial implementation. Sadly, the virsh 'snapshot-list --tree' code DOES use it, and therefore so does the new 'checkpoint-list --tree' code. (On the bright side, it also means that if I refactor things into a common base class for both snapshots and checkpoints to reuse, I won't be open-coding the stuff twice). Yes, all faithfully copied from snapshots. But some of it might not be necessary - while snapshots HAS to cater to the older public API that returns a count or list of names (rather than the list of objects directly), the checkpoint code does not have that older API. Then again, hiding the common code into a common parent class rather than re-coding it can't hurt, other than the time it takes for a good refactoring. (But less technical debt now may be worth it, compared to the pain of having to revisit this down the road). NULL is the special case for getting at the metaroot (idea copied from snapshots), for the external code that manipulates the metaroot's nchildren/first_child directly. (And as you've pointed out above, hiding that in saner accessor functions in a common base class would be a worthwhile refactoring). No, metaroot is a placeholder which can never be created or deleted. Rather, the user's first checkpoint is a child of metaroot. For the DAG: A - B - C - D The descendends of A are B, C, D; the children of A are just B, D. The function returns 1 + 3 (for a total of 4 nodes visited), because: ActOnDescendent(A) => 1 + ForEachDescendent(A) => 1 + ForEachChild(A, ActOnDescendent) => 1 + ActOnDescendent(B) + ActOnDescendent(D) => 1 + 1 + ForEachDescendent(B) + 1 + ForEachDescendent(D) => 1 + 1 + ForEachChild(B, ActOnDescendent) + 1 + ForEachChild(D, ActOnDescendent) => 1 + 1 + ActOnDescendent(C) + 1 => 1 + 1 + 1 + ForEachDescendent(C) + 1 => 1 + 1 + 1 + ForEachChild(C, ActOnDescendent) + 1 => 1 + 1 + 1 + 1 the ForEachDescendent Yeah, it's hairy, but it's a straight copy from snapshots, so I should really refactor that first into a common base class. The simplest implementation is that you are indeed creating a linear link of checkpoints. But I don't know how snapshot reversion will play into the future (for qemu, or more importantly, for other domains that may track checkpoints differently via tracking generation ids on every cluster rather than a bitmap of which clusters were changed since a point in time). And since reverting to a snapshot IS a very likely action, I'd rather leave the door open to having a forest of DAGs for checkpoints just as we permit a forest of DAGs for snapshots (especially if I enhance virDomainSnapshotCreateXML to also capture a checkpoint at that time). The snapshot operation is definitely tree-based, but the linked list approach of first_child/next_sibling was sufficient rather than having a full-on hash table of children. Easy - see my recent patch on VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE_LIST. When we read in multiple checkpoints at once (and not necessarily in topological order), we then do a post-parse pass over the list of checkpoints to reconstruct the parent/child relationship pointers. I track: A - B - C - D - E - F - G - H by having B->nchildren == 2, B->first_child = C, C->sibling = D, D->sibling = NULL, B->sibling = E, E->sibling = F, F->first_child = G, G->sibling = H, H->sibling = NULL, F->sibling = NULL Not so much inserting in the middle of the list, but at the front (any time I associate a child with its parent, the child's sibling pointer takes over the parent's original first_child, the parent's nchildren increments, and the parent's first_child becomes the newly inserted child). The lists are generally short (in fact, never more than one element long if you don't branch into having multiple children), so it isn't worth keeping a pointer to the tail of a list. Rebuilding the tree is needed for when libvirtd is first started (reading multiple files of <domainsnapshot>/<domaincheckpoint>), as well as when doing REDEFINE_LIST (which has a precondition of no existing snapshots because it is about to read in multiple and then rebuild the relationships all at once). Copied from snapshots; if there's a latent bug there, it should be a separate fix. Meanwhile, refactoring to a common base class should get rid of the worst of understanding this code (it's easier to check if code motion is correct than it is to see if a duplicated open-code repeat is correct). Except for the fact that the public APIs have nice filters for listing roots only vs. roots and all their descendents, for both the domain and for a subtree rooted in a particular checkpoint, and that virsh already takes advantage of those filters in implementing its --tree output. Works if we aren't filtering to a particular subtree, but if any filtering is involved, then you don't want the return value to include the objects that were filtered out. No, because when the answer is 0,... ...we still return a valid NULL-terminated list. Thanks for the feedback so far. (And yes, I was totally expecting your point that refactoring the snapshot code to be cleaner, rather than just blindly copying its existing code into checkpoints, is a better reduction of overall technical debt, even if it will cost more time to get the patches polished). Yeah, and now you see why I have 'wip' because testing is not finalized :) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org