Skip to content

Commit

Permalink
Add injected sidecars to TaskRun Status for proper cleanup
Browse files Browse the repository at this point in the history
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 result in the status of injected sidecars being added
to the TaskRun Status so that podconvert.IsSidecarStatusRunning can successfully
track their state and exit stopSidecars early in the case that no
sidecars are injected or defined in the spec to prevent unnecesary
api calls to get pods. Side effects of this change are that
non-user-defined sidecars will show in the TaskRun status.

/kind bug

Resolves #4731
  • Loading branch information
tomkennedy513 committed Sep 27, 2022
1 parent a5970b1 commit 8f686f8
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 12 deletions.
4 changes: 0 additions & 4 deletions pkg/pod/entrypoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,10 +285,6 @@ func IsSidecarStatusRunning(tr *v1beta1.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
// represents a sidecar.
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) }

Expand Down
2 changes: 1 addition & 1 deletion pkg/pod/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev
for _, s := range pod.Status.ContainerStatuses {
if IsContainerStep(s.Name) {
stepStatuses = append(stepStatuses, s)
} else if isContainerSidecar(s.Name) {
} else {
sidecarStatuses = append(sidecarStatuses, s)
}
}
Expand Down
12 changes: 10 additions & 2 deletions pkg/pod/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -425,8 +425,16 @@ func TestMakeTaskRunStatus(t *testing.T) {
want: v1beta1.TaskRunStatus{
Status: statusFailure(ReasonCreateContainerConfigError, "Failed to create pod due to config error"),
TaskRunStatusFields: v1beta1.TaskRunStatusFields{
Steps: []v1beta1.StepState{},
Sidecars: []v1beta1.SidecarState{},
Steps: []v1beta1.StepState{},
Sidecars: []v1beta1.SidecarState{
{
ContainerState: corev1.ContainerState{
Waiting: &corev1.ContainerStateWaiting{
Reason: "CreateContainerConfigError",
},
},
},
},
},
},
}, {
Expand Down
5 changes: 0 additions & 5 deletions pkg/reconciler/taskrun/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,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 {
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 {
Expand Down
60 changes: 60 additions & 0 deletions pkg/reconciler/taskrun/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4336,6 +4336,66 @@ status:
}
}

func TestStopSidecars_WithInjectedSidecarsNoTaskSpecSidecars(t *testing.T) {
taskRun := parse.MustParseTaskRun(t, `
metadata:
name: test-taskrun-injected-sidecars
namespace: foo
spec:
taskRef:
name: test-task
status:
podName: test-taskrun-injected-sidecars-pod
conditions:
- message: Build succeeded
reason: Build succeeded
status: "True"
type: Succeeded
sidecars:
- container: injected-sidecar
imageID: image-id
name: sidecar
running:
startedAt: "2022-01-01T00:00:00Z"
`)

pod := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "test-taskrun-injected-sidecars-pod",
Namespace: "foo",
},
}

d := test.Data{
Pods: []*corev1.Pod{pod},
TaskRuns: []*v1beta1.TaskRun{taskRun},
Tasks: []*v1beta1.Task{simpleTask},
}

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)
}

// Verify that the pod was retrieved.
getPodFound := false
for _, action := range clients.Kube.Actions() {
if action.Matches("get", "pods") {
getPodFound = true
break
}
}
if !getPodFound {
t.Errorf("expected the pod to be retrieved to check if sidecars need to be stopped")
}
}

func TestStopSidecars_NoClientGetPodForTaskSpecWithoutRunningSidecars(t *testing.T) {
for _, tc := range []struct {
desc string
Expand Down

0 comments on commit 8f686f8

Please sign in to comment.