Fix volume modify e2e tests because pv controller set the current vac once the pvc is bound

test/e2e/framework/pv/wait.go

@@ -0,0 +1,70 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package pv
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	clientset "k8s.io/client-go/kubernetes"
+	"k8s.io/kubernetes/test/e2e/framework"
+	"k8s.io/kubernetes/test/utils/format"
+	"k8s.io/utils/ptr"
+)
+
+// WaitForPersistentVolumeClaimModified waits the given timeout duration for the specified claim to become bound with the
+// desired volume attributes class.
+// Returns an error if timeout occurs first.
+func WaitForPersistentVolumeClaimModified(ctx context.Context, c clientset.Interface, claim *v1.PersistentVolumeClaim, timeout time.Duration) error {
+	desiredClass := ptr.Deref(claim.Spec.VolumeAttributesClassName, "")
+
+	var match = func(claim *v1.PersistentVolumeClaim) bool {
+		for _, condition := range claim.Status.Conditions {
+			// conditions that indicate the claim is being modified
+			// or has an error when modifying the volume
+			if condition.Type == v1.PersistentVolumeClaimVolumeModifyVolumeError ||
+				condition.Type == v1.PersistentVolumeClaimVolumeModifyingVolume {
+				return false
+			}
+		}
+
+		// check if claim is bound with the desired volume attributes class
+		currentClass := ptr.Deref(claim.Status.CurrentVolumeAttributesClassName, "")
+		return claim.Status.Phase == v1.ClaimBound &&
+			desiredClass == currentClass && claim.Status.ModifyVolumeStatus == nil
+	}
+
+	if match(claim) {
+		return nil
+	}
+
+	return framework.Gomega().
+		Eventually(ctx, framework.GetObject(c.CoreV1().PersistentVolumeClaims(claim.Namespace).Get, claim.Name, metav1.GetOptions{})).
+		WithTimeout(timeout).
+		Should(framework.MakeMatcher(func(claim *v1.PersistentVolumeClaim) (func() string, error) {
+			if match(claim) {
+				return nil, nil
+			}
+
+			return func() string {
+				return fmt.Sprintf("expected claim's status to be modified with the given VolumeAttirbutesClass %s, got instead:\n%s", desiredClass, format.Object(claim, 1))
+			}, nil
+		}))
+}

test/e2e/storage/testsuites/volume_modify.go

@@ -33,6 +33,7 @@ import (
 	e2efeature "k8s.io/kubernetes/test/e2e/feature"
 	"k8s.io/kubernetes/test/e2e/framework"
 	e2epod "k8s.io/kubernetes/test/e2e/framework/pod"
+	e2epv "k8s.io/kubernetes/test/e2e/framework/pv"
 	e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper"
 	e2evolume "k8s.io/kubernetes/test/e2e/framework/volume"
 	storageframework "k8s.io/kubernetes/test/e2e/storage/framework"
@@ -164,10 +165,9 @@ func (v *volumeModifyTestSuite) DefineTests(driver storageframework.TestDriver,
 		ginkgo.DeferCleanup(e2epod.DeletePodWithWait, f.ClientSet, pod)
 		framework.ExpectNoError(err, "While creating test pod with VAC")
 
-		createdPVC, err := f.ClientSet.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Get(ctx, l.resource.Pvc.Name, metav1.GetOptions{})
+		ginkgo.By("Checking PVC status")
-		framework.ExpectNoError(err, "While getting created PVC")
+		err = e2epv.WaitForPersistentVolumeClaimModified(ctx, f.ClientSet, l.resource.Pvc, modifyVolumeWaitPeriod)
-		// Check VAC matches on created PVC, but not current VAC in status
+		framework.ExpectNoError(err, "While waiting for PVC to have expected VAC")
-		gomega.Expect(vacMatches(createdPVC, l.vac.Name, false)).To(gomega.BeTrueBecause("Created PVC should match expected VAC"))
 	})
 
 	ginkgo.It("should modify volume with no VAC", func(ctx context.Context) {
@@ -191,11 +191,9 @@ func (v *volumeModifyTestSuite) DefineTests(driver storageframework.TestDriver,
 		l.resource.Pvc = SetPVCVACName(ctx, l.resource.Pvc, l.vac.Name, f.ClientSet, setVACWaitPeriod)
 		gomega.Expect(l.resource.Pvc).NotTo(gomega.BeNil())
 
-		ginkgo.By("Waiting for modification to finish")
+		ginkgo.By("Checking PVC status")
-		l.resource.Pvc = WaitForVolumeModification(ctx, l.resource.Pvc, f.ClientSet, modifyVolumeWaitPeriod)
+		err = e2epv.WaitForPersistentVolumeClaimModified(ctx, f.ClientSet, l.resource.Pvc, modifyVolumeWaitPeriod)
-
+		framework.ExpectNoError(err, "While waiting for PVC to have expected VAC")
-		pvcConditions := l.resource.Pvc.Status.Conditions
-		gomega.Expect(pvcConditions).To(gomega.BeEmpty(), "PVC should not have conditions")
 	})
 
 	ginkgo.It("should modify volume that already has a VAC", func(ctx context.Context) {
@@ -225,11 +223,9 @@ func (v *volumeModifyTestSuite) DefineTests(driver storageframework.TestDriver,
 		l.resource.Pvc = SetPVCVACName(ctx, l.resource.Pvc, newVAC.Name, f.ClientSet, setVACWaitPeriod)
 		gomega.Expect(l.resource.Pvc).NotTo(gomega.BeNil())
 
-		ginkgo.By("Waiting for modification to finish")
+		ginkgo.By("Checking PVC status")
-		l.resource.Pvc = WaitForVolumeModification(ctx, l.resource.Pvc, f.ClientSet, modifyVolumeWaitPeriod)
+		err = e2epv.WaitForPersistentVolumeClaimModified(ctx, f.ClientSet, l.resource.Pvc, modifyVolumeWaitPeriod)
-
+		framework.ExpectNoError(err, "While waiting for PVC to have expected VAC")
-		pvcConditions := l.resource.Pvc.Status.Conditions
-		gomega.Expect(pvcConditions).To(gomega.BeEmpty(), "PVC should not have conditions")
 	})
 }
 
@@ -250,45 +246,8 @@ func SetPVCVACName(ctx context.Context, origPVC *v1.PersistentVolumeClaim, name
 	return patchedPVC
 }
 
-// WaitForVolumeModification waits for the volume to be modified
-// The input PVC is assumed to have a VolumeAttributesClassName set
-func WaitForVolumeModification(ctx context.Context, pvc *v1.PersistentVolumeClaim, c clientset.Interface, timeout time.Duration) *v1.PersistentVolumeClaim {
-	var newPVC *v1.PersistentVolumeClaim
-	pvName := pvc.Spec.VolumeName
-	gomega.Eventually(ctx, func(g gomega.Gomega) {
-		pv, err := c.CoreV1().PersistentVolumes().Get(ctx, pvName, metav1.GetOptions{})
-		framework.ExpectNoError(err, "While getting existing PV")
-		g.Expect(pv.Spec.VolumeAttributesClassName).NotTo(gomega.BeNil())
-		newPVC, err = c.CoreV1().PersistentVolumeClaims(pvc.Namespace).Get(ctx, pvc.Name, metav1.GetOptions{})
-		framework.ExpectNoError(err, "While getting new PVC")
-		g.Expect(vacMatches(newPVC, *pv.Spec.VolumeAttributesClassName, true)).To(gomega.BeTrueBecause("Modified PVC should match expected VAC"))
-	}, timeout, modifyPollInterval).Should(gomega.Succeed())
-	return newPVC
-}
-
 func CleanupVAC(ctx context.Context, vac *storagev1beta1.VolumeAttributesClass, c clientset.Interface, timeout time.Duration) {
 	gomega.Eventually(ctx, func() error {
 		return c.StorageV1beta1().VolumeAttributesClasses().Delete(ctx, vac.Name, metav1.DeleteOptions{})
 	}, timeout, modifyPollInterval).Should(gomega.BeNil())
 }
-
-func vacMatches(pvc *v1.PersistentVolumeClaim, expectedVac string, checkStatusCurrentVac bool) bool {
-	// Check the following to ensure the VAC matches and that all pending modifications are complete:
-	// 1. VAC Name matches Expected
-	// 2. PVC Modify Volume status is either nil or has an empty status string
-	// 3. PVC Status Current VAC Matches Expected (only if checkStatusCurrentVac is true)
-	// (3) is only expected to be true after a VAC is modified, but not when a VAC is used to create a volume
-	if pvc.Spec.VolumeAttributesClassName == nil || *pvc.Spec.VolumeAttributesClassName != expectedVac {
-		return false
-	}
-	if pvc.Status.ModifyVolumeStatus != nil && (pvc.Status.ModifyVolumeStatus.Status != "" || pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName != expectedVac) {
-		return false
-	}
-	if checkStatusCurrentVac {
-		if pvc.Status.CurrentVolumeAttributesClassName == nil || *pvc.Status.CurrentVolumeAttributesClassName != expectedVac {
-			return false
-		}
-	}
-
-	return true
-}