Don't rely on .status.podName to find Pod associated with a TaskRun

This adds Reconciler.getPod, which looks up the Pod for a TaskRun by performing a label selector query on Pods, looking for the label we apply to Pods generated by TaskRuns. If zero Pods are returned, it's the same as .status.podName being "". If multiple Pods are returned, that's an error. Also, clean up metrics_test.go a bit while I'm in that area
tektoncd · Dec 10, 2019 · da288e0 · da288e0
1 parent 0f9be6d
commit da288e0
Show file tree

Hide file tree

Showing 7 changed files with 302 additions and 417 deletions.
diff --git a/pkg/reconciler/taskrun/cancel.go b/pkg/reconciler/taskrun/cancel.go
@@ -22,32 +22,26 @@ import (
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes"
 	"knative.dev/pkg/apis"
 )
 
-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)
+func (c *Reconciler) cancelTaskRun(tr *v1alpha1.TaskRun) error {
+	c.Logger.Warnf("Cancelling TaskRun %q", tr.Name)
 	tr.Status.SetCondition(&apis.Condition{
 		Type:    apis.ConditionSucceeded,
 		Status:  corev1.ConditionFalse,
 		Reason:  "TaskRunCancelled",
 		Message: fmt.Sprintf("TaskRun %q was cancelled", tr.Name),
 	})
 
-	if tr.Status.PodName == "" {
-		logger.Warnf("task run %q has no pod running yet", tr.Name)
+	pod, err := c.getPod(tr)
+	if err == errNoPodForTaskRun {
+		c.Logger.Warnf("TaskRun %q has no Pod running yet", tr.Name)
 		return nil
-	}
-
-	if err := clientSet.CoreV1().Pods(tr.Namespace).Delete(tr.Status.PodName, &metav1.DeleteOptions{}); err != nil {
+	} else if err != nil {
 		return err
 	}
-	return nil
+
+	return c.KubeClientSet.CoreV1().Pods(tr.Namespace).Delete(pod.Name, &metav1.DeleteOptions{})
 }
diff --git a/pkg/reconciler/taskrun/cancel_test.go b/pkg/reconciler/taskrun/cancel_test.go
@@ -22,78 +22,77 @@ 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) {
-	testCases := []struct {
-		name           string
-		taskRun        *v1alpha1.TaskRun
-		pod            *corev1.Pod
-		expectedStatus apis.Condition
+	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
 	}{{
-		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: "no-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"))),
+		desc: "pod-scheduled",
+		taskRun: &v1alpha1.TaskRun{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      taskRunName,
+				Namespace: namespace,
+			},
+			Spec: v1alpha1.TaskRunSpec{
+				Status: v1alpha1.TaskRunSpecStatusCancelled,
+			},
+		},
 		pod: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{
-			Namespace: "foo",
-			Name:      "foo-is-bar",
+			Namespace: namespace,
+			Name:      "the-pod",
+			Labels: map[string]string{
+				"tekton.dev/taskRun": taskRunName,
+			},
 		}},
-		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) {
+	}} {
+		t.Run(c.desc, func(t *testing.T) {
 			d := test.Data{
-				TaskRuns: []*v1alpha1.TaskRun{tc.taskRun},
+				TaskRuns: []*v1alpha1.TaskRun{c.taskRun},
 			}
-			if tc.pod != nil {
-				d.Pods = []*corev1.Pod{tc.pod}
+			if c.pod != nil {
+				d.Pods = []*corev1.Pod{c.pod}
 			}
 
-			ctx, _ := ttesting.SetupFakeContext(t)
-			ctx, cancel := context.WithCancel(ctx)
+			testAssets, cancel := getTaskRunController(t, d)
 			defer cancel()
-			c, _ := test.SeedTestData(t, ctx, d)
-			observer, _ := observer.New(zap.InfoLevel)
-			err := cancelTaskRun(tc.taskRun, c.Kube, zap.New(observer).Sugar())
-			if err != nil {
+			if err := testAssets.Controller.Reconciler.Reconcile(context.Background(), getRunName(c.taskRun)); err != nil {
 				t.Fatal(err)
 			}
-			if d := cmp.Diff(tc.taskRun.Status.GetCondition(apis.ConditionSucceeded), &tc.expectedStatus, ignoreLastTransitionTime); d != "" {
-				t.Fatalf("-want, +got: %v", d)
+			if d := cmp.Diff(c.taskRun.Status.GetCondition(apis.ConditionSucceeded), wantStatus, 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)
+				}
 			}
 		})
 	}

diff --git a/pkg/reconciler/taskrun/metrics.go b/pkg/reconciler/taskrun/metrics.go
@@ -120,7 +120,7 @@ func NewRecorder() (*Recorder, error) {
 	}
 	r.pod = pod
 
-	err = view.Register(
+	if err := view.Register(
 		&view.View{
 			Description: trDuration.Description(),
 			Measure:     trDuration,
@@ -150,9 +150,7 @@ func NewRecorder() (*Recorder, error) {
 			Aggregation: view.LastValue(),
 			TagKeys:     []tag.Key{r.task, r.taskRun, r.namespace, r.pod},
 		},
-	)
-
-	if err != nil {
+	); err != nil {
 		r.initialized = false
 		return r, err
 	}
@@ -257,9 +255,15 @@ 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")
 	}
 
-	scheduledTime := getScheduledTime(pod)
+	var scheduledTime metav1.Time
+	for _, c := range pod.Status.Conditions {
+		if c.Type == corev1.PodScheduled {
+			scheduledTime = c.LastTransitionTime
+			break
+		}
+	}
 	if scheduledTime.IsZero() {
-		return errors.New("pod has never got scheduled")
+		return errors.New("pod was never scheduled")
 	}
 
 	latency := scheduledTime.Sub(pod.CreationTimestamp.Time)
@@ -283,13 +287,3 @@ 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{}
-}