tektoncd / pipeline

A cloud-native Pipeline resource.
https://tekton.dev
Apache License 2.0
8.49k stars 1.78k forks source link

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

Closed rkimmel-hv closed 1 year ago

rkimmel-hv commented 2 years ago

Right now I have tekton-pipelines installed through the cncf tekton-pipelines chart here https://github.com/cdfoundation/tekton-helm-chart using v0.23.0 and triggers v0.13.0. Currently when my task runs spin up pods the istio-proxy image is replaced by the nop image and everything exits successfully. I am currently trying to change my pipeline version to v0.34.0 using the install process listed in this repo and triggers to v0.19.1. Now when I run a taskrun the istio-proxy container is no longer being replaced by the nop image and the pod hangs around for 45 mins or so before finally exiting. Can I configure something to get the previous behavior or is there some way to exit the istio-proxy container after my task run code completes? I'd prefer NOT to send the /quitquitquit message to istio-proxy or specify the istio-proxy as a sidecar for every task if I don't need to.

Comparing versions of taskrun.go from v0.23.0 to current v0.34.0 there seems to be a decent amount of logic in the stopSidecars method. In the old version any non-step containers were considered as sidecars but in the newer version it seems the sidecars must be listed on the taskrun to be considered. Specifically this change added in v0.30.0 with the following commit https://github.com/tektoncd/pipeline/commit/2a70aedf09b0279a7e77a27e317ccb725ce94a9a

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

Is this change really worth breaking anyone depending on automatically injected sidecars? Is there a more convenient way to keep the same behavior as the older version?

dibyom commented 2 years ago

Hmm...this sounds like a bug /cc @tektoncd/core-maintainers

ChrisJBurns commented 2 years ago

I'm also receiving this!

satata-clgx commented 2 years ago

We are seeing the same issue.

pritidesai commented 2 years ago

/cc @SaschaSchwarze0

SaschaSchwarze0 commented 2 years ago

@rkimmel-hv @ChrisJBurns @satata-clgx I do not have an env with Istio at hand. Can somebody verify the change of above pull request?

tekton-robot commented 2 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale with a justification. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

tomkennedy513 commented 2 years ago

/remove-lifecycle stale

I opened a pr (#5565) to fix this issue

jwitrick commented 1 year ago

This is a side topic... but do you have an example of your task/pipeline configured to use istio? I am facing a similar issue where my NS has istio-injection enabled and my task will keep hanging around (like you described above).

tekton-robot commented 1 year ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale with a justification. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

mmlac commented 1 year ago

/remove-lifecycle stale

PR #5565 is pretty much ready to merge barring a test failure, which is possibly just a matter of renaming a method.

tomkennedy513 commented 1 year ago

I will try to update the pr this week, I got a little sidetracked with other stuff

tomkennedy513 commented 1 year ago

/remove-lifecycle stale

PR #5565 is pretty much ready to merge barring a test failure, which is possibly just a matter of renaming a method.

updated the pr