Never clean backoff in job controller #63650
Merged
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
k8s-ci-robot
added
release-note-none
This was referenced May 10, 2018
|
LGTM 👍 |
janetkuo
reviewed
May 14, 2018
janetkuo
added
priority/critical-urgent
|
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process Pull Request Labels
|
|
Haven't heard back from you so I opened #63990 with your commit and a test for it. |
|
/milestone v1.11 |
k8s-ci-robot
added
the
do-not-merge/hold
Closed
|
/retest |
As a proof, I'm just reaching 300 iteration of the test without a flake. So I guess the flakiness is a low probability, as well as hard to fix :/ |
janetkuo
reviewed
Jun 5, 2018
pkg/controller/job/job_controller.go
Outdated
| @@ -393,7 +393,9 @@ func (jm *JobController) processNextWorkItem() bool { | |||
| } | |||
|
|
|||
| utilruntime.HandleError(fmt.Errorf("Error syncing job: %v", err)) | |||
| jm.queue.AddRateLimited(key) | |||
| if !errors.IsConflict(err) { | |||
| jm.queue.AddRateLimited(key) | |||
There was a problem hiding this comment.
If status update failed, we still need to requeue this job; otherwise, the job status may never be updated.
There was a problem hiding this comment.
As a workaround, we can probably retry on update conflict. But in the long run we should not use # of requeues to compare against backoff limit.
There was a problem hiding this comment.
Yeah, I totally agree that the re-queues are not good for that. They're causing us more harm than good :(
There was a problem hiding this comment.
I've created #64787 to track this.
pkg/controller/job/job_controller.go
Outdated
| // new failures happen when status does not reflect the failures and active | ||
| // is different than parallelism, otherwise the previous controller loop | ||
| // failed updating status so even if we pick up failure it is not a new one | ||
| exceedsBackoffLimit := jobHaveNewFailure && (active != *job.Spec.Parallelism) && |
There was a problem hiding this comment.
Would you explain why active != *job.Spec.Parallelism is needed?
There was a problem hiding this comment.
This is for the case when a controller failed an update, but went through the whole process, which means that the number of active is exactly what it should be. There's no other way right now to figure that the previous loop failed.
pkg/controller/job/job_controller.go
Outdated
| @@ -494,8 +496,12 @@ func (jm *JobController) syncJob(key string) (bool, error) { | |||
| var failureReason string | |||
| var failureMessage string | |||
|
|
|||
| jobHaveNewFailure := failed > job.Status.Failed | |||
| exceedsBackoffLimit := jobHaveNewFailure && (int32(previousRetry)+1 > *job.Spec.BackoffLimit) | |||
| jobHaveNewFailure := (failed > job.Status.Failed) | |||
There was a problem hiding this comment.
This could happen when Job Status failed to be updated?
There was a problem hiding this comment.
This did not change, it is as it was before I was trying different scenarios, so that change is nothing new. During tests I've just added parenthesis here.
|
@janetkuo updated, ptal once again |
soltysh
force-pushed
the
issue62382
branch
3 times, most recently
from
a430523
to
abd9e0d
Compare
janetkuo
reviewed
Jun 5, 2018
pkg/controller/job/job_controller.go
Outdated
| st := job.Status | ||
|
|
||
| var err error | ||
| for i, job := 0, job; i < statusUpdateRetries; i, job = i+1, refresh(jobClient, job) { |
pkg/controller/job/job_controller.go
Outdated
| if err == nil { | ||
| return newJob | ||
| } else { | ||
| return job |
There was a problem hiding this comment.
nit: we should probably give up updating status when GET returns an error
janetkuo
approved these changes
Jun 5, 2018
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: janetkuo, soltysh 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 |
|
/retest |
|
@soltysh: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/test pull-kubernetes-verify |
|
Automatic merge from submit-queue (batch tested with PRs 64009, 64780, 64354, 64727, 63650). If you want to cherry-pick this change to another branch, please follow the instructions here. |
k8s-ci-robot
added
release-note
k8s-github-robot
pushed a commit
that referenced
this pull request
Jun 11, 2018
#63650-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #58972: Fix job's backoff limit for restart policy OnFailure #63650: Never clean backoff in job controller Cherry pick of #58972 #63650 on release-1.10. #58972: Fix job's backoff limit for restart policy OnFailure #63650: Never clean backoff in job controller Fixes #62382. **Release Note:** ```release-note Fix regression in `v1.JobSpec.backoffLimit` that caused failed Jobs to be restarted indefinitely. ```
|
@soltysh I run the test locally, failed 4 times related log: fail scenario 2： related log: |
|
For posterity, this was backported in 1.10.5 -> #64813 |
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.
What this PR does / why we need it:
In #60985 I've added a mechanism which allows immediate job status update, unfortunately that broke the backoff logic seriously. I'm sorry for that. I've changed the
immediatemechanism so that it NEVER cleans the backoff, but for the cases when we want fast status update it uses a zero backoff.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #62382
Special notes for your reviewer:
/assign @janetkuo
Release note: