From 5b560b3ab928d50d2d78dba022a1fcded98daa20 Mon Sep 17 00:00:00 2001 From: Arnon Gilboa Date: Mon, 21 Aug 2023 11:52:16 +0300 Subject: [PATCH] [release-v1.56] Do not fail DV reconcile if storage class is not found Manual backport of #2860, fixing bz #2232347 Currently there is a repeating reconcile error due to exponential backoff: "DataVolume.storage spec is missing accessMode and no storageClass to choose profile", however the error is not needed as we already have SC watch for this case, e.g. when the DV is using the default SC but no default SC is configured yet. Note the SC Watch() was mistakenly looking for DVs with unset phase (like in the main/1.57 branch), but should look for Pending DVs instead. Signed-off-by: Arnon Gilboa --- pkg/controller/datavolume/controller-base.go | 6 +++++- pkg/controller/datavolume/import-controller_test.go | 13 +++++++++++++ pkg/controller/datavolume/util.go | 12 ++++++++++-- tests/datavolume_test.go | 2 +- 4 files changed, 29 insertions(+), 4 deletions(-) diff --git a/pkg/controller/datavolume/controller-base.go b/pkg/controller/datavolume/controller-base.go index 93d4402225..7bbb13de09 100644 --- a/pkg/controller/datavolume/controller-base.go +++ b/pkg/controller/datavolume/controller-base.go @@ -265,7 +265,7 @@ func addDataVolumeControllerCommonWatches(mgr manager.Manager, dataVolumeControl if err := dataVolumeController.Watch(&source.Kind{Type: &storagev1.StorageClass{}}, handler.EnqueueRequestsFromMapFunc( func(obj client.Object) (reqs []reconcile.Request) { dvList := &cdiv1.DataVolumeList{} - if err := mgr.GetClient().List(context.TODO(), dvList, client.MatchingFields{dvPhaseField: ""}); err != nil { + if err := mgr.GetClient().List(context.TODO(), dvList, client.MatchingFields{dvPhaseField: string(cdiv1.Pending)}); err != nil { return } for _, dv := range dvList.Items { @@ -394,6 +394,10 @@ func (r *ReconcilerBase) syncDvPvcState(log logr.Logger, req reconcile.Request, syncState.pvcSpec, err = renderPvcSpec(r.client, r.recorder, log, dv) if err != nil { + if errors.Is(err, ErrStorageClassNotFound) { + syncState.result = &reconcile.Result{} + return syncState, nil + } return syncState, err } diff --git a/pkg/controller/datavolume/import-controller_test.go b/pkg/controller/datavolume/import-controller_test.go index 8919407aee..b87b5de62b 100644 --- a/pkg/controller/datavolume/import-controller_test.go +++ b/pkg/controller/datavolume/import-controller_test.go @@ -89,6 +89,19 @@ var _ = Describe("All DataVolume Tests", func() { } }) + It("Should return nil when storage spec has no AccessModes, no StorageClassName, and no default storage class set", func() { + importDataVolume := newImportDataVolumeWithPvc("test-dv", nil) + importDataVolume.Spec.Storage = &cdiv1.StorageSpec{} + + reconciler = createImportReconciler(importDataVolume) + res, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}}) + Expect(err).ToNot(HaveOccurred()) + Expect(res).To(Equal(reconcile.Result{})) + + event := <-reconciler.recorder.(*record.FakeRecorder).Events + Expect(event).To(ContainSubstring(MessageErrStorageClassNotFound)) + }) + It("Should create a PVC on a valid import DV", func() { reconciler = createImportReconciler(NewImportDataVolume("test-dv")) _, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}}) diff --git a/pkg/controller/datavolume/util.go b/pkg/controller/datavolume/util.go index 743ea27388..73fed32830 100644 --- a/pkg/controller/datavolume/util.go +++ b/pkg/controller/datavolume/util.go @@ -44,6 +44,14 @@ import ( const ( // AnnOwnedByDataVolume annotation has the owner DataVolume name AnnOwnedByDataVolume = "cdi.kubevirt.io/ownedByDataVolume" + + // MessageErrStorageClassNotFound provides a const to indicate the DV storage spec is missing accessMode and no storageClass to choose profile + MessageErrStorageClassNotFound = "DataVolume.storage spec is missing accessMode and no storageClass to choose profile" +) + +var ( + // ErrStorageClassNotFound indicates the DV storage spec is missing accessMode and no storageClass to choose profile + ErrStorageClassNotFound = errors.New(MessageErrStorageClassNotFound) ) // renderPvcSpec creates a new PVC Spec based on either the dv.spec.pvc or dv.spec.storage section @@ -82,8 +90,8 @@ func pvcFromStorage(client client.Client, recorder record.EventRecorder, log log // Not even default storageClass on the cluster, cannot apply the defaults, verify spec is ok if len(pvcSpec.AccessModes) == 0 { log.V(1).Info("Cannot set accessMode for new pvc", "namespace", dv.Namespace, "name", dv.Name) - recorder.Eventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid, "DataVolume.storage spec is missing accessMode and no storageClass to choose profile") - return nil, errors.Errorf("DataVolume spec is missing accessMode") + recorder.Eventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid, MessageErrStorageClassNotFound) + return nil, ErrStorageClassNotFound } } else { pvcSpec.StorageClassName = &storageClass.Name diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index 53e17a1200..9fca29027e 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -1895,7 +1895,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", events, err := f.RunKubectlCommand("get", "events", "-n", dataVolume.Namespace, "--field-selector=involvedObject.kind=DataVolume") if err == nil { fmt.Fprintf(GinkgoWriter, "%s", events) - return strings.Contains(events, controller.ErrClaimNotValid) && strings.Contains(events, "DataVolume.storage spec is missing accessMode and no storageClass to choose profile") + return strings.Contains(events, controller.ErrClaimNotValid) && strings.Contains(events, dvc.MessageErrStorageClassNotFound) } fmt.Fprintf(GinkgoWriter, "ERROR: %s\n", err.Error()) return false