Recreate DIC import DV when first default SC is set by noamasu · Pull Request #4074 · kubevirt/containerized-data-importer

noamasu · 2026-03-25T15:44:07Z

What this PR does / why we need it:
The default SC change condition only handled changes from one SC to another, but did not handle the first default SC being set.

To get the DIC-created DV with the DIC-special RWO preference, it must be deleted (as DV is immutable) and recreated with the right settings from the new default StorageClass.

Which issue(s) this PR fixes:
Jira: https://redhat.atlassian.net/browse/CNV-81826

Special notes for your reviewer:

Release note:

NONE

kubevirt-bot · 2026-03-25T15:44:21Z

[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 akalenyu 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:

OWNERS

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

coveralls · 2026-03-29T08:46:38Z

coverage: 49.737% (-0.04%) from 49.781% — noamasu:bugfix/dataimportcron-reconciliation-fix into kubevirt:main

arnongilboa · 2026-03-29T09:30:44Z

+		if hasCurrent {
+			storageClassChanged = currentSc != desiredSc
+		} else {
+			storageClassChanged = len(dataImportCron.Status.CurrentImports) > 0


len(imports) > 0 ?

yep, updating

arnongilboa · 2026-03-29T09:32:41Z

 				Expect(dv.Spec.Storage.AccessModes).To(BeEmpty())
 			})

+			It("Should delete old DV and requeue when first default StorageClass is set", func() {


shouldn't it test the creation of a new dv?

akalenyu

Bit confused about the issue

akalenyu · 2026-03-29T11:57:44Z

 	if desiredStorageClass != nil {
-		if deleted, err := r.deleteOutdatedPendingPvc(ctx, pvc, desiredStorageClass.Name, dataImportCron.Name); deleted || err != nil {
+		desiredSc := desiredStorageClass.Name
+		if deleted, err := r.deleteOutdatedPendingPvc(ctx, pvc, desiredSc, dataImportCron.Name); deleted || err != nil {


I'm not sure why the described issue ins't being handled by deleteOutdatedPendingPvc but that was going to solve the no sc->set sc

Because in the problematic scenario the DV was already created, and will prefer RWX as specified by the default StorageProfile, as it was created before the DIC controller reconciled to prefer RWO, so just deleting the PVC (and not recreating DV) will recreate the PVC again with RWX. I think we better refactor this code and do all the deletion magics in a single call deleting the DV.

I agree but for backportability why don't we just whack the DV in the deleteOutdatedPendingPvc path?

deleteOutdatedPendingPvc returns immediately when pvc status != Pending, and also when the pvc SC is the default/desired one, so afaiu it won't help much in the mentioned scenario. @noamasu ?

containerized-data-importer/pkg/controller/dataimportcron-controller.go

Lines 1536 to 1547 in 33bb159

func (r *DataImportCronReconciler) deleteOutdatedPendingPvc(ctx context.Context, pvc *corev1.PersistentVolumeClaim, desiredStorageClass, cronName string) (bool, error) {

if pvc == nil || pvc.Status.Phase != corev1.ClaimPending || pvc.Labels[common.DataImportCronLabel] != cronName {

return false, nil

}

sc := pvc.Spec.StorageClassName

if sc == nil || *sc == desiredStorageClass {

return false, nil

}

r.log.Info("Delete pending pvc", "name", pvc.Name, "ns", pvc.Namespace, "sc", *sc)

if err := r.client.Delete(ctx, pvc); cc.IgnoreNotFound(err) != nil {

when SC is nil (and no accessmode specificed in DIC), the DV is stuck with error "PVC spec is missing accessMode and no storageClass to choose profile" and this happens before the DV's PVC is created...
because the PVC is nil, the logic in deleteOutdatedPendingPvc is skipped...

if we want to put the DV deletion in deleteOutdatedPendingPvc, we might need to do some extra changes there (and also change the this func name to reflect DV deletion as well)
I can go to that direction if you think this is worth it

I tried to keep the fix as contained as possible, but i can refactor if needed

Thinking about it some more, I think we're better off handling a DV spec mismatch at all times, similarly to the snapshot thing (handleSnapshotClassChange). The reasoning is that I don't think this type of issues will be limited to the storage class switching scenario

the thing with default SC is that the DV spec are the same for current and desired DV in case a default SC is being set, therefore a DV spec mismatch will probably wont detect that edge case change.. (same for the outdated PVC edge case, you still need to check the PVC's SC name and compare it to desired SC... what we can do is instead of removing the PVC, we can remove the DV instead).

would you prefer to keep that fix contained with what we have right now and later open another PR for a broader mismatch refactor, or would you rather do the refactor in this PR? (which again, will probably be unrelated to this specific nil to default SC case... we will probably still need to track down the desiredStorageClass and decide if to delete DV according to its changes).

The desired DV spec will be created explicitly with RWO, so reflect.DeepEqual() should detect the change, right?

containerized-data-importer/pkg/controller/dataimportcron-controller.go

Lines 1797 to 1803 in 4e4aaa3

// Apply RWO access mode as default for DataImportCron (from StorageProfile annotation)

// Only applies if the DV doesn't already have AccessModes configured

if storageProfile != nil && storageProfile.Annotations[cc.AnnUseReadWriteOnceForDataImportCron] == "true" {

if dv.Spec.Storage != nil && len(dv.Spec.Storage.AccessModes) == 0 {

dv.Spec.Storage.AccessModes = []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}

}

}

I think I remember that reconciling over the DV desired != current was trickier to implement but as @arnongilboa said it's definitely doable. Maybe snap has to carry source access mode so you know when to reconcile

I see we took a hybrid approach instead of strictly focusing on the spec drift, which makes me worry a little since we have to reason about the broader introduced condition.

IMO, the backport-friendly version of this change is to strictly handle spec drifts on the DV and act accordingly, and a follow up to main for a bigger refactor.

That said, I fully acknowledge that I am on the cautious side, and if we can get a 2-1 vote on this, I am willing to proceed.

noamasu · 2026-05-13T13:26:33Z

/retest

kubevirt-bot · 2026-05-13T22:50:44Z

@noamasu: 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-containerized-data-importer-e2e-destructive	`3d24d4d`	link	true	`/test pull-containerized-data-importer-e2e-destructive`
pull-containerized-data-importer-e2e-hpp-latest	`3d24d4d`	link	true	`/test pull-containerized-data-importer-e2e-hpp-latest`
pull-containerized-data-importer-e2e-nfs	`3d24d4d`	link	true	`/test pull-containerized-data-importer-e2e-nfs`
pull-containerized-data-importer-e2e-hpp-previous	`3d24d4d`	link	true	`/test pull-containerized-data-importer-e2e-hpp-previous`
pull-containerized-data-importer-e2e-istio	`3d24d4d`	link	true	`/test pull-containerized-data-importer-e2e-istio`
pull-containerized-data-importer-non-csi-hpp	`3d24d4d`	link	true	`/test pull-containerized-data-importer-non-csi-hpp`
pull-containerized-data-importer-e2e-upg	`3d24d4d`	link	true	`/test pull-containerized-data-importer-e2e-upg`
pull-containerized-data-importer-e2e-ceph	`3d24d4d`	link	true	`/test pull-containerized-data-importer-e2e-ceph`
pull-containerized-data-importer-e2e-ceph-wffc	`3d24d4d`	link	true	`/test pull-containerized-data-importer-e2e-ceph-wffc`

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.

kubevirt-prow · 2026-06-29T14:49:02Z

[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 akalenyu 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:

OWNERS

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

noamasu · 2026-07-02T07:47:31Z

/retest

The default SC change condition only handled changes from one SC to another, but did not handle the first default SC being set. To get the DIC-created DV with the DIC-special RWO preference, it must be deleted (as DV is immutable) and recreated with the right settings from the new default StorageClass. Signed-off-by: Noam Assouline <nassouli@redhat.com>

akalenyu

main worry in #4074 (comment) 🙏

akalenyu · 2026-07-02T10:06:29Z

I think the destructive lane failure is related https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_containerized-data-importer/4074/pull-containerized-data-importer-e2e-destructive/2072598145111429120

kubevirt-prow · 2026-07-02T15:29:12Z

@noamasu: 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-containerized-data-importer-e2e-destructive	`d4dd4c7`	link	true	`/test pull-containerized-data-importer-e2e-destructive`
pull-containerized-data-importer-e2e-nfs	`d4dd4c7`	link	true	`/test pull-containerized-data-importer-e2e-nfs`
pull-containerized-data-importer-non-csi-hpp	`d4dd4c7`	link	true	`/test pull-containerized-data-importer-non-csi-hpp`
pull-containerized-data-importer-e2e-hpp-latest	`d4dd4c7`	link	true	`/test pull-containerized-data-importer-e2e-hpp-latest`
pull-containerized-data-importer-e2e-istio	`d4dd4c7`	link	true	`/test pull-containerized-data-importer-e2e-istio`
pull-containerized-data-importer-e2e-hpp-previous	`d4dd4c7`	link	true	`/test pull-containerized-data-importer-e2e-hpp-previous`
pull-containerized-data-importer-e2e-upg	`d4dd4c7`	link	true	`/test pull-containerized-data-importer-e2e-upg`
pull-containerized-data-importer-e2e-ceph	`d4dd4c7`	link	true	`/test pull-containerized-data-importer-e2e-ceph`
pull-containerized-data-importer-e2e-ceph-wffc	`d4dd4c7`	link	true	`/test pull-containerized-data-importer-e2e-ceph-wffc`
pull-containerized-data-importer-e2e-s390x	`d4dd4c7`	link	unknown	`/test pull-containerized-data-importer-e2e-s390x`

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.

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 25, 2026

kubevirt-bot added the size/XS label Mar 25, 2026

kubevirt-bot requested review from akalenyu and mhenriks March 25, 2026 15:44

noamasu changed the title ~~Fix DataImportCron reconciliation when first default StorageClass is set~~ Recreate DIC import DV when first default SC is set Mar 26, 2026

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch from cbc4f06 to 08dd124 Compare March 29, 2026 08:33

kubevirt-bot added size/M and removed size/XS labels Mar 29, 2026

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch from 08dd124 to 37bb528 Compare March 29, 2026 08:49

arnongilboa reviewed Mar 29, 2026

View reviewed changes

akalenyu reviewed Mar 29, 2026

View reviewed changes

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 29, 2026

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch from b298650 to 37bb528 Compare March 29, 2026 13:03

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Mar 29, 2026

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch from 37bb528 to b4dc55a Compare March 29, 2026 14:46

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch from b4dc55a to 3d24d4d Compare May 12, 2026 10:31

kubevirt-bot added size/L and removed size/M labels May 12, 2026

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch from 3d24d4d to 782019d Compare June 17, 2026 21:20

noamasu marked this pull request as draft June 17, 2026 22:31

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 17, 2026

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch from 782019d to c4a39f9 Compare June 29, 2026 14:48

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch from c4a39f9 to f75b3d6 Compare June 29, 2026 15:01

noamasu marked this pull request as ready for review June 29, 2026 15:02

kubevirt-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2026

kubevirt-prow Bot requested review from akalenyu and awels June 29, 2026 15:02

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch 2 times, most recently from 33f1aad to a765b41 Compare July 1, 2026 08:04

arnongilboa reviewed Jul 2, 2026

View reviewed changes

Comment thread pkg/controller/dataimportcron-controller.go Outdated

arnongilboa reviewed Jul 2, 2026

View reviewed changes

Comment thread pkg/controller/dataimportcron-controller.go Outdated

arnongilboa reviewed Jul 2, 2026

View reviewed changes

Comment thread pkg/controller/dataimportcron-controller.go Outdated

noamasu force-pushed the bugfix/dataimportcron-reconciliation-fix branch from a765b41 to d4dd4c7 Compare July 2, 2026 08:27

akalenyu reviewed Jul 2, 2026

View reviewed changes

	func (r DataImportCronReconciler) deleteOutdatedPendingPvc(ctx context.Context, pvc corev1.PersistentVolumeClaim, desiredStorageClass, cronName string) (bool, error) {
	if pvc == nil \|\| pvc.Status.Phase != corev1.ClaimPending \|\| pvc.Labels[common.DataImportCronLabel] != cronName {
	return false, nil
	}

	sc := pvc.Spec.StorageClassName
	if sc == nil \|\| *sc == desiredStorageClass {
	return false, nil
	}

	r.log.Info("Delete pending pvc", "name", pvc.Name, "ns", pvc.Namespace, "sc", *sc)
	if err := r.client.Delete(ctx, pvc); cc.IgnoreNotFound(err) != nil {

	// Apply RWO access mode as default for DataImportCron (from StorageProfile annotation)
	// Only applies if the DV doesn't already have AccessModes configured
	if storageProfile != nil && storageProfile.Annotations[cc.AnnUseReadWriteOnceForDataImportCron] == "true" {
	if dv.Spec.Storage != nil && len(dv.Spec.Storage.AccessModes) == 0 {
	dv.Spec.Storage.AccessModes = []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}
	}
	}

Uh oh!

Conversation

noamasu commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubevirt-bot commented Mar 25, 2026

Uh oh!

coveralls commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akalenyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnongilboa Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamasu Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akalenyu Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamasu Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamasu commented May 13, 2026

Uh oh!

kubevirt-bot commented May 13, 2026

Uh oh!

kubevirt-prow Bot commented Jun 29, 2026

Uh oh!

noamasu commented Jul 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akalenyu left a comment

Choose a reason for hiding this comment

Uh oh!

akalenyu commented Jul 2, 2026

Uh oh!

kubevirt-prow Bot commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

noamasu commented Mar 25, 2026 •

edited

Loading

coveralls commented Mar 29, 2026 •

edited

Loading

arnongilboa Mar 29, 2026 •

edited

Loading

noamasu Mar 29, 2026 •

edited

Loading

akalenyu Mar 29, 2026 •

edited

Loading

noamasu Apr 19, 2026 •

edited

Loading