Only make a PVC for a PipelineRun if we need to. 🗑️ #1545

dlorenc · 2019-11-08T17:58:33Z

Changes

We only need to make a PVC if a Pipeline contains tasks that declare Output
resources of the allowed types. This PR changes our detection to only create
a PVC under those conditions.

This was originally attempted in #1007 and
then subsequently rolled back in #1071. I
think this one gets the logic correct :)

Co-authored-by: Christie Wilson [email protected]
Fixes: #937

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

PVCs for output -> input linking will now be created only when that linking is actually present in a Pipeline, fixing #937 (used to be created for all Pipelines that included any Task with an Output).

googlebot · 2019-11-08T17:58:36Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

tekton-robot · 2019-11-08T18:01:29Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/artifacts_storage.go	81.5%	80.7%	-0.8
pkg/reconciler/taskrun/resources/output_resource.go	89.7%	90.5%	0.7

tekton-robot · 2019-11-08T18:02:35Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/artifacts_storage.go	81.5%	80.7%	-0.8
pkg/reconciler/taskrun/resources/output_resource.go	89.7%	90.5%	0.7

bobcatfish

Amazing!!! 😎

@dlorenc the one thing I can't verify is if after #1068 we made sure to add an example and/or integration test that covered that case (a pipeline with one task that had an output) b/c i feel like before we merge this we should be sure that we don't cause that bug to come back (reasoning through it i'm pretty sure we won't but just in case!)

/approve

bobcatfish · 2019-11-08T18:17:42Z

pkg/reconciler/taskrun/resources/output_resource.go

 			var newSteps []v1alpha1.Step
 			for _, dPath := range boundResource.Paths {
 				newSteps = append(newSteps, as.GetCopyToStorageFromSteps(resource.GetName(), sourcePath, dPath)...)
+				needsPvc = true


nice, i think this is the key that makes it work which we were missing in our previous attempts @dlorenc ?

b/c in the first attempt we didnt change this at all, and then in #1069 my massive hack was too hacky - i was looking to see if the PVC existed when actually we could just check if this path was bound 🤦‍♀️

One detail is that we had sort of intended that path be something that could be generically provided by users - but im pretty sure that actually didn't work (and hope we'll remove it forever one day soon 🙏 ) and this block should only be hit in the PipelineRun case anyway, in which case there's no way for the user to provide the value sooooo

long story short 🎉 🎉 🎉

googlebot · 2019-11-08T18:20:12Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

We only need to make a PVC if a Pipeline contains tasks that declare Output resources of the allowed types. This PR changges our detection to only create a PVC under those conditions. This was originally attempted in tektoncd#1007 and then subsequently rolled back in tektoncd#1071. I *think* this one gets the logic correct :) Co-authored-by: Christie Wilson <[email protected]> Fixes: tektoncd#937

googlebot · 2019-11-08T20:50:05Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

tekton-robot · 2019-11-08T20:52:14Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/artifacts_storage.go	81.5%	80.7%	-0.8
pkg/reconciler/taskrun/resources/output_resource.go	89.7%	90.5%	0.7

googlebot · 2019-11-08T21:08:06Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

dibyom

/lgtm

tekton-robot · 2019-11-12T21:27:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, dibyom

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~~ [bobcatfish,dibyom]

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

googlebot added the cla: no label Nov 8, 2019

tekton-robot requested review from abayer and afrittoli November 8, 2019 17:58

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 8, 2019

dlorenc force-pushed the fixpvc branch from 60d3057 to c25d6a3 Compare November 8, 2019 17:59

bobcatfish reviewed Nov 8, 2019

View reviewed changes

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

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Nov 8, 2019

dlorenc force-pushed the fixpvc branch from c25d6a3 to 877086c Compare November 8, 2019 20:50

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Nov 8, 2019

dlorenc added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Nov 8, 2019

dibyom approved these changes Nov 12, 2019

View reviewed changes

tekton-robot assigned dibyom Nov 12, 2019

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

tekton-robot merged commit ffadda1 into tektoncd:master Nov 12, 2019

dlorenc deleted the fixpvc branch November 13, 2019 13:37

bobcatfish mentioned this pull request Feb 18, 2020

Tekton Creates PVC unnecessarily if volumes are defined #937

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only make a PVC for a PipelineRun if we need to. 🗑️ #1545

Only make a PVC for a PipelineRun if we need to. 🗑️ #1545

dlorenc commented Nov 8, 2019 •

edited by bobcatfish

Loading

googlebot commented Nov 8, 2019

tekton-robot commented Nov 8, 2019

tekton-robot commented Nov 8, 2019

bobcatfish left a comment

bobcatfish Nov 8, 2019

googlebot commented Nov 8, 2019

googlebot commented Nov 8, 2019

tekton-robot commented Nov 8, 2019

googlebot commented Nov 8, 2019

dibyom left a comment

tekton-robot commented Nov 12, 2019

Only make a PVC for a PipelineRun if we need to. 🗑️ #1545

Only make a PVC for a PipelineRun if we need to. 🗑️ #1545

Conversation

dlorenc commented Nov 8, 2019 • edited by bobcatfish Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

googlebot commented Nov 8, 2019

tekton-robot commented Nov 8, 2019

tekton-robot commented Nov 8, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Nov 8, 2019

Choose a reason for hiding this comment

googlebot commented Nov 8, 2019

googlebot commented Nov 8, 2019

tekton-robot commented Nov 8, 2019

googlebot commented Nov 8, 2019

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented Nov 12, 2019

dlorenc commented Nov 8, 2019 •

edited by bobcatfish

Loading