Re: [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions

[Markus Armbruster]

John, your MUA turned the QMP examples to mush.  You may want to teach
it manners.

John Snow <address@hidden> writes:

> On 09/22/2015 06:34 PM, Eric Blake wrote:
>> On 09/22/2015 03:08 PM, John Snow wrote:
>>> Eric, Markus: I've CC'd you for some crazy what-if QAPI questions
>>> after my R-B on this patch.
>>>
>>
>>> The current design of this patch is such that the blockdev-backup
>>> and drive-backup QMP commands are extended for use in the
>>> transaction action. This means that as part of the arguments for
>>> the action, we can specify "transactional-cancel" as on/off,
>>> defaulting to off. This maintains backwards compatible behavior.
>>>
>>>
>>> As an example of a lone command (for simplicity...)
>>>
>>> { "execute": "transaction",
>>>   "arguments": {
>>>     "actions": [
>>>       {"type": "drive-backup",
>>>        "data": {"device": "drive0",
>>>                 "target": "/path/to/new_full_backup.img",
>>>                 "sync": "full", "format": "qcow2",
>>>                 "transactional-cancel": true
>>>                }
>>>       }
>>>     ]
>>>   }
>>> }
>>>
>>> This means that this command should fail if any of the other
>>> block jobs in the transaction (that have also set
>>> transactional-cancel(!)) fail.
>>
>> Just wondering - is there ever a case where someone would want to
>> create a transaction with multiple sub-groups?  That is, I want
>> actions A, B, C, D to all occur at the same point in time, but with
>> grouping [A,B] and [C, D] (so that if A fails B gets cancelled, but
>> C and D can still

You make my head hurt.

> Only two sub-groups:
>
> (A) actions that live and die together
> (B) actions that proceed no matter what.
>
> The only reason we even allow these two is because the default
> behavior has been B historically, so we need to allow that to continue on.
>
> I don't think we need to architect multiple subgroups of "live and die
> together" semantics.
>
>> continue)?  Worse, is there a series of actions to accomplish
>> something that cannot happen unless it is interleaved across
>> multiple such subgroups?
>>
>
> Not sure we'll need to address this.
>
>> Here's hoping that, for design simplicity, we only ever need one
>> subgroup per 'transaction' (auto-cancel semantics applies to all
>> commands in the group that opted in, with no way to further refine
>> into sub-groups of commands).
>>
>
> I think this is correct.

Puh!

>>>
>>> This is nice because it allows us to specify per-action which
>>> actions should live or die on the success of the transaction as a
>>> whole.
>>>
>>> What I was wondering is if we wanted to pidgeon-hole ourselves
>>> like this by adding arguments per-action and instead opt for a
>>> more generic, transaction-wide setting.
>>>
>>> In my mind, the transactional cancel has nothing to do with each
>>> /specific/ action, but has more to do with a property of
>>> transactions and actions in general.
>>>
>>>
>>> So I was thinking of two things:
>>>
>>> (1) Transactional cancel, while false by default, could be
>>> toggled to true by default for an entire transaction all-at-once
>>>
>>> { "execute": "transaction",
>>>   "arguments": {
>>>     "actions": [ ... ],
>>>     "transactional-cancel": true
>>>   }
>>> }
>>>
>>> This would toggle the default state for all actions to "fail if
>>> anything else in the transaction does."
>>
>> Or worded in another way - what is the use case for having a batch
>> of actions where some commands are grouped-cancel, but the
>> remaining actions are independent?  Is there ever a case where you
>> would supply "transactional-cancel":true to one action, but not all
>> of them?  If not, then this idea of hoisting the bool to the
>> transaction arguments level makes more sense (it's an all-or-none
>> switch, not a per-action switch).
>>
>> Qapi-wise, here's how you would do this:
>>
>> { 'command': 'transaction',
>>   'data': { 'actions': [ 'TransactionAction' ],
>>             '*transactional-cancel': 'bool' } }
>>
>
> Right. If there's no need for us to specify per-action settings at
> all, this becomes the obvious "correct" solution.
>
> If we do want to be able to specify this sub-grouping per-action (for
> whatever reason), this might still be nice as a convenience feature.

A common flag is semantically simpler than a per-action flag.  As
always, the more complex solution needs to be justified with an actual
use case.

A common @transactional-cancel defaulting to false suffices for backward
compatibility.

We think users will generally want to set it to true for all actions.
Again, a common flag suffices.

Unless someone comes up with actual use case for a per-action flag,
let's stick to the simpler common flag.

>>> Of course, as of right now, not all actions actually support this
>>>feature, but they might need to in the future -- and as more and more
>>>actions use this feature, it might be nice to have the global
>>>setting.

Uh-oh, you mean the flag is currently per-action because only some kinds
of actions actually implement it being set?

>>> If "transactional-cancel" was specified and is true (i.e. not
>>> the default) and actions are provided that do not support the
>>> argument explicitly, the transaction command itself should fail
>>> up-front.
>>
>> This actually makes sense to me, and might be simpler to
>> implement.

Yes, that's the stupidest solution that could possibly work, and
therefore the one that should be tried first.

> I'm not sure how to implement this, per-se, but in my mind it's
> something like:
>
> - A property of the transaction gets set (transactional-cancel) in
> qmp_transaction.
> - Each action has to prove that it has retrieved this value
>       - drive-backup of course actually uses it,
>       - other commands might fetch it to intentionally ignore it
>         (i.e. if cancel y/n would have no effect)
> - If an action does not fetch this property, the transactional setup
> flags an error and aborts the transaction with an error
>       - e.g. "Attempted to use property unsupported by action ..."
>
> With perhaps an allowance that, as long as the property is set to
> default, actions aren't required to check it.

Do we really need code to detect commands that don't know about the
flag?  As long as the number of transaction-capable commands is small,
why not simple make them all aware of the flag?  Make the ones that
don't implement it fail, which nicely fails the whole transaction.

[...]