From 2659995c8867cb5b6e0ab40277f7ebd1a1b4bcfc Mon Sep 17 00:00:00 2001 From: divyansh42 Date: Tue, 22 Oct 2024 18:36:37 +0530 Subject: [PATCH] Remove permanent error and improve skip logic Signed-off-by: divyansh42 --- pkg/reconciler/pipelinerun/pipelinerun.go | 13 ++----------- pkg/reconciler/pipelinerun/pipelinerun_test.go | 10 +++++----- .../pipelinerun/resources/pipelinerunresolution.go | 2 +- .../pipelinerun/resources/pipelinerunstate.go | 2 +- 4 files changed, 9 insertions(+), 18 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index d21ef4671f6..8be1af972bf 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -848,20 +848,11 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1.Pipeline logger.Infof("Failed to resolve task result reference for %q with error %v", pr.Name, err) // If there is an error encountered, no new task // will be scheduled, hence nextRpts should be empty - // if finally tasks are found, then those tasks will + // If finally tasks are found, then those tasks will // be added to the nextRpts nextRpts = nil + logger.Infof("Adding the task %q to the validation failed list", rpt.ResolvedTask) pipelineRunFacts.ValidationFailedTask = append(pipelineRunFacts.ValidationFailedTask, rpt) - fTaskNames := pipelineRunFacts.GetFinalTaskNames() - if len(fTaskNames) == 0 { - // If finally is not present, we should mark pipelinerun as - // failed so that no further execution happens. Also, - // this will set the completion time of the pipelineRun. - // NewPermanentError should also be returned so that - // reconcilation stops here - pr.Status.MarkFailed(v1.PipelineRunReasonInvalidTaskResultReference.String(), err.Error()) - return controller.NewPermanentError(err) - } } } // GetFinalTasks only returns final tasks when a DAG is complete diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 3327606b142..820a3883c06 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -1284,8 +1284,8 @@ status: image: busybox script: 'exit 0' conditions: - - message: "Invalid task result reference: Could not find result with name result2 for task task1" - reason: InvalidTaskResultReference + - message: "Tasks Completed: 2 (Failed: 1, Cancelled 0), Skipped: 0" + reason: Failed status: "False" type: Succeeded childReferences: @@ -1301,15 +1301,15 @@ status: prt := newPipelineRunTest(t, d) defer prt.Cancel() - reconciledRun, clients := prt.reconcileRun("foo", "test-pipeline-missing-results", []string{}, true) + reconciledRun, clients := prt.reconcileRun("foo", "test-pipeline-missing-results", []string{}, false) if reconciledRun.Status.CompletionTime == nil { t.Errorf("Expected a CompletionTime on invalid PipelineRun but was nil") } - // The PipelineRun should be marked as failed due to InvalidTaskResultReference. + // The PipelineRun should be marked as failed if d := cmp.Diff(expectedPipelineRun, reconciledRun, ignoreResourceVersion, ignoreLastTransitionTime, ignoreTypeMeta, ignoreStartTime, ignoreCompletionTime, ignoreProvenance); d != "" { - t.Errorf("Expected to see PipelineRun run marked as failed with the reason: InvalidTaskResultReference. Diff %s", diff.PrintWantGot(d)) + t.Errorf("Expected to see PipelineRun run marked as failed. Diff %s", diff.PrintWantGot(d)) } // Check that the expected TaskRun was created diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 211eb7bd458..0a41159e705 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -349,7 +349,7 @@ func (t *ResolvedPipelineTask) skip(facts *PipelineRunFacts) TaskSkipStatus { var skippingReason v1.SkippingReason switch { - case facts.isFinalTask(t.PipelineTask.Name) || t.isScheduled(): + case facts.isFinalTask(t.PipelineTask.Name) || t.isScheduled() || t.isValidationFailed(facts.ValidationFailedTask): skippingReason = v1.None case facts.IsStopping(): skippingReason = v1.StoppingSkip diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go index acae391a692..8ca10e4903e 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go @@ -364,7 +364,7 @@ func (state PipelineRunState) getNextTasks(candidateTasks sets.String) []*Resolv func (facts *PipelineRunFacts) IsStopping() bool { for _, t := range facts.State { if facts.isDAGTask(t.PipelineTask.Name) { - if t.isFailure() && t.PipelineTask.OnError != v1.PipelineTaskContinue { + if (t.isFailure() || t.isValidationFailed(facts.ValidationFailedTask)) && t.PipelineTask.OnError != v1.PipelineTaskContinue { return true } }