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
continue)? Worse, is there a series of actions to accomplish something
that cannot happen unless it is interleaved across multiple such subgroups?
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).
>
> 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' } }
>
> 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.
>
> (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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
