[virt-tools-list] [virt-manager RFC PATCH 0/2] Warn about saved state behaviour with snapshots

[Michael Weiser]

Hello Cole,

I've done a bit of work on the issue of saved memory state not becoming
part of snapshots:

On 10/25/19 2:28 PM, Michael Weiser wrote:

> * Run/Revert of a snapshot should be rejected if the VM is in the
> 'Saved' state, aka has been 'virsh managedsave'. This should be done at
> the libvirt level for completeness IMO. Maybe the API needs a flag to
> override this behavior if users know what they are doing

VIR_ERR_SNAPSHOT_REVERT_RISKY seems to be meant for exactly this kind of
thing and can be overridden using VIR_DOMAIN_SNAPSHOT_REVERT_FORCE. A
number of cases are already handled that way in
src/qemu/qemu_driver.c:qemuDomainRevertToSnapshot().

I've added this small patch to my local install of libvirt:

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bad2fb52f3..560e35beba 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16578,6 +16578,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
                            _("must respawn qemu to start inactive snapshot"));
             goto endjob;
         }
+        if (vm->hasManagedSave &&
+            !(snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING ||
+              snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED)) {
+            virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s",
+                           _("revert to snapshot while there is a managed "
+                             "saved state will cause corruption when run, "
+                             "remove saved state first"));
+            goto endjob;
+        }
     }
 
     if (snap->def->dom) {

Without additional changes it produces the following error message
dialog from virt-manager when trying to restore a non-running snapshot
while there's saved state:

Error running snapshot 'snapshot1': revert requires force: revert to snapshot while there is a managed saved state will cause corruption when run, remove saved state first

Traceback (most recent call last):
  File "/usr/share/virt-manager/virtManager/asyncjob.py", line 75, in cb_wrapper
    callback(asyncjob, *args, **kwargs)
  File "/usr/share/virt-manager/virtManager/asyncjob.py", line 111, in tmpcb
    callback(*args, **kwargs)
  File "/usr/share/virt-manager/virtManager/object/libvirtobject.py", line 66, in newfn
    ret = fn(self, *args, **kwargs)
  File "/usr/share/virt-manager/virtManager/object/domain.py", line 1055, in revert_to_snapshot
    self._backend.revertToSnapshot(snap.get_backend())
  File "/usr/lib64/python3.6/site-packages/libvirt.py", line 2088, in revertToSnapshot
    if ret == -1: raise libvirtError ('virDomainRevertToSnapshot() failed', dom=self)
libvirt.libvirtError: revert requires force: revert to snapshot while there is a managed saved state will cause corruption when run, remove saved state first

Do you think this could go upstream as-is (plus a proper commit message
of course)?

> * virt-manager prompts to discard saved state first otherwise it doesn't
> all snapshot revert.

> * Maybe reject snapshot creation when VM is in 'saved' state too, to
> avoid ambiguity, but it's likely not as bad.

The two patches of this series add confirmation/warning dialogs in the
respective places. I'm not a native speaker and tend to be on the
verbose side in this kind of thing. Please let me know if and possibly
how the messages could be improved. Once we've settled on a wording I
can provide German translations as well.

Michael Weiser (2):
  details: snapshots: Drop saved state on restore
  details: snapshots: Warn of saved state on creation

 virtManager/details/snapshots.py | 25 +++++++++++++++++++++++++
 virtManager/object/domain.py     |  6 ++++++
 2 files changed, 31 insertions(+)

-- 
2.24.1