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

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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

(Based on my immature knowledge.)

A hook error can occur before the unit is "started": if either of the
install or config-changed hooks fail. Moreover, I don't see any blocking
states where the unit is not started, and not in ModeHookError. Please
see my proposed solution in the inline response.

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
On 2013/08/09 13:52:20, fwereade wrote:
> Hitherto, were we in ModeAbide, we must have been started.

Understood.

https://codereview.appspot.com/12517043/diff/1/worker/uniter/modes.go#newcode348
worker/uniter/modes.go:348: return modeAbideDyingLoop(u)
On 2013/08/09 13:52:20, fwereade wrote:
> 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.

Sorry, a little more subtle than I thought. Here's what I was thinking
here:

In modeAbideDyingLoop, (unless the unit hasn't started) each of the
unit's relationers is set to dying, and then the unit then waits for
relation hooks. If the hook fails, then we go back to ModeHookError,
whereupon we'll return to modeAbideDyingLoop on account of the unit
being in Dying state, and continue to execute hooks until they're all
depleted.

So, from your comments it sounds like the unit must remain in Dying
state until the user resolves whatever problem is preventing the -broken
hook from successfully completing.

Anyway, I overshot; I should only have modified behaviour for when the
unit hasn't started. How about I modify ModeHookError's new select
clause to be a function call that returns nil if the agent is started,
else u.f.UnitDying()? And then go straight to ModeTerminating. AFAICT,
if a unit is blocked and not started, then it is in ModeHookError
(install or config-changed).

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

« Back to merge proposal