Fix StorageProfile PVC rendering (#3709)

When rendering a DataVolume storage-api-defined PVC:

We should not set its missing VolumeMode to any value if its specified
AccessMode has no match in the relevant StorageProfile. In this case,
fallback to k8s default (Filesysetem).

Similarly, we should not set its missing AccessModes if its specified
VolumeMode has no match in the relevant StorageProfile. In that case,
return an error as the PVC spec is incomplete.

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

pkg/controller/datavolume/BUILD.bazel

@@ -79,6 +79,7 @@ go_test(
         "//pkg/feature-gates:go_default_library",
         "//pkg/token:go_default_library",
         "//staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1:go_default_library",
+        "//vendor/github.com/go-logr/logr:go_default_library",
         "//vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1:go_default_library",
         "//vendor/github.com/onsi/ginkgo/v2:go_default_library",
         "//vendor/github.com/onsi/gomega:go_default_library",

pkg/controller/datavolume/util.go

@@ -179,7 +179,7 @@ func renderPvcSpecVolumeModeAndAccessModesAndStorageClass(client client.Client,
 		pvcSpec.AccessModes = append(pvcSpec.AccessModes, accessModes...)
 		pvcSpec.VolumeMode = volumeMode
 	} else if len(pvcSpec.AccessModes) == 0 {
-		accessModes, err := getDefaultAccessModes(client, storageClass, pvcSpec.VolumeMode)
+		accessModes, err := getDefaultAccessModes(client, storageClass, *pvcSpec.VolumeMode)
 		if err != nil {
 			logInfo("Cannot set accessMode for new pvc", "Error", err)
 			recordEventf(v1.EventTypeWarning, cc.ErrClaimNotValid,
@@ -358,6 +358,7 @@ func renderPvcSpecFromAvailablePv(c client.Client, pvcSpec *v1.PersistentVolumeC
 	return nil
 }
 
+// Get the StorageProfile preferred VolumeMode and AccessModes based on the StorageClass
 func getDefaultVolumeAndAccessMode(c client.Client, storageClass *storagev1.StorageClass) ([]v1.PersistentVolumeAccessMode, *v1.PersistentVolumeMode, error) {
 	if storageClass == nil {
 		return nil, nil, errors.Errorf("no accessMode specified and StorageClass not found")
@@ -380,6 +381,7 @@ func getDefaultVolumeAndAccessMode(c client.Client, storageClass *storagev1.Stor
 	return nil, nil, errors.Errorf("no accessMode specified in StorageProfile %s", storageProfile.Name)
 }
 
+// Get the StorageProfile preferred VolumeMode based on the StorageClass and AccessModes
 func getDefaultVolumeMode(c client.Client, storageClass *storagev1.StorageClass, pvcAccessModes []v1.PersistentVolumeAccessMode) (*v1.PersistentVolumeMode, error) {
 	if storageClass == nil {
 		// fallback to k8s defaults
@@ -392,10 +394,6 @@ func getDefaultVolumeMode(c client.Client, storageClass *storagev1.StorageClass,
 		return nil, errors.Wrap(err, "cannot get StorageProfile")
 	}
 	if len(storageProfile.Status.ClaimPropertySets) > 0 {
-		volumeMode := storageProfile.Status.ClaimPropertySets[0].VolumeMode
-		if len(pvcAccessModes) == 0 {
-			return volumeMode, nil
-		}
 		// check for volume mode matching with given pvc access modes
 		for _, cps := range storageProfile.Status.ClaimPropertySets {
 			for _, accessMode := range cps.AccessModes {
@@ -406,15 +404,14 @@ func getDefaultVolumeMode(c client.Client, storageClass *storagev1.StorageClass,
 				}
 			}
 		}
-		// if not found return default volume mode for the storage class
-		return volumeMode, nil
 	}
 
-	// since volumeMode is optional - > gracefully fallback to k8s defaults,
+	// since volumeMode is optional, gracefully fallback to k8s default when no matching volumeMode
 	return nil, nil
 }
 
-func getDefaultAccessModes(c client.Client, storageClass *storagev1.StorageClass, pvcVolumeMode *v1.PersistentVolumeMode) ([]v1.PersistentVolumeAccessMode, error) {
+// Get the StorageProfile preferred AccessModes based on the StorageClass and VolumeMode
+func getDefaultAccessModes(c client.Client, storageClass *storagev1.StorageClass, pvcVolumeMode v1.PersistentVolumeMode) ([]v1.PersistentVolumeAccessMode, error) {
 	if storageClass == nil {
 		return nil, errors.Errorf("no accessMode specified and no StorageProfile")
 	}
@@ -427,24 +424,17 @@ func getDefaultAccessModes(c client.Client, storageClass *storagev1.StorageClass
 
 	if len(storageProfile.Status.ClaimPropertySets) > 0 {
 		// check for access modes matching with given pvc volume mode
-		defaultAccessModes := []v1.PersistentVolumeAccessMode{}
 		for _, cps := range storageProfile.Status.ClaimPropertySets {
-			if cps.VolumeMode != nil && pvcVolumeMode != nil && *cps.VolumeMode == *pvcVolumeMode {
+			if cps.VolumeMode != nil && *cps.VolumeMode == pvcVolumeMode {
 				if len(cps.AccessModes) > 0 {
 					return cps.AccessModes, nil
 				}
-			} else if len(cps.AccessModes) > 0 && len(defaultAccessModes) == 0 {
-				defaultAccessModes = cps.AccessModes
 			}
 		}
-		// if not found return default access modes for the storage profile
-		if len(defaultAccessModes) > 0 {
-			return defaultAccessModes, nil
-		}
 	}
 
-	// no accessMode configured on storageProfile
-	return nil, errors.Errorf("no accessMode specified in StorageProfile %s", storageProfile.Name)
+	// no matching accessMode configured on storageProfile
+	return nil, errors.Errorf("no matching accessMode specified in StorageProfile %s", storageProfile.Name)
 }
 
 // storageClassCSIDriverExists returns true if the passed storage class has CSI drivers available

pkg/controller/datavolume/util_test.go

@@ -7,6 +7,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
+	"github.com/go-logr/logr"
 	snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	ocpconfigv1 "github.com/openshift/api/config/v1"
 
@@ -111,6 +112,69 @@ var _ = Describe("renderPvcSpecVolumeSize", func() {
 	})
 })
 
+var _ = Describe("renderPvcSpec", func() {
+	block := corev1.PersistentVolumeBlock
+	filesystem := corev1.PersistentVolumeFilesystem
+	rwo := corev1.ReadWriteOnce
+	rwx := corev1.ReadWriteMany
+
+	DescribeTable("Rendering pvcSpec based on storageProfile should", func(
+		storageVolumeMode *corev1.PersistentVolumeMode, storageAccessMode *corev1.PersistentVolumeAccessMode,
+		expectedVolumeMode *corev1.PersistentVolumeMode, expectedAccessMode *corev1.PersistentVolumeAccessMode,
+		expectedError *string) {
+		scName := "testSC"
+		sc := CreateStorageClassWithProvisioner(scName, nil, nil, "")
+		sp := createStorageProfile(scName, []corev1.PersistentVolumeAccessMode{rwo}, block)
+		cdiconfig := &cdiv1.CDIConfig{ObjectMeta: metav1.ObjectMeta{Name: "config"}}
+		client := createClient(sc, sp, cdiconfig)
+
+		storageSpec := &cdiv1.StorageSpec{
+			StorageClassName: &scName,
+			Resources: corev1.VolumeResourceRequirements{
+				Requests: corev1.ResourceList{
+					corev1.ResourceStorage: resource.MustParse("1Gi"),
+				},
+			},
+		}
+		if storageVolumeMode != nil {
+			storageSpec.VolumeMode = storageVolumeMode
+		}
+		if storageAccessMode != nil {
+			storageSpec.AccessModes = []corev1.PersistentVolumeAccessMode{*storageAccessMode}
+		}
+		dv := createDataVolumeWithStorageAPI("testDV", metav1.NamespaceDefault, &cdiv1.DataVolumeSource{}, storageSpec)
+
+		pvcSpec, err := renderPvcSpec(client, nil, logr.Logger{}, dv, nil)
+		if expectedError != nil {
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring(*expectedError))
+			Expect(pvcSpec).To(BeNil())
+			return
+		}
+
+		Expect(err).ToNot(HaveOccurred())
+		if expectedVolumeMode != nil {
+			Expect(pvcSpec.VolumeMode).ToNot(BeNil())
+			Expect(*pvcSpec.VolumeMode).To(Equal(*expectedVolumeMode))
+		} else {
+			Expect(pvcSpec.VolumeMode).To(BeNil())
+		}
+		if expectedAccessMode != nil {
+			Expect(pvcSpec.AccessModes).ToNot(BeNil())
+			Expect(pvcSpec.AccessModes[0]).To(Equal(*expectedAccessMode))
+		} else {
+			Expect(pvcSpec.AccessModes).To(BeNil())
+		}
+	},
+		Entry("set default volumeMode and accessMode if not passed", nil, nil, &block, &rwo, nil),
+		Entry("set a matching accessMode for the volumeMode", &block, nil, &block, &rwo, nil),
+		Entry("set a matching volumeMode for the accessMode", nil, &rwo, &block, &rwo, nil),
+		Entry("fail when volumeMode has no matching accessMode", &filesystem, nil, nil, nil, ptr.To("no matching accessMode specified in StorageProfile testSC")),
+		Entry("fallback to k8s default when accessMode has no matching volumeMode", nil, &rwx, nil, &rwx, nil),
+		Entry("use the passed volumeMode and accessMode even if not in storageProfile", &filesystem, &rwx, &filesystem, &rwx, nil),
+	)
+})
+
 var _ = Describe("updateDataVolumeDefaultInstancetypeLabels", func() {
 
 	const (

tests/datavolume_test.go

@@ -2138,7 +2138,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
 				v1.PersistentVolumeFilesystem)
 			requestedSize := resource.MustParse("100Mi")
 
-			expectedVolumeMode := v1.PersistentVolumeBlock
+			expectedVolumeMode := v1.PersistentVolumeFilesystem
 			spec := cdiv1.StorageSpec{
 				AccessModes: nil,
 				VolumeMode:  &expectedVolumeMode,