plank: enforce pending timeout when build cluster is unreachable - #817
plank: enforce pending timeout when build cluster is unreachable#817Prucek wants to merge 1 commit into
Conversation
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Prucek The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
cncf-cla: yes
| maxPodPending := r.config().Plank.PodPendingTimeout.Duration | ||
| if pj.Spec.DecorationConfig != nil && pj.Spec.DecorationConfig.PodPendingTimeout != nil { | ||
| maxPodPending = pj.Spec.DecorationConfig.PodPendingTimeout.Duration | ||
| } |
There was a problem hiding this comment.
this is duplicated from original lines 551-553. I think we could use a helper here incase the logic needs to merge something else in the future.
| pj.SetComplete() | ||
| pj.Status.State = prowv1.ErrorState | ||
| pj.Status.Description = fmt.Sprintf("Pod pending timeout: could not get pod from build cluster: %v.", err) | ||
| r.log.WithFields(pjutil.ProwJobFields(pj)).WithError(err).Info("Marked job as errored: build cluster unreachable and pending timeout exceeded.") | ||
| pj.Status.URL, err = pjutil.JobURL(r.config().Plank, *pj, r.log) | ||
| if err != nil { | ||
| r.log.WithFields(pjutil.ProwJobFields(pj)).WithError(err).Warn("failed to get jobURL") | ||
| } | ||
| if patchErr := r.pjClient.Patch(ctx, pj.DeepCopy(), ctrlruntimeclient.MergeFrom(prevPJ)); patchErr != nil { | ||
| return nil, fmt.Errorf("patch prowjob: %w", patchErr) | ||
| } |
There was a problem hiding this comment.
This also seems like something that must be duplicated. Can we get a helper to error a pj that is used here?
6e36518 to
3ac48b9
Compare
kubernetes-prow
Bot
added
size/L
|
Isn't this too strict? Single failed Still I feel something like a grace period is in order. |
petr-muller
left a comment
petr-muller
left a comment
There was a problem hiding this comment.
I dug into the Get() failure signal and have several more thoughts - I don't consider them blocking but I'd like them to be considered.
- I wonder if instead of handling the error in-situ it should be re-translated to a
TerminalErrorthat's handled almost identically the call chain? We already have the "stop reconciling, set to abort" behavior, just at different place. - We don't have the "wait for the change to propagate into the informer cache behavior" in the
TerminalErrorhandler though -> that kinda defeats (1) but I wonder if theTerminalErrorhandler should do the wait too (and then we could do (1))
And lastly, I really wonder if the Pod Get() really actually errors if a build cluster dies - if I read the code correctly it is an informer cache-backed read, which is local and not affected by build farm outages - so the Get IMO never errors, you just stop getting new state and you stop getting reconcile events about the Pod.
| return maxPodPending | ||
| } | ||
|
|
||
| func (r *reconciler) updateProwJobStatus(ctx context.Context, pj *prowv1.ProwJob, prevPJ *prowv1.ProwJob) error { |
There was a problem hiding this comment.
This helper could use a godoc that describes how exactly is the the prowjob status updated, because it is absolutely unclear without reading the code. It should probably retain the comment that was at its original location because the wait could be surprising and a reader can just guess why it is there.
When a build cluster becomes unreachable, syncPendingJob enters a retry loop that never terminates. The pod lookup (informer cache read) returns not-found, startPod attempts client.Create which fails with a network error, and because isRequestError only matches 4xx API responses, the error is returned for requeue — indefinitely. Check the ProwJob's pendingTime against pod_pending_timeout when startPod fails with a non-request error. If the timeout is exceeded, mark the job as errored instead of retrying forever. Also extract maxPodPendingTimeout helper to deduplicate the timeout resolution logic between the pod-missing and pod-pending code paths.
3ac48b9 to
e9b0c93
Compare
kubernetes-prow
Bot
added
size/M
|
/retest |
3 participants
Summary
When a build cluster becomes unreachable,
syncPendingJobenters a retry loop that never terminates:r.pod()does an informer cache lookup — pod not found (cache is empty after plank restart while the cluster was down)!podExistsbranch → callsstartPod()→client.Create()(direct API write)isRequestError()returnsfalse— it only matches 4xx API responses, not raw network errorsThe
pod_pending_timeoutis never evaluated because the timeout check only runs after the pod is found in the informer cache and observed in a PodPending phase. When the pod is missing from the cache entirely, neither the scheduling nor the pending timeout code paths are reached.Fix: In the
!podExistspath, whenstartPod()fails with a non-request error, check the ProwJob'spendingTimeagainst the configuredpod_pending_timeout. If the timeout has been exceeded, mark the job as errored instead of retrying forever. If the timeout hasn't elapsed yet, continue retrying (the cluster may recover).Also extracts
maxPodPendingTimeouthelper to deduplicate the timeout resolution logic between the pod-missing and pod-pending code paths (addresses review feedback from the previous revision).Example: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-build-farm-canary-build06/2080695972983214080 — this canary job was stuck in pending state since July 24 because build06 was temporarily unreachable. Despite
pod_pending_timeout: 30mbeing configured globally, it was never enforced. The prowjob.json confirms:state: "pending",pod_nameset (pod was originally created), nofinished.jsonorbuild-log.txt.Why the informer cache read doesn't help
As confirmed in review discussion:
buildClient.Get()reads from the informer cache — a purely in-memory lookup that never fails with network errors. When a build cluster goes down:startPod()network error loop (this fix)Changes
pkg/plank/reconciler.go: Add pending timeout check in the!podExists+startPodfailure path for non-request errors. ExtractmaxPodPendingTimeouthelper.pkg/plank/controller_test.go: AddExpectErrorfield to test struct. Add two test cases: "build cluster unreachable, pending timeout exceeded" (expects ErrorState) and "build cluster unreachable, pending timeout not yet exceeded" (expects retry error).