Optimize start time for TaskRuns with no sidecars #2158

dibyom · 2020-03-04T21:55:46Z

Changes

When no sidercars are present and the cluster does not use
injected sidecars, the pipeline controller can optimize the
TaskRun Pod creation process by setting the tekton.dev/ready
annotation before pod creation itself instead of setting it after
the pod has been created.

This commit adds an option to a new config-features ConfigMap
called running-in-environment-with-injected-sidecars. Enabling
this option will decrease the time it takes for a TaskRun to start
running(when no sidecars are present). However, for clusters that
use injected sidecars e.g. istio enabling this option can lead to
unexpected behavior.By default, the option is set to "true" for
backwards compatibility.

Fixes #2080

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

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Users can optimize the start time for TaskRuns without sidecars 
running in clusters that do not use injected sidecars (e.g. istio) 
by enabling the following option in the `feature-flags` ConfigMap:
`running-in-environment-with-injected-sidecars`: "false"

ghost

This looks good to me! I intend to replace these configmap lookups with the configmap watcher but that will happen in a future PR.

docs/install.md

dibyom · 2020-03-09T18:08:43Z

I intend to replace these configmap lookups with the configmap watcher but that will happen in a future PR

Yeah, I was just thinking about that -- feature falgs watching can probably be extracted out to its own package or something

dibyom · 2020-03-09T20:56:08Z

pkg/pod/pod_test.go

@@ -780,6 +884,12 @@ script-heredoc-randomly-generated-78c5n
 			if d := cmp.Diff(c.want, &got.Spec, resourceQuantityCmp); d != "" {
 				t.Errorf("Diff(-want, +got):\n%s", d)
 			}
+
+			if c.wantAnnotations != nil {


One thing I realized while adding the tests, is that we were never using the wantAnnotations filed at all (it was only specified for the sidecar tests). I added this block and started ignoring the releaseAnnotation that gets added by default.

thanks for catching this! might also be a sign that the test cases are a bit too complex

bobcatfish

I have lots of minor feedback, one more major thing: I'm not convinced we definitely need a feature flag for this, it's very hard to imagine why anyone would have been relying on the previous functionality, and feature flags make testing + maintenance more complex

docs/install.md

bobcatfish · 2020-03-13T17:19:08Z

config/config-feature-flags.yaml

+  # enabling this option can lead to unexpected behavior.
+  #
+  # See https://github.com/tektoncd/pipeline/issues/2080 for more info.
+  enable-ready-annotation-on-pod-create: "false"


are we 100% sure we want to use a feature flag for this? i feel like we're quickly going to get to a point where most new features use feature flags - maybe this is what we want!

either way, I assume the idea is that in a future release we'd turn this to "true" by default, and maybe remove the flag? If so can we create an issue to track this and make sure we do it?

The reason we need this in a feature-flag is due to us supporting injected sidecars like Istio. If the cluster uses injected sidecars, the controller can not set the Ready annotation early since the sidecar injector might inject a sidecar container that the controller does not know about during pod creation time.

If we decide that injected sidecars are something we do not support injected sidecars, then yes, we do not need to feature-flag this. Though if I recall correctly, the feature was initially designed this way because we explicitly wanted to support the injected sidecar use case (correct me if I'm wrong @sbwsg!)

hm interesting - we definitely want to support injected sidecars!

My understanding of the term "feature flag" is that it's used to control rollouts of features that we eventually intend to make the default - does that match other ppl's understanding? If so, I think this would make sense as a command line config param maybe? or it's own config map? i.e. something separate from feature flags

Or I'm totally wrong and it's normal to use "feature flags" to refer to functionality you intend to retain over the long run.

If the cluster uses injected sidecars, the controller can not set the Ready annotation early since the sidecar injector might inject a sidecar container that the controller does not know about during pod creation time.

I'm a bit sad that we can't find a solution that handles both cases well. 🤔 Have we brainstormed any ideas around this? e.g. what if we did something like:

set the annotation to ready immediately

be notified if the pod is modified (i.e. an admission controller has injected a sidecar), set the annotation to not ready <-- is it possible to do this before the pod actually started running? i wonder how crazy it would be to have our own admission controller for this

Is it possible we could name the flag after after exactly what it's intended for? e.g. something like running-in-environment-with-injected-sidecars? I think it would even be reasonable to default it to false in that case eventually, i.e. apply this optimization by default

My understanding of the term "feature flag" is that it's used to control rollouts of features that we eventually intend to make the default - does that match other ppl's understanding?

Will add a new options or features configMap for this.

I'm a bit sad that we can't find a solution that handles both cases well. 🤔 Have we brainstormed any ideas around this? e.g. what if we did something like:

Some discussion in #2080 and #701. Short of the Sidecar KEP becoming a reality, I think this is the best we can do at this moment

<-- is it possible to do this before the pod actually started running?
No way to guarantee that unfortunately.

Is it possible we could name the flag after after exactly what it's intended for? e.g. something like running-in-environment-with-injected-sidecars? I think it would even be reasonable to default it to false in that case eventually, i.e. apply this optimization by default

I'll change the flag name for now. Maybe we can set the default to false in a later change.

pkg/pod/pod_test.go

pkg/pod/pod.go

bobcatfish · 2020-03-13T17:24:10Z

pkg/pod/pod.go

@@ -224,6 +226,10 @@ func MakePod(images pipeline.Images, taskRun *v1alpha1.TaskRun, taskSpec v1alpha
 	podAnnotations := taskRun.Annotations
 	podAnnotations[ReleaseAnnotation] = ReleaseAnnotationValue

+	if shouldAddReadyAnnotationonPodCreate(taskSpec.Sidecars, kubeclient) {
+		podAnnotations[readyAnnotation] = readyAnnotationValue


it feels a bit odd to me that we are using readyAnnotation directly here AND we're using it in UpdateReady in a totally different file - could it make sense to call UpdateReady here, or in some way refactor this a bit so we can share the code that accesses readyAnnotation?

It is a different file but in the same package though. I don't think we can call UpdateReady here because that is updating an already existing Pod while the usage here is adding to a Pod's definition before it is created.
One thing we could do is move this and UpdateReady to its own file in the same package...but that puts UpdateReady in a different file from all the other Sidecar functions in that file.

bobcatfish · 2020-03-13T17:25:55Z

pkg/pod/pod_test.go


 	for _, c := range []struct {
 		desc            string
 		trs             v1alpha1.TaskRunSpec
 		ts              v1alpha1.TaskSpec
+		featureFlags    map[string]string


this might be a subject for a larger discussion but a couple thoughts here:

the more feature flags we add, the larger the matrix of test cases we're going to need

does it make sense for the pod package to know about feature flags, or could it be that we tell the pod package something more specific like a bool for "update status early if no sidecars"?

the more feature flags we add, the larger the matrix of test cases we're going to need
Indeed though I can't think of a good way around adding more tests at the moment!

does it make sense for the pod package to know about feature flags, or could it be that we tell the pod package something more specific like a bool for "update status early if no sidecars"?

I think we should refactor the feature flags bit into its own package (#2363 is somewhat related)

bobcatfish · 2020-03-13T17:26:26Z

pkg/pod/pod_test.go

@@ -780,6 +884,12 @@ script-heredoc-randomly-generated-78c5n
 			if d := cmp.Diff(c.want, &got.Spec, resourceQuantityCmp); d != "" {
 				t.Errorf("Diff(-want, +got):\n%s", d)
 			}
+
+			if c.wantAnnotations != nil {


thanks for catching this! might also be a sign that the test cases are a bit too complex

dibyom · 2020-04-10T20:58:57Z

I have lots of minor feedback, one more major thing: I'm not convinced we definitely need a feature flag for this, it's very hard to imagine why anyone would have been relying on the previous functionality, and feature flags make testing + maintenance more complex

Sorry for the delay on this. I hope the explanation in #2158 (comment) makes sense. Happy to discuss whether or not we should actually support injected sidecars. If not, we can get rid of this feature-flag!

ghost · 2020-05-21T13:36:44Z

@dibyom @bobcatfish How do we want to proceed here? Seems like we're at an impasse on the fact that this is a feature flag? Does it satisfy if we just rename it for now to running-in-environment-with-injected-sidecars and then decide later if we want to add another separate configmap for runtime-configurations-that-arent-feature-flags (or do something else entirely)?

If not, do we want to explore other concrete solutions to this problem as part of this PR? Or, finally, do we want to close this PR for now and explore them out-of-band?

FWIW I've worked on teams that used the term "feature flags" interchangeably for both 1) limiting exposure to in-development features and 2) long-lived configuration options that give users control over an application's behaviour or layout. I don't feel super strongly either way about Tekton's notion of "feature flags" but I am wondering if we can make some decisions around where this PR is heading next.

/kind misc

dibyom · 2020-05-21T16:48:05Z

@sbwsg Sorry, forgot about this PR. I'm ok with both renaming the flag. I don't feel too strongly about adding this in a features configMap vs a feature-flag one - I'm ok with adding a new one.
I think this is a useful feature until the sidecar KEP becomes a reality. I won't get to this today but I'll fix it up next Tuesday!

dibyom · 2020-05-28T15:06:28Z

@sbwsg @bobcatfish I've updated the PR (finally!)....I've both renamed the feature/option name (running-in-environment-with-injected-sidecars) as well as put it in a different "features" configMap.
While updating the docs, however, I realized that having two configMaps to point users to in the Customizing Pipeline Controller behavior is confusing - you have to know both the option to configure as well as the right configMap (feature-flags vs features) it belongs in. One option is to revert to using a single configMap like before. The other, perhaps, is to maybe put this option in the config-defaults configMap
What do you think?

ghost · 2020-05-28T15:26:32Z

One option is to revert to using a single configMap like before. The other, perhaps, is to maybe put this option in the config-defaults configMap

I'm totally fine with either of these options, with a very very slight preference towards putting it in feature-flags. No harm no foul if it goes the other way though.

ghost

Other than the open question of precisely which configmap to put this in I think the code looks good! Cheers @dibyom !

When no sidercars are present **and** the cluster does not use injected sidecars, the pipeline controller can optimize the TaskRun Pod creation process by setting the `tekton.dev/ready` annotation before pod creation itself instead of setting it after the pod has been created. This commit adds an option to a new `config-features` ConfigMap called `running-in-environment-with-injected-sidecars`. Enabling this option will decrease the time it takes for a TaskRun to start running(when no sidecars are present). However, for clusters that use injected sidecars e.g. istio enabling this option can lead to unexpected behavior.By default, the option is set to "true" for backwards compatibility. Fixes tektoncd#2080 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

dibyom · 2020-05-29T19:25:38Z

Ok, updated PR. Keeping the feature-flags configMap for now!

dibyom · 2020-06-01T21:01:01Z

/test pull-tekton-pipeline-integration-tests

vdemeester

/lgtm
/hold

@bobcatfish for a last look
/cc @adshmh as it will affect #2637 I think

tekton-robot · 2020-06-02T09:36:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, 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~~ [sbwsg,vdemeester]

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

bobcatfish · 2020-06-03T22:25:00Z

Let's do it!

/lgtm

/hold cancel

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 Mar 4, 2020

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 4, 2020

tekton-robot requested review from afrittoli and vdemeester March 4, 2020 21:55

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 4, 2020

dibyom removed request for vdemeester and afrittoli March 4, 2020 21:55

dibyom force-pushed the sidecars branch 4 times, most recently from 19d304e to 2d85c85 Compare March 9, 2020 16:38

dibyom changed the title ~~wip: set sidecar ready annotation early~~ Set sidecar ready annotation early Mar 9, 2020

dibyom marked this pull request as ready for review March 9, 2020 16:39

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 Mar 9, 2020

dibyom requested review from a user and vdemeester March 9, 2020 16:42

ghost approved these changes Mar 9, 2020

View reviewed changes

docs/install.md Outdated Show resolved Hide resolved

docs/install.md 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 9, 2020

dibyom force-pushed the sidecars branch from 2d85c85 to 3268eaf Compare March 9, 2020 20:54

dibyom commented Mar 9, 2020

View reviewed changes

bobcatfish reviewed Mar 13, 2020

View reviewed changes

ghost mentioned this pull request Mar 31, 2020

Report downwardAPI volumes are not allowed to be used in a shared cluster #2307

Closed

dibyom force-pushed the sidecars branch from 3268eaf to f64e567 Compare April 10, 2020 20:57

tekton-robot added the kind/misc Categorizes issue or PR as a miscellaneuous one. label May 21, 2020

dibyom force-pushed the sidecars branch 2 times, most recently from b66c776 to fb20f5e Compare May 26, 2020 20:06

dibyom force-pushed the sidecars branch from fb20f5e to 9e1827b Compare May 28, 2020 15:01

ghost approved these changes May 28, 2020

View reviewed changes

dibyom force-pushed the sidecars branch from 9e1827b to c29ab1d Compare May 29, 2020 19:13

dibyom changed the title ~~Set sidecar ready annotation early~~ Optimize start time for TaskRuns with no sidecars May 29, 2020

vdemeester approved these changes Jun 2, 2020

View reviewed changes

tekton-robot assigned vdemeester Jun 2, 2020

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 2, 2020

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2020

tekton-robot assigned bobcatfish Jun 3, 2020

tekton-robot merged commit 5175c4a into tektoncd:master Jun 3, 2020

jlpettersson mentioned this pull request Jun 4, 2020

Introduce config map watcher for feature flags #2637

Merged

3 tasks

dibyom deleted the sidecars branch June 5, 2020 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize start time for TaskRuns with no sidecars #2158

Optimize start time for TaskRuns with no sidecars #2158

dibyom commented Mar 4, 2020 •

edited

Loading

ghost left a comment

dibyom commented Mar 9, 2020

dibyom Mar 9, 2020

bobcatfish Mar 13, 2020

bobcatfish left a comment

bobcatfish Mar 13, 2020

dibyom Apr 10, 2020

bobcatfish Apr 10, 2020

dibyom May 26, 2020

bobcatfish Mar 13, 2020

dibyom Apr 10, 2020

bobcatfish Mar 13, 2020

dibyom Apr 10, 2020

bobcatfish Mar 13, 2020

dibyom commented Apr 10, 2020

ghost commented May 21, 2020

dibyom commented May 21, 2020

dibyom commented May 28, 2020

ghost commented May 28, 2020

ghost left a comment

dibyom commented May 29, 2020

dibyom commented Jun 1, 2020

vdemeester left a comment

tekton-robot commented Jun 2, 2020

bobcatfish commented Jun 3, 2020

Optimize start time for TaskRuns with no sidecars #2158

Optimize start time for TaskRuns with no sidecars #2158

Conversation

dibyom commented Mar 4, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

ghost left a comment

Choose a reason for hiding this comment

dibyom commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Apr 10, 2020

ghost commented May 21, 2020

dibyom commented May 21, 2020

dibyom commented May 28, 2020

ghost commented May 28, 2020

ghost left a comment

Choose a reason for hiding this comment

dibyom commented May 29, 2020

dibyom commented Jun 1, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 2, 2020

bobcatfish commented Jun 3, 2020

dibyom commented Mar 4, 2020 •

edited

Loading