Skip to content

Commit f75b3d6

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 f75b3d6

2 files changed

Lines changed: 120 additions & 25 deletions

File tree

‎pkg/controller/dataimportcron-controller.go‎

Lines changed: 38 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,37 @@ 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+
scChanged := currentSc != desiredSC.Name
1116+
needsReset := hasCurrent || len(cron.Status.CurrentImports) > 0
1117+
if scChanged && needsReset {
1118+
r.log.Info("Storage class changed, resetting import", "currentSc", currentSc, "desiredSc", desiredSC.Name)
1119+
return true, nil
1120+
}
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+
1129+
if currentDV != nil {
1130+
sp := &cdiv1.StorageProfile{}
1131+
if err := r.client.Get(ctx, types.NamespacedName{Name: desiredSC.Name}, sp); err != nil {
1132+
return false, err
1133+
}
1134+
desiredDV := r.newSourceDataVolume(cron, currentDV.Name, sp)
1135+
if !reflect.DeepEqual(currentDV.Spec, desiredDV.Spec) {
1136+
r.log.Info("Import DV spec changed, resetting import", "dv", currentDV.Name)
1137+
return true, nil
1138+
}
1139+
}
1140+
1141+
return false, nil
1142+
}
1143+
11141144
// getSnapshotClassForDataImportCron returns the VolumeSnapshotClass name to use for DataImportCron snapshots.
11151145
func (r *DataImportCronReconciler) getSnapshotClassForDataImportCron(pvc *corev1.PersistentVolumeClaim, storageProfile *cdiv1.StorageProfile) (string, error) {
11161146
if vscName := storageProfile.Annotations[cc.AnnSnapshotClassForDataImportCron]; vscName != "" {
@@ -1533,23 +1563,6 @@ func getReconcileRequestsForDicsWithoutExplicitStorageClass(ctx context.Context,
15331563
return reqs, nil
15341564
}
15351565

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-
}
15531566

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

‎pkg/controller/dataimportcron-controller_test.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1844,6 +1844,88 @@ 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+
By("First reconcile: detect SC change and delete old DV")
1866+
res, err := reconciler.Reconcile(context.TODO(), cronReq)
1867+
Expect(err).ToNot(HaveOccurred())
1868+
Expect(res.RequeueAfter).To(Equal(time.Second))
1869+
1870+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
1871+
Expect(k8serrors.IsNotFound(err)).To(BeTrue())
1872+
1873+
err = reconciler.client.Get(context.TODO(), cronKey, cron)
1874+
Expect(err).ToNot(HaveOccurred())
1875+
Expect(cron.Annotations[AnnStorageClass]).To(Equal(storageClassName))
1876+
1877+
By("Second reconcile: create new DV with the correct SC context")
1878+
_, err = reconciler.Reconcile(context.TODO(), cronReq)
1879+
Expect(err).ToNot(HaveOccurred())
1880+
1881+
dv = &cdiv1.DataVolume{}
1882+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
1883+
Expect(err).ToNot(HaveOccurred())
1884+
})
1885+
1886+
It("Should delete DV and requeue when StorageProfile RWO annotation changes", func() {
1887+
dvName := "test-datasource-68b44fc891f3"
1888+
sp := &cdiv1.StorageProfile{
1889+
ObjectMeta: metav1.ObjectMeta{
1890+
Name: storageClassName,
1891+
Annotations: map[string]string{cc.AnnUseReadWriteOnceForDataImportCron: "true"},
1892+
},
1893+
}
1894+
reconciler = createDataImportCronReconciler(sc, sp)
1895+
1896+
cron = newDataImportCron(cronName)
1897+
cron.Spec.Template.Spec.Storage.AccessModes = nil
1898+
cc.AddAnnotation(cron, AnnSourceDesiredDigest, testDigest)
1899+
cc.AddAnnotation(cron, AnnStorageClass, storageClassName)
1900+
cron.Status.CurrentImports = []cdiv1.ImportStatus{{DataVolumeName: dvName, Digest: testDigest}}
1901+
err := reconciler.client.Create(context.TODO(), cron)
1902+
Expect(err).ToNot(HaveOccurred())
1903+
dataSource = &cdiv1.DataSource{}
1904+
1905+
dv := cron.Spec.Template.DeepCopy()
1906+
dv.Name = dvName
1907+
dv.Namespace = metav1.NamespaceDefault
1908+
err = reconciler.client.Create(context.TODO(), dv)
1909+
Expect(err).ToNot(HaveOccurred())
1910+
1911+
By("First reconcile: detect DV spec drift and delete old DV")
1912+
res, err := reconciler.Reconcile(context.TODO(), cronReq)
1913+
Expect(err).ToNot(HaveOccurred())
1914+
Expect(res.RequeueAfter).To(Equal(time.Second))
1915+
1916+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
1917+
Expect(k8serrors.IsNotFound(err)).To(BeTrue())
1918+
1919+
By("Second reconcile: create new DV with RWO access mode")
1920+
_, err = reconciler.Reconcile(context.TODO(), cronReq)
1921+
Expect(err).ToNot(HaveOccurred())
1922+
1923+
dv = &cdiv1.DataVolume{}
1924+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
1925+
Expect(err).ToNot(HaveOccurred())
1926+
Expect(dv.Spec.Storage.AccessModes).To(Equal([]corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}))
1927+
})
1928+
18471929
})
18481930
})
18491931
})

0 commit comments

Comments
 (0)