-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: avoid fan out matrix task failed due to result ref #8487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hi @l-qing. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
name: "combinations count is zero", | ||
matrix: &v1.Matrix{ | ||
Params: v1.Params{{}}}, | ||
Params: v1.Params{}}, |
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 behavior is different between having no param and having a param with an empty value.
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.
btw, the indentation is really weird here. We should format it better.
&v1.Matrix{
Params: v1.Params{},
}
// or
&v1.Matrix{Params: v1.Params{}}
Params: v1.Params{{ | ||
Name: "GOARCH", Value: v1.ParamValue{ArrayVal: []string{"linux/amd64", "linux/ppc64le", "linux/s390x"}}, | ||
}, { | ||
Name: "version", Value: v1.ParamValue{StringVal: "$(tasks.platforms.results.str[*])"}}, |
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 problem to be solved this time involves referencing an array parameter within a string value.
- name: b-task | ||
taskRef: | ||
name: mytask | ||
taskSpec: |
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.
Avoid returning early due to the PipelineRunReasonCouldntGetTask
error.
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.
Does this require a change in the codebase instead, so that we don't return early? Skipping dereferencing a task here to avoid returning early would be a workaround then is my only concern.
pt: pts[2], | ||
want: &ResolvedPipelineTask{ | ||
TaskRunNames: nil, | ||
TaskRunNames: []string{"pipelinerun-pipelinetask-with-whole-array-results"}, |
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 current behavior will return a definition of TaskSpec, facilitating subsequent verification.
matrix: | ||
params: | ||
- name: platform | ||
value: $(tasks.matrix-include.results.str[*]) |
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.
Added a usage that is supported this time.
d4c465d
to
def2c31
Compare
It seems that this usage might bypass the limitation of pipeline/pkg/apis/pipeline/v1/matrix_types.go Lines 294 to 301 in 1dd488e
|
Hi, @afrittoli can you help me review it when you have time? |
Looking forward for this fix 👍🏽 |
/retest |
name: "combinations count is zero", | ||
matrix: &v1.Matrix{ | ||
Params: v1.Params{{}}}, | ||
Params: v1.Params{}}, |
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.
btw, the indentation is really weird here. We should format it better.
&v1.Matrix{
Params: v1.Params{},
}
// or
&v1.Matrix{Params: v1.Params{}}
/ok-to-test |
c971dc1
to
f8a3ecd
Compare
f8a3ecd
to
71e9e08
Compare
The logical changes in this PR are minimal, almost just three lines. |
71e9e08
to
f5ba9b8
Compare
/assign @vdemeester @afrittoli |
I will adjust this branch after the golangci-lint pipeline passes on the main branch. When I use the same version 1.62.2 of the golangci-lint tool for local scanning, my computer freezes. |
f5ba9b8
to
c026aa1
Compare
@afrittoli Can you help assign a suitable person for this PR? |
5e15f05
to
5723a3d
Compare
/assign |
- name: b-task | ||
taskRef: | ||
name: mytask | ||
taskSpec: |
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.
Does this require a change in the codebase instead, so that we don't return early? Skipping dereferencing a task here to avoid returning early would be a workaround then is my only concern.
5723a3d
to
c6fbd8b
Compare
@waveywaves I adjusted the unit test code here. Do you think this is okay? c6fbd8b Due to the previous reconciliation, the calculated pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go Lines 613 to 631 in 1d2ea8b
So in order to proceed to the subsequent tests, I need to resolve the issue of the task not existing in the original unit tests. |
It seems that the failure was due to a failure in pulling the image. However, I am unsure of the specific reason for the failure.
|
c6fbd8b
to
babc976
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
babc976
to
ebbf13a
Compare
@waveywaves are your requested changes still blocking? Can this be merged? |
/lgtm |
/cherry-pick release-v0.68.x |
@savitaashture: new pull request created: #8828 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-v0.65.x |
@savitaashture: new pull request created: #8829 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
fix #8324
Continuing development based on this PR: #8327
Changes
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
/kind bug
1. Reproducible steps
2. Error message
invalid result reference in pipeline task "printer-matrix": unable to validate result referencing pipeline task "platforms": task spec not found
3. Analysis
a.
validateResultRef
:unable to validate result referencing pipeline task
if ptMap[ref.PipelineTask].ResolvedTask == nil || ptMap[ref.PipelineTask].ResolvedTask.TaskSpec == nil {
pipeline/pkg/reconciler/pipelinerun/resources/validate_dependencies.go
Lines 75 to 77 in 1dd488e
b.
ValidatePipelineTaskResults
:invalid result reference in pipeline task
pipeline/pkg/reconciler/pipelinerun/resources/validate_dependencies.go
Lines 31 to 36 in 1dd488e
c.
PipelineRun-Reconcile
: callValidatePipelineTaskResults
pipeline/pkg/reconciler/pipelinerun/pipelinerun.go
Lines 697 to 702 in 1dd488e
d.
pipelineRunFacts.State
: come fromresolvePipelineState
pipeline/pkg/reconciler/pipelinerun/pipelinerun.go
Lines 612 to 627 in 1dd488e
e.
resolvePipelineState
: resolvedTask -ResolvePipelineTask
pipeline/pkg/reconciler/pipelinerun/pipelinerun.go
Lines 368 to 377 in 1dd488e
f.
ResolvePipelineTask
: callCountCombinations
pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
Lines 602 to 630 in 1dd488e
h1.
CountCombinations
: Calculate the number of TaskRunspipeline/pkg/apis/pipeline/v1/matrix_types.go
Lines 220 to 242 in 1dd488e
h2. Error: The calculated count is 0.
pipeline/pkg/apis/pipeline/v1/matrix_types.go
Lines 233 to 239 in 1dd488e
Because the
param.Value.StringVale
is$(tasks.platforms.results.str[*])
andparam.Value.ArrayVal
is empty.j1.
ResolvePipelineTask
: callGetNamesOfTaskRuns
pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
Lines 628 to 630 in 1dd488e
j2.
GetNamesOfTaskRuns
: callgetNewRunNames
thenumberOfRuns
is 0, the resulttaskRunNames
is emptypipeline/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
Lines 749 to 771 in 1dd488e
k.
ResolvePipelineTask
:setTaskRunsAndResolvedTask
has not been called.pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
Lines 628 to 632 in 1dd488e
l.
setTaskRunsAndResolvedTask
:ResolvedTask
has not been set.pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
Lines 641 to 662 in 1dd488e
m. So it led to the error seen at the top.