Handle possible race conditions in SetAnnotations

Handle possible race conditions in SetAnnotations

Updated annotator.SetAnnotations: Perform two attempts 
to update annotations so that possible DocMissing/DocExists
transaction errors can be gracefully handled.

https://code.launchpad.net/~frankban/juju-core/bug-1156714-retry-set-annotations/+merge/154742

(do not edit description out of merge proposal)

[frankban, 2013-03-21 15:57:21]

Please take a look.

[rog, 2013-03-21 16:28:59]

LGTM with a suggestion for a comment below.

nice.

i wish we had a decent way of testing the contention logic (the only way that
occurs to me is to allow the tests to tell the code to halt temporarily after
it's inspected the state but before it runs the transaction)

https://codereview.appspot.com/7947043/diff/1/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/1/state/annotator.go#newcode48
state/annotator.go:48: // Perform two attempts to update annotations in order to
avoid possible
// Two attempts should be enough to update annotations even with racing
// clients - if the document does not already exist, one of the clients
// will create it and the others will fail, then all the rest of the
// clients should succeed on their second attempt.  If the referred-to
// entity has disappeared, and removed its annotations in the meantime,
// we consider that worthy of an error (will be fixed when new entities
// can never share names with old ones).

?

https://codereview.appspot.com/7947043/diff/1/state/annotator.go#newcode87
state/annotator.go:87: // The document exists: update and remove pairs.
that worked out rather nicely, thanks!

[frankban, 2013-03-21 16:36:42]

Please take a look.

https://codereview.appspot.com/7947043/diff/1/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/1/state/annotator.go#newcode48
state/annotator.go:48: // Perform two attempts to update annotations in order to
avoid possible
On 2013/03/21 16:28:59, rog wrote:
> // Two attempts should be enough to update annotations even with racing
> // clients - if the document does not already exist, one of the clients
> // will create it and the others will fail, then all the rest of the
> // clients should succeed on their second attempt.  If the referred-to
> // entity has disappeared, and removed its annotations in the meantime,
> // we consider that worthy of an error (will be fixed when new entities
> // can never share names with old ones).
> 
> ?

Done. Thanks a lot for this very nice suggestion.

[fwereade, 2013-03-21 23:40:17]

Good direction; a few comments follow.

Rog, this too has bothered me. It crosses my mind that the fuzzing built into
txn may be helpful to use here; does this set off any sparks in your mind? I've
never used that stuff myself, I just have a vague understanding that it exists
;).

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode59
state/annotator.go:59: }
I think we also need a docExists check on the entity document, so we can avoid
races between entity removal and annotation creation. This will be a bit tedious
to set up -- you'll need to be able to map from entity name back to
collection/_id -- but should basically be simple.

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode63
state/annotator.go:63: return fmt.Errorf("cannot update annotations on %s: %v",
a.globalKey, err)
Please use entityName rather than globalKey (which should remain an
internal-only detail, I think).

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode71
state/annotator.go:71: }
I think the block above should be inside the loop; no point rebuilding and
running a whole txn again if we have a chance to figure out an unambiguous cause
of failure for the cost of a trivial query. (I think the check and error message
remain valid and accurate in the new context.)

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode72
state/annotator.go:72: return fmt.Errorf("cannot update annotations on %s: %v",
a.globalKey, ErrExcessiveContention)
Ditto globalKey.

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode77
state/annotator.go:77: func (a *annotator) setAnnotationsOp(toInsert, toUpdate
map[string]string, toRemove map[string]bool) (txn.Op, error) {
Maybe do the entity existence assert mentioned above in here, and return an
[]txn.Op? I think the only case where it's actually necessary is on insert: if
we're updating then the existing DocExists on the annotations is enough, but if
we're inserting we also need to be sure that the new annotations doc references
an entity doc that still exists.

[frankban, 2013-03-22 11:30:23]

Please take a look.

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode59
state/annotator.go:59: }
On 2013/03/21 23:40:17, fwereade wrote:
> I think we also need a docExists check on the entity document, so we can avoid
> races between entity removal and annotation creation. This will be a bit
tedious
> to set up -- you'll need to be able to map from entity name back to
> collection/_id -- but should basically be simple.

Done. Implemented a method of State returning the colection/_id, given an entity
name.

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode63
state/annotator.go:63: return fmt.Errorf("cannot update annotations on %s: %v",
a.globalKey, err)
On 2013/03/21 23:40:17, fwereade wrote:
> Please use entityName rather than globalKey (which should remain an
> internal-only detail, I think).

Done.

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode71
state/annotator.go:71: }
On 2013/03/21 23:40:17, fwereade wrote:
> I think the block above should be inside the loop; no point rebuilding and
> running a whole txn again if we have a chance to figure out an unambiguous
cause
> of failure for the cost of a trivial query. (I think the check and error
message
> remain valid and accurate in the new context.)

Good point. Done.

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode72
state/annotator.go:72: return fmt.Errorf("cannot update annotations on %s: %v",
a.globalKey, ErrExcessiveContention)
On 2013/03/21 23:40:17, fwereade wrote:
> Ditto globalKey.

Done.

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode77
state/annotator.go:77: func (a *annotator) setAnnotationsOp(toInsert, toUpdate
map[string]string, toRemove map[string]bool) (txn.Op, error) {
On 2013/03/21 23:40:17, fwereade wrote:
> Maybe do the entity existence assert mentioned above in here, and return an
> []txn.Op? I think the only case where it's actually necessary is on insert: if
> we're updating then the existing DocExists on the annotations is enough, but
if
> we're inserting we also need to be sure that the new annotations doc
references
> an entity doc that still exists.

Done.

[rog, 2013-03-22 11:43:38]

> Rog, this too has bothered me. It crosses my mind that the fuzzing built
> into txn may be helpful to use here; does this set off any sparks in
> your mind? I've never used that stuff myself, I just have a vague
> understanding that it exists ;).

i think that's more for testing the transaction subsystem itself.
i trust implicitly that that's working - but i'd like some way of
testing our own logic (what *does* happen when one
of our operations gets ErrAborted? we are currently never
testing that path)

i have some half-formed ideas about a general transaction-space explorer that
hinges on making st.runner.Run into a function pointer so we can know when a
transaction is called and delay it until we see fit to run it, but i need more
thinking time.

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode59
state/annotator.go:59: }
On 2013/03/21 23:40:17, fwereade wrote:
> I think we also need a docExists check on the entity document, so we can avoid
> races between entity removal and annotation creation. This will be a bit
tedious
> to set up -- you'll need to be able to map from entity name back to
> collection/_id -- but should basically be simple.

yeah, that sounds reasonable, though it's a little bit of a pain.

instead of having an extra field in annotator, we could have an interface value,
something like:

type annotatorEntity interface {
    EntityName() string
    globalKey() string
    collection() string
    id() interface{}
}

then we'd implement relevant methods to return collection and id from the
various entity types.

..... alternatively, we could define a function (choose a better name than
this!):

// collectionAndIdForGlobalKey returns the collection
// storing the entity with the given global key
// and the value of its _id field.
func (st *State) collectionAndIdForGlobalKey(key string) (collection string, id
interface{})

and use that, with not much other change.

https://codereview.appspot.com/7947043/diff/5001/state/annotator.go#newcode71
state/annotator.go:71: }
On 2013/03/21 23:40:17, fwereade wrote:
> I think the block above should be inside the loop; no point rebuilding and
> running a whole txn again if we have a chance to figure out an unambiguous
cause
> of failure for the cost of a trivial query.

i'm not sure that's right. we already do the same query inside the loop to
decide what kind of transaction to build. how will adding another query help
there?

[fwereade, 2013-03-23 14:25:44]

On 2013/03/22 11:43:38, rog wrote:
> > Rog, this too has bothered me. It crosses my mind that the fuzzing built
> > into txn may be helpful to use here; does this set off any sparks in
> > your mind? I've never used that stuff myself, I just have a vague
> > understanding that it exists ;).
> 
> i think that's more for testing the transaction subsystem itself.
> i trust implicitly that that's working - but i'd like some way of
> testing our own logic (what *does* happen when one
> of our operations gets ErrAborted? we are currently never
> testing that path)

Not quite *never*, but surely not everywhere, and usually not explicitly.

> i have some half-formed ideas about a general transaction-space explorer that
> hinges on making st.runner.Run into a function pointer so we can know when a
> transaction is called and delay it until we see fit to run it, but i need more
> thinking time.

Cool -- ISTM that we probably could do something pretty tightly focused if we
just wrapped Run allowed the tests to register a callback for just before every
txn was run. (If we unregister the callback while running callbacks, or
something, we should be able to simulate exactly what happens when "other"
clients hit state at bad times.)

Hmm. I might be inspired here. We'll see if I find time this w/e...

[fwereade, 2013-03-23 15:39:21]

LGTM assuming you eliminate the extra roundtrip I entrapped you into before,
either as I suggest or otherwise; but I still think there's no point running a
second txn when the first has failed and no annotations doc exists.

https://codereview.appspot.com/7947043/diff/10001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/10001/state/annotator.go#newcode66
state/annotator.go:66: if count, err :=
a.st.annotations.FindId(a.globalKey).Count(); err != nil {
Rog makes a good point about extra roundtrips here. I *think* a neater answer
might be something like:

for i := 0; i < 2; i++ {
    var ops []txn.Op
    if count, err := a.st.annotations.FindId(a.globalKey).Count(); err != nil {
        return err
    } else if count == 0 {
        if i != 0 {
            return fmt.Errorf("%s was removed", a.entityName)
        }
        ops = a.insertOps(toInsert)
    } else {
        ops = a.updateOps(toInsert, toUpdate)
    }
    if err := a.st.runner.Run(ops, "", nil); err != txn.ErrAborted {
        return err
    }
}
return ErrExcessiveContention

...with suitably trivial insertOps/updateOps methods just to keep this one
clean... sane?

https://codereview.appspot.com/7947043/diff/10001/state/annotator.go#newcode99
state/annotator.go:99: ops = append(ops, entityOp)
I'd be inclined to leave entityOp anonymous and put it directly in the call,
myself, but mileage may reasonably vary.

[rog, 2013-03-24 12:13:22]

On 23 March 2013 14:25,  <fwereade@gmail.com> wrote:
> Cool -- ISTM that we probably could do something pretty tightly focused
> if we just wrapped Run allowed the tests to register a callback for just
> before every txn was run. (If we unregister the callback while running
> callbacks, or something, we should be able to simulate exactly what
> happens when "other" clients hit state at bad times.)
>
> Hmm. I might be inspired here. We'll see if I find time this w/e...

I had some time yesterday and came up with something
which might work. Will show you tomorrow.

[frankban, 2013-03-25 09:26:49]

Please take a look.

https://codereview.appspot.com/7947043/diff/10001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/10001/state/annotator.go#newcode66
state/annotator.go:66: if count, err :=
a.st.annotations.FindId(a.globalKey).Count(); err != nil {
On 2013/03/23 15:39:21, fwereade wrote:
> Rog makes a good point about extra roundtrips here. I *think* a neater answer
> might be something like:
> 
> for i := 0; i < 2; i++ {
>     var ops []txn.Op
>     if count, err := a.st.annotations.FindId(a.globalKey).Count(); err != nil
{
>         return err
>     } else if count == 0 {
>         if i != 0 {
>             return fmt.Errorf("%s was removed", a.entityName)
>         }
>         ops = a.insertOps(toInsert)
>     } else {
>         ops = a.updateOps(toInsert, toUpdate)
>     }
>     if err := a.st.runner.Run(ops, "", nil); err != txn.ErrAborted {
>         return err
>     }
> }
> return ErrExcessiveContention
> 
> ...with suitably trivial insertOps/updateOps methods just to keep this one
> clean... sane?

Done.

https://codereview.appspot.com/7947043/diff/10001/state/annotator.go#newcode99
state/annotator.go:99: ops = append(ops, entityOp)
On 2013/03/23 15:39:21, fwereade wrote:
> I'd be inclined to leave entityOp anonymous and put it directly in the call,
> myself, but mileage may reasonably vary.

Done.

[fwereade, 2013-03-25 14:02:45]

LGTM, just vague thoughts

https://codereview.appspot.com/7947043/diff/19001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/19001/state/annotator.go#newcode74
state/annotator.go:74: return fmt.Errorf("cannot update annotations on %s: %v",
a.entityName, err)
I'm a little bit ambivalent about the error annotation here, but that's an
ongoing discussion. If you really want to do so, though, I think this might be
better expressed with a trivial.ErrorContextf; so long as you have a named err
return value, you can start the method with:

    defer trivial.ErrorContextf(&err, "cannot update annotations on %s",
a.entityName)

...and then (1) make the error returns neater throughout, because that context
will automatically be added and (2) make this particular block neater, because
you can just return err if != txn.ErrAborted.

https://codereview.appspot.com/7947043/diff/19001/state/annotator.go#newcode103
state/annotator.go:103: txn.Op{
For consistency's sake, it might be nice to smoosh the above 3 lines together...

https://codereview.appspot.com/7947043/diff/19001/state/annotator.go#newcode109
state/annotator.go:109: ), nil
...and also these two. YMMV, as usual, but I think that ends up both compact and
readable.

[frankban, 2013-03-25 15:36:56]

Please take a look.

https://codereview.appspot.com/7947043/diff/19001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/19001/state/annotator.go#newcode74
state/annotator.go:74: return fmt.Errorf("cannot update annotations on %s: %v",
a.entityName, err)
On 2013/03/25 14:02:45, fwereade wrote:
> I'm a little bit ambivalent about the error annotation here, but that's an
> ongoing discussion. If you really want to do so, though, I think this might be
> better expressed with a trivial.ErrorContextf; so long as you have a named err
> return value, you can start the method with:
> 
>     defer trivial.ErrorContextf(&err, "cannot update annotations on %s",
> a.entityName)
> 
> ...and then (1) make the error returns neater throughout, because that context
> will automatically be added and (2) make this particular block neater, because
> you can just return err if != txn.ErrAborted.

Didn't know about trivial.ErrorContextf. Fixed, thanks.

https://codereview.appspot.com/7947043/diff/19001/state/annotator.go#newcode103
state/annotator.go:103: txn.Op{
On 2013/03/25 14:02:45, fwereade wrote:
> For consistency's sake, it might be nice to smoosh the above 3 lines
together...

Done.

https://codereview.appspot.com/7947043/diff/19001/state/annotator.go#newcode109
state/annotator.go:109: ), nil
On 2013/03/25 14:02:45, fwereade wrote:
> ...and also these two. YMMV, as usual, but I think that ends up both compact
and
> readable.

Done.

[fwereade, 2013-03-25 17:23:40]

Thanks very much. LGTM.

https://codereview.appspot.com/7947043/diff/24001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/24001/state/annotator.go#newcode100
state/annotator.go:100: },
I might suggest similar smooshing here, sorry I missed that.

[rog, 2013-03-26 11:48:40]

LGTM with one suggestion below

https://codereview.appspot.com/7947043/diff/24001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7947043/diff/24001/state/state.go#newcode371
state/state.go:371: prefixCollMap := map[string]string{
rather than making a map just to throw it away, i'd use a switch statement. then
you've naturally got a place for the st.units special case.

id = parts[1]
switch parts[0] {
case "machine":
    coll = st.machines.Name
case "unit":
    coll = st.units.Name
    replace unit id etc
case ...
default:
    return "", "", fmt.Errorf(...)
}

https://codereview.appspot.com/7947043/diff/24001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/7947043/diff/24001/state/state_test.go#newcode...
state/state_test.go:1500: // Parse a service entity name.
a blank line before each section would help the eye, i think.

[frankban, 2013-03-26 16:01:31]

*** Submitted:

Handle possible race conditions in SetAnnotations

Updated annotator.SetAnnotations: Perform two attempts 
to update annotations so that possible DocMissing/DocExists
transaction errors can be gracefully handled.

R=rog, fwereade
CC=
https://codereview.appspot.com/7947043

https://codereview.appspot.com/7947043/diff/24001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7947043/diff/24001/state/annotator.go#newcode100
state/annotator.go:100: },
On 2013/03/25 17:23:41, fwereade wrote:
> I might suggest similar smooshing here, sorry I missed that.

Done.

https://codereview.appspot.com/7947043/diff/24001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/7947043/diff/24001/state/state_test.go#newcode...
state/state_test.go:1500: // Parse a service entity name.
On 2013/03/26 11:48:41, rog wrote:
> a blank line before each section would help the eye, i think.

Done.