DV Recreated with Wrong Status #3912

Dsanatar · 2025-09-17T18:16:22Z

What this PR does / why we need it:

When a DV that is apart of a VM's DVTemplate gets deleted occasionally during the recreation, the new DV will be seen to immediately have status of Succeeded with Progress at 100% and then will eventually switch back to Pending. This is due to a race condition where the associated PVC is not deleted/cleaned up in time for when the new DV goes to update its status. Since the new DV will see the old PVC is bound, it will immediately update it's first status to Succeeded.

Added a new check during updateStatus() to prevent DVs from using PVCs that have a DeletionTimeStamp

Release note:

Check for PVC DeletionTimeStamp before updating DV status

its status from a PVC that is marked for deletion Signed-off-by: dsanatar <[email protected]>

alromeros

Good catch! Testing this would be great but seems tricky to do.
/lgtm

akalenyu · 2025-09-18T09:39:54Z

Good catch! Testing this would be great but seems tricky to do. /lgtm

I think we should be able to supply the erroneous state to a fake reconciler in a unit test, tbh, that would also make me feel a little better about this change as I don't completely understand the scenario

alromeros · 2025-09-18T09:54:13Z

Good catch! Testing this would be great but seems tricky to do. /lgtm

I think we should be able to supply the erroneous state to a fake reconciler in a unit test, tbh, that would also make me feel a little better about this change as I don't completely understand the scenario

Agree but isolating this logic might not be straightforward, and also there're no utests for this file. The behavior is racy but maybe it's worth having a functest that deletes the pvc and ensures the DV completes nicely to ensure there're no weird behaviors in between?

alromeros · 2025-09-18T09:54:34Z

/hold
Let's discuss first the testing options

akalenyu · 2025-09-18T10:00:08Z

Good catch! Testing this would be great but seems tricky to do. /lgtm

I think we should be able to supply the erroneous state to a fake reconciler in a unit test, tbh, that would also make me feel a little better about this change as I don't completely understand the scenario

Agree but isolating this logic might not be straightforward, and also there're no utests for this file. The behavior is racy but maybe it's worth having a functest that deletes the pvc and ensures the DV completes nicely to ensure there're no weird behaviors in between?

That's prone to flakes, I'd unit test it with a pinpointed state identified to be wrong
We can use any of the unit test suites for controller that piggyback on the controller base, so import for example

coveralls · 2025-09-22T18:00:36Z

coverage: 59.153%. remained the same
when pulling b39fb7d on Dsanatar:dv-recreated-status
into 2b04a3d on kubevirt:main.

alromeros · 2025-09-23T11:40:53Z

/unhold
Looks great!
/lgtm

akalenyu · 2025-09-25T08:13:41Z

pkg/controller/datavolume/import-controller_test.go

+			// Reset the DV status phase
+			newPhase := cdiv1.Pending
+			dv.Status.Phase = newPhase
+			err = reconciler.client.Status().Update(context.TODO(), dv)
+			Expect(err).ToNot(HaveOccurred())


Which entity would be causing this in a "real" setup?

The problem spawns from when a deleted DV from a VM's DVTemplate is recreated. Here I am trying to emulate that behavior of the "new" DV by just resetting the status and making sure it doesn't inherit a new status from it's associated PVC that is in the process of deletion.

I see, maybe we can reduce the test to that then? IOW are all the setup steps really needed?

Yea good point, I can give that a go and see.

akalenyu

I believe you can further reduce this to passing the entire state to the fake reconciler, but up to you
/approve

akalenyu · 2025-09-28T09:04:52Z

pkg/controller/datavolume/import-controller_test.go

+			// Set finalizer so PVC is not removed when Delete is called
+			pvc.Finalizers = append(pvc.Finalizers, "test-finalizer")
+			err = reconciler.client.Update(context.TODO(), pvc)
+			Expect(err).ToNot(HaveOccurred())
+
+			// Mark PVC for deletion
+			err = reconciler.client.Delete(context.TODO(), pvc)
+			Expect(err).ToNot(HaveOccurred())


You don't have to use the client to setup the state, you can just pass the objects in the target state to createImportReconciler

kubevirt-bot · 2025-09-28T09:08:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu

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:

~~OWNERS~~ [akalenyu]

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

update the status of existing DVs Signed-off-by: dsanatar <[email protected]>

alromeros · 2025-10-08T15:24:25Z

/lgtm

Add check during updateStatus that prevents DVs from updating

9fbd733

its status from a PVC that is marked for deletion Signed-off-by: dsanatar <[email protected]>

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. size/XS labels Sep 17, 2025

kubevirt-bot requested review from aglitke and awels September 17, 2025 18:16

alromeros reviewed Sep 18, 2025

View reviewed changes

kubevirt-bot assigned alromeros Sep 18, 2025

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2025

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2025

kubevirt-bot added size/M and removed lgtm Indicates that a PR is ready to be merged. size/XS labels Sep 22, 2025

Dsanatar force-pushed the dv-recreated-status branch from 86dc206 to c8f800b Compare September 22, 2025 17:48

kubevirt-bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 23, 2025

akalenyu reviewed Sep 25, 2025

View reviewed changes

Dsanatar force-pushed the dv-recreated-status branch from c8f800b to d35c0f7 Compare September 26, 2025 15:35

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2025

akalenyu reviewed Sep 28, 2025

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2025

add unit test to make sure PVCs marked for deletion do not

b39fb7d

update the status of existing DVs Signed-off-by: dsanatar <[email protected]>

Dsanatar force-pushed the dv-recreated-status branch from d35c0f7 to b39fb7d Compare September 29, 2025 21:51

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2025

kubevirt-bot merged commit d71a929 into kubevirt:main Oct 8, 2025
21 checks passed

DV Recreated with Wrong Status #3912

DV Recreated with Wrong Status #3912

Uh oh!

Conversation

Dsanatar commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alromeros left a comment

Choose a reason for hiding this comment

Uh oh!

akalenyu commented Sep 18, 2025

Uh oh!

alromeros commented Sep 18, 2025

Uh oh!

alromeros commented Sep 18, 2025

Uh oh!

akalenyu commented Sep 18, 2025

Uh oh!

coveralls commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alromeros commented Sep 23, 2025

Uh oh!

akalenyu Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Dsanatar Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

akalenyu Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Dsanatar Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

akalenyu left a comment

Choose a reason for hiding this comment

Uh oh!

akalenyu Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

kubevirt-bot commented Sep 28, 2025

Uh oh!

alromeros commented Oct 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dsanatar commented Sep 17, 2025 •

edited

Loading

coveralls commented Sep 22, 2025 •

edited

Loading