Bug 1738747 - Current snapshot shouldn't be set to null after "snapshot-revert" failed
Summary: Current snapshot shouldn't be set to null after "snapshot-revert" failed
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux Advanced Virtualization
Classification: Red Hat
Component: libvirt
Version: 8.1
Hardware: x86_64
OS: Linux
medium
medium
Target Milestone: rc
: 8.0
Assignee: Eric Blake
QA Contact: gaojianan
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-08-08 04:17 UTC by gaojianan
Modified: 2020-11-19 08:46 UTC (History)
8 users (show)

Fixed In Version: libvirt-6.0.0-1.el8
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-05-05 09:47:43 UTC
Type: Bug
Target Upstream Version:


Attachments (Terms of Use)

Description gaojianan 2019-08-08 04:17:41 UTC
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:

Comment 1 gaojianan 2019-08-08 07:43:52 UTC
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@redhat.com>
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@redhat.com>
    Reviewed-by: John Ferlan <jferlan@redhat.com>

Comment 2 Eric Blake 2019-09-09 20:44:38 UTC
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@redhat.com>
Date:   Mon Mar 18 22:56:19 2019 -0500

    snapshot: Drop virDomainSnapshotDef.current
    
Upstream patch will be posted soon.

Comment 4 Peter Krempa 2020-02-03 11:16:26 UTC
Fixed upstream in:

commit 4933445a18f689f5cc16251eda322d643a3a5838
Author: Eric Blake <eblake@redhat.com>
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@redhat.com>
    Message-Id: <20190909205242.15406-1-eblake@redhat.com>
    Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

v5.7.0-99-g4933445a18

Comment 7 gaojianan 2020-02-04 03:20:41 UTC
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.

Comment 9 errata-xmlrpc 2020-05-05 09:47:43 UTC
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


Note You need to log in before you can comment on or make changes to this bug.