Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
The uniter now sets workload and agent status according to the new spec #1964
Conversation
wallyworld
added some commits
Mar 27, 2015
fwereade
reviewed
Mar 30, 2015
| @@ -175,13 +194,10 @@ func ModeAbide(u *Uniter) (next Mode, err error) { | ||
| return nil, errors.Trace(err) | ||
| } | ||
| if !u.ranConfigChanged { | ||
| - return continueAfter(u, newSimpleRunHookOp(hooks.ConfigChanged)) | ||
| + return continueAfter(u, "running config-changed hook", newSimpleRunHookOp(hooks.ConfigChanged)) | ||
| } | ||
| if !opState.Started { |
fwereade
Mar 30, 2015
Contributor
This feels like it should be part of the op, but I think it's ok as a transitional state,
fwereade
reviewed
Mar 30, 2015
| @@ -207,11 +223,15 @@ func ModeAbide(u *Uniter) (next Mode, err error) { | ||
| // is in an Alive state. | ||
| func modeAbideAliveLoop(u *Uniter) (Mode, error) { | ||
| for { | ||
| + if err := setAgentStatus(u, params.StatusIdle, "", nil); err != nil { |
fwereade
Mar 30, 2015
Contributor
I'm worried about this -- ModeAbide doesn't imply idle. A few seconds without some other channel triggerinng does imply idle, but just entering ModeAbide doesn't.
fwereade
reviewed
Mar 30, 2015
| @@ -221,22 +241,30 @@ func modeAbideAliveLoop(u *Uniter) (Mode, error) { | ||
| return ModeUpgrading(curl), nil | ||
| case ids := <-u.f.RelationsEvents(): | ||
| creator = newUpdateRelationsOp(ids) | ||
| + userMessage = "updating relation ids" |
fwereade
Mar 30, 2015
Contributor
do we really want this level of detail? This feels like pure implementation...
fwereade
reviewed
Mar 30, 2015
| case hookInfo := <-u.storage.Hooks(): | ||
| creator = newRunHookOp(hookInfo) | ||
| + userMessage = fmt.Sprintf("running %s hook", hookInfo.Kind) | ||
| } |
fwereade
Mar 30, 2015
Contributor
...and it's really only when we hit a default case here that we can reasonably say "idle"...
...but we can't reasonably say that, I think, without a timeout for "something interesting" to happen.
fwereade
reviewed
Mar 30, 2015
| case <-u.f.ConfigEvents(): | ||
| creator = newSimpleRunHookOp(hooks.ConfigChanged) | ||
| + userMessage = "running config-changed hook" |
fwereade
Mar 30, 2015
Contributor
Is this useful as a pure "user message"? If we're going to expose what we're running, don't we want to make it machine-consumable?
fwereade
reviewed
Mar 30, 2015
| @@ -304,7 +336,10 @@ func ModeHookError(u *Uniter) (next Mode, err error) { | ||
| u.f.WantResolvedEvent() | ||
| u.f.WantUpgradeEvent(true) | ||
| for { | ||
| - if err = u.unit.SetUnitStatus(params.StatusError, statusMessage, statusData); err != nil { | ||
| + // The spec says we should set the workload status to Error, but that's crazy talk. |
fwereade
reviewed
Mar 30, 2015
| case params.ResolvedNoHooks: | ||
| creator = newSkipHookOp(hookInfo) | ||
| + userMessage = fmt.Sprintf("finishing %s hook", hookInfo.Kind) |
fwereade
Mar 30, 2015
Contributor
quibble quibble: "skipping %s hook"? that's what we're really doing
fwereade
reviewed
Mar 30, 2015
| @@ -330,7 +368,7 @@ func ModeHookError(u *Uniter) (next Mode, err error) { | ||
| } | ||
| return ModeContinue, nil | ||
| case actionId := <-u.f.ActionEvents(): | ||
| - if err := u.runOperation(newActionOp(actionId)); err != nil { | ||
| + if err := u.runOperation(newActionOp(actionId), fmt.Sprintf("running action %q", actionId)); err != nil { |
fwereade
Mar 30, 2015
Contributor
I really do worry about the degree of spam we're subjecting the api server to here -- if a user cares that we're executing X action, that should be recorded against the action itself, shouldn't it?
fwereade
reviewed
Mar 30, 2015
| @@ -344,21 +382,27 @@ func ModeConflicted(curl *charm.URL) Mode { | ||
| return func(u *Uniter) (next Mode, err error) { | ||
| defer modeContext("ModeConflicted", &err)() | ||
| // TODO(mue) Add helpful data here too in later CL. | ||
| - if err = u.unit.SetUnitStatus(params.StatusBlocked, "upgrade failed", nil); err != nil { | ||
| + // The spec says we should set the workload status to Error, but that's crazy talk. |
fwereade
Mar 30, 2015
Contributor
does the spec really deal with modeconflicted here? I don't believe that anyone who wrote the spec has the faintest understanding of what modeconflicted actually implies...
wallyworld
Mar 30, 2015
Owner
In the case of conflicted, the agent is is simply set to reflect that upgrade failed.
fwereade
reviewed
Mar 30, 2015
| @@ -374,8 +418,8 @@ func modeContext(name string, err *error) func() { | ||
| // continueAfter is commonly used at the end of a Mode func to execute the | ||
| // operation returned by creator and return ModeContinue (or any error). | ||
| -func continueAfter(u *Uniter, creator creator) (Mode, error) { | ||
| - if err := u.runOperation(creator); err != nil { | ||
| +func continueAfter(u *Uniter, doing string, creator creator) (Mode, error) { |
fwereade
Mar 30, 2015
Contributor
I am bothered by this -- if we're performing some operation, isn't it up to the operation to know what we're doing?
fwereade
reviewed
Mar 30, 2015
| - } | ||
| - err := opc.u.unit.SetAgentStatus(status, "", nil) | ||
| - if err != nil { | ||
| - return "", err | ||
| } |
fwereade
Mar 30, 2015
Contributor
so +1 to simplifying this but I'm not sure we're pushing in the right direction... given the level of detail we seem to be going into, shouldn't we be moving this status-reporting more towards the ops than into the modes?
wallyworld
Mar 30, 2015
Owner
Yes, I've reworked it so that the executing status is set in the relevant operations implementation (run hook, run action, run commands) rather than in modes.
wallyworld
added some commits
Mar 30, 2015
wallyworld commentedMar 27, 2015
The uniter updates both workload and agent status according to the Juju Unit and Service Status spec.
When first deploying a charm, its status is "unknown" and is waiting for the machine to be allocated. Then, the unit status goes to "maintenance" when the install hook is run. After the Start hook as completed, uniter will set the unit status to "unknown", but only if the charm hasn't called status-set itself.
New beforeHook() and afterHook() methods are added to the hook runner. These perform the necessary unit status setting, and an attribute on the run context tracks any status-set calls. The local uniter state also persists this attribute.
The agent status is also maintained, using the uniter Mode functions. These primarily set the agent status to "executing" when running a hook or action etc, and back to "idle" when waiting for events. If any error occurs, the agent status becomes "failed".