Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use permanent errors in the pipelinerun reconciler #2798

Merged
merged 1 commit into from
Jun 25, 2020
Merged

Use permanent errors in the pipelinerun reconciler #2798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 22 additions & 17 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Open in desktop
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)
afrittoli marked this conversation as resolved.
Show resolved Hide resolved
}

// 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
101 changes: 60 additions & 41 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")),
Expand Down Expand Up @@ -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,
},
}

Expand All @@ -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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nittiest nit, since reconcileError == nil fatals, feels like this might be a bit less code:

			if reconcileError := c.Reconciler.Reconcile(context.Background(), getRunName(tc.pipelineRun)); reconcileError != nil {
			    if controller.IsPermanentError(reconcileError) != tc.permanentError {
		    	        	t.Fatalf("Expected the error to be permanent: %v but got perman...")
                              }    
			else {
				t.Fatalf("Expected an error to be returned by Reconcile, got nil instead")
			}

maybe that's less clear tho

im definitely over polishing here!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought in a test it would be more readable the way I wrote it :)

}
// 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