Default storage class for virtualization purposes (#2913)

* Default virt storage class

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Add alert for multiple default virt storage classes

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* Refactor content type funcs to not return strings

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

---------

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

cmd/cdi-controller/controller.go

@@ -369,6 +369,7 @@ func getTokenPrivateKey() *rsa.PrivateKey {
 func registerMetrics() {
 	metrics.Registry.MustRegister(controller.IncompleteProfileGauge)
 	controller.IncompleteProfileGauge.Set(-1)
+	metrics.Registry.MustRegister(controller.DefaultVirtStorageClassesGauge)
 	metrics.Registry.MustRegister(controller.DataImportCronOutdatedGauge)
 }
 

doc/metrics.md

@@ -11,6 +11,8 @@ CDI install ready. Type: Gauge.
 DataImportCron has an outdated import. Type: Gauge.
 ### kubevirt_cdi_dataimportcron_outdated_aggregated
 Total count of outdated DataImportCron imports. Type: Gauge.
+### kubevirt_cdi_default_virt_storageclasses
+Number of default virt storage classes currently configured. Type: Gauge.
 ### kubevirt_cdi_import_pods_high_restart
 The number of CDI import pods with high restart count. Type: Gauge.
 ### kubevirt_cdi_incomplete_storageprofiles

pkg/controller/clone-controller.go

@@ -753,7 +753,7 @@ func ValidateCanCloneSourceAndTargetContentType(sourcePvc, targetPvc *corev1.Per
 	if sourceContentType != targetContentType {
 		return "", fmt.Errorf("source contentType (%s) and target contentType (%s) do not match", sourceContentType, targetContentType)
 	}
-	return cdiv1.DataVolumeContentType(sourceContentType), nil
+	return sourceContentType, nil
 }
 
 // ValidateCanCloneSourceAndTargetSpec validates the specs passed in are compatible for cloning.

pkg/controller/common/util.go

@@ -27,6 +27,7 @@ import (
 	"net/http"
 	"reflect"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -211,6 +212,8 @@ const (
 
 	//AnnDefaultStorageClass is the annotation indicating that a storage class is the default one.
 	AnnDefaultStorageClass = "storageclass.kubernetes.io/is-default-class"
+	// AnnDefaultVirtStorageClass is the annotation indicating that a storage class is the default one for virtualization purposes
+	AnnDefaultVirtStorageClass = "storageclass.kubevirt.io/is-default-virt-class"
 
 	// AnnOpenShiftImageLookup is the annotation for OpenShift image stream lookup
 	AnnOpenShiftImageLookup = "alpha.image.policy.openshift.io/resolve-names"
@@ -434,38 +437,90 @@ func GetVolumeMode(pvc *corev1.PersistentVolumeClaim) corev1.PersistentVolumeMod
 	return util.ResolveVolumeMode(pvc.Spec.VolumeMode)
 }
 
-// GetStorageClassByName looks up the storage class based on the name. If no storage class is found returns nil
-func GetStorageClassByName(ctx context.Context, client client.Client, name *string) (*storagev1.StorageClass, error) {
+// getStorageClassByName looks up the storage class based on the name.
+// If name is nil, it performs fallback to default according to the provided content type
+// If no storage class is found, returns nil
+func getStorageClassByName(ctx context.Context, client client.Client, name *string, contentType cdiv1.DataVolumeContentType) (*storagev1.StorageClass, error) {
+	if name == nil {
+		return getFallbackStorageClass(ctx, client, contentType)
+	}
+
 	// look up storage class by name
-	if name != nil {
-		storageClass := &storagev1.StorageClass{}
-		if err := client.Get(ctx, types.NamespacedName{Name: *name}, storageClass); err != nil {
-			if k8serrors.IsNotFound(err) {
-				return nil, nil
-			}
-			klog.V(3).Info("Unable to retrieve storage class", "storage class name", *name)
-			return nil, errors.Errorf("unable to retrieve storage class %s", *name)
+	storageClass := &storagev1.StorageClass{}
+	if err := client.Get(ctx, types.NamespacedName{Name: *name}, storageClass); err != nil {
+		if k8serrors.IsNotFound(err) {
+			return nil, nil
 		}
-		return storageClass, nil
+		klog.V(3).Info("Unable to retrieve storage class", "storage class name", *name)
+		return nil, errors.Errorf("unable to retrieve storage class %s", *name)
 	}
-	// No storage class found, just return nil for storage class and let caller deal with it.
-	return GetDefaultStorageClass(ctx, client)
+
+	return storageClass, nil
 }
 
-// GetDefaultStorageClass returns the default storage class or nil if none found
-func GetDefaultStorageClass(ctx context.Context, client client.Client) (*storagev1.StorageClass, error) {
+// GetStorageClassByNameWithK8sFallback looks up the storage class based on the name
+// If name is nil, it looks for the default k8s storage class storageclass.kubernetes.io/is-default-class
+// If no storage class is found, returns nil
+func GetStorageClassByNameWithK8sFallback(ctx context.Context, client client.Client, name *string) (*storagev1.StorageClass, error) {
+	return getStorageClassByName(ctx, client, name, cdiv1.DataVolumeArchive)
+}
+
+// GetStorageClassByNameWithVirtFallback looks up the storage class based on the name
+// If name is nil, it looks for the following, in this order:
+// default kubevirt storage class (if the caller is interested) storageclass.kubevirt.io/is-default-class
+// default k8s storage class storageclass.kubernetes.io/is-default-class
+// If no storage class is found, returns nil
+func GetStorageClassByNameWithVirtFallback(ctx context.Context, client client.Client, name *string, contentType cdiv1.DataVolumeContentType) (*storagev1.StorageClass, error) {
+	return getStorageClassByName(ctx, client, name, contentType)
+}
+
+// getFallbackStorageClass looks for a default virt/k8s storage class according to the boolean
+// If no storage class is found, returns nil
+func getFallbackStorageClass(ctx context.Context, client client.Client, contentType cdiv1.DataVolumeContentType) (*storagev1.StorageClass, error) {
 	storageClasses := &storagev1.StorageClassList{}
 	if err := client.List(ctx, storageClasses); err != nil {
 		klog.V(3).Info("Unable to retrieve available storage classes")
 		return nil, errors.New("unable to retrieve storage classes")
 	}
+
+	if GetContentType(contentType) == cdiv1.DataVolumeKubeVirt {
+		virtSc := GetPlatformDefaultStorageClass(ctx, storageClasses, AnnDefaultVirtStorageClass)
+		if virtSc != nil {
+			return virtSc, nil
+		}
+	}
+	return GetPlatformDefaultStorageClass(ctx, storageClasses, AnnDefaultStorageClass), nil
+}
+
+// GetPlatformDefaultStorageClass returns the default storage class according to the provided annotation or nil if none found
+func GetPlatformDefaultStorageClass(ctx context.Context, storageClasses *storagev1.StorageClassList, defaultAnnotationKey string) *storagev1.StorageClass {
+	defaultClasses := []storagev1.StorageClass{}
+
 	for _, storageClass := range storageClasses.Items {
-		if storageClass.Annotations["storageclass.kubernetes.io/is-default-class"] == "true" {
-			return &storageClass, nil
+		if storageClass.Annotations[defaultAnnotationKey] == "true" {
+			defaultClasses = append(defaultClasses, storageClass)
 		}
 	}
 
-	return nil, nil
+	if len(defaultClasses) == 0 {
+		return nil
+	}
+
+	// Primary sort by creation timestamp, newest first
+	// Secondary sort by class name, ascending order
+	// Follows k8s behavior
+	// https://github.com/kubernetes/kubernetes/blob/731068288e112c8b5af70f676296cc44661e84f4/pkg/volume/util/storageclass.go#L58-L59
+	sort.Slice(defaultClasses, func(i, j int) bool {
+		if defaultClasses[i].CreationTimestamp.UnixNano() == defaultClasses[j].CreationTimestamp.UnixNano() {
+			return defaultClasses[i].Name < defaultClasses[j].Name
+		}
+		return defaultClasses[i].CreationTimestamp.UnixNano() > defaultClasses[j].CreationTimestamp.UnixNano()
+	})
+	if len(defaultClasses) > 1 {
+		klog.V(3).Infof("%d default StorageClasses were found, choosing: %s", len(defaultClasses), defaultClasses[0].Name)
+	}
+
+	return &defaultClasses[0]
 }
 
 // GetFilesystemOverheadForStorageClass determines the filesystem overhead defined in CDIConfig for the storageClass.
@@ -480,10 +535,10 @@ func GetFilesystemOverheadForStorageClass(ctx context.Context, client client.Cli
 		return "0", err
 	}
 
-	targetStorageClass, err := GetStorageClassByName(ctx, client, storageClassName)
+	targetStorageClass, err := GetStorageClassByNameWithK8sFallback(ctx, client, storageClassName)
 	if err != nil || targetStorageClass == nil {
 		klog.V(3).Info("Storage class", storageClassName, "not found, trying default storage class")
-		targetStorageClass, err = GetStorageClassByName(ctx, client, nil)
+		targetStorageClass, err = GetStorageClassByNameWithK8sFallback(ctx, client, nil)
 		if err != nil {
 			klog.V(3).Info("No default storage class found, continuing with global overhead")
 			return cdiConfig.Status.FilesystemOverhead.Global, nil
@@ -1230,7 +1285,7 @@ func CreateImporterTestPod(pvc *corev1.PersistentVolumeClaim, dvname string, scr
 		},
 		{
 			Name:  common.ImporterContentType,
-			Value: contentType,
+			Value: string(contentType),
 		},
 		{
 			Name:  common.ImporterImageSize,
@@ -1291,27 +1346,27 @@ func ErrQuotaExceeded(err error) bool {
 }
 
 // GetContentType returns the content type. If invalid or not set, default to kubevirt
-func GetContentType(contentType string) string {
+func GetContentType(contentType cdiv1.DataVolumeContentType) cdiv1.DataVolumeContentType {
 	switch contentType {
 	case
-		string(cdiv1.DataVolumeKubeVirt),
-		string(cdiv1.DataVolumeArchive):
+		cdiv1.DataVolumeKubeVirt,
+		cdiv1.DataVolumeArchive:
 	default:
 		// TODO - shouldn't archive be the default?
-		contentType = string(cdiv1.DataVolumeKubeVirt)
+		contentType = cdiv1.DataVolumeKubeVirt
 	}
 	return contentType
 }
 
 // GetPVCContentType returns the content type of the source image. If invalid or not set, default to kubevirt
-func GetPVCContentType(pvc *corev1.PersistentVolumeClaim) string {
+func GetPVCContentType(pvc *corev1.PersistentVolumeClaim) cdiv1.DataVolumeContentType {
 	contentType, found := pvc.Annotations[AnnContentType]
 	if !found {
 		// TODO - shouldn't archive be the default?
-		return string(cdiv1.DataVolumeKubeVirt)
+		return cdiv1.DataVolumeKubeVirt
 	}
 
-	return GetContentType(contentType)
+	return GetContentType(cdiv1.DataVolumeContentType(contentType))
 }
 
 // GetNamespace returns the given namespace if not empty, otherwise the default namespace
@@ -1811,7 +1866,7 @@ func GetSnapshotClassForSmartClone(dvName string, targetPvcStorageClassName *str
 		return "", nil
 	}
 
-	targetStorageClass, err := GetStorageClassByName(context.TODO(), client, targetPvcStorageClassName)
+	targetStorageClass, err := GetStorageClassByNameWithK8sFallback(context.TODO(), client, targetPvcStorageClassName)
 	if err != nil {
 		return "", err
 	}

pkg/controller/common/util_test.go

@@ -2,6 +2,7 @@ package common
 
 import (
 	"context"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -56,15 +57,15 @@ var _ = Describe("validateContentTypes", func() {
 	)
 })
 
-var _ = Describe("GetDefaultStorageClass", func() {
+var _ = Describe("GetStorageClassByName", func() {
 	It("Should return the default storage class name", func() {
 		client := CreateClient(
 			CreateStorageClass("test-storage-class-1", nil),
 			CreateStorageClass("test-storage-class-2", map[string]string{
 				AnnDefaultStorageClass: "true",
 			}),
 		)
-		sc, _ := GetDefaultStorageClass(context.Background(), client)
+		sc, _ := GetStorageClassByNameWithK8sFallback(context.Background(), client, nil)
 		Expect(sc.Name).To(Equal("test-storage-class-2"))
 	})
 
@@ -73,9 +74,54 @@ var _ = Describe("GetDefaultStorageClass", func() {
 			CreateStorageClass("test-storage-class-1", nil),
 			CreateStorageClass("test-storage-class-2", nil),
 		)
-		sc, _ := GetDefaultStorageClass(context.Background(), client)
+		sc, _ := GetStorageClassByNameWithK8sFallback(context.Background(), client, nil)
 		Expect(sc).To(BeNil())
 	})
+
+	It("Should return default virt class even if there's not default k8s storage class", func() {
+		client := CreateClient(
+			CreateStorageClass("test-storage-class-1", nil),
+			CreateStorageClass("test-storage-class-2", map[string]string{
+				AnnDefaultVirtStorageClass: "true",
+			}),
+		)
+		sc, _ := GetStorageClassByNameWithVirtFallback(context.Background(), client, nil, cdiv1.DataVolumeKubeVirt)
+		Expect(sc.Name).To(Equal("test-storage-class-2"))
+	})
+
+	DescribeTable("Should return newer default", func(annotation string) {
+		olderSc := CreateStorageClass("test-storage-class-new", map[string]string{
+			annotation: "true",
+		})
+		olderSc.SetCreationTimestamp(metav1.NewTime(time.Now().Add(-1 * time.Second)))
+		newerSc := CreateStorageClass("test-storage-class-old", map[string]string{
+			annotation: "true",
+		})
+		newerSc.SetCreationTimestamp(metav1.NewTime(time.Now()))
+		client := CreateClient(newerSc, olderSc)
+		sc, _ := GetStorageClassByNameWithVirtFallback(context.Background(), client, nil, cdiv1.DataVolumeKubeVirt)
+		Expect(sc.Name).To(Equal(newerSc.Name))
+	},
+		Entry("virt storage class", AnnDefaultVirtStorageClass),
+		Entry("k8s storage class", AnnDefaultStorageClass),
+	)
+
+	DescribeTable("Should fall back to lexicographic order when same timestamp", func(annotation string) {
+		firstSc := CreateStorageClass("test-storage-class-1", map[string]string{
+			annotation: "true",
+		})
+		firstSc.SetCreationTimestamp(metav1.NewTime(time.Now()))
+		secondSc := CreateStorageClass("test-storage-class-2", map[string]string{
+			annotation: "true",
+		})
+		secondSc.SetCreationTimestamp(metav1.NewTime(time.Now()))
+		client := CreateClient(firstSc, secondSc)
+		sc, _ := GetStorageClassByNameWithVirtFallback(context.Background(), client, nil, cdiv1.DataVolumeKubeVirt)
+		Expect(sc.Name).To(Equal(firstSc.Name))
+	},
+		Entry("virt storage class", AnnDefaultVirtStorageClass),
+		Entry("k8s storage class", AnnDefaultStorageClass),
+	)
 })
 
 var _ = Describe("Rebind", func() {

pkg/controller/dataimportcron-controller.go

@@ -319,7 +319,7 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
 
 	dataVolume := dataImportCron.Spec.Template
 	explicitScName := getStorageClassFromTemplate(&dataVolume)
-	desiredStorageClass, err := cc.GetStorageClassByName(ctx, r.client, explicitScName)
+	desiredStorageClass, err := cc.GetStorageClassByNameWithVirtFallback(ctx, r.client, explicitScName, dataVolume.Spec.ContentType)
 	if err != nil {
 		return res, err
 	}
@@ -1007,7 +1007,7 @@ func addDataImportCronControllerWatches(mgr manager.Manager, c controller.Contro
 		for _, cron := range crons.Items {
 			dataVolume := cron.Spec.Template
 			explicitScName := getStorageClassFromTemplate(&dataVolume)
-			templateSc, err := cc.GetStorageClassByName(context.TODO(), mgr.GetClient(), explicitScName)
+			templateSc, err := cc.GetStorageClassByNameWithVirtFallback(context.TODO(), mgr.GetClient(), explicitScName, dataVolume.Spec.ContentType)
 			if err != nil || templateSc == nil {
 				c.GetLogger().Error(err, "Unable to get storage class", "templateSc", templateSc)
 				return