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é (berra...@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 / dgilb...@redhat.com / Manchester, UK
