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 10, 2020
1 parent 7df013b commit ed20e5f
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 53 deletions.
41 changes: 23 additions & 18 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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():
Expand Down Expand Up @@ -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)
}
}

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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -289,31 +294,31 @@ 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 parameters from the PipelineRun are overriding Pipeline parameters with the same type.
Expand All @@ -324,23 +329,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 @@ -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() {
Expand All @@ -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)
}
}

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

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

var as artifacts.ArtifactStorageInterface

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 {
Expand Down
88 changes: 53 additions & 35 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import (
"knative.dev/pkg/apis"
duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1"
"knative.dev/pkg/configmap"
"knative.dev/pkg/controller"
)

var (
Expand Down Expand Up @@ -382,6 +383,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 @@ -445,52 +448,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,
},
}

Expand All @@ -500,12 +515,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 ed20e5f

Please sign in to comment.