Optimize start time for TaskRuns with no sidecars

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

Signed-off-by: Dibyo Mukherjee <[email protected]>

Author: dibyom

config/config-feature-flags.yaml

@@ -41,3 +41,11 @@ data:
   # See https://github.com/tektoncd/pipeline/issues/1836 for more
   # info.
   disable-working-directory-overwrite: "false"
+  # This option should be set to false when Pipelines is running in a
+  # cluster that does not use injected sidecars such as Istio. Setting
+  # it to false should decrease the time it takes for a TaskRun to start
+  # running. For clusters that use injected sidecars, setting this
+  # option to false can lead to unexpected behavior.
+  #
+  # See https://github.com/tektoncd/pipeline/issues/2080 for more info.
+  running-in-environment-with-injected-sidecars: "true"

docs/install.md

@@ -291,6 +291,12 @@ The default value is `false`, which causes Tekton to override the working direct
 for each `Step` that does not have its working directory explicitly set with `/workspace`.
 For more information, see the [associated issue](https://github.com/tektoncd/pipeline/issues/1836).
 
+- `running-in-environment-with-injected-sidecars`: set this flag to `"true"` to allow the
+Tekton controller to set the `tekon.dev/ready` annotation at pod creation time for 
+TaskRuns with no Sidecars specified. Enabling this option should decrease the time it takes for a TaskRun to
+start running. However, for clusters that use injected sidecars e.g. istio
+enabling this option can lead to unexpected behavior.
+
 For example:
 
 ```yaml

pkg/pod/pod.go

@@ -46,8 +46,11 @@ const (
 	// ResultsDir is the folder used by default to create the results file
 	ResultsDir = "/tekton/results"
 
-	featureFlagDisableHomeEnvKey    = "disable-home-env-overwrite"
-	featureFlagDisableWorkingDirKey = "disable-working-directory-overwrite"
+	featureInjectedSidecar                   = "running-in-environment-with-injected-sidecars"
+	featureFlagDisableHomeEnvKey             = "disable-home-env-overwrite"
+	featureFlagDisableWorkingDirKey          = "disable-working-directory-overwrite"
+	featureFlagConfigMapName                 = "feature-flags"
+	featureFlagSetReadyAnnotationOnPodCreate = "enable-ready-annotation-on-pod-create"
 
 	taskRunLabelKey = pipeline.GroupName + pipeline.TaskRunLabelKey
 )
@@ -250,6 +253,10 @@ func MakePod(images pipeline.Images, taskRun *v1beta1.TaskRun, taskSpec v1beta1.
 	podAnnotations := taskRun.Annotations
 	podAnnotations[ReleaseAnnotation] = ReleaseAnnotationValue
 
+	if shouldAddReadyAnnotationOnPodCreate(taskSpec.Sidecars, kubeclient) {
+		podAnnotations[readyAnnotation] = readyAnnotationValue
+	}
+
 	return &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			// We execute the build's pod in the same namespace as where the build was
@@ -382,3 +389,21 @@ func shouldOverrideWorkingDir(kubeclient kubernetes.Interface) bool {
 	}
 	return true
 }
+
+// shouldAddReadyAnnotationonPodCreate returns a bool indicating whether the
+// controller should add the `Ready` annotation when creating the Pod. We cannot
+// add the annotation if Tekton is running in a cluster with injected sidecars
+// or if the Task specifies any sidecars.
+func shouldAddReadyAnnotationOnPodCreate(sidecars []v1beta1.Sidecar, kubeclient kubernetes.Interface) bool {
+	// If the TaskRun has sidecars, we cannot set the READY annotation early
+	if len(sidecars) > 0 {
+		return false
+	}
+	// If the TaskRun has no sidecars, check if we are running in a cluster where sidecars can be injected by other
+	// controllers.
+	configMap, err := kubeclient.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetFeatureFlagsConfigName(), metav1.GetOptions{})
+	if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureInjectedSidecar] == "false" {
+		return true
+	}
+	return false
+}

pkg/pod/pod_test.go

@@ -24,7 +24,9 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/system"
 	"github.com/tektoncd/pipeline/test/diff"
@@ -41,11 +43,13 @@ var (
 		CredsImage:      "override-with-creds:latest",
 		ShellImage:      "busybox",
 	}
+
+	ignoreReleaseAnnotation = func(k string, v string) bool {
+		return k == ReleaseAnnotation
+	}
 )
 
 func TestMakePod(t *testing.T) {
-	names.TestingSeed()
-
 	implicitEnvVars := []corev1.EnvVar{{
 		Name:  "HOME",
 		Value: pipeline.HomeDir,
@@ -58,14 +62,12 @@ func TestMakePod(t *testing.T) {
 		Name:         "tekton-internal-secret-volume-multi-creds-9l9zj",
 		VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: "multi-creds"}},
 	}
-
 	placeToolsInit := corev1.Container{
 		Name:         "place-tools",
 		Image:        images.EntrypointImage,
 		Command:      []string{"cp", "/ko-app/entrypoint", "/tekton/tools/entrypoint"},
 		VolumeMounts: []corev1.VolumeMount{toolsMount},
 	}
-
 	runtimeClassName := "gvisor"
 	automountServiceAccountToken := false
 	dnsPolicy := corev1.DNSNone
@@ -77,6 +79,7 @@ func TestMakePod(t *testing.T) {
 		trs             v1beta1.TaskRunSpec
 		trAnnotation    map[string]string
 		ts              v1beta1.TaskSpec
+		featureFlags    map[string]string
 		want            *corev1.PodSpec
 		wantAnnotations map[string]string
 	}{{
@@ -115,6 +118,48 @@ func TestMakePod(t *testing.T) {
 			}},
 			Volumes: append(implicitVolumes, toolsVolume, downwardVolume),
 		},
+	}, {
+		desc: "simple with running-in-environment-with-injected-sidecar set to false",
+		ts: v1beta1.TaskSpec{
+			Steps: []v1alpha1.Step{{Container: corev1.Container{
+				Name:    "name",
+				Image:   "image",
+				Command: []string{"cmd"}, // avoid entrypoint lookup.
+			}}},
+		},
+		featureFlags: map[string]string{
+			featureInjectedSidecar: "false",
+		},
+		want: &corev1.PodSpec{
+			RestartPolicy:  corev1.RestartPolicyNever,
+			InitContainers: []corev1.Container{placeToolsInit},
+			Containers: []corev1.Container{{
+				Name:    "step-name",
+				Image:   "image",
+				Command: []string{"/tekton/tools/entrypoint"},
+				Args: []string{
+					"-wait_file",
+					"/tekton/downward/ready",
+					"-wait_file_content",
+					"-post_file",
+					"/tekton/tools/0",
+					"-termination_path",
+					"/tekton/termination",
+					"-entrypoint",
+					"cmd",
+					"--",
+				},
+				Env:                    implicitEnvVars,
+				VolumeMounts:           append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...),
+				WorkingDir:             pipeline.WorkspaceDir,
+				Resources:              corev1.ResourceRequirements{Requests: allZeroQty()},
+				TerminationMessagePath: "/tekton/termination",
+			}},
+			Volumes: append(implicitVolumes, toolsVolume, downwardVolume),
+		},
+		wantAnnotations: map[string]string{
+			readyAnnotation: readyAnnotationValue,
+		},
 	}, {
 		desc: "with service account",
 		ts: v1beta1.TaskSpec{
@@ -473,6 +518,58 @@ sidecar-script-heredoc-randomly-generated-mz4c7
 			}},
 			Volumes: append(implicitVolumes, scriptsVolume, toolsVolume, downwardVolume),
 		},
+	}, {
+		desc: "sidecar container with enable-ready-annotation-on-pod-create",
+		ts: v1beta1.TaskSpec{
+			Steps: []v1alpha1.Step{{Container: corev1.Container{
+				Name:    "primary-name",
+				Image:   "primary-image",
+				Command: []string{"cmd"}, // avoid entrypoint lookup.
+			}}},
+			Sidecars: []v1alpha1.Sidecar{{
+				Container: corev1.Container{
+					Name:  "sc-name",
+					Image: "sidecar-image",
+				},
+			}},
+		},
+		featureFlags: map[string]string{
+			featureFlagSetReadyAnnotationOnPodCreate: "true",
+		},
+		wantAnnotations: map[string]string{}, // no ready annotations on pod create since sidecars are present
+		want: &corev1.PodSpec{
+			RestartPolicy:  corev1.RestartPolicyNever,
+			InitContainers: []corev1.Container{placeToolsInit},
+			Containers: []corev1.Container{{
+				Name:    "step-primary-name",
+				Image:   "primary-image",
+				Command: []string{"/tekton/tools/entrypoint"},
+				Args: []string{
+					"-wait_file",
+					"/tekton/downward/ready",
+					"-wait_file_content",
+					"-post_file",
+					"/tekton/tools/0",
+					"-termination_path",
+					"/tekton/termination",
+					"-entrypoint",
+					"cmd",
+					"--",
+				},
+				Env:                    implicitEnvVars,
+				VolumeMounts:           append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...),
+				WorkingDir:             pipeline.WorkspaceDir,
+				Resources:              corev1.ResourceRequirements{Requests: allZeroQty()},
+				TerminationMessagePath: "/tekton/termination",
+			}, {
+				Name:  "sidecar-sc-name",
+				Image: "sidecar-image",
+				Resources: corev1.ResourceRequirements{
+					Requests: nil,
+				},
+			}},
+			Volumes: append(implicitVolumes, toolsVolume, downwardVolume),
+		},
 	}, {
 		desc: "resource request",
 		ts: v1beta1.TaskSpec{
@@ -839,6 +936,10 @@ script-heredoc-randomly-generated-78c5n
 		t.Run(c.desc, func(t *testing.T) {
 			names.TestingSeed()
 			kubeclient := fakek8s.NewSimpleClientset(
+				&corev1.ConfigMap{
+					ObjectMeta: metav1.ObjectMeta{Name: featureFlagConfigMapName, Namespace: system.GetNamespace()},
+					Data:       c.featureFlags,
+				},
 				&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}},
 				&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "service-account", Namespace: "default"},
 					Secrets: []corev1.ObjectReference{{
@@ -892,6 +993,12 @@ script-heredoc-randomly-generated-78c5n
 			if d := cmp.Diff(c.want, &got.Spec, resourceQuantityCmp); d != "" {
 				t.Errorf("Diff %s", diff.PrintWantGot(d))
 			}
+
+			if c.wantAnnotations != nil {
+				if d := cmp.Diff(c.wantAnnotations, got.ObjectMeta.Annotations, cmpopts.IgnoreMapEntries(ignoreReleaseAnnotation)); d != "" {
+					t.Errorf("Annotation Diff(-want, +got):\n%s", d)
+				}
+			}
 		})
 	}
 }
@@ -1031,3 +1138,82 @@ func TestShouldOverrideWorkingDir(t *testing.T) {
 		})
 	}
 }
+
+func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) {
+	sd := v1beta1.Sidecar{
+		Container: corev1.Container{
+			Name: "a-sidecar",
+		},
+	}
+	tcs := []struct {
+		description string
+		sidecars    []v1beta1.Sidecar
+		configMap   *corev1.ConfigMap
+		expected    bool
+	}{{
+		description: "Default behavior with sidecars present: Ready annotation not set on pod create",
+		sidecars:    []v1beta1.Sidecar{sd},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
+			Data:       map[string]string{},
+		},
+		expected: false,
+	}, {
+		description: "Default behavior with no sidecars present: Ready annotation not set on pod create",
+		sidecars:    []v1beta1.Sidecar{},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
+			Data:       map[string]string{},
+		},
+		expected: false,
+	}, {
+		description: "Setting running-in-environment-with-injected-sidecars to true with sidecars present results in false",
+		sidecars:    []v1beta1.Sidecar{sd},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureInjectedSidecar: "true",
+			},
+		},
+		expected: false,
+	}, {
+		description: "Setting running-in-environment-with-injected-sidecars to true with no sidecars present results in false",
+		sidecars:    []v1beta1.Sidecar{},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureInjectedSidecar: "true",
+			},
+		},
+		expected: false,
+	}, {
+		description: "Setting running-in-environment-with-injected-sidecars to false with sidecars present results in false",
+		sidecars:    []v1beta1.Sidecar{sd},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureInjectedSidecar: "false",
+			},
+		},
+		expected: false,
+	}, {
+		description: "Setting running-in-environment-with-injected-sidecars to false with no sidecars present results in true",
+		sidecars:    []v1beta1.Sidecar{},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureInjectedSidecar: "false",
+			},
+		},
+		expected: true,
+	}}
+
+	for _, tc := range tcs {
+		t.Run(tc.description, func(t *testing.T) {
+			kubclient := fakek8s.NewSimpleClientset(tc.configMap)
+			if result := shouldAddReadyAnnotationOnPodCreate(tc.sidecars, kubclient); result != tc.expected {
+				t.Errorf("expected: %t Recieved: %t", tc.expected, result)
+			}
+		})
+	}
+}