Only fetch the definitions once 🧙 #3941

vdemeester · 2021-05-12T11:03:28Z

Changes

Since we "dereference" fetched definition and store them into the
status, we can use this as a source of truth. This allows us to
fetch only once the definitions (Task, Pipeline) and has the benefit
to make the runs (PipelineRun, TaskRun) immune to change to what they
refer, while the are executing.

Without this change, if you run a PipelineRun that reference a
Pipeline that is deleted during execution, your PipelineRun will fail
at some point because it cannot fetch the definition anymore, even if
it stored them in its status. This is the case for TaskRun too.

Fixes #3916

Signed-off-by: Vincent Demeester [email protected]

/kind bug

Submitter Checklist

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

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Only fetch the definition once, and then used the spec stored in the status as source of truth. 
This reduce the probable race condition when a `PipelineRun` or a `TaskRun` refers to a `Pipeline` or `Task` that changes during its execution.

vdemeester · 2021-05-12T11:04:56Z

This probably needs some more discussions (in the API WG or somewhere) and some e2e tests.

vdemeester · 2021-05-12T11:05:48Z

/cc @tektoncd/core-maintainers

tekton-robot · 2021-05-12T11:06: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/reconciler/pipelinerun/resources/pipelineref.go	66.7%	63.0%	-3.7
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	91.8%	-0.3
pkg/reconciler/taskrun/resources/taskref.go	75.8%	62.5%	-13.3

vdemeester · 2021-05-12T12:02:24Z

One thing to discuss here too is related to this comment : should this be behind a configuration ? (and if yes, what should be the default behavior)

ghost · 2021-05-12T12:43:19Z

This looks good to me! Agree it needs testing but otherwise I'm happy to see this move forward.

I don't think this specific PR should be behind a flag. From my pov this behaviour should simply be the way it works: if the specs are cached then we use them. A configuration option to turn off spec caching is then a separate feature request.

tekton-robot · 2021-05-17T14:48: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/resources/pipelineref.go	66.7%	63.0%	-3.7
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	91.8%	-0.3

tekton-robot · 2021-05-17T15:23:47Z

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/resources/pipelineref.go	66.7%	63.0%	-3.7
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	91.8%	-0.3
pkg/reconciler/taskrun/resources/taskref.go	75.8%	61.0%	-14.8

ghost · 2021-05-17T15:58:10Z

/test pull-tekton-pipeline-integration-tests

ghost · 2021-05-17T18:10:37Z

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

ghost · 2021-05-18T18:54:18Z

test/pipelinerun_test.go


+// TestPipelineRunRefDeleted tests that a running PipelineRun doesn't fail when the Pipeline
+// it references is deleted.
+func TestPipelineRunRefDeletede(t *testing.T) {


nit: Deletede -> Deleted

tekton-robot · 2021-05-18T20:04:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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~~ [sbwsg]

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

pkg/reconciler/pipelinerun/resources/pipelineref.go

pritidesai · 2021-06-03T21:32:19Z

pkg/reconciler/pipelinerun/resources/pipelineref.go

+				Spec: *pipelineRun.Status.PipelineSpec,
+			}, nil
+		}, nil
+	}


can we please add a unit test for this? 🥺

So this function is implemented to fetch the definition from the cluster or bundle reference but we are checking if the pipeline is already running i.e. status is not nil, return the stored specifications irrespective of bundle or no bundle.

done 😉
Yes, the pipeline doesn't really have to run though, just the pipelineSpec in status being populated

pritidesai · 2021-06-03T22:46:30Z

thanks @vdemeester the changes look great and if I can recollect the specifications are stored in the status after resolving params so they are ready to use as is after fetching. One request, it will be great to see a unit test for changes in pipelineRef.go

Also, we might have to apply similar changes to run in addition to taskRun, not necessarily in this PR, can be a separate effort.

Edit: Or rather the custom controller is responsible for the implementation 🤔

tekton-robot · 2021-06-04T12:26:19Z

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/resources/pipelineref.go	66.7%	70.4%	3.7
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.2%	91.9%	-0.3
pkg/reconciler/taskrun/resources/taskref.go	75.8%	61.0%	-14.8

tekton-robot · 2021-06-04T12:49:51Z

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/resources/pipelineref.go	66.7%	70.4%	3.7
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.2%	91.9%	-0.3
pkg/reconciler/taskrun/resources/taskref.go	75.8%	68.3%	-7.5

Since we "dereference" fetched definition and store them into the `status`, we can use this as a source of truth. This allows us to fetch only once the definitions (Task, Pipeline) *and* has the benefit to make the runs (PipelineRun, TaskRun) immune to change to what they refer, while the are executing. Without this change, if you run a PipelineRun that reference a Pipeline that is deleted during execution, your PipelineRun will fail at some point because it cannot fetch the definition anymore, even if it stored them in its status. This is the case for TaskRun too. Signed-off-by: Vincent Demeester <[email protected]>

tekton-robot · 2021-06-04T12:53:49Z

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/resources/pipelineref.go	66.7%	70.4%	3.7
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.2%	91.9%	-0.3
pkg/reconciler/taskrun/resources/taskref.go	75.8%	68.3%	-7.5

pritidesai · 2021-06-04T17:40:44Z

thanks @vdemeester

/lgtm

pritidesai · 2021-06-04T18:12:20Z

/test pull-tekton-pipeline-integration-tests

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 May 12, 2021

tekton-robot requested review from dibyom and jerop May 12, 2021 11:03

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 12, 2021

tekton-robot requested a review from a team May 12, 2021 11:05

vdemeester force-pushed the 3916-fetch-only-when-needed branch from 2d7a276 to ac6ee04 Compare May 17, 2021 14:45

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 17, 2021

vdemeester changed the title ~~wip: Only fetch the definitions once 🧙~~ Only fetch the definitions once 🧙 May 17, 2021

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 17, 2021

vdemeester force-pushed the 3916-fetch-only-when-needed branch from ac6ee04 to 8f9bce9 Compare May 17, 2021 15:20

ghost approved these changes May 18, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2021

pritidesai reviewed May 19, 2021

View reviewed changes

pkg/reconciler/pipelinerun/resources/pipelineref.go Show resolved Hide resolved

pritidesai added this to the Pipelines v0.25 milestone Jun 2, 2021

pritidesai reviewed Jun 3, 2021

View reviewed changes

vdemeester force-pushed the 3916-fetch-only-when-needed branch from 8f9bce9 to 016c1f6 Compare June 4, 2021 12:24

vdemeester force-pushed the 3916-fetch-only-when-needed branch from 016c1f6 to 0733392 Compare June 4, 2021 12:47

vdemeester force-pushed the 3916-fetch-only-when-needed branch from 0733392 to 750cb2e Compare June 4, 2021 12:52

tekton-robot assigned pritidesai Jun 4, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2021

tekton-robot merged commit 7ceaffb into tektoncd:main Jun 4, 2021

vdemeester deleted the 3916-fetch-only-when-needed branch June 7, 2021 09:52

pritidesai mentioned this pull request Jul 28, 2021

adding label memberOf to identify a taskRun in the pipeline #4121

Merged

5 tasks

vdemeester mentioned this pull request Jan 13, 2022

Annotations and Labels from Task are not propagated to the Pod #4477

Closed

pritidesai mentioned this pull request Feb 10, 2022

avoid validating task specs when read from the taskRun status #4562

Closed

5 tasks

Uh oh!

Only fetch the definitions once 🧙 #3941

Only fetch the definitions once 🧙 #3941

Uh oh!

Conversation

vdemeester commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

vdemeester commented May 12, 2021

Uh oh!

vdemeester commented May 12, 2021

Uh oh!

tekton-robot commented May 12, 2021

Uh oh!

vdemeester commented May 12, 2021

Uh oh!

ghost commented May 12, 2021

Uh oh!

tekton-robot commented May 17, 2021

Uh oh!

tekton-robot commented May 17, 2021

Uh oh!

ghost commented May 17, 2021

Uh oh!

ghost commented May 17, 2021

Uh oh!

ghost May 18, 2021

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented May 18, 2021

Uh oh!

Uh oh!

pritidesai Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

pritidesai Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

vdemeester Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

pritidesai commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tekton-robot commented Jun 4, 2021

Uh oh!

tekton-robot commented Jun 4, 2021

Uh oh!

tekton-robot commented Jun 4, 2021

Uh oh!

pritidesai commented Jun 4, 2021

Uh oh!

pritidesai commented Jun 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vdemeester commented May 12, 2021 •

edited

Loading

pritidesai commented Jun 3, 2021 •

edited

Loading