Skip to content
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

Pipeline failing with "invalid pipelineresults" when referencing results of skipped tasks #6139

Closed
thomascube opened this issue Feb 9, 2023 · 30 comments · Fixed by #6157 or #7407
Closed
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@thomascube
Copy link

Expected Behavior

According to the documentation pipeline results can contain references to task results. If a task did not run because of it's whenConditions, the pipeline result is not emitted but the pipeline succeeds. This worked with v0.37.5 of Tekton Pipelines available on OpenShift.

Actual Behavior

With the update of the RedHat OpenShift Pipelines Operator, which brings Tekton Pipeline v0.41.0, pipelines with skipped tasks and composed results now fail with the message invalid pipelineresults [result-goodbye], the referred results don't exist.

Steps to Reproduce the Problem

Run the following PipelineRun:

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: pipelinerun-test
spec:
  pipelineSpec:
    params:
      - name: say-hello
        default: 'false'
    tasks:
      - name: hello
        taskSpec:
          results:
            - name: result-one
          steps:
          - image: alpine
            script: |
              #!/bin/sh
              echo "Hello world!"
              echo -n "RES1" > $(results.result-one.path)
  
      - name: goodbye
        runAfter:
          - hello
        taskSpec:
          results:
            - name: result-two
          steps:
          - image: alpine
            script: |
              #!/bin/sh
              echo "Goodbye world!"
              echo -n "RES2" > $(results.result-two.path)
        when:
          - input: $(params.say-hello)
            operator: in
            values: ["true"]

    results:
      - name: result-hello
        description: Result one
        value: '$(tasks.hello.results.result-one)'
      - name: result-goodbye
        description: Result two
        value: '$(tasks.goodbye.results.result-two)'

Additional Info

  • Kubernetes version:
Server Version: version.Info{Major:"1", Minor:"25", GitVersion:"v1.25.4+a34b9e9", GitCommit:"b6d1f054747e9886f61dd85316deac3415e2726f", GitTreeState:"clean", BuildDate:"2023-01-10T15:55:28Z", GoVersion:"go1.19.4", Compiler:"gc", Platform:"linux/amd64"}
  • Tekton Pipeline version:
Pipeline version: v0.41.0
Triggers version: v0.22.0
Openshift Pipelines Version: 1.9.0
@thomascube thomascube added the kind/bug Categorizes issue or PR as related to a bug. label Feb 9, 2023
@thomascube
Copy link
Author

Success on v0.37.5:
pipeline-success-v0 37 5

Failure on v0.41.0:
pipeline-failure-v0 41 0

@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Feb 9, 2023

Hi @thomascube, thanks!
I think the validation is added intentionally, do you think it makes more sense to remove the check?
Since I may lack the knowledge of the use cases, I'd like to learn more about your opinons.

@kyubisation
Copy link

@Yongxuanzhang We would like to display these values in our UI tool (if they are available). Is there any way to default to empty, if the task was not run?
e.g. something like '$(tasks.hello.results.result-one || '')' or '$(tasks.hello && tasks.hello.results.result-one)'?

@jerop
Copy link
Member

jerop commented Feb 9, 2023

@kyubisation we don't have default result yet but it's something that @vinamra28 has been looking into -- considering a syntax similar to the first one you suggested: '$(tasks.hello.results.result-one || '')'

@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Feb 9, 2023

@Yongxuanzhang We would like to display these values in our UI tool (if they are available). Is there any way to default to empty, if the task was not run? e.g. something like '$(tasks.hello.results.result-one || '')' or '$(tasks.hello && tasks.hello.results.result-one)'?

I think default results would be a solution. But it may take much longer time?

I wonder before this update, if the results are not available, there will be nothing showing in UI?
The reason I add the check is to help debugging. So you would know which results are not emitted.
I'm thinking if there is a better way to meet both needs?
e.g. We don't fail the pipelinerun but only log the warning message?

Or actually we don't want to have any check/validation on this? If so I think we could just remove the validation.

@kyubisation @jerop any suggestions?

@thomascube
Copy link
Author

@Yongxuanzhang I think the old behavior which is also described in the documentation under "A Pipeline Result is not emitted if any of the following are true" makes perfectly sense and we were running fine with it. With the new check and the failure of the pipeline it's impossible to use task results in conjunction with when conditions.

@kyubisation
Copy link

@Yongxuanzhang Having validation is reasonable, but directly breaking the pipeline without a workaround/migration possibility is not ideal. I'd prefer to have this either be opt-in or have to have a fallback/default available at the same time.

@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Feb 10, 2023

@Yongxuanzhang I think the old behavior which is also described in the documentation under "A Pipeline Result is not emitted if any of the following are true" makes perfectly sense and we were running fine with it. With the new check and the failure of the pipeline it's impossible to use task results in conjunction with when conditions.

In the doc you linked it says:

A PipelineTask referenced by the Pipeline Result didn’t emit the referenced Task Result. This should be considered a bug in the Task and may fail a PipelineTask in future.

It looks like we want to fail the run if the results are not emitted but referenced?
You case is like the results may or may not emit and it depends on the task's when condition.

I'm ok with @kyubisation's suggestion.

@thomascube
Copy link
Author

@Yongxuanzhang apparently I read the following statement differently

This should be considered a bug in the Task and may fail a PipelineTask in the future.

For me this sounds like a case where a task specifies a result but no write to that result file was made in any of the steps. I didn't make the connection to skipped tasks.

Anyway, you asked about my use-case: we have pipelines for building software projects such as a Java Maven application. The pipelines includes tasks for build the jar, testing it, running quality checks (sonar scan) and creating a Docker image from it. Some tasks are optional (e.g. sonar scan or docker build) and can be enabled/disabled by pipeline parameters consumed by when expressions guarding the task execution. These tasks also emit results (if executed) which are mapped into pipeline results. If for example a docker image was built, the image name and tag are visible in the pipeline results and vice versa: if the docker build was disabled by parameter, no such pipeline result exists.

Or a bit shorter: we run pipelines with optional tasks that yield optional results.

I was just surprised to suddenly see pipeline fail although all tasks have executed successfully. For possible solutions I can just copy @kyubisation's suggestions.

@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Feb 13, 2023

Thanks for your input and suggestions! @thomascube @kyubisation
I have a better understanding of the use cases. I will fix it soon!
Will keep you updated. :D

@Yongxuanzhang
Copy link
Member

/assign
I believe tektoncd/community#954 is a long term solution. But we should fix this to unblock the use cases

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Feb 13, 2023
This commit skip the validation for skipped task results in pipeline
results. This fixes tektoncd#6139. The skipped task results are not emitted by
design thus we shouldn't validate if they are emitted.

Signed-off-by: Yongxuan Zhang [email protected]
@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Feb 13, 2023

I have opened this PR to fix it. @kyubisation @thomascube. Plz take a look 🙏 and let me know if this works for you?

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Feb 13, 2023
This commit skip the validation for skipped task results in pipeline
results. This fixes tektoncd#6139. The skipped task results are not emitted by
design thus we shouldn't validate if they are emitted.

Signed-off-by: Yongxuan Zhang [email protected]
@jerop
Copy link
Member

jerop commented Feb 13, 2023

we run pipelines with optional tasks that yield optional results

@thomascube what do you mean by optional tasks? is it that they are guarded using when expressions?

don't think results were ever meant to be optional, we were planning to enforce it (#3497) but looks like users have built around the current behavior 🤔 -- cc @pritidesai @vinamra28

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Feb 13, 2023
This commit skip the validation for skipped task results in pipeline
results. This fixes tektoncd#6139. The skipped task results are not emitted by
design thus we shouldn't validate if they are emitted. Pipeline results
referenced to skipped task will remain as the original unsubstituted
references.

Signed-off-by: Yongxuan Zhang [email protected]
Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Feb 13, 2023
This commit skip the validation for skipped task results in pipeline
results. This fixes tektoncd#6139. The skipped task results are not emitted by
design thus we shouldn't validate if they are emitted. Pipeline results
referenced to skipped task will remain as the original unsubstituted
references.

Signed-off-by: Yongxuan Zhang [email protected]
Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Feb 14, 2023
This commit skip the validation for skipped task results in pipeline
results. This fixes tektoncd#6139. The skipped task results are not emitted by
design thus we shouldn't validate if they are emitted. Pipeline results
referenced to skipped task will remain as the original unsubstituted
references.

Signed-off-by: Yongxuan Zhang [email protected]
@Yongxuanzhang
Copy link
Member

we run pipelines with optional tasks that yield optional results

@thomascube what do you mean by optional tasks? is it that they are guarded using when expressions?

don't think results were ever meant to be optional, we were planning to enforce it (#3497) but looks like users have built around the current behavior 🤔 -- cc @pritidesai @vinamra28

@jerop Do you think this worth discussing in a wg or some other way? I think this is about how we handle skipped tasks results.
Options:

  1. fail the pipelinerun
  2. skip the validation and log the messages

either way I think we will still need tektoncd/community#954

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Mar 6, 2023
This commit skip the validation for skipped task results in pipeline
results. This fixes tektoncd#6139. The skipped task results are not emitted by
design thus we shouldn't validate if they are emitted. Pipeline results
referenced to skipped task will remain as the original unsubstituted
references.

Signed-off-by: Yongxuan Zhang [email protected]
Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Mar 10, 2023
This commit skip the validation for skipped task results in pipeline
results. This fixes tektoncd#6139. The skipped task results are not emitted by
design thus we shouldn't validate if they are emitted. Pipeline results
referenced to skipped task will remain as the original unsubstituted
references.

Signed-off-by: Yongxuan Zhang [email protected]
@lbernick
Copy link
Member

@Yongxuanzhang just want to confirm, it seems like this regression was introduced in #5088, i.e. it was not the result of an intentional change to add more validation to pipeline results?

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Mar 16, 2023
This commit skip the validation for skipped task results in pipeline
results. This fixes tektoncd#6139. The skipped task results are not emitted by
design thus we shouldn't validate if they are emitted. Pipeline results
referenced to skipped task will not be emitted.
tekton-robot pushed a commit that referenced this issue Mar 16, 2023
This commit skip the validation for skipped task results in pipeline
results. This fixes #6139. The skipped task results are not emitted by
design thus we shouldn't validate if they are emitted. Pipeline results
referenced to skipped task will not be emitted.
@Yongxuanzhang
Copy link
Member

Hi @kyubisation @thomascube, the fix PR is merged, so the release v0.46 should have this fix. And I will cherry pick this PR into previous releases.
I think you're using v0.41.0 ? Maybe we could reopen this issue or I create a new one to track it?

yyzxw pushed a commit to yyzxw/pipeline that referenced this issue May 19, 2023
This commit aims to fix tektoncd#6383, tektoncd#6139 and supports tektoncd#3749. This commits
skip the validation if the taskrun is not successful (e.g. failed or skipped) and
omit the pipelinerun coresponding result. This way the skipped taskrun
won't get validation error for pipelinerun. And the pipelinerun error
won't be overwritten by the validation error.

Signed-off-by: Yongxuan Zhang [email protected]
tekton-robot pushed a commit that referenced this issue May 19, 2023
This commit aims to fix #6383, #6139 and supports #3749. This commits
skip the validation if the taskrun is not successful (e.g. failed or skipped) and
omit the pipelinerun coresponding result. This way the skipped taskrun
won't get validation error for pipelinerun. And the pipelinerun error
won't be overwritten by the validation error.

Signed-off-by: Yongxuan Zhang [email protected]
@thomascube
Copy link
Author

Hello. We just upgraded to Tekton v0.47.5 (via OpenShift Pipelines) and now this issue is back!

Our pipeline has defined a result which copies the value of a task result:

  results:
    - description: The URL to the generated junit test report
      name: REPORT_URL
      value: $(finally.generate-report.results.REPORT_URL)

Now if the task generate-report is skipped due to a when condition, the pipeline fails with the following error:

invalid pipelineresults [REPORT_URL], the referred results don't exist

With Tekton v0.44.2 the same pipeline terminated without an error.

@Yongxuanzhang Yongxuanzhang reopened this Nov 16, 2023
@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Nov 16, 2023

Hello. We just upgraded to Tekton v0.47.5 (via OpenShift Pipelines) and now this issue is back!

Our pipeline has defined a result which copies the value of a task result:

  results:
    - description: The URL to the generated junit test report
      name: REPORT_URL
      value: $(finally.generate-report.results.REPORT_URL)

Now if the task generate-report is skipped due to a when condition, the pipeline fails with the following error:

invalid pipelineresults [REPORT_URL], the referred results don't exist

With Tekton v0.44.2 the same pipeline terminated without an error.

So there's are no issues with v0.47.4, just trying to know which release introduce the new change

@thomascube
Copy link
Author

@Yongxuanzhang we upgraded from v0.44.2 to v0.47.5 (this is what the Openshift Pipelines operator has bundled in its releases). Thus I don’t know in which version in between the old behavior came back.

@Yongxuanzhang
Copy link
Member

@Yongxuanzhang we upgraded from v0.44.2 to v0.47.5 (this is what the Openshift Pipelines operator has bundled in its releases). Thus I don’t know in which version in between the old behavior came back.

Ok I will take a look

@thomascube
Copy link
Author

To be more precise, the issue only occurs when results of a skipped task from the finally section are involved. Regular tasks which are skipped to not trigger the said validation error but the following test pipeline does:

apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  generateName: pipelinerun-test-
spec:
  pipelineSpec:
    params:
      - name: say-goobye
        default: 'false'
      - name: do-shutdown
        default: 'false'
    tasks:
      - name: hello
        taskSpec:
          results:
            - name: result-one
          steps:
          - image: alpine
            script: |
              #!/bin/sh
              echo "Hello world!"
              echo -n "RES1" > $(results.result-one.path)
  
      - name: goodbye
        runAfter:
          - hello
        taskSpec:
          results:
            - name: result-two
          steps:
          - image: alpine
            script: |
              #!/bin/sh
              echo "Goodbye world!"
              echo -n "RES2" > $(results.result-two.path)
        when:
          - input: $(params.say-goobye)
            operator: in
            values: ["true"]

    finally:
      - name: shutdown
        taskSpec:
          results:
            - name: result-three
          steps:
          - image: alpine
            script: |
              #!/bin/sh
              echo "Shutdown world!"
              echo -n "RES3" > $(results.result-three.path)
        when:
          - input: $(params.do-shutdown)
            operator: in
            values: ["true"]

    results:
      - name: result-hello
        description: Result one
        value: '$(tasks.hello.results.result-one)'
      - name: result-goodbye
        description: Result two
        value: '$(tasks.goodbye.results.result-two)'
      - name: result-shutdown
        description: Result shutdown
        value: '$(finally.shutdown.results.result-three)'

I'll now test through the versions between v0.44.2 and v0.47.5 to figure out the exaction version the behavior changed.

@thomascube
Copy link
Author

All right, I now tested through the releases and the given example pipeline still succeeds in v0.47.2 but starts failing with v0.47.3. I hope this helps you tracking down the change. It still fails with v0.50.2 though.

@Yongxuanzhang
Copy link
Member

Thanks! Looking into it

@Yongxuanzhang
Copy link
Member

I suspect the skipped finally tasks are not taken into consideration in the previous fix.

@thomascube
Copy link
Author

I suspect the skipped finally tasks are not taken into consideration in the previous fix.

But it worked in versions until v0.47.2...

@Yongxuanzhang
Copy link
Member

I suspect the skipped finally tasks are not taken into consideration in the previous fix.

But it worked in versions until v0.47.2...

There are 2 examples, 1 is your original example without final task, and the other with final task. The one without final task has no issues running on latest main. And I think the final task is not working since the validation was introduced

@Yongxuanzhang
Copy link
Member

Yongxuanzhang commented Nov 23, 2023

@thomascube I will create a fix for this, which Tekton Pipeline Version do we need this fix? I wonder when we have the fix, which major versions we need to patch on to unblock you.
We should support the LTS which are: 44, 47, 50, 53. Is that ok?

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Nov 23, 2023
This commit closes tektoncd#6139, in previous fix PR:
tektoncd#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
@thomascube
Copy link
Author

@thomascube I will create a fix for this, which Tekton Pipeline Version do we need this fix? I wonder when we have the fix, which major versions we need to patch on to unblock you. We should support the LTS which are: 44, 47, 50, 53. Is that ok?

That's perfect. We consume Tekton via OpenShift Pipelines (>= 1.11) where v0.47.x and v0.50.x versions are deployed:
https://docs.openshift.com/pipelines/1.12/about/op-release-notes.html#compatibility-support-matrix_op-release-notes

Yongxuanzhang added a commit to Yongxuanzhang/pipeline that referenced this issue Dec 6, 2023
This commit closes tektoncd#6139, in previous fix PR:
tektoncd#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
tekton-robot pushed a commit that referenced this issue Dec 8, 2023
This commit closes #6139, in previous fix PR:
#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
tekton-robot pushed a commit to tekton-robot/pipeline that referenced this issue Dec 13, 2023
This commit closes tektoncd#6139, in previous fix PR:
tektoncd#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
tekton-robot pushed a commit to tekton-robot/pipeline that referenced this issue Dec 13, 2023
This commit closes tektoncd#6139, in previous fix PR:
tektoncd#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
tekton-robot pushed a commit that referenced this issue Dec 13, 2023
This commit closes #6139, in previous fix PR:
#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
tekton-robot pushed a commit that referenced this issue Dec 13, 2023
This commit closes #6139, in previous fix PR:
#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
vdemeester pushed a commit to vdemeester/tektoncd-pipeline that referenced this issue Dec 13, 2023
This commit closes tektoncd#6139, in previous fix PR:
tektoncd#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
(cherry picked from commit 7c040de)
vdemeester pushed a commit to vdemeester/tektoncd-pipeline that referenced this issue Dec 13, 2023
This commit closes tektoncd#6139, in previous fix PR:
tektoncd#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
(cherry picked from commit 7c040de)
Signed-off-by: Vincent Demeester <[email protected]>
tekton-robot pushed a commit to tekton-robot/pipeline that referenced this issue Dec 20, 2023
This commit closes tektoncd#6139, in previous fix PR:
tektoncd#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
tekton-robot pushed a commit that referenced this issue Dec 20, 2023
This commit closes #6139, in previous fix PR:
#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
tekton-robot pushed a commit that referenced this issue Jan 3, 2024
This commit closes #6139, in previous fix PR:
#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang [email protected]
(cherry picked from commit 7c040de)
Signed-off-by: Vincent Demeester <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
5 participants