Graceful sidecar support #936

joseblas · 2019-05-31T12:09:04Z

Changes

TL;DR Tekton Pipelines now gracefully start and stop with injected sidecars running alongside TaskRun pods.

Long Version :-

TaskRun pods can have sidecars injected by Admission Controllers. This is common practice with Istio where a proxy sidecar is injected into pods so that they can be included as part of a service mesh. Unfortunately there is no built-in Kubernetes mechanism to set the lifetime of a sidecar container to match that of the pod's "primary" container. In practice this means that pods injected with sidecars will not stop until both the primary and all sidecar containers are stopped.

Prior to this commit any non-istio injected sidecars would cause a TaskRun pod to run forever, even when all Step containers of a Task had completed. Istio injection was also disabled completely on TR pods. This commit introduces mechanisms to start and stop sidecars gracefully meaning that a) Step containers wait until all sidecars are in a ready state before starting and b) Sidecar containers are stopped when a TaskRun's Step containers are done. Additionally, the annotation that prevented Istio sidecar injection has been removed. Istio now adds sidecars to TaskRun pods as expected.

No end-to-end tests have been added as part of this PR because doing so would require Istio to be enabled on the CI cluster (or a dummy Admission Controller / Mutating Webhook thing to be written just for injecting containers into test Pods). The Istio requirement would put an undue burden on those users running tests in non-GKE environments. It's currently planned for a follow-up PR to introduce a) user-defined sidecars and b) a Tekton-injected sidecar that performs logging. Once that work is complete it will be much simpler to e2e test this sidecar code.

For reference, see the POC PR that was originally created here: #728

For the design proposal and associated discussion around the original POC please see #727

For the remaining work, test coverage, code organization, etc, see #926

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

Release Notes

Sidecar containers injected by, for example, Istio, will now gracefully start up and shutdown in tandem with a Task Steps' containers.

googlebot · 2019-05-31T12:09:12Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

ghost · 2019-05-31T12:34:25Z

/ok-to-test

ghost · 2019-05-31T13:12:23Z

/test pull-tekton-pipeline-integration-tests

ghost · 2019-05-31T14:26:03Z

/test pull-tekton-pipeline-integration-tests

ghost · 2019-05-31T14:36:50Z

Hm, from the integration tests:

I0531 14:26:40.473] 2019/05/31 14:26:40 main.go:761: --gcp-project is missing, trying to fetch a project from boskos.
I0531 14:26:40.474] (for local runs please set --gcp-project to your dev project)
I0531 14:26:40.474] 2019/05/31 14:26:40 main.go:773: provider gke, will acquire project type gke-project from boskos
I0531 14:26:40.484] 2019/05/31 14:26:40 main.go:312: Something went wrong: failed to prepare test environment: --provider=gke boskos failed to acquire project: resource not found

This looks suspiciously like a configuration or platform issue.

joseblas · 2019-05-31T14:39:00Z

/test all

joseblas · 2019-05-31T14:55:36Z

/retest

joseblas · 2019-06-04T13:55:53Z

Hi @dicarlo2 @bobcatfish @sbwsg,
thinking about e2e testing I'm thinking about adding Istio to our Boskos config in order to be able to have Sidecars-by-istio in some namespaces to cover this.
Do you agree?

ghost · 2019-06-04T14:01:18Z

Yeah I'm in favor of that if you can make it work, but defer to @bobcatfish and also @vdemeester in case they have concerns given our existing test infra. How would this work if someone is trying to run e2e tests locally or on a non-GKE Kubernetes provider?

joseblas · 2019-06-04T14:03:30Z

Istio/sdecar related tests could be skipped if there is no Istio namespace.

vdemeester · 2019-06-05T09:11:09Z

Having to install istio just for the e2e tests seems a bit much (and heavy) I feel. Discussed a bit with @bobcatfish and @sbwsg and we fell we can defer the e2e a bit (until we work on logging sidecar and thus using this as e2e tests for sidecars).

ghost · 2019-06-05T19:53:57Z

/retest

ghost · 2019-06-05T20:47:11Z

Hey @dicarlo2 I think we're pretty close to having your sidecar PR mergeable here. If you've got some time would you mind reviewing our updates to your work and verifying that the documentation and code comments we've added are accurate and gel with your intentions? Cheers!

ghost · 2019-06-06T13:02:19Z

/retest

dicarlo2 · 2019-06-10T16:50:33Z

LGTM, sorry for the delayed response!

googlebot · 2019-06-10T16:52:45Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

vdemeester

/lgtm
/meow boxes

tekton-robot · 2019-06-17T15:01:19Z

@vdemeester:

In response to this:

/lgtm
/meow boxes

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.

vdemeester · 2019-06-17T15:10:08Z

/test pull-tekton-pipeline-integration-tests

ghost · 2019-06-17T15:12:44Z

/test pull-tekton-pipeline-integration-tests

ghost · 2019-06-17T15:16:26Z

/test pull-tekton-pipeline-integration-tests

ghost · 2019-06-17T15:23:48Z

ಠ_ಠ

main.go:312: Something went wrong: failed to prepare test environment: --provider=gke boskos failed to acquire project: resource not found

ghost · 2019-06-17T15:24:15Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-06-17T16:12:19Z

/test pull-tekton-pipeline-integration-tests

TL;DR Tekton Pipelines now gracefully start and stop with sidecars running alongside TaskRun pods. Long Version :- TaskRun pods can have sidecars injected by Admission Controllers. This is common practice with Istio where a proxy sidecar is injected into pods so that they can be included as part of a service mesh. Unfortunately there is no built-in Kubernetes mechanism to set the lifetime of a sidecar container to match that of the pod's "primary" container. In practice this means that pods injected with sidecars will not stop until both the primary and all sidecar containers are stopped. Prior to this commit injected sidecars would cause a TaskRun pod to run forever, even when all Step containers of a Task had completed. This commit introduces mechanisms to start and stop sidecars gracefully meaning that a) Step containers wait until all sidecars are in a ready state before starting and b) Sidecar containers are stopped when a TaskRun's Step containers are done. No end-to-end tests have been added as part of this PR because doing so would require Istio to be enabled on the CI cluster (or a dummy Admission Controller / Mutating Webhook thing to be written just for injecting containers into test Pods). The Istio requirement would put an undue burden on those users running tests in non-GKE environments. It's currently planned for a follow-up PR to introduce a) user-defined sidecars and b) a Tekton-injected sidecar that performs logging. Once that work is complete it will be much simpler to e2e test this sidecar code. Further to the above, a number of smaller refactors have taken place in this PR: - a sidecars package has been created to encapsulate the logic for stopping sidecars with the nop container image - the updateStatusFromPod func has been moved into a taskrun/status package and broken down into constituent helpers - a small amount of dead code has been eliminated Co-authored-by: Jose Blas Camacho Taboada <joseblas@gmail.com> Co-authored-by: Scott Seaward <sbws@google.com>

googlebot · 2019-06-17T16:23:07Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-06-17T16:23:37Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

vdemeester

/lgtm
🙏

tekton-robot · 2019-06-17T16:47:15Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [vdemeester]

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

ghost · 2019-06-17T16:53:20Z

🙌

dicarlo2 · 2019-06-17T19:08:35Z

awesome work!

Scott has been provided super detailed helpful reviews for a while now (58 reviews as of https://tekton.devstats.cd.foundation/d/46/pr-reviews-by-contributor?orgId=1&var-period=d&var-repo_name=tektoncd%2Fpipeline&var-reviewers="sbwsg"). He has contributed useful and technically challenging features such as tektoncd#905 (recreate pods in face of ResourceQuota errors), drove completion of tektoncd#936 (Graceful sidecar support) and also tektoncd#871 (enforcing default TaskRun timeouts).

Scott has been provided super detailed helpful reviews for a while now (58 reviews as of https://tekton.devstats.cd.foundation/d/46/pr-reviews-by-contributor?orgId=1&var-period=d&var-repo_name=tektoncd%2Fpipeline&var-reviewers="sbwsg"). He has contributed useful and technically challenging features such as #905 (recreate pods in face of ResourceQuota errors), drove completion of #936 (Graceful sidecar support) and also #871 (enforcing default TaskRun timeouts).

This commit adds user-defined sidecars to the TaskSpec. A sidecar is a container intended to provide some auxiliary support to a pod's primary containers. An example usecase is a mock API server for your app to hit during testing. This commit doesn't introduce any extra lifecycle management beyond that added in #936 - sidecars will be stopped without any warning or cleanup time. At the moment I'm thinking of leaving this as-is until the sidecars KEP reaches general availability in Kubernetes.

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2019

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 31, 2019

googlebot added the cla: no label May 31, 2019

tekton-robot requested review from dlorenc and imjasonh May 31, 2019 12:09

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 31, 2019

ghost mentioned this pull request May 31, 2019

WIP/POC: handle sidecar containers #728

Closed

3 tasks

tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 31, 2019

ghost mentioned this pull request May 31, 2019

enhancements for the exporting output image digest #939

Merged

3 tasks

vdemeester mentioned this pull request Jun 6, 2019

Proposal: Tekton objects should be self descriptive using annotations instead of insider info (container name prefix...) #961

Closed

ghost changed the title ~~[WIP] Sidecars (Based on Scott and DiCarlo works)~~ Graceful sidecar support Jun 7, 2019

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 Jun 7, 2019

ghost added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Jun 10, 2019

vdemeester approved these changes Jun 17, 2019

View reviewed changes

tekton-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 17, 2019

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2019

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Jun 17, 2019

ghost added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Jun 17, 2019

vdemeester approved these changes Jun 17, 2019

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2019

tekton-robot merged commit ace3ab6 into tektoncd:master Jun 17, 2019

bobcatfish mentioned this pull request Aug 19, 2019

Add @sbswg as an owner 😼 #1220

Merged

1 task

ghost mentioned this pull request Aug 22, 2019

Add sidecars to Tasks #1236

Merged

3 tasks

pritidesai mentioned this pull request Jan 25, 2024

Adopt Kubernetes native Sidecar Containers #7617

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graceful sidecar support #936

Graceful sidecar support #936

joseblas commented May 31, 2019 •

edited by ghost

Loading

googlebot commented May 31, 2019

ghost commented May 31, 2019

ghost commented May 31, 2019

ghost commented May 31, 2019

ghost commented May 31, 2019

joseblas commented May 31, 2019

joseblas commented May 31, 2019

joseblas commented Jun 4, 2019

ghost commented Jun 4, 2019

joseblas commented Jun 4, 2019

vdemeester commented Jun 5, 2019

ghost commented Jun 5, 2019

ghost commented Jun 5, 2019

ghost commented Jun 6, 2019

dicarlo2 commented Jun 10, 2019

googlebot commented Jun 10, 2019

vdemeester left a comment

tekton-robot commented Jun 17, 2019

vdemeester commented Jun 17, 2019

ghost commented Jun 17, 2019

ghost commented Jun 17, 2019

ghost commented Jun 17, 2019

ghost commented Jun 17, 2019

vdemeester commented Jun 17, 2019

googlebot commented Jun 17, 2019

googlebot commented Jun 17, 2019

vdemeester left a comment

tekton-robot commented Jun 17, 2019

ghost commented Jun 17, 2019

dicarlo2 commented Jun 17, 2019

Graceful sidecar support #936

Graceful sidecar support #936

Conversation

joseblas commented May 31, 2019 • edited by ghost Loading

Changes

Submitter Checklist

Release Notes

googlebot commented May 31, 2019

ghost commented May 31, 2019

ghost commented May 31, 2019

ghost commented May 31, 2019

ghost commented May 31, 2019

joseblas commented May 31, 2019

joseblas commented May 31, 2019

joseblas commented Jun 4, 2019

ghost commented Jun 4, 2019

joseblas commented Jun 4, 2019

vdemeester commented Jun 5, 2019

ghost commented Jun 5, 2019

ghost commented Jun 5, 2019

ghost commented Jun 6, 2019

dicarlo2 commented Jun 10, 2019

googlebot commented Jun 10, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 17, 2019

vdemeester commented Jun 17, 2019

ghost commented Jun 17, 2019

ghost commented Jun 17, 2019

ghost commented Jun 17, 2019

ghost commented Jun 17, 2019

vdemeester commented Jun 17, 2019

googlebot commented Jun 17, 2019

googlebot commented Jun 17, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 17, 2019

ghost commented Jun 17, 2019

dicarlo2 commented Jun 17, 2019

joseblas commented May 31, 2019 •

edited by ghost

Loading