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

If no resources are linked, don't make a PVC πŸ—‘οΈ #1007

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

dlorenc
Copy link
Contributor

@dlorenc dlorenc commented Jun 25, 2019

Changes

When outputs of a Task are linked to the inputs of another we create a
PVC (or use a bucket) to share the data between executing Tasks. However
we were doing this even when no Resources were linked! Now we only do it
when we need to.

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:

Release Notes

Fixed an issue that caused PVCs to be created in some PipelineRuns when they were not necessary.

@googlebot
Copy link

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 have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2019
@dlorenc 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 Jun 25, 2019
@googlebot
Copy link

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.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good ! Just one nit comment πŸ‘Ό

pkg/artifacts/artifacts_storage.go Outdated Show resolved Hide resolved
@abayer
Copy link
Contributor

abayer commented Jun 25, 2019

/lgtm
yay!

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2019
When ouputs of a Task are linked to the inputs of another we create a
PVC (or use a bucket) to share the data between executing Tasks. However
we were doing this even when no Resources were linked! Now we only do it
when we need to.

Co-authored-by: Christie Wilson <[email protected]>
Fixes: tektoncd#937
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2019
@googlebot
Copy link

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 have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot 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 Jun 26, 2019
@dlorenc 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 Jun 26, 2019
@googlebot
Copy link

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.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dlorenc, 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:

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

@tekton-robot tekton-robot merged commit 725d6f8 into tektoncd:master Jun 26, 2019
bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Jul 12, 2019
In tektoncd#1007 @dlorenc and I tried to fix the case where a PVC wasn't needed
for output -> input linking and it was being created anyway. What we
forgot to do was check to see where that PVC was being mounted. It turns
out that if a TaskRun has an output and is created by a PipelineRun
(this is checked via the owner reference), the TaskRun assumes it needs
to mount the volume and further adds containers to copy the output data
to the (possibly) non-existent PVC. @castlemilk caught this problem in tektoncd#1068.

The real fix here is probably going to involve an interface change b/c
we can't assume that just being owned by a PipelineRun means that a
linking PVC is going to be involved. This commit is a terrible and
probably race condition hack to make it so that if the PVC the TaskRun
is expecting doesn't exist, it doesn't attempt to add containers that
will copy data to it.

Making the hack even worse is that instead of adding more actual unit
tests, I updated the test to run all the existing unit tests twice, once
with this PVC existing and once with it not, and I manipulated the test
so that in the case where it doesn't exist, the expected outcome is
different. This is a terrible way to write tests and I hope we either
don't merge this or we fix it quickly afterward. @dlorenc and I are
going to work on a better fix tomorrow.

I also modified our end to end PipelineRun test to include an output
resource so we could reproduce the issue that @castlemilk reported.
bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Jul 12, 2019
In tektoncd#1007 @dlorenc and I tried to fix the case where a PVC wasn't needed
for output -> input linking and it was being created anyway. What we
forgot to do was check to see where that PVC was being mounted. It turns
out that if a TaskRun has an output and is created by a PipelineRun
(this is checked via the owner reference), the TaskRun assumes it needs
to mount the volume and further adds containers to copy the output data
to the (possibly) non-existent PVC. @castlemilk caught this problem in tektoncd#1068.

The real fix here is probably going to involve an interface change b/c
we can't assume that just being owned by a PipelineRun means that a
linking PVC is going to be involved. This commit is a terrible and
probably race condition hack to make it so that if the PVC the TaskRun
is expecting doesn't exist, it doesn't attempt to add containers that
will copy data to it.

Making the hack even worse is that instead of adding more actual unit
tests, I updated the test to run all the existing unit tests twice, once
with this PVC existing and once with it not, and I manipulated the test
so that in the case where it doesn't exist, the expected outcome is
different. This is a terrible way to write tests and I hope we either
don't merge this or we fix it quickly afterward. @dlorenc and I are
going to work on a better fix tomorrow.

I also modified our end to end PipelineRun test to include an output
resource so we could reproduce the issue that @castlemilk reported.
@dlorenc dlorenc deleted the nonestore branch November 8, 2019 17:57
dlorenc added a commit to dlorenc/build-pipeline that referenced this pull request Nov 8, 2019
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
dlorenc added a commit to dlorenc/build-pipeline that referenced this pull request Nov 8, 2019
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
dlorenc added a commit to dlorenc/build-pipeline that referenced this pull request Nov 8, 2019
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
tekton-robot pushed a commit that referenced this pull request Nov 12, 2019
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 #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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tekton Creates PVC unnecessarily if volumes are defined
5 participants