fix: the pr may lose finallyStartTime when pipeline controller is not synchronized to all current state #7186

cugykw · 2023-10-08T11:34:40Z

Changes

Prior to this PR, the informer of the pipeline controller is not synchronized to the latest pipelinerun resources (including finallyStartTime), but is synchronized to the latest taskrun resources, resulting in finallyStartTime not being reset. The reconcile function is processed and pipelinerun is updated, then finallyStartTime may be lost.

This commit will try to set the finallyStartTime field when one or more final tasks has been created and current finallyStartTime is empty.

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

Reset the finallyStartTime field when one or more final tasks have been created and the current finallyStartTime is empty.

/kind bug

tekton-robot · 2023-10-08T11:34:49Z

Hi @cugykw. 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 · 2023-10-08T11:43:08Z

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/reconciler/pipelinerun/pipelinerun.go	92.4%	92.4%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	96.5%	-3.0

khrm

/ok-to-test

tekton-robot · 2023-10-08T17:06:40Z

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/reconciler/pipelinerun/pipelinerun.go	92.4%	92.4%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	96.5%	-3.0

cugykw · 2023-10-09T00:58:08Z

/retest

tekton-robot · 2023-10-09T01:03:22Z

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/reconciler/pipelinerun/pipelinerun.go	92.4%	92.4%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	96.5%	-3.0

tekton-robot · 2023-10-09T01:05:48Z

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/reconciler/pipelinerun/pipelinerun.go	92.4%	92.4%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	96.5%	-3.0

tekton-robot · 2023-10-09T11:20:33Z

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/reconciler/pipelinerun/pipelinerun.go	92.4%	92.4%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-10-09T11:21:43Z

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/reconciler/pipelinerun/pipelinerun.go	92.4%	92.4%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

cugykw · 2023-10-27T02:04:00Z

/assign @jerop @vdemeester

tekton-robot · 2023-10-27T02:09:17Z

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/reconciler/pipelinerun/pipelinerun.go	92.5%	92.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-10-27T02:21:08Z

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/reconciler/pipelinerun/pipelinerun.go	92.5%	92.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-11-02T11:18:40Z

[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

tekton-robot · 2023-11-09T01:17:54Z

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/reconciler/pipelinerun/pipelinerun.go	92.5%	92.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-11-09T01:20:06Z

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/reconciler/pipelinerun/pipelinerun.go	92.5%	92.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

Yongxuanzhang · 2023-11-15T21:39:54Z

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

+			if facts.isFinalTask(t.PipelineTask.Name) {
+				if len(t.TaskRuns) > 0 || len(t.CustomRuns) > 0 {
+					return true
+				}


I wonder if this is equal to

t.IsRunning()

I wonder if this is equal to

t.IsRunning()

Thanks for your tip, I adjusted and used the t.isScheduled() function to replace it. IsRunning may not be particularly accurate as it excludes the end state.

Oh nice, thanks!

Yongxuanzhang · 2023-11-15T21:40:37Z

So sorry for the late review. 😅

… synchronized to all current state

tekton-robot · 2023-11-16T05:00:00Z

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/reconciler/pipelinerun/pipelinerun.go	92.5%	92.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

tekton-robot · 2023-11-16T05:00:19Z

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/reconciler/pipelinerun/pipelinerun.go	92.5%	92.5%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunstate.go	99.5%	99.5%	0.0

cugykw · 2023-11-16T06:05:13Z

/test pull-tekton-pipeline-alpha-integration-tests

Yongxuanzhang · 2023-11-16T15:34:41Z

/lgtm

tekton-robot requested review from imjasonh and wlynch October 8, 2023 11:34

khrm reviewed Oct 8, 2023

View reviewed changes

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 Oct 8, 2023

cugykw force-pushed the fix-final-start-time branch from 565a111 to 415914f Compare October 9, 2023 00:57

cugykw force-pushed the fix-final-start-time branch from 415914f to d5bdb3b Compare October 9, 2023 11:14

cugykw force-pushed the fix-final-start-time branch from d5bdb3b to 1593085 Compare October 27, 2023 02:02

tekton-robot assigned jerop and vdemeester Oct 27, 2023

wlynch removed their request for review October 30, 2023 17:02

vdemeester approved these changes Nov 2, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2023

cugykw force-pushed the fix-final-start-time branch from 1593085 to 20b2cbf Compare November 9, 2023 01:10

jerop added this to the Pipelines v0.54 milestone Nov 9, 2023

Yongxuanzhang reviewed Nov 15, 2023

View reviewed changes

fix: the pr may lose finallyStartTime when pipeline controller is not…

a75fb63

… synchronized to all current state

cugykw force-pushed the fix-final-start-time branch from 20b2cbf to a75fb63 Compare November 16, 2023 04:51

tekton-robot assigned Yongxuanzhang Nov 16, 2023

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2023

tekton-robot merged commit 23581c5 into tektoncd:main Nov 16, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: the pr may lose finallyStartTime when pipeline controller is not synchronized to all current state #7186

fix: the pr may lose finallyStartTime when pipeline controller is not synchronized to all current state #7186

cugykw commented Oct 8, 2023 •

edited

Loading

tekton-robot commented Oct 8, 2023

tekton-robot commented Oct 8, 2023

khrm left a comment

tekton-robot commented Oct 8, 2023

cugykw commented Oct 9, 2023

tekton-robot commented Oct 9, 2023

tekton-robot commented Oct 9, 2023

tekton-robot commented Oct 9, 2023

tekton-robot commented Oct 9, 2023

cugykw commented Oct 27, 2023

tekton-robot commented Oct 27, 2023

tekton-robot commented Oct 27, 2023

tekton-robot commented Nov 2, 2023

tekton-robot commented Nov 9, 2023

tekton-robot commented Nov 9, 2023

Yongxuanzhang Nov 15, 2023

cugykw Nov 16, 2023

Yongxuanzhang Nov 16, 2023

Yongxuanzhang commented Nov 15, 2023

tekton-robot commented Nov 16, 2023

tekton-robot commented Nov 16, 2023

cugykw commented Nov 16, 2023

Yongxuanzhang commented Nov 16, 2023

fix: the pr may lose finallyStartTime when pipeline controller is not synchronized to all current state #7186

fix: the pr may lose finallyStartTime when pipeline controller is not synchronized to all current state #7186

Conversation

cugykw commented Oct 8, 2023 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Oct 8, 2023

tekton-robot commented Oct 8, 2023

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 8, 2023

cugykw commented Oct 9, 2023

tekton-robot commented Oct 9, 2023

tekton-robot commented Oct 9, 2023

tekton-robot commented Oct 9, 2023

tekton-robot commented Oct 9, 2023

cugykw commented Oct 27, 2023

tekton-robot commented Oct 27, 2023

tekton-robot commented Oct 27, 2023

tekton-robot commented Nov 2, 2023

tekton-robot commented Nov 9, 2023

tekton-robot commented Nov 9, 2023

Yongxuanzhang Nov 15, 2023

Choose a reason for hiding this comment

cugykw Nov 16, 2023

Choose a reason for hiding this comment

Yongxuanzhang Nov 16, 2023

Choose a reason for hiding this comment

Yongxuanzhang commented Nov 15, 2023

tekton-robot commented Nov 16, 2023

tekton-robot commented Nov 16, 2023

cugykw commented Nov 16, 2023

Yongxuanzhang commented Nov 16, 2023

cugykw commented Oct 8, 2023 •

edited

Loading