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

[John Snow]

On 09/23/2015 07:09 AM, Markus Armbruster wrote:
> John, your MUA turned the QMP examples to mush.  You may want to teach
> it manners.
> 

Ugh, sorry. I apparently can't trust

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

"I think this is correct*"

"*If we do want to allow per-action specificity of this option."

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

More like: This is the first time we've ever needed it, and we've only
been considering how drive-backup will behave under this flag.

We are implementing this property for the first time for (essentially)
one command. Other commands don't support the behavior yet because we've
just added it.

I am intervening and asking how we want to express the property.

>>>> 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.
> 
> [...]
> 

Oh, I see: you're saying ...

each action fetches the property, and if it's true, fail (for 2.5, at
least) with e.g. a message saying "This property is not [yet?] supported
for this action"

That's simple. I always overcomplicate everything ...

--js