-
Notifications
You must be signed in to change notification settings - Fork 994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix error resync logic #385
Conversation
@@ -265,10 +265,6 @@ func (cc *Controller) syncJob(jobInfo *apis.JobInfo, updateStatus state.UpdateSt | |||
pod.Name, job.Name, err) | |||
creationErrs = append(creationErrs, fmt.Errorf("failed to create pod %s, err: %#v", pod.Name, err)) | |||
} else { | |||
if err != nil && apierrors.IsAlreadyExists(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need provide more detail why we delete this resync logic here rather than fix the "can not find pod <namespace/pod> in cache" issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a race between the core job controller and the resync controller.
And same race between JobInfo.AddPod
and controller.syncJob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a race between the core job controller and the resync controller.
Please share your analysis of this issue firstly before PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add the analysis to Pr description
Do you mean this will always happen no matter we delete pod or not
This change can help fix this issue, but I am wondering if the resync logic is used to cover the case intentionaly |
I think so. For eventual consistent model, resync is a regular way to catch up. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu, TommyLike 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 |
fixes: #384
There is time latency between
Controller.syncJob
creating a pod and the k8s informer get the notification. So there are cases job controller callingController.syncJob
creating same pod twice,