mbox series

[v4,0/9] migration: bring improved savevm/loadvm/delvm to QMP

Message ID 20200915113523.2520317-1-berrange@redhat.com
Headers show
Series migration: bring improved savevm/loadvm/delvm to QMP | expand

Message

Daniel P. Berrangé Sept. 15, 2020, 11:35 a.m. UTC
v1: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00866.html
 v2: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg07523.html
 v3: https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg07076.html

This series aims to provide a better designed replacement for the
savevm/loadvm/delvm HMP commands, which despite their flaws continue
to be actively used in the QMP world via the HMP command passthrough
facility.

The main problems addressed are:

 - The logic to pick which disk to store the vmstate in is not
   satsifactory.

   The first block driver state cannot be assumed to be the root disk
   image, it might be OVMF varstore and we don't want to store vmstate
   in there.

 - The logic to decide which disks must be snapshotted is hardwired
   to all disks which are writable

   Again with OVMF there might be a writable varstore, but this can be
   raw rather than qcow2 format, and thus unable to be snapshotted.
   While users might wish to snapshot their varstore, in some/many/most
   cases it is entirely uneccessary. Users are blocked from snapshotting
   their VM though due to this varstore.

 - The commands are synchronous blocking execution and returning
   errors immediately.

   This is partially addressed by integrating with the job framework.
   This forces the client to use the async commands to determine
   the completion status or error message from the operations.

In the block code I've only dealt with node names for block devices, as
IIUC, this is all that libvirt should need in the -blockdev world it now
lives in. IOW, I've made not attempt to cope with people wanting to use
these QMP commands in combination with -drive args, as libvirt will
never use -drive with a QEMU new enough to have these new commands.

The main limitations of this current impl

 - The snapshot process runs serialized in the main thread. ie QEMU
   guest execution is blocked for the duration. The job framework
   lets us fix this in future without changing the QMP semantics
   exposed to the apps.

 - Most vmstate loading errors just go to stderr, as they are not
   using Error **errp reporting. Thus the job framework just
   reports a fairly generic message

     "Error -22 while loading VM state"

   Again this can be fixed later without changing the QMP semantics
   exposed to apps.

I've done some minimal work in libvirt to start to make use of the new
commands to validate their functionality, but this isn't finished yet.

My ultimate goal is to make the GNOME Boxes maintainer happy again by
having internal snapshots work with OVMF:

  https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
f45c5f64048f16a6e

Changed in v4:

 - Make the device lists mandatory, dropping all support for
   QEMU's built-in heuristics to select devices.

 - Improve some error reporting and I/O test coverage

Changed in v3:

 - Schedule a bottom half to escape from coroutine context in
   the jobs. This is needed because the locking in the snapshot
   code goes horribly wrong when run from a background coroutine
   instead of the main event thread.

 - Re-factor way we iterate over devices, so that we correctly
   report non-existant devices passed by the user over QMP.

 - Add QAPI docs notes about limitations wrt vmstate error
   reporting (it all goes to stderr not an Error **errp)
   so QMP only gets a fairly generic error message currently.

 - Add I/O test to validate many usage scenarios / errors

 - Add I/O test helpers to handle QMP events with a deterministic
   ordering

 - Ensure 'delete-snapshot' reports an error if requesting
   delete from devices that don't support snapshot, instead of
   silently succeeding with no erro.

Changed in v2:

 - Use new command names "snapshot-{load,save,delete}" to make it
   clear that these are different from the "savevm|loadvm|delvm"
   as they use the Job framework

 - Use an include list for block devs, not an exclude list

Daniel P. Berrang=C3=A9 (9):
  block: push error reporting into bdrv_all_*_snapshot functions
  migration: stop returning errno from load_snapshot()
  block: add ability to specify list of blockdevs during snapshot
  block: allow specifying name of block device for vmstate storage
  migration: control whether snapshots are ovewritten
  migration: wire up support for snapshot device selection
  migration: introduce a delete_snapshot wrapper
  iotests: add support for capturing and matching QMP events
  migration: introduce snapshot-{save,load,delete} QMP commands

 block/monitor/block-hmp-cmds.c |   7 +-
 block/snapshot.c               | 248 ++++++++++++++------
 include/block/snapshot.h       |  23 +-
 include/migration/snapshot.h   |  14 +-
 migration/savevm.c             | 260 +++++++++++++++++----
 monitor/hmp-cmds.c             |  11 +-
 qapi/job.json                  |   9 +-
 qapi/migration.json            | 120 ++++++++++
 replay/replay-snapshot.c       |   4 +-
 softmmu/vl.c                   |   2 +-
 tests/qemu-iotests/267.out     |  10 +-
 tests/qemu-iotests/310         | 338 +++++++++++++++++++++++++++
 tests/qemu-iotests/310.out     | 412 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/common.qemu | 107 ++++++++-
 tests/qemu-iotests/group       |   1 +
 15 files changed, 1425 insertions(+), 141 deletions(-)
 create mode 100755 tests/qemu-iotests/310
 create mode 100644 tests/qemu-iotests/310.out

--=20
2.26.2

Comments

Markus Armbruster Sept. 16, 2020, 7:47 a.m. UTC | #1
Daniel P. Berrangé <berrange@redhat.com> writes:

> The traditional HMP "savevm" command will overwrite an existing snapshot
> if it already exists with the requested name. This new flag allows this
> to be controlled allowing for safer behaviour with a future QMP command.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/migration/snapshot.h | 2 +-
>  migration/savevm.c           | 4 ++--
>  monitor/hmp-cmds.c           | 2 +-
>  replay/replay-snapshot.c     | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
> index c85b6ec75b..d7db1174ef 100644
> --- a/include/migration/snapshot.h
> +++ b/include/migration/snapshot.h
> @@ -15,7 +15,7 @@
>  #ifndef QEMU_MIGRATION_SNAPSHOT_H
>  #define QEMU_MIGRATION_SNAPSHOT_H
>  
> -int save_snapshot(const char *name, Error **errp);
> +int save_snapshot(const char *name, bool overwrite, Error **errp);
>  int load_snapshot(const char *name, Error **errp);
>  
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 76972d69b0..2025e3e579 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2658,7 +2658,7 @@ int qemu_load_device_state(QEMUFile *f)
>      return 0;
>  }
>  
> -int save_snapshot(const char *name, Error **errp)
> +int save_snapshot(const char *name, bool overwrite, Error **errp)
>  {
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -2685,7 +2685,7 @@ int save_snapshot(const char *name, Error **errp)
>      }
>  
>      /* Delete old snapshots of the same name */
> -    if (name) {
> +    if (name && overwrite) {
>          if (bdrv_all_delete_snapshot(name, false, NULL, errp) < 0) {
>              return ret;
>          }

Are you sure this is sane?

To see what happens, I set a breakpoint on this function, set overwrite
to false.  I got a *second* snapshot with the same ID.

> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 98c98ae182..c1b8533d0c 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1131,7 +1131,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
>  
> -    save_snapshot(qdict_get_try_str(qdict, "name"), &err);
> +    save_snapshot(qdict_get_try_str(qdict, "name"), true, &err);
>      hmp_handle_error(mon, err);
>  }
>  
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index e26fa4c892..8e7ff97d11 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -77,7 +77,7 @@ void replay_vmstate_init(void)
>  
>      if (replay_snapshot) {
>          if (replay_mode == REPLAY_MODE_RECORD) {
> -            if (save_snapshot(replay_snapshot, &err) != 0) {
> +            if (save_snapshot(replay_snapshot, true, &err) != 0) {
>                  error_report_err(err);
>                  error_report("Could not create snapshot for icount record");
>                  exit(1);
Daniel P. Berrangé Sept. 16, 2020, 8:25 a.m. UTC | #2
On Wed, Sep 16, 2020 at 09:47:32AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:

> 

> > The traditional HMP "savevm" command will overwrite an existing snapshot

> > if it already exists with the requested name. This new flag allows this

> > to be controlled allowing for safer behaviour with a future QMP command.

> >

> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

> > ---

> >  include/migration/snapshot.h | 2 +-

> >  migration/savevm.c           | 4 ++--

> >  monitor/hmp-cmds.c           | 2 +-

> >  replay/replay-snapshot.c     | 2 +-

> >  4 files changed, 5 insertions(+), 5 deletions(-)

> >

> > diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h

> > index c85b6ec75b..d7db1174ef 100644

> > --- a/include/migration/snapshot.h

> > +++ b/include/migration/snapshot.h

> > @@ -15,7 +15,7 @@

> >  #ifndef QEMU_MIGRATION_SNAPSHOT_H

> >  #define QEMU_MIGRATION_SNAPSHOT_H

> >  

> > -int save_snapshot(const char *name, Error **errp);

> > +int save_snapshot(const char *name, bool overwrite, Error **errp);

> >  int load_snapshot(const char *name, Error **errp);

> >  

> >  #endif

> > diff --git a/migration/savevm.c b/migration/savevm.c

> > index 76972d69b0..2025e3e579 100644

> > --- a/migration/savevm.c

> > +++ b/migration/savevm.c

> > @@ -2658,7 +2658,7 @@ int qemu_load_device_state(QEMUFile *f)

> >      return 0;

> >  }

> >  

> > -int save_snapshot(const char *name, Error **errp)

> > +int save_snapshot(const char *name, bool overwrite, Error **errp)

> >  {

> >      BlockDriverState *bs;

> >      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;

> > @@ -2685,7 +2685,7 @@ int save_snapshot(const char *name, Error **errp)

> >      }

> >  

> >      /* Delete old snapshots of the same name */

> > -    if (name) {

> > +    if (name && overwrite) {

> >          if (bdrv_all_delete_snapshot(name, false, NULL, errp) < 0) {

> >              return ret;

> >          }

> 

> Are you sure this is sane?

> 

> To see what happens, I set a breakpoint on this function, set overwrite

> to false.  I got a *second* snapshot with the same ID.


Sigh. No, it doesn't do what I was meaning it to, and I forgot to add
a test case for this scenario in the last patch.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|