On 2/2/21 9:41 AM, Daniel P. Berrangé wrote:
> savevm, loadvm and delvm are some of the few HMP commands that have never
> been converted to use QMP. The reasons for the lack of conversion are
> that they blocked execution of the event thread, and the semantics
> around choice of disks were ill-defined.
>
> Note that the existing "query-named-block-nodes" can be used to query
> what snapshots currently exist for block nodes.
>
> Acked-by: Markus Armbruster <arm...@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> ---
> migration/savevm.c | 184 +++++++
> qapi/job.json | 9 +-
> qapi/migration.json | 157 ++++++
> .../tests/internal-snapshots-qapi | 386 +++++++++++++
> .../tests/internal-snapshots-qapi.out | 520 ++++++++++++++++++
Not this patch's fault: I find the name tests/qemu-iotests/tests/name to
be rather long and a bit repetitive; maybe we want to rename the
directory structure to something simpler, like:
tests/iotests/name
(that is, move the named tests into a sibling directory of
qemu-iotests/check, rather than a subdirectory). And maybe rename
qemu-iotests/check to something that requires less typing. Oh, and
while I'm asking for rainbows and ponies, being able to run check from
the same directory where I run make, instead of having to change
directories, would be nice. But as I said, that's a wish list for a
separate series.
For _this_ patch,
> +++ b/migration/savevm.c
> +++ b/qapi/migration.json
> +# Example:
> +#
> +# -> { "execute": "snapshot-save",
> +# "data": {
> +# "job-id": "snapsave0",
> +# "tag": "my-snap",
> +# "vmstate": "disk0",
> +# "devices": ["disk0", "disk1"]
> +# }
> +# }
> +# <- { "return": { } }
> +# <- {"event": "JOB_STATUS_CHANGE",
> +# "data": {"status": "created", "id": "snapsave0"}}
Nice example here with events...
> +##
> +# @snapshot-delete:
> +#
> +# Delete a VM snapshot
> +#
> +# @job-id: identifier for the newly created job
> +# @tag: name of the snapshot to delete.
> +# @devices: list of block device node names to delete a snapshot from
> +#
> +# Applications should not assume that the snapshot save is complete
> +# when this command returns. The job commands / events must be used
> +# to determine completion and to fetch details of any errors that arise.
...which makes this paragraph feel out of place: there is no snapshot
save going on during snapshot-delete, and...
> +#
> +# Returns: nothing
> +#
> +# Example:
> +#
> +# -> { "execute": "snapshot-delete",
> +# "data": {
> +# "job-id": "snapdelete0",
> +# "tag": "my-snap",
> +# "devices": ["disk0", "disk1"]
> +# }
> +# }
> +# <- { "return": { } }
...the example shows no events. On the other hand...
> +++ b/tests/qemu-iotests/tests/internal-snapshots-qapi
> @@ -0,0 +1,386 @@
> +#!/usr/bin/env bash
> +# group: rw auto quick snapshot
> +#
> +_cleanup()
> +{
> + _cleanup_qemu
> + _cleanup_test_img
> + TEST_IMG="$TEST_IMG.alt1" _cleanup_test_img
> + TEST_IMG="$TEST_IMG.alt2" _cleanup_test_img
> + rm -f "$SOCK_DIR/nbd"
$SOCK_DIR/nbd seems like a rather generic name, liable to collide with
other tests. Oh right, we still haven't improved './check' to be able
to run tests in parallel each in their own subdirectory, so we aren't
pulling the rug out from any other parallel test because there are no
other parallel tests.
> +run_delete()
> +{
> + local job=$1
> + local devices=$2
> + local fail=$3
> +
> + _send_qemu_cmd $QEMU_HANDLE "{\"execute\": \"snapshot-delete\",
> + \"arguments\": {
> + \"job-id\": \"$job\",
> + \"tag\": \"snap0\",
> + \"devices\": $devices}}" "return"
> + if [ $fail = 0 ]; then
> + # job status: waiting, pending
> + wait_job $job "JOB_STATUS_CHANGE" "JOB_STATUS_CHANGE"
> + else
> + # job status: aborting
> + wait_job $job "JOB_STATUS_CHANGE"
...you ARE testing that it uses events. Looks like you have a tweak to
make to the QAPI docs still.
> +echo
> +echo "===== Snapshot bad error reporting to stderr ====="
> +echo
> +
> +# This demonstrates that we're not capturing vmstate loading failures
> +# into QMP errors, they're ending up in stderr instead. vmstate needs
> +# to report errors via Error object but that is a major piece of work
> +# for the future. This test case's expected output log will need
> +# adjusting when that is done.
At least you documented what to expect down the road ;)
Overall, we're really close. If you post the necessary tweaks to squash
in to the migration.json file, I could give R-b.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
