On Tue, Aug 31, 2021 at 16:46:25 +0300, Nikolay Shirokovskiy wrote: Hi, sorry for the late reply I was on PTO. One additional thing is that me and phrdina started discussing this (in person so I can't point you to a discussion) 2 weeks ago. I'll summarize the points we agreed upon. There's one implementation snag we currently have which complicates stuff. Let's expand your above scenario with the snapshot states: s s n n a a p p 1 2 p o │ │ r r base.qcow2 │ snap1.qcow2 │ snap2.qcow2 e i ───────────────────────► │ ────────────────────►│ ────────────────────► s g │ │ e i │ │ n n t A rather big set of problems which we will necessarily encounter when implementing this comes from the following: 1) users can modify the VM betwen snapshots or between the last snapshot and the abandoned state and this modification (e.g. changing a disk image) must be considered to prevent data loss when manipulating the images. 2) libvirt doesn't record the XML used to create the snapshot (e.g. snap1.xm) thus we don't actually know what the (historical) snapshot was actually recording. Coupled with the fact that the user could have changed the VM definition between 'snap1' and 'snap2' we can't even infer what the state was. Exactly. Reversion of external snapshots will necessarily require a new API, which will take a new "snapshot" XML describing the new overlays as you describe below. In the simple case such as with local files we can use the same algorithm for creating overlay filenames as we do when creating snapshots but generally we need to give the MGMT the ability to specify the overlay name. It's possible to avoid doing a combined "take snapshot&revert" operation as long as we have the possibility to take a snapshot and destroy the VM as running it after the point you want to return to is undesirable and pointless. One thing that you need to consider here is that when you are reverting to an arbitrary snapshot, the overlay files after the last snapshot (e.g. snap2.qcow2 (in my diagram), which is the state between snap2 and present) are abandoned and will never be used again. At this point we disucssed that we should be removing those as semantically it's the only point where we can do that as we know the state of the VM which will be abandoned. The problem lies in the fact that between 'snap2' and present the user could have exchanged disks and then we don't have enough metadata to create the overlays. One big additional caveat is that if the user exchanged disk images we _must not_ delete the changed image, so the metadata wrangling might be non-trivial in this case. That is an acceptable caveat for the user. As noted above as long as you can take a snapshot & atomicaly destroy the VM it's acceptable. For libvirt it's harder a bit as described above especially if we don't want to keep litering the disk with unused and invalid images. No in our discussion with Pavel we've ruled out all special cases as too much hassle to implement with little benefit. If a user wants to rever&abandon some state, they can do 2 operations of reverting and then deleting the unwanted snapshots. IMO we can't really change the naming any more, users baked scripts on top of this. Using a flag is also mostly pointless as users will keep creating snapshots without it. Thus I don't think this is a viable approach. The only advantage it would have that it would be simpler to vizualize what happened, but ideally users would not need to care about the steps and filenames at all. Yup, too much hassle, questionable outcome. Not worth it. I was briefly considering that the new snapshot api should also have provisions for specifying a snapshot XML to take the snapshot of the abandoned state, but as discussed it's not really necessary if you can do it in 2 steps. I don't see any other option. In case you have e.g. networked storage where we can't figure out stuff by ourselves you basically don't have other option. Yes, this would be possible, but quite fragile. But with regular snapshots without any magic it should work. One thing you've missed though is that deletion of snapshots now becomes quite a challenge. s s s n n n a a a p p p 1 2 3 o │ │ │ r base.qcow2 │ snap1.qcow2 │ snap2.qcow2 │ i ─────────────────► │ ────────────►│ ────────────►│ g │ │ │ i │ │ │ n │ │ p │ r │ alternatehistory.qcow2 e │ ────────────────────────────────► s │ e │ n t Specifically the premise is that we don't want to keep unnecessary images (e.g. the trivial solution for deleting 'snap1' would be to remove the metadata and keep base.qcow2 intact) as without the metadata it will become very hard to determine which images are actually still used. So in the above scenario: - User wants to delete snap3: 'snap2.qcow2' overlay becomes unreachable and should be deleted. - User wants to delete snap2: There are 2 options in this case: 1) block-pull snap1 into snap2 2) block-commit snap2 into snap1 The optimal solution is the one that transfers less data, but in the end either of them is equally good in terms of saved storage. - User wants to delete snap1: This is a bit harder: 1) commit into base.qcow2 would invalidate 'alternatehistory.qcow2' 2) pull of base into both snap1.qcow2 and alternatehistory.qcow2 is possible, but may yield big images 3) the last option is to forbid deletion of snapshots that has alternate histories The first thing we should do though is to forbid creation of mixed trees of internal and external snapshots because the matrix of stuff to handle would explode way beyond this already rather complex situation.

On Mon, Sep 13, 2021 at 17:52:49 +0300, Nikolay Shirokovskiy wrote: [...] Well, if the management deletes them (at least 'base.qcow2'), it's a problem because it can no longer be reverted to 'snap1'. As you've correctly pointed out 'snap1.qcow2' is unreachable and thus should have been deleted. Sure but programatically checking all the possibilities (e.g. in this scenario: s s n n a a p p 1 2 vda.qcow2 │ vda.snap1 │ vda.snap2 o ────────────► │ ──────────────────────► │ ────────── p r vdb.qcow2 │ vdb.snap2 r i ──────────────────────────────────────► │ ────────── e g vdc.qcow2 vdc.snap1 vdc.other │ vdc.snap2 s i ────────────► │ ─────────X ──────────► │ ────────── e n vdd.qcow2 │ vdc.snap1 vdc.snap1 │ vdd.snap2 n ────────────► │ ─────────X ──────────► │ ────────── t will become a bit of a nightmare. E.g. checking that 'vdc' was replaced by a different image if the names don't match might be feasible, but if a user just rewrites the image it's actually impossible for us to detect. This will require a bit more of thinking how to deal with certain situations and we might even need to be able to introduce a flag for reverting from an unsafe scenario such as: s n a p 1 vda.qcow2 │ vda.snap1 ────────────► │ ────────────────────── vdc.qcow2 vdc.snap1 vdc.other ────────────► │ ─────────X ────────── Even here we need to be careful, because reverting back to snap1 will possibly leak 'vdc.other'. To manage user expectations properly we might want to add a flag which will e.g. disallow reversion if files which arent recorded in 'snap1' are going to be left behind but we must never delete 'vdc.other' to prevent data loss. No, you are right. I forgot that we actually do, which is good. This will provide the possibility to do a lot of checks to prevent breaking user setup. This point is no longer applicable. We do record the snapshot XML too so this allows us to do checking whether the current configuration of the VM is consistent with the state when the snapshot was created. Basically the hardest part of this is to ensure that we don't remove or corrupt any image with user data which still might be needed. For simple VM definitions this is very simple but since we give the users quite a lot of freedom what to do (including replacing disks, taking partial snapshots, etc.) we then need to be prepared for a comparatively large number of scenarios we'll need to deal with. Unfortunately we're risking data loss here, so we need to approach it very carefully especially for scenarios where individual users use e.g. virsh to do management where they might not have tested whether everything works on throwaway VMs. [...] Yeah, both need to be implemented at the same time. We definitely plan to work on it, but I can't give you any time estimates yet. More importantly, since you are interested into this as well, it would be great if you could elaborate on how you want to use this if it's ready especially any special scenarios. For us the basic goal is to achieve feature parity between internal and external snapshots so that they can be used interchangably and eventually prefering external snapshots as the way forward.

On Thu, Sep 16, 2021 at 02:53:35PM +0300, Nikolay Shirokovskiy wrote: Hi Nikolay, I was assigned to work on this feature as Peter mentioned. I was getting familiar with the snapshot code and I'm starting working on this feature. Pavel