Emit events when we fail to update the taskrun

Similarly to how the pipeline controller does, we should emit events when we fail to update the taskrun.
tektoncd · May 2, 2020 · a468b43 · a468b43
1 parent 7bbc7eb
commit a468b43
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 54 deletions.
diff --git a/pkg/reconciler/event.go b/pkg/reconciler/event.go
@@ -63,3 +63,10 @@ func EmitEvent(c record.EventRecorder, beforeCondition *apis.Condition, afterCon
 		}
 	}
 }
+
+// EmitErrorEvent emits a failure associated to an error
+func EmitErrorEvent(c record.EventRecorder, err error, object runtime.Object) {
+	if err != nil {
+		c.Event(object, corev1.EventTypeWarning, EventReasonFailed, err.Error())
+	}
+}
diff --git a/pkg/reconciler/event_test.go b/pkg/reconciler/event_test.go
@@ -17,6 +17,9 @@ limitations under the License.
 package reconciler
 
 import (
+	"errors"
+	"fmt"
+	"strings"
 	"testing"
 	"time"
 
@@ -28,11 +31,10 @@ import (
 
 func TestEmitEvent(t *testing.T) {
 	testcases := []struct {
-		name          string
-		before        *apis.Condition
-		after         *apis.Condition
-		expectEvent   bool
-		expectedEvent string
+		name      string
+		before    *apis.Condition
+		after     *apis.Condition
+		wantEvent string
 	}{{
 		name: "unknown to true with message",
 		before: &apis.Condition{
@@ -44,8 +46,7 @@ func TestEmitEvent(t *testing.T) {
 			Status:  corev1.ConditionTrue,
 			Message: "all done",
 		},
-		expectEvent:   true,
-		expectedEvent: "Normal Succeeded all done",
+		wantEvent: "Normal Succeeded all done",
 	}, {
 		name: "true to true",
 		before: &apis.Condition{
@@ -58,8 +59,7 @@ func TestEmitEvent(t *testing.T) {
 			Status:             corev1.ConditionTrue,
 			LastTransitionTime: apis.VolatileTime{Inner: metav1.NewTime(time.Now().Add(5 * time.Minute))},
 		},
-		expectEvent:   false,
-		expectedEvent: "",
+		wantEvent: "",
 	}, {
 		name: "false to false",
 		before: &apis.Condition{
@@ -70,8 +70,7 @@ func TestEmitEvent(t *testing.T) {
 			Type:   apis.ConditionSucceeded,
 			Status: corev1.ConditionFalse,
 		},
-		expectEvent:   false,
-		expectedEvent: "",
+		wantEvent: "",
 	}, {
 		name: "unknown to unknown",
 		before: &apis.Condition{
@@ -86,26 +85,23 @@ func TestEmitEvent(t *testing.T) {
 			Reason:  "foo",
 			Message: "bar",
 		},
-		expectEvent:   true,
-		expectedEvent: "Normal foo bar",
+		wantEvent: "Normal foo bar",
 	}, {
 		name:  "true to nil",
 		after: nil,
 		before: &apis.Condition{
 			Type:   apis.ConditionSucceeded,
 			Status: corev1.ConditionTrue,
 		},
-		expectEvent:   false,
-		expectedEvent: "",
+		wantEvent: "",
 	}, {
 		name:   "nil to true",
 		before: nil,
 		after: &apis.Condition{
 			Type:   apis.ConditionSucceeded,
 			Status: corev1.ConditionTrue,
 		},
-		expectEvent:   true,
-		expectedEvent: "Normal Succeeded ",
+		wantEvent: "Normal Succeeded ",
 	}, {
 		name:   "nil to unknown with message",
 		before: nil,
@@ -114,8 +110,7 @@ func TestEmitEvent(t *testing.T) {
 			Status:  corev1.ConditionUnknown,
 			Message: "just starting",
 		},
-		expectEvent:   true,
-		expectedEvent: "Normal Started ",
+		wantEvent: "Normal Started ",
 	}, {
 		name: "unknown to false with message",
 		before: &apis.Condition{
@@ -127,43 +122,70 @@ func TestEmitEvent(t *testing.T) {
 			Status:  corev1.ConditionFalse,
 			Message: "really bad",
 		},
-		expectEvent:   true,
-		expectedEvent: "Warning Failed really bad",
+		wantEvent: "Warning Failed really bad",
 	}, {
 		name:   "nil to false",
 		before: nil,
 		after: &apis.Condition{
 			Type:   apis.ConditionSucceeded,
 			Status: corev1.ConditionFalse,
 		},
-		expectEvent:   true,
-		expectedEvent: "Warning Failed ",
+		wantEvent: "Warning Failed ",
 	}}
 
 	for _, ts := range testcases {
 		fr := record.NewFakeRecorder(1)
 		tr := &corev1.Pod{}
 		EmitEvent(fr, ts.before, ts.after, tr)
-		timer := time.NewTimer(1 * time.Second)
 
-		select {
-		case event := <-fr.Events:
-			if event == "" {
-				// The fake recorder reported empty, it should not happen
-				t.Fatalf("Expected event but got empty for %s", ts.name)
-			}
-			if !ts.expectEvent {
-				// The fake recorder reported an event which we did not expect
-				t.Errorf("Unxpected event \"%s\" but got one for %s", event, ts.name)
-			}
-			if !(event == ts.expectedEvent) {
-				t.Errorf("Expected event \"%s\" but got \"%s\" instead for %s", ts.expectedEvent, event, ts.name)
-			}
-		case <-timer.C:
-			if ts.expectEvent {
-				// The fake recorder did not report, the timer timeout expired
-				t.Errorf("Expected event but got none for %s", ts.name)
-			}
+		err := checkEvents(fr, ts.name, ts.wantEvent)
+		if err != nil {
+			t.Errorf(err.Error())
 		}
 	}
 }
+
+func TestEmitErrorEvent(t *testing.T) {
+	testcases := []struct {
+		name      string
+		err       error
+		wantEvent string
+	}{{
+		name:      "with error",
+		err:       errors.New("something went wrong"),
+		wantEvent: "Warning Failed something went wrong",
+	}, {
+		name:      "without error",
+		err:       nil,
+		wantEvent: "",
+	}}
+
+	for _, ts := range testcases {
+		fr := record.NewFakeRecorder(1)
+		tr := &corev1.Pod{}
+		EmitErrorEvent(fr, ts.err, tr)
+
+		err := checkEvents(fr, ts.name, ts.wantEvent)
+		if err != nil {
+			t.Errorf(err.Error())
+		}
+	}
+}
+
+func checkEvents(fr *record.FakeRecorder, testName string, wantEvent string) error {
+	timer := time.NewTimer(1 * time.Second)
+	select {
+	case event := <-fr.Events:
+		if wantEvent == "" {
+			return fmt.Errorf("received event \"%s\" for %s but none expected", event, testName)
+		}
+		if !(strings.HasPrefix(event, wantEvent)) {
+			return fmt.Errorf("expected event \"%s\" but got \"%s\" instead for %s", wantEvent, event, testName)
+		}
+	case <-timer.C:
+		if wantEvent != "" {
+			return fmt.Errorf("received no events for %s but %s expected", testName, wantEvent)
+		}
+	}
+	return nil
+}
diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go
@@ -168,9 +168,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 		before := tr.Status.GetCondition(apis.ConditionSucceeded)
 		message := fmt.Sprintf("TaskRun %q was cancelled", tr.Name)
 		err := c.failTaskRun(tr, v1beta1.TaskRunReasonCancelled, message)
-		after := tr.Status.GetCondition(apis.ConditionSucceeded)
-		reconciler.EmitEvent(c.Recorder, before, after, tr)
-		return multierror.Append(err, c.updateStatusLabelsAndAnnotations(tr, original)).ErrorOrNil()
+		return c.finishReconcileUpdateEmitEvents(tr, original, before, err)
 	}
 
 	// Check if the TaskRun has timed out; if it is, this will set its status
@@ -179,9 +177,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 		before := tr.Status.GetCondition(apis.ConditionSucceeded)
 		message := fmt.Sprintf("TaskRun %q failed to finish within %q", tr.Name, tr.GetTimeout())
 		err := c.failTaskRun(tr, podconvert.ReasonTimedOut, message)
-		after := tr.Status.GetCondition(apis.ConditionSucceeded)
-		reconciler.EmitEvent(c.Recorder, before, after, tr)
-		return multierror.Append(err, c.updateStatusLabelsAndAnnotations(tr, original)).ErrorOrNil()
+		return c.finishReconcileUpdateEmitEvents(tr, original, before, err)
 	}
 
 	// prepare fetches all required resources, validates them together with the
@@ -190,11 +186,9 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 	taskSpec, rtr, err := c.prepare(ctx, tr)
 	if err != nil {
 		c.Logger.Errorf("TaskRun prepare error: %v", err.Error())
-		after := tr.Status.GetCondition(apis.ConditionSucceeded)
-		reconciler.EmitEvent(c.Recorder, nil, after, tr)
 		// We only return an error if update failed, otherwise we don't want to
 		// reconcile an invalid TaskRun anymore
-		return c.updateStatusLabelsAndAnnotations(tr, original)
+		return c.finishReconcileUpdateEmitEvents(tr, original, nil, nil)
 	}
 
 	// Store the condition before reconcile
@@ -206,10 +200,15 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 		c.Logger.Errorf("Reconcile error: %v", err.Error())
 	}
 	// Emit events (only when ConditionSucceeded was changed)
-	after := tr.Status.GetCondition(apis.ConditionSucceeded)
-	reconciler.EmitEvent(c.Recorder, before, after, tr)
+	return c.finishReconcileUpdateEmitEvents(tr, original, before, err)
+}
 
-	return multierror.Append(err, c.updateStatusLabelsAndAnnotations(tr, original)).ErrorOrNil()
+func (c *Reconciler) finishReconcileUpdateEmitEvents(tr, original *v1alpha1.TaskRun, beforeCondition *apis.Condition, previousError error) error {
+	afterCondition := tr.Status.GetCondition(apis.ConditionSucceeded)
+	reconciler.EmitEvent(c.Recorder, beforeCondition, afterCondition, tr)
+	err := c.updateStatusLabelsAndAnnotations(tr, original)
+	reconciler.EmitErrorEvent(c.Recorder, err, tr)
+	return multierror.Append(previousError, err).ErrorOrNil()
 }
 
 func (c *Reconciler) getTaskResolver(tr *v1alpha1.TaskRun) (*resources.LocalTaskRefResolver, v1alpha1.TaskKind) {