Massive hack to prevent attempted mount of non-existent PVC 💀 #1069
Conversation
googlebot
added
the
cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobcatfish 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 |
tekton-robot
added
approved
|
p.s. if we decide not to merge this no hard feelings at all; tomorrow @dlorenc and I are going to see if we can add a better fix quickly, if not, this or something like it could be an okay (ugh) interim solution |
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: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@bobcatfish @dlorenc is it better to add this hack in 0.5.x or try to revert that feature (in 0.5.x) and rework it better for next release ? 👼 |
|
omg reverting it is a way better idea XD i should have just done that last night instead of spending a whole evening making this monstrosity (it was kind of fun tho? 😅 )
If we're gonna fix stuff in feature branches i think it's important we make the same change in master otherwise itll be hard to keep track of what is where |
|
Closing this in favor of #1071 |
Changes
In #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 #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.
p.s. the change to output_resource_test isn't as bad as it looks but the fact that everything is wrapped in a for loop really confuses the diff tools :(
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task and TaskRun to build and release this image
Release Notes