Set state to failed if there is a problem initializing job #219
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
* Clean up the code; there's no longer a reason to have a separate function triggerCreatePhase. * Refactor setup to ensure we always set phase and state. * We should always call updateTPRStatus after setup is called * Before the update was called in two different places (e.g. triggerCreatPhase) depending on the state.
* Call reconcile before the loop and inside it.
|
@ wackxu could you please review this? |
|
@wackxu could you please review this? |
zjj2wry
reviewed
Dec 13, 2017
pkg/trainer/training.go
Outdated
| err := j.job.Spec.SetDefaults() | ||
| if err != nil { | ||
| return fmt.Errorf("there was a problem setting defaults for job spec: %v", err) | ||
| j.status.SetReason("Internal error; tried to setup a job with no spec.") |
There was a problem hiding this comment.
maybe tried to --> failed to would be better
zjj2wry
reviewed
Dec 13, 2017
| // Make sure the runtime id is set. | ||
| if job.job.Spec.RuntimeId == "" { | ||
| if job.status.Phase != c.expectPhase { | ||
| t.Errorf("job.job.Status.Phase Want: %v Got:%v ", c.expectPhase, job.status.Phase) |
zjj2wry
reviewed
Dec 13, 2017
| if err := j.updateTPRStatus(); err != nil { | ||
| log.Warningf("Job %v; failed to update TPR status error: %v", j.job.Metadata.Name, err) | ||
| } | ||
| j.reconcile(config) |
There was a problem hiding this comment.
seems we will do this every 8 minute intervals, if i understand this code, maybe we only need do setup when my job is created and phase is none.
There was a problem hiding this comment.
The reconcile function is the code that periodically checks the job and figures out what needs to be done. The reconcile function takes advantage of state (e.g. phase) preserved in the CRD to avoid repeating unnecessary steps like setup.
|
this big enhance code, LGTM |
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.
This change is