Re: [PATCH v10 12/12] migration: introduce snapshot-{save,load,delete} QMP commands

[Eric Blake]

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 <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@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