[libvirt] [PATCH] qemu: Warn of restore with managed save being risky

[Michael Weiser]

Hello Daniel,

On Thu, Jan 02, 2020 at 09:58:19AM -0300, Daniel Henrique Barboza wrote:

> > > > This patch marks the restore operation as risky at the libvirt level,
> > > > requiring the user to remove the saved memory state first or force the
> > > > operation.
> > > > 
> > > > [1] https://www.redhat.com/archives/virt-tools-list/2019-November/msg00011.html
> > > > [2] https://www.redhat.com/archives/virt-tools-list/2019-December/msg00049.html
> > > > 
> > > > Signed-off-by: Michael Weiser <michael.weiser at gmx.de>
> > > > Cc: Cole Robinson <crobinso at redhat.com>
> > > > ---
> > > As said in [1], I agree that the API needs a flag override to allow the user
> > > to roll with it despite the risks. Given that this is somewhat a corner case,
> > > I also believe that such override can go in a separated patch/series later
> > > on.
> > 
> > Do you mean a separate override flag beyond
> > VIR_DOMAIN_SNAPSHOT_REVERT_FORCE? Or should I just update the commit
> > message to make clear how the user can force the operation?
> Ah, I overlooked the "or force the operation" part of the commit message after
> reading the discussions in [1] and [2].

> Instead of updating the commit message, I think you can update the error message
> to mention the '--force' option, e.g. :


> +            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 or use the "
> +                             "--force option"));

I'd rather not reference virsh command options in the error message as
it would be highly confusing in any other context. For example, python
clients get the error message wrapped in an exception, augmented already
by a prefix telling them they need to force the operation:

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

The same is actually the case for virsh already:

virsh # snapshot-revert debian --snapshotname snapshot1
error: revert requires force: revert to snapshot while there is a
managed saved state will cause corruption when run, remove saved state
first

virsh # 

We could of course reword to better take context and prefix into
account, e.g.:

error: revert requires force: Removal of existing managed saved state
strongly recommended to avoid corruption

Any ideas?

> I also noticed that the man page for 'snapshot-revert' mentions two cases where
> the use of --force is required. One of them talks about snapshots created using old
> Libvirt versions. The other goes as follows:

> ----
> (docs/manpages/virsh.rst)


> The other is the case of reverting from a running domain to an active state
> where a new hypervisor has to be created rather than reusing the existing
> hypervisor, because it implies drawbacks such as breaking any existing
> VNC or Spice connections; this condition happens with an active
> snapshot that uses a provably incompatible configuration, as well as
> with an inactive snapshot that is combined with the --start or
> --pause flag.
> ----

> I am almost certain that scenario you're handling here is not covered by this
> excerpt. In this case, since you're adding a new '--force' condition, I believe a
> second patch adding this new '--force' condition in the documentation is a
> nice touch.

Will do. v2 coming as soon as we've agreed on where to go with the error message.
-- 
Thanks,
Michael