Skip to content

Commit 782019d

Browse files
noamasunoamasu
committed
Fix DataImportCron reconciliation when first default StorageClass is set
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>
1 parent 5591d89 commit 782019d

2 files changed

Lines changed: 101 additions & 25 deletions

File tree

‎pkg/controller/dataimportcron-controller.go‎

Lines changed: 37 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -359,19 +359,18 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
359359
return res, err
360360
}
361361
if desiredStorageClass != nil {
362-
if deleted, err := r.deleteOutdatedPendingPvc(ctx, pvc, desiredStorageClass.Name, dataImportCron.Name); deleted || err != nil {
362+
desiredSc := desiredStorageClass.Name
363+
outdated, err := r.importOutdated(ctx, dataImportCron, dv, pvc, desiredStorageClass)
364+
if err != nil {
363365
return res, err
364366
}
365-
currentSc, hasCurrent := dataImportCron.Annotations[AnnStorageClass]
366-
desiredSc := desiredStorageClass.Name
367-
if hasCurrent && currentSc != desiredSc {
368-
r.log.Info("Storage class changed, delete most recent source on the old sc as it's no longer the desired", "currentSc", currentSc, "desiredSc", desiredSc)
369-
if err := r.handleStorageClassChange(ctx, dataImportCron, desiredSc); err != nil {
367+
if outdated {
368+
if err := r.recreateImport(ctx, dataImportCron, desiredSc); err != nil {
370369
return res, err
371370
}
372371
return reconcile.Result{RequeueAfter: time.Second}, nil
373372
}
374-
cc.AddAnnotation(dataImportCron, AnnStorageClass, desiredStorageClass.Name)
373+
cc.AddAnnotation(dataImportCron, AnnStorageClass, desiredSc)
375374
}
376375
format, err := r.getSourceFormat(ctx, desiredStorageClass)
377376
if err != nil {
@@ -974,7 +973,7 @@ func (p *authProxy) GetDataSource(namespace, name string) (*cdiv1.DataSource, er
974973
return das, nil
975974
}
976975

977-
func (r *DataImportCronReconciler) handleStorageClassChange(ctx context.Context, dataImportCron *cdiv1.DataImportCron, desiredStorageClass string) error {
976+
func (r *DataImportCronReconciler) recreateImport(ctx context.Context, dataImportCron *cdiv1.DataImportCron, desiredStorageClass string) error {
978977
digest, ok := dataImportCron.Annotations[AnnSourceDesiredDigest]
979978
if !ok {
980979
// nothing to delete
@@ -1111,6 +1110,36 @@ func (r *DataImportCronReconciler) handleSnapshotClassChange(ctx context.Context
11111110
return true, nil
11121111
}
11131112

1113+
func (r *DataImportCronReconciler) importOutdated(ctx context.Context, cron *cdiv1.DataImportCron, currentDV *cdiv1.DataVolume, pvc *corev1.PersistentVolumeClaim, desiredSC *storagev1.StorageClass) (bool, error) {
1114+
currentSc, hasCurrent := cron.Annotations[AnnStorageClass]
1115+
if hasCurrent && currentSc != desiredSC.Name {
1116+
r.log.Info("Storage class changed, resetting import", "currentSc", currentSc, "desiredSc", desiredSC.Name)
1117+
return true, nil
1118+
} else if !hasCurrent && len(cron.Status.CurrentImports) > 0 {
1119+
r.log.Info("Storage class changed, resetting import", "currentSc", currentSc, "desiredSc", desiredSC.Name)
1120+
return true, nil
1121+
}
1122+
if pvc != nil && pvc.Status.Phase == corev1.ClaimPending &&
1123+
pvc.Labels[common.DataImportCronLabel] == cron.Name &&
1124+
pvc.Spec.StorageClassName != nil && *pvc.Spec.StorageClassName != desiredSC.Name {
1125+
r.log.Info("Pending PVC has outdated storage class, resetting import", "pvc", pvc.Name, "pvcSc", *pvc.Spec.StorageClassName, "desiredSc", desiredSC.Name)
1126+
return true, nil
1127+
}
1128+
if currentDV == nil {
1129+
return false, nil
1130+
}
1131+
sp := &cdiv1.StorageProfile{}
1132+
if err := r.client.Get(ctx, types.NamespacedName{Name: desiredSC.Name}, sp); err != nil {
1133+
return false, err
1134+
}
1135+
desiredDV := r.newSourceDataVolume(cron, currentDV.Name, sp)
1136+
if !reflect.DeepEqual(currentDV.Spec, desiredDV.Spec) {
1137+
r.log.Info("Import DV spec changed, resetting import", "dv", currentDV.Name)
1138+
return true, nil
1139+
}
1140+
return false, nil
1141+
}
1142+
11141143
// getSnapshotClassForDataImportCron returns the VolumeSnapshotClass name to use for DataImportCron snapshots.
11151144
func (r *DataImportCronReconciler) getSnapshotClassForDataImportCron(pvc *corev1.PersistentVolumeClaim, storageProfile *cdiv1.StorageProfile) (string, error) {
11161145
if vscName := storageProfile.Annotations[cc.AnnSnapshotClassForDataImportCron]; vscName != "" {
@@ -1533,23 +1562,6 @@ func getReconcileRequestsForDicsWithoutExplicitStorageClass(ctx context.Context,
15331562
return reqs, nil
15341563
}
15351564

1536-
func (r *DataImportCronReconciler) deleteOutdatedPendingPvc(ctx context.Context, pvc *corev1.PersistentVolumeClaim, desiredStorageClass, cronName string) (bool, error) {
1537-
if pvc == nil || pvc.Status.Phase != corev1.ClaimPending || pvc.Labels[common.DataImportCronLabel] != cronName {
1538-
return false, nil
1539-
}
1540-
1541-
sc := pvc.Spec.StorageClassName
1542-
if sc == nil || *sc == desiredStorageClass {
1543-
return false, nil
1544-
}
1545-
1546-
r.log.Info("Delete pending pvc", "name", pvc.Name, "ns", pvc.Namespace, "sc", *sc)
1547-
if err := r.client.Delete(ctx, pvc); cc.IgnoreNotFound(err) != nil {
1548-
return false, err
1549-
}
1550-
1551-
return true, nil
1552-
}
15531565

15541566
func (r *DataImportCronReconciler) cronJobExistsAndUpdated(ctx context.Context, cron *cdiv1.DataImportCron) (bool, error) {
15551567
cronJob := &batchv1.CronJob{}

‎pkg/controller/dataimportcron-controller_test.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1844,6 +1844,70 @@ var _ = Describe("All DataImportCron Tests", func() {
18441844
Expect(dv.Spec.Storage.AccessModes).To(BeEmpty())
18451845
})
18461846

1847+
It("Should delete old DV and requeue when first default StorageClass is set", func() {
1848+
dvName := "test-datasource-68b44fc891f3"
1849+
sp := &cdiv1.StorageProfile{
1850+
ObjectMeta: metav1.ObjectMeta{Name: storageClassName},
1851+
}
1852+
reconciler = createDataImportCronReconciler(sc, sp)
1853+
1854+
cron = newDataImportCron(cronName)
1855+
cc.AddAnnotation(cron, AnnSourceDesiredDigest, testDigest)
1856+
cron.Status.CurrentImports = []cdiv1.ImportStatus{{DataVolumeName: dvName, Digest: testDigest}}
1857+
err := reconciler.client.Create(context.TODO(), cron)
1858+
Expect(err).ToNot(HaveOccurred())
1859+
dataSource = &cdiv1.DataSource{}
1860+
1861+
dv := &cdiv1.DataVolume{ObjectMeta: metav1.ObjectMeta{Name: dvName, Namespace: metav1.NamespaceDefault}}
1862+
err = reconciler.client.Create(context.TODO(), dv)
1863+
Expect(err).ToNot(HaveOccurred())
1864+
1865+
res, err := reconciler.Reconcile(context.TODO(), cronReq)
1866+
Expect(err).ToNot(HaveOccurred())
1867+
Expect(res.RequeueAfter).To(Equal(time.Second))
1868+
1869+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
1870+
Expect(k8serrors.IsNotFound(err)).To(BeTrue())
1871+
1872+
err = reconciler.client.Get(context.TODO(), cronKey, cron)
1873+
Expect(err).ToNot(HaveOccurred())
1874+
Expect(cron.Annotations[AnnStorageClass]).To(Equal(storageClassName))
1875+
})
1876+
1877+
It("Should delete DV and requeue when StorageProfile RWO annotation changes", func() {
1878+
dvName := "test-datasource-68b44fc891f3"
1879+
sp := &cdiv1.StorageProfile{
1880+
ObjectMeta: metav1.ObjectMeta{
1881+
Name: storageClassName,
1882+
Annotations: map[string]string{cc.AnnUseReadWriteOnceForDataImportCron: "true"},
1883+
},
1884+
}
1885+
reconciler = createDataImportCronReconciler(sc, sp)
1886+
1887+
cron = newDataImportCron(cronName)
1888+
cron.Spec.Template.Spec.Storage.AccessModes = nil
1889+
cc.AddAnnotation(cron, AnnSourceDesiredDigest, testDigest)
1890+
cc.AddAnnotation(cron, AnnStorageClass, storageClassName)
1891+
cron.Status.CurrentImports = []cdiv1.ImportStatus{{DataVolumeName: dvName, Digest: testDigest}}
1892+
err := reconciler.client.Create(context.TODO(), cron)
1893+
Expect(err).ToNot(HaveOccurred())
1894+
dataSource = &cdiv1.DataSource{}
1895+
1896+
// DV created without RWO (before SP annotation was set)
1897+
dv := cron.Spec.Template.DeepCopy()
1898+
dv.Name = dvName
1899+
dv.Namespace = metav1.NamespaceDefault
1900+
err = reconciler.client.Create(context.TODO(), dv)
1901+
Expect(err).ToNot(HaveOccurred())
1902+
1903+
res, err := reconciler.Reconcile(context.TODO(), cronReq)
1904+
Expect(err).ToNot(HaveOccurred())
1905+
Expect(res.RequeueAfter).To(Equal(time.Second))
1906+
1907+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
1908+
Expect(k8serrors.IsNotFound(err)).To(BeTrue())
1909+
})
1910+
18471911
})
18481912
})
18491913
})

0 commit comments

Comments
 (0)