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

[Kevin Wolf]

Am 03.09.2020 um 11:48 hat Markus Armbruster geschrieben:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, Sep 02, 2020 at 11:27:17AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Tue, Sep 01, 2020 at 04:20:47PM +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 primary reason for this lack of 
> >> >> > conversion
> >> >> > is that they block execution of the thread for as long as they run.
> >> >> 
> >> >> Nope.  The primary reason is that the HMP interface is bonkers.
> >> >
> >> > I don't think that's very helpful description. The HMP interface has
> >> > some limitations, but it isn't bonkers - it just doesn't cope with
> >> > all the use cases we want. Many people use it succesfully without
> >> > issue
> >> 
> >> It's non-bonkers for the case it was designed for: one disk backed by
> >> QCOW2, plus maybe a CD-ROM.  The user is responsible for ensuring the
> >> CD-ROM's media at loadvm time matches the one at savevm time.  The user
> >> is further responsible for ensuring the guest-visible configuration
> >> matches the one at savevm time.  No worse than migration.
> >
> > It is fine for multiple writable disks too, if they're all qcow2 backed.
> >
> >> It becomes useless as soon as you have writable non-QCOW2 block devices.
> >> "Stop machine, take external snapshots, savevm, restart machine" should
> >> work, but doesn't.

This is because savevm can't snapshot all writable disks and fails in
this case, right? So the proposed alternative with a list of nodes to
create disk snapshots for would solve this.

> > External snapshots can be made to work today if you're willing to pause
> > the VM CPUs while you take the external snapshots, and run the migration
> > to capture VM state.
> 
> Yes.
> 
> Pausing is necessary because we don't have a way to take snapshots when
> migration completes.
> 
> Even if we had it, live migration to file would be a problematic way to
> capture VM state, because it doesn't update changing state in place.

It's also not the semantics you usally want for snapshots. You want to
take a snapshot of the state _now_, not of the state at some arbitrary
time in the future.

That's why I mentioned the post-copy semantics for live snapshots.

> savevm is not live either, of course.
> 
> My point is that savevm is "QCOW2 or bust" unnecessarily.  Your
> proposed QMP interface isn't.

Strictly speaking, not qcow2, but "block driver that supports
snapshots". Not that it matters in practice, but this happens to include
sheepdog.

It also means that we could implement a filter driver that saves the VM
state to an external file, which would allow saving the VM state even if
you use only raw images (of course, only useful if you have means to
snapshot the disk state externally or in combination with external
snapshots).

> > If you want to see some examples, take a look at the very last patch
> > in the series which introduces I/O test 310. The test covers various
> > success and failure scenarios, so you can see the behaviour we are
> > currently generating with this series.
> >
> >
> >> >> > +#
> >> >> > +# If @vmstate is not specified, then the first valid block
> >> >> > +# device will be used for vmstate.
> >> >> 
> >> >> Why is this useful for QMP?
> >> >
> >> > Both of these makes QEMU just "do the right thing" with the majority
> >> > of QEMU guest configurations with no special knowledge needed by
> >> > the mgmt app.
> >> >
> >> > It makes it possible for all existing apps to immediately stop using
> >> > the loadvm/savevm commands via HMP passthrough, and convert to the
> >> > QMP commands.
> >> 
> >> I appreciate your concern for easy migration from HMP savevm/loadvm to
> >> QMP.  I'm unwilling to permanently screw up the QMP interface for it,
> >> though.
> >> 
> >> > Without this, applications will need to first convert to use -blockdev
> >> > before they can use the load-snapshot/save-snapshot commands, because
> >> > the devices are specified exclusively using blockdev node names, not
> >> > the legacy drive IDs.
> >> 
> >> Not true.  *Every* block node has a node name.  If the user doesn't
> >> specify one, the system makes one up.  query-named-block-nodes shows
> >> it.  So does query-block.
> >
> > query-named-block-nodes isn't friendly as a way to lookup node names,
> > as it doesn't include the "id" value for the original -drive, so
> > correlating block nodes to drives is not straightforward.  query-block
> > does seem a bit better in respect.
> >
> > So if an app is currently using loadvm/savevm with human_monitor_command,
> > and is using -drive, they have to go through a mapping process to figure
> > out node names. Not especially friendly if they were happy with the
> > current choice of disks QEMU makes by default. 
> 
> I take "not especially friendly to clients converting from (bonkers) HMP
> commands" over "similarly bonkers" any day.  Okay, I'm exaggerating for
> effect, your proposed interface is much better even with this default.
> I do hate this default, though.
> 
> QMP is (meant to be) explicit and non-magic.
> 
> If having to map from drive ID to node-name really is too much of a
> burden, we can look for ways to make it easier, or we can make savem
> optionally accept drive IDs instead of node-names, like we do in several
> other places for backward compatibility.

Yes, letting commands accept both node-names and drive IDs is trivial
and we do it pretty much everywhere. Much better than randomly selecting
an image to save the VM state to.

But I'm not even sure if makign people switch to -blockdev would be a
bad thing. If they insist on bad interfaces, they can keep using HMP
'savevm', too...

Kevin