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

[Cole Robinson]

On 12/19/19 4:52 PM, Michael Weiser wrote:
> 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)?
> 

Ah nice, yes that looks good to me, I didn't know about the RISKY flag.
Other folks are the experts in this area though. Please propose that as
a patch to libvir-list at redhat.com and CC me. A lot of people will be
offline for the holidays for a while so if you don't get a response
until mid january don't be surprised.

- Cole