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

[Daniel P . Berrangé]

On Mon, Sep 21, 2020 at 01:16:16PM -0500, Eric Blake wrote:
> On 9/16/20 3:17 AM, 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?
> 
> Is it necessary?  Several of our newer block interfaces have required node
> names, rather than permitting alternation.  If we rewrite the existing HMP
> commands to operate on top of the new QMP command, it is still possible for
> HMP to support drive names even when QMP does not.  I don't think the
> complexity of worrying about drive names is worth it; after all, the QMP
> command is new enough that the only libvirt that will use it is also a
> libvirt that knows how to use -blockdev, and thus node names are sufficient.
> 
> Yes, we can add drive ids later if I turn out to be wrong, but for now, I'm
> hoping their exclusion is intentional.

I didn't realize we have precedent for new commands only accepting
node names. Given that, I'm going to stick with this design and
only support node names.

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 :|