Code review comment for lp://staging/~axwalk/juju-core/lp1176740-destroy-unit-short-circuit

Revision history for this message
William Reade (fwereade) wrote :

Sorry, NOT LGTM as it stands. I think it's a matter of:

select {
case <- u.f.UnitDying():
     ...
default:
}

...at appropriate points in the modes which *can* run before we're
started, and short-circuiting to ModeTerminating only out of those. Once
relations and hook errors could be involved, the proposed approach
becomes, er, risky at best.

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

https://codereview.appspot.com/12517043/diff/1/worker/uniter/modes.go#newcode291
worker/uniter/modes.go:291: return ModeTerminating, nil
Hitherto, were we in ModeAbide, we must have been started.

https://codereview.appspot.com/12517043/diff/1/worker/uniter/modes.go#newcode348
worker/uniter/modes.go:348: return modeAbideDyingLoop(u)
I don't think this is right. If we've had a hook error we're in an
unknown state, and subsequent hooks are also likely to fail; and if we
fail a -broken hook we won't depart the relation; and if a unit goes
away without departing all its relations, those relations (and their
services) will become unkillable. Which would be a Bad Thing.

https://codereview.appspot.com/12517043/

« Back to merge proposal