Many comments, because I like the sound of my own typing, but very
little actionable. I'd appreciate your thoughts but this basically LGTM.

Fix any trivials and let me know when it's ready to merge, and I'll
approve it.


https://codereview.appspot.com/98260043/diff/40001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1325
state/unit.go:1325: if notDead, err := isNotDead(u.st.units,
u.doc.Name); err != nil {
This one is interesting -- it's a not-strictly-necessary DB hit before
running the txn, and I'd ordinarily shy away from that in favour of
checking u.doc.Life -- but let's see how it goes. I suspect we'll need
to go with the other approach in the end, as we start to care about
things like charm versions, but we can wait and see if I'm right about
that.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1331
state/unit.go:1331: sel := bson.D{{"_id", newActionID}}
This block is almost redundant, because we should be able to depend on
the uniqueness of sequence IDs (even in the case of a new unit being
created with the same name as an old one (which is itself a bug
(although it only happens when a service is destroyed and recreated))).

If it didn't involve an extra DB hit I'd have no problem with it; but as
it is I'm gently leaning towards dropping it. If there's a problem here
we'll fall out to ErrExcessiveContention anyway, and it should actually
be pretty easy to diagnose from there.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1340
state/unit.go:1340: ops := []txn.Op{{
As discussed yesterday, in this particular case the ops don't have to be
rebuilt each time, but it's probably good to keep the familiar
structure.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1351
state/unit.go:1351: return newActionID, err
returning the id is not "normal" for this package, but IMO it's a good
idea. thanks.

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1304
state/unit_test.go:1304: // verify action Id() is of expected form (unit
id prefix, + sequence)
Add a note that this is temporary, and we shouldn't really leak the
actual _id.

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1335
state/unit_test.go:1335: unitDead := state.TransactionHook{
s/unitDead/killUnit/ perhaps?

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1348
state/unit_test.go:1348: //TODO(jcw4): need to figure out how to inject
'contention' into the transaction
Haha. I've thought of a way, and I kinda endorse you doing it, so long
as you clearly comment that it'll stop working when lp:1174610 is fixed.
In the Before hook, you get st.Unit(name) and make it Dead; in the After
hook, you remove the unit and the service, and create a new service with
the same name. From the POV of that method the unit will appear to be
resurrected.

But that's also dirty as sin, and will break when we fix other stuff, so
I'm reasonably comfortable skipping it. (The other possibility, which is
*maybe* cleaner, is to figure out the action doc id and create/remove a
doc with that name in the before/after hooks. But figuring out the id
without dirt may be tricky -- expose sequence() in export_test, and
subtract 1? Could work, but still depends on knowing _id structure;
don't really love either.)

https://codereview.appspot.com/98260043/