-
Notifications
You must be signed in to change notification settings - Fork 93
fix(validators): retry transient reads while polling gang test pods #2407
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
Changes from all commits
21ef0e7
da26a02
bb711e8
cba192d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import ( | |
| "crypto/rand" | ||
| "encoding/hex" | ||
| "fmt" | ||
| "log/slog" | ||
| "strings" | ||
| "time" | ||
|
|
||
|
|
@@ -331,6 +332,10 @@ func waitForGangTestPods(ctx context.Context, clientset kubernetes.Interface, ru | |
| waitCtx, cancel := context.WithTimeout(ctx, defaults.GangTestPodTimeout) | ||
| defer cancel() | ||
|
|
||
| // Per-pod, not a single latch: a pod whose read recovers must clear its | ||
| // entry, otherwise one early blip would mislabel a genuine "never | ||
| // completed" timeout as "unreadable" long after reads recovered. | ||
| readErrs := make(map[string]error, gangMinMembers) | ||
| err := wait.PollUntilContextCancel(waitCtx, defaults.PodPollInterval, true, | ||
| func(ctx context.Context) (bool, error) { | ||
| allDone := true | ||
|
|
@@ -341,10 +346,30 @@ func waitForGangTestPods(ctx context.Context, clientset kubernetes.Interface, ru | |
| pod, err := clientset.CoreV1().Pods(run.namespace).Get( | ||
| ctx, run.pods[i], metav1.GetOptions{}) | ||
| if err != nil { | ||
| return false, errors.Wrap(errors.ErrCodeInternal, | ||
| fmt.Sprintf("failed to get gang test pod %s", run.pods[i]), err) | ||
| // A read that could not land is not a verdict. Returning a | ||
| // non-nil error here aborts the whole poll, so one throttled | ||
| // or timed-out call would fail a healthy cluster even though | ||
| // the next interval would have succeeded — the same defect | ||
| // #1513 fixed one step earlier in this function. Let the | ||
| // enclosing GangTestPodTimeout decide instead. | ||
| if isK8sTimeoutErr(err) { | ||
| // The wait context ending during this Get is the poll's | ||
| // terminal signal, not evidence of sustained read failures. | ||
| if readFailedBecauseContextEnded(ctx, err) { | ||
| allDone = false | ||
| continue | ||
| } | ||
| readErrs[run.pods[i]] = err | ||
| slog.Debug("transient read while polling gang test pod; retrying", | ||
| "pod", run.pods[i], "error", err) | ||
| allDone = false | ||
| continue | ||
| } | ||
| return false, classifyK8sReadError(err, | ||
| fmt.Sprintf("gang test pod %s", run.pods[i])) | ||
| } | ||
| switch pod.Status.Phase { //nolint:exhaustive // only terminal states matter | ||
| delete(readErrs, run.pods[i]) // this read landed | ||
| switch pod.Status.Phase { //nolint:exhaustive // only terminal states matter | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| case corev1.PodSucceeded, corev1.PodFailed: | ||
| result[i] = pod | ||
| default: | ||
|
|
@@ -355,10 +380,27 @@ func waitForGangTestPods(ctx context.Context, clientset kubernetes.Interface, ru | |
| }, | ||
| ) | ||
| if err != nil { | ||
| if ctx.Err() != nil || waitCtx.Err() != nil { | ||
| // Caller cancellation is an external abort, not the gang timing out. | ||
| if ctx.Err() != nil { | ||
| return result, errors.Wrap(errors.ErrCodeTimeout, "waiting for gang test pods canceled", ctx.Err()) | ||
| } | ||
| if waitCtx.Err() != nil { | ||
| // Preserve the last transient read error: a sustained throttle | ||
| // otherwise looks identical to pods that never completed. | ||
| // Only if a still-pending pod's most recent read failed. | ||
| for i := range gangMinMembers { | ||
| if result[i] != nil { | ||
| continue | ||
| } | ||
| if readErr, ok := readErrs[run.pods[i]]; ok { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor — A deadline landing mid-Get mislabels "never completed" as "unreadable (reads kept failing)" If waitCtx (GangTestPodTimeout=5m) expires DURING an in-flight Get on the final poll cycle, that Get returns context.DeadlineExceeded, isK8sTimeoutErr is true, and the pod's entry is written into readErrs on the last iteration with no subsequent landed read to delete it. This block then reports "pod X unreadable (reads kept failing)" even though reads were succeeding and the pod merely never reached a terminal phase. The same shape exists at helpers.go:235 (lastReadErr). Both branches wrap ErrCodeTimeout, so the verdict is identical — message-only — but it points an operator at a throttling ghost instead of "pod never completed." Sharpest sub-point: the PR's own TestWaitFor*RecoveredReadNotReportedAsUnreadable tests assert this invariant, but fake-client Gets are instant, so they only exercise expiry-during-sleep; the real-cluster expiry-during-Get path is unproven. Blast radius: Diagnostic message only; no wrong pass/fail. Window ~ Get-RTT/(Get-RTT+500ms) on the final cycle. Fix: In the expiry scan, don't attribute "unreadable" when the recorded error is the wait's own cancellation (stderrors.Is(readErr, context.DeadlineExceeded) && waitCtx.Err() != nil), or accept-and-document it as a known cosmetic edge.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Addressed in 5d13295. Both polling sites now ignore a final read error only when it is owned by the ended poll context; a prior genuine transient error remains available for sustained-throttle diagnostics. |
||
| return result, errors.Wrap(errors.ErrCodeTimeout, | ||
| fmt.Sprintf("gang test pod %s unreadable (reads kept failing)", run.pods[i]), | ||
| readErr) | ||
| } | ||
| } | ||
| return result, errors.Wrap(errors.ErrCodeTimeout, "gang test pods did not complete in time", err) | ||
| } | ||
| return result, errors.Wrap(errors.ErrCodeInternal, "gang test pod polling failed", err) | ||
| return result, errors.PropagateOrWrap(err, errors.ErrCodeInternal, "gang test pod polling failed") | ||
| } | ||
|
|
||
| return result, nil | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.