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.
Events on reconcile error use a dedicated reason.

Author: afrittoli

pkg/reconciler/event.go

@@ -30,6 +30,8 @@ const (
 	EventReasonFailed = "Failed"
 	// EventReasonStarted is the reason set for events about the start of TaskRuns / PipelineRuns
 	EventReasonStarted = "Started"
+	// EventReasonError is the reason set for events related to TaskRuns / PipelineRuns reconcile errors
+	EventReasonError = "Error"
 )
 
 // EmitEvent emits an event for object if afterCondition is different from beforeCondition
@@ -63,3 +65,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, EventReasonError, err.Error())
+	}
+}

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 Error 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
+}

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

test/cancel_test.go

@@ -20,6 +20,7 @@ package test
 
 import (
 	"encoding/json"
+	"fmt"
 	"sync"
 	"testing"
 
@@ -171,14 +172,22 @@ func TestTaskRunPipelineRunCancel(t *testing.T) {
 			wg.Wait()
 
 			matchKinds := map[string][]string{"PipelineRun": {"pear"}, "TaskRun": trName}
+			// Expected failure events: 1 for the pipelinerun cancel, 1 for each TaskRun
 			expectedNumberOfEvents := 1 + len(trName)
 			t.Logf("Making sure %d events were created from pipelinerun with kinds %v", expectedNumberOfEvents, matchKinds)
 			events, err := collectMatchingEvents(c.KubeClient, namespace, matchKinds, "Failed")
 			if err != nil {
 				t.Fatalf("Failed to collect matching events: %q", err)
 			}
 			if len(events) != expectedNumberOfEvents {
-				t.Fatalf("Expected %d number of successful events from pipelinerun and taskrun but got %d; list of received events : %#v", expectedNumberOfEvents, len(events), events)
+				collectedEvents := ""
+				for i, event := range events {
+					collectedEvents += fmt.Sprintf("%#v", event)
+					if i < (len(events) - 1) {
+						collectedEvents += ", "
+					}
+				}
+				t.Fatalf("Expected %d number of successful events from pipelinerun and taskrun but got %d; list of received events : %#v", expectedNumberOfEvents, len(events), collectedEvents)
 			}
 		})
 	}