state: unit with subordinates cannot become Dead

state: unit with subordinates cannot become Dead



https://code.launchpad.net/~fwereade/juju-core/unit-death-restrictions/+merge/141930

Requires: https://code.launchpad.net/~fwereade/juju-core/machine-death-restrictions/+merge/141088

(do not edit description out of merge proposal)

[fwereade, 2013-01-04 15:07:27]

Please take a look.

[fwereade, 2013-01-04 21:52:52]

On 2013/01/04 15:07:27, fwereade wrote:
> Please take a look.

Whoops, I think I need to merge juju-core back into the parent and repropose.
Sorry.

[fwereade, 2013-01-07 09:09:35]

Please take a look.

[fwereade, 2013-01-07 09:12:42]

On 2013/01/07 09:09:35, fwereade wrote:
> Please take a look.

OK, this is a complete screwup, I could swear I reverted modes.go which is
tested in the followup. Merging that (https://codereview.appspot.com/7039056/)
in here and dropping that one, I think.

[fwereade, 2013-01-07 09:20:26]

Please take a look.

[rog, 2013-01-07 13:45:52]

looks good except the below.

https://codereview.appspot.com/7035060/diff/7001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/7035060/diff/7001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:198: case _, ok := <-w.Changes():
as discussed on line, it's not clear in the state API that you'll see a change
when the number of subordinate units changes. i think that should be made clear
(possibly in another CL) by updating the documentation, and probably adding
another method to fetch the count or names of the unit (or even just a
"HasSubordinates" method).

[fwereade, 2013-01-07 20:32:41]

On 2013/01/07 13:45:52, rog wrote:
> looks good except the below.
> 
> https://codereview.appspot.com/7035060/diff/7001/worker/uniter/modes.go
> File worker/uniter/modes.go (right):
> 
>
https://codereview.appspot.com/7035060/diff/7001/worker/uniter/modes.go#newco...
> worker/uniter/modes.go:198: case _, ok := <-w.Changes():
> as discussed on line, it's not clear in the state API that you'll see a change
> when the number of subordinate units changes. i think that should be made
clear
> (possibly in another CL) by updating the documentation, and probably adding
> another method to fetch the count or names of the unit (or even just a
> "HasSubordinates" method).

I'm least uncomfortable with HasSubordinates, and I remain uncertain about watch
documentation; opened lp:1097018 to track doc suggestion.

Honestly, though, IMO using HasSubordinates makes the modes.go code strictly
worse :-/.

[fwereade, 2013-01-07 20:38:50]

Please take a look.

[rog, 2013-01-08 15:49:41]

LGTM

https://codereview.appspot.com/7035060/diff/11001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/7035060/diff/11001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:205: if u.unit.HasSubordinates() {
although I suggested you add Unit.HasSubordinates, i don't think it's worth
actually calling here :-)

i'd just go with the old code. my gripe was with state, not here.

[TheMue, 2013-01-08 16:22:24]

Mostly LGTM, one comment.

https://codereview.appspot.com/7035060/diff/11001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/7035060/diff/11001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:208: if err := u.unit.EnsureDead(); err ==
state.ErrUnitHasSubordinates {
This error should not be returned if HasSubordinates() returned true and the
loop continued before. So IMHO the test before is not needed.

[fwereade, 2013-01-08 20:57:11]

*** Submitted:

state: unit with subordinates cannot become Dead

R=rog, TheMue
CC=
https://codereview.appspot.com/7035060

https://codereview.appspot.com/7035060/diff/11001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/7035060/diff/11001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:205: if u.unit.HasSubordinates() {
On 2013/01/08 15:49:41, rog wrote:
> although I suggested you add Unit.HasSubordinates, i don't think it's worth
> actually calling here :-)
> 
> i'd just go with the old code. my gripe was with state, not here.

Given TheMue's observation, I think I'd prefer to drop the specific-error
checking below.

https://codereview.appspot.com/7035060/diff/11001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:208: if err := u.unit.EnsureDead(); err ==
state.ErrUnitHasSubordinates {
On 2013/01/08 16:22:24, TheMue wrote:
> This error should not be returned if HasSubordinates() returned true and the
> loop continued before. So IMHO the test before is not needed.

This is only correct given an additional assumption (that the unit is Dying)...
which should indeed always be true, so I guess we can drop it. Cheers.