Fixes: #1131, Enable graceful sidecar kill

docs/taskruns.md

@@ -225,6 +225,17 @@ a `TaskRun` with the condition that each `Sidecar` running inside the
 This might result in the `Pod` including each affected `Sidecar` with a 
 retry count of 1 and a different container image than expected.
 
+However, this behaviour can be overridden by the `waitForTermination` flag
+in each `Sidecar` definition. 
+
+If you set this flag to `true` for a given `Sidecar`, Tekton would not attempt to 
+terminate the `Sidecar` even if all the steps were finished in the task run. You 
+should make sure your `Sidecar` container `knows` when and how to terminate itself 
+to avoid timeout scenarios. 
+
+This does not apply to the injected sidecars. They will get force terminated as 
+explained above.
+
 We are aware of the following issues affecting Tekton's implementation of `Sidecars`:
 
 - The configured `nop` image **must not** provide the command that the

internal/builder/v1beta1/task.go

@@ -185,7 +185,7 @@ func Step(image string, ops ...StepOp) TaskSpecOp {
 
 // Sidecar adds a sidecar container with the specified name and image to the TaskSpec.
 // Any number of Container modifier can be passed to transform it.
-func Sidecar(name, image string, ops ...ContainerOp) TaskSpecOp {
+func Sidecar(name, image string, waitForTermination bool, ops ...ContainerOp) TaskSpecOp {
 	return func(spec *v1beta1.TaskSpec) {
 		c := corev1.Container{
 			Name:  name,
@@ -194,7 +194,7 @@ func Sidecar(name, image string, ops ...ContainerOp) TaskSpecOp {
 		for _, op := range ops {
 			op(&c)
 		}
-		spec.Sidecars = append(spec.Sidecars, v1beta1.Sidecar{Container: c})
+		spec.Sidecars = append(spec.Sidecars, v1beta1.Sidecar{Container: c, WaitForTermination: waitForTermination})
 	}
 }
 
@@ -356,7 +356,7 @@ func TaskRunNamespace(namespace string) TaskRunOp {
 	}
 }
 
-// TaskRunStatus sets the TaskRunStatus to tshe TaskRun
+// TaskRunStatus sets the TaskRunStatus to the TaskRun
 func TaskRunStatus(ops ...TaskRunStatusOp) TaskRunOp {
 	return func(tr *v1beta1.TaskRun) {
 		status := &tr.Status
@@ -420,6 +420,17 @@ func SidecarState(ops ...SidecarStateOp) TaskRunStatusOp {
 	}
 }
 
+// TaskSpec adds a TaskSpec to the TaskRunStatus.
+func TaskrunStatusTaskSpec(ops ...TaskSpecOp) TaskRunStatusOp {
+	return func(s *v1beta1.TaskRunStatus) {
+		spec := &v1beta1.TaskSpec{}
+		for _, op := range ops {
+			op(spec)
+		}
+		s.TaskSpec = spec
+	}
+}
+
 // TaskRunStartTime sets the start time to the TaskRunStatus.
 func TaskRunStartTime(startTime time.Time) TaskRunStatusOp {
 	return func(s *v1beta1.TaskRunStatus) {

pkg/apis/pipeline/v1beta1/task_types.go

@@ -134,6 +134,14 @@ type Sidecar struct {
 	//
 	// If Script is not empty, the Step cannot have an Command or Args.
 	Script string `json:"script,omitempty"`
+
+	// WaitForTermination if set will disable the automatic force termination of sidecars. Your sidecar will run as
+	// long as it needs without Tekton interrupting it. Caution: If your sidecar does not know when/how to terminate
+	// by itself, your pipeline may hang till the pipeline timeout without showing proper status even though all the
+	// steps in your pipeline are completed.
+	//
+	// +optional
+	WaitForTermination bool `json:"waitForTermination,omitempty,default:false"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

pkg/pod/entrypoint.go

@@ -17,8 +17,10 @@ limitations under the License.
 package pod
 
 import (
+	"context"
 	"errors"
 	"fmt"
+	"knative.dev/pkg/logging"
 	"path/filepath"
 	"strings"
 
@@ -182,27 +184,27 @@ func UpdateReady(kubeclient kubernetes.Interface, pod corev1.Pod) error {
 
 // StopSidecars updates sidecar containers in the Pod to a nop image, which
 // exits successfully immediately.
-func StopSidecars(nopImage string, kubeclient kubernetes.Interface, pod corev1.Pod) error {
+func StopSidecars(nopImage string, kubeclient kubernetes.Interface, pod corev1.Pod, containers *[]corev1.Container) (error) {
+	logger := logging.FromContext(context.Background())
+	updated := false
 	newPod, err := kubeclient.CoreV1().Pods(pod.Namespace).Get(pod.Name, metav1.GetOptions{})
 	if err != nil {
 		return fmt.Errorf("error getting Pod %q when stopping sidecars: %w", pod.Name, err)
 	}
-
-	updated := false
 	if newPod.Status.Phase == corev1.PodRunning {
 		for _, s := range newPod.Status.ContainerStatuses {
-			// Stop any running container that isn't a step.
-			// An injected sidecar container might not have the
-			// "sidecar-" prefix, so we can't just look for that
-			// prefix.
-			if !IsContainerStep(s.Name) && s.State.Running != nil {
-				for j, c := range newPod.Spec.Containers {
-					if c.Name == s.Name && c.Image != nopImage {
-						updated = true
-						newPod.Spec.Containers[j].Image = nopImage
+			for _, containerItem := range *containers {
+				if s.Name == containerItem.Name && s.State.Running != nil {
+					for j, c := range newPod.Spec.Containers {
+						if c.Name == s.Name && c.Image != nopImage {
+							updated = true
+							logger.Infof("Attempting to terminate Sidecar container %s since it was not marked for exemption from force termination in pod using waitForTermination flag %s.", s.Name, newPod.Name)
+							newPod.Spec.Containers[j].Image = nopImage
+						}
 					}
 				}
 			}
+
 		}
 	}
 	if updated {
@@ -229,9 +231,9 @@ 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
+// 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) }

pkg/pod/entrypoint_test.go

@@ -412,7 +412,8 @@ func TestStopSidecars(t *testing.T) {
 	}} {
 		t.Run(c.desc, func(t *testing.T) {
 			kubeclient := fakek8s.NewSimpleClientset(&c.pod)
-			if err := StopSidecars(nopImage, kubeclient, c.pod); err != nil {
+			stoppableContainers := append([]corev1.Container{}, sidecarContainer, injectedSidecar)
+			if err := StopSidecars(nopImage, kubeclient, c.pod, &stoppableContainers); err != nil {
 				t.Errorf("error stopping sidecar: %v", err)
 			}
 
@@ -424,4 +425,4 @@ func TestStopSidecars(t *testing.T) {
 			}
 		})
 	}
-}
+}

pkg/pod/status.go

@@ -126,7 +126,7 @@ func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev
 				ContainerName:  s.Name,
 				ImageID:        s.ImageID,
 			})
-		} else if isContainerSidecar(s.Name) {
+		} else if IsContainerSidecar(s.Name) {
 			trs.Sidecars = append(trs.Sidecars, v1beta1.SidecarState{
 				ContainerState: *s.State.DeepCopy(),
 				Name:           TrimSidecarPrefix(s.Name),

pkg/reconciler/taskrun/taskrun.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"go.uber.org/zap"
 	"reflect"
 	"strings"
 	"time"
@@ -123,7 +124,10 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg
 		c.timeoutHandler.Release(tr)
 		pod, err := c.KubeClientSet.CoreV1().Pods(tr.Namespace).Get(tr.Status.PodName, metav1.GetOptions{})
 		if err == nil {
-			err = podconvert.StopSidecars(c.Images.NopImage, c.KubeClientSet, *pod)
+			logger.Info("Stopping Sidecars that were not marked for exemption.")
+			sidecars := tr.Status.TaskSpec.Sidecars
+			stoppableContainers := getStoppableContainers(logger, pod, sidecars)
+			err := podconvert.StopSidecars(c.Images.NopImage, c.KubeClientSet, *pod, &stoppableContainers)
 			if err == nil {
 				// Check if any SidecarStatuses are still shown as Running after stopping
 				// Sidecars. If any Running, update SidecarStatuses based on Pod ContainerStatuses.
@@ -692,7 +696,7 @@ func updateStoppedSidecarStatus(ctx context.Context, pod *corev1.Pod, tr *v1beta
 		if !podconvert.IsContainerStep(s.Name) {
 			var sidecarState corev1.ContainerState
 			if s.LastTerminationState.Terminated != nil {
-				// Sidecar has successfully by terminated by nop image
+				// Sidecar has been terminated successfully by nop image
 				lastTerminatedState := s.LastTerminationState.Terminated
 				sidecarState = corev1.ContainerState{
 					Terminated: &corev1.ContainerStateTerminated{
@@ -750,6 +754,27 @@ func storeTaskSpec(ctx context.Context, tr *v1beta1.TaskRun, ts *v1beta1.TaskSpe
 	return nil
 }
 
+func getStoppableContainers(logger *zap.SugaredLogger, pod *corev1.Pod, sidecars []v1beta1.Sidecar) []corev1.Container {
+	stoppables := []corev1.Container{}
+	// Sidecars that does not have waitForTermination enabled should be terminated.
+	logger.Info("Gathering Tekton Sidecars...")
+	for _, sidecar := range sidecars {
+		if sidecar.WaitForTermination != true {
+			stoppables = append(stoppables, sidecar.Container)
+		}
+	}
+
+	logger.Info("Gathering injected Sidecars...")
+	// Anything that's not a sidecar or a step must be injected sidecar and thus should be marked for force termination.
+	for _, containerItem := range pod.Spec.Containers {
+		if podconvert.IsContainerStep(containerItem.Name) || podconvert.IsContainerSidecar(containerItem.Name) {
+			continue
+		}
+		stoppables = append(stoppables, containerItem)
+	}
+	return stoppables
+}
+
 // validateWorkspaceCompatibilityWithAffinityAssistant validates the TaskRun's compatibility
 // with the Affinity Assistant - if associated with an Affinity Assistant.
 // No more than one PVC-backed workspace can be used for a TaskRun that is associated with an

pkg/reconciler/taskrun/taskrun_test.go

@@ -112,11 +112,11 @@ var (
 	), tb.TaskNamespace("foo"))
 	clustertask = tb.ClusterTask("test-cluster-task", tb.ClusterTaskSpec(simpleStep))
 	taskSidecar = tb.Task("test-task-sidecar", tb.TaskSpec(
-		tb.Sidecar("sidecar", "image-id"),
+		tb.Sidecar("sidecar", "image-id", false),
 	), tb.TaskNamespace("foo"))
 	taskMultipleSidecars = tb.Task("test-task-sidecar", tb.TaskSpec(
-		tb.Sidecar("sidecar", "image-id"),
-		tb.Sidecar("sidecar2", "image-id"),
+		tb.Sidecar("sidecar", "image-id", false),
+		tb.Sidecar("sidecar2", "image-id", false),
 	), tb.TaskNamespace("foo"))
 
 	outputTask = tb.Task("test-output-task", tb.TaskSpec(
@@ -3185,3 +3185,147 @@ func Test_storeTaskSpec(t *testing.T) {
 		t.Fatalf(diff.PrintWantGot(d))
 	}
 }
+
+func TestReconcile_Multiple_SidecarStates_With_WaitForTermination_Flag_Enabled(t *testing.T) {
+	trueB := true
+	nopImage := "override-with-nop:latest"
+	runningState := corev1.ContainerStateRunning{StartedAt: metav1.Time{Time: time.Now()}}
+	taskMultipleSidecarsWithWaitForTerminationEnabled := tb.Task("test-task-sidecar-termination-flag", tb.TaskSpec(
+		tb.Step("foo", tb.StepName("step-simple"), tb.StepCommand("/mycmd")),
+		tb.Sidecar("sidecar-sidecar1", "image-id", false),
+		tb.Sidecar("sidecar-sidecar2", "image-id", true),
+	), tb.TaskNamespace("foo"))
+
+	taskRun := tb.TaskRun("test-taskrun-sidecars-termination-flag",
+		tb.TaskRunSpec(
+			tb.TaskRunTaskRef(taskMultipleSidecarsWithWaitForTerminationEnabled.Name),
+		),
+		tb.TaskRunStatus(
+			tb.TaskrunStatusTaskSpec(
+				tb.Step("foo", tb.StepName("step-simple"), tb.StepCommand("/mycmd")),
+				tb.Sidecar("sidecar-sidecar1", "image-id", false),
+				tb.Sidecar("sidecar-sidecar2", "image-id", true),
+			),
+			tb.SidecarState(
+				tb.SidecarStateName("sidecar-sidecar1"),
+				tb.SidecarStateImageID("image-id"),
+				tb.SidecarStateContainerName("sidecar-sidecar1"),
+				tb.SetSidecarStateRunning(runningState),
+			),
+			tb.SidecarState(
+				tb.SidecarStateName("sidecar-sidecar2"),
+				tb.SidecarStateImageID("image-id"),
+				tb.SidecarStateContainerName("sidecar-sidecar2"),
+				tb.SetSidecarStateRunning(runningState),
+			),
+			tb.StepState(tb.StateTerminated(0)),
+			tb.PodName("test-taskrun-sidecars-termination-flag-pod"),
+			tb.StatusCondition(apis.Condition{
+				Type:   apis.ConditionSucceeded,
+				Status: corev1.ConditionFalse,
+			}),
+			tb.StatusCondition(apis.Condition{Type: apis.ConditionSucceeded, Status: corev1.ConditionTrue, Reason: "Completed"}),
+		),
+		tb.TaskRunNamespace("foo"),
+	)
+
+	d := test.Data{
+		TaskRuns: []*v1beta1.TaskRun{taskRun},
+		Tasks:    []*v1beta1.Task{taskMultipleSidecarsWithWaitForTerminationEnabled},
+		Pods: []*corev1.Pod{
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-taskrun-sidecars-termination-flag-pod",
+					Namespace: "foo",
+					OwnerReferences: []metav1.OwnerReference{{
+						Kind:               "TaskRun",
+						Name:               "test-taskrun-sidecars-termination-flag",
+						APIVersion:         "a1",
+						Controller:         &trueB,
+						BlockOwnerDeletion: &trueB,
+					}},
+				},
+				Spec: corev1.PodSpec{
+					ServiceAccountName: "sa",
+					RestartPolicy:      corev1.RestartPolicyNever,
+					Containers: []corev1.Container{
+						{Name: "step-simple", Image: "foo"},
+						{Name: "injected-container", Image: "injected"},
+						{Name: "sidecar-sidecar1", Image: "image-id"},
+						{Name: "sidecar-sidecar2", Image: "image-id"},
+					},
+				},
+				Status: corev1.PodStatus{
+					Phase: corev1.PodRunning,
+					ContainerStatuses: []corev1.ContainerStatus{
+						{
+							Name:  "step-simple",
+							Image: "foo",
+							State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{}},
+						}, {
+							Name: "injected-container",
+							// Sidecar is running.
+							Image: "injected",
+							State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: metav1.NewTime(time.Now())}},
+						}, {
+							Name: "sidecar-sidecar1",
+							// Sidecar is running.
+							Image: "image-id",
+							State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: metav1.NewTime(time.Now())}},
+						}, {
+							Name:  "sidecar-sidecar2",
+							Image: "image-id",
+							// sidecar is running.
+							State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: metav1.NewTime(time.Now())}},
+						}},
+				},
+			},
+		},
+	}
+
+	testAssets, cancel := getTaskRunController(t, d)
+	defer cancel()
+	clients := testAssets.Clients
+
+	if err := testAssets.Controller.Reconciler.Reconcile(context.Background(), getRunName(taskRun)); err != nil {
+		t.Errorf("expected no error reconciling valid TaskRun but got %v", err)
+	}
+
+	retrievedPod, err := clients.Kube.CoreV1().Pods(taskRun.Namespace).Get(taskRun.Status.PodName, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Failed retrieving the pod for comparing")
+	}
+
+	if retrievedPod.Spec.Containers[0].Name != "step-simple" {
+		t.Errorf(".Pod.Spec.Containers[0].Name must be %s", "step-simple")
+	}
+
+	if retrievedPod.Spec.Containers[0].Image != "foo" {
+		t.Errorf(".Pod.Spec.Containers[0].Image must be %s", "foo")
+	}
+
+	if retrievedPod.Spec.Containers[1].Name != "injected-container" {
+		t.Errorf(".Pod.Spec.Containers[1].Name must be %s", "injected-container")
+	}
+
+	if retrievedPod.Spec.Containers[1].Image != nopImage {
+		t.Errorf(".Pod.Spec.Containers[1].Image must be %s", nopImage)
+	}
+
+	if retrievedPod.Spec.Containers[2].Name != "sidecar-sidecar1" {
+		t.Errorf(".Pod.Spec.Containers[2].Name must be %s", "sidecar-sidecar1")
+	}
+
+	if retrievedPod.Spec.Containers[2].Image != nopImage {
+		t.Errorf(".Pod.Spec.Containers[2].Image must be %s", nopImage)
+	}
+
+	if retrievedPod.Spec.Containers[3].Name != "sidecar-sidecar2" {
+		t.Errorf(".Pod.Spec.Containers[3].Name must be %s", "sidecar-sidecar2")
+	}
+
+	if retrievedPod.Spec.Containers[3].Image != "image-id" {
+		t.Errorf(".Pod.Spec.Containers[3].Image must be %s", "image-id")
+	}
+
+}