From 7ae602c9f5d53c0c505f00843bd790530e782ca3 Mon Sep 17 00:00:00 2001 From: Andrew Bayer Date: Tue, 6 Feb 2024 14:39:03 -0500 Subject: [PATCH] Exclude stopped injected sidecars from TaskRun status fixes #7640 In #5565, we started stopping injected sidecars with `nop` in the same way we stop explicitly defined sidecar containers. `MakeTaskRunStatus` was updated to only include explicitly defined sidecars in the `TaskRun` status, rather than just any container in the `TaskRun` pod that doesn't start with `step-`, so while the pod is running, the injected sidecar doesn't show up in the status. However, once the pod has completed and the sidecars are stopped, if the `TaskRun`'s spec contains a sidecar, `updateStoppedSidecarStatus` will be called, and that function's logic for what containers to include in `TaskRun.Status.Sidecars` is still including everything but `step-`-prefixed containers. That should be updated to behave in the same way as `MakeTaskRunStatus`. Signed-off-by: Andrew Bayer --- pkg/pod/entrypoint.go | 4 +-- pkg/pod/status.go | 2 +- pkg/reconciler/taskrun/taskrun.go | 2 +- pkg/reconciler/taskrun/taskrun_test.go | 45 +++++++++++++++++++++++--- 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/pkg/pod/entrypoint.go b/pkg/pod/entrypoint.go index f8c73d7491b..5997131e5bd 100644 --- a/pkg/pod/entrypoint.go +++ b/pkg/pod/entrypoint.go @@ -338,9 +338,9 @@ func IsSidecarStatusRunning(tr *v1.TaskRun) bool { // represents a step. func IsContainerStep(name string) bool { return strings.HasPrefix(name, stepPrefix) } -// isContainerSidecar returns true if the container name indicates that it +// IsContainerSidecar returns true if the container name indicates that it // represents a sidecar. -func isContainerSidecar(name string) bool { return strings.HasPrefix(name, sidecarPrefix) } +func IsContainerSidecar(name string) bool { return strings.HasPrefix(name, sidecarPrefix) } // trimStepPrefix returns the container name, stripped of its step prefix. func trimStepPrefix(name string) string { return strings.TrimPrefix(name, stepPrefix) } diff --git a/pkg/pod/status.go b/pkg/pod/status.go index 2b1cff90d40..f3ad845c3f6 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -147,7 +147,7 @@ func MakeTaskRunStatus(ctx context.Context, logger *zap.SugaredLogger, tr v1.Tas for _, s := range pod.Status.ContainerStatuses { if IsContainerStep(s.Name) { stepStatuses = append(stepStatuses, s) - } else if isContainerSidecar(s.Name) { + } else if IsContainerSidecar(s.Name) { sidecarStatuses = append(sidecarStatuses, s) } } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 0c94d1781ca..ae78e158b8e 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -914,7 +914,7 @@ func isPodAdmissionFailed(err error) bool { func updateStoppedSidecarStatus(pod *corev1.Pod, tr *v1.TaskRun) error { tr.Status.Sidecars = []v1.SidecarState{} for _, s := range pod.Status.ContainerStatuses { - if !podconvert.IsContainerStep(s.Name) { + if podconvert.IsContainerSidecar(s.Name) { var sidecarState corev1.ContainerState if s.LastTerminationState.Terminated != nil { // Sidecar has successfully by terminated by nop image diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index a93f416aee0..6ce0db07982 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -5379,13 +5379,24 @@ status: } func TestStopSidecars_WithInjectedSidecarsNoTaskSpecSidecars(t *testing.T) { + sidecarTask := &v1.Task{ + ObjectMeta: objectMeta("test-task-injected-sidecar", "foo"), + Spec: v1.TaskSpec{ + Steps: []v1.Step{simpleStep}, + Sidecars: []v1.Sidecar{{ + Name: "sidecar1", + Image: "image-id", + }}, + }, + } + taskRun := parse.MustParseV1TaskRun(t, ` metadata: name: test-taskrun-injected-sidecars namespace: foo spec: taskRef: - name: test-task + name: test-task-injected-sidecar status: podName: test-taskrun-injected-sidecars-pod conditions: @@ -5393,6 +5404,11 @@ status: reason: Build succeeded status: "True" type: Succeeded + sidecars: + - name: sidecar1 + container: sidecar-sidecar1 + running: + startedAt: "2000-01-01T01:01:01Z" `) pod := &corev1.Pod{ @@ -5406,6 +5422,10 @@ status: Name: "step-do-something", Image: "my-step-image", }, + { + Name: "sidecar1", + Image: "image-id", + }, { Name: "injected-sidecar", Image: "some-image", @@ -5419,6 +5439,10 @@ status: Name: "step-do-something", State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{}}, }, + { + Name: "sidecar-sidecar1", + State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, + }, { Name: "injected-sidecar", State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, @@ -5430,7 +5454,7 @@ status: d := test.Data{ Pods: []*corev1.Pod{pod}, TaskRuns: []*v1.TaskRun{taskRun}, - Tasks: []*v1.Task{simpleTask}, + Tasks: []*v1.Task{sidecarTask}, } testAssets, cancel := getTaskRunController(t, d) @@ -5450,14 +5474,27 @@ status: t.Fatalf("error retrieving pod: %s", err) } - if len(retrievedPod.Spec.Containers) != 2 { + if len(retrievedPod.Spec.Containers) != 3 { t.Fatalf("expected pod with two containers") } // check that injected sidecar is replaced with nop image - if d := cmp.Diff(images.NopImage, retrievedPod.Spec.Containers[1].Image); d != "" { + if d := cmp.Diff(images.NopImage, retrievedPod.Spec.Containers[2].Image); d != "" { t.Errorf("expected injected sidecar image to be replaced with nop image %s", diff.PrintWantGot(d)) } + + // Get the updated TaskRun. + reconciledRun, err := clients.Pipeline.TektonV1().TaskRuns(taskRun.Namespace).Get(testAssets.Ctx, taskRun.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Error getting updated TaskRun after reconcile: %v", err) + } + + // Verify that the injected sidecar isn't present in the TaskRun's status. + for _, sc := range reconciledRun.Status.Sidecars { + if sc.Container == "injected-sidecar" { + t.Errorf("expected not to find injected-sidecar in TaskRun status, but found %v", sc) + } + } } func Test_validateTaskSpecRequestResources_ValidResources(t *testing.T) {