Hey, It seems that there's an issue of how external disk snapshots currently work. The snapshot definition xml[1] that is supplied to the API has a list of disks each of which specify where the new overlay disks should be placed. This leads to ambiguities when we want to revert the domain to a snapshot with children. Consider the following situation: 0. Initial state: Domain definition: disk at disk-base.qcow2 1. We create a snapshot snap1 placing the overlay at disk-snap1.qcow2 Domain definition: disk at disk-snap1.qcow2 Disks: - disk-base.qcow2 (read-only) - disk-snap1.qcow2 (overlay, base: disk-base.qcow2) Snapshots: - snap1 (snapshot domain disk list: disk-base.qcow2, snapshot disk list: disk-snap1.qcow2) 2. We create another snapshot snap2 placing the overlay at disk-snap2.qcow2 VM definition: disk at disk-snap2.qcow2 Disks: - disk-base.qcow2 (read-only) - disk-snap1.qcow2 (overlay, base: disk-base.qcow2, read-only) - disk-snap2.qcow2 (overlay, base: disk-snap1.qcow2) Snapshots: - snap1 (snapshot domain disk list: disk-base.qcow2, snapshot disk list: disk-snap1.qcow2) - snap2 (snapshot domain disk list: disk-snap1.qcow2, snapshot disk list: disk-snap2.qcow2) Now what happens if we want to revert to snap1? We can't use any of the paths that snap1 itself references: both disk-base.qcow2 and disk-snap1.qcow2 are read-only as part of the backing chain for disks used by snap2. Possible options are these: - we can reuse disk-snap2.qcow2 which we no longer need as it contains overlay on top of snap2. This is non-optimal because now the path of the disk after revert depends on the disk path before it. - we can think of some name with the expectation that the user will change it later anyway. I think that both of the above are non-optimal because we don't actually revert to the exact state we took the snapshot of. The user is effectively no longer in control of the disk path after the revert. One of possible solutions would be add additional flag during the snapshot creation that would change the current behavior. Instead of creating a new disk for the overlay we could move the base disk to the specified path and create the overlay in its place. The advantages of this behavior would be the following: - the path to the current disk used in the domain would not change when taking snapshots. This way we could avoid the problem listed above and easily revert to any snapshot. - the confusion in naming of the overlay disks would be reduced. Currently the path specified when creating snapshot refers to the overlay image. Most of the time user does not know what will be changed in the domain until the next snapshot. So any path he chooses will likely not reflect what will end up in the overlay at the time of the next snapshot. If it was possible to specify the path where the current disk should be put instead, then the user could just name it after the snapshot and it would correctly represent the contents of that disk. The major disadvantage of the approach is that it would introduce extra complexity in the presently simple interface and also the underlying code. I've completely disregarded the case of taking snapshots while the domain is running, so there might be hidden complications there as well. Personally, I don't mind the current situation too much, but if libvirt developers think it's a thing worth fixing I would be willing to work on it. Any opinions?

On 11/2/18 8:49 AM, Povilas Kanapickas wrote: > On 21/10/2018 21:31, Povilas Kanapickas wrote: >> Hey, >> >> It seems that there's an issue of how external disk snapshots currently >> work. The snapshot definition xml[1] that is supplied to the API has a >> list of disks each of which specify where the new overlay disks should >> be placed. >> >> This leads to ambiguities when we want to revert the domain to a >> snapshot with children. Consider the following situation: >> >> 0. Initial state: >> Domain definition: disk at disk-base.qcow2 >> >> 1. We create a snapshot snap1 placing the overlay at disk-snap1.qcow2 >> >> Domain definition: disk at disk-snap1.qcow2 >> Disks: >> - disk-base.qcow2 (read-only) >> - disk-snap1.qcow2 (overlay, base: disk-base.qcow2) >> >> Snapshots: >> - snap1 (snapshot domain disk list: disk-base.qcow2, >> snapshot disk list: disk-snap1.qcow2) >> >> 2. We create another snapshot snap2 placing the overlay at disk-snap2.qcow2 >> >> VM definition: disk at disk-snap2.qcow2 >> Disks: >> - disk-base.qcow2 (read-only) >> - disk-snap1.qcow2 (overlay, base: disk-base.qcow2, read-only) >> - disk-snap2.qcow2 (overlay, base: disk-snap1.qcow2) >> >> Snapshots: >> - snap1 (snapshot domain disk list: disk-base.qcow2, >> snapshot disk list: disk-snap1.qcow2) >> - snap2 (snapshot domain disk list: disk-snap1.qcow2, >> snapshot disk list: disk-snap2.qcow2) >> >> Now what happens if we want to revert to snap1? We can't use any of the >> paths that snap1 itself references: both disk-base.qcow2 and >> disk-snap1.qcow2 are read-only as part of the backing chain for disks >> used by snap2. Possible options are these: >> >> - we can reuse disk-snap2.qcow2 which we no longer need as it contains >> overlay on top of snap2. This is non-optimal because now the path of the >> disk after revert depends on the disk path before it. >> >> - we can think of some name with the expectation that the user will >> change it later anyway. >> >> I think that both of the above are non-optimal because we don't actually >> revert to the exact state we took the snapshot of. The user is >> effectively no longer in control of the disk path after the revert. >> >> One of possible solutions would be add additional flag during the >> snapshot creation that would change the current behavior. Instead of >> creating a new disk for the overlay we could move the base disk to the >> specified path and create the overlay in its place. The advantages of >> this behavior would be the following: >> >> - the path to the current disk used in the domain would not change when >> taking snapshots. This way we could avoid the problem listed above and >> easily revert to any snapshot. >> >> - the confusion in naming of the overlay disks would be reduced. >> Currently the path specified when creating snapshot refers to the >> overlay image. Most of the time user does not know what will be changed >> in the domain until the next snapshot. So any path he chooses will >> likely not reflect what will end up in the overlay at the time of the >> next snapshot. If it was possible to specify the path where the current >> disk should be put instead, then the user could just name it after the >> snapshot and it would correctly represent the contents of that disk. >> >> The major disadvantage of the approach is that it would introduce extra >> complexity in the presently simple interface and also the underlying >> code. I've completely disregarded the case of taking snapshots while the >> domain is running, so there might be hidden complications there as well. >> >> Personally, I don't mind the current situation too much, but if libvirt >> developers think it's a thing worth fixing I would be willing to work on it. >> >> Any opinions? >> > > Friendly ping :-) > Please be aware that the typical Red Hat reviewers of the list were away at KVM Forum from Oct 22-26 and some took some extended vacation time after that. In particular, Peter Krempa who generally understand the snapshot code best is away. So, it may take longer than usual for the patches to be considered especially since beyond the Forum/Vacation time we had other rather major news to disrupt our normal work flow.