Skip to content

Commit

Permalink
Avoid retry on permanent errors in pipelinerun
Browse files Browse the repository at this point in the history
fixes #6298

When creating taskrun and other run resources in pipelinerun,
if a permanent error occurs due to missing workspace configuration,
it will fail immediately instead of continuously retrying until timeout.
  • Loading branch information
l-qing committed Mar 17, 2023
1 parent 1d2e259 commit efc7799
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 7 deletions.
1 change: 1 addition & 0 deletions docs/pipelineruns.md
Original file line number Diff line number Diff line change
Expand Up @@ -1261,6 +1261,7 @@ False | Failed | Yes |
False | \[Error message\] | Yes | The `PipelineRun` failed with a permanent error (usually validation).
False | Cancelled | Yes | The `PipelineRun` was cancelled successfully.
False | PipelineRunTimeout | Yes | The `PipelineRun` timed out.
False | CreateRunFailed | Yes | The `PipelineRun` create run resources failed.

When a `PipelineRun` changes status, [events](events.md#pipelineruns) are triggered accordingly.

Expand Down
30 changes: 23 additions & 7 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ const (
// ReasonResourceVerificationFailed indicates that the pipeline fails the trusted resource verification,
// it could be the content has changed, signature is invalid or public key is invalid
ReasonResourceVerificationFailed = "ResourceVerificationFailed"
// ReasonCreateRunFailed indicates that the pipeline fails to create the taskrun or other run resources
ReasonCreateRunFailed = "CreateRunFailed"
)

// constants used as kind descriptors for various types of runs; these constants
Expand Down Expand Up @@ -755,30 +757,41 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip
continue
}

defer func() {
// If it is a permanent error, set pipelinerun to a failure state directly to avoid unnecessary retries.
if err != nil && controller.IsPermanentError(err) {
pr.Status.MarkFailed(ReasonCreateRunFailed, err.Error())
}
}()

switch {
case rpt.IsCustomTask() && rpt.IsMatrixed():
rpt.RunObjects, err = c.createRunObjects(ctx, rpt, pr)
if err != nil {
recorder.Eventf(pr, corev1.EventTypeWarning, "RunsCreationFailed", "Failed to create Runs %q: %v", rpt.RunObjectNames, err)
return fmt.Errorf("error creating Runs called %s for PipelineTask %s from PipelineRun %s: %w", rpt.RunObjectNames, rpt.PipelineTask.Name, pr.Name, err)
err = fmt.Errorf("error creating Runs called %s for PipelineTask %s from PipelineRun %s: %w", rpt.RunObjectNames, rpt.PipelineTask.Name, pr.Name, err)
return err
}
case rpt.IsCustomTask():
rpt.RunObject, err = c.createRunObject(ctx, rpt.RunObjectName, nil, rpt, pr)
if err != nil {
recorder.Eventf(pr, corev1.EventTypeWarning, "RunCreationFailed", "Failed to create Run %q: %v", rpt.RunObjectName, err)
return fmt.Errorf("error creating Run called %s for PipelineTask %s from PipelineRun %s: %w", rpt.RunObjectName, rpt.PipelineTask.Name, pr.Name, err)
err = fmt.Errorf("error creating Run called %s for PipelineTask %s from PipelineRun %s: %w", rpt.RunObjectName, rpt.PipelineTask.Name, pr.Name, err)
return err
}
case rpt.IsMatrixed():
rpt.TaskRuns, err = c.createTaskRuns(ctx, rpt, pr)
if err != nil {
recorder.Eventf(pr, corev1.EventTypeWarning, "TaskRunsCreationFailed", "Failed to create TaskRuns %q: %v", rpt.TaskRunNames, err)
return fmt.Errorf("error creating TaskRuns called %s for PipelineTask %s from PipelineRun %s: %w", rpt.TaskRunNames, rpt.PipelineTask.Name, pr.Name, err)
err = fmt.Errorf("error creating TaskRuns called %s for PipelineTask %s from PipelineRun %s: %w", rpt.TaskRunNames, rpt.PipelineTask.Name, pr.Name, err)
return err
}
default:
rpt.TaskRun, err = c.createTaskRun(ctx, rpt.TaskRunName, nil, rpt, pr)
if err != nil {
recorder.Eventf(pr, corev1.EventTypeWarning, "TaskRunCreationFailed", "Failed to create TaskRun %q: %v", rpt.TaskRunName, err)
return fmt.Errorf("error creating TaskRun called %s for PipelineTask %s from PipelineRun %s: %w", rpt.TaskRunName, rpt.PipelineTask.Name, pr.Name, err)
err = fmt.Errorf("error creating TaskRun called %s for PipelineTask %s from PipelineRun %s: %w", rpt.TaskRunName, rpt.PipelineTask.Name, pr.Name, err)
return err
}
}
}
Expand Down Expand Up @@ -1010,6 +1023,7 @@ func propagateWorkspaces(rpt *resources.ResolvedPipelineTask) (*resources.Resolv
}

func getTaskrunWorkspaces(ctx context.Context, pr *v1beta1.PipelineRun, rpt *resources.ResolvedPipelineTask) ([]v1beta1.WorkspaceBinding, string, error) {
var err error
var workspaces []v1beta1.WorkspaceBinding
var pipelinePVCWorkspaceName string
pipelineRunWorkspaces := make(map[string]v1beta1.WorkspaceBinding)
Expand All @@ -1020,10 +1034,10 @@ func getTaskrunWorkspaces(ctx context.Context, pr *v1beta1.PipelineRun, rpt *res
if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields != config.StableAPIFields {
// Propagate required workspaces from pipelineRun to the pipelineTasks
if rpt.PipelineTask.TaskSpec != nil {
var err error
rpt, err = propagateWorkspaces(rpt)
if err != nil {
return nil, "", err
// This error cannot be recovered without modifying the TaskSpec
return nil, "", controller.NewPermanentError(err)
}
}
}
Expand Down Expand Up @@ -1052,7 +1066,9 @@ func getTaskrunWorkspaces(ctx context.Context, pr *v1beta1.PipelineRun, rpt *res
}
}
if !workspaceIsOptional {
return nil, "", fmt.Errorf("expected workspace %q to be provided by pipelinerun for pipeline task %q", pipelineWorkspace, rpt.PipelineTask.Name)
err = fmt.Errorf("expected workspace %q to be provided by pipelinerun for pipeline task %q", pipelineWorkspace, rpt.PipelineTask.Name)
// This error cannot be recovered without modifying the PipelineRun
return nil, "", controller.NewPermanentError(err)
}
}
}
Expand Down
67 changes: 67 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7263,6 +7263,8 @@ spec:
t.Errorf("Pipeline.getTaskrunWorkspaces() did not return error for invalid workspace")
} else if d := cmp.Diff(tt.expectedError, err.Error(), cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" {
t.Errorf("Pipeline.getTaskrunWorkspaces() errors diff %s", diff.PrintWantGot(d))
} else if !controller.IsPermanentError(err) {
t.Errorf("Pipeline.getTaskrunWorkspaces() not returned a permanent error for invalid workspace")
}
})
}
Expand Down Expand Up @@ -10079,3 +10081,68 @@ spec:
})
}
}

func TestReconcileForPipelineRunCreateRunFailed(t *testing.T) {
// TestReconcileForPipelineRunCreateRunFailed runs "Reconcile" on a PipelineRun that has permanent error.
// It verifies that reconcile is successful, no TaskRun is created, the PipelineTask is marked as CreateRunFailed, and the
// pipeline status updated and events generated.
ps := []*v1beta1.Pipeline{}
prs := []*v1beta1.PipelineRun{parse.MustParseV1beta1PipelineRun(t, `
metadata:
name: test-pipeline-run-with-create-run-failed
namespace: foo
spec:
pipelineSpec:
tasks:
- name: hello-world
taskRef:
name: unit-test-task
workspaces:
- name: source
`)}

ts := []*v1beta1.Task{
parse.MustParseV1beta1Task(t, `
metadata:
name: unit-test-task
namespace: foo
spec:
workspaces:
- name: source
steps:
- image: alpine:latest
name: test
script: |
echo "hello world"
workspaces:
- name: source
`)}

trs := []*v1beta1.TaskRun{}

d := test.Data{
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
TaskRuns: trs,
}
prt := newPipelineRunTest(t, d)
defer prt.Cancel()

wantEvents := []string{
"Normal Started",
"Warning TaskRunCreationFailed",
"error creating TaskRun called test-pipeline-run-with-create-run-failed-hello-world for PipelineTask hello-world from PipelineRun test-pipeline-run-with-create-run-failed: expected workspace \"source\" to be provided by pipelinerun for pipeline task \"hello-world\"",
"error creating TaskRun called test-pipeline-run-with-create-run-failed-hello-world for PipelineTask hello-world from PipelineRun test-pipeline-run-with-create-run-failed: expected workspace \"source\" to be provided by pipelinerun for pipeline task \"hello-world\"",
}
reconciledRun, _ := prt.reconcileRun("foo", "test-pipeline-run-with-create-run-failed", wantEvents, true)

if reconciledRun.Status.CompletionTime == nil {
t.Errorf("Expected a CompletionTime on invalid PipelineRun but was nil")
}

// The PipelineRun should be create run failed.
if reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason != "CreateRunFailed" {
t.Errorf("Expected PipelineRun to be create run failed, but condition reason is %s", reconciledRun.Status.GetCondition(apis.ConditionSucceeded))
}
}

0 comments on commit efc7799

Please sign in to comment.