Change ordering of ResolveResultRefs/ApplyTaskResults #6792
Conversation
tekton-robot
added
kind/feature
tekton-robot
added
needs-rebase
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
from
ac72d98
to
70957d6
Compare
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
from
70957d6
to
9ca533f
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
from
9ca533f
to
128c2bf
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
2 times, most recently
from
47dbfd0
to
7c33032
Compare
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
2 times, most recently
from
d2ecb93
to
dd01206
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
from
dd01206
to
78ff4d7
Compare
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
from
6df0ea1
to
83857aa
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| ) (*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 |
There was a problem hiding this comment.
Under what circumstances could result references be missing? In these circumstances, will pipelineTask.Matrix.CountCombinations() return the correct number of combinations, and will resolvePipelineTask create 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, ResolvePipelineTask will generate TaskRunNames w/ length 1 for matrixed tasks fanned out from results, and on later reconcile loops will have the correct value.
I think having TaskRunNames in 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.
The circumstance when result references will be missing is TestMissingResultWhenStepErrorIsIgnored
https://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 matrix PR and it's passing all of the tests. #6603
pipeline/pkg/reconciler/pipelinerun/pipelinerun_test.go
Lines 10397 to 10592 in 2e2f7a1
Let me know if that answers your question/concern.
|
|
||
| // 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
tekton-robot
added
the
needs-rebase
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
from
83857aa
to
57a9636
Compare
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
from
57a9636
to
b5d6047
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
| // TaskRun. The final validation is handled in skipBecauseResultReferencesAreMissing. | ||
| 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 ValidateArrayResultsIndex() exists within resolutionrefresolution_test.go.
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 validateArrayResultsIndex but not tests the ResolvePipelineTask where we call the function.
The 2nd link makes sense! The validation happened in pipelinerun.go before but in this PR we moved it to the ResolvePipelineTask, I think it should be ok to rely on that test.
This line is red in the coverage report but I think that's fine.
| // CheckMissingResultReferences returns an error if it is missing any result references | ||
| // validates any array result indexes to ensure the reference is in bounds. | ||
| // 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 { |
There was a problem hiding this comment.
not a block for the PR: the docstrings says "validates any array result indexes to ensure the reference is in bounds.", is it referring to validateArrayResultsIndex. Can we call validateArrayResultsIndex inside this function?
There was a problem hiding this comment.
@Yongxuanzhang Good catch. I forgot I updated this so that validateArrayResultIndex() is called outside of this function
so that we are able to return an error that immediately fails the pipeline run. Removed this from the doc string.
| @@ -3823,6 +3809,7 @@ func TestResolvePipelineRunTask_WithMatrix(t *testing.T) { | |||
| name string | |||
| pt v1.PipelineTask | |||
| want *ResolvedPipelineTask | |||
| pst PipelineRunState | |||
There was a problem hiding this comment.
is this pst used in the tests? I cannot find it used in the 2 test cases
There was a problem hiding this comment.
No, I don't think it's relevant to these test cases since we aren't dealing with results so removed it altogether, but have added it with whole array result examples in the subsequent PR: https://github.com/tektoncd/pipeline/blob/c164f71ab448c4ac263d0113bf1b58d1c0199be3/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go#L3822-L#3888
This refactor changes the ordering of when we resolve references to results and apply task results within the reconciler. Prior to this change, we were waiting until `RunNextScheduableTask()` to resolve and apply task results. This becomes an issue when trying to consume whole array results in a Matrix since the TaskRuns are created prior to `RunNextScheduableTask()` and the number of TaskRuns before resolving references is nondeterministic [*]. Consuming whole array results is a feature required in order to promote Matrix to beta and will be implemented in a subsequent PR. In this commit, we move this logic before `RunNextScheduableTask()` into `ResolvePipelineTask()` and resolve and apply the task results upfront so that by the time we go to create the TaskRuns, all of the replacements have been applied and we know exactly how many TaskRuns to create. Changing the ordering of the reconciler logic means we also have to change some of the error handling with ResolveResultRefs because we currently fail a PipelineRun if any of the result values are missing, however this could be a valid PR if it's a skipped Task due to `MissingResultsSkip`. This commit removes some of the initial errors that would cause the PipelineRun to fail the first time we ResolveResultRefs since some tasks could be missing result references, yet are valid at this point in time and need to be marked as "skipBecauseResultReferencesAreMissing". This enables us to mark all of the task that are supposed to be skipped as "skipBecauseResultReferencesAreMissing" rather than failing the pipelinerun immediately. We later check for any actual missing result references in the Skipped Tasks and fail the PipelineRun in RunNextSchedulableTask(). An example of this is in TestMissingResultWhenStepErrorIsIgnored()when Task 1 produces Result A successfully and fails to produce Result B, but has onError continue. Task 2 consumes the results from Task 1, which results in the Failed PipelineRun because Task2 cannot reference Result B that was never produced in Task 1. However because On Error: Continue was used, Task 1 succeeded and the PipelineRun needs to now be marked as failed due to "ReasonInvalidTaskResultReference".
EmmaMunley
force-pushed
the
RefactorApplyingTaskResults
branch
from
b5d6047
to
eb779cc
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| // TaskRun. The final validation is handled in skipBecauseResultReferencesAreMissing. | ||
| resolvedResultRefs, _, _ := ResolveResultRefs(pst, PipelineRunState{&rpt}) | ||
| if err := validateArrayResultsIndex(resolvedResultRefs); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
The test in TestValidateArrayResultsIndex only tests the validateArrayResultsIndex but not tests the ResolvePipelineTask where we call the function.
The 2nd link makes sense! The validation happened in pipelinerun.go before but in this PR we moved it to the ResolvePipelineTask, I think it should be ok to rely on that test.
This line is red in the coverage report but I think that's fine.
| name: "Valid: successful result references resolution - params", | ||
| pipelineRunState: pipelineRunState, | ||
| targets: PipelineRunState{ | ||
| pipelineRunState[1], |
There was a problem hiding this comment.
not related to this PR, I think it's hard to review this kind of test. Maybe one solution is to separate them into different vars with meaningful names?😂
Changes
This refactor changes the ordering of when we resolve references to results and apply task results within the reconciler. Prior to this change, we were waiting until
RunNextScheduableTask()to resolve and apply task results. This becomes an issue when trying to consume whole array results in a Matrix since the TaskRuns are created prior toRunNextScheduableTask()and the number of TaskRuns before resolving references is nondeterministic [*]. Consuming whole array results is a feature required in order to promote Matrix to beta and will be implemented in a subsequent PR #6603.In this PR, we move this logic before
RunNextScheduableTask()intoResolvePipelineTask()and resolve and apply the task results upfront so that by the time we go to create the TaskRuns, all of the replacements have been applied and we know exactly how many TaskRuns to create. Changing the ordering of the reconciler logic means we also have to change some of the error handling with ResolveResultRefs because we currently fail a PipelineRun if any of the result values are missing, however this could be a valid PipelineRun if it's a skipped Task due toMissingResultsSkip. This PR removes some of the initial errors that would cause the PipelineRun to fail the first time we ResolveResultRefs since some tasks could be missing result references, yet are valid at this point in time and need to be marked as "skipBecauseResultReferencesAreMissing". This enables us to mark all of the task that are supposed to be skipped as "skipBecauseResultReferencesAreMissing" rather than failing the pipelinerun immediately.We later check for any actual missing result references in the Skipped Tasks and fail the PipelineRun in RunNextSchedulableTask().
An example of this is in TestMissingResultWhenStepErrorIsIgnored()when Task 1 produces Result A successfully and fails to produce Result B, but has onError continue. Task 2 consumes the results from Task 1, which results in the Failed PipelineRun because Task2 cannot reference Result B that was never produced in Task 1. However because On Error: Continue was used, Task 1 succeeded and the PipelineRun needs to now be marked as failed due to "ReasonInvalidTaskResultReference".
For more information on why this is necessary please see the discussion here: #6056 (comment)
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes