block/monitor/block-hmp-cmds.c | 7 +- block/snapshot.c | 233 ++++++++++++++------- include/block/snapshot.h | 19 +- include/migration/snapshot.h | 10 +- migration/savevm.c | 258 +++++++++++++++++++---- monitor/hmp-cmds.c | 11 +- qapi/job.json | 9 +- qapi/migration.json | 135 ++++++++++++ replay/replay-snapshot.c | 4 +- softmmu/vl.c | 2 +- tests/qemu-iotests/267.out | 14 +- tests/qemu-iotests/310 | 255 +++++++++++++++++++++++ tests/qemu-iotests/310.out | 369 +++++++++++++++++++++++++++++++++ tests/qemu-iotests/common.qemu | 107 +++++++++- tests/qemu-iotests/group | 1 + 15 files changed, 1289 insertions(+), 145 deletions(-) create mode 100755 tests/qemu-iotests/310 create mode 100644 tests/qemu-iotests/310.out
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 When QMP was first introduced some 10+ years ago now, the snapshot related commands (savevm/loadvm/delvm) were not converted. This was primarily because their implementation causes blocking of the thread running the monitor commands. This was (and still is) considered undesirable behaviour both in HMP and QMP. In theory someone was supposed to fix this flaw at some point in the past 10 years and bring them into the QMP world. Sadly, thus far it hasn't happened as people always had more important things to work on. Enterprise apps were much more interested in external snapshots than internal snapshots as they have many more features. Meanwhile users still want to use internal snapshots as there is a certainly simplicity in having everything self-contained in one image, even though it has limitations. Thus the apps that end up executing the savevm/loadvm/delvm via the "human-monitor-command" QMP command. IOW, the problematic blocking behaviour that was one of the reasons for not having savevm/loadvm/delvm in QMP is experienced by applications regardless. By not portting the commands to QMP due to one design flaw, we've forced apps and users to suffer from other design flaws of HMP ( bad error reporting, strong type checking of args, no introspection) for an additional 10 years. This feels rather sub-optimal :-( In practice users don't appear to care strongly about the fact that these commands block the VM while they run. I might have seen one bug report about it, but it certainly isn't something that comes up as a frequent topic except among us QEMU maintainers. Users do care about having access to the snapshot feature. Where I am seeing frequent complaints is wrt the use of OVMF combined with snapshots which has some serious pain points. This is getting worse as the push to ditch legacy BIOS in favour of UEFI gain momentum both across OS vendors and mgmt apps. Solving it requires new parameters to the commands, but doing this in HMP is super unappealing. After 10 years, I think it is time for us to be a little pragmatic about our handling of snapshots commands. My desire is that libvirt should never use "human-monitor-command" under any circumstances, because of the inherant flaws in HMP as a protocol for machine consumption. Thus in this series I'm proposing a fairly direct mapping of the existing HMP commands for savevm/loadvm/delvm into QMP as a first step. This does not solve the blocking thread problem, but it does put in a place a design using the jobs framework which can facilitate solving it later. It does also solve the error reporting, type checking and introspection problems inherant to HMP. So we're winning on 3 out of the 4 problems, and pushed apps to a QMP design that will let us solve the last remaining problem. With a QMP variant, we reasonably deal with the problems related to OVMF: - 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. These are solved by adding two parameters to the commands. The first is a block device node name that identifies the image to store vmstate in, and the second is a list of node names to include for the snapshots. If the list of nodes isn't given, it falls back to the historical behaviour of using all disks matching some undocumented criteria. 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 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 (7): migration: improve error reporting of block driver state name 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 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 | 233 ++++++++++++++------- include/block/snapshot.h | 19 +- include/migration/snapshot.h | 10 +- migration/savevm.c | 258 +++++++++++++++++++---- monitor/hmp-cmds.c | 11 +- qapi/job.json | 9 +- qapi/migration.json | 135 ++++++++++++ replay/replay-snapshot.c | 4 +- softmmu/vl.c | 2 +- tests/qemu-iotests/267.out | 14 +- tests/qemu-iotests/310 | 255 +++++++++++++++++++++++ tests/qemu-iotests/310.out | 369 +++++++++++++++++++++++++++++++++ tests/qemu-iotests/common.qemu | 107 +++++++++- tests/qemu-iotests/group | 1 + 15 files changed, 1289 insertions(+), 145 deletions(-) create mode 100755 tests/qemu-iotests/310 create mode 100644 tests/qemu-iotests/310.out --=20 2.26.2
Kevin: While we're still arguing about details of the last commit; can we get the first few commits in - they seem to be generally nice cleanups/error handling. Dave * Daniel P. Berrangé (berrange@redhat.com) wrote: > 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 > > When QMP was first introduced some 10+ years ago now, the snapshot > related commands (savevm/loadvm/delvm) were not converted. This was > primarily because their implementation causes blocking of the thread > running the monitor commands. This was (and still is) considered > undesirable behaviour both in HMP and QMP. > > In theory someone was supposed to fix this flaw at some point in the > past 10 years and bring them into the QMP world. Sadly, thus far it > hasn't happened as people always had more important things to work > on. Enterprise apps were much more interested in external snapshots > than internal snapshots as they have many more features. > > Meanwhile users still want to use internal snapshots as there is > a certainly simplicity in having everything self-contained in one > image, even though it has limitations. Thus the apps that end up > executing the savevm/loadvm/delvm via the "human-monitor-command" > QMP command. > > IOW, the problematic blocking behaviour that was one of the reasons > for not having savevm/loadvm/delvm in QMP is experienced by applications > regardless. By not portting the commands to QMP due to one design flaw, > we've forced apps and users to suffer from other design flaws of HMP ( > bad error reporting, strong type checking of args, no introspection) for > an additional 10 years. This feels rather sub-optimal :-( > > In practice users don't appear to care strongly about the fact that these > commands block the VM while they run. I might have seen one bug report > about it, but it certainly isn't something that comes up as a frequent > topic except among us QEMU maintainers. Users do care about having > access to the snapshot feature. > > Where I am seeing frequent complaints is wrt the use of OVMF combined > with snapshots which has some serious pain points. This is getting worse > as the push to ditch legacy BIOS in favour of UEFI gain momentum both > across OS vendors and mgmt apps. Solving it requires new parameters to > the commands, but doing this in HMP is super unappealing. > > After 10 years, I think it is time for us to be a little pragmatic about > our handling of snapshots commands. My desire is that libvirt should never > use "human-monitor-command" under any circumstances, because of the > inherant flaws in HMP as a protocol for machine consumption. > > Thus in this series I'm proposing a fairly direct mapping of the existing > HMP commands for savevm/loadvm/delvm into QMP as a first step. This does > not solve the blocking thread problem, but it does put in a place a > design using the jobs framework which can facilitate solving it later. > It does also solve the error reporting, type checking and introspection > problems inherant to HMP. So we're winning on 3 out of the 4 problems, > and pushed apps to a QMP design that will let us solve the last > remaining problem. > > With a QMP variant, we reasonably deal with the problems related to OVMF: > > - 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. > > These are solved by adding two parameters to the commands. The first is > a block device node name that identifies the image to store vmstate in, > and the second is a list of node names to include for the snapshots. > If the list of nodes isn't given, it falls back to the historical > behaviour of using all disks matching some undocumented criteria. > > 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 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 (7): > migration: improve error reporting of block driver state name > 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 > 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 | 233 ++++++++++++++------- > include/block/snapshot.h | 19 +- > include/migration/snapshot.h | 10 +- > migration/savevm.c | 258 +++++++++++++++++++---- > monitor/hmp-cmds.c | 11 +- > qapi/job.json | 9 +- > qapi/migration.json | 135 ++++++++++++ > replay/replay-snapshot.c | 4 +- > softmmu/vl.c | 2 +- > tests/qemu-iotests/267.out | 14 +- > tests/qemu-iotests/310 | 255 +++++++++++++++++++++++ > tests/qemu-iotests/310.out | 369 +++++++++++++++++++++++++++++++++ > tests/qemu-iotests/common.qemu | 107 +++++++++- > tests/qemu-iotests/group | 1 + > 15 files changed, 1289 insertions(+), 145 deletions(-) > create mode 100755 tests/qemu-iotests/310 > create mode 100644 tests/qemu-iotests/310.out > > --=20 > 2.26.2 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Sep 11, 2020 at 12:52:04PM +0100, Dr. David Alan Gilbert wrote: > Kevin: > While we're still arguing about details of the last commit; can we get > the first few commits in - they seem to be generally nice cleanups/error > handling. I'm going to try to send a update next week which just cuts out the features that are causing disagreements. Someone else can worry about them later... 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 :|
© 2016 - 2024 Red Hat, Inc.