Properly cleanup injected sidecars #5565

tomkennedy513 · 2022-09-27T15:23:48Z

Changes

Injected sidecars are only cleaned up if sidecars are defined in the task spec because of a check that exits early if the TaskRun does not include any sidecars. This breaks users that are running in environments with injected sidecars, but are not defining sidecars in their TaskRuns.

This change will remove the early exit condition when there are no sidecars defined on the TaskRun Spec. This will cause the podconvert.StopSidecars method to run every time, properly cleaning up sidecars the TaskRun doesn't know about.

/kind bug

Resolves #4731

Submitter Checklist

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Fixed istio-proxy container not stopped after running as taskrun #4731. This will result in a TaskRun failing if all sidecars can not be stopped.

linux-foundation-easycla · 2022-09-27T15:23:51Z

The committers listed above are authorized under a signed CLA.

✅ login: tomkennedy513 / name: Tom Kennedy (8f686f8)

tekton-robot · 2022-09-27T15:23:57Z

Hi @tomkennedy513. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

dibyom · 2022-09-27T20:53:24Z

/ok-to-test

tekton-robot · 2022-09-27T20:58:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/pod/entrypoint.go	89.5%	89.4%	-0.1
pkg/pod/status.go	90.9%	90.9%	-0.0
pkg/reconciler/taskrun/taskrun.go	81.3%	81.4%	0.2

vdemeester · 2022-09-28T15:06:42Z

/test check-pr-has-kind-label

tekton-robot · 2022-09-28T15:06:44Z

@vdemeester: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-build-tests
/test pull-tekton-pipeline-integration-tests
/test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test check-pr-has-kind-label

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.

dibyom · 2022-09-28T20:53:51Z

Side effects of this change are that non-user-defined sidecars will show in the TaskRun status.

@chuangw6 @wlynch does this affect chains at all?

tomkennedy513 · 2022-09-28T21:13:37Z

Side effects of this change are that non-user-defined sidecars will show in the TaskRun status.

@chuangw6 @wlynch does this affect chains at all?

If this does end up having a negative effect, an alternate approach could be relaxing the early exit optimizations (I believe these were added to minimize api calls to retrieve pods) for the stopSidecar method in the case where the running-in-environment-with-injected-sidecars feature flag is true so that we don't check to the task status to find running containers and call the api to get the pod. In the event that running-in-environment-with-injected-sidecars is false (this would have to be manually changed since it defaults to true), then we would proceed as normal

wlynch · 2022-09-28T23:01:59Z

@chuangw6 @wlynch does this affect chains at all?

Chains shouldn't need additional changes. 🎉

For Tekton based provenance we just spit out the status: https://github.com/tektoncd/chains/blob/bc296d1ee1491df7204211cf6d5bd8df9a89c157/pkg/chains/formats/tekton/tekton.go#L34-L37

In-toto is WIP - @chitrangpatel pointed out we aren't capturing sidecars and other fields in the predicate at the moment, but we'll look to populate from the Run status once we support it.

dibyom · 2022-11-10T22:55:38Z

Sorry for the delay on this @tomkennedy513 - This looks good to me but wanted to see if any of the other @tektoncd/core-maintainers had any opinions on this

XinruZhang

Thanks for your PR! Mostly look good~

XinruZhang · 2022-11-22T14:39:26Z

pkg/reconciler/taskrun/taskrun.go

-	// do not continue if the TaskSpec had no sidecars
-	if tr.Status.TaskSpec != nil && len(tr.Status.TaskSpec.Sidecars) == 0 {
-		return nil
-	}
-


Do we need to delete this considering the change in pkg/pod/status.go?

I don't think we need to delete it, but do to the changes in status.go, this check should be sufficient to exit early imo.

tomkennedy513 · 2022-11-29T21:59:20Z

/assign @abayer

tekton-robot · 2023-03-24T15:23:55Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

tekton-robot · 2023-03-24T15:46:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

tekton-robot · 2023-03-24T15:46:41Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

pkg/reconciler/taskrun/taskrun_test.go

tekton-robot · 2023-03-27T13:46:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick, XinruZhang

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:

~~OWNERS~~ [lbernick]

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

Injected sidecars are only cleaned up if sidecars are defined in the task spec because of a check that exits early if the TaskRun does not include any sidecars. This breaks users that are running in environments with injected sidecars, but are not defining sidecars in their TaskRuns. This change will remove the early exit condition when there are no sidecars defined on the TaskRun Spec. This will cause the podconvert.StopSidecars method to run every time, properly cleaning up sidecars the TaskRun doesn't know about. Resolves tektoncd#4731

tomkennedy513 · 2023-03-27T14:37:34Z

@lbernick I made those test changes. Thank you for the suggestions!

tekton-robot · 2023-03-27T14:41:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

tekton-robot · 2023-03-27T14:42:56Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/taskrun.go	84.8%	84.9%	0.1

afrittoli

Thanks for this fix. See my comment about the release notes.
/lgtm

afrittoli · 2023-03-28T09:26:30Z

pkg/reconciler/taskrun/taskrun.go

-	if !podconvert.IsSidecarStatusRunning(tr) {
-		return nil
-	}
-


The test in https://github.com/tektoncd/pipeline/pull/5565/files#diff-6e67e9c647bbe2a08807ff5ccbdd7dc9036df373e56b9774d3996f92ab7ceabaL289 will mark a TaskRun failed if we fail to stop one of the injected sidecars.

I don't run an environment with istio, so I'm not sure what the best behaviour would be; I think failing the TaskRun because the injected sidecar could not be stopped seems a bit harsh, but at least it would be aligned with the behaviour we have to sidecars defined in the TaskRun.

I think it would be worth adding a note to the release notes about this.
You can do that even if the PR is merged once you read this comment, since the release notes will be picked up later by the release automation.

fixes tektoncd#7640 In tektoncd#5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

fixes #7640 In #5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

fixes tektoncd#7640 In tektoncd#5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

fixes #7640 In #5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

fixes tektoncd#7640 In tektoncd#5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

tekton-robot requested review from imjasonh and jerop September 27, 2022 15:23

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 27, 2022

tomkennedy513 marked this pull request as ready for review September 27, 2022 15:40

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2022

tekton-robot requested a review from dlorenc September 27, 2022 15:40

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 27, 2022

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 Sep 28, 2022

This was referenced Oct 4, 2022

Run stopSidecar logic even if TaskSpec has no sidecars to support injected sidecars such as istio-proxy #5067

Closed

istio-proxy container not stopped after running as taskrun #4731

Closed

XinruZhang approved these changes Nov 22, 2022

View reviewed changes

tekton-robot assigned abayer Nov 29, 2022

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2022

tomkennedy513 requested review from afrittoli and lbernick and removed request for pritidesai, afrittoli and lbernick March 24, 2023 15:27

tomkennedy513 force-pushed the injected-sidecars branch from c2dd68a to a5e31a9 Compare March 24, 2023 15:39

lbernick self-assigned this Mar 24, 2023

lbernick approved these changes Mar 27, 2023

View reviewed changes

pkg/reconciler/taskrun/taskrun_test.go Outdated Show resolved Hide resolved

pkg/reconciler/taskrun/taskrun_test.go Outdated Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2023

tomkennedy513 force-pushed the injected-sidecars branch from a5e31a9 to fdf295c Compare March 27, 2023 14:36

afrittoli reviewed Mar 28, 2023

View reviewed changes

tekton-robot assigned afrittoli Mar 28, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2023

tekton-robot merged commit 545bdbb into tektoncd:main Mar 28, 2023

abayer mentioned this pull request Feb 6, 2024

Injected sidecars still can show up in TaskRun status when the TaskRun has explicit sidecars #7640

Closed

abayer mentioned this pull request Feb 6, 2024

Exclude stopped injected sidecars from TaskRun status #7642

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly cleanup injected sidecars #5565

Properly cleanup injected sidecars #5565

tomkennedy513 commented Sep 27, 2022 •

edited

Loading

linux-foundation-easycla bot commented Sep 27, 2022 •

edited

Loading

tekton-robot commented Sep 27, 2022

dibyom commented Sep 27, 2022

tekton-robot commented Sep 27, 2022

vdemeester commented Sep 28, 2022

tekton-robot commented Sep 28, 2022

dibyom commented Sep 28, 2022

tomkennedy513 commented Sep 28, 2022

wlynch commented Sep 28, 2022

dibyom commented Nov 10, 2022

XinruZhang left a comment

XinruZhang Nov 22, 2022

tomkennedy513 Nov 29, 2022 •

edited

Loading

tomkennedy513 commented Nov 29, 2022

tekton-robot commented Mar 24, 2023

tekton-robot commented Mar 24, 2023

tekton-robot commented Mar 24, 2023

tekton-robot commented Mar 27, 2023

tomkennedy513 commented Mar 27, 2023

tekton-robot commented Mar 27, 2023

tekton-robot commented Mar 27, 2023

afrittoli left a comment

afrittoli Mar 28, 2023

Properly cleanup injected sidecars #5565

Properly cleanup injected sidecars #5565

Conversation

tomkennedy513 commented Sep 27, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

linux-foundation-easycla bot commented Sep 27, 2022 • edited Loading

tekton-robot commented Sep 27, 2022

dibyom commented Sep 27, 2022

tekton-robot commented Sep 27, 2022

vdemeester commented Sep 28, 2022

tekton-robot commented Sep 28, 2022

dibyom commented Sep 28, 2022

tomkennedy513 commented Sep 28, 2022

wlynch commented Sep 28, 2022

dibyom commented Nov 10, 2022

XinruZhang left a comment

Choose a reason for hiding this comment

XinruZhang Nov 22, 2022

Choose a reason for hiding this comment

tomkennedy513 Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

tomkennedy513 commented Nov 29, 2022

tekton-robot commented Mar 24, 2023

tekton-robot commented Mar 24, 2023

tekton-robot commented Mar 24, 2023

tekton-robot commented Mar 27, 2023

tomkennedy513 commented Mar 27, 2023

tekton-robot commented Mar 27, 2023

tekton-robot commented Mar 27, 2023

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Mar 28, 2023

Choose a reason for hiding this comment

tomkennedy513 commented Sep 27, 2022 •

edited

Loading

linux-foundation-easycla bot commented Sep 27, 2022 •

edited

Loading

tomkennedy513 Nov 29, 2022 •

edited

Loading