Re: [Qemu-devel] [PATCH v4 07/11] qapi: Convert savevm

[Markus Armbruster]

[Note Cc: Kevin]

Eric Blake <address@hidden> writes:

> On 04/09/2013 11:23 AM, Markus Armbruster wrote:
>>> If we mandate that a tag always be supplied, then we cannot create qcow2
>>> files with a snapshot that lacks a tag using just QMP; but even if we do
>> 
>> Are you sure you can do that with the proposed patches?  If I read them
>> correctly, we make up a tag vm-%Y%m%d%H%M%S when none is given.
>
> HMP is higher-level than QMP, so having HMP default to always generating
> a tag seems like a reasonable thing.  In my opinion, it's okay for
> higher layers to ensure a name even when lower layers leave it optional.
>  Besides, HMP aims to ease user interface and it is easier to work with
> named snapshots than it is to work with anonymous ones, even if that
> means HMP generates a name.  We already have instances where HMP is
> intentionally less powerful than QMP, because if you are using the human
> interface, you don't need to be bothered by the subtleties - and if you
> need the full power, then you use the lower-level interface.  On the
> other hand, we could always change our mind later to have HMP create
> anonymous snapshots (by adding another flag to HMP 'savevm') instead of
> generating a name.

We agree on HMP.

> Where we would get into trouble is if QMP enforces a name but we change
> our mind and later want HMP to again expose the possibility of anonymous
> snapshots.

The proposed patches don't let you create anonymous snapshots via QMP,
as far as I can tell.  The same default tag is used.

>             Also, if QMP doesn't expose the full power of the file
> format on creation, but still has to support the full power of the file
> format on the loadvm side, it becomes that much harder to test that the
> loadvm support is correct in the presence of anonymous snapshots.

Are anonymous snapshots actually good for anything?  Or are they just a
regrettable historical accident we need to cope with somehow?

In the latter case, we can handle them in qemu (loadvm accepts them), or
in qemu-img (set snapshot tag).

Regardless of where we support anonymous snapshots, we need to be able
to create them for testing purposes.  We can create them either in qemu
(vm-snapshot-save), or in qemu-img (unset snapshot tag).

>>> that, we STILL have to support use of existing qcow2 files that were
>>> created by earlier qemu and/or other tools that have snapshots without a
>>> tag.
>> 
>> I understand the need to deal with existing qcow2 files lacking tags.
>> 
>> I understand the desire to be able to create such files via QMP.  I
>> don't share it, though :)
>
> I'm on the fence - I could live with the requirement that QMP requires a
> name, and that creation of anonymous snapshots requires other tools.
> Libvirt will always pass a name, for what it is worth, regardless of
> whether HMP generates a name, and regardless of whether QMP requires a
> name.  But testing nameless support is harder if it requires an extra
> tool to create a nameless snapshot in the first place.

Not really hard if we limit anonymous snapshot support to qemu-img:
change snapshot tag.

As far as I can tell, qemu can't create anonymous snapshots anymore
since commit 7d631a1 "savevm: Generate a name when run without one" from
August 2010.  Hasn't impeded testing so far (possibly because there has
been none).

>> For block driver method bdrv_snapshot_create() both tag and ID are
>> optional.  If ID is missing, it picks one.  qcow2 picks max. existing ID
>> + 1.  If tag is missing, the snapshot doesn't get one.
>> 
>> When savevm overwrites an existing snapshot, it reuses both tag and ID.
>> One of them matches @name.
>> 
>> When savevm creates a new snapshot, @name becomes the tag.  If @name
>> isn't given, we make one up.  ID is left to the block driver.
>> 
>> vm-snapshot-save behaves the same.
>> 
>> Say we have two snapshots, with IDs 1 and 5.  Say I want to overwrite
>> the first one, and chose to identify it by its ID.  Since I got dirt on
>> my monitor, I misread ID 7.  Since neither ID nor tag 7 exist, I
>> accidentally create a new snapshot with tag 7 and ID 6 (picked by qcow2
>> block driver).  Next new snapshot will inevitably get ID 7 and the
>> confusion is complete.  All because the stupid command doesn't let me
>> express myself clearly: I mean *ID* 7, *not* tag 7!
>
> Yep - another instance of the confusion that current HMP 'savevm' has,
> and that I want to get rid of by adding -f as a force option at a
> minimum to the HMP layer.

Current patches' -f doesn't help in my example.

>> I think I'd try the following for QMP:
>> 
>> * Drop the capability to overwrite existing snapshot.  No real loss, as
>>   it's not an atomic overwrite: it first deletes the old one, then
>>   creates the new one, and if create fails, the old one's still gone.
>
> We'd _still_ want to add HMP 'savevm -f' to force an overwrite, but you
> are saying that the HMP command would then be a sequence of _two_ QMP
> calls - delete then create, where QMP create _always_ fails if a
> snapshot already exists with that name.  Makes sense - it just means
> that the 'force' semantics are now put as a burden on the caller,
> instead of on QMP.

I'm fine with HMP 'savevm -f'.  In HMP, we go for user convenience in
common use cases.

For QMP, it's generally better to go for building blocks, i.e. commands
that do just one thing.  Let the client combine them however it sees
fit.

>> * Take parameter @tag and @id.
>> 
>>   Fail if @tag is given and matches any existing tag.  If it's not
>>   given, we make one up that doesn't match any existing tag.
>
> Generating an id makes sense.  And the command must return the id that
> got used (whether passed in by the user or generated).

Yes.

>>   Fail if @id is given and matches any existing ID.  If it's not given,
>>   we make one up that doesn't match any existing ID.
>
> I don't know if I like the idea of QMP generating a tag, vs. leaving the
> snapshot anonymous.  But note that the answer to the next question
> determines whether we can bypass this question, since if...
>
>> 
>> * Seriously consider making @tag mandatory.
>
> ...tag is made mandatory, then QMP never has to generate a tag.  I'm
> still on the fence on whether to always have a tag in place (whether by
> QMP generation or by mandatory argument) vs. allowing anonymous
> snapshots.  But again, libvirt always names its snapshots, and I trust
> your judgment as maintainer of this area of code in which way we should go.

>From a QMP point of view, I like a mandatory tag, because it reduces
headaches.

For the QCOW2 / block layer / qemu-img point of view, I'm cc'ing Kevin.