fix the reconcile flow #1111
Merged
fix the reconcile flow #1111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/assign @gaocegege @richardsliu |
Travis tests have failedHey @ChanYiLin, 1st Buildhack/verify-codegen.shgoveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/client/*/*.go,pkg/client/*/*/*.go,pkg/client/*/*/*/*.go,pkg/client/*/*/*/*/*.go,pkg/client/*/*/*/*/*/*.go,pkg/client/*/*/*/*/*/*/*.go,pkg/util/*.go,pkg/util/*/*.go,pkg/apis/tensorflow/*/zz_generated.*.go,pkg/apis/tensorflow/*/*_generated.go,pkg/apis/common/*/zz_generated.*.go,pkg/apis/common/*/*_generated.go"TravisBuddy Request Identifier: 69e8a340-1a6e-11ea-8ac4-55ffd8dc6fb9 |
|
Hey @ChanYiLin, TravisBuddy Request Identifier: 18b88040-1a72-11ea-8ac4-55ffd8dc6fb9 |
|
/retest |
gaocegege
reviewed
Dec 10, 2019
| if tc.Config.EnableGangScheduling { | ||
| minAvailableReplicas := getTotalReplicas(tfjob) | ||
| _, err := tc.SyncPodGroup(tfjob, minAvailableReplicas) | ||
| err := updateTFJobConditions(tfjob, common.JobFailed, tfJobFailedReason, failureMessage) | ||
| if err != nil { |
There was a problem hiding this comment.
Suggested change
| if err != nil { | |
| if err := updateTFJobConditions( | |
| tfjob, common.JobFailed, tfJobFailedReason, failureMessage); err != nil { |
|
Could you please update pytorch-operator, too? |
Sure, no problem! |
gaocegege
reviewed
Dec 10, 2019
|
Hey @ChanYiLin, TravisBuddy Request Identifier: 52f49570-1af2-11ea-b9ce-6f43500ed087 |
…e and backofflimit
|
Hey @ChanYiLin, TravisBuddy Request Identifier: d26bab50-1af6-11ea-b9ce-6f43500ed087 |
gaocegege
reviewed
Dec 10, 2019
|
Since there is no more reconcile after completion, does it also solve #965? |
|
I think so. |
Yes, by moving The complete jobs will only do the cleanup process(if set to cleanup) then return nil(if no status update) |
|
Does it improve the performance issue in #965? After a relook, It looks like the the return conditions are exactly same as before. Cleanup check will happen for every reconcile call(before and after the changes) and if job status hasn't changed, the control returns instead of further reconcile(before and after the changes) I can merge this unless you have some thoughts on it. |
|
@johnugeorge Yes you can merge this PR first, I think this is enough for this PR. Also I can create the same PR to the pytorch-operator then. Thanks! For the performance issue, we can discuss in #965 . However this PR indeed prevents checking backofflimit and activedeadline for those complete jobs which might have some help. |
|
Great. Thanks @ChanYiLin |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnugeorge 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 |
Jeffwan
added a commit
to Jeffwan/common
that referenced
this pull request
May 17, 2020
This is originally from kubeflow/training-operator#1111 Signed-off-by: Jiaxin Shan <[email protected]>
k8s-ci-robot
pushed a commit
to kubeflow/common
that referenced
this pull request
May 17, 2020
) * Skip check activeDeadline or backoffLimit if job terminated This is originally from kubeflow/training-operator#1111 Signed-off-by: Jiaxin Shan <[email protected]> * Add PodGroup reconcile logic This is missing in kubeflow/common. We need this to make sure minAvailableReplicas is correct in PodGroup for each training job Signed-off-by: Jiaxin Shan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the tfjob has already terminated, we don't need to check activedeadline and backofflimit.
Originally, even the job has terminated it still checks the Activedeadline and appends the event to it.
So the event that shows the job failed after it succeeded might happen as follow,
the log of tf-operator will also keep showing the failure massage of past Activedeadline
In this MR, I reorder the checking process so if the job has terminated(Succeed, Failed), it will return instead of further reconcile.
This change is