Re: [Qemu-devel] [PATCH v4 10/10] Add the drive-reopen command

[Kevin Wolf]

Am 14.03.2012 14:11, schrieb Eric Blake:
>>> * We could use the transaction APIs without actually making it
>>>   transaction (if we fail in the middle we can't rollback). The only
>>>   advantage of this is that we'd provide a consistent API to libvirt
>>>   and we would postpone the problem to the future. Anyway I strongly
>>>   discourage this as it's completely unsafe and it's going to break
>>>   the transaction semantic. Moreover it's a solution that relies too
>>>   much on the hope of finding something appropriate in the future.
>>
>> This is not an option. Advertising transactional behaviour and not
>> implementing it is just plain wrong.
> 
> Absolutely concur.  From libvirt's perspective, I will be assuming that:
> 
> if 'transaction' exists, then it supports 'blockdev-snapshot-sync' and
> 'drive-mirror' (assuming that these are the only two commands that are
> in 'transaction' in qemu 1.1), but nothing else is safe to use without a
> further probe.

I don't think it's good to conclude alone from the presence of
'transaction' that subcommands exist. Current master does have
'transaction' (and the 'blockdev-snapshot-sync' action), but not yet
'drive-mirror'.

>  Then, if down the road, we go through the pain of making
> 'drive-reopen' transactionable, then there will be some new
> introspection command (one idea was an optional argument to
> query-commands which limits output to just the commands that can also
> occur in a 'transaction'), and libvirt will have to use that
> introspection before using the additional features.

We can have the introspection command now. Either a new option like you
suggest, or even simpler, just add a 'transactionable': 'bool' to the
existing CommandInfo.

>>> * We could leave it as it is, a distinct command that is not part of
>>>   the transaction and that it's closing the old image before opening
>>>   the new one.
>>
>> Yes, this would be the short-term preliminary solution. I would tend to
>> leave it to downstreams to implement it as an extension, though.
> 
> Correct - I'm fine with libvirt using the direct, non-atomic,
> 'drive-reopen' for the immediate needs of oVirt while we work on a more
> complete solution for down the road.

One important difference that probably matters even now is that the
proposed drive-reopen can fail in the middle, where the old BDS is
closed, but the new one couldn't be opened. In this case the disk will
be lost. libvirt must handle this case in some way.

>>> This is not completely correct, the main intent was to not spread one
>>> image chain across two storage domains (making it incomplete if one of
>>> them was missing). In the next oVirt release a VM can have different
>>> disks on different storage domains, so this wouldn't be a special case
>>> but just a normal situation.
>>
>> The problem with this kind of argument is that we're not developing only
>> for oVirt, but need to look for what makes sense for any management tool
>> (or even just direct users of qemu).
> 
> Indeed - that's why I still think it's dangerous to not have an atomic
> 'drive-reopen', even if oVirt can be coded to work in spite of an
> initial implementation being non-atomic.  But there's a difference
> between wish-lists (atomic reopen) and practicality (what can we
> implement now), and I'm not going to insist on the impossible :)

Well, at least upstream you always have the option to reject a feature
instead of taking something broken. And I'm still not quite sure whether
to use this option with part of this series or not.

Kevin