Change ordering of ResolveResultRefs/ApplyTaskResults #6792
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -542,14 +542,25 @@ func ResolvePipelineTask( | |
| getTaskRun resources.GetTaskRun, | ||
| getRun GetRun, | ||
| pipelineTask v1.PipelineTask, | ||
| pst PipelineRunState, | ||
| ) (*ResolvedPipelineTask, error) { | ||
| rpt := ResolvedPipelineTask{ | ||
| PipelineTask: &pipelineTask, | ||
| } | ||
| rpt.CustomTask = rpt.PipelineTask.TaskRef.IsCustomTask() || rpt.PipelineTask.TaskSpec.IsCustomTask() | ||
| numCombinations := 1 | ||
| // We want to resolve all of the result references and ignore any errors at this point since there could be | ||
| // instances where result references are missing here, but will be later skipped or resolved in a subsequent | ||
| // TaskRun. The final validation is handled in skipBecauseResultReferencesAreMissing. | ||
|
There was a problem hiding this comment. question: instead of passing in the pipelinerunState struct to ResolvePipelineTask, could we resolve the references right after we call ResolvedPipelineTask but before we append it to the pipelinerun state i.e. after line 379 |
||
| resolvedResultRefs, _, _ := ResolveResultRefs(pst, PipelineRunState{&rpt}) | ||
| if err := validateArrayResultsIndex(resolvedResultRefs); err != nil { | ||
| return nil, err | ||
|
There was a problem hiding this comment. nit (not a block for the PR): I think this line is not covered? There was a problem hiding this comment. For this PR the coverage for However I do have a pipelinerun.go unit test that covers an invalid array result index in the whole array results implementation https://github.com/tektoncd/pipeline/blob/c164f71ab448c4ac263d0113bf1b58d1c0199be3/pkg/reconciler/pipelinerun/pipelinerun_test.go#LL10603C6-L10603C66 There was a problem hiding this comment. The test in TestValidateArrayResultsIndex only tests the The 2nd link makes sense! The validation happened in This line is red in the coverage report but I think that's fine. |
||
| } | ||
|
|
||
| ApplyTaskResults(PipelineRunState{&rpt}, resolvedResultRefs) | ||
|
|
||
| if rpt.PipelineTask.IsMatrixed() { | ||
| numCombinations = rpt.PipelineTask.Matrix.CountCombinations() | ||
| } | ||
| if rpt.IsCustomTask() { | ||
| rpt.CustomRunNames = getNamesOfCustomRuns(pipelineRun.Status.ChildReferences, pipelineTask.Name, pipelineRun.Name, numCombinations) | ||
|
|
@@ -748,3 +759,28 @@ func (t *ResolvedPipelineTask) hasResultReferences() bool { | |
| func isCustomRunCancelledByPipelineRunTimeout(cr *v1beta1.CustomRun) bool { | ||
| return cr.Spec.StatusMessage == v1beta1.CustomRunCancelledByPipelineTimeoutMsg | ||
| } | ||
|
|
||
| // CheckMissingResultReferences returns an error if it is missing any result references. | ||
| // Missing result references can occur if task fails to produce a result but has | ||
| // OnError: continue (ie TestMissingResultWhenStepErrorIsIgnored) | ||
| func CheckMissingResultReferences(pipelineRunState PipelineRunState, targets PipelineRunState) error { | ||
| for _, target := range targets { | ||
| for _, resultRef := range v1.PipelineTaskResultRefs(target.PipelineTask) { | ||
| referencedPipelineTask := pipelineRunState.ToMap()[resultRef.PipelineTask] | ||
| if referencedPipelineTask.IsCustomTask() { | ||
| customRun := referencedPipelineTask.CustomRuns[0] | ||
| _, err := findRunResultForParam(customRun, resultRef) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| taskRun := referencedPipelineTask.TaskRuns[0] | ||
| _, err := findTaskResultForParam(taskRun, resultRef) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what circumstances could result references be missing? In these circumstances, will pipelineTask.Matrix.CountCombinations() return the correct number of combinations, and will
resolvePipelineTaskcreate the correct number of TaskRuns?I think we may try to resolve all pipeline tasks immediately, before running any. It seems to me like on the first reconcile loop,
ResolvePipelineTaskwill generateTaskRunNamesw/ length 1 for matrixed tasks fanned out from results, and on later reconcile loops will have the correct value.I think having
TaskRunNamesin ResolvedPipelineTask doesn't work so well for matrixed pipeline tasks fanned out from array results, and would like to revisit the idea of label selectors. (We could also just merge #6603 if that gets too complicated.) I think this PR is running into a similar problem as #6603 though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The circumstance when result references will be missing is
TestMissingResultWhenStepErrorIsIgnoredhttps://github.com/tektoncd/pipeline/blob/main/pkg/reconciler/pipelinerun/pipelinerun_test.go#L1052-L1090
The reason why we are reordering where we resolve and applyTaskResults in this PR is so that a matrixed pipelinetask that contains references to whole array results will not contain missing result references because the references will have already been resolved before where we try to generate the taskNames based on the number of combination in
runNextSchedulableTask()so that we have the accurate number of combinations when fanning out the TaskRun() the first and only time it's called.I have rebased and applied theses changes in the
Add support for consuming whole array results in matrixPR and it's passing all of the tests. #6603pipeline/pkg/reconciler/pipelinerun/pipelinerun_test.go
Lines 10397 to 10592 in 2e2f7a1
Let me know if that answers your question/concern.