diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 963469363d0..1210e9b0cc2 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -51,6 +51,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "knative.dev/pkg/apis" "knative.dev/pkg/configmap" + "knative.dev/pkg/controller" pkgreconciler "knative.dev/pkg/reconciler" "knative.dev/pkg/tracker" ) @@ -147,6 +148,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1beta1.PipelineRun) // 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 *multierror.Error + var reconcileErr error switch { case pr.IsDone(): @@ -197,9 +199,9 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1beta1.PipelineRun) // Reconcile this copy of the pipelinerun and then write back any status or label // updates regardless of whether the reconciliation errored out. - if err = c.reconcile(ctx, pr); err != nil { - c.Logger.Errorf("Reconcile error: %v", err.Error()) - merr = multierror.Append(merr, err) + if reconcileErr = c.reconcile(ctx, pr); reconcileErr != nil { + c.Logger.Errorf("Reconcile error: %v", reconcileErr.Error()) + merr = multierror.Append(merr, reconcileErr) } } @@ -213,6 +215,9 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, pr *v1beta1.PipelineRun) } } + if controller.IsPermanentError(reconcileErr) { + return controller.NewPermanentError(merr) + } return merr.ErrorOrNil() } @@ -252,13 +257,13 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err if err != nil { if ce, ok := err.(*v1beta1.CannotConvertError); ok { pr.Status.MarkResourceNotConvertible(ce) - return nil + return controller.NewPermanentError(err) } c.Logger.Errorf("Failed to determine Pipeline spec to use for pipelinerun %s: %v", pr.Name, 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 @@ -289,7 +294,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 { @@ -297,7 +302,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 { @@ -305,7 +310,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 { @@ -313,7 +318,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 parameters from the PipelineRun are overriding Pipeline parameters with the same type. @@ -324,7 +329,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. @@ -332,7 +337,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. @@ -340,7 +345,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 @@ -379,7 +384,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() { @@ -393,7 +398,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err if err != nil { c.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) } } @@ -405,7 +410,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) } } @@ -416,7 +421,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) } } } @@ -431,7 +436,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err if err != nil { c.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) @@ -439,7 +444,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err if as, err = artifacts.InitializeArtifactStorage(c.Images, pr, pipelineSpec, c.KubeClientSet, c.Logger); err != nil { c.Logger.Infof("PipelineRun failed to initialize artifact storage %s", pr.Name) - return err + return controller.NewPermanentError(err) } for _, rprt := range nextRprts { diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index cbe61f7e9bb..8c37777172a 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( @@ -451,52 +453,64 @@ 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, }, } @@ -506,12 +520,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 {