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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Sep 16, 2020 at 10:17:52AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > 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.
>> >
>> > Despite this downside, however, libvirt and applications using libvirt
>> > have used these commands for as long as QMP has existed, via the
>> > "human-monitor-command" passthrough command. IOW, while it is clearly
>> > desirable to be able to fix the problems, they are not a blocker to
>> > all real world usage.
>> >
>> > Meanwhile there is a need for other features which involve adding new
>> > parameters to the commands. This is possible with HMP passthrough, but
>> > it provides no reliable way for apps to introspect features, so using
>> > QAPI modelling is highly desirable.
>> >
>> > This patch thus introduces new snapshot-{load,save,delete} commands to
>> > QMP that are intended to replace the old HMP counterparts. The new
>> > commands are given different names, because they will be using the new
>> > QEMU job framework and thus will have diverging behaviour from the HMP
>> > originals. It would thus be misleading to keep the same name.
>> >
>> > While this design uses the generic job framework, the current impl is
>> > still blocking. The intention that the blocking problem is fixed later.
>> > None the less applications using these new commands should assume that
>> > they are asynchronous and thus wait for the job status change event to
>> > indicate completion.
>> >
>> > In addition to using the job framework, the new commands require the
>> > caller to be explicit about all the block device nodes used in the
>> > snapshot operations, with no built-in default heuristics in use.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> [...]
>> > diff --git a/qapi/job.json b/qapi/job.json
>> > index 280c2f76f1..b2cbb4fead 100644
>> > --- a/qapi/job.json
>> > +++ b/qapi/job.json
>> > @@ -22,10 +22,17 @@
>> >  #
>> >  # @amend: image options amend job type, see "x-blockdev-amend" (since 5.1)
>> >  #
>> > +# @snapshot-load: snapshot load job type, see "snapshot-load" (since 5.2)
>> > +#
>> > +# @snapshot-save: snapshot save job type, see "snapshot-save" (since 5.2)
>> > +#
>> > +# @snapshot-delete: snapshot delete job type, see "snapshot-delete" 
>> > (since 5.2)
>> > +#
>> >  # Since: 1.7
>> >  ##
>> >  { 'enum': 'JobType',
>> > -  'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
>> > +  'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
>> > +           'snapshot-load', 'snapshot-save', 'snapshot-delete'] }
>> >  
>> >  ##
>> >  # @JobStatus:
>> > diff --git a/qapi/migration.json b/qapi/migration.json
>> > index 675f70bb67..b584c0be31 100644
>> > --- a/qapi/migration.json
>> > +++ b/qapi/migration.json
>> > @@ -1720,3 +1720,123 @@
>> >  ##
>> >  { 'event': 'UNPLUG_PRIMARY',
>> >    'data': { 'device-id': 'str' } }
>> > +
>> > +##
>> > +# @snapshot-save:
>> > +#
>> > +# Save a VM snapshot
>> > +#
>> > +# @job-id: identifier for the newly created job
>> > +# @tag: name of the snapshot to create
>> > +# @devices: list of block device node names to save a snapshot to
>> 
>> Looks like you dropped the idea to also accept drive IDs.  Is that for
>> good, or would you like to add it later?
>
> I'm still kind of on the fence, but if general opinion is that we should
> accept drive IDs, I'll add it.

I'm fine with accepting only node names.  But unless we're fairly
certain node names will do, we should try to pick an interface that can
be extended to drive IDs painlessly.

> I wonder what the other blockdev-* APIs accept - some consistency between
> APIs is desirable.

The common pattern appears to be

    # Either @device or @node-name must be set but not both.
    #
    # @device: the name of the device to get the image resized
    #
    # @node-name: graph node name to get the image resized (Since 2.0)
    #
    [...]
                '*device': 'str',
                '*node-name': 'str',

For snapshot-save & friends, I can see two reasonably consistent ways:

1. Have two optional lists, must specify exactly one of them.

2. Change the list element from 'str' to a struct with the two optional
members, must specify exactly one.

The second way lets you mix drive IDs and node-names freely.  Do we want
to?

If yes, we can still use a variation of the first way: accept *both*
lists.

Permitting mixing makes it possible to specify the same device twice.
Could be silently accepted, or made a hard error.  Matter of taste.