Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not fail DV reconcile if storage class is not found #2860

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/controller/datavolume/controller-base.go
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,10 @@ func (r *ReconcilerBase) syncDvPvcState(log logr.Logger, req reconcile.Request,
Event{corev1.EventTypeWarning, cc.ErrClaimNotValid, err.Error()}); syncErr != nil {
log.Error(syncErr, "failed to sync DataVolume status with event")
}
if errors.Is(err, ErrStorageClassNotFound) {
syncState.result = &reconcile.Result{}
return syncState, nil
}
return syncState, err
}

Expand Down
19 changes: 19 additions & 0 deletions pkg/controller/datavolume/import-controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/tools/record"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
Expand Down Expand Up @@ -85,6 +86,24 @@ var _ = Describe("All DataVolume Tests", func() {
}
})

DescribeTable("Should return nil when storage spec has no AccessModes and", func(scName *string) {
importDataVolume := newImportDataVolumeWithPvc("test-dv", nil)
importDataVolume.Spec.Storage = &cdiv1.StorageSpec{
StorageClassName: scName,
}

reconciler = createImportReconciler(importDataVolume)
res, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
Expect(err).ToNot(HaveOccurred())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also
Expect(result).To(Equal(reconcile.Result{}))
To ensure requeue/requeueafter don't get set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Expect(res).To(Equal(reconcile.Result{}))

event := <-reconciler.recorder.(*record.FakeRecorder).Events
Expect(event).To(ContainSubstring(MessageErrStorageClassNotFound))
},
Entry("no StorageClassName, and no default storage class set", nil),
Entry("non-existing StorageClassName", pointer.String("nosuch")),
)

It("Should create volumeImportSource if should use cdi populator", func() {
scName := "testSC"
sc := CreateStorageClassWithProvisioner(scName, map[string]string{AnnDefaultStorageClass: "true"}, map[string]string{}, "csi-plugin")
Expand Down
12 changes: 10 additions & 2 deletions pkg/controller/datavolume/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,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
Expand Down Expand Up @@ -97,8 +105,8 @@ func renderPvcSpecVolumeModeAndAccessModes(client client.Client, recorder record
// 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 errors.Errorf("DataVolume spec is missing accessMode")
recorder.Eventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid, MessageErrStorageClassNotFound)
return ErrStorageClassNotFound
}
return nil
}
Expand Down
8 changes: 4 additions & 4 deletions tests/datavolume_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2036,7 +2036,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
Expand Down Expand Up @@ -2451,19 +2451,19 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
Expect(err).ToNot(HaveOccurred())

By("verifying event occurred")
f.ExpectEvent(dataVolume.Namespace).Should(And(ContainSubstring(controller.ErrClaimNotValid), ContainSubstring("DataVolume spec is missing accessMode")))
f.ExpectEvent(dataVolume.Namespace).Should(And(ContainSubstring(controller.ErrClaimNotValid), ContainSubstring(dvc.MessageErrStorageClassNotFound)))

By("verifying conditions")
boundCondition := &cdiv1.DataVolumeCondition{
Type: cdiv1.DataVolumeBound,
Status: v1.ConditionUnknown,
Message: "DataVolume spec is missing accessMode",
Message: dvc.MessageErrStorageClassNotFound,
Reason: controller.ErrClaimNotValid,
}
readyCondition := &cdiv1.DataVolumeCondition{
Type: cdiv1.DataVolumeReady,
Status: v1.ConditionFalse,
Message: "DataVolume spec is missing accessMode",
Message: dvc.MessageErrStorageClassNotFound,
Reason: controller.ErrClaimNotValid,
}
utils.WaitForConditions(f, dataVolume.Name, f.Namespace.Name, timeout, pollingInterval, boundCondition, readyCondition)
Expand Down