Merge pull request kubernetes#127790 from torredil/fix-e2e-retroactive-sc-12513

Fix Storage e2e test `should assign default SC to PVCs that have no SC set`

Author: k8s-ci-robot

test/e2e/storage/pvc_storageclass.go

@@ -18,172 +18,152 @@ package storage
 
 import (
 	"context"
-	"fmt"
 	"time"
 
+	"k8s.io/klog/v2"
+
 	"github.com/onsi/ginkgo/v2"
 	"github.com/onsi/gomega"
-
 	v1 "k8s.io/api/core/v1"
 	storagev1 "k8s.io/api/storage/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/wait"
 	clientset "k8s.io/client-go/kubernetes"
 	storageutil "k8s.io/kubernetes/pkg/apis/storage/util"
 	"k8s.io/kubernetes/test/e2e/framework"
 	e2epv "k8s.io/kubernetes/test/e2e/framework/pv"
-	"k8s.io/kubernetes/test/e2e/storage/testsuites"
 	"k8s.io/kubernetes/test/e2e/storage/utils"
 	admissionapi "k8s.io/pod-security-admission/api"
 )
 
-var _ = utils.SIGDescribe("Persistent Volume Claim and StorageClass", func() {
-	f := framework.NewDefaultFramework("pvc-retroactive-storageclass")
+var _ = utils.SIGDescribe("Retroactive StorageClass Assignment", func() {
+	f := framework.NewDefaultFramework("retroactive-storageclass")
 	f.NamespacePodSecurityLevel = admissionapi.LevelBaseline
 
 	var (
 		client    clientset.Interface
 		namespace string
-		prefixPVC string
-		prefixSC  string
-		t         testsuites.StorageClassTest
-		pvc       *v1.PersistentVolumeClaim
-		err       error
 	)
 
 	ginkgo.BeforeEach(func() {
 		client = f.ClientSet
 		namespace = f.Namespace.Name
-		prefixPVC = "retro-pvc-"
-		prefixSC = "retro"
-		t = testsuites.StorageClassTest{
-			Timeouts:  f.Timeouts,
-			ClaimSize: "1Gi",
-		}
 	})
 
-	f.Describe("Retroactive StorageClass assignment", framework.WithSerial(), framework.WithDisruptive(), func() {
-		ginkgo.It("should assign default SC to PVCs that have no SC set", func(ctx context.Context) {
-
-			// Temporarily set all default storage classes as non-default
-			restoreClasses := temporarilyUnsetDefaultClasses(ctx, client)
-			defer restoreClasses()
-
-			// Create PVC with nil SC
-			pvcObj := e2epv.MakePersistentVolumeClaim(e2epv.PersistentVolumeClaimConfig{
-				NamePrefix: prefixPVC,
-				ClaimSize:  t.ClaimSize,
-				VolumeMode: &t.VolumeMode,
-			}, namespace)
-			pvc, err = client.CoreV1().PersistentVolumeClaims(pvcObj.Namespace).Create(ctx, pvcObj, metav1.CreateOptions{})
-			framework.ExpectNoError(err, "Error creating PVC")
-			defer func(pvc *v1.PersistentVolumeClaim) {
-				// Remove test PVC
-				err := client.CoreV1().PersistentVolumeClaims(pvc.Namespace).Delete(ctx, pvc.Name, metav1.DeleteOptions{})
-				framework.ExpectNoError(err, "Error cleaning up PVC")
-			}(pvc)
-
-			// Create custom default SC
-			storageClass := testsuites.SetupStorageClass(ctx, client, makeStorageClass(prefixSC))
-
-			// Wait for PVC to get updated with the new default SC
-			pvc, err = waitForPVCStorageClass(ctx, client, namespace, pvc.Name, storageClass.Name, f.Timeouts.ClaimBound)
-			framework.ExpectNoError(err, "Error updating PVC with the correct storage class")
-
-			// Create PV with specific class
-			pv := e2epv.MakePersistentVolume(e2epv.PersistentVolumeConfig{
-				NamePrefix:       "pv-",
-				StorageClassName: storageClass.Name,
-				VolumeMode:       pvc.Spec.VolumeMode,
-				PVSource: v1.PersistentVolumeSource{
-					HostPath: &v1.HostPathVolumeSource{
-						Path: "/tmp/test",
-					},
-				},
-			})
-			_, err = e2epv.CreatePV(ctx, client, f.Timeouts, pv)
-			framework.ExpectNoError(err, "Error creating pv %v", err)
-			ginkgo.DeferCleanup(e2epv.DeletePersistentVolume, client, pv.Name)
-
-			// Verify the PVC is bound and has the new default SC
-			claimNames := []string{pvc.Name}
-			err = e2epv.WaitForPersistentVolumeClaimsPhase(ctx, v1.ClaimBound, client, namespace, claimNames, 2*time.Second /* Poll */, t.Timeouts.ClaimProvisionShort, false)
-			framework.ExpectNoError(err)
-			updatedPVC, err := client.CoreV1().PersistentVolumeClaims(namespace).Get(ctx, pvc.Name, metav1.GetOptions{})
-			framework.ExpectNoError(err)
-			gomega.Expect(*updatedPVC.Spec.StorageClassName).To(gomega.Equal(storageClass.Name), "Expected PVC %v to have StorageClass %v, but it has StorageClass %v instead", updatedPVC.Name, prefixSC, updatedPVC.Spec.StorageClassName)
-			framework.Logf("Success - PersistentVolumeClaim %s got updated retroactively with StorageClass %v", updatedPVC.Name, storageClass.Name)
+	f.It("should assign default StorageClass to PVCs retroactively", f.WithDisruptive(), f.WithSerial(), func(ctx context.Context) {
+		defaultSCs, err := getDefaultStorageClasses(ctx, client)
+		framework.ExpectNoError(err, "Failed to get default StorageClasses")
+
+		ginkgo.DeferCleanup(func(cleanupContext context.Context) {
+			// Restore existing default StorageClasses at the end of the test
+			for _, sc := range defaultSCs {
+				setStorageClassDefault(cleanupContext, client, sc.Name, "true")
+			}
+		})
+
+		// Unset all default StorageClasses
+		for _, sc := range defaultSCs {
+			klog.InfoS("Unsetting default StorageClass", "StorageClass", sc.Name)
+			setStorageClassDefault(ctx, client, sc.Name, "false")
+		}
+
+		// Ensure no default StorageClasses exist
+		if len(defaultSCs) > 0 {
+			ensureNoDefaultStorageClasses(ctx, client)
+		}
+
+		// Create a PVC with nil StorageClass
+		pvc := createPVC(ctx, client, namespace)
+		ginkgo.DeferCleanup(func(cleanupContext context.Context) {
+			err := client.CoreV1().PersistentVolumeClaims(namespace).Delete(ctx, pvc.Name, *metav1.NewDeleteOptions(0))
+			framework.ExpectNoError(err, "Error deleting PVC")
 		})
+
+		// Create a default StorageClass
+		sc := createDefaultStorageClass(ctx, client)
+		ginkgo.DeferCleanup(func(cleanupContext context.Context) {
+			deleteStorageClass(cleanupContext, client, sc.Name)
+		})
+
+		// Verify that the PVC is assigned the default StorageClass
+		gomega.Eventually(ctx, framework.GetObject(client.CoreV1().PersistentVolumeClaims(pvc.Namespace).Get, pvc.Name, metav1.GetOptions{})).
+			WithPolling(framework.Poll).
+			WithTimeout(2*time.Minute).
+			Should(gomega.HaveField("Spec.StorageClassName", gomega.Equal(&sc.Name)),
+				"failed to wait for PVC to have the storageclass %s", sc.Name)
 	})
 })
 
-func makeStorageClass(prefixSC string) *storagev1.StorageClass {
-	return &storagev1.StorageClass{
-		TypeMeta: metav1.TypeMeta{
-			Kind: "StorageClass",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			GenerateName: prefixSC,
-			Annotations: map[string]string{
-				storageutil.IsDefaultStorageClassAnnotation: "true",
-			},
-		},
-		Provisioner: "fake-1",
+func getDefaultStorageClasses(ctx context.Context, client clientset.Interface) ([]storagev1.StorageClass, error) {
+	scList, err := client.StorageV1().StorageClasses().List(ctx, metav1.ListOptions{})
+	if err != nil {
+		return nil, err
 	}
-}
-
-func temporarilyUnsetDefaultClasses(ctx context.Context, client clientset.Interface) func() {
-	classes, err := client.StorageV1().StorageClasses().List(ctx, metav1.ListOptions{})
-	framework.ExpectNoError(err)
-
-	changedClasses := make(map[string]bool)
 
-	for _, sc := range classes.Items {
+	var defaultSCs []storagev1.StorageClass
+	for _, sc := range scList.Items {
 		if sc.Annotations[storageutil.IsDefaultStorageClassAnnotation] == "true" {
-			changedClasses[sc.GetName()] = true
-			sc.Annotations[storageutil.IsDefaultStorageClassAnnotation] = "false"
-			_, err := client.StorageV1().StorageClasses().Update(ctx, &sc, metav1.UpdateOptions{})
-			framework.ExpectNoError(err)
+			defaultSCs = append(defaultSCs, sc)
 		}
 	}
+	return defaultSCs, nil
+}
 
-	return func() {
-		classes, err = client.StorageV1().StorageClasses().List(ctx, metav1.ListOptions{})
-		framework.ExpectNoError(err)
-		for _, sc := range classes.Items {
-			if _, found := changedClasses[sc.GetName()]; found {
-				sc.Annotations[storageutil.IsDefaultStorageClassAnnotation] = "true"
-				_, err := client.StorageV1().StorageClasses().Update(ctx, &sc, metav1.UpdateOptions{})
-				framework.ExpectNoError(err)
-			}
-		}
+func setStorageClassDefault(ctx context.Context, client clientset.Interface, scName string, isDefault string) {
+	sc, err := client.StorageV1().StorageClasses().Get(ctx, scName, metav1.GetOptions{})
+	framework.ExpectNoError(err, "Error getting StorageClass")
+
+	if sc.Annotations == nil {
+		sc.Annotations = make(map[string]string)
 	}
+	sc.Annotations[storageutil.IsDefaultStorageClassAnnotation] = isDefault
 
+	_, err = client.StorageV1().StorageClasses().Update(ctx, sc, metav1.UpdateOptions{})
+	framework.ExpectNoError(err, "Error updating StorageClass")
 }
 
-func waitForPVCStorageClass(ctx context.Context, c clientset.Interface, namespace, pvcName, scName string, timeout time.Duration) (*v1.PersistentVolumeClaim, error) {
-	var watchedPVC *v1.PersistentVolumeClaim
-
-	err := wait.PollUntilContextTimeout(ctx, 1*time.Second, timeout, false, func(ctx context.Context) (bool, error) {
-		var err error
-		watchedPVC, err = c.CoreV1().PersistentVolumeClaims(namespace).Get(ctx, pvcName, metav1.GetOptions{})
+func ensureNoDefaultStorageClasses(ctx context.Context, client clientset.Interface) {
+	gomega.Eventually(ctx, func() (int, error) {
+		defaultSCs, err := getDefaultStorageClasses(ctx, client)
 		if err != nil {
-			return true, err
+			return 0, err
 		}
+		return len(defaultSCs), nil
+	}).WithPolling(framework.Poll).WithTimeout(2*time.Minute).
+		Should(gomega.BeZero(), "Expected no default StorageClasses")
+}
 
-		if watchedPVC.Spec.StorageClassName == nil {
-			return false, nil // Poll until PVC has correct SC
-		}
-		if watchedPVC.Spec.StorageClassName != nil && *watchedPVC.Spec.StorageClassName != scName {
-			framework.Logf("PersistentVolumeClaim %s has unexpected StorageClass %v, expected StorageClass is: %v", watchedPVC.Name, *watchedPVC.Spec.StorageClassName, scName)
-			return false, nil // Log unexpected SC and continue poll (something might be changing default SC)
-		}
-		return true, nil // Correct SC name found on PVC
-	})
+func createPVC(ctx context.Context, client clientset.Interface, namespace string) *v1.PersistentVolumeClaim {
+	ginkgo.By("Creating a PVC")
 
-	if err != nil {
-		return watchedPVC, fmt.Errorf("error waiting for claim %s to have StorageClass set to %s: %w", pvcName, scName, err)
+	c := e2epv.PersistentVolumeClaimConfig{
+		Name: "test-pvc",
+	}
+
+	pvcObj := e2epv.MakePersistentVolumeClaim(c, namespace)
+	pvc, err := client.CoreV1().PersistentVolumeClaims(pvcObj.Namespace).Create(ctx, pvcObj, metav1.CreateOptions{})
+	framework.ExpectNoError(err, "Error creating PVC")
+
+	return pvc
+}
+
+func createDefaultStorageClass(ctx context.Context, client clientset.Interface) *storagev1.StorageClass {
+	ginkgo.By("Creating a default StorageClass")
+
+	c := &storagev1.StorageClass{
+		TypeMeta: metav1.TypeMeta{
+			Kind: "StorageClass",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-default-sc",
+			Annotations: map[string]string{
+				storageutil.IsDefaultStorageClassAnnotation: "true",
+			},
+		},
+		Provisioner: "fake-1",
 	}
 
-	return watchedPVC, nil
+	sc, err := client.StorageV1().StorageClasses().Create(ctx, c, metav1.CreateOptions{})
+	framework.ExpectNoError(err, "Error creating StorageClass")
+
+	return sc
 }