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

[John Snow <jsnow@redhat.com>]

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

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.

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

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

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.

>> 
>> (2) Overridable per-action settings as a property of "action"
>> 
>> Instead of adding the argument per-each action, bake it in as a
>> property of the action itself. The original idea behind this was
>> to decouple it from the QMP arguments definition, but this patch
>> here also accomplishes this -- at the expense of subclassing each
>> QMP argument set manually.
>> 
>> Something like this is what I was originally thinking of doing:
>> 
>> { "execute": "transaction", "arguments": { "actions": [ { "type":
>> "drive-backup", "data": { ... }, "properties": {
>> "transactional-cancel": true } } ] } }
>> 
>> In light of how Fam and Eric solved the problem of "not polluting
>> the QMP arguments" for this patch, #2 is perhaps less pressing or
>> interesting.
>> 
>> A benefit, though, is that we can define a set of generic
>> transactional action properties that all actions can inspect to
>> adjust their behavior without us needing to specify additional
>> argument subclasses in the QAPI every time.
> 
> And here's how we'd do it in qapi.  We'd have to convert 
> TransactionAction from simple union into flat union (here, using
> syntax that doesn't work on qemu.git mainline, but requires my v5
> 00/46 mega-patch of introspection followups - hmm, it's still
> verbose, so maybe we want to invent yet more sugar to avoid having
> to declare all those wrapper types):
> 
> { 'enum': 'TransactionActionKind', 'data': [ 
> 'blockdev-snapshot-sync', 'drive-backup', 'blockdev-backup', 
> 'abort', 'blockdev-snapshot-internal-sync' ] } { 'struct':
> 'TransactionProperties', 'data': { '*transactional-cancel': 'bool'
> } } { 'struct': 'BlockdevSnapshotSyncWrapper', 'data': { 'data':
> 'BlockdevSnapshotSync' } } { 'union': 'TransactionAction', 'base':
> { 'type': 'TransactionActionKind', '*properties':
> 'TransactionProperties'}, 'discriminator': 'type', 'data': {
> 'blockdev-snapshot-sync': 'BlockdevSnapshotSyncWrapper', ... } }
> 
>> 
>> Mostly, at this point, I want to ask if we are OK with adding
>> the "transactional-cancel" argument ad-hoc per-action forever, or
>> if we'd like to allow a global setting or move the property "up
>> the stack" to some extent.
>> 
>> Maybe the answer is "no," but I wanted to throw the idea out
>> there before we committed to an API.
> 
> No, it's a good question. And adding it once at the 'transaction'
> layer or in the 'TransactionAction' union may indeed make more
> sense from the design perspective, rather than ad-hoc adding to
> each (growing) member of the union.
> 

#2 was just another method of hoisting the QAPI arguments that are
transaction-specific away from the QMP arguments, and won't
necessarily be needed in conjunction with #1.

--js