Avoid creating snapshot of old storage class DataImportCron PVCs

api/openapi-spec/swagger.json

@@ -5311,6 +5311,10 @@
      "lastImportedPVC": {
       "description": "LastImportedPVC is the last imported PVC",
       "$ref": "#/definitions/v1beta1.DataVolumeSourcePVC"
+     },
+     "sourceFormat": {
+      "description": "SourceFormat defines the format of the DataImportCron-created disk image sources",
+      "type": "string"
      }
     }
    },

pkg/controller/dataimportcron-controller.go

@@ -319,14 +319,14 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
 
 	dataVolume := dataImportCron.Spec.Template
 	explicitScName := getStorageClassFromTemplate(&dataVolume)
-	dvStorageClass, err := cc.GetStorageClassByName(ctx, r.client, explicitScName)
+	desiredStorageClass, err := cc.GetStorageClassByName(ctx, r.client, explicitScName)
 	if err != nil {
 		return res, err
 	}
-	if dvStorageClass != nil {
-		cc.AddAnnotation(dataImportCron, AnnStorageClass, dvStorageClass.Name)
+	if desiredStorageClass != nil {
+		cc.AddAnnotation(dataImportCron, AnnStorageClass, desiredStorageClass.Name)
 	}
-	format, err := r.getSourceFormat(ctx, dataImportCron, dvStorageClass)
+	format, err := r.getSourceFormat(ctx, dataImportCron, desiredStorageClass)
 	if err != nil {
 		return res, err
 	}
@@ -342,7 +342,7 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
 			}
 		}
 		importSucceeded = true
-		if err := r.handleCronFormat(ctx, dataImportCron, format, dvStorageClass); err != nil {
+		if err := r.handleCronFormat(ctx, dataImportCron, pvc, format, desiredStorageClass); err != nil {
 			return err
 		}
 
@@ -673,29 +673,24 @@ func (r *DataImportCronReconciler) createImportDataVolume(ctx context.Context, d
 	return nil
 }
 
-func (r *DataImportCronReconciler) handleCronFormat(ctx context.Context, dataImportCron *cdiv1.DataImportCron, format cdiv1.DataImportCronSourceFormat, dvStorageClass *storagev1.StorageClass) error {
+func (r *DataImportCronReconciler) handleCronFormat(ctx context.Context, dataImportCron *cdiv1.DataImportCron, pvc *corev1.PersistentVolumeClaim, format cdiv1.DataImportCronSourceFormat, desiredStorageClass *storagev1.StorageClass) error {
 	switch format {
 	case cdiv1.DataImportCronSourceFormatPvc:
 		return nil
 	case cdiv1.DataImportCronSourceFormatSnapshot:
-		return r.handleSnapshot(ctx, dataImportCron, &dataImportCron.Spec.Template, dvStorageClass)
+		return r.handleSnapshot(ctx, dataImportCron, pvc, desiredStorageClass)
 	default:
 		return fmt.Errorf("unknown source format for snapshot")
 	}
 }
 
-func (r *DataImportCronReconciler) handleSnapshot(ctx context.Context, dataImportCron *cdiv1.DataImportCron, dataVolume *cdiv1.DataVolume, dvStorageClass *storagev1.StorageClass) error {
-	dataSourceName := dataImportCron.Spec.ManagedDataSource
-	digest := dataImportCron.Annotations[AnnSourceDesiredDigest]
-	if digest == "" {
+func (r *DataImportCronReconciler) handleSnapshot(ctx context.Context, dataImportCron *cdiv1.DataImportCron, pvc *corev1.PersistentVolumeClaim, desiredStorageClass *storagev1.StorageClass) error {
+	if sc := pvc.Spec.StorageClassName; sc != nil && *sc != desiredStorageClass.Name {
+		r.log.Info("Attempt to change storage class, will not try making a snapshot of the old PVC")
 		return nil
 	}
-	dvName, err := createDvName(dataSourceName, digest)
-	if err != nil {
-		return err
-	}
 
-	className, err := cc.GetSnapshotClassForSmartClone(dataVolume.Name, &dvStorageClass.Name, r.log, r.client)
+	className, err := cc.GetSnapshotClassForSmartClone(pvc.Name, &desiredStorageClass.Name, r.log, r.client)
 	if err != nil {
 		return err
 	}
@@ -705,13 +700,13 @@ func (r *DataImportCronReconciler) handleSnapshot(ctx context.Context, dataImpor
 	}
 	desiredSnapshot := &snapshotv1.VolumeSnapshot{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      dvName,
+			Name:      pvc.Name,
 			Namespace: dataImportCron.Namespace,
 			Labels:    labels,
 		},
 		Spec: snapshotv1.VolumeSnapshotSpec{
 			Source: snapshotv1.VolumeSnapshotSource{
-				PersistentVolumeClaimName: &dvName,
+				PersistentVolumeClaimName: &pvc.Name,
 			},
 			VolumeSnapshotClassName: &className,
 		},
@@ -741,6 +736,8 @@ func (r *DataImportCronReconciler) handleSnapshot(ctx context.Context, dataImpor
 }
 
 func (r *DataImportCronReconciler) updateDataImportCronSuccessCondition(ctx context.Context, dataImportCron *cdiv1.DataImportCron, format cdiv1.DataImportCronSourceFormat, snapshot *snapshotv1.VolumeSnapshot) error {
+	dataImportCron.Status.SourceFormat = &format
+
 	switch format {
 	case cdiv1.DataImportCronSourceFormatPvc:
 		updateDataImportCronCondition(dataImportCron, cdiv1.DataImportCronUpToDate, corev1.ConditionTrue, "Latest import is up to date", upToDate)
@@ -761,14 +758,14 @@ func (r *DataImportCronReconciler) updateDataImportCronSuccessCondition(ctx cont
 	return nil
 }
 
-func (r *DataImportCronReconciler) getSourceFormat(ctx context.Context, dataImportCron *cdiv1.DataImportCron, dvStorageClass *storagev1.StorageClass) (cdiv1.DataImportCronSourceFormat, error) {
+func (r *DataImportCronReconciler) getSourceFormat(ctx context.Context, dataImportCron *cdiv1.DataImportCron, desiredStorageClass *storagev1.StorageClass) (cdiv1.DataImportCronSourceFormat, error) {
 	format := cdiv1.DataImportCronSourceFormatPvc
-	if dvStorageClass == nil {
+	if desiredStorageClass == nil {
 		return format, nil
 	}
 
 	storageProfile := &cdiv1.StorageProfile{}
-	if err := r.client.Get(context.TODO(), types.NamespacedName{Name: dvStorageClass.Name}, storageProfile); err != nil {
+	if err := r.client.Get(context.TODO(), types.NamespacedName{Name: desiredStorageClass.Name}, storageProfile); err != nil {
 		return format, err
 	}
 	if storageProfile.Status.DataImportCronSourceFormat != nil {

pkg/controller/dataimportcron-controller_test.go

@@ -902,6 +902,51 @@ var _ = Describe("All DataImportCron Tests", func() {
 				Expect(err).ToNot(HaveOccurred())
 				Expect(snapList.Items).To(BeEmpty())
 			})
+
+			It("Should not create snapshot from old storage class PVCs", func() {
+				cron = newDataImportCron(cronName)
+				dataSource = nil
+				retentionPolicy := cdiv1.DataImportCronRetainNone
+				cron.Spec.RetentionPolicy = &retentionPolicy
+				err := reconciler.client.Create(context.TODO(), cron)
+				Expect(err).ToNot(HaveOccurred())
+				verifyConditions("Before DesiredDigest is set", false, false, false, noImport, noDigest, "", &snapshotv1.VolumeSnapshot{})
+
+				cc.AddAnnotation(cron, AnnSourceDesiredDigest, testDigest)
+				err = reconciler.client.Update(context.TODO(), cron)
+				Expect(err).ToNot(HaveOccurred())
+				dataSource = &cdiv1.DataSource{}
+				verifyConditions("After DesiredDigest is set", false, false, false, noImport, outdated, noSource, &snapshotv1.VolumeSnapshot{})
+
+				imports := cron.Status.CurrentImports
+				Expect(imports).ToNot(BeNil())
+				Expect(imports).ToNot(BeEmpty())
+				dvName := imports[0].DataVolumeName
+				Expect(dvName).ToNot(BeEmpty())
+				digest := imports[0].Digest
+				Expect(digest).To(Equal(testDigest))
+
+				dv := &cdiv1.DataVolume{}
+				err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(*dv.Spec.Source.Registry.URL).To(Equal(testRegistryURL + "@" + testDigest))
+				Expect(dv.Annotations[cc.AnnImmediateBinding]).To(Equal("true"))
+
+				prevSc := "previous-storage-class"
+				pvc := cc.CreatePvcInStorageClass(dv.Name, dv.Namespace, &prevSc, nil, nil, corev1.ClaimBound)
+				err = reconciler.client.Create(context.TODO(), pvc)
+				Expect(err).ToNot(HaveOccurred())
+				// DV GCed after hitting succeeded
+				err = reconciler.client.Delete(context.TODO(), dv)
+				Expect(err).ToNot(HaveOccurred())
+
+				_, err = reconciler.Reconcile(context.TODO(), cronReq)
+				Expect(err).ToNot(HaveOccurred())
+				snap := &snapshotv1.VolumeSnapshot{}
+				err = reconciler.client.Get(context.TODO(), dvKey(dvName), snap)
+				Expect(err).To(HaveOccurred())
+				Expect(k8serrors.IsNotFound(err)).To(BeTrue())
+			})
 		})
 	})
 })

pkg/operator/resources/crds_generated.go

@@ -4925,7 +4925,12 @@ spec:
     singular: dataimportcron
   scope: Namespaced
   versions:
-  - name: v1beta1
+  - additionalPrinterColumns:
+    - description: The format in which created sources are saved
+      jsonPath: .status.sourceFormat
+      name: Format
+      type: string
+    name: v1beta1
     schema:
       openAPIV3Schema:
         description: DataImportCron defines a cron job for recurring polling/importing
@@ -5772,12 +5777,17 @@ spec:
                 - name
                 - namespace
                 type: object
+              sourceFormat:
+                description: SourceFormat defines the format of the DataImportCron-created
+                  disk image sources
+                type: string
             type: object
         required:
         - spec
         type: object
     served: true
     storage: true
+    subresources: {}
 status:
   acceptedNames:
     kind: ""

staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/types.go

@@ -533,6 +533,7 @@ type DataSourceList struct {
 // +kubebuilder:object:root=true
 // +kubebuilder:storageversion
 // +kubebuilder:resource:shortName=dic;dics,categories=all
+// +kubebuilder:printcolumn:name="Format",type="string",JSONPath=".status.sourceFormat",description="The format in which created sources are saved"
 type DataImportCron struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
@@ -591,8 +592,10 @@ type DataImportCronStatus struct {
 	// LastExecutionTimestamp is the time of the last polling
 	LastExecutionTimestamp *metav1.Time `json:"lastExecutionTimestamp,omitempty"`
 	// LastImportTimestamp is the time of the last import
-	LastImportTimestamp *metav1.Time              `json:"lastImportTimestamp,omitempty"`
-	Conditions          []DataImportCronCondition `json:"conditions,omitempty" optional:"true"`
+	LastImportTimestamp *metav1.Time `json:"lastImportTimestamp,omitempty"`
+	// SourceFormat defines the format of the DataImportCron-created disk image sources
+	SourceFormat *DataImportCronSourceFormat `json:"sourceFormat,omitempty"`
+	Conditions   []DataImportCronCondition   `json:"conditions,omitempty" optional:"true"`
 }
 
 // ImportStatus of a currently in progress import