Description of problem: Option "--tree" can't correctly display external snapshot inheritance relationship after executing "snapshot-revert" failed Version-Release number of selected component (if applicable): libvirt-5.5.0-2.virtcov.el8.x86_64 How reproducible: 100% Steps to Reproduce: 1.Create several disk-only snapshot for guest # virsh snapshot-create-as demo s1 --disk-only Domain snapshot s1 created # virsh snapshot-create-as demo s2 --disk-only Domain snapshot s2 created 2.Check the snapshot list with option "--tree" # virsh snapshot-list demo --tree s1 | +- s2 3.Execute the command "snapshot-revert" and create a new snapshot # virsh snapshot-revert demo s1 error:unsupported configuration: revert to external snapshot not supported yet # virsh snapshot-current demo --name s2 # virsh snapshot-create-as demo s3 --disk-only Domain snapshot s3 created # virsh snapshot-list demo --tree s1 | +- s2 s3 Actual results: As step 3 Expected results: The new snapshot s3 should be at the bottom of the s2 chain and should not be juxtaposed with s1. The correct look should be: s1 | +- s2 | +- s3 Additional info:
I have tested it in libvirt-5.1.0-9.el8.x86_64. It works well like s1 | +- s2 | +- s3 in the last step. So the patch below may be the main cause of this bug. A recent change in snapshot revert introduced since libvirt-5.2: commit 4819f54bd3b2417847fe40ead03e3b1a6f5d334e Author: Eric Blake <eblake> Date: Thu Mar 21 15:00:08 2019 -0500 snapshot: Track current snapshot in virDomainSnapshotObjList It is easier to track the current snapshot as part of the list of snapshots. In particular, doing so lets us guarantee that the current snapshot is cleared if that snapshot is removed from the list (rather than depending on the caller to do so, and risking a use-after-free problem, such as the one recently patched in 1db9d0efbf). This requires the addition of several new accessor functions, as well as a useful return type for virDomainSnapshotObjListRemove(). A few error handling sites that were previously setting vm->current_snapshot = NULL can now be dropped, because the previous function call has now done it already. Also, qemuDomainRevertToSnapshot() was setting the current vm twice, so keep only the one used on the success path. Signed-off-by: Eric Blake <eblake> Reviewed-by: John Ferlan <jferlan>
Confirmed that this is a regression, but it was this commit that introduced it (although both the one you guessed and this one landed in 5.2). commit f105627992e2bfe5ff6a56d21f301e1252a24eb5 Author: Eric Blake <eblake> Date: Mon Mar 18 22:56:19 2019 -0500 snapshot: Drop virDomainSnapshotDef.current Upstream patch will be posted soon.
Upstream patch https://www.redhat.com/archives/libvir-list/2019-September/msg00364.html
Fixed upstream in: commit 4933445a18f689f5cc16251eda322d643a3a5838 Author: Eric Blake <eblake> Date: Mon Sep 9 15:52:42 2019 -0500 qemu: Fix regression in snapshot-revert Commit f10562799 introduced a regression: if reverting to a snapshot fails early (such as when we refuse to revert to an external snapshot), we lose track of the domain's current snapshot. Before that patch, we were tracking the notion of the domain's current snapshot via two means: vm->current_snapshot (which was left untouched on early exit) and snap->def->current (which only controls what gets written to XML to remember snapshots across libvirtd restarts). That patch was fixing a real bug: if a revert operation failed early, later questions from the same libvirtd did not see any change to the current snapsthot, but restarting libvirtd would now claim there is no current snapshot. But it fixed it in the wrong direction, in that the current snapshot was forgotten unconditionally, rather than only when the snapshot to revert to has a chance of being useful. It didn't help that the code after that patch had two separate spots clearing the old notion of the current snapshot - one after determining the snapshot to revert to was viable, the other unconditionally on all failure exit paths. At any rate, the fix is simple: drop the unconditional cleanup on error paths, and rely only on the normal cleanup after early checks. Sadly, it is not possible to test this bug in the existing tests/virsh-snapshot, as the test driver does not have the same prohibition against reverting to an external snapshot as the qemu driver. See: https://bugzilla.redhat.com/1738747 Signed-off-by: Eric Blake <eblake> Message-Id: <20190909205242.15406-1-eblake> Reviewed-by: Daniel Henrique Barboza <danielhb413> v5.7.0-99-g4933445a18
Verified on: libvirt-6.0.0-2.virtcov.el8.x86_64 qemu-kvm-4.2.0-8.module+el8.2.0+5607+dc756904.x86_64 Step: 1.Make external snapshot for the guest: # virsh snapshot-create-as demo --disk-only --diskspec vdf,file=/tmp/iscsi.s1 Domain snapshot 1580785364 created # virsh snapshot-create-as demo --disk-only --diskspec vdf,file=/tmp/iscsi.s2 Domain snapshot 1580785367 created 2.Try to revert the snapshot and then create a new snapshot: # virsh snapshot-revert demo 1580785364 error: unsupported configuration: revert to external snapshot not supported yet # virsh snapshot-create-as demo --disk-only --diskspec vdf,file=/tmp/iscsi.s3 Domain snapshot 1580785403 created # virsh snapshot-list demo --tree 1580785364 | +- 1580785367 | +- 1580785403 3.Continue to make snapshot and try step2 again: # virsh snapshot-create-as demo s4 --disk-only Domain snapshot s4 created # virsh snapshot-create-as demo s5 --disk-only Domain snapshot s5 created # virsh snapshot-list demo --tree 1580785364 | +- 1580785367 | +- 1580785403 | +- s4 | +- s5 # virsh snapshot-revert demo s4 error: unsupported configuration: revert to external snapshot not supported yet # virsh snapshot-create-as demo s6 --disk-only Domain snapshot s6 created [root@jgao-test1 ~]# virsh snapshot-list demo --tree 1580785364 | +- 1580785367 | +- 1580785403 | +- s4 | +- s5 | +- s6 Tree logic works well after failing to revert snapshot.
Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory, and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://access.redhat.com/errata/RHBA-2020:2017