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.

Sign up for GitHub

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

Jump to bottom

Deprecate DataVolume garbage collection #3552

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Deprecate DataVolume garbage collection #3552

merged 1 commit into from
Dec 4, 2024

Conversation

arnongilboa
Copy link
Collaborator

@arnongilboa arnongilboa commented Dec 2, 2024 •
edited
Loading

What this PR does / why we need it:
After several releases with DataVolume garbage collection disabled by default, we decided to deprecate it, as unfortunately it violates fundamental principle of Kubernetes. CR should not be auto-deleted when it completes its role (Job with TTLSecondsAfterFinished is an exception), and once CR was created we can assume it is there until explicitly deleted. In addition, CR should keep idempotency, so the same CR manifest can be applied multiple times, as long as it is a valid update (e.g. DataVolume validation webhook does not allow updating the spec).

Which issue(s) this PR fixes:
jira-ticket: https://issues.redhat.com/browse/CNV-30806

Special notes for your reviewer:

Release note:

Deprecate DataVolume garbage collection

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XL labels Dec 2, 2024
@coveralls
Copy link

coveralls commented Dec 2, 2024 •
edited
Loading

Coverage Status

coverage: 59.303% (+0.03%) from 59.276%
when pulling e49c956 on arnongilboa:dv_gc_deprecate
into f8fa903 on kubevirt:main.

@akalenyu
Copy link
Collaborator

akalenyu commented Dec 3, 2024

@fabiand @mnecas @mrnold do you know if any forklift related project is using dv garbage collection?

@arnongilboa
Deprecate DV garbage collection
e49c956 
After several releases with GC disabled by default, we decided to
deprecate it, as unfortunately it violates fundamental principle of
Kubernetes. CR should not be auto-deleted when it completes its role
(Job with TTLSecondsAfterFinished is an exception), and once CR was
created we can assume it is there until explicitly deleted. In addition,
CR should keep idempotency, so the same CR manifest can be applied
multiple times, as long as it is a valid update (e.g. DataVolume
validation webhook does not allow updating the spec).

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>
@arnongilboa arnongilboa changed the title [WIP] Deprecate DataVolume garbage collection Deprecate DataVolume garbage collection Dec 4, 2024
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2024
@arnongilboa
Copy link
Collaborator Author

/test pull-cdi-goveralls

@mnecas
Copy link
Contributor

mnecas commented Dec 4, 2024

The forklfit disables the GC [1].
I don't want to remove it from Forklift yet for backwards compatibility.
And having additional annotation even tho it's not used should not harm the users
[1] https://github.com/kubev2v/forklift/blob/87e3925da8cea784c7698e82afe729dbdf7e29eb/pkg/controller/plan/kubevirt.go#L1241

@arnongilboa
Copy link
Collaborator Author

@ksimon1 @0xFelix does the deprecation require any fix in kubevirt-tekton-tasks? I can't forget this tiny adjustment PR.

awels
awels approved these changes Dec 4, 2024
View reviewed changes
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2024
@kubevirt-bot kubevirt-bot merged commit be3bca3 into kubevirt:main Dec 4, 2024
20 checks passed
@0xFelix 0xFelix mentioned this pull request Dec 5, 2024
Open
@0xFelix
Copy link
Member

0xFelix commented Dec 5, 2024

@arnongilboa kubevirt-tekton-tasks do not assume that garbage collection is active by default AFAIK. I created an issue to keep track of it.

kubevirt/kubevirt-tekton-tasks#566

This was referenced Dec 8, 2024
Merged
Merged
@Ahmad-Hafe Ahmad-Hafe mentioned this pull request Dec 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awels awels awels approved these changes

@akalenyu akalenyu Awaiting requested review from akalenyu

@mhenriks mhenriks Awaiting requested review from mhenriks

Assignees

@awels awels

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@arnongilboa @coveralls @akalenyu @mnecas @kubevirt-bot @0xFelix @awels