Merge lp:~johnweldon4/juju-core/action into lp:~go-bot/juju-core/trunk

[John Weldon]

Actions: Work in Progress

Adding action documents to the State

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

Actions: Work in Progress

Adding action documents to the State

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

[William Reade (fwereade)]

This is a good start, but I have a few issues. Setting WIP; ping me when you're online, I'd be happy to chat about all this.

FWIW, I'd really appreciate an `lbox propose` so I could do this line-by-line :).

23 + Payload string

I'm a bit surprised payload is a string -- I was expecting an interface{}, that would have been checked against (or rather would at least be cleanly check*able* against) the action schema on creation.

35 + doc: *adoc,

I think that if you just passed an actionDoc by value you'd get the same copying behaviour, with less punctuation. Have I missed something?

56 +func (a *Action) setRunning() error {
71 +func (a *Action) setPending() error {

These don't seem to be used (tested)? And I don't see that setPending will ever be called, will it?

169 +// AddAction takes a unit Id, and an action name, and a payload containing
170 +// json-schema options for the action and inserts it into the actions
171 +// collection

Shouldn't the payload be "structured data conforming to the json-schema defined in the charm"? ...and at this point questions of compatibility creep in. There's no way to deal with them right now, because we haven't dealt with the charm changes, so that's not mandatory here; but we'll need to get the schemas into the DB sooner rather than later, I think.

173 + newActionID := fmt.Sprintf("%s_%s", unit, string(bson.NewObjectId()))

Given that we'll have actions targeted to things other than units in the long term, we shoudl probably go with a globalKey-style id here: something like "u#<unit-id>#<action-id>". And I'm not wholly convinced by the embedded ObjectId -- those things do have useful properties but we'll be hard-pressed to extract them usefully.

If you get the "u#foo/3#" prefix, you can use st.sequence(prefix) to get a unique int to tack on the end; this will give us ordering for free (if not *alphabetical* ordering... although you could go with leading zeros for that). We might also want a timestamp or two (queued-at, started-at) in the doc somewhere, but that's probably not necessary in this CL.

174 + doc := actionDoc{newActionID, name, unit, payload, ActionPending}

Please use field names in constructors this size (or any size above... 1, really :)).

222 +// Destroy , when called

revert please

### missing

We should make sure that when units are destroyed, their pending actions are destroyed (and, I guess, marked as failed, but ofc you can't do that yet). I'm pretty sure there's already a cleanup action that gets triggered on unit removal, you can extend that one.

[John Weldon (johnweldon4)]

Reviewers: mp+219225_code.launchpad.net,

Message:
Please take a look.

Description:
Actions: Work in Progress

Adding action documents to the State

https://code.launchpad.net/~johnweldon4/juju-core/action/+merge/219225

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/98260043/

Affected files (+173, -4 lines):
   A [revision details]
   A state/action.go
   A state/action_test.go
   M state/open.go
   M state/state.go
   M state/state_test.go
   M state/unit.go

[William Reade (fwereade)]

Should just be another round, I think. Ping me when you're online and we
can chat.

https://codereview.appspot.com/98260043/diff/1/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode23
state/action.go:23: Status ActionStatus
Hmm, wondering now about the consequences on watches if we have the
status direct in the action doc. Would it make sense to leave status out
for now? The unit's going to be watching this collection, and doesn't
need to respond to status changes; but the client will be watching the
*results* collection, and might well want to respond to changes there.
Sane?

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode34
state/action.go:34: action := &Action{
return &Action{

?

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode70
state/action.go:70: err := a.st.runTransaction(ops)
we should handle txn.ErrAborted and give a real error back -- in this
case it indicates (I think) that the action has already completed.
Right?

https://codereview.appspot.com/98260043/diff/1/state/action_test.go
File state/action_test.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode27
state/action_test.go:27: }
FWIW we usually leave these out, but I think the explicitness of this
approach may actually be better. Don't feel obliged to include them if
you don't think it's necessary.

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode30
state/action_test.go:30: action, err :=
s.SettingsSuite.state.AddAction("fakeunit/0", "fakeaction", nil)
Hmm, I think we *should* make sure that the unit exists and isn't Dead.

(I've been thinking, and have tentatively concluded that actions on
dying units are fine, they just might not run in time; but actions on
dead units are definitely insane.)

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

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode320
state/state.go:320: for k := range oldConfig.AllAttrs() {
thanks for this cleanup btw

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1379
state/state.go:1379: func (st *State) AddAction(unit string, name
string, payload interface{}) (*Action, error) {
Thinking about this: I think it should be (u *Unit) AddAction(name
string, payload interface{}) (...).

Should be a simple move (and also make it easier to assert that the unit
isn't dead).

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1395
state/state.go:1395: return nil, onAbort(err, errDead)
I don't think errDead is a sensible error message for "doc already
exists".

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1396
state/state.go:1396: }
Given that we'll want two asserts here, we'll have two possible failure
modes (even if the one you're currently handling should be a cant-happen
one). Let's talk today about how to write the code in (what I think is)
the cleanest way, because it's not actually very obvious, I'm afraid.

Yeah, this is another thing I should be documenting.

https://codereview.appspot.com/98260043/diff/1/state/sta...

[John Weldon (johnweldon4)]

A few comments and responses. We can chat on irc or hangouts whenever.

https://codereview.appspot.com/98260043/diff/1/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode34
state/action.go:34: action := &Action{
On 2014/05/15 12:47:26, fwereade wrote:
> return &Action{

> ?

Done.

https://codereview.appspot.com/98260043/diff/1/state/action_test.go
File state/action_test.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode27
state/action_test.go:27: }

On 2014/05/15 12:47:26, fwereade wrote:
> FWIW we usually leave these out, but I think the explicitness of this
approach
> may actually be better. Don't feel obliged to include them if you
don't think
> it's necessary.

Do you mean the (SetUp|TearDown)(Suite|Test) funcs?
Because they're just calling the inner SettingsSuite funcs does that
mean the test harness will do it automatically?

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode30
state/action_test.go:30: action, err :=
s.SettingsSuite.state.AddAction("fakeunit/0", "fakeaction", nil)
On 2014/05/15 12:47:27, fwereade wrote:
> Hmm, I think we *should* make sure that the unit exists and isn't
Dead.

> (I've been thinking, and have tentatively concluded that actions on
dying units
> are fine, they just might not run in time; but actions on dead units
are
> definitely insane.)

In this case I'm actually not even testing any "real" unit. Of course
when we do the refactor to make func (u* Unit) AddAction(...) then we'll
need a real Unit for this test.
But this test is kind of superfluous any way. state_test has a very
similar test already.

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

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode320
state/state.go:320: for k := range oldConfig.AllAttrs() {
On 2014/05/15 12:47:27, fwereade wrote:
> thanks for this cleanup btw

I love cleanup :)

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1395
state/state.go:1395: return nil, onAbort(err, errDead)
On 2014/05/15 12:47:27, fwereade wrote:
> I don't think errDead is a sensible error message for "doc already
exists".

Er. Yes. Copy / Paste dumbness

https://codereview.appspot.com/98260043/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/98260043/diff/1/state/state_test.go#newcode182
state/state_test.go:182: answerAction, err :=
s.State.Action(testAction.Id())
On 2014/05/15 12:47:27, fwereade wrote:
> It might be nice to have some sort of check on the form of the Id.

Makes sense... I'll do something like "u#" == answerAction.Id()[:2],
maybe min length check and substring match on "#" in id[2:]. If you have
any suggestions let me know.

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

[William Reade (fwereade)]

https://codereview.appspot.com/98260043/diff/1/state/action_test.go
File state/action_test.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode8
state/action_test.go:8: SettingsSuite
Ooh, didn't spot that: this will include all the Test* methods from
SettingsSuite as well, and we don't want that. I'd recommend ConnSuite
(the one right here in state_test, not the one in... er juju/testing?).

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode27
state/action_test.go:27: }
On 2014/05/15 16:23:28, johnweldon4 wrote:

> On 2014/05/15 12:47:26, fwereade wrote:
> > FWIW we usually leave these out, but I think the explicitness of
this approach
> > may actually be better. Don't feel obliged to include them if you
don't think
> > it's necessary.

> Do you mean the (SetUp|TearDown)(Suite|Test) funcs?
> Because they're just calling the inner SettingsSuite funcs does that
mean the
> test harness will do it automatically?

Yeah, if you embed a type you get all its methods (so long as there's no
ambiguity, which there isn't in this case).

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode30
state/action_test.go:30: action, err :=
s.SettingsSuite.state.AddAction("fakeunit/0", "fakeaction", nil)
On 2014/05/15 16:23:28, johnweldon4 wrote:
> On 2014/05/15 12:47:27, fwereade wrote:
> > Hmm, I think we *should* make sure that the unit exists and isn't
Dead.
> >
> > (I've been thinking, and have tentatively concluded that actions on
dying
> units
> > are fine, they just might not run in time; but actions on dead units
are
> > definitely insane.)

> In this case I'm actually not even testing any "real" unit. Of course
when we
> do the refactor to make func (u* Unit) AddAction(...) then we'll need
a real
> Unit for this test.
> But this test is kind of superfluous any way. state_test has a very
similar
> test already.

I think this suite's existence is good, it's nice to cluster the tests
for related functionality, and there's an awful lot on the direct State
tests already.

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

[John Weldon (johnweldon4)]

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

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1379
state/state.go:1379: func (st *State) AddAction(unit string, name
string, payload interface{}) (*Action, error) {
On 2014/05/15 12:47:27, fwereade wrote:
> Thinking about this: I think it should be (u *Unit) AddAction(name
string,
> payload interface{}) (...).

> Should be a simple move (and also make it easier to assert that the
unit isn't
> dead).

I'm assuming we put the Action doc in the same actions collection on
state? We're just doing the add on the Unit?

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

[John Weldon (johnweldon4)]

Updated code to address items in this review.

Still need to do:

1) Transaction loop
2) Cleanup (maybe wait til fwereade refactor makes it to juju-core?)

https://codereview.appspot.com/98260043/diff/1/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode23
state/action.go:23: Status ActionStatus
On 2014/05/15 12:47:26, fwereade wrote:
> Hmm, wondering now about the consequences on watches if we have the
status
> direct in the action doc. Would it make sense to leave status out for
now? The
> unit's going to be watching this collection, and doesn't need to
respond to
> status changes; but the client will be watching the *results*
collection, and
> might well want to respond to changes there. Sane?

Done.

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode70
state/action.go:70: err := a.st.runTransaction(ops)
On 2014/05/15 12:47:26, fwereade wrote:
> we should handle txn.ErrAborted and give a real error back -- in this
case it
> indicates (I think) that the action has already completed. Right?

Done.

https://codereview.appspot.com/98260043/diff/1/state/action_test.go
File state/action_test.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode8
state/action_test.go:8: SettingsSuite
On 2014/05/15 18:02:34, fwereade wrote:
> Ooh, didn't spot that: this will include all the Test* methods from
> SettingsSuite as well, and we don't want that. I'd recommend ConnSuite
(the one
> right here in state_test, not the one in... er juju/testing?).

Done.

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode27
state/action_test.go:27: }
On 2014/05/15 18:02:34, fwereade wrote:
> On 2014/05/15 16:23:28, johnweldon4 wrote:
> >
> > On 2014/05/15 12:47:26, fwereade wrote:
> > > FWIW we usually leave these out, but I think the explicitness of
this
> approach
> > > may actually be better. Don't feel obliged to include them if you
don't
> think
> > > it's necessary.
> >
> >
> > Do you mean the (SetUp|TearDown)(Suite|Test) funcs?
> > Because they're just calling the inner SettingsSuite funcs does that
mean the
> > test harness will do it automatically?

> Yeah, if you embed a type you get all its methods (so long as there's
no
> ambiguity, which there isn't in this case).

Done.

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode30
state/action_test.go:30: action, err :=
s.SettingsSuite.state.AddAction("fakeunit/0", "fakeaction", nil)
On 2014/05/15 18:02:34, fwereade wrote:
> On 2014/05/15 16:23:28, johnweldon4 wrote:
> > On 2014/05/15 12:47:27, fwereade wrote:
> > > Hmm, I think we *should* make sure that the unit exists and isn't
Dead.
> > >
> > > (I've been thinking, and have tentatively concluded that actions
on dying
> > units
> > > are fine, they just might not run in time; but actions on dead
units are
> > > definitely insane.)
> >
> >
> > In this case I'm actually not even testing any "real" unit. Of
course when we
> > do the refactor to make func (u* Unit) AddAction(...) then we'll
need a real
> > Unit for this test.
> > But this test is kind of superfluous any way. state_test has a very
s...

[John Weldon (johnweldon4)]

Please take a look.

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

[William Reade (fwereade)]

Looking good. Ping me when you're on, and we can go through the
transaction loop stuff.

https://codereview.appspot.com/98260043/diff/20001/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/20001/state/action.go#newcode8
state/action.go:8: Unit string
Drop the Unit field.

https://codereview.appspot.com/98260043/diff/20001/state/action.go#newcode31
state/action.go:31: // Id returns the mongo Id of the Action
even if it *is* the mongo id, we shouldn't *tell* people it is (if you
see what I mean).

I think that what we end up exposing to the user will want to be some
long hex string (identifiable by first few characters -- think git
revisions)... but let's not quibble about that now. This is the PK we
want, and until we expose the functionality we're free to fix it later.

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

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1314
state/unit.go:1314: func (u *Unit) AddAction(name string, payload
map[string]interface{}) (*Action, error) {
is it an interface{}, or a map[string]interface{}? I think it's the
latter, but then the doc needs to match as well.

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1332
state/unit.go:1332: // TODO(jcw4) transaction loop
also needs assert on unit life in the loop.

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

https://codereview.appspot.com/98260043/diff/20001/state/unit_test.go#newcode1299
state/unit_test.go:1299: c.Assert(action.Id(), gc.Matches,
"^u#"+s.unit.Name()+"#\\d+")
I'd drop the empty TokenTest in the other file and just add a couple of
bits here: check that you can get the action by Id, and that the objects
match.

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

[William Reade (fwereade)]

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

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1332
state/unit.go:1332: // TODO(jcw4) transaction loop
See
https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode932
for comments on loops -- ping me when you've read it.

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

[John Weldon (johnweldon4)]

Addressed all of these issues I believe.

https://codereview.appspot.com/98260043/diff/20001/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/20001/state/action.go#newcode8
state/action.go:8: Unit string
On 2014/05/20 07:12:17, fwereade wrote:
> Drop the Unit field.

Done.

https://codereview.appspot.com/98260043/diff/20001/state/action.go#newcode31
state/action.go:31: // Id returns the mongo Id of the Action
On 2014/05/20 07:12:17, fwereade wrote:
> even if it *is* the mongo id, we shouldn't *tell* people it is (if you
see what
> I mean).

> I think that what we end up exposing to the user will want to be some
long hex
> string (identifiable by first few characters -- think git
revisions)... but
> let's not quibble about that now. This is the PK we want, and until we
expose
> the functionality we're free to fix it later.

Done.

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

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1314
state/unit.go:1314: func (u *Unit) AddAction(name string, payload
map[string]interface{}) (*Action, error) {
On 2014/05/20 07:12:17, fwereade wrote:
> is it an interface{}, or a map[string]interface{}? I think it's the
latter, but
> then the doc needs to match as well.

Done.

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1332
state/unit.go:1332: // TODO(jcw4) transaction loop
On 2014/05/20 14:32:08, fwereade wrote:
> See
https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode932
> for comments on loops -- ping me when you've read it.

Done.

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

https://codereview.appspot.com/98260043/diff/20001/state/unit_test.go#newcode1299
state/unit_test.go:1299: c.Assert(action.Id(), gc.Matches,
"^u#"+s.unit.Name()+"#\\d+")
On 2014/05/20 07:12:17, fwereade wrote:
> I'd drop the empty TokenTest in the other file and just add a couple
of bits
> here: check that you can get the action by Id, and that the objects
match.

Done.

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

[John Weldon (johnweldon4)]

Please take a look.

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

[William Reade (fwereade)]

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

[John Weldon (johnweldon4)]

Addressed the few changes needed. Couple questions I'll ask about in
#juju-dev

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 {
On 2014/05/21 07:39:48, fwereade wrote:
> 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.

How is this different than doing a clone of the unit as you initially
suggested. I went this route because it seemed more direct and I
thought the db hit would be comparable, if not slightly lighter than a
full clone?

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1331
state/unit.go:1331: sel := bson.D{{"_id", newActionID}}
On 2014/05/21 07:39:48, fwereade wrote:
> 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.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1340
state/unit.go:1340: ops := []txn.Op{{
On 2014/05/21 07:39:48, fwereade wrote:
> 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.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1351
state/unit.go:1351: return newActionID, err
On 2014/05/21 07:39:48, fwereade wrote:
> returning the id is not "normal" for this package, but IMO it's a good
idea.
> thanks.

Done.

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)
On 2014/05/21 07:39:48, fwereade wrote:
> Add a note that this is temporary, and we shouldn't really leak the
actual _id.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode1335
state/unit_test.go:1335: unitDead := state.TransactionHook{
On 2014/05/21 07:39:48, fwereade wrote:
> s/unitDead/killUnit/ perhaps?

Done.

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
On 2014/05/21 07:39:48, fwereade wrote:
> Haha. I've thought of a way, and I kinda endorse you doing it, so l...

[John Weldon (johnweldon4)]

Please take a look.

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

[Martin Packman (gz)]

LGTM.

https://codereview.appspot.com/98260043/diff/60001/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/60001/state/action.go#newcode8
state/action.go:8: Payload map[string]interface{}
I think I'd comment these members. Clarifies thinks like what defines
the action name, and what determines the payload type.

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

https://codereview.appspot.com/98260043/diff/60001/state/unit_test.go#newcode1328
state/unit_test.go:1328: c.Assert(err, gc.NotNil)
Probably want to ErrorMatches this even though it's from below the
AddAction function.

https://codereview.appspot.com/98260043/diff/60001/state/unit_test.go#newcode1349
state/unit_test.go:1349: //TODO(jcw4): need to figure out how to inject
'contention' into the transaction
This is where we really want a generic contention helper so each add
doesn't have to futz around with testing its loop. I think William has
hopes for that at some point later.

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

[John Weldon (johnweldon4)]

Okay proposing again.

https://codereview.appspot.com/98260043/diff/60001/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/60001/state/action.go#newcode8
state/action.go:8: Payload map[string]interface{}
On 2014/05/21 20:35:25, gz wrote:
> I think I'd comment these members. Clarifies thinks like what defines
the action
> name, and what determines the payload type.

Done.

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

https://codereview.appspot.com/98260043/diff/60001/state/unit_test.go#newcode1328
state/unit_test.go:1328: c.Assert(err, gc.NotNil)
On 2014/05/21 20:35:25, gz wrote:
> Probably want to ErrorMatches this even though it's from below the
AddAction
> function.

Done.

https://codereview.appspot.com/98260043/diff/60001/state/unit_test.go#newcode1349
state/unit_test.go:1349: //TODO(jcw4): need to figure out how to inject
'contention' into the transaction
On 2014/05/21 20:35:25, gz wrote:
> This is where we really want a generic contention helper so each add
doesn't
> have to futz around with testing its loop. I think William has hopes
for that at
> some point later.

Yep

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

[John Weldon (johnweldon4)]

Please take a look.

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

[John Weldon (johnweldon4)]

Please take a look.

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

[William Reade (fwereade)]

LGTM with final trivials. Sorry :(.

https://codereview.appspot.com/98260043/diff/100001/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode6
state/action.go:6: Id string `bson:"_id"`
blank lines before comments please

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode9
state/action.go:9: // action to actually perform
Name identifies the action; it should match an action defined by the
unit's charm.

?

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode12
state/action.go:12: // when this action is being performed.
Payload holds the action's parameters, if any; it should validate
against the schema defined by the named action in the unit's charm.

?

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

https://codereview.appspot.com/98260043/diff/100001/state/unit.go#newcode1344
state/unit.go:1344: return newActionID, err
ah, dammit. We shouldn't really return newActionID if err != nil.
`switch err := ...; err {`?

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

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcode1295
state/unit_test.go:1295: func (s *UnitSuite) TestAddAction(c *gc.C) {
hey, can we move these to a new suite in action_test.go? unit_test is
already too damn big IMO.

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcode1311
state/unit_test.go:1311: c.Assert(action.Id(), gc.Matches,
"^u#"+s.unit.Name()+"#\\d+")
how about an assertSaneActionId? You can use it below where you're
currently discarding the new ids.

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcode1318
state/unit_test.go:1318: func (s *UnitSuite)
TestAddActionFailsOnDeadUnit(c *gc.C) {
TestAddActionLifecycle?

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcode1357
state/unit_test.go:1357: func (s *UnitSuite)
TestAddActionFailsOnExcessiveContention(c *gc.C) {
I'd just drop this.

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

[John Weldon (johnweldon4)]

Okay... addressed all
Running the tests and then I'll lbox propose

https://codereview.appspot.com/98260043/diff/100001/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode6
state/action.go:6: Id string `bson:"_id"`
On 2014/05/22 08:02:13, fwereade wrote:
> blank lines before comments please

Done.

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode9
state/action.go:9: // action to actually perform
On 2014/05/22 08:02:13, fwereade wrote:
> Name identifies the action; it should match an action defined by the
unit's
> charm.

> ?

Done.

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode12
state/action.go:12: // when this action is being performed.
On 2014/05/22 08:02:13, fwereade wrote:
> Payload holds the action's parameters, if any; it should validate
against the
> schema defined by the named action in the unit's charm.

> ?

Done.

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

https://codereview.appspot.com/98260043/diff/100001/state/unit.go#newcode1344
state/unit.go:1344: return newActionID, err
On 2014/05/22 08:02:13, fwereade wrote:
> ah, dammit. We shouldn't really return newActionID if err != nil.
`switch err :=
> ...; err {`?

Done.

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

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcode1295
state/unit_test.go:1295: func (s *UnitSuite) TestAddAction(c *gc.C) {
On 2014/05/22 08:02:14, fwereade wrote:
> hey, can we move these to a new suite in action_test.go? unit_test is
already
> too damn big IMO.

Done.

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcode1311
state/unit_test.go:1311: c.Assert(action.Id(), gc.Matches,
"^u#"+s.unit.Name()+"#\\d+")
On 2014/05/22 08:02:14, fwereade wrote:
> how about an assertSaneActionId? You can use it below where you're
currently
> discarding the new ids.

Done. but only applies to the places where we don't expect an error...

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcode1318
state/unit_test.go:1318: func (s *UnitSuite)
TestAddActionFailsOnDeadUnit(c *gc.C) {
On 2014/05/22 08:02:13, fwereade wrote:
> TestAddActionLifecycle?

Done.

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcode1357
state/unit_test.go:1357: func (s *UnitSuite)
TestAddActionFailsOnExcessiveContention(c *gc.C) {
On 2014/05/22 08:02:14, fwereade wrote:
> I'd just drop this.

Done.

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

[John Weldon (johnweldon4)]

Please take a look.

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

[William Reade (fwereade)]

LGTM, approving

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

[Go Bot (go-bot)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== added file 'state/action.go'
--- state/action.go	1970-01-01 00:00:00 +0000
+++ state/action.go	2014-05-23 14:50:36 +0000
@@ -0,0 +1,46 @@
+package state
+
+import ()
+
+type actionDoc struct {
+	Id string `bson:"_id"`
+
+	// Name identifies the action; it should match an action defined by
+	// the unit's charm.
+	Name string
+
+	// Payload holds the action's parameters, if any; it should validate
+	// against the schema defined by the named action in the unit's charm
+	Payload map[string]interface{}
+}
+
+// Action represents an instruction to do some "action" and is expected
+// to match an action definition in a charm.
+type Action struct {
+	st  *State
+	doc actionDoc
+}
+
+func newAction(st *State, adoc actionDoc) *Action {
+	return &Action{
+		st:  st,
+		doc: adoc,
+	}
+}
+
+// Name returns the name of the Action
+func (a *Action) Name() string {
+	return a.doc.Name
+}
+
+// Id returns the id of the Action
+func (a *Action) Id() string {
+	return a.doc.Id
+}
+
+// Payload will contain a structure representing arguments or parameters to
+// an action, and is expected to be validated by the Unit using the Charm
+// definition of the Action
+func (a *Action) Payload() map[string]interface{} {
+	return a.doc.Payload
+}

=== added file 'state/action_test.go'
--- state/action_test.go	1970-01-01 00:00:00 +0000
+++ state/action_test.go	2014-05-23 14:50:36 +0000
@@ -0,0 +1,99 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package state_test
+
+import (
+	jc "github.com/juju/testing/checkers"
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/state"
+)
+
+type ActionSuite struct {
+	ConnSuite
+	charm   *state.Charm
+	service *state.Service
+	unit    *state.Unit
+}
+
+var _ = gc.Suite(&ActionSuite{})
+
+func (s *ActionSuite) SetUpTest(c *gc.C) {
+	s.ConnSuite.SetUpTest(c)
+	s.charm = s.AddTestingCharm(c, "wordpress")
+	var err error
+	s.service = s.AddTestingService(c, "wordpress", s.charm)
+	c.Assert(err, gc.IsNil)
+	s.unit, err = s.service.AddUnit()
+	c.Assert(err, gc.IsNil)
+	c.Assert(s.unit.Series(), gc.Equals, "quantal")
+}
+
+func (s *ActionSuite) TestAddAction(c *gc.C) {
+	actionName := "fakeaction"
+	actionParams := map[string]interface{}{"outfile": "outfile.tar.bz2"}
+
+	// verify can add an Action
+	actionId, err := s.unit.AddAction(actionName, actionParams)
+	c.Assert(err, gc.IsNil)
+	assertSaneActionId(c, actionId, s.unit.Name())
+
+	// verify we can get it back out by Id
+	action, err := s.State.Action(actionId)
+	c.Assert(err, gc.IsNil)
+	c.Assert(action, gc.NotNil)
+	c.Assert(action.Id(), gc.Equals, actionId)
+
+	// verify we get out what we put in
+	c.Assert(action.Name(), gc.Equals, actionName)
+	c.Assert(action.Payload(), jc.DeepEquals, actionParams)
+}
+
+func (s *ActionSuite) TestAddActionLifecycle(c *gc.C) {
+	unit, err := s.State.Unit(s.unit.Name())
+	c.Assert(err, gc.IsNil)
+	preventUnitDestroyRemove(c, unit)
+
+	// make unit state Dying
+	err = unit.Destroy()
+	c.Assert(err, gc.IsNil)
+
+	// can add action to a dying unit
+	actionId, err := unit.AddAction("fakeaction1", map[string]interface{}{})
+	c.Assert(err, gc.IsNil)
+	assertSaneActionId(c, actionId, s.unit.Name())
+
+	// make sure unit is dead
+	err = unit.EnsureDead()
+	c.Assert(err, gc.IsNil)
+
+	// cannot add action to a dead unit
+	_, err = unit.AddAction("fakeaction2", map[string]interface{}{})
+	c.Assert(err, gc.ErrorMatches, "unit .* is dead")
+}
+
+func (s *ActionSuite) TestAddActionFailsOnDeadUnitInTransaction(c *gc.C) {
+	unit, err := s.State.Unit(s.unit.Name())
+	c.Assert(err, gc.IsNil)
+	preventUnitDestroyRemove(c, unit)
+
+	killUnit := state.TransactionHook{
+		Before: func() {
+			c.Assert(unit.Destroy(), gc.IsNil)
+			c.Assert(unit.EnsureDead(), gc.IsNil)
+		},
+	}
+	defer state.SetTransactionHooks(c, s.State, killUnit).Check()
+
+	_, err = unit.AddAction("fakeaction", map[string]interface{}{})
+	c.Assert(err, gc.ErrorMatches, "unit .* is dead")
+}
+
+// assertSaneActionId verifies that the actionId is of the expected
+// form (unit id prefix + sequence)
+// This is a temporary assertion, we shouldn't be leaking the actual
+// mongo _id
+func assertSaneActionId(c *gc.C, actionId, unitName string) {
+	c.Assert(actionId, gc.Matches, "^u#"+unitName+"#\\d+")
+}

=== modified file 'state/open.go'
--- state/open.go	2014-05-13 04:30:48 +0000
+++ state/open.go	2014-05-23 14:50:36 +0000
@@ -292,6 +292,7 @@
 		settingsrefs:      db.C("settingsrefs"),
 		constraints:       db.C("constraints"),
 		units:             db.C("units"),
+		actions:           db.C("actions"),
 		users:             db.C("users"),
 		presence:          pdb.C("presence"),
 		cleanups:          db.C("cleanups"),

=== modified file 'state/state.go'
--- state/state.go	2014-05-20 22:58:59 +0000
+++ state/state.go	2014-05-23 14:50:36 +0000
@@ -1,7 +1,7 @@
 // Copyright 2012, 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-// The state package enables reading, observing, and changing
+// Package state enables reading, observing, and changing
 // the state stored in MongoDB of a whole environment
 // managed by juju.
 package state
@@ -64,6 +64,7 @@
 	settingsrefs      *mgo.Collection
 	constraints       *mgo.Collection
 	units             *mgo.Collection
+	actions           *mgo.Collection
 	users             *mgo.Collection
 	presence          *mgo.Collection
 	cleanups          *mgo.Collection
@@ -316,7 +317,7 @@
 	}
 
 	validAttrs := validCfg.AllAttrs()
-	for k, _ := range oldConfig.AllAttrs() {
+	for k := range oldConfig.AllAttrs() {
 		if _, ok := validAttrs[k]; !ok {
 			settings.Delete(k)
 		}
@@ -1394,6 +1395,19 @@
 	return rdc[i].Id < rdc[j].Id
 }
 
+// Action returns an Action by Id.
+func (st *State) Action(id string) (*Action, error) {
+	doc := actionDoc{}
+	err := st.actions.FindId(id).One(&doc)
+	if err == mgo.ErrNotFound {
+		return nil, errors.NotFoundf("action %q", id)
+	}
+	if err != nil {
+		return nil, fmt.Errorf("cannot get action %q: %v", id, err)
+	}
+	return newAction(st, doc), nil
+}
+
 // Unit returns a unit by name.
 func (st *State) Unit(name string) (*Unit, error) {
 	if !names.IsUnit(name) {

=== modified file 'state/unit.go'
--- state/unit.go	2014-05-20 17:57:10 +0000
+++ state/unit.go	2014-05-23 14:50:36 +0000
@@ -803,6 +803,7 @@
 	return fmt.Sprintf("unit %q is not assigned to a machine", e.Unit)
 }
 
+// IsNotAssigned verifies that err is an instance of NotAssignedError
 func IsNotAssigned(err error) bool {
 	_, ok := err.(*NotAssignedError)
 	return ok
@@ -1327,6 +1328,49 @@
 	return nil
 }
 
+// AddAction adds a new Action of type name and using arguments payload to
+// this Unit, and returns its ID
+func (u *Unit) AddAction(name string, payload map[string]interface{}) (string, error) {
+
+	prefix := fmt.Sprintf("u#%s#", u.doc.Name)
+	suffix, err := u.st.sequence(prefix)
+	if err != nil {
+		return "", fmt.Errorf("cannot assign new sequence for prefix '%s'. %s", prefix, err)
+	}
+
+	newActionID := fmt.Sprintf("%s%d", prefix, suffix)
+
+	doc := actionDoc{Id: newActionID, Name: name, Payload: payload}
+	ops := []txn.Op{{
+		C:      u.st.units.Name,
+		Id:     u.doc.Name,
+		Assert: notDeadDoc,
+	}, {
+		C:      u.st.actions.Name,
+		Id:     doc.Id,
+		Assert: txn.DocMissing,
+		Insert: doc,
+	}}
+
+	for i := 0; i < 3; i++ {
+		if notDead, err := isNotDead(u.st.units, u.doc.Name); err != nil {
+			return "", err
+		} else if !notDead {
+			return "", fmt.Errorf("unit %q is dead", u)
+		}
+
+		switch err := u.st.runTransaction(ops); err {
+		case txn.ErrAborted:
+			continue
+		case nil:
+			return newActionID, nil
+		default:
+			return "", err
+		}
+	}
+	return "", ErrExcessiveContention
+}
+
 // Resolve marks the unit as having had any previous state transition
 // problems resolved, and informs the unit that it may attempt to
 // reestablish normal workflow. The retryHooks parameter informs