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(a)gmx.de>
> > > Cc: Cole Robinson <crobinso(a)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