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

Tekton Creates PVC unnecessarily if volumes are defined #937

Closed
mustafaakin opened this issue May 31, 2019 · 6 comments · Fixed by #1007 or #1545
Closed

Tekton Creates PVC unnecessarily if volumes are defined #937

mustafaakin opened this issue May 31, 2019 · 6 comments · Fixed by #1007 or #1545
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@mustafaakin
Copy link

Expected Behavior

I have task with no inputs or outputs. It should not have any PVCs.

Actual Behavior

But a PVC is created if the Task spec has volumes in it.

Steps to Reproduce the Problem

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: msg
spec:
  steps:
  - name: msg
    image: alpine
    command:
    - echo
    args:
    - slm
  volumes: []
---
apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: demo-pipeline
spec:
  tasks:
  - name: test-1
    taskRef:
      name: msg
---
apiVersion: tekton.dev/v1alpha1
kind: PipelineRun
metadata:
  name: tst-run
spec:
  pipelineRef:
    name: demo-pipeline
$  kubectl apply -f test-1.yaml
task.tekton.dev/msg created
pipeline.tekton.dev/demo-pipeline created
pipelinerun.tekton.dev/tst-run created
$ kubectl get pvc
NAME          STATUS    VOLUME   CAPACITY   ACCESS MODES   STORAGECLASS   AGE
tst-run-pvc   Pending                                      standard       2s

Additional Info

tekton 0.4

@abayer abayer added the kind/bug Categorizes issue or PR as related to a bug. label May 31, 2019
@abayer abayer self-assigned this May 31, 2019
@bobcatfish
Copy link
Collaborator

I see the same thing:

➜  pipeline git:(stepTemplate) ✗ k get pvc          
NAME          STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
tst-run-pvc   Bound    pvc-ab83fef7-83f9-11e9-8c6b-42010a800ff4   5Gi        RWO            standard       6s

@bobcatfish bobcatfish added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels May 31, 2019
dlorenc added a commit to dlorenc/build-pipeline that referenced this issue 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 <bobcatfish@gmail.com>
Fixes: tektoncd#937
dlorenc added a commit to dlorenc/build-pipeline that referenced this issue Jun 26, 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 <bobcatfish@gmail.com>
Fixes: tektoncd#937
tekton-robot pushed a commit that referenced this issue Jun 26, 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 <bobcatfish@gmail.com>
Fixes: #937
@bobcatfish
Copy link
Collaborator

Re-opening this b/c we had a massive bug (#1068) in our first attempt to fix this! 😅 🙏

@bobcatfish bobcatfish reopened this Jul 12, 2019
@bobcatfish bobcatfish assigned bobcatfish and dlorenc and unassigned abayer Jul 12, 2019
@bobcatfish
Copy link
Collaborator

Quick update: @dlorenc and I have been looking into this, and instead of tackling it directly, want to propose some updates to the way we've designed input + output linking. Stay tuned!

@bobcatfish
Copy link
Collaborator

I'm going to close this in favor of #1076: this will remain a known issue for the moment, in the meantime we will work on #1076 and remove this problem by changing the way resource linking works.

dlorenc added a commit to dlorenc/build-pipeline that referenced this issue Nov 8, 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 <bobcatfish@gmail.com>
Fixes: tektoncd#937
dlorenc added a commit to dlorenc/build-pipeline that referenced this issue Nov 8, 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 <bobcatfish@gmail.com>
Fixes: tektoncd#937
dlorenc added a commit to dlorenc/build-pipeline that referenced this issue 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.

Co-authored-by: Christie Wilson <bobcatfish@gmail.com>
Fixes: tektoncd#937
dlorenc added a commit to dlorenc/build-pipeline that referenced this issue 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 <bobcatfish@gmail.com>
Fixes: tektoncd#937
dlorenc added a commit to dlorenc/build-pipeline that referenced this issue 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 <bobcatfish@gmail.com>
Fixes: tektoncd#937
dlorenc added a commit to dlorenc/build-pipeline that referenced this issue 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 <bobcatfish@gmail.com>
Fixes: tektoncd#937
tekton-robot pushed a commit that referenced this issue 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 <bobcatfish@gmail.com>
Fixes: #937
dlorenc added a commit to dlorenc/build-pipeline that referenced this issue Nov 19, 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 <bobcatfish@gmail.com>
Fixes: tektoncd#937
@ghost ghost mentioned this issue Dec 3, 2019
@bobcatfish
Copy link
Collaborator

Okay so long story short, we've been working on completely overhauling PipelineResources #1673 by adding other features that may potentially replace them (e.g. workspaces + task results) - but this means we haven't addressed this issue and we definitely need to before beta, so I'm re-opening this.

@bobcatfish
Copy link
Collaborator

Oh! #1545 fixed this after all (apparently causing sorrow for folks who were relying on this implementation detail 😅 ) but good in the long run!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
4 participants