From ad91dbf62f7be742fa9e8ec17baed2c45de481f7 Mon Sep 17 00:00:00 2001 From: Priti Desai Date: Thu, 6 Aug 2020 13:50:55 -0700 Subject: [PATCH] avoid requeuing taskrun in case of permanent error When a taskrun is rejected with permanent error, reconciler should not try to requeue the taskrun. After a permanent error is raised in prepare function call, reconciler enters the tr.IsDone block. In that block, sidecars were being terminated without any check on pod name. Such rejected taskrun has no pod name associated with it since a pod is never created. Reconciler fails to run such Get command and returns a normal error. Next reconciler cycle runs with this normal error instead of permanent error and tries to requeue the taskrun until reconciler exhausts the allowed retries. These changes are introduced to add a check if pod was created for a taskrun before cleaning up the sidecars. Most of the changes in this PR are introduced based on the pipelinerun.go --- pkg/apis/pipeline/v1beta1/taskrun_types.go | 2 + pkg/reconciler/taskrun/taskrun.go | 82 +++++++++++++--------- pkg/reconciler/taskrun/taskrun_test.go | 53 +++++++++++++- 3 files changed, 104 insertions(+), 33 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/taskrun_types.go b/pkg/apis/pipeline/v1beta1/taskrun_types.go index a314760bcef..d5fb6fe9b8a 100644 --- a/pkg/apis/pipeline/v1beta1/taskrun_types.go +++ b/pkg/apis/pipeline/v1beta1/taskrun_types.go @@ -156,6 +156,8 @@ func (trs *TaskRunStatus) MarkResourceFailed(reason TaskRunReason, err error) { Reason: reason.String(), Message: err.Error(), }) + succeeded := trs.GetCondition(apis.ConditionSucceeded) + trs.CompletionTime = &succeeded.LastTransitionTime.Inner } // TaskRunStatusFields holds the fields of TaskRun's status. This is defined diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 19c29f51b17..048c2962f08 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -24,6 +24,8 @@ import ( "strings" "time" + "go.uber.org/zap" + "github.com/ghodss/yaml" "github.com/hashicorp/go-multierror" "github.com/tektoncd/pipeline/pkg/apis/config" @@ -103,54 +105,46 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg // on the event to perform user facing initialisations, such has reset a CI check status afterCondition := tr.Status.GetCondition(apis.ConditionSucceeded) events.Emit(ctx, nil, afterCondition, tr) + + // We already sent an event for start, so update `before` with the current status + before = tr.Status.GetCondition(apis.ConditionSucceeded) } // If the TaskRun is complete, run some post run fixtures when applicable if tr.IsDone() { logger.Infof("taskrun done : %s \n", tr.Name) - var merr *multierror.Error + + // We may be reading a version of the object that was stored at an older version + // and may not have had all of the assumed default specified. + tr.SetDefaults(contexts.WithUpgradeViaDefaulting(ctx)) + // Try to send cloud events first cloudEventErr := cloudevent.SendCloudEvents(tr, c.cloudEventClient, logger) // Regardless of `err`, we must write back any status update that may have // been generated by `sendCloudEvents` - _, updateErr := c.updateLabelsAndAnnotations(tr) - merr = multierror.Append(cloudEventErr, updateErr) if cloudEventErr != nil { // Let's keep timeouts and sidecars running as long as we're trying to // send cloud events. So we stop here an return errors encountered this far. - return merr.ErrorOrNil() + return cloudEventErr } + c.timeoutHandler.Release(tr) - pod, err := c.KubeClientSet.CoreV1().Pods(tr.Namespace).Get(tr.Status.PodName, metav1.GetOptions{}) - if err == nil { - err = podconvert.StopSidecars(c.Images.NopImage, c.KubeClientSet, *pod) - if err == nil { - // Check if any SidecarStatuses are still shown as Running after stopping - // Sidecars. If any Running, update SidecarStatuses based on Pod ContainerStatuses. - if podconvert.IsSidecarStatusRunning(tr) { - err = updateStoppedSidecarStatus(ctx, pod, tr, c) - } - } - } else if k8serrors.IsNotFound(err) { - return merr.ErrorOrNil() - } - if err != nil { - logger.Errorf("Error stopping sidecars for TaskRun %q: %v", tr.Name, err) - merr = multierror.Append(merr, err) - } + + pod := c.stopSidecars(ctx, tr) go func(metrics *Recorder) { err := metrics.DurationAndCount(tr) if err != nil { logger.Warnf("Failed to log the metrics : %v", err) } - err = metrics.RecordPodLatency(pod, tr) - if err != nil { - logger.Warnf("Failed to log the metrics : %v", err) + if pod != nil { + err = metrics.RecordPodLatency(pod, tr) + if err != nil { + logger.Warnf("Failed to log the metrics : %v", err) + } } }(c.metrics) - - return merr.ErrorOrNil() + return c.finishReconcileUpdateEmitEvents(ctx, tr, before, nil) } // If the TaskRun is cancelled, kill resources and update status @@ -176,12 +170,9 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg logger.Errorf("TaskRun prepare error: %v", err.Error()) // We only return an error if update failed, otherwise we don't want to // reconcile an invalid TaskRun anymore - return c.finishReconcileUpdateEmitEvents(ctx, tr, nil, err) + return c.finishReconcileUpdateEmitEvents(ctx, tr, before, err) } - // Store the condition before reconcile - before = tr.Status.GetCondition(apis.ConditionSucceeded) - // Reconcile this copy of the task run and then write back any status // updates regardless of whether the reconciliation errored out. if err = c.reconcile(ctx, tr, taskSpec, rtr); err != nil { @@ -190,8 +181,32 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, tr *v1beta1.TaskRun) pkg // Emit events (only when ConditionSucceeded was changed) return c.finishReconcileUpdateEmitEvents(ctx, tr, before, err) } +func (c *Reconciler) stopSidecars(ctx context.Context, tr *v1beta1.TaskRun) *corev1.Pod { + logger := logging.FromContext(ctx) + if tr.Status.PodName == "" { + return nil + } + pod, err := c.KubeClientSet.CoreV1().Pods(tr.Namespace).Get(tr.Status.PodName, metav1.GetOptions{}) + if err == nil { + err = podconvert.StopSidecars(c.Images.NopImage, c.KubeClientSet, *pod) + if err == nil { + // Check if any SidecarStatuses are still shown as Running after stopping + // Sidecars. If any Running, update SidecarStatuses based on Pod ContainerStatuses. + if podconvert.IsSidecarStatusRunning(tr) { + err = updateStoppedSidecarStatus(ctx, pod, tr, c) + } + } + } + if err != nil { + logger.Errorf("Error stopping sidecars for TaskRun %q: %v", tr.Name, err) + tr.Status.MarkResourceFailed(podconvert.ReasonFailedResolution, err) + } + return pod +} func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, tr *v1beta1.TaskRun, beforeCondition *apis.Condition, previousError error) error { + logger := logging.FromContext(ctx) + afterCondition := tr.Status.GetCondition(apis.ConditionSucceeded) // Send k8s events and cloud events (when configured) @@ -199,12 +214,15 @@ func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, tr *v1 _, err := c.updateLabelsAndAnnotations(tr) if err != nil { + logger.Warn("Failed to update PipelineRun labels/annotations", zap.Error(err)) events.EmitError(controller.GetEventRecorder(ctx), err, tr) } + + merr := multierror.Append(previousError, err).ErrorOrNil() if controller.IsPermanentError(previousError) { - return controller.NewPermanentError(multierror.Append(previousError, err)) + return controller.NewPermanentError(merr) } - return multierror.Append(previousError, err).ErrorOrNil() + return merr } func (c *Reconciler) getTaskResolver(tr *v1beta1.TaskRun) (*resources.LocalTaskRefResolver, v1beta1.TaskKind) { diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 7ad27d2f3f3..2f5a21a3f1f 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -39,7 +39,7 @@ import ( "github.com/tektoncd/pipeline/pkg/system" "github.com/tektoncd/pipeline/pkg/timeout" "github.com/tektoncd/pipeline/pkg/workspace" - test "github.com/tektoncd/pipeline/test" + "github.com/tektoncd/pipeline/test" "github.com/tektoncd/pipeline/test/diff" "github.com/tektoncd/pipeline/test/names" corev1 "k8s.io/api/core/v1" @@ -1733,6 +1733,57 @@ func TestReconcileInvalidTaskRuns(t *testing.T) { } +func TestReconcileTaskRunWithPermanentError(t *testing.T) { + noTaskRun := tb.TaskRun("notaskrun", tb.TaskRunNamespace("foo"), tb.TaskRunSpec(tb.TaskRunTaskRef("notask")), + tb.TaskRunStatus(tb.TaskRunStartTime(time.Now()), + tb.StatusCondition(apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: podconvert.ReasonFailedResolution, + Message: "error when listing tasks for taskRun taskrun-failure: tasks.tekton.dev \"notask\" not found", + }))) + + taskRuns := []*v1beta1.TaskRun{noTaskRun} + + d := test.Data{ + TaskRuns: taskRuns, + } + + testAssets, cancel := getTaskRunController(t, d) + defer cancel() + c := testAssets.Controller + clients := testAssets.Clients + reconcileErr := c.Reconciler.Reconcile(context.Background(), getRunName(noTaskRun)) + + // When a TaskRun was rejected with a permanent error, reconciler must stop and forget about the run + // Such TaskRun enters Reconciler and from within the isDone block, marks the run success so that + // reconciler does not keep trying to reconcile + if reconcileErr != nil { + t.Fatalf("Expected to see no error when reconciling TaskRun with Permanent Error but was not none") + } + + // Check actions + actions := clients.Kube.Actions() + if len(actions) != 3 || actions[0].Matches("namespaces", "list") { + t.Errorf("expected 3 actions (list namespaces, list configmaps, and watch configmaps) created by the reconciler,"+ + " got %d. Actions: %#v", len(actions), actions) + } + + newTr, err := clients.Pipeline.TektonV1beta1().TaskRuns(noTaskRun.Namespace).Get(noTaskRun.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Expected TaskRun %s to exist but instead got error when getting it: %v", noTaskRun.Name, err) + } + + // Since the TaskRun is invalid, the status should say it has failed + condition := newTr.Status.GetCondition(apis.ConditionSucceeded) + if condition == nil || condition.Status != corev1.ConditionFalse { + t.Errorf("Expected invalid TaskRun to have failed status, but had %v", condition) + } + if condition != nil && condition.Reason != podconvert.ReasonFailedResolution { + t.Errorf("Expected failure to be because of reason %q but was %s", podconvert.ReasonFailedResolution, condition.Reason) + } +} + func TestReconcilePodFetchError(t *testing.T) { taskRun := tb.TaskRun("test-taskrun-run-success", tb.TaskRunNamespace("foo"),