fix(pipelinerun): resolve issue with PipelineRun not timing out successfully #8236

l-qing · 2024-09-03T02:48:41Z

When the PipelineRun timeout, validation errors returned when patch a completed TaskRun should be ignored.

Changes

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
pre-commit Passed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

fix(pipelinerun): resolve issue with PipelineRun not timing out successfully

/kind bug

tekton-robot · 2024-09-03T02:48:51Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

tekton-robot · 2024-09-03T03:02:02Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

vdemeester

/ok-to-test
/cc @chitrangpatel

tekton-robot · 2024-09-03T13:12:12Z

[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:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vdemeester · 2024-09-03T13:12:19Z

/cherry-pick release-v0.63.x

tekton-robot · 2024-09-03T13:12:20Z

@vdemeester: once the present PR merges, I will cherry-pick it on top of release-v0.63.x in a new PR and assign it to you.

In response to this:

/cherry-pick release-v0.63.x

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.

tekton-robot · 2024-09-03T13:23:02Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

l-qing · 2024-09-03T14:45:39Z

pipeline/test/timeout_test.go

Lines 197 to 217 in 462771c

    
           failMsg := "\"step-timeout\" exited because the step exceeded the specified timeout limit" 
        
           t.Logf("Waiting for %s in namespace %s to time out", "step-timeout", namespace) 
        
           if err := WaitForTaskRunState(ctx, c, taskRun.Name, FailedWithMessage(failMsg, taskRun.Name), "StepTimeout", v1Version); err != nil { 
        
           	t.Logf("Error in taskRun %s status: %s\n", taskRun.Name, err) 
        
           	t.Errorf("Expected: %s", failMsg) 
        
           } 
        
           tr, err := c.V1TaskRunClient.Get(ctx, taskRun.Name, metav1.GetOptions{}) 
        
           if err != nil { 
        
           	t.Errorf("Error getting Taskrun: %v", err) 
        
           } 
        
           if tr == nil { 
        
           	t.Fatalf("no TaskRun details available") 
        
           } 
        
           if tr.Status.Steps[0].Terminated == nil { 
        
           	t.Errorf("step-no-timeout should have Completed.") 
        
           } else if tr.Status.Steps[0].Terminated.Reason != "Completed" { 
        
           	t.Errorf("step-no-timeout should not have been terminated") 
        
           } 
        
           if tr.Status.Steps[2].Terminated == nil { 
        
           	t.Errorf("step-canceled should have been canceled after step-timeout timed out")

Sometimes, the tr.Status.Steps[2].Terminated is nil.

          - container: step-canceled
            imageID: docker.io/library/busybox@sha256:82742949a3709938cbeb9cec79f5eaf3e48b255389f2dcedf2de29ef96fd841c
            name: canceled
            running:
              startedAt: "2024-09-03T13:30:04Z"

Ref: https://prow.tekton.dev/view/gs/tekton-prow/pr-logs/pull/tektoncd_pipeline/8236/pull-tekton-pipeline-alpha-integration-tests/1830956965430300672

    timeout_test.go:217: step-canceled should have been canceled after step-timeout timed out
    timeout_test.go:221: ############################################
    timeout_test.go:221: ### Dumping objects from arendelle-sq24k ###
    timeout_test.go:221: ############################################
    timeout_test.go:221: 
        ---
        apiVersion: tekton.dev/v1
        kind: TaskRun
        metadata:
          annotations:
            pipeline.tekton.dev/release: 7499202-dirty
          creationTimestamp: "2024-09-03T13:30:00Z"
          generation: 1
          labels:
            app.kubernetes.io/managed-by: tekton-pipelines
          name: step-timeout-taevnmsh
          namespace: arendelle-sq24k
          resourceVersion: "9515"
          uid: 47de3643-fda7-416d-844a-8645e049adac
        spec:
          serviceAccountName: default
          taskSpec:
          timeout: 1h0m0s
        status:
          artifacts: {}
          completionTime: "2024-09-03T13:30:08Z"
          conditions:
          - lastTransitionTime: "2024-09-03T13:30:08Z"
            message: '"step-timeout" exited because the step exceeded the specified timeout
              limit'
            reason: Failed
            status: "False"
            type: Succeeded
          podName: step-timeout-taevnmsh-pod
          provenance:
          startTime: "2024-09-03T13:30:00Z"
          steps:
          - container: step-no-timeout
            imageID: docker.io/library/busybox@sha256:82742949a3709938cbeb9cec79f5eaf3e48b255389f2dcedf2de29ef96fd841c
            name: no-timeout
            terminated:
              containerID: containerd://3bf613441e8bd2c2451c65b72279f9082a4e03e175a735420f77a1a0192c8ad3
              exitCode: 0
              finishedAt: "2024-09-03T13:30:07Z"
              reason: Completed
              startedAt: "2024-09-03T13:30:06Z"
            terminationReason: Completed
          - container: step-timeout
            imageID: docker.io/library/busybox@sha256:82742949a3709938cbeb9cec79f5eaf3e48b255389f2dcedf2de29ef96fd841c
            name: timeout
            terminated:
              containerID: containerd://bebb5264ae4a7452217ed66c2aad4956f4e498bc035605c601f9d8c778d6c930
              exitCode: 1
              finishedAt: "2024-09-03T13:30:07Z"
              reason: Error
              startedAt: "2024-09-03T13:30:07Z"
            terminationReason: TimeoutExceeded
          - container: step-canceled
            imageID: docker.io/library/busybox@sha256:82742949a3709938cbeb9cec79f5eaf3e48b255389f2dcedf2de29ef96fd841c
            name: canceled
            running:
              startedAt: "2024-09-03T13:30:04Z"
          taskSpec:
            steps:

l-qing · 2024-09-04T02:04:02Z

Many times, automation fails due to an unstable known e2e.

fix(e2e): stabilize TestTaskRunFailure test #8174

tekton-robot · 2024-09-05T14:25:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

tekton-robot · 2024-09-05T14:27:01Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

tekton-robot · 2024-09-06T04:09:50Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

tekton-robot · 2024-09-06T04:21:55Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

tekton-robot · 2024-09-13T02:23:57Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

tekton-robot · 2024-09-13T02:27:27Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

l-qing · 2024-09-13T03:53:46Z

/retest

l-qing · 2024-09-13T07:41:54Z

/retest

l-qing · 2024-09-13T07:42:50Z

The instability in this integration test is related to another PR #8171.

It's mainly caused by the informer cache not updating in a timely manner. I'll look into how to completely avoid this issue within a week.

tekton-robot · 2024-09-17T12:45:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

tekton-robot · 2024-09-17T12:46:37Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

…ssfully fix tektoncd#8230 When the PipelineRun timeout, validation errors returned when patch a completed TaskRun should be ignored.

tekton-robot · 2024-09-19T14:01:01Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

tekton-robot · 2024-09-19T14:02:09Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/errors/errors.go	100.0%	92.3%	-7.7
pkg/reconciler/pipelinerun/timeout.go	89.7%	87.8%	-1.9

l-qing · 2024-09-20T03:48:11Z

/retest

l-qing · 2024-09-20T04:33:23Z

/retest

l-qing · 2024-09-20T10:19:01Z

/retest

afrittoli

Thank you!
/lgtm

l-qing · 2024-09-24T15:02:14Z

/retest

l-qing · 2024-09-24T15:34:52Z

/retest

l-qing · 2024-09-24T16:03:39Z

/retest

tekton-robot · 2024-09-24T16:32:56Z

@vdemeester: new pull request created: #8292

In response to this:

/cherry-pick release-v0.63.x

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.

vdemeester · 2024-11-06T12:54:53Z

/cherry-pick release-v0.62.x

tekton-robot · 2024-11-06T12:55:41Z

@vdemeester: #8236 failed to apply on top of branch "release-v0.62.x":

Applying: fix(pipelinerun): resolve issue with PipelineRun not timing out successfully
Using index info to reconstruct a base tree...
M	pkg/reconciler/pipelinerun/cancel.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/reconciler/pipelinerun/cancel.go
CONFLICT (content): Merge conflict in pkg/reconciler/pipelinerun/cancel.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix(pipelinerun): resolve issue with PipelineRun not timing out successfully
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.62.x

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.

l-qing · 2024-11-06T13:37:51Z

/cherry-pick release-v0.62.x

I saw that the automatic cherry-pick bot reported a merge conflict error. I manually created a PR.
Could you please help review it when you have time? Thank you!

#8376

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Sep 3, 2024

tekton-robot requested review from jerop and pritidesai September 3, 2024 02:48

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 3, 2024

vdemeester approved these changes Sep 3, 2024

View reviewed changes

tekton-robot requested a review from chitrangpatel September 3, 2024 13:12

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 3, 2024

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2024

l-qing force-pushed the fix/pipelinerun-timeout-issue branch from 7e3a45b to 3856add Compare September 5, 2024 14:13

l-qing force-pushed the fix/pipelinerun-timeout-issue branch from 3856add to 4737501 Compare September 6, 2024 03:59

l-qing force-pushed the fix/pipelinerun-timeout-issue branch from 4737501 to dac0c48 Compare September 13, 2024 02:11

l-qing mentioned this pull request Sep 17, 2024

fix(TaskRun): fixed the issue where some step statuses might not be correctly updated in failed TaskRun #8270

Merged

8 tasks

l-qing force-pushed the fix/pipelinerun-timeout-issue branch from dac0c48 to 9a0cad2 Compare September 17, 2024 12:30

fix(pipelinerun): resolve issue with PipelineRun not timing out succe…

08d78e8

…ssfully fix tektoncd#8230 When the PipelineRun timeout, validation errors returned when patch a completed TaskRun should be ignored.

l-qing force-pushed the fix/pipelinerun-timeout-issue branch from 9a0cad2 to 08d78e8 Compare September 19, 2024 13:50

afrittoli added this to the Pipeline v0.64 milestone Sep 23, 2024

afrittoli reviewed Sep 24, 2024

View reviewed changes

tekton-robot assigned afrittoli Sep 24, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2024

tekton-robot merged commit 63a1667 into tektoncd:main Sep 24, 2024
14 checks passed

tekton-robot mentioned this pull request Sep 24, 2024

[release-v0.63.x] fix(pipelinerun): resolve issue with PipelineRun not timing out successfully #8292

Merged

l-qing deleted the fix/pipelinerun-timeout-issue branch September 25, 2024 08:32

l-qing mentioned this pull request Nov 6, 2024

fix(pipelinerun): resolve issue with PipelineRun not timing out successfully #8376

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pipelinerun): resolve issue with PipelineRun not timing out successfully #8236

fix(pipelinerun): resolve issue with PipelineRun not timing out successfully #8236

l-qing commented Sep 3, 2024

tekton-robot commented Sep 3, 2024

tekton-robot commented Sep 3, 2024

vdemeester left a comment

tekton-robot commented Sep 3, 2024

vdemeester commented Sep 3, 2024

tekton-robot commented Sep 3, 2024

tekton-robot commented Sep 3, 2024

l-qing commented Sep 3, 2024

l-qing commented Sep 4, 2024

tekton-robot commented Sep 5, 2024

tekton-robot commented Sep 5, 2024

tekton-robot commented Sep 6, 2024

tekton-robot commented Sep 6, 2024

tekton-robot commented Sep 13, 2024

tekton-robot commented Sep 13, 2024

l-qing commented Sep 13, 2024

l-qing commented Sep 13, 2024

l-qing commented Sep 13, 2024

tekton-robot commented Sep 17, 2024

tekton-robot commented Sep 17, 2024

tekton-robot commented Sep 19, 2024

tekton-robot commented Sep 19, 2024

l-qing commented Sep 20, 2024

l-qing commented Sep 20, 2024

l-qing commented Sep 20, 2024

afrittoli left a comment

l-qing commented Sep 24, 2024

l-qing commented Sep 24, 2024

l-qing commented Sep 24, 2024

tekton-robot commented Sep 24, 2024

vdemeester commented Nov 6, 2024

tekton-robot commented Nov 6, 2024

l-qing commented Nov 6, 2024

fix(pipelinerun): resolve issue with PipelineRun not timing out successfully #8236

fix(pipelinerun): resolve issue with PipelineRun not timing out successfully #8236

Conversation

l-qing commented Sep 3, 2024

Changes

Submitter Checklist

Release Notes

tekton-robot commented Sep 3, 2024

tekton-robot commented Sep 3, 2024

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 3, 2024

vdemeester commented Sep 3, 2024

tekton-robot commented Sep 3, 2024

tekton-robot commented Sep 3, 2024

l-qing commented Sep 3, 2024

l-qing commented Sep 4, 2024

tekton-robot commented Sep 5, 2024

tekton-robot commented Sep 5, 2024

tekton-robot commented Sep 6, 2024

tekton-robot commented Sep 6, 2024

tekton-robot commented Sep 13, 2024

tekton-robot commented Sep 13, 2024

l-qing commented Sep 13, 2024

l-qing commented Sep 13, 2024

l-qing commented Sep 13, 2024

tekton-robot commented Sep 17, 2024

tekton-robot commented Sep 17, 2024

tekton-robot commented Sep 19, 2024

tekton-robot commented Sep 19, 2024

l-qing commented Sep 20, 2024

l-qing commented Sep 20, 2024

l-qing commented Sep 20, 2024

afrittoli left a comment

Choose a reason for hiding this comment

l-qing commented Sep 24, 2024

l-qing commented Sep 24, 2024

l-qing commented Sep 24, 2024

tekton-robot commented Sep 24, 2024

vdemeester commented Nov 6, 2024

tekton-robot commented Nov 6, 2024

l-qing commented Nov 6, 2024