delete PVCs created by the controller using `volumeClaimTemplate` once pipelineRun is completed #6635

pritidesai · 2023-05-09T05:38:20Z

Changes

PVCs can be specified using volumeClaimTemplates as part of a pipelineRun. The pipelineRun controller creates a new PVC using the specifications from the template. Once the pipelineRun is complete, the PVC exist to facilitate any analysis on the completed pod. Such PVCs are helpful but requires an extra cleanup if not needed. In the cases where pipelineRuns are kept for a long period of time, such PVC can waste resources.

This commit introduces an option for the user such that the users can configure pipelineRun to delete such PVCs. There is no API change introduced to support this cleanup.

PipelineRun/TaskRun controller now reads a label from the volumeClaimTemplate to decide whether to delete PVC or not. Set the label " pvc-protection-finalizer" to "remove" to take advantage of this kind of cleanup.

kind: PipelineRun
metadata:
  generateName: pipeline-run-
spec:
  workspaces:
    - name: source
       volumeClaimTemplate:
         metadata:
           labels:
             pvc-protection-finalizer: remove

Closes #5776

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2023-05-09T05:38:22Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

linux-foundation-easycla · 2023-05-09T05:38:23Z

❌ The commit (d3d36a9). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

tekton-robot · 2023-05-09T05:38:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from pritidesai after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

tekton-robot · 2023-05-09T05:46:37Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	89.1%	-0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	84.8%	-0.2
pkg/reconciler/volumeclaim/pvchandler.go	90.0%	55.1%	-34.9

tekton-robot · 2023-05-09T05:47:24Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	89.1%	-0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	84.8%	-0.2
pkg/reconciler/volumeclaim/pvchandler.go	90.0%	55.1%	-34.9

PVCs can be specified using volumeClaimTemplates as part of a pipelineRun. The pipelineRun controller creates a new PVC using the specifications from the template. Once the pipelineRun is complete, the PVC exist to faciliate any analysis on the completed pod. Such PVCs are helpful but requires an extra cleanup if not needed. In the cases where pipelineRuns are kept for a long period of time, such PVC can waste resources. This commit introduces an option for the user such that the users can configure pipelineRun controller to delete such PVCs. There is no API change needed to support this cleanup. PipelineRun/TaskRun controller now reads a label from the volumeClaimTemplate to decide whether to delete PVC or not. Set the label " pvc-protection-finalizer" to "remove" to take advantage of this kind of cleanup. kind: PipelineRun metadata: generateName: pipeline-run- spec: workspaces: - name: source volumeClaimTemplate: metadata: labels: pvc-protection-finalizer: remove Closes tektoncd#5776 Signed-off-by: pritidesai <[email protected]> Signed-off-by: Priti Desai <[email protected]>

tekton-robot · 2023-05-09T05:58:04Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	89.1%	-0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	84.8%	-0.2
pkg/reconciler/volumeclaim/pvchandler.go	90.0%	55.1%	-34.9

tekton-robot · 2023-05-09T05:58:10Z

The following Tekton test failed:

Test name	Commit	Details	Required	Rerun command
check-pr-has-kind-label	`d3d36a9`	link	true	`/test check-pr-has-kind-label`

tekton-robot · 2023-05-09T06:04:13Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	89.2%	89.1%	-0.1
pkg/reconciler/taskrun/taskrun.go	85.0%	84.8%	-0.2
pkg/reconciler/volumeclaim/pvchandler.go	90.0%	55.1%	-34.9

jimmyjones2 · 2023-05-09T20:55:54Z

Looks great, thanks!

Should it be an annotation rather than a label, prefixed with tekton.dev/ though?

pritidesai · 2023-05-09T21:40:38Z

Should it be an annotation rather than a label, prefixed with tekton.dev/ though?

I can move it under annotation rather than a label but a prefix tekton.dev is reserved for the controller and cannot be part of the user specified labels/annotations. The pipeline controller adds multiple labels with such prefix:

kind: PipelineRun
metadata:
 labels:
    tekton.dev/pipeline: test-pipeline

jimmyjones2 · 2023-05-10T07:28:18Z

My (limited) understanding is that keys without a prefix are private to the user, so any keys used by some kind of 3rd party automation should have a prefix. But this is a nit, I'm excited about the feature 😊

jimmyjones2 · 2023-05-29T18:48:57Z

@pritidesai Hey, still super interested in this, what's stopping it from being merged?

lbernick · 2023-05-30T18:56:02Z

@pritidesai Just want to make sure I'm understanding this correctly:

We create PVCs with ReclaimPolicy=Delete when users specify volumeClaimTemplates in their workspace bindings. Users can't delete PVCs of completed PipelineRuns, because kubernetes adds a protection finalizer preventing the PVC from being deleted while the PersistentVolume backing the PVC still exists. This PR removes the finalizer, allowing the PVC to be deleted without deleting the PersistentVolume. Is that right?

I'm curious if instead of removing the finalizer, we can add the configuration option @jimmyjones2 suggested to auto-delete PVCs when a PipelineRun completes, and delete the PersistentVolume along with the PVC. It seems like removing the finalizer would allow the user to delete the PVCs, but doesn't ensure the PVs get deleted too, meaning storage wouldn't actually get freed up. Is my understanding here correct?

tekton-robot · 2023-06-09T05:25:35Z

@pritidesai: PR needs rebase.

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.

pritidesai · 2023-06-16T22:46:26Z

The way pvc are created now has changed in #6741. The pvcs are created as part of the StatefulSet. When the StatefulSet is deleted once the pipelineRun is complete, the pvc is left in terminating state #6741 (comment).

I do not think keeping such pvcs in terminating state for the cluster operator to cleanup is a good practice. As part of the change in #6741, we could implement it such that pvcs are deleted by design rather than giving the users to choose whether to delete them or not. Or, we change the pvc creation such that they do not end up in a terminating state.

@tektoncd/core-maintainers thoughts?

lbernick · 2023-06-20T12:56:10Z

The way pvc are created now has changed in #6741. The pvcs are created as part of the StatefulSet. When the StatefulSet is deleted once the pipelineRun is complete, the pvc is left in terminating state #6741 (comment).

I do not think keeping such pvcs in terminating state for the cluster operator to cleanup is a good practice. As part of the change in #6741, we could implement it such that pvcs are deleted by design rather than giving the users to choose whether to delete them or not. Or, we change the pvc creation such that they do not end up in a terminating state.

@tektoncd/core-maintainers thoughts?

Thanks Priti! I commented on #6741 but I think it makes sense to delete PVCs when the PipelineRun is completed.

Part of [tektoncd#6740] and unblocks [tektoncd#6635]. `PVCs` are created if a user specifies `VolumeClaimTemplate` as the source of a `pipelinerun workspace`. Prior to this change, such `pvcs` are created via `affinity assistant statefulset` when `affinity assistant` is enabled (in both `workspaces` or `pipelineruns` coschedule mode). To delete such pvcs when the owning `pipelinerun` is completed, we must explicitly delete those pvcs in the reconciler since the owner of such pvcs is the `affinity assistant statefulset` instead of the `pipelinerun`. The problem of such deletion mechanism is that such `pvcs` are left in `terminating` state when the owning `pipelinerun` is `completed` but not `deleted`. This is because the pvcs are protected by `kubernetes.io/pvc-protection` `finalizer`, which does not allow the `pvcs` to be deleted until the `pipelinerun` consuming the `pvc` is deleted. Leaving pvcs in `terminating` state is confusing to cluster operators. Instead of changing the pvc deletion behavior in such backward incompatible way, it is better to make it configurable so that it is backward compatible, as prototyped in [tektoncd#6635]. This commit reverts the pvc creation behavior `per-workspace` coschedule mode, which changes the owner of the `pvcs` back to the `pipelinerun` instead of the `affinity assistant statefulset`. After the commit, the pvcs created by specifying `VolumeClaimTemplate` are left in `bounded` state instead of `terminating`. This commit is the prerequisite of [tektoncd#6635]. This commit does NOT reverts the pvc creation behavior `per-pipelinerun` coschedule mode as we will enforce the deletion of pvcs when the owning `pipelinerun` is completed. This is a better practice and there is no backward compatability concern since `per-pipelinerun` coschedule mode is a new feature. [tektoncd#6740]: tektoncd#6740 [tektoncd#6635]: tektoncd#6635

Part of [#6740] and unblocks [#6635]. `PVCs` are created if a user specifies `VolumeClaimTemplate` as the source of a `pipelinerun workspace`. Prior to this change, such `pvcs` are created via `affinity assistant statefulset` when `affinity assistant` is enabled (in both `workspaces` or `pipelineruns` coschedule mode). To delete such pvcs when the owning `pipelinerun` is completed, we must explicitly delete those pvcs in the reconciler since the owner of such pvcs is the `affinity assistant statefulset` instead of the `pipelinerun`. The problem of such deletion mechanism is that such `pvcs` are left in `terminating` state when the owning `pipelinerun` is `completed` but not `deleted`. This is because the pvcs are protected by `kubernetes.io/pvc-protection` `finalizer`, which does not allow the `pvcs` to be deleted until the `pipelinerun` consuming the `pvc` is deleted. Leaving pvcs in `terminating` state is confusing to cluster operators. Instead of changing the pvc deletion behavior in such backward incompatible way, it is better to make it configurable so that it is backward compatible, as prototyped in [#6635]. This commit reverts the pvc creation behavior `per-workspace` coschedule mode, which changes the owner of the `pvcs` back to the `pipelinerun` instead of the `affinity assistant statefulset`. After the commit, the pvcs created by specifying `VolumeClaimTemplate` are left in `bounded` state instead of `terminating`. This commit is the prerequisite of [#6635]. This commit does NOT reverts the pvc creation behavior `per-pipelinerun` coschedule mode as we will enforce the deletion of pvcs when the owning `pipelinerun` is completed. This is a better practice and there is no backward compatability concern since `per-pipelinerun` coschedule mode is a new feature. [#6740]: #6740 [#6635]: #6635

Part of [tektoncd#6740] and unblocks [tektoncd#6635]. `PVCs` are created if a user specifies `VolumeClaimTemplate` as the source of a `pipelinerun workspace`. Prior to this change, such `pvcs` are created via `affinity assistant statefulset` when `affinity assistant` is enabled (in both `workspaces` or `pipelineruns` coschedule mode). To delete such pvcs when the owning `pipelinerun` is completed, we must explicitly delete those pvcs in the reconciler since the owner of such pvcs is the `affinity assistant statefulset` instead of the `pipelinerun`. The problem of such deletion mechanism is that such `pvcs` are left in `terminating` state when the owning `pipelinerun` is `completed` but not `deleted`. This is because the pvcs are protected by `kubernetes.io/pvc-protection` `finalizer`, which does not allow the `pvcs` to be deleted until the `pipelinerun` consuming the `pvc` is deleted. Leaving pvcs in `terminating` state is confusing to cluster operators. Instead of changing the pvc deletion behavior in such backward incompatible way, it is better to make it configurable so that it is backward compatible, as prototyped in [tektoncd#6635]. This commit reverts the pvc creation behavior `per-workspace` coschedule mode, which changes the owner of the `pvcs` back to the `pipelinerun` instead of the `affinity assistant statefulset`. After the commit, the pvcs created by specifying `VolumeClaimTemplate` are left in `bounded` state instead of `terminating`. This commit is the prerequisite of [tektoncd#6635]. This commit does NOT reverts the pvc creation behavior `per-pipelinerun` coschedule mode as we will enforce the deletion of pvcs when the owning `pipelinerun` is completed. This is a better practice and there is no backward compatability concern since `per-pipelinerun` coschedule mode is a new feature. [tektoncd#6740]: tektoncd#6740 [tektoncd#6635]: tektoncd#6635

Part of [tektoncd#6740][tektoncd#6740], developed based on Priti's [prototype][prototype] and partially completes the PVC deletion behavior [discussion][discussion]. Prior to this commit, the `PVCs` created from `PipelineRun's` `VolumeClaimTemplate` are not auto deleted when the owning `PipelineRun` is completed. This commit updates the `cleanupAffinityAssistantsAndPVCs` function to remove the `kubernetes.io/pvc-protection` finalizer protection (so that the pvc is allowed to be deleted while the pod consuming it is not deleted). The function then explicitly delete such `PVC` when cleaning up the `Affinity Assistants` at pr completion time. This change is NOT applied to `coschedule: workspaces` mode as there is backward compatability concern. See more details in this [discussion][discussion] [tektoncd#6740]: tektoncd#6740 [prototype]: tektoncd#6635 [discussion]: tektoncd#6741 (comment)

Part of [#6740][#6740], developed based on Priti's [prototype][prototype] and partially completes the PVC deletion behavior [discussion][discussion]. Prior to this commit, the `PVCs` created from `PipelineRun's` `VolumeClaimTemplate` are not auto deleted when the owning `PipelineRun` is completed. This commit updates the `cleanupAffinityAssistantsAndPVCs` function to remove the `kubernetes.io/pvc-protection` finalizer protection (so that the pvc is allowed to be deleted while the pod consuming it is not deleted). The function then explicitly delete such `PVC` when cleaning up the `Affinity Assistants` at pr completion time. This change is NOT applied to `coschedule: workspaces` mode as there is backward compatability concern. See more details in this [discussion][discussion] [#6740]: #6740 [prototype]: #6635 [discussion]: #6741 (comment)

vdemeester · 2023-07-21T09:53:45Z

@pritidesai is it still something for 0.50 ?

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-none Denotes a PR that doesnt merit a release note. labels May 9, 2023

tekton-robot requested review from bobcatfish and dibyom May 9, 2023 05:38

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

pritidesai force-pushed the fix-5776 branch from 8870c5a to 6db8d88 Compare May 9, 2023 05:39

pritidesai mentioned this pull request May 9, 2023

Allow PVC of completed PipelineRun to be deleted #5776

Open

pritidesai mentioned this pull request May 9, 2023

Query: How do people run pipelines concurrently with defined PVC workspaces? #5412

Open

pritidesai force-pushed the fix-5776 branch from 6db8d88 to 48d6cff Compare May 9, 2023 05:51

pritidesai force-pushed the fix-5776 branch from 48d6cff to d3d36a9 Compare May 9, 2023 05:57

lbernick self-assigned this May 9, 2023

pritidesai added this to the Pipelines v0.49 milestone May 31, 2023

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2023

lbernick mentioned this pull request Jun 20, 2023

TEP-0135: Refactor Affinity Assistant PVC creation #6741

Merged

7 tasks

afrittoli modified the milestones: Pipelines v0.49, Pipelines v0.50 (LTS) Jun 27, 2023

QuanZhang-William mentioned this pull request Jun 30, 2023

[TEP-0135] Revert PVC creation #6893

Merged

7 tasks

QuanZhang-William mentioned this pull request Jul 18, 2023

[TEP-0135] Purge finalizer and delete PVC #6940

Merged

7 tasks

vdemeester modified the milestones: Pipelines v0.50 (LTS), Pipelines v0.51 Jul 25, 2023

vdemeester modified the milestones: Pipelines v0.51, Pipelines v0.52 Aug 22, 2023

pritidesai modified the milestones: Pipelines v0.52, Pipelines v0.53 Sep 13, 2023

pritidesai modified the milestones: Pipelines v0.53 LTS, Pipelines v0.54 Oct 16, 2023

jerop removed this from the Pipelines v0.54 milestone Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delete PVCs created by the controller using `volumeClaimTemplate` once pipelineRun is completed #6635

delete PVCs created by the controller using `volumeClaimTemplate` once pipelineRun is completed #6635

pritidesai commented May 9, 2023

tekton-robot commented May 9, 2023

linux-foundation-easycla bot commented May 9, 2023 •

edited

Loading

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

jimmyjones2 commented May 9, 2023

pritidesai commented May 9, 2023

jimmyjones2 commented May 10, 2023

jimmyjones2 commented May 29, 2023

lbernick commented May 30, 2023

tekton-robot commented Jun 9, 2023

pritidesai commented Jun 16, 2023

lbernick commented Jun 20, 2023

vdemeester commented Jul 21, 2023

delete PVCs created by the controller using volumeClaimTemplate once pipelineRun is completed #6635

Are you sure you want to change the base?

delete PVCs created by the controller using volumeClaimTemplate once pipelineRun is completed #6635

Conversation

pritidesai commented May 9, 2023

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 9, 2023

linux-foundation-easycla bot commented May 9, 2023 • edited Loading

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

jimmyjones2 commented May 9, 2023

pritidesai commented May 9, 2023

jimmyjones2 commented May 10, 2023

jimmyjones2 commented May 29, 2023

lbernick commented May 30, 2023

tekton-robot commented Jun 9, 2023

pritidesai commented Jun 16, 2023

lbernick commented Jun 20, 2023

vdemeester commented Jul 21, 2023

delete PVCs created by the controller using `volumeClaimTemplate` once pipelineRun is completed #6635

delete PVCs created by the controller using `volumeClaimTemplate` once pipelineRun is completed #6635

linux-foundation-easycla bot commented May 9, 2023 •

edited

Loading