Skip to content

Commit

Permalink
Only mount Downward API when necessary
Browse files Browse the repository at this point in the history 
Instead of always mounting a Downward API volume for reading the "ready"
annotation on task Pods, we only create it if the pod will not be
"immediately ready" (i.e. if it has sidecars).
  • Loading branch information
@hWorblehat
hWorblehat committed Jun 10, 2022
1 parent 4c5a505 commit 88a71c3
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 43 deletions.
43 changes: 21 additions & 22 deletions pkg/pod/entrypoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,34 +108,31 @@ var (
// command, we must have fetched the image's ENTRYPOINT before calling this
// method, using entrypoint_lookup.go.
// Additionally, Step timeouts are added as entrypoint flag.
func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Container, taskSpec *v1beta1.TaskSpec, breakpointConfig *v1beta1.TaskRunDebug) ([]corev1.Container, error) {
func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Container, taskSpec *v1beta1.TaskSpec, breakpointConfig *v1beta1.TaskRunDebug, waitForReadyAnnotation bool) ([]corev1.Container, error) {
if len(steps) == 0 {
return nil, errors.New("No steps specified")
}

for i, s := range steps {
var argsForEntrypoint []string
var argsForEntrypoint = []string{}
idx := strconv.Itoa(i)
switch i {
case 0:
argsForEntrypoint = []string{
// First step waits for the Downward volume file.
"-wait_file", filepath.Join(downwardMountPoint, downwardMountReadyFile),
"-wait_file_content", // Wait for file contents, not just an empty file.
// Start next step.
"-post_file", filepath.Join(runDir, idx, "out"),
"-termination_path", terminationPath,
"-step_metadata_dir", filepath.Join(runDir, idx, "status"),
}
default:
// All other steps wait for previous file, write next file.
argsForEntrypoint = []string{
"-wait_file", filepath.Join(runDir, strconv.Itoa(i-1), "out"),
"-post_file", filepath.Join(runDir, idx, "out"),
"-termination_path", terminationPath,
"-step_metadata_dir", filepath.Join(runDir, idx, "status"),
if i == 0 {
if waitForReadyAnnotation {
argsForEntrypoint = append(argsForEntrypoint,
// First step waits for the Downward volume file.
"-wait_file", filepath.Join(downwardMountPoint, downwardMountReadyFile),
"-wait_file_content", // Wait for file contents, not just an empty file.
)
}
} else { // Not the first step - wait for previous
argsForEntrypoint = append(argsForEntrypoint, "-wait_file", filepath.Join(runDir, strconv.Itoa(i-1), "out"))
}
argsForEntrypoint = append(argsForEntrypoint,
// Start next step.
"-post_file", filepath.Join(runDir, idx, "out"),
"-termination_path", terminationPath,
"-step_metadata_dir", filepath.Join(runDir, idx, "status"),
)
argsForEntrypoint = append(argsForEntrypoint, commonExtraEntrypointArgs...)
if taskSpec != nil {
if taskSpec.Steps != nil && len(taskSpec.Steps) >= i+1 {
Expand Down Expand Up @@ -173,8 +170,10 @@ func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Containe
steps[i].Args = argsForEntrypoint
steps[i].TerminationMessagePath = terminationPath
}
// Mount the Downward volume into the first step container.
steps[0].VolumeMounts = append(steps[0].VolumeMounts, downwardMount)
if waitForReadyAnnotation {
// Mount the Downward volume into the first step container.
steps[0].VolumeMounts = append(steps[0].VolumeMounts, downwardMount)
}

return steps, nil
}
Expand Down
58 changes: 52 additions & 6 deletions pkg/pod/entrypoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,53 @@ func TestOrderContainers(t *testing.T) {
},
TerminationMessagePath: "/tekton/termination",
}}
got, err := orderContainers([]string{}, steps, nil, nil)
got, err := orderContainers([]string{}, steps, nil, nil, true)
if err != nil {
t.Fatalf("orderContainers: %v", err)
}
if d := cmp.Diff(want, got); d != "" {
t.Errorf("Diff %s", diff.PrintWantGot(d))
}
}

func TestOrderContainersWithNoWait(t *testing.T) {
steps := []corev1.Container{{
Image: "step-1",
Command: []string{"cmd"},
Args: []string{"arg1", "arg2"},
}, {
Image: "step-2",
Command: []string{"cmd1", "cmd2", "cmd3"}, // multiple cmd elements
Args: []string{"arg1", "arg2"},
VolumeMounts: []corev1.VolumeMount{volumeMount}, // pre-existing volumeMount
}}
want := []corev1.Container{{
Image: "step-1",
Command: []string{entrypointBinary},
Args: []string{
"-post_file", "/tekton/run/0/out",
"-termination_path", "/tekton/termination",
"-step_metadata_dir", "/tekton/run/0/status",
"-entrypoint", "cmd", "--",
"arg1", "arg2",
},
TerminationMessagePath: "/tekton/termination",
}, {
Image: "step-2",
Command: []string{entrypointBinary},
Args: []string{
"-wait_file", "/tekton/run/0/out",
"-post_file", "/tekton/run/1/out",
"-termination_path", "/tekton/termination",
"-step_metadata_dir", "/tekton/run/1/status",
"-entrypoint", "cmd1", "--",
"cmd2", "cmd3",
"arg1", "arg2",
},
VolumeMounts: []corev1.VolumeMount{volumeMount},
TerminationMessagePath: "/tekton/termination",
}}
got, err := orderContainers([]string{}, steps, nil, nil, false)
if err != nil {
t.Fatalf("orderContainers: %v", err)
}
Expand Down Expand Up @@ -127,7 +173,7 @@ func TestOrderContainersWithDebugOnFailure(t *testing.T) {
taskRunDebugConfig := &v1beta1.TaskRunDebug{
Breakpoint: []string{"onFailure"},
}
got, err := orderContainers([]string{}, steps, nil, taskRunDebugConfig)
got, err := orderContainers([]string{}, steps, nil, taskRunDebugConfig, true)
if err != nil {
t.Fatalf("orderContainers: %v", err)
}
Expand Down Expand Up @@ -205,7 +251,7 @@ func TestEntryPointResults(t *testing.T) {
},
TerminationMessagePath: "/tekton/termination",
}}
got, err := orderContainers([]string{}, steps, &taskSpec, nil)
got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
if err != nil {
t.Fatalf("orderContainers: %v", err)
}
Expand Down Expand Up @@ -246,7 +292,7 @@ func TestEntryPointResultsSingleStep(t *testing.T) {
VolumeMounts: []corev1.VolumeMount{downwardMount},
TerminationMessagePath: "/tekton/termination",
}}
got, err := orderContainers([]string{}, steps, &taskSpec, nil)
got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
if err != nil {
t.Fatalf("orderContainers: %v", err)
}
Expand Down Expand Up @@ -283,7 +329,7 @@ func TestEntryPointSingleResultsSingleStep(t *testing.T) {
VolumeMounts: []corev1.VolumeMount{downwardMount},
TerminationMessagePath: "/tekton/termination",
}}
got, err := orderContainers([]string{}, steps, &taskSpec, nil)
got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
if err != nil {
t.Fatalf("orderContainers: %v", err)
}
Expand Down Expand Up @@ -340,7 +386,7 @@ func TestEntryPointOnError(t *testing.T) {
},
TerminationMessagePath: "/tekton/termination",
}}
got, err := orderContainers([]string{}, steps, &taskSpec, nil)
got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
if err != nil {
t.Fatalf("orderContainers: %v", err)
}
Expand Down
17 changes: 11 additions & 6 deletions pkg/pod/pod.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,15 +183,20 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
return nil, err
}

readyImmediately := isPodReadyImmediately(ctx, taskSpec.Sidecars)

if alphaAPIEnabled {
stepContainers, err = orderContainers(credEntrypointArgs, stepContainers, &taskSpec, taskRun.Spec.Debug)
stepContainers, err = orderContainers(credEntrypointArgs, stepContainers, &taskSpec, taskRun.Spec.Debug, !readyImmediately)
} else {
stepContainers, err = orderContainers(credEntrypointArgs, stepContainers, &taskSpec, nil)
stepContainers, err = orderContainers(credEntrypointArgs, stepContainers, &taskSpec, nil, !readyImmediately)
}
if err != nil {
return nil, err
}
volumes = append(volumes, binVolume, downwardVolume)
volumes = append(volumes, binVolume)
if !readyImmediately {
volumes = append(volumes, downwardVolume)
}

// Add implicit env vars.
// They're prepended to the list, so that if the user specified any
Expand Down Expand Up @@ -289,7 +294,7 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
}
podAnnotations[ReleaseAnnotation] = version

if shouldAddReadyAnnotationOnPodCreate(ctx, taskSpec.Sidecars) {
if readyImmediately {
podAnnotations[readyAnnotation] = readyAnnotationValue
}

Expand Down Expand Up @@ -371,11 +376,11 @@ func makeLabels(s *v1beta1.TaskRun) map[string]string {
return labels
}

// shouldAddReadyAnnotationOnPodCreate returns a bool indicating whether the
// isPodReadyImmediately 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(ctx context.Context, sidecars []v1beta1.Sidecar) bool {
func isPodReadyImmediately(ctx context.Context, sidecars []v1beta1.Sidecar) bool {
// If the TaskRun has sidecars, we cannot set the READY annotation early
if len(sidecars) > 0 {
return false
Expand Down
15 changes: 6 additions & 9 deletions pkg/pod/pod_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,6 @@ func TestPodBuild(t *testing.T) {
Image: "image",
Command: []string{"/tekton/bin/entrypoint"},
Args: []string{
"-wait_file",
"/tekton/downward/ready",
"-wait_file_content",
"-post_file",
"/tekton/run/0/out",
"-termination_path",
Expand All @@ -225,13 +222,13 @@ func TestPodBuild(t *testing.T) {
"cmd",
"--",
},
VolumeMounts: append([]corev1.VolumeMount{binROMount, runMount(0, false), downwardMount, {
VolumeMounts: append([]corev1.VolumeMount{binROMount, runMount(0, false), {
Name: "tekton-creds-init-home-0",
MountPath: "/tekton/creds",
}}, implicitVolumeMounts...),
TerminationMessagePath: "/tekton/termination",
}},
Volumes: append(implicitVolumes, binVolume, runVolume(0), downwardVolume, corev1.Volume{
Volumes: append(implicitVolumes, binVolume, runVolume(0), corev1.Volume{
Name: "tekton-creds-init-home-0",
VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}},
}),
Expand Down Expand Up @@ -2035,7 +2032,7 @@ func TestMakeLabels(t *testing.T) {
}
}

func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) {
func TestIsPodReadyImmediately(t *testing.T) {
sd := v1beta1.Sidecar{
Name: "a-sidecar",
}
Expand All @@ -2045,15 +2042,15 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) {
configMap *corev1.ConfigMap
expected bool
}{{
description: "Default behavior with sidecars present: Ready annotation not set on pod create",
description: "Default behavior with sidecars present: Pod is not ready on create",
sidecars: []v1beta1.Sidecar{sd},
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
Data: map[string]string{},
},
expected: false,
}, {
description: "Default behavior with no sidecars present: Ready annotation not set on pod create",
description: "Default behavior with no sidecars present: Pod is not ready on create",
sidecars: []v1beta1.Sidecar{},
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
Expand Down Expand Up @@ -2106,7 +2103,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) {
t.Run(tc.description, func(t *testing.T) {
store := config.NewStore(logtesting.TestLogger(t))
store.OnConfigChanged(tc.configMap)
if result := shouldAddReadyAnnotationOnPodCreate(store.ToContext(context.Background()), tc.sidecars); result != tc.expected {
if result := isPodReadyImmediately(store.ToContext(context.Background()), tc.sidecars); result != tc.expected {
t.Errorf("expected: %t Received: %t", tc.expected, result)
}
})
Expand Down

0 comments on commit 88a71c3

Please sign in to comment.