Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Uniter reacts to relation suspended flag; new suspending/joining states #7857
Conversation
| return errors.Trace(err) | ||
| } | ||
| - if currentStatus.Status != relStatus || currentStatus.Message != change.StatusMessage { | ||
| + if *change.Suspended { |
axw
Sep 15, 2017
Member
maybe dedup the below, so it looks like:
st := status.Suspending
action := "suspending"
if !*change.Suspended {
st = status.Joining
action = "resuming"
}
<same code here, using st & action>| + return common.ErrPerm | ||
| + } else if err != nil { | ||
| + return errors.Trace(err) | ||
| + } |
axw
Sep 15, 2017
Member
check here that the relation involves the unit, and return common.ErrPerm if not?
axw
Sep 15, 2017
Member
also, further to later comments, I think we need to require that the unit is the application leader
| @@ -116,26 +116,33 @@ func (r *Relation) Status() (status.StatusInfo, error) { | ||
| // SetStatus sets the status of the relation. | ||
| func (r *Relation) SetStatus(statusInfo status.StatusInfo) error { | ||
| + logger.Criticalf("SET STATUS: %+v", statusInfo) |
| + return errors.Trace(err) | ||
| + } | ||
| + rel := r.ru.Relation() | ||
| + if r.suspended { |
axw
Sep 15, 2017
Member
I think you want to set the status first, in case something goes wrong in between. As it is, if the agent failed after die(), and then came back up, it wouldn't set status (I think?)
wallyworld
Sep 15, 2017
Owner
moved setting the status and refactored a bit so that a failure to set status does not prevent cleanup but is still reported as an error
| + } | ||
| + rel := r.ru.Relation() | ||
| + if r.suspended { | ||
| + return rel.SetStatus(relation.Suspended) |
axw
Sep 15, 2017
Member
Oof, I just realised something: I don't think we can set relation status from any old unit. I think it has to be the application leader, otherwise they could trample each other.
axw
requested changes
Sep 15, 2017
and please look at moving the relation SetStatus to worker/uniter/operation
| @@ -1428,13 +1428,25 @@ func (u *UniterAPI) WatchRelationUnits(args params.RelationUnits) (params.Relati | ||
| func (u *UniterAPI) SetRelationStatus(args params.RelationStatusArgs) (params.ErrorResults, error) { | ||
| var statusResults params.ErrorResults | ||
| + checker := u.st.LeadershipChecker() | ||
| + token := checker.LeadershipCheck(u.unit.ApplicationName(), u.unit.Name()) |
axw
Sep 15, 2017
Member
this token should be passed down to the SetStatus call. state.setStatusParams has room for a token.
wallyworld
Sep 15, 2017
Owner
Trouble is SetStatus() is an interface method from the StatusSetter interface. So we'd need to do some refactoring. The application facade also doesn't use the token in the desired way. Could we keep as is and refactor holistically in another PR?
axw
Sep 15, 2017
Member
I'm OK with that, since it's non critical. Please file a bug and leave a TODO(wallyworld).
FWIW, I don't think it's necessary to pass a token in the application facade (I assume you're referring to SetRelationsSuspended), because that's not being done by the application leader, but rather a client. And that code's meant to be going away anyway, right? In fact I think this PR is meant to be replacing it... except you're not setting the Joined status yet.
wallyworld
Sep 15, 2017
Owner
Joined is only set by the uniter. When resuming a relation, the resume op sets the status to Joining.
The thing I'm referring to is setting the application status on behalf of the unit leader - it also uses the pattern here, ie grab and check a token before calling SetStatus
axw
Sep 15, 2017
Member
Joined is only set by the uniter. When resuming a relation, the resume op sets the status to Joining.
See SetRelationsSuspended in the application facade, specifically the TODO you left.
The thing I'm referring to is setting the application status on behalf of the unit leader - it also uses the pattern here, ie grab and check a token before calling SetStatus
Write
Ah, right. Yes, that's historical, and should also be fixed (later).
wallyworld
Sep 15, 2017
Owner
There's no TODO left in SetRelationsSuspended
The facade code sets the status to Joining or Suspending as needed.
axw
reviewed
Sep 15, 2017
Thanks, I think that looks better. LGTM once the token is propagated to setStatus
| Status: string(status.Unknown), | ||
| }) | ||
| + case hooks.RelationBroken: | ||
| + var isLeader bool | ||
| + isLeader, err = ctx.IsLeader() |
axw
Sep 15, 2017
Member
check error.
also, if !isLeader, you should just break here. no point getting the relation and so on
| - return false, err | ||
| - } | ||
| - return hasRunStatusSet, nil | ||
| + return hasRunStatusSet && err == nil, err |
axw
Sep 15, 2017
Member
what's the point of this change? the first value should be ignored if err != nil anyway
|
!!build!! |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
wallyworld commentedSep 15, 2017
Description of change
The uniter now listens to the relation suspended flag to determine if a relation needs to be suspended, rather than status itself. Two new status values are introduced - "joining" and "suspending". When the CLI updates a relation status flag, the status changes to one of the above values; when the uniter has processed the change, it then sets "suspended" or "joined" accordingly. A relation status also starts out as "joining" until the uniter first runs the necessary hooks.
QA steps
Bootstrap a CMR scenario, mediawiki related to mysql in another model.
watch juju status on the mediawiki model
in the mysql model:
suspend the remote relation
observe the status change in the watch status to suspending and then suspended, relation becomes broken
resume the remote relation
observe the status change in the watch status to joining and then joined, relation becomes ok again
for both the above, listing offers in mysql model reflects the correct status