Skip to content

Conversation

@prad9192
Copy link
Contributor

@prad9192 prad9192 commented Nov 7, 2025

What

Changed StopSidecars() to use Patch() instead of Update() when stopping sidecar containers by replacing their images with the nop image.

Why

The original implementation used Update(), which requires an exact resourceVersion match. This causes 409 conflicts when the kubelet updates the pod status between our GET and UPDATE calls (which happens frequently as containers terminate).

This causes the below errors on the pod.

error stopping sidecars of Pod: Operation cannot be fulfilled on pods "...": 
the object has been modified; please apply your changes to the latest version

And the task runs fail with TaskRunResolutionFailed

image

Even though the task succeeded and sidecars eventually stop, the TaskRun gets marked as failed due to this race condition.
The fix uses JSON Patch (same pattern as UpdateReady() and CancelPod() in this file), which only patches the specific container image fields

Release Notes

Does this PR introduce a user-facing change?

Fixed race condition causing TaskRuns to fail with 409 conflict error when stopping sidecars. 
StopSidecars now uses Patch instead of Update to avoid conflicts with concurrent kubelet pod status updates.

@tekton-robot tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 7, 2025
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 7, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: prad9192 / name: Pradeep Ashwathanarayan (ea7d2ca)

@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 7, 2025
@prad9192 prad9192 force-pushed the tekton-stop-sidecar-patch-instead-of-update branch from 934d222 to 434ad9a Compare November 7, 2025 01:22
@prad9192
Copy link
Contributor Author

prad9192 commented Nov 9, 2025

/retest

@waveywaves waveywaves added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 13, 2025
@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 18, 2025
@vdemeester
Copy link
Member

@prad9192 you will need to rebase against main branch. The e2e failure are fixed in the latest main commits.

@prad9192
Copy link
Contributor Author

/label kind/bug

@tekton-robot
Copy link
Collaborator

@prad9192: The label(s) /label kind/bug cannot be applied. These labels are supported: ``

Details

In response to this:

/label kind/bug

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.

@prad9192
Copy link
Contributor Author

/retest

@AlanGreene
Copy link
Member

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 18, 2025
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix, it's much appreciated!
Could you add some testing for the new functionality? It should be possible to simulate the situation you described in the PR description of the pod being updated.

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 20, 2025
@prad9192
Copy link
Contributor Author

Thanks for this fix, it's much appreciated! Could you add some testing for the new functionality? It should be possible to simulate the situation you described in the PR description of the pod being updated.

Thank you for the feedback, as requested the tests are added, kindly review.
cc: @afrittoli

@prad9192 prad9192 force-pushed the tekton-stop-sidecar-patch-instead-of-update branch from 0aca735 to 0877c4c Compare November 20, 2025 05:47
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the update, lgtm!

One last ask before it's approved - we don't use merge commits in Tekton, could you please squash your PR to a single commit? Thank you!

@prad9192 prad9192 force-pushed the tekton-stop-sidecar-patch-instead-of-update branch from d0cc83b to cdf36c5 Compare November 20, 2025 19:11
@prad9192
Copy link
Contributor Author

Thanks a lot for the update, lgtm!

One last ask before it's approved - we don't use merge commits in Tekton, could you please squash your PR to a single commit? Thank you!

Done, could you please review, when you get a chance?

@afrittoli
Copy link
Member

/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2025
@prad9192 prad9192 force-pushed the tekton-stop-sidecar-patch-instead-of-update branch from 9b63960 to ea7d2ca Compare November 21, 2025 06:40
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2025
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [afrittoli,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vdemeester
Copy link
Member

/retest

1 similar comment
@vdemeester
Copy link
Member

/retest

@tekton-robot tekton-robot merged commit 68bb12e into tektoncd:main Nov 21, 2025
59 of 65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants