Skip to content

Commit

Permalink
Allow setting sidecar ready annotation early
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@dibyom
dibyom committed May 26, 2020
1 parent f7710bc commit b66c776
Show file tree
Hide file tree
Showing 4 changed files with 233 additions and 10 deletions.
16 changes: 12 additions & 4 deletions config/config-feature-flags.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,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.
Expand All @@ -38,6 +37,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"
6 changes: 6 additions & 0 deletions docs/install.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,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
Expand Down
27 changes: 25 additions & 2 deletions pkg/pod/pod.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,10 @@ 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"
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
)
Expand Down Expand Up @@ -238,6 +240,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
Expand Down Expand Up @@ -351,3 +357,20 @@ 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. This allows for safe behavior
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
}
194 changes: 190 additions & 4 deletions pkg/pod/pod_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -76,6 +78,7 @@ func TestMakePod(t *testing.T) {
desc string
trs v1beta1.TaskRunSpec
ts v1beta1.TaskSpec
featureFlags map[string]string
want *corev1.PodSpec
wantAnnotations map[string]string
}{{
Expand Down Expand Up @@ -114,6 +117,48 @@ func TestMakePod(t *testing.T) {
}},
Volumes: append(implicitVolumes, toolsVolume, downwardVolume),
},
}, {
desc: "simple with enable-ready-annotation-on-pod-create",
ts: 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: v1beta1.TaskSpec{
Expand Down Expand Up @@ -472,6 +517,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{
Expand Down Expand Up @@ -779,6 +876,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{{
Expand Down Expand Up @@ -825,6 +926,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)
}
}
})
}
}
Expand Down Expand Up @@ -964,3 +1071,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)
}
})
}
}

0 comments on commit b66c776

Please sign in to comment.