Conversation
tekton-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
tekton-robot
added
the
size/M
There was a problem hiding this comment.
Pull request overview
This PR fixes a TaskRun reconciler edge case when enable-kubernetes-sidecar is enabled: a transient failure from Discovery().ServerVersion() was previously memoized forever (via sync.OnceValues), delaying done-path processing (including sidecar cleanup) until controller restart. The new approach caches the native-sidecar decision only after a successful discovery call, so transient errors are retried on later reconciles.
Changes:
- Replace
sync.OnceValues-based memoization with a mutex-guarded, success-onlynativeSidecarCache. - Ensure discovery errors are returned for the current reconcile but do not populate the cache.
- Add a regression test intended to verify discovery errors are not cached across reconciles.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/reconciler/taskrun/taskrun.go | Replaces error-memoizing discovery memoization with a success-only cache for native sidecar detection. |
| pkg/reconciler/taskrun/taskrun_test.go | Adds a regression test for “do not cache discovery errors” behavior (currently needs adjustment to avoid version-dependent brittleness). |
Comments suppressed due to low confidence (1)
pkg/reconciler/taskrun/taskrun_test.go:8558
- This test asserts that useTektonSidecarMode() returns true after the transient error, but that depends on what ServerVersion() returns (which can change as client-go/Kubernetes version defaults change). The regression being tested is that discovery errors are not memoized; the boolean result isn't important here.
Consider asserting that ServerVersion() is called twice (error then success) and that a subsequent call does not hit discovery again (cached after success).
useTektonSidecar, err := r.useTektonSidecarMode(ctx, logger)
if err != nil {
t.Fatalf("expected useTektonSidecarMode to retry discovery after a transient error and succeed, got err: %v", err)
}
if !useTektonSidecar {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| discoveryFailed := true | ||
| kubeClient.PrependReactor("get", "version", func(action ktesting.Action) (bool, runtime.Object, error) { | ||
| if discoveryFailed { | ||
| return true, nil, errors.New("transient discovery error") | ||
| } | ||
| return false, nil, nil | ||
| }) |
There was a problem hiding this comment.
Good catch, thanks. I've updated the test so it no longer depends on the fake discovery client's default (client-go build) version: it now sets FakedServerVersion explicitly to {Major: "1", Minor: "28"}, and the reactor always returns handled=true and increments a discoveryCalls counter. The assertions now check that counter directly (1 call after the failed reconcile, 2 after the retry succeeds, still 2 after a third reconcile to confirm the successful result is cached), rather than asserting on the boolean result of useTektonSidecarMode. Ran go test ./pkg/reconciler/taskrun/... and it passes.
tekton-robot
added
size/L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pkg/reconciler/taskrun/taskrun_test.go:8544
- The success-path comment in the reactor is misleading: the reactor doesn't return a version object; FakeDiscovery.ServerVersion() ignores the reactor's object and returns the
FakedServerVersionconfigured below. Clarifying this avoids future confusion when modifying the test.
// Explicitly return a version predating native sidecar support (1.29) so the
// assertions below don't depend on the fake client's default build version.
return true, nil, nil
})
|
Thanks for the follow-up review. The comment on the reactor's success path was indeed misleading — it implied the reactor's return value supplied version "1.29", but |
| // Signal a successful discovery call (handled=true, no error). The version | ||
| // FakeDiscovery.ServerVersion() actually returns comes from FakedServerVersion, | ||
| // set below, not from this reactor's return value. |
|
Thanks for catching that — fixed the grammar in the comment (added "that" so it reads "The version that FakeDiscovery.ServerVersion() actually returns comes from FakedServerVersion..."). Ran |
|
/easycla |
|
/retest |
|
@pujitha24 can you follow the pull request template please ? |
|
/retest |
bcd9ea9 to
83086ec
Compare
83086ec to
680145e
Compare
tekton-robot
added
the
needs-rebase
Motivation: When EnableKubernetesSidecar is set, the TaskRun reconciler's done path (tr.IsDone()) calls useTektonSidecarMode to decide whether to run the Tekton nop-sidecar teardown or rely on native Kubernetes sidecars. That check memoized client.Discovery().ServerVersion() with sync.OnceValues, which caches both successful values and errors forever. If the very first ServerVersion() call failed transiently (e.g. the API server was briefly unavailable), every later done-path reconcile for every TaskRun in that process kept returning the same stale error, since the cache lives on the shared Reconciler struct, not per-TaskRun. To be precise about impact: this does not crash or panic the controller. useTektonSidecarMode returning an error just makes the done-path handler return err instead of calling finishReconcileUpdateEmitEvents; knative's reconciler logs it and requeues with backoff like any other reconcile error. The observable effect is that final done-path processing (including sidecar cleanup via stopSidecars) for completed TaskRuns keeps getting delayed/ retried until the controller restarts or a leader change resets the cache, rather than the discovery call being retried on a later, healthy reconcile. Approach: Replace the sync.Once + sync.OnceValues pair with a small mutex-guarded nativeSidecarCache that only marks the result cached after client.Discovery().ServerVersion() succeeds. On error, the error is returned for the current reconcile but the cache is left unpopulated so the next reconcile retries discovery. Once a call succeeds, the result is cached for the lifetime of the Reconciler, same as before (ServerVersion is still queried at most once on the happy path). Validation: go build ./... go test ./pkg/reconciler/taskrun/... -count=1 Both pass, including the new regression test: go test ./pkg/reconciler/taskrun -run TestUseTektonSidecarModeDoesNotCacheDiscoveryErrors -count=1 -v which fails against the old sync.OnceValues-based code (verified locally) and passes against this fix. /kind bug ```release-note Fixed a bug where, with `enable-kubernetes-sidecar` set, a transient error from the Kubernetes API server during native-sidecar discovery could be cached forever, repeatedly delaying final done-path processing (including sidecar cleanup) for completed TaskRuns instead of being retried once the API server recovered. ``` Fixes tektoncd#10101 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: Claude Sonnet 5 (via Claude Code)
…ersion Assert on a discovery-call counter and an explicit FakedServerVersion instead of the fake client's default build version, and always return handled=true from the reactor, per review feedback. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: Claude Sonnet 5 (via Claude Code)
The reactor's return value doesn't influence the version reported by FakeDiscovery.ServerVersion(); that comes from FakedServerVersion, set right after. Clarify the comment to avoid confusion (per Copilot review). Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: Claude Sonnet 5 (via Claude Code)
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: Claude Sonnet 5 (via Claude Code)
680145e to
b65e9cc
Compare
|
Sorry for the slow follow-up on the template. The description now uses the repo's PR template (Changes, checklist, release-note block), and I left the Docs and "action required" boxes unchecked since neither applies. I also rebased onto current main to clear the merge conflict; the only conflict was in the |
tekton-robot
removed
the
needs-rebase
Changes
With
enable-kubernetes-sidecarset, the TaskRun reconciler's done path memoizedclient.Discovery().ServerVersion()withsync.OnceValues, which cached both successes and errors forever. A transient discovery failure on the first call meant every later done-path reconcile kept returning that same stale error until the controller restarted, delaying final done-path processing (including sidecar cleanup) for completed TaskRuns.This replaces that memoization with a mutex-guarded cache that's only populated after a successful discovery call, so a transient error is retried on the next reconcile instead of being cached forever. On the happy path the behavior is unchanged — discovery is still queried at most once per process.
Fixes #10101
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes