Revert "Don't rely on .status.podName to find Pod associated with a T…

…askRun"

This reverts commit 0f20c35.

pkg/reconciler/taskrun/cancel.go

@@ -26,19 +26,28 @@ import (
 	"knative.dev/pkg/apis"
 )
 
-// cancelTaskRun marks the TaskRun as cancelled and deletes pods linked to it.
-func cancelTaskRun(tr *v1alpha1.TaskRun, clientset kubernetes.Interface) error {
+type logger interface {
+	Warn(args ...interface{})
+	Warnf(template string, args ...interface{})
+}
+
+// cancelTaskRun marks the TaskRun as cancelled and delete pods linked to it.
+func cancelTaskRun(tr *v1alpha1.TaskRun, clientSet kubernetes.Interface, logger logger) error {
+	logger.Warn("task run %q has been cancelled", tr.Name)
 	tr.Status.SetCondition(&apis.Condition{
 		Type:    apis.ConditionSucceeded,
 		Status:  corev1.ConditionFalse,
 		Reason:  "TaskRunCancelled",
 		Message: fmt.Sprintf("TaskRun %q was cancelled", tr.Name),
 	})
 
-	pod, err := getPod(tr, clientset)
-	if err != nil {
-		return err
+	if tr.Status.PodName == "" {
+		logger.Warnf("task run %q has no pod running yet", tr.Name)
+		return nil
 	}
 
-	return clientset.CoreV1().Pods(tr.Namespace).Delete(pod.Name, &metav1.DeleteOptions{})
+	if err := clientSet.CoreV1().Pods(tr.Namespace).Delete(tr.Status.PodName, &metav1.DeleteOptions{}); err != nil {
+		return err
+	}
+	return nil
 }

pkg/reconciler/taskrun/cancel_test.go

@@ -22,77 +22,78 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
+	ttesting "github.com/tektoncd/pipeline/pkg/reconciler/testing"
 	"github.com/tektoncd/pipeline/test"
+	tb "github.com/tektoncd/pipeline/test/builder"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zaptest/observer"
 	corev1 "k8s.io/api/core/v1"
-	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/apis"
 )
 
 func TestCancelTaskRun(t *testing.T) {
-	namespace := "the-namespace"
-	taskRunName := "the-taskrun"
-	wantStatus := &apis.Condition{
-		Type:    apis.ConditionSucceeded,
-		Status:  corev1.ConditionFalse,
-		Reason:  "TaskRunCancelled",
-		Message: `TaskRun "the-taskrun" was cancelled`,
-	}
-	for _, c := range []struct {
-		desc    string
-		taskRun *v1alpha1.TaskRun
-		pod     *corev1.Pod
+	testCases := []struct {
+		name           string
+		taskRun        *v1alpha1.TaskRun
+		pod            *corev1.Pod
+		expectedStatus apis.Condition
 	}{{
-		desc: "no pod scheduled",
-		taskRun: &v1alpha1.TaskRun{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      taskRunName,
-				Namespace: namespace,
-			},
-			Spec: v1alpha1.TaskRunSpec{
-				Status: v1alpha1.TaskRunSpecStatusCancelled,
-			},
+		name: "no-pod-scheduled",
+		taskRun: tb.TaskRun("test-taskrun-run-cancelled", "foo", tb.TaskRunSpec(
+			tb.TaskRunTaskRef(simpleTask.Name),
+			tb.TaskRunCancelled,
+		), tb.TaskRunStatus(tb.StatusCondition(apis.Condition{
+			Type:   apis.ConditionSucceeded,
+			Status: corev1.ConditionUnknown,
+		}))),
+		expectedStatus: apis.Condition{
+			Type:    apis.ConditionSucceeded,
+			Status:  corev1.ConditionFalse,
+			Reason:  "TaskRunCancelled",
+			Message: `TaskRun "test-taskrun-run-cancelled" was cancelled`,
 		},
 	}, {
-		desc: "pod scheduled",
-		taskRun: &v1alpha1.TaskRun{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      taskRunName,
-				Namespace: namespace,
-			},
-			Spec: v1alpha1.TaskRunSpec{
-				Status: v1alpha1.TaskRunSpecStatusCancelled,
-			},
-		},
+		name: "pod-scheduled",
+		taskRun: tb.TaskRun("test-taskrun-run-cancelled", "foo", tb.TaskRunSpec(
+			tb.TaskRunTaskRef(simpleTask.Name),
+			tb.TaskRunCancelled,
+		), tb.TaskRunStatus(tb.StatusCondition(apis.Condition{
+			Type:   apis.ConditionSucceeded,
+			Status: corev1.ConditionUnknown,
+		}), tb.PodName("foo-is-bar"))),
 		pod: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{
-			Namespace: namespace,
-			Name:      "the-pod",
-			Labels: map[string]string{
-				"tekton.dev/taskRun": taskRunName,
-			},
+			Namespace: "foo",
+			Name:      "foo-is-bar",
 		}},
-	}} {
-		t.Run(c.desc, func(t *testing.T) {
+		expectedStatus: apis.Condition{
+			Type:    apis.ConditionSucceeded,
+			Status:  corev1.ConditionFalse,
+			Reason:  "TaskRunCancelled",
+			Message: `TaskRun "test-taskrun-run-cancelled" was cancelled`,
+		},
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
 			d := test.Data{
-				TaskRuns: []*v1alpha1.TaskRun{c.taskRun},
+				TaskRuns: []*v1alpha1.TaskRun{tc.taskRun},
 			}
-			if c.pod != nil {
-				d.Pods = []*corev1.Pod{c.pod}
+			if tc.pod != nil {
+				d.Pods = []*corev1.Pod{tc.pod}
 			}
 
-			testAssets, cancel := getTaskRunController(t, d)
+			ctx, _ := ttesting.SetupFakeContext(t)
+			ctx, cancel := context.WithCancel(ctx)
 			defer cancel()
-			if err := testAssets.Controller.Reconciler.Reconcile(context.Background(), getRunName(c.taskRun)); err != nil {
+			c, _ := test.SeedTestData(t, ctx, d)
+			observer, _ := observer.New(zap.InfoLevel)
+			err := cancelTaskRun(tc.taskRun, c.Kube, zap.New(observer).Sugar())
+			if err != nil {
 				t.Fatal(err)
 			}
-			if d := cmp.Diff(wantStatus, c.taskRun.Status.GetCondition(apis.ConditionSucceeded), ignoreLastTransitionTime); d != "" {
-				t.Errorf("Diff(-want, +got): %s", d)
-			}
-
-			if c.pod != nil {
-				if _, err := testAssets.Controller.Reconciler.(*Reconciler).KubeClientSet.CoreV1().Pods(c.taskRun.Namespace).Get(c.pod.Name, metav1.GetOptions{}); !kerrors.IsNotFound(err) {
-					t.Errorf("Pod was not deleted; wanted not-found error, got %v", err)
-				}
+			if d := cmp.Diff(tc.taskRun.Status.GetCondition(apis.ConditionSucceeded), &tc.expectedStatus, ignoreLastTransitionTime); d != "" {
+				t.Fatalf("-want, +got: %v", d)
 			}
 		})
 	}

pkg/reconciler/taskrun/metrics.go

@@ -120,7 +120,7 @@ func NewRecorder() (*Recorder, error) {
 	}
 	r.pod = pod
 
-	if err := view.Register(
+	err = view.Register(
 		&view.View{
 			Description: trDuration.Description(),
 			Measure:     trDuration,
@@ -150,7 +150,9 @@ func NewRecorder() (*Recorder, error) {
 			Aggregation: view.LastValue(),
 			TagKeys:     []tag.Key{r.task, r.taskRun, r.namespace, r.pod},
 		},
-	); err != nil {
+	)
+
+	if err != nil {
 		r.initialized = false
 		return r, err
 	}
@@ -255,15 +257,9 @@ func (r *Recorder) RecordPodLatency(pod *corev1.Pod, tr *v1alpha1.TaskRun) error
 		return errors.New("ignoring the metrics recording for pod , failed to initialize the metrics recorder")
 	}
 
-	var scheduledTime metav1.Time
-	for _, c := range pod.Status.Conditions {
-		if c.Type == corev1.PodScheduled {
-			scheduledTime = c.LastTransitionTime
-			break
-		}
-	}
+	scheduledTime := getScheduledTime(pod)
 	if scheduledTime.IsZero() {
-		return errors.New("pod was never scheduled")
+		return errors.New("pod has never got scheduled")
 	}
 
 	latency := scheduledTime.Sub(pod.CreationTimestamp.Time)
@@ -287,3 +283,13 @@ func (r *Recorder) RecordPodLatency(pod *corev1.Pod, tr *v1alpha1.TaskRun) error
 
 	return nil
 }
+
+func getScheduledTime(pod *corev1.Pod) metav1.Time {
+	for _, c := range pod.Status.Conditions {
+		if c.Type == corev1.PodScheduled {
+			return c.LastTransitionTime
+		}
+	}
+
+	return metav1.Time{}
+}