Fix: do not fail TaskRun for concurrent modification errors #7467
Conversation
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
1759704 to
433e847
Compare
|
So far I have managed to add a bare minimum unit test because it doesn't seem to be easy(or even possible) to mock an injection of the k8s concurrent modification error. If there are any suggestions for adding more testing around, I'm all ears 🙏 |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
This commit fixes the behaviour that a concurrent modification error when stopping sidecar will fail the TaskRun, which could cause successful Tasks to fail even though it could succeed after retrying. /kind bug
433e847 to
5f07c37
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| @@ -1014,6 +1020,28 @@ func isResourceQuotaConflictError(err error) bool { | |||
| return k8ErrStatus.Details != nil && k8ErrStatus.Details.Kind == "resourcequotas" | |||
| } | |||
|
|
|||
| const ( | |||
| // TODO(#7466) Currently this appears as a local constant due to upstream dependencies bump blocker. | |||
There was a problem hiding this comment.
～is 7466 a pr?～
Oh I see, #7466 this one
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester, Yongxuanzhang 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 |
| // isConcurrentModificationError determines whether it is a concurrent | ||
| // modification error depending on its error type and error message. | ||
| func isConcurrentModificationError(err error) bool { | ||
| if !k8serrors.IsConflict(err) { |
There was a problem hiding this comment.
Is there a reason why just checking k8serrors.IsConflict() isn't sufficient and we have to check the exact optimisticLockErrorMsg?
There was a problem hiding this comment.
I think there are actual other cases where the NewConflict() errors are thrown i.e. there is an invalid storage error or uid mismatch where there could be a case of an object was missing unexpectedly.
It might be safe to only retry on concurrent modifications for this case IIUC.
|
/test pull-tekton-pipeline-alpha-integration-tests |
|
/cherry-pick release-v0.53.x |
|
@JeromeJu: new pull request created: #7479 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changes
This commit fixes the behaviour that a concurrent modification error when stopping sidecar will fail the TaskRun, which could cause successful Tasks to fail even though it could succeed after retrying.
/kind bug
fixes: #7452
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