Skip to content

Commit

Permalink
Use permanent errors in the pipelinerun reconciler
Browse files Browse the repository at this point in the history 
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 tektoncd#2474
  • Loading branch information
@afrittoli
afrittoli committed Jun 23, 2020
1 parent 8d9aa37 commit 190ad5f
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 49 deletions.
39 changes: 22 additions & 17 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -285,39 +290,39 @@ 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 {
// This Run has failed, so we need to mark it as failed and stop reconciling it
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 {
// This Run has failed, so we need to mark it as failed and stop reconciling it
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 {
// This Run has failed, so we need to mark it as failed and stop reconciling it
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 {
// This Run has failed, so we need to mark it as failed and stop reconciling it
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.
Expand All @@ -328,23 +333,23 @@ 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.
if err := resources.ValidateWorkspaceBindings(pipelineSpec, pr); err != nil {
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.
if err := resources.ValidateServiceaccountMapping(pipelineSpec, pr); err != nil {
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
Expand Down Expand Up @@ -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() {
Expand All @@ -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)
}
}

Expand All @@ -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)
}
}

Expand All @@ -420,15 +425,15 @@ 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)
}
}
}

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
Expand Down Expand Up @@ -469,15 +474,15 @@ 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)
resolvedResultRefs, err := resources.ResolveResultRefs(pipelineState, nextRprts)
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)

Expand Down
82 changes: 50 additions & 32 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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,
permanentError: true,
}, {
name: "invalid-pipeline-run-missing-params-shd-stop-reconciling",
pipelineRun: prs[11],
reason: ReasonParameterMissing,
permanentError: true,
},
}

Expand All @@ -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 {
Expand Down

0 comments on commit 190ad5f

Please sign in to comment.