Retry TaskRef resolution on etcd errors #4392
Conversation
tekton-robot
added
release-note
tekton-robot
added
the
size/L
|
The following is the coverage report on the affected files.
|
|
Something I'm not sure about: Does the taskrun or pipelinerun automatically get requeued for processing again if the returned error isn't permanent? If not, is there a risk that the tr or pr ends up just "hanging around" waiting for further updates to trigger re-reconcile? |
There was a problem hiding this comment.
Thank for this change @lbernick.
I like the approach to add a new function - so we can add to it when we find more transient errors to be handled.
I agree with @sbwsg that is would be nice to name it something that starts with Is.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli 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 |
tekton-robot
added
the
approved
|
The following is the coverage report on the affected files.
|
|
/lgtm |
Prior to this change, querying etcd for a Task referenced by a TaskRun could fail due to an error "etcdserver: leader changed", as reported in tektoncd#4116. I believe this error is transient. This error doesn't appear to be wrapped in any error type exported by the Kubernetes Go client or Knative, so I've reported this upstream to the Go client in #kubernetes/kubernetes/106491. To address the user issue until the upstream issue can be resolved, this commit uses string comparison to detect and retry the error. A similar strategy is used in Knative for retrying this error during their integration tests (see https://github.com/knative/pkg/blob/517ef0292b5362ec316d227a866821bc31ec99a8/reconciler/retry.go#L50-L66). We don't currently have a strategy for reproducing this error, so it's not known whether this commit will solve the problem. We will have to see if this error continues to occur after this commit is released. Co-authored-by: Dibyo Mukherjee dibyajyoti@google.com Co-authored-by: Scott Seaward sbws@google.com
|
The following is the coverage report on the affected files.
|
My understanding is that the taskrun/pipelinerun will be requeued if the error returned is not permanent (based on the sentence "If event is an error ... the error will be returned from Reconciler and key will requeue back into the reconciler key queue" in the knative generated reconciler docs and also this commit). |
|
Awesome, thanks a lot! /lgtm |
Changes
Prior to this change, querying etcd for a Task referenced by a TaskRun
could fail due to an error "etcdserver: leader changed", as reported in #4116.
I believe this error is transient.
This error doesn't appear to be wrapped in any error type exported by
the Kubernetes Go client or Knative, so I've reported this upstream to
the Go client in #kubernetes/kubernetes/106491.
To address the user issue until the upstream issue can be resolved,
this commit uses string comparison to detect and retry the error.
A similar strategy is used in Knative for retrying this error
during their integration tests (see
https://github.com/knative/pkg/blob/517ef0292b5362ec316d227a866821bc31ec99a8/reconciler/retry.go#L50-L66).
We don't currently have a strategy for reproducing this error, so it's
not known whether this commit will solve the problem.
We will have to see if this error continues to occur after this commit is released.
Co-authored-by: Dibyo Mukherjee dibyajyoti@google.com @dibyom
Co-authored-by: Scott Seaward sbws@google.com @sbwsg
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes