Skip to content

Comments

Add DataVolume Finalizer#166

Open
kellydx wants to merge 1 commit intokubevirt:mainfrom
kellydx:data-volume-finalizer
Open

Add DataVolume Finalizer#166
kellydx wants to merge 1 commit intokubevirt:mainfrom
kellydx:data-volume-finalizer

Conversation

@kellydx
Copy link

@kellydx kellydx commented Nov 17, 2025

Add a finalizer to the DataVolume to prevent it from being accidentally deleted manually or by external workloads.

What this PR does / why we need it:
I encountered a scenario where some DataVolumes were accidentally deleted, leading to data loss and VMs being unable to boot. As a precaution, a DataVolume created by KVCSI should have a finalizer to prevent it from being accidentally deleted, ensuring resources are cleaned up properly.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Added finalizer "cdi.kubevirt.io/user-volume-protection" to DataVolume.

Add a finalizer to the DataVolume to prevent it from being accidentally deleted manually or by external workloads.

Signed-off-by: Kelly Duong <kellyduong@google.com>
@kubevirt-bot kubevirt-bot added 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. labels Nov 17, 2025
@kubevirt-bot
Copy link

Hi @kellydx. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

@kubevirt-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mhenriks for approval. For more information see the Code Review Process.

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

Details 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

@mhenriks
Copy link
Member

/lgtm
Thanks @kellydx!

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2026
@kubevirt-bot
Copy link

@kellydx: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-csi-driver-linter 9d8936f link true /test pull-csi-driver-linter
pull-csi-driver-split-e2e-k8s 9d8936f link true /test pull-csi-driver-split-e2e-k8s
pull-csi-driver-e2e-k8s 9d8936f link true /test pull-csi-driver-e2e-k8s
pull-csi-driver-split-k8s-suite-k8s 9d8936f link true /test pull-csi-driver-split-k8s-suite-k8s
Details

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-sigs/prow repository. I understand the commands that are listed here.

@kellydx
Copy link
Author

kellydx commented Jan 20, 2026

/retest

@kubevirt-bot
Copy link

@kellydx: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

/retest

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-sigs/prow repository.

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.

Looks like in some cases the finalizer is not removed from the infra PVC, and this is causing a ton of tests to fail.
You should be able to reproduce the issue with doing

make cluster-up
make cluster-sync-split
./hack/run-e2e.sh

This will run some of the tests and you should be able to see what is failing

It("DeleteDataVolume should successfully remove finalizer and call delete", func() {
dataVolume := createValidDataVolume()
dataVolume.Name = "pvc-test4"
_, err := c.CreateDataVolume(context.Background(), testNamespace, dataVolume)
Copy link
Member

Choose a reason for hiding this comment

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

Missing Expect(err).ToNot(HaveOccurred()) this is causing one of the linter failures

dataVolume := createValidDataVolume()
dataVolume.Name = "pvc-test5"
_, err := c.CreateDataVolume(context.Background(), testNamespace, dataVolume)
// Set up a custom client that returns an error on Patch
Copy link
Member

Choose a reason for hiding this comment

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

Missing Expect(err).ToNot(HaveOccurred()) this is causing a linter failure

@kubevirt-bot
Copy link

Pull requests that are marked with lgtm should receive a review
from an approver within 1 week.

After that period the bot marks them with the label needs-approver-review.

/label needs-approver-review

@kubevirt-bot kubevirt-bot added the needs-approver-review Indicates that a PR requires a review from an approver. label Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. needs-approver-review Indicates that a PR requires a review from an approver. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants