Update status and labels even on error

The TaskRun controller reconcile returns errors immediately.
It should instead first attempt to update status and labels and then
return both the original error plus any further error raised during
the update.

Fixes #1059

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -22,6 +22,7 @@ import (
 	"reflect"
 	"time"
 
+	"github.com/hashicorp/go-multierror"
 	"github.com/tektoncd/pipeline/pkg/apis/config"
 	apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
@@ -102,8 +103,14 @@ type Reconciler struct {
 	timeoutHandler    *reconciler.TimeoutSet
 }
 
-// Check that our Reconciler implements controller.Reconciler
-var _ controller.Reconciler = (*Reconciler)(nil)
+var (
+	// Check that our Reconciler implements controller.Reconciler
+	_ controller.Reconciler = (*Reconciler)(nil)
+
+	// In case of reconcile errors, we store the error in a multierror, attempt
+	// to update, and return the original error combined with any update error
+	merr error
+)
 
 // Reconcile compares the actual state with the desired, and attempts to
 // converge the two. It then updates the Status block of the Pipeline Run
@@ -167,7 +174,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 		// updates regardless of whether the reconciliation errored out.
 		if err = c.reconcile(ctx, pr); err != nil {
 			c.Logger.Errorf("Reconcile error: %v", err.Error())
-			return err
+			merr = multierror.Append(merr, err)
 		}
 	}
 
@@ -179,19 +186,19 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 	} else if _, err := c.updateStatus(pr); err != nil {
 		c.Logger.Warn("Failed to update PipelineRun status", zap.Error(err))
 		c.Recorder.Event(pr, corev1.EventTypeWarning, eventReasonFailed, "PipelineRun failed to update")
-		return err
+		return multierror.Append(merr, err)
 	}
 	// Since we are using the status subresource, it is not possible to update
 	// the status and labels/annotations simultaneously.
 	if !reflect.DeepEqual(original.ObjectMeta.Labels, pr.ObjectMeta.Labels) || !reflect.DeepEqual(original.ObjectMeta.Annotations, pr.ObjectMeta.Annotations) {
 		if _, err := c.updateLabelsAndAnnotations(pr); err != nil {
 			c.Logger.Warn("Failed to update PipelineRun labels/annotations", zap.Error(err))
 			c.Recorder.Event(pr, corev1.EventTypeWarning, eventReasonFailed, "PipelineRun failed to update labels/annotations")
-			return err
+			return multierror.Append(merr, err)
 		}
 	}
 
-	return err
+	return merr
 }
 
 func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) error {

pkg/reconciler/taskrun/taskrun.go

@@ -34,6 +34,7 @@ import (
 	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources/cloudevent"
 	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/sidecars"
 	"github.com/tektoncd/pipeline/pkg/status"
+
 	"go.uber.org/zap"
 	"golang.org/x/xerrors"
 	corev1 "k8s.io/api/core/v1"
@@ -80,6 +81,10 @@ var _ controller.Reconciler = (*Reconciler)(nil)
 // converge the two. It then updates the Status block of the Task Run
 // resource with the current status of the resource.
 func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
+	// In case of reconcile errors, we store the error in a multierror, attempt
+	// to update, and return the original error combined with any update error
+	var merr error
+
 	// Convert the namespace/name string into a distinct namespace and name
 	namespace, name, err := cache.SplitMetaNamespaceKey(key)
 	if err != nil {
@@ -140,13 +145,12 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 	// updates regardless of whether the reconciliation errored out.
 	if err := c.reconcile(ctx, tr); err != nil {
 		c.Logger.Errorf("Reconcile error: %v", err.Error())
-		return err
+		merr = multierror.Append(merr, err)
 	}
-	return c.updateStatusLabelsAndAnnotations(tr, original)
+	return multierror.Append(merr, c.updateStatusLabelsAndAnnotations(tr, original)).ErrorOrNil()
 }
 
 func (c *Reconciler) updateStatusLabelsAndAnnotations(tr, original *v1alpha1.TaskRun) error {
-	var err error
 	if equality.Semantic.DeepEqual(original.Status, tr.Status) {
 		// If we didn't change anything then don't call updateStatus.
 		// This is important because the copy we loaded from the informer's
@@ -164,7 +168,8 @@ func (c *Reconciler) updateStatusLabelsAndAnnotations(tr, original *v1alpha1.Tas
 			return err
 		}
 	}
-	return err
+
+	return nil
 }
 
 func (c *Reconciler) getTaskFunc(tr *v1alpha1.TaskRun) (resources.GetTask, v1alpha1.TaskKind) {

pkg/reconciler/taskrun/taskrun_test.go

@@ -1303,12 +1303,8 @@ func TestReconcilePodFetchError(t *testing.T) {
 	c := testAssets.Controller
 	clients := testAssets.Clients
 
-	clients.Kube.PrependReactor("*", "*", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
-		if action.GetVerb() == "get" && action.GetResource().Resource == "pods" {
-			// handled fetching pods
-			return true, nil, xerrors.New("induce failure fetching pods")
-		}
-		return false, nil, nil
+	clients.Kube.PrependReactor("get", "pods", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
+		return true, nil, xerrors.New("induce failure fetching pods")
 	})
 
 	if err := c.Reconciler.Reconcile(context.Background(), fmt.Sprintf("%s/%s", taskRun.Namespace, taskRun.Name)); err == nil {