controller: Separate ps and worker pods #481
Conversation
Signed-off-by: Ce Gao <gaoce@caicloud.io>
Signed-off-by: Ce Gao <gaoce@caicloud.io>
Signed-off-by: Ce Gao <gaoce@caicloud.io>
| @@ -135,7 +136,7 @@ func (tc *TFJobController) reconcilePods( | |||
| } | |||
|
|
|||
| // Update the active status since we have created -diff pods during the loop. | |||
| tfjob.Status.TFReplicaStatuses[rtype].Active = int32(len(activePods) - diff) | |||
| tfjob.Status.TFReplicaStatuses[rtype].Active = expected | |||
There was a problem hiding this comment.
What if expected is negative?
There was a problem hiding this comment.
I think it will never happen sine succeeded pods always < replicas, WDYT
There was a problem hiding this comment.
How about use activePods directly to make the code more readability?
There was a problem hiding this comment.
activePods is outdated since we have already created or deleted some pods during the loop
There was a problem hiding this comment.
But creation maybe failed so we should only trust the active pods we get in this loop.
There was a problem hiding this comment.
I think if the creation is failed, the functions returns errors and the control flow can not reach here.
There was a problem hiding this comment.
I mean creation API is return ok, but Pod are not running successfully because some scheduling or resource limit error, in another word Pod are pending not active.
There was a problem hiding this comment.
In our definition pending pods is also active pods 🤔
func IsPodActive(p *v1.Pod) bool {
return v1.PodSucceeded != p.Status.Phase &&
v1.PodFailed != p.Status.Phase &&
p.DeletionTimestamp == nil
}
There was a problem hiding this comment.
I opened a new issue #484
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ScorpioCPH The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @ScorpioCPH
We do not separate the pods of PS and workers in the controller, then it has bugs when the replicas are succeeded.
If the PR LGTM, I will file another PR for service.
Signed-off-by: Ce Gao gaoce@caicloud.io
This change is