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 4bb17316c66..64906567f99 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -387,6 +387,8 @@ func TestReconcile_PipelineSpecTaskSpec(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. func TestReconcile_InvalidPipelineRuns(t *testing.T) { ts := []*v1beta1.Task{ tb.Task("a-task-that-exists", tb.TaskNamespace("foo")), @@ -458,60 +460,74 @@ 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, }, { - name: "invalid-pipeline-with-invalid-dag-graph", - pipelineRun: prs[12], - reason: ReasonInvalidGraph, + name: "invalid-pipeline-with-invalid-dag-graph", + pipelineRun: prs[12], + reason: ReasonInvalidGraph, + permanentError: true, }, } @@ -521,12 +537,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 {