Issue 98260043: Actions: Work in Progress (Closed)
DescriptionActions: 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)
Patch Set 1 #
Total comments: 29
Patch Set 2 : Actions: Work in Progress #
Total comments: 11
Patch Set 3 : Actions: Work in Progress #
Total comments: 14
Patch Set 4 : Actions: Work in Progress #
Total comments: 6
Patch Set 5 : Actions: Work in Progress #Patch Set 6 : Actions: Work in Progress #
Total comments: 16
Patch Set 7 : Actions: Work in Progress #
MessagesTotal messages: 22
Please take a look.
Sign in to reply to this message.
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/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()) It might be nice to have some sort of check on the form of the Id.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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 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. Done. 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 16:23:29, johnweldon4 wrote: > On 2014/05/15 12:47:27, fwereade wrote: > > thanks for this cleanup btw > > I love cleanup :) Done. 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/16 05:37:30, johnweldon4 wrote: > 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? Done. https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1395 state/state.go:1395: return nil, onAbort(err, errDead) On 2014/05/15 16:23:28, johnweldon4 wrote: > 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 Done. 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 16:23:29, johnweldon4 wrote: > 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. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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#newcode... 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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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#newcode... 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.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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#newcode... 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#newcode... state/unit_test.go:1335: unitDead := state.TransactionHook{ s/unitDead/killUnit/ perhaps? https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode... 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.)
Sign in to reply to this message.
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#newcode... 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#newcode... 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#newcode... 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 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.) 1. 'dirty as sin', and 2. lots of work Hmm, since you gave me an out I'd be happy to skip :)
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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#newcode... 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#newcode... 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.
Sign in to reply to this message.
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#newcode... 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#newcode... 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
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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#newcod... 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#newcod... 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#newcod... state/unit_test.go:1318: func (s *UnitSuite) TestAddActionFailsOnDeadUnit(c *gc.C) { TestAddActionLifecycle? https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcod... state/unit_test.go:1357: func (s *UnitSuite) TestAddActionFailsOnExcessiveContention(c *gc.C) { I'd just drop this.
Sign in to reply to this message.
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#newcod... 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#newcod... 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#newcod... 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#newcod... 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.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
