Allow setting sidecar ready annotation early

dibyom · dibyom · commit 3268eafc66c5 · 2020-03-09T16:53:56.000-04:00
If no sidercars are present, we can set the `tekton.dev/ready` annotation during pod creation itself instead waiting for the controller to set it after pod creation. This commit adds an option to the `feature-flags` ConfigMap to control this behavior (`enable-ready-annotation-on-pod-create`). By default, the option is set to "false".Setting this flag to "true" will set the annotation at pod creation time for Taskruns without Sidercars. 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. Fixes #2080 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
diff --git a/config/config-feature-flags.yaml b/config/config-feature-flags.yaml
@@ -25,8 +25,7 @@ data:
   # $HOME environment variable but this will change in an upcoming
   # release.
   #
-  # See https://github.com/tektoncd/pipeline/issues/2013 for more
-  # info.
+  # See https://github.com/tektoncd/pipeline/issues/2013 for more info.
   disable-home-env-overwrite: "false"
   # Setting this flag to "true" will prevent Tekton overriding your
   # Task container's working directory.
@@ -35,6 +34,15 @@ data:
   # working directory if not set by the user but this will change
   # in an upcoming release.
   #
-  # See https://github.com/tektoncd/pipeline/issues/1836 for more
-  # info.
+  # See https://github.com/tektoncd/pipeline/issues/1836 for more info.
   disable-working-directory-overwrite: "false"
+
+  # Setting this flag to "true" will set the `tekton.dev/ready` annotation
+  # at pod creation time for Taskruns without sidecars.
+  #
+  # 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.
+  #
+  # See https://github.com/tektoncd/pipeline/issues/2080 for more info.
+  enable-ready-annotation-on-pod-create: "false"
diff --git a/docs/install.md b/docs/install.md
@@ -273,6 +273,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).
 
+- `enable-ready-annotation-on-pod-create`: 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
diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go
@@ -22,6 +22,7 @@ import (
 
 	"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/names"
 	"github.com/tektoncd/pipeline/pkg/system"
 	"github.com/tektoncd/pipeline/pkg/version"
@@ -37,9 +38,10 @@ const (
 	// ResultsDir is the folder used by default to create the results file
 	ResultsDir = "/tekton/results"
 
-	featureFlagConfigMapName        = "feature-flags"
-	featureFlagDisableHomeEnvKey    = "disable-home-env-overwrite"
-	featureFlagDisableWorkingDirKey = "disable-working-directory-overwrite"
+	featureFlagConfigMapName                 = "feature-flags"
+	featureFlagDisableHomeEnvKey             = "disable-home-env-overwrite"
+	featureFlagDisableWorkingDirKey          = "disable-working-directory-overwrite"
+	featureFlagSetReadyAnnotationOnPodCreate = "enable-ready-annotation-on-pod-create"
 
 	taskRunLabelKey = pipeline.GroupName + pipeline.TaskRunLabelKey
 )
@@ -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
+	}
+
 	return &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			// We execute the build's pod in the same namespace as where the build was
@@ -336,3 +342,22 @@ 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. The
+// default behavior is to add the Ready annotation after  Pod has been created
+// and all of its sidecars are in a Ready state.
+//
+// For further reference see https://github.com/tektoncd/pipeline/issues/2080
+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 the feature is enabled.
+	configMap, err := kubeclient.CoreV1().ConfigMaps(system.GetNamespace()).Get(featureFlagConfigMapName, metav1.GetOptions{})
+	if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureFlagSetReadyAnnotationOnPodCreate] == "true" {
+		return true
+	}
+	return false
+}
diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go
@@ -23,6 +23,7 @@ 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"
@@ -40,41 +41,46 @@ 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: homeDir,
-	}}
-	secretsVolumeMount := corev1.VolumeMount{
-		Name:      "tekton-internal-secret-volume-multi-creds-9l9zj",
-		MountPath: "/tekton/creds-secrets/multi-creds",
-	}
-	secretsVolume := corev1.Volume{
-		Name:         "tekton-internal-secret-volume-multi-creds-9l9zj",
-		VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: "multi-creds"}},
-	}
+	var (
+		secretsVolumeMount = corev1.VolumeMount{
+			Name:      "tekton-internal-secret-volume-multi-creds-9l9zj",
+			MountPath: "/tekton/creds-secrets/multi-creds",
+		}
+		secretsVolume = corev1.Volume{
+			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},
-	}
+		implicitEnvVars = []corev1.EnvVar{{
+			Name:  "HOME",
+			Value: homeDir,
+		}}
 
-	runtimeClassName := "gvisor"
-	automountServiceAccountToken := false
-	dnsPolicy := corev1.DNSNone
-	enableServiceLinks := false
-	priorityClassName := "system-cluster-critical"
+		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
+		enableServiceLinks           = false
+		priorityClassName            = "system-cluster-critical"
+	)
 
 	for _, c := range []struct {
 		desc            string
 		trs             v1alpha1.TaskRunSpec
 		ts              v1alpha1.TaskSpec
+		featureFlags    map[string]string
 		want            *corev1.PodSpec
 		wantAnnotations map[string]string
 	}{{
@@ -113,6 +119,48 @@ func TestMakePod(t *testing.T) {
 			}},
 			Volumes: append(implicitVolumes, toolsVolume, downwardVolume),
 		},
+	}, {
+		desc: "simple with enable-ready-annotation-on-pod-create",
+		ts: v1alpha1.TaskSpec{TaskSpec: v1beta1.TaskSpec{
+			Steps: []v1alpha1.Step{{Container: corev1.Container{
+				Name:    "name",
+				Image:   "image",
+				Command: []string{"cmd"}, // avoid entrypoint lookup.
+			}}},
+		}},
+		featureFlags: map[string]string{
+			featureFlagSetReadyAnnotationOnPodCreate: "true",
+		},
+		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: v1alpha1.TaskSpec{TaskSpec: v1beta1.TaskSpec{
@@ -471,6 +519,58 @@ sidecar-script-heredoc-randomly-generated-mz4c7
 			}},
 			Volumes: append(implicitVolumes, scriptsVolume, toolsVolume, downwardVolume),
 		},
+	}, {
+		desc: "sidecar container with enable-ready-annotation-on-pod-create",
+		ts: v1alpha1.TaskSpec{TaskSpec: 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: v1alpha1.TaskSpec{TaskSpec: v1beta1.TaskSpec{
@@ -734,6 +834,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{{
@@ -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 {
+				if d := cmp.Diff(c.wantAnnotations, got.ObjectMeta.Annotations, cmpopts.IgnoreMapEntries(ignoreReleaseAnnotation)); d != "" {
+					t.Errorf("Annotation Diff(-want, +got):\n%s", d)
+				}
+			}
 		})
 	}
 }
@@ -888,3 +998,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: featureFlagConfigMapName, 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: featureFlagConfigMapName, Namespace: system.GetNamespace()},
+			Data:       map[string]string{},
+		},
+		expected: false,
+	}, {
+		description: "Setting enable-ready-annotation-on-pod-create to false with sidecars present results in false",
+		sidecars:    []v1beta1.Sidecar{sd},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: featureFlagConfigMapName, Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureFlagSetReadyAnnotationOnPodCreate: "false",
+			},
+		},
+		expected: false,
+	}, {
+		description: "Setting enable-ready-annotation-on-pod-create to false with no sidecars present results in false",
+		sidecars:    []v1beta1.Sidecar{},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: featureFlagConfigMapName, Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureFlagSetReadyAnnotationOnPodCreate: "false",
+			},
+		},
+		expected: false,
+	}, {
+		description: "Setting enable-ready-annotation-on-pod-create to true with sidecars present results in false",
+		sidecars:    []v1beta1.Sidecar{sd},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: featureFlagConfigMapName, Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureFlagSetReadyAnnotationOnPodCreate: "true",
+			},
+		},
+		expected: false,
+	}, {
+		description: "Setting enable-ready-annotation-on-pod-create to true with no sidecars present results in true",
+		sidecars:    []v1beta1.Sidecar{},
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: featureFlagConfigMapName, Namespace: system.GetNamespace()},
+			Data: map[string]string{
+				featureFlagSetReadyAnnotationOnPodCreate: "true",
+			},
+		},
+		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)
+			}
+		})
+	}
+}
