e2e/storage: improve PV checking

TestDynamicProvisioning had multiple ways of choosing additional
checks:
- the PvCheck callback
- the builtin write/read check controlled by a boolean
- the snapshot testing

Complicating matters further, that builtin write/read test had been
more customizable with new fields `NodeSelector` and
`ExpectUnschedulable` which were only set by one particular test (see
kubernetes#70941).

That is confusing and will only get more confusing when adding more
checks in the future. Therefore the write/read check is now a separate
function that must be enabled explicitly by tests that want to run it.
The snapshot checking is also defined only for the snapshot test.

The test that expects unschedulable pods now also checks for that
particular situation itself. Instead of testing it with two pods (the
behavior from the write/read check) that both fail to start, only a
single unschedulable pod is created.

Because node name, node selector and the `ExpectUnschedulable` were
only used for checking, it is possible to simplify `StorageClassTest`
by removing all of these fields.

Expect(err).NotTo(HaveOccurred()) is an anti-pattern in Ginkgo testing
because a test failure doesn't explain what failed (see
kubernetes#34059). We avoid it
now by making the check function itself responsible for checking
errors and including more information in those checks.

Author: pohly

test/e2e/storage/csi_volumes.go

@@ -261,9 +261,11 @@ var _ = utils.SIGDescribe("CSI Volumes", func() {
 					Parameters:   sc.Parameters,
 					ClaimSize:    "1Gi",
 					ExpectedSize: "1Gi",
-					NodeName:     nodeName,
 				}
-				class, claim, pod := startPausePod(cs, scTest, ns.Name)
+				nodeSelection := testsuites.NodeSelection{
+					Name: nodeName,
+				}
+				class, claim, pod := startPausePod(cs, scTest, nodeSelection, ns.Name)
 				if class != nil {
 					defer cs.StorageV1().StorageClasses().Delete(class.Name, nil)
 				}
@@ -381,16 +383,16 @@ var _ = utils.SIGDescribe("CSI Volumes", func() {
 					Parameters:   sc.Parameters,
 					ClaimSize:    "1Gi",
 					ExpectedSize: "1Gi",
-					// The mock driver only works when everything runs on a single node.
-					NodeName: nodeName,
 					// Provisioner and storage class name must match what's used in
 					// csi-storageclass.yaml, plus the test-specific suffix.
 					Provisioner:      sc.Provisioner,
 					StorageClassName: "csi-mock-sc-" + f.UniqueName,
-					// Mock driver does not provide any persistency.
-					SkipWriteReadCheck: true,
 				}
-				class, claim, pod := startPausePod(cs, scTest, ns.Name)
+				nodeSelection := testsuites.NodeSelection{
+					// The mock driver only works when everything runs on a single node.
+					Name: nodeName,
+				}
+				class, claim, pod := startPausePod(cs, scTest, nodeSelection, ns.Name)
 				if class != nil {
 					defer cs.StorageV1().StorageClasses().Delete(class.Name, nil)
 				}
@@ -429,7 +431,7 @@ func testTopologyPositive(cs clientset.Interface, suffix, namespace string, dela
 	claim.Spec.StorageClassName = &class.Name
 
 	if delayBinding {
-		_, node := testsuites.TestBindingWaitForFirstConsumer(test, cs, claim, class)
+		_, node := testsuites.TestBindingWaitForFirstConsumer(test, cs, claim, class, nil /* node selector */, false /* expect unschedulable */)
 		Expect(node).ToNot(BeNil(), "Unexpected nil node found")
 	} else {
 		testsuites.TestDynamicProvisioning(test, cs, claim, class)
@@ -450,16 +452,22 @@ func testTopologyNegative(cs clientset.Interface, suffix, namespace string, dela
 
 	test := createGCEPDStorageClassTest()
 	test.DelayBinding = delayBinding
-	test.NodeSelector = map[string]string{v1.LabelZoneFailureDomain: podZone}
-	test.ExpectUnschedulable = true
+	nodeSelector := map[string]string{v1.LabelZoneFailureDomain: podZone}
 
 	class := newStorageClass(test, namespace, suffix)
 	addSingleCSIZoneAllowedTopologyToStorageClass(cs, class, pvZone)
 	claim := newClaim(test, namespace, suffix)
 	claim.Spec.StorageClassName = &class.Name
 	if delayBinding {
-		testsuites.TestBindingWaitForFirstConsumer(test, cs, claim, class)
+		testsuites.TestBindingWaitForFirstConsumer(test, cs, claim, class, nodeSelector, true /* expect unschedulable */)
 	} else {
+		test.PvCheck = func(claim *v1.PersistentVolumeClaim, volume *v1.PersistentVolume) {
+			// Ensure that a pod cannot be scheduled in an unsuitable zone.
+			pod := testsuites.StartInPodWithVolume(cs, namespace, claim.Name, "pvc-tester-unschedulable", "sleep 100000",
+				testsuites.NodeSelection{Selector: nodeSelector})
+			defer testsuites.StopPod(cs, pod)
+			framework.ExpectNoError(framework.WaitForPodNameUnschedulableInNamespace(cs, pod.Name, pod.Namespace), "pod should be unschedulable")
+		}
 		testsuites.TestDynamicProvisioning(test, cs, claim, class)
 	}
 }
@@ -500,7 +508,7 @@ func getVolumeHandle(cs clientset.Interface, claim *v1.PersistentVolumeClaim) st
 	return pv.Spec.CSI.VolumeHandle
 }
 
-func startPausePod(cs clientset.Interface, t testsuites.StorageClassTest, ns string) (*storagev1.StorageClass, *v1.PersistentVolumeClaim, *v1.Pod) {
+func startPausePod(cs clientset.Interface, t testsuites.StorageClassTest, node testsuites.NodeSelection, ns string) (*storagev1.StorageClass, *v1.PersistentVolumeClaim, *v1.Pod) {
 	class := newStorageClass(t, ns, "")
 	class, err := cs.StorageV1().StorageClasses().Create(class)
 	framework.ExpectNoError(err, "Failed to create class : %v", err)
@@ -514,6 +522,9 @@ func startPausePod(cs clientset.Interface, t testsuites.StorageClassTest, ns str
 			GenerateName: "pvc-volume-tester-",
 		},
 		Spec: v1.PodSpec{
+			NodeName:     node.Name,
+			NodeSelector: node.Selector,
+			Affinity:     node.Affinity,
 			Containers: []v1.Container{
 				{
 					Name:  "volume-tester",
@@ -541,9 +552,6 @@ func startPausePod(cs clientset.Interface, t testsuites.StorageClassTest, ns str
 		},
 	}
 
-	if len(t.NodeName) != 0 {
-		pod.Spec.NodeName = t.NodeName
-	}
 	pod, err = cs.CoreV1().Pods(ns).Create(pod)
 	framework.ExpectNoError(err, "Failed to create pod: %v", err)
 	return class, claim, pod

test/e2e/storage/regional_pd.go

@@ -108,12 +108,14 @@ func testVolumeProvisioning(c clientset.Interface, ns string) {
 			},
 			ClaimSize:    "1.5Gi",
 			ExpectedSize: "2Gi",
-			PvCheck: func(volume *v1.PersistentVolume) error {
-				err := checkGCEPD(volume, "pd-standard")
-				if err != nil {
-					return err
-				}
-				return verifyZonesInPV(volume, sets.NewString(cloudZones...), true /* match */)
+			PvCheck: func(claim *v1.PersistentVolumeClaim, volume *v1.PersistentVolume) {
+				var err error
+				err = checkGCEPD(volume, "pd-standard")
+				Expect(err).NotTo(HaveOccurred(), "checkGCEPD")
+				err = verifyZonesInPV(volume, sets.NewString(cloudZones...), true /* match */)
+				Expect(err).NotTo(HaveOccurred(), "verifyZonesInPV")
+
+				testsuites.PVWriteReadCheck(c, claim, volume, testsuites.NodeSelection{})
 			},
 		},
 		{
@@ -126,16 +128,16 @@ func testVolumeProvisioning(c clientset.Interface, ns string) {
 			},
 			ClaimSize:    "1.5Gi",
 			ExpectedSize: "2Gi",
-			PvCheck: func(volume *v1.PersistentVolume) error {
-				err := checkGCEPD(volume, "pd-standard")
-				if err != nil {
-					return err
-				}
+			PvCheck: func(claim *v1.PersistentVolumeClaim, volume *v1.PersistentVolume) {
+				var err error
+				err = checkGCEPD(volume, "pd-standard")
+				Expect(err).NotTo(HaveOccurred(), "checkGCEPD")
 				zones, err := framework.GetClusterZones(c)
-				if err != nil {
-					return err
-				}
-				return verifyZonesInPV(volume, zones, false /* match */)
+				Expect(err).NotTo(HaveOccurred(), "GetClusterZones")
+				err = verifyZonesInPV(volume, zones, false /* match */)
+				Expect(err).NotTo(HaveOccurred(), "verifyZonesInPV")
+
+				testsuites.PVWriteReadCheck(c, claim, volume, testsuites.NodeSelection{})
 			},
 		},
 	}
@@ -317,7 +319,7 @@ func testRegionalDelayedBinding(c clientset.Interface, ns string, pvcCount int)
 		claim.Spec.StorageClassName = &class.Name
 		claims = append(claims, claim)
 	}
-	pvs, node := testsuites.TestBindingWaitForFirstConsumerMultiPVC(test, c, claims, class)
+	pvs, node := testsuites.TestBindingWaitForFirstConsumerMultiPVC(test, c, claims, class, nil /* node selector */, false /* expect unschedulable */)
 	if node == nil {
 		framework.Failf("unexpected nil node found")
 	}
@@ -374,7 +376,7 @@ func testRegionalAllowedTopologiesWithDelayedBinding(c clientset.Interface, ns s
 		claim.Spec.StorageClassName = &class.Name
 		claims = append(claims, claim)
 	}
-	pvs, node := testsuites.TestBindingWaitForFirstConsumerMultiPVC(test, c, claims, class)
+	pvs, node := testsuites.TestBindingWaitForFirstConsumerMultiPVC(test, c, claims, class, nil /* node selector */, false /* expect unschedulable */)
 	if node == nil {
 		framework.Failf("unexpected nil node found")
 	}