Skip to content

TEP: Add optional flag to enable PVCs auto-removal in "workspaces" coschedule mode #1203

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fambelic
Copy link

This TEP proposes adding a new optional enable-pvc-auto-removal flag to the feature-flags ConfigMap, allowing automatic deletion of workspace PVCs when using the workspaces coschedule mode. The change maintains backward compatibility while improving resource cleanup and usability.

Copy link

linux-foundation-easycla bot commented May 26, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: fambelic / name: fambelic (92ae4cf)

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign mnuttall after the PR has been reviewed.
You can assign the PR to them by writing /assign @mnuttall in a comment when ready.

The full list of commands accepted by this bot can be found 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 added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 26, 2025
@fambelic fambelic force-pushed the main branch 7 times, most recently from a29b35e to b59f0e2 Compare May 27, 2025 06:24
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thanks @fambelic for this TEP.

One existing alternative today is to use tektoncd/results and configure it so that it deletes PipelineRuns once they are ingested into permanent storage. I appreciate that it does not do the exact same thing, I just wanted to make sure you were aware of it.

Generally speaking it seems fine for me to add this feature. I assume it would only apply to PVC created by Tekton, correct?

It would be good to get some eyes from the chains and results teams, as they add annotations to PipelineRuns after their execution is finished, and I just wanted to make sure that a missing PVC wouldn't break anything - although I don't think it would.

@vdemeester @lcarva @adambkaplan WDYT?

teps/README.md Outdated
@@ -147,3 +147,5 @@ This is the complete list of Tekton TEPs:
|[TEP-0155](0155-store-pipeline-events-in-db.md) | Store Pipeline Events in Tekton Results | proposed | 2024-04-19 |
|[TEP-0156](0156-whenexpressions-in-step.md) | WhenExpressions in Steps | implemented | 2024-07-22 |
|[TEP-0160](0160-enhance-results-cli.md) | Enhance Tekton Results CLI | proposed | 2025-03-13 |
|[TEP-0161](0161-optional-flag-for-enabling-pvcs-auto-removal-behavior.md) | Optional Flag for Enabling PVCs Auto Removal Behavior | proposed | 2025-05-26 |

Copy link
Member

Choose a reason for hiding this comment

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

NIT: The extra line here is breaking the lint job

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I'll fix it asap

@adambkaplan
Copy link
Contributor

One existing alternative today is to use tektoncd/results and configure it so that it deletes PipelineRuns once they are ingested into permanent storage.

Agree that Results pruning of PipelineRuns should be listed as an alternative. The mechanism is different (Kubernetes cascading deletion), and has the downside of removing the PipelineRun + related objects - most notably the TaskRun containers and their logs.

It would be good to get some eyes from the chains and results teams, as they add annotations to PipelineRuns after their execution is finished, and I just wanted to make sure that a missing PVC wouldn't break anything - although I don't think it would.

@khrm @enarha @jkhelil may be in a better position to answer this. I don't think Results adds annotations to PVCs that are owned by a TaskRun or PipelineRun.

@afrittoli
Copy link
Member

@pritidesai FYI

@afrittoli
Copy link
Member

@fambelic this seems somewhat related tektoncd/pipeline#6635 but it has been abandoned, perhaps you could reuse some of that work too

@fambelic
Copy link
Author

fambelic commented Jun 3, 2025

Hi @afrittoli! Thanks all for the responses, really appreciate it! :)

One existing alternative today is to use tektoncd/results and configure it so that it deletes PipelineRuns once they are ingested into permanent storage. I appreciate that it does not do the exact same thing, I just wanted to make sure you were aware of it.

Yes, I’m aware of that, but my proposal is more focused on PVCs. It extends a logic that’s already implemented for the two newer coscheduling modes. In short, it aims to handle the case where someone wants to clean up PVCs immediately after a PipelineRun completes. This would bring the default coschedule mode in line with the behavior already present in the other two modes.

Generally speaking it seems fine for me to add this feature. I assume it would only apply to PVC created by Tekton, correct?

Yes!

It would be good to get some eyes from the chains and results teams, as they add annotations to PipelineRuns after their execution is finished, and I just wanted to make sure that a missing PVC wouldn't break anything - although I don't think it would.

Just out of curiosity and to understand better: since the other two coscheduling modes (pipelineruns and isolate-pipelineruns) already automatically remove PVCs, wouldn’t any issues related to missing PVCs and their effect on Chains or Results already be present in those modes?

@afrittoli
Copy link
Member

Just out of curiosity and to understand better: since the other two coscheduling modes (pipelineruns and isolate-pipelineruns) already automatically remove PVCs, wouldn’t any issues related to missing PVCs and their effect on Chains or Results already be present in those modes?

That's a fair point; it most likely will be fine.
I'm not sure how extensively those combinations have been tested, but in any case, it's not likely that Chains or Results will be affected by the missing PVC. Results may be looking for logs, so as long as the Pod stays healthy it should be ok.

@fambelic fambelic force-pushed the main branch 2 times, most recently from 1fba8ca to 5b5981c Compare June 3, 2025 14:28
@tekton-robot
Copy link
Contributor

The following Tekton test failed:

Test name Commit Details Required Rerun command
pull-community-teps-lint 92ae4cf link true /test pull-community-teps-lint

@vdemeester vdemeester self-assigned this Jun 11, 2025
@fambelic
Copy link
Author

Hi @vdemeester! Sorry for the delay! If this feature is considered valuable, I’d be happy to work on its implementation. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants