From a727630335e531456584a56af1a75f1ad8fa6ece Mon Sep 17 00:00:00 2001 From: Alex Kalenyuk Date: Sun, 1 Oct 2023 17:17:51 +0300 Subject: [PATCH] Default virt storage class Signed-off-by: Alex Kalenyuk --- pkg/controller/common/util.go | 105 +++++++++++++++++++++++++---- pkg/controller/datavolume/util.go | 3 +- tests/clone-populator_test.go | 2 +- tests/cloner_test.go | 2 +- tests/datavolume_test.go | 108 +++++++++++++++++++++++++++++- tests/framework/framework.go | 18 +++-- tests/utils/datavolume.go | 27 +++++++- 7 files changed, 240 insertions(+), 25 deletions(-) diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go index 77ce1b1a52..618f422f37 100644 --- a/pkg/controller/common/util.go +++ b/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-class" // AnnOpenShiftImageLookup is the annotation for OpenShift image stream lookup AnnOpenShiftImageLookup = "alpha.image.policy.openshift.io/resolve-names" @@ -434,22 +437,50 @@ 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 +// GetStorageClassByName 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 GetStorageClassByName(ctx context.Context, client client.Client, name *string) (*storagev1.StorageClass, error) { + if name == nil { + return GetFallbackStorageClass(ctx, client, false) + } + // 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 +} + +// 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, considerVirtSc bool) (*storagev1.StorageClass, error) { + if name == nil { + return GetFallbackStorageClass(ctx, client, considerVirtSc) + } + + // look up storage class by name + 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) + } + + return storageClass, nil } // GetDefaultStorageClass returns the default storage class or nil if none found @@ -460,7 +491,7 @@ func GetDefaultStorageClass(ctx context.Context, client client.Client) (*storage return nil, errors.New("unable to retrieve storage classes") } for _, storageClass := range storageClasses.Items { - if storageClass.Annotations["storageclass.kubernetes.io/is-default-class"] == "true" { + if storageClass.Annotations[AnnDefaultStorageClass] == "true" { return &storageClass, nil } } @@ -468,6 +499,54 @@ func GetDefaultStorageClass(ctx context.Context, client client.Client) (*storage return nil, nil } +// GetFallbackStorageClass returns nil +func GetFallbackStorageClass(ctx context.Context, client client.Client, considerVirtStorageClass bool) (*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 considerVirtStorageClass { + 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[defaultAnnotationKey] == "true" { + defaultClasses = append(defaultClasses, storageClass) + } + } + + 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. func GetFilesystemOverheadForStorageClass(ctx context.Context, client client.Client, storageClassName *string) (cdiv1.Percent, error) { cdiConfig := &cdiv1.CDIConfig{} diff --git a/pkg/controller/datavolume/util.go b/pkg/controller/datavolume/util.go index 6d7fc5b7d2..7a4db740b9 100644 --- a/pkg/controller/datavolume/util.go +++ b/pkg/controller/datavolume/util.go @@ -94,7 +94,8 @@ func renderPvcSpecVolumeModeAndAccessModes(client client.Client, recorder record pvcSpec.VolumeMode = &volumeMode } - storageClass, err := cc.GetStorageClassByName(context.TODO(), client, dv.Spec.Storage.StorageClassName) + shouldConsiderVirt := cc.GetContentType(string(dv.Spec.ContentType)) == string(cdiv1.DataVolumeKubeVirt) + storageClass, err := cc.GetStorageClassByNameWithVirtFallback(context.TODO(), client, dv.Spec.Storage.StorageClassName, shouldConsiderVirt) if err != nil { return err } diff --git a/tests/clone-populator_test.go b/tests/clone-populator_test.go index 59e0307bfa..4334edb2cc 100644 --- a/tests/clone-populator_test.go +++ b/tests/clone-populator_test.go @@ -342,7 +342,7 @@ var _ = Describe("Clone Populator tests", func() { } sc, err := f.K8sClient.StorageV1().StorageClasses().Get(context.TODO(), utils.DefaultStorageClass.GetName(), metav1.GetOptions{}) Expect(err).ToNot(HaveOccurred()) - noExpansionStorageClass, err = f.CreateVariationOfStorageClass(sc, disableVolumeExpansion) + noExpansionStorageClass, err = f.CreateNonDefaultVariationOfStorageClass(sc, disableVolumeExpansion) Expect(err).ToNot(HaveOccurred()) }) diff --git a/tests/cloner_test.go b/tests/cloner_test.go index 3eea7decd5..efcf5ec60f 100644 --- a/tests/cloner_test.go +++ b/tests/cloner_test.go @@ -2716,7 +2716,7 @@ var _ = Describe("all clone tests", func() { } sc, err := f.K8sClient.StorageV1().StorageClasses().Get(context.TODO(), utils.DefaultStorageClass.GetName(), metav1.GetOptions{}) Expect(err).ToNot(HaveOccurred()) - noExpansionStorageClass, err = f.CreateVariationOfStorageClass(sc, disableVolumeExpansion) + noExpansionStorageClass, err = f.CreateNonDefaultVariationOfStorageClass(sc, disableVolumeExpansion) Expect(err).ToNot(HaveOccurred()) }) diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index f972662684..7443e2a136 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -1276,7 +1276,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Expect(err).ToNot(HaveOccurred()) // verify PVC was created - By("verifying pvc was created and is Bound") + By("verifying pvc was created") pvc, err := utils.WaitForPVC(f.K8sClient, dataVolume.Namespace, dataVolume.Name) Expect(err).ToNot(HaveOccurred()) @@ -1291,6 +1291,112 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Entry("for clone DataVolume", createCloneDataVolume, fillCommand), ) + Context("default virt storage class", func() { + var defaultVirtStorageClass *storagev1.StorageClass + var dummyStorageClass *storagev1.StorageClass + var defaultStorageClass *storagev1.StorageClass + + getDefaultStorageClassName := func() string { + return utils.DefaultStorageClass.GetName() + } + getDefaultVirtStorageClassName := func() string { + return defaultVirtStorageClass.GetName() + } + getDummyStorageClassName := func() string { + return dummyStorageClass.GetName() + } + importFunc := func() *cdiv1.DataVolume { + return utils.NewDataVolumeWithHTTPImportAndStorageSpec("dv-virt-sc-test-import", "1Gi", tinyCoreIsoURL()) + } + importFuncPVCAPI := func() *cdiv1.DataVolume { + return utils.NewDataVolumeWithHTTPImport("dv-virt-sc-test-import", "1Gi", tinyCoreIsoURL()) + } + importExplicitScFunc := func() *cdiv1.DataVolume { + dv := utils.NewDataVolumeWithHTTPImportAndStorageSpec("dv-virt-sc-test-import", "1Gi", tinyCoreIsoURL()) + sc := getDummyStorageClassName() + dv.Spec.Storage.StorageClassName = &sc + return dv + } + uploadFunc := func() *cdiv1.DataVolume { + return utils.NewDataVolumeForUploadWithStorageAPI("dv-virt-sc-test-upload", "1Gi") + } + cloneFunc := func() *cdiv1.DataVolume { + sourcePodFillerName := fmt.Sprintf("%s-filler-pod", dataVolumeName) + pvcDef := utils.NewPVCDefinition(pvcName, "1Gi", nil, nil) + sourcePvc := f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand) + + By(fmt.Sprintf("creating a new target PVC (datavolume) to clone %s", sourcePvc.Name)) + return utils.NewDataVolumeForImageCloningAndStorageSpec("dv-virt-sc-test-clone", "1Gi", f.Namespace.Name, sourcePvc.Name, nil, nil) + } + archiveFunc := func() *cdiv1.DataVolume { + return utils.NewDataVolumeWithArchiveContentStorage("dv-virt-sc-test-archive", "1Gi", tarArchiveURL()) + } + + BeforeEach(func() { + addVirtParam := func(sc *storagev1.StorageClass) { + if len(sc.Parameters) == 0 { + sc.Parameters = map[string]string{} + } + sc.Parameters["better.for.kubevirt.io"] = "true" + controller.AddAnnotation(sc, controller.AnnDefaultVirtStorageClass, "true") + } + addDummyAnn := func(sc *storagev1.StorageClass) { + controller.AddAnnotation(sc, "dummy", "true") + } + sc, err := f.K8sClient.StorageV1().StorageClasses().Get(context.TODO(), getDefaultStorageClassName(), metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + defaultStorageClass = sc + defaultVirtStorageClass, err = f.CreateNonDefaultVariationOfStorageClass(sc, addVirtParam) + Expect(err).ToNot(HaveOccurred()) + dummyStorageClass, err = f.CreateNonDefaultVariationOfStorageClass(sc, addDummyAnn) + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + err := f.K8sClient.StorageV1().StorageClasses().Delete(context.TODO(), defaultVirtStorageClass.Name, metav1.DeleteOptions{}) + Expect(err).ToNot(HaveOccurred()) + err = f.K8sClient.StorageV1().StorageClasses().Delete(context.TODO(), dummyStorageClass.Name, metav1.DeleteOptions{}) + Expect(err).ToNot(HaveOccurred()) + + if defaultStorageClass.Annotations[controller.AnnDefaultStorageClass] != "true" { + controller.AddAnnotation(defaultStorageClass, controller.AnnDefaultStorageClass, "true") + _, err = f.K8sClient.StorageV1().StorageClasses().Update(context.TODO(), defaultStorageClass, metav1.UpdateOptions{}) + Expect(err).ToNot(HaveOccurred()) + } + }) + + DescribeTable("Should", func(dvFunc func() *cdiv1.DataVolume, getExpectedStorageClassName func() string, removeDefault bool) { + var err error + // Default storage class exists check + _ = utils.GetDefaultStorageClass(f.K8sClient) + if removeDefault { + controller.AddAnnotation(defaultStorageClass, controller.AnnDefaultStorageClass, "false") + defaultStorageClass, err = f.K8sClient.StorageV1().StorageClasses().Update(context.TODO(), defaultStorageClass, metav1.UpdateOptions{}) + Expect(err).ToNot(HaveOccurred()) + } + + dataVolume := dvFunc() + By(fmt.Sprintf("creating new datavolume %s", dataVolume.Name)) + dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume) + Expect(err).ToNot(HaveOccurred()) + + // verify PVC was created + By("verifying pvc was created") + pvc, err := utils.WaitForPVC(f.K8sClient, dataVolume.Namespace, dataVolume.Name) + Expect(err).ToNot(HaveOccurred()) + + Expect(pvc.Spec.StorageClassName).To(HaveValue(Equal(getExpectedStorageClassName()))) + }, + Entry("respect default virt storage class for import DataVolume", importFunc, getDefaultVirtStorageClassName, false), + Entry("respect default virt storage class for upload DataVolume", uploadFunc, getDefaultVirtStorageClassName, false), + Entry("respect default virt storage class for clone DataVolume", cloneFunc, getDefaultVirtStorageClassName, false), + Entry("respect default virt storage class even if no k8s default exists", importFunc, getDefaultVirtStorageClassName, true), + Entry("not respect default virt storage class for contenType other than kubevirt", archiveFunc, getDefaultStorageClassName, false), + Entry("not respect default virt storage class for PVC api", importFuncPVCAPI, getDefaultStorageClassName, false), + Entry("not respect default virt storage class if explicit storage class provided", importExplicitScFunc, getDummyStorageClassName, false), + ) + }) + It("Should handle a pre populated DV", func() { By(fmt.Sprintf("initializing dataVolume marked as prePopulated %s", dataVolumeName)) dataVolume := utils.NewDataVolumeWithHTTPImport(dataVolumeName, "1Gi", cirrosURL()) diff --git a/tests/framework/framework.go b/tests/framework/framework.go index 1f736f0212..4cb38902cf 100644 --- a/tests/framework/framework.go +++ b/tests/framework/framework.go @@ -577,24 +577,28 @@ func (f *Framework) CreateWFFCVariationOfStorageClass(sc *storagev1.StorageClass sc.VolumeBindingMode = &wffc } - return f.CreateVariationOfStorageClass(sc, setWaitForFirstConsumer) + return f.CreateNonDefaultVariationOfStorageClass(sc, setWaitForFirstConsumer) } -// CreateVariationOfStorageClass creates a variation of a storage class following mutationFunc's changes -func (f *Framework) CreateVariationOfStorageClass(sc *storagev1.StorageClass, mutationFunc func(*storagev1.StorageClass)) (*storagev1.StorageClass, error) { +// CreateNonDefaultVariationOfStorageClass creates a variation of a storage class following mutationFunc's changes +func (f *Framework) CreateNonDefaultVariationOfStorageClass(sc *storagev1.StorageClass, mutationFunc func(*storagev1.StorageClass)) (*storagev1.StorageClass, error) { scCopy := sc.DeepCopy() - mutationFunc(sc) + mutationFunc(scCopy) if reflect.DeepEqual(sc, scCopy) { return sc, nil } - sc.ObjectMeta = metav1.ObjectMeta{ - GenerateName: fmt.Sprintf("%s-temp-variation", sc.Name), + if val, ok := scCopy.Annotations[cc.AnnDefaultStorageClass]; ok && val == "true" { + scCopy.Annotations[cc.AnnDefaultStorageClass] = "false" + } + scCopy.ObjectMeta = metav1.ObjectMeta{ + GenerateName: fmt.Sprintf("%s-temp-variation", scCopy.Name), Labels: map[string]string{ "cdi.kubevirt.io/testing": "", }, + Annotations: scCopy.Annotations, } - return f.K8sClient.StorageV1().StorageClasses().Create(context.TODO(), sc, metav1.CreateOptions{}) + return f.K8sClient.StorageV1().StorageClasses().Create(context.TODO(), scCopy, metav1.CreateOptions{}) } // UpdateCdiConfigResourceLimits sets the limits in the CDIConfig object diff --git a/tests/utils/datavolume.go b/tests/utils/datavolume.go index d492b149a7..31382ab6e3 100644 --- a/tests/utils/datavolume.go +++ b/tests/utils/datavolume.go @@ -969,7 +969,7 @@ func NewDataVolumeWithArchiveContent(dataVolumeName string, size string, httpURL URL: httpURL, }, }, - ContentType: "archive", + ContentType: cdiv1.DataVolumeArchive, PVC: &k8sv1.PersistentVolumeClaimSpec{ AccessModes: []k8sv1.PersistentVolumeAccessMode{k8sv1.ReadWriteOnce}, Resources: k8sv1.ResourceRequirements{ @@ -982,6 +982,31 @@ func NewDataVolumeWithArchiveContent(dataVolumeName string, size string, httpURL } } +// NewDataVolumeWithArchiveContentStorage initializes a DataVolume struct with 'archive' ContentType +func NewDataVolumeWithArchiveContentStorage(dataVolumeName string, size string, httpURL string) *cdiv1.DataVolume { + return &cdiv1.DataVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: dataVolumeName, + }, + Spec: cdiv1.DataVolumeSpec{ + Source: &cdiv1.DataVolumeSource{ + HTTP: &cdiv1.DataVolumeSourceHTTP{ + URL: httpURL, + }, + }, + ContentType: cdiv1.DataVolumeArchive, + Storage: &cdiv1.StorageSpec{ + AccessModes: []k8sv1.PersistentVolumeAccessMode{k8sv1.ReadWriteOnce}, + Resources: k8sv1.ResourceRequirements{ + Requests: k8sv1.ResourceList{ + k8sv1.ResourceName(k8sv1.ResourceStorage): resource.MustParse(size), + }, + }, + }, + }, + } +} + // PersistentVolumeClaimFromDataVolume creates a PersistentVolumeClaim definition so we can use PersistentVolumeClaim for various operations. func PersistentVolumeClaimFromDataVolume(datavolume *cdiv1.DataVolume) *corev1.PersistentVolumeClaim { // This function can work with DVs that use the 'Storage' field instead of the 'PVC' field