From fdf295cacdb9644dab4d11bbea70337e46f6db01 Mon Sep 17 00:00:00 2001 From: Tom Kennedy Date: Fri, 24 Mar 2023 11:15:09 -0400 Subject: [PATCH] Always call podconvert.StopSidecars to handle injected sidecars Injected sidecars are only cleaned up if sidecars are defined in the task spec because of a check that exits early if the TaskRun does not include any sidecars. This breaks users that are running in environments with injected sidecars, but are not defining sidecars in their TaskRuns. This change will remove the early exit condition when there are no sidecars defined on the TaskRun Spec. This will cause the podconvert.StopSidecars method to run every time, properly cleaning up sidecars the TaskRun doesn't know about. Resolves #4731 --- pkg/reconciler/taskrun/taskrun.go | 10 -- pkg/reconciler/taskrun/taskrun_test.go | 122 ++++++++++++++----------- 2 files changed, 71 insertions(+), 61 deletions(-) diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 168077bacdf..27828cda433 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -253,11 +253,6 @@ func (c *Reconciler) stopSidecars(ctx context.Context, tr *v1beta1.TaskRun) erro return nil } - // do not continue if the TaskSpec had no sidecars - if tr.Status.TaskSpec != nil && len(tr.Status.TaskSpec.Sidecars) == 0 && len(tr.Status.Sidecars) == 0 { - return nil - } - // do not continue if the TaskRun was canceled or timed out as this caused the pod to be deleted in failTaskRun condition := tr.Status.GetCondition(apis.ConditionSucceeded) if condition != nil { @@ -267,11 +262,6 @@ func (c *Reconciler) stopSidecars(ctx context.Context, tr *v1beta1.TaskRun) erro } } - // do not continue if there are no Running sidecars. - if !podconvert.IsSidecarStatusRunning(tr) { - return nil - } - pod, err := podconvert.StopSidecars(ctx, c.Images.NopImage, c.KubeClientSet, tr.Namespace, tr.Status.PodName) if err == nil { // Check if any SidecarStatuses are still shown as Running after stopping diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index de6b97d203e..bc71a4c2fb2 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -4078,65 +4078,85 @@ status: } } -func TestStopSidecars_NoClientGetPodForTaskSpecWithoutRunningSidecars(t *testing.T) { - for _, tc := range []struct { - desc string - tr *v1beta1.TaskRun - }{{ - desc: "no sidecars", - tr: parse.MustParseV1beta1TaskRun(t, ` -metadata: - name: test-taskrun - namespace: foo -status: - conditions: - - status: "True" - type: Succeeded - podName: test-taskrun-pod - startTime: "2000-01-01T01:01:01Z" -`), - }, { - desc: "sidecars are terminated", - tr: parse.MustParseV1beta1TaskRun(t, ` +func TestStopSidecars_WithInjectedSidecarsNoTaskSpecSidecars(t *testing.T) { + taskRun := parse.MustParseV1beta1TaskRun(t, ` metadata: - name: test-taskrun + name: test-taskrun-injected-sidecars namespace: foo +spec: + taskRef: + name: test-task status: + podName: test-taskrun-injected-sidecars-pod conditions: - - status: "True" + - message: Build succeeded + reason: Build succeeded + status: "True" type: Succeeded - podName: test-taskrun-pod - sidecars: - - terminated: - exitCode: 0 - finishedAt: "2000-01-01T01:01:01Z" - startedAt: "2000-01-01T01:01:01Z" - startTime: "2000-01-01T01:01:01Z" -`), - }} { - t.Run(tc.desc, func(t *testing.T) { - d := test.Data{ - TaskRuns: []*v1beta1.TaskRun{tc.tr}, - } +`) - testAssets, cancel := getTaskRunController(t, d) - defer cancel() - c := testAssets.Controller - clients := testAssets.Clients - reconcileErr := c.Reconciler.Reconcile(testAssets.Ctx, getRunName(tc.tr)) + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun-injected-sidecars-pod", + Namespace: "foo", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "step-do-something", + Image: "my-step-image", + }, + { + Name: "injected-sidecar", + Image: "some-image", + }, + }, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "step-do-something", + State: v1.ContainerState{Terminated: &corev1.ContainerStateTerminated{}}, + }, + { + Name: "injected-sidecar", + State: v1.ContainerState{Running: &corev1.ContainerStateRunning{}}, + }, + }, + }, + } - // We do not expect an error - if reconcileErr != nil { - t.Errorf("Expected no error to be returned by reconciler: %v", reconcileErr) - } + d := test.Data{ + Pods: []*corev1.Pod{pod}, + TaskRuns: []*v1beta1.TaskRun{taskRun}, + Tasks: []*v1beta1.Task{simpleTask}, + } - // Verify that the pod was not retrieved - for _, action := range clients.Kube.Actions() { - if action.Matches("get", "pods") { - t.Errorf("expected the pod not to be retrieved because the TaskRun has no sidecars") - } - } - }) + testAssets, cancel := getTaskRunController(t, d) + defer cancel() + c := testAssets.Controller + clients := testAssets.Clients + + reconcileErr := c.Reconciler.Reconcile(testAssets.Ctx, getRunName(taskRun)) + + // We do not expect an error + if reconcileErr != nil { + t.Errorf("Expected no error to be returned by reconciler: %v", reconcileErr) + } + + retrievedPod, err := clients.Kube.CoreV1().Pods(pod.Namespace).Get(testAssets.Ctx, pod.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error retrieving pod: %s", err) + } + + if len(retrievedPod.Spec.Containers) != 2 { + t.Fatalf("expected pod with two containers") + } + + // check that injected sidecar is replaced with nop image + if d := cmp.Diff(retrievedPod.Spec.Containers[1].Image, images.NopImage); d != "" { + t.Errorf("expected injected sidecar image to be replaced with nop image %s", diff.PrintWantGot(d)) } }