From 2d8ed3712d56d4c082347d0fe4bdc70d52834523 Mon Sep 17 00:00:00 2001 From: Andrea Frittoli Date: Tue, 9 Jun 2020 22:21:23 +0100 Subject: [PATCH] Use permanent errors in the pipelinerun reconciler Instead of returning nil on error during reconcile, if the error is transient return it, so that the key is requeue. If the error is permanent return a permanent error, so that the key is not requeued. Partially addresses #2474 --- pkg/reconciler/pipelinerun/pipelinerun.go | 39 ++++---- .../pipelinerun/pipelinerun_test.go | 94 +++++++++++-------- 2 files changed, 78 insertions(+), 55 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 2d0ab0cf564..0c861b9cf0e 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -214,7 +214,11 @@ func (c *Reconciler) finishReconcileUpdateEmitEvents(ctx context.Context, pr *v1 events.EmitError(recorder, err, pr) } - return multierror.Append(previousError, err).ErrorOrNil() + merr := multierror.Append(previousError, err).ErrorOrNil() + if controller.IsPermanentError(previousError) { + return controller.NewPermanentError(merr) + } + return merr } func (c *Reconciler) updatePipelineResults(ctx context.Context, pr *v1beta1.PipelineRun) { @@ -236,6 +240,7 @@ func (c *Reconciler) updatePipelineResults(ctx context.Context, pr *v1beta1.Pipe resolvedResultRefs := resources.ResolvePipelineResultRefs(pr.Status, pipelineSpec.Results) pr.Status.PipelineResults = getPipelineRunResults(pipelineSpec, resolvedResultRefs) } + func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) error { logger := logging.FromContext(ctx) recorder := controller.GetEventRecorder(ctx) @@ -254,7 +259,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonCouldntGetPipeline, "Error retrieving pipeline for pipelinerun %s/%s: %s", pr.Namespace, pr.Name, err) - return nil + return controller.NewPermanentError(err) } // Store the fetched PipelineSpec on the PipelineRun for auditing @@ -285,7 +290,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonInvalidGraph, "PipelineRun %s/%s's Pipeline DAG is invalid: %s", pr.Namespace, pr.Name, err) - return nil + return controller.NewPermanentError(err) } if err := pipelineSpec.Validate(ctx); err != nil { @@ -293,7 +298,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonFailedValidation, "Pipeline %s/%s can't be Run; it has an invalid spec: %s", pipelineMeta.Namespace, pipelineMeta.Name, err) - return nil + return controller.NewPermanentError(err) } if err := resources.ValidateResourceBindings(pipelineSpec, pr); err != nil { @@ -301,7 +306,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonInvalidBindings, "PipelineRun %s/%s doesn't bind Pipeline %s/%s's PipelineResources correctly: %s", pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err) - return nil + return controller.NewPermanentError(err) } providedResources, err := resources.GetResourcesFromBindings(pr, c.resourceLister.PipelineResources(pr.Namespace).Get) if err != nil { @@ -309,7 +314,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonCouldntGetResource, "PipelineRun %s/%s can't be Run; it tries to bind Resources that don't exist: %s", pipelineMeta.Namespace, pr.Name, err) - return nil + return controller.NewPermanentError(err) } // Ensure that the PipelineRun provides all the parameters required by the Pipeline if err := resources.ValidateRequiredParametersProvided(&pipelineSpec.Params, &pr.Spec.Params); err != nil { @@ -317,7 +322,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonParameterMissing, "PipelineRun %s parameters is missing some parameters required by Pipeline %s's parameters: %s", pr.Namespace, pr.Name, err) - return nil + return controller.NewPermanentError(err) } // Ensure that the parameters from the PipelineRun are overriding Pipeline parameters with the same type. @@ -328,7 +333,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonParameterTypeMismatch, "PipelineRun %s/%s parameters have mismatching types with Pipeline %s/%s's parameters: %s", pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err) - return nil + return controller.NewPermanentError(err) } // Ensure that the workspaces expected by the Pipeline are provided by the PipelineRun. @@ -336,7 +341,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonInvalidWorkspaceBinding, "PipelineRun %s/%s doesn't bind Pipeline %s/%s's Workspaces correctly: %s", pr.Namespace, pr.Name, pr.Namespace, pipelineMeta.Name, err) - return nil + return controller.NewPermanentError(err) } // Ensure that the ServiceAccountNames defined correct. @@ -344,7 +349,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonInvalidServiceAccountMapping, "PipelineRun %s/%s doesn't define ServiceAccountNames correctly: %s", pr.Namespace, pr.Name, err) - return nil + return controller.NewPermanentError(err) } // Apply parameter substitution from the PipelineRun @@ -383,7 +388,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err "PipelineRun %s/%s can't be Run; couldn't resolve all references: %s", pipelineMeta.Namespace, pr.Name, err) } - return nil + return controller.NewPermanentError(err) } if pipelineState.IsDone() && pr.IsDone() { @@ -397,7 +402,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err if err != nil { logger.Errorf("Failed to validate pipelinerun %q with error %v", pr.Name, err) pr.Status.MarkFailed(ReasonFailedValidation, err.Error()) - return nil + return controller.NewPermanentError(err) } } @@ -409,7 +414,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC, "Failed to create PVC for PipelineRun %s/%s Workspaces correctly: %s", pr.Namespace, pr.Name, err) - return nil + return controller.NewPermanentError(err) } } @@ -420,7 +425,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err pr.Status.MarkFailed(ReasonCouldntCreateAffinityAssistantStatefulSet, "Failed to create StatefulSet for PipelineRun %s/%s correctly: %s", pr.Namespace, pr.Name, err) - return nil + return controller.NewPermanentError(err) } } } @@ -428,7 +433,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err as, err := artifacts.InitializeArtifactStorage(c.Images, pr, pipelineSpec, c.KubeClientSet, logger) if err != nil { logger.Infof("PipelineRun failed to initialize artifact storage %s", pr.Name) - return err + return controller.NewPermanentError(err) } // When the pipeline run is stopping, we don't schedule any new task and only @@ -469,7 +474,7 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip candidateTasks, err := dag.GetSchedulable(d, pipelineState.SuccessfulPipelineTaskNames()...) if err != nil { logger.Errorf("Error getting potential next tasks for valid pipelinerun %s: %v", pr.Name, err) - return nil + return controller.NewPermanentError(err) } nextRprts := pipelineState.GetNextTasks(candidateTasks) @@ -477,7 +482,7 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip if err != nil { logger.Infof("Failed to resolve all task params for %q with error %v", pr.Name, err) pr.Status.MarkFailed(ReasonFailedValidation, err.Error()) - return nil + return controller.NewPermanentError(err) } resources.ApplyTaskResults(nextRprts, resolvedResultRefs) diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 41a6cf03d86..b1e2160c171 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -388,6 +388,8 @@ func TestReconcile_PipelineSpecTaskSpec(t *testing.T) { } func TestReconcile_InvalidPipelineRuns(t *testing.T) { + // TestReconcile_InvalidPipelineRuns runs "Reconcile" on several PipelineRuns that are invalid in different ways. + // It verifies that reconcile fails, how it fails and which events are triggered. ts := []*v1beta1.Task{ tb.Task("a-task-that-exists", tb.TaskNamespace("foo")), tb.Task("a-task-that-needs-params", tb.TaskSpec( @@ -455,56 +457,69 @@ func TestReconcile_InvalidPipelineRuns(t *testing.T) { pipelineRun *v1beta1.PipelineRun reason string hasNoDefaultLabels bool + permanentError bool }{ { name: "invalid-pipeline-shd-be-stop-reconciling", pipelineRun: prs[0], reason: ReasonCouldntGetPipeline, hasNoDefaultLabels: true, + permanentError: true, }, { - name: "invalid-pipeline-run-missing-tasks-shd-stop-reconciling", - pipelineRun: prs[1], - reason: ReasonCouldntGetTask, + name: "invalid-pipeline-run-missing-tasks-shd-stop-reconciling", + pipelineRun: prs[1], + reason: ReasonCouldntGetTask, + permanentError: true, }, { - name: "invalid-pipeline-run-params-dont-exist-shd-stop-reconciling", - pipelineRun: prs[2], - reason: ReasonFailedValidation, + name: "invalid-pipeline-run-params-dont-exist-shd-stop-reconciling", + pipelineRun: prs[2], + reason: ReasonFailedValidation, + permanentError: true, }, { - name: "invalid-pipeline-run-resources-not-bound-shd-stop-reconciling", - pipelineRun: prs[3], - reason: ReasonInvalidBindings, + name: "invalid-pipeline-run-resources-not-bound-shd-stop-reconciling", + pipelineRun: prs[3], + reason: ReasonInvalidBindings, + permanentError: true, }, { - name: "invalid-pipeline-run-missing-resource-shd-stop-reconciling", - pipelineRun: prs[4], - reason: ReasonCouldntGetResource, + name: "invalid-pipeline-run-missing-resource-shd-stop-reconciling", + pipelineRun: prs[4], + reason: ReasonCouldntGetResource, + permanentError: true, }, { - name: "invalid-pipeline-missing-declared-resource-shd-stop-reconciling", - pipelineRun: prs[5], - reason: ReasonFailedValidation, + name: "invalid-pipeline-missing-declared-resource-shd-stop-reconciling", + pipelineRun: prs[5], + reason: ReasonFailedValidation, + permanentError: true, }, { - name: "invalid-pipeline-mismatching-parameter-types", - pipelineRun: prs[6], - reason: ReasonParameterTypeMismatch, + name: "invalid-pipeline-mismatching-parameter-types", + pipelineRun: prs[6], + reason: ReasonParameterTypeMismatch, + permanentError: true, }, { - name: "invalid-pipeline-missing-conditions-shd-stop-reconciling", - pipelineRun: prs[7], - reason: ReasonCouldntGetCondition, + name: "invalid-pipeline-missing-conditions-shd-stop-reconciling", + pipelineRun: prs[7], + reason: ReasonCouldntGetCondition, + permanentError: true, }, { - name: "invalid-embedded-pipeline-resources-bot-bound-shd-stop-reconciling", - pipelineRun: prs[8], - reason: ReasonInvalidBindings, + name: "invalid-embedded-pipeline-resources-bot-bound-shd-stop-reconciling", + pipelineRun: prs[8], + reason: ReasonInvalidBindings, + permanentError: true, }, { - name: "invalid-embedded-pipeline-bad-name-shd-stop-reconciling", - pipelineRun: prs[9], - reason: ReasonFailedValidation, + name: "invalid-embedded-pipeline-bad-name-shd-stop-reconciling", + pipelineRun: prs[9], + reason: ReasonFailedValidation, + permanentError: true, }, { - name: "invalid-embedded-pipeline-mismatching-parameter-types", - pipelineRun: prs[10], - reason: ReasonParameterTypeMismatch, + name: "invalid-embedded-pipeline-mismatching-parameter-types", + pipelineRun: prs[10], + reason: ReasonParameterTypeMismatch, + permanentError: true, }, { - name: "invalid-pipeline-run-missing-params-shd-stop-reconciling", - pipelineRun: prs[11], - reason: ReasonParameterMissing, + name: "invalid-pipeline-run-missing-params-shd-stop-reconciling", + pipelineRun: prs[11], + reason: ReasonParameterMissing, + permanentError: true, }, } @@ -514,12 +529,15 @@ func TestReconcile_InvalidPipelineRuns(t *testing.T) { defer cancel() c := testAssets.Controller - if err := c.Reconciler.Reconcile(context.Background(), getRunName(tc.pipelineRun)); err != nil { - t.Fatalf("Error reconciling: %s", err) + // When a PipelineRun is invalid and can't run, we expect a permanent error that will + // tell the Reconciler to not keep trying to reconcile. + reconcileError := c.Reconciler.Reconcile(context.Background(), getRunName(tc.pipelineRun)) + if reconcileError == nil { + t.Fatalf("Expected an error to be returned by Reconcile, got nil instead") + } + if controller.IsPermanentError(reconcileError) != tc.permanentError { + t.Fatalf("Expected the error to be permanent: %v but got permanent: %v", tc.permanentError, controller.IsPermanentError(reconcileError)) } - // When a PipelineRun is invalid and can't run, we don't want to return an error because - // an error will tell the Reconciler to keep trying to reconcile; instead we want to stop - // and forget about the Run. reconciledRun, err := testAssets.Clients.Pipeline.TektonV1beta1().PipelineRuns(tc.pipelineRun.Namespace).Get(tc.pipelineRun.Name, metav1.GetOptions{}) if err != nil {