Repository: kubernetes-csi/external-provisioner (go)

Issue:
Remove topology 1.13 fallback logic once 1.13 is unsupported as a node version
We can do this in 1.16.
/assign

Pull request:
Remove topology fallback logic for nodes < 1.14
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
/kind cleanup

**What this PR does / why we need it**:
We now require the API server to be on at least 1.17, which means we can remove node support for < 1.15.

Another major change in behavior made here is that if the topology feature gate is enabled, then we assume that the driver on the nodes are correctly reporting topology. We no longer fallback to "no topology" behavior. This means that if you are a driver that is going to upgrade to support topology, then it's a two step process. First, upgrade the driver on the nodes to start reporting topology, then flip the feature gate on the controller.

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #257

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
The topology feature is GA and requires K8s api server >= 1.17 and K8s nodes >= 1.15
```


Patch:
diff --git a/cmd/csi-provisioner/csi-provisioner.go b/cmd/csi-provisioner/csi-provisioner.go
index 5ae3b713f..f6f39602b 100644
--- a/cmd/csi-provisioner/csi-provisioner.go
+++ b/cmd/csi-provisioner/csi-provisioner.go
@@ -74,9 +74,6 @@ var (
 
 	defaultFSType = flag.String("default-fstype", "", "The default filesystem type of the volume to provision when fstype is unspecified in the StorageClass. If the default is not set and fstype is unset in the StorageClass, then no fstype will be set")
 
-	kubeAPIQPS   = flag.Float32("kube-api-qps", 5, "QPS to use while communicating with the kubernetes apiserver. Defaults to 5.0.")
-	kubeAPIBurst = flag.Int("kube-api-burst", 10, "Burst to use while communicating with the kubernetes apiserver. Defaults to 10.")
-
 	featureGates        map[string]bool
 	provisionController *controller.ProvisionController
 	version             = "unknown"
@@ -127,15 +124,10 @@ func main() {
 	if err != nil {
 		klog.Fatalf("Failed to create config: %v", err)
 	}
-
-	config.QPS = *kubeAPIQPS
-	config.Burst = *kubeAPIBurst
-
 	clientset, err := kubernetes.NewForConfig(config)
 	if err != nil {
 		klog.Fatalf("Failed to create client: %v", err)
 	}
-
 	// snapclientset.NewForConfig creates a new Clientset for VolumesnapshotV1beta1Client
 	snapClient, err := snapclientset.NewForConfig(config)
 	if err != nil {
@@ -290,13 +282,7 @@ func main() {
 		// to preserve backwards compatibility
 		lockName := strings.Replace(provisionerName, "/", "-", -1)
 
-		// create a new clientset for leader election
-		leClientset, err := kubernetes.NewForConfig(config)
-		if err != nil {
-			klog.Fatalf("Failed to create leaderelection client: %v", err)
-		}
-
-		le := leaderelection.NewLeaderElection(leClientset, lockName, run)
+		le := leaderelection.NewLeaderElection(clientset, lockName, run)
 
 		if *leaderElectionNamespace != "" {
 			le.WithNamespace(*leaderElectionNamespace)
diff --git a/pkg/controller/topology.go b/pkg/controller/topology.go
index a415873a2..b4c8bb4d7 100644
--- a/pkg/controller/topology.go
+++ b/pkg/controller/topology.go
@@ -31,7 +31,6 @@ import (
 	storagev1 "k8s.io/api/storage/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/util/version"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/kubernetes"
 	corelisters "k8s.io/client-go/listers/core/v1"
@@ -39,8 +38,6 @@ import (
 	"k8s.io/klog"
 )
 
-var k8sTopologyBetaVersion = version.MustParseSemantic("1.14.0")
-
 // topologyTerm represents a single term where its topology key value pairs are AND'd together.
 type topologyTerm map[string]string
 
@@ -84,67 +81,36 @@ func SupportsTopology(pluginCapabilities rpc.PluginCapabilitySet) bool {
 // GenerateAccessibilityRequirements returns the CSI TopologyRequirement
 // to pass into the CSI CreateVolume request.
 //
-// This function is only called if the topology feature is enabled
+// This function is called if the topology feature is enabled
 // in the external-provisioner and the CSI driver implements the
 // CSI accessibility capability. It is disabled by default.
 //
 // If enabled, we require that the K8s API server is on at least
-// K8s 1.14, and that the K8s CSINode feature gate is enabled. In
-// order for the topology feature to fully function correctly, the
-// K8s Nodes also have to be upgraded to 1.14 in order for kubelet
-// to register and create the new CSINode beta objects.
+// K8s 1.17 and that the K8s Nodes are on at least K8s 1.15 in
+// accordance with the 2 version skew between control plane and
+// nodes.
 //
-// However, due to K8s master/node 2 version skew support, it's
-// possible that the Nodes have not been upgraded yet, so this
-// feature needs to handle that skew. There are two main cases
-// to consider:
+// There are two main cases to consider:
 //
 // 1) selectedNode is not set (immediate binding):
 //
 //    In this case, we list all CSINode objects to find a Node that
-//    the driver has registered topology keys with. The List() API
-//    call should succeed because we at least require the K8s API
-//    server to be on 1.14. If it doesn't succeed, then we error.
+//    the driver has registered topology keys with.
 //
 //    Once we get the list of CSINode objects, we find one that has
 //    topology keys registered. If none are found, then we assume
-//    that either no Nodes have been upgraded and/or the driver has
-//    not registered any topology keys. In that case, we fallback to
-//    "topology disabled" behavior.
+//    that the driver has not started on any node yet, and we error
+//    and retry.
 //
 //    If at least one CSINode object is found with topology keys,
 //    then we continue and use that for assembling the topology
-//    requirement. While Nodes are being upgraded, the available
-//    topologies will be limited to the upgraded Nodes.
+//    requirement. The available topologies will be limited to the
+//    Nodes that the driver has registered with.
 //
 // 2) selectedNode is set (delayed binding):
 //
-//    This case never worked well in a multi-topology environment
-//    without the topology feature, because we would not pass in
-//    any topology to the driver (including any AllowedTopologies),
-//    and the driver would choose a random topology to provision in,
-//    often resulting in Pod scheduling failures.
-//
-//    So for the multi-topology environment, topology is considered
-//    to be a new feature, and requires both K8s master and nodes to
-//    be upgraded to 1.14 to fully function.
-//
-//    However, it was still possible for a K8s user to use delayed
-//    binding if the driver was deployed in a single-topology
-//    environment (such as a single zone), where topology could be
-//    effectively ignored. That use case still needs to be supported
-//    with this topology feature enabled.
-//
-//    With the feature enabled, now we will try to get the CSINode
-//    object for the selectedNode.
-//
-//    If it doesn't exist, then we need to check the Node version.
-//    If the Node is on a pre-1.14 version, then we fallback to
-//    "topology disabled" behavior.
-//
-//    Otherwise, the node is on 1.14+ and we error because either:
-//      * the driver hasn't registered a topology key yet, or
-//      * there's some temporary API server issue.
+//    We will get the topology from the CSINode object for the selectedNode
+//    and error if we can't (and retry).
 //
 func GenerateAccessibilityRequirements(
 	kubeClient kubernetes.Interface,
@@ -170,11 +136,6 @@ func GenerateAccessibilityRequirements(
 		if err != nil {
 			return nil, err
 		}
-		if selectedCSINode == nil {
-			// Fallback to "topology disabled" behavior
-			// This should only happen if the Node is on a pre-1.14 version
-			return nil, nil
-		}
 		topologyKeys := getTopologyKeys(selectedCSINode, driverName)
 		if len(topologyKeys) == 0 {
 			// The scheduler selected a node with no topology information.
@@ -225,16 +186,17 @@ func GenerateAccessibilityRequirements(
 			if err != nil {
 				return nil, err
 			}
+			if len(requisiteTerms) == 0 {
+				// We may reach here if the driver has not registered on any nodes.
+				// We should wait for at least one driver to start so that we can
+				// provision in a supported topology.
+				return nil, fmt.Errorf("no available topology found")
+			}
 		}
 	}
 
-	// It might be possible to reach here if:
-	//
-	// * aggregateTopologies couldn't find any topology information on nodes for immediate binding.
-	//   This could be due to nodes not upgraded to use the beta CSINode feature.
-	// * allowedTopologies had empty entries.
-	//
-	// Either way, we fallback to the "topology disabled" behavior.
+	// It might be possible to reach here if allowedTopologies had empty entries.
+	// We fallback to the "topology disabled" behavior.
 	if len(requisiteTerms) == 0 {
 		return nil, nil
 	}
@@ -282,26 +244,13 @@ func getSelectedCSINode(
 
 	selectedCSINode, err := csiNodeLister.Get(selectedNode.Name)
 	if err != nil {
-		// If the Node is before 1.14, then we fallback to "topology disabled" behavior
-		// to retain backwards compatibility in a single-topology environment with
-		// delayed binding.
-		//
-		// TODO (#257): Once K8s 1.13 is no longer supported, then this check can be removed
-		// and we can require that CSINode exists.
-		nodeVersion, parseErr := version.ParseSemantic(selectedNode.Status.NodeInfo.KubeletVersion)
-		if parseErr != nil {
-			return nil, fmt.Errorf("Failed to parse kubelet version from node %q: %v", selectedNode.Name, parseErr)
-		}
-		if nodeVersion.LessThan(k8sTopologyBetaVersion) {
-			klog.Warningf("Selected node %q version %q is less than %q, falling back to no topology", selectedNode.Name, nodeVersion, k8sTopologyBetaVersion)
-			return nil, nil
-		}
-
-		// Otherwise, require the CSINode beta feature on the K8s node to also be enabled.
 		// We don't want to fallback and provision in the wrong topology if there's some temporary
 		// error with the API server.
 		return nil, fmt.Errorf("error getting CSINode for selected node %q: %v", selectedNode.Name, err)
 	}
+	if selectedCSINode == nil {
+		return nil, fmt.Errorf("CSINode for selected node %q not found", selectedNode.Name)
+	}
 	return selectedCSINode, nil
 }
 
diff --git a/release-tools/cloudbuild.yaml b/release-tools/cloudbuild.yaml
index 8f678924e..a22c0e7ac 100644
--- a/release-tools/cloudbuild.yaml
+++ b/release-tools/cloudbuild.yaml
@@ -16,7 +16,7 @@
 # To promote release images, see https://github.com/kubernetes/k8s.io/tree/master/k8s.gcr.io/images/k8s-staging-sig-storage.
 
 # This must be specified in seconds. If omitted, defaults to 600s (10 mins).
-timeout: 1800s
+timeout: 1200s
 # This prevents errors if you don't use both _GIT_TAG and _PULL_BASE_REF,
 # or any new substitutions added in the future.
 options:
diff --git a/release-tools/filter-junit.go b/release-tools/filter-junit.go
index cf1cb4abf..2f51be00e 100644
--- a/release-tools/filter-junit.go
+++ b/release-tools/filter-junit.go
@@ -15,10 +15,10 @@ limitations under the License.
 */
 
 /*
-This command filters a JUnit file such that only tests with a name
-matching a regular expression are passed through. By concatenating
-multiple input files it is possible to merge them into a single file.
-*/
+ * This command filters a JUnit file such that only tests with a name
+ * matching a regular expression are passed through. By concatenating
+ * multiple input files it is possible to merge them into a single file.
+ */
 package main
 
 import (


Test patch:
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
index 7e8e8e2b7..09b0c8aa7 100644
--- a/pkg/controller/controller_test.go
+++ b/pkg/controller/controller_test.go
@@ -2821,7 +2821,7 @@ func TestProvisionWithTopologyEnabled(t *testing.T) {
 				controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(createVolumeOut, nil).Times(1)
 			}
 
-			nodes := buildNodes(tc.nodeLabels, k8sTopologyBetaVersion.String())
+			nodes := buildNodes(tc.nodeLabels)
 			csiNodes := buildCSINodes(tc.topologyKeys)
 
 			var (
@@ -2928,7 +2928,7 @@ func TestProvisionErrorHandling(t *testing.T) {
 					errOut := status.Error(code, "fake error")
 					controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(nil, errOut).Times(1)
 
-					nodes := buildNodes(nodeLabels, k8sTopologyBetaVersion.String())
+					nodes := buildNodes(nodeLabels)
 					csiNodes := buildCSINodes(topologyKeys)
 					clientSet := fakeclientset.NewSimpleClientset(nodes, csiNodes)
 					scLister, csiNodeLister, nodeLister, claimLister, vaLister, stopChan := listers(clientSet)
diff --git a/pkg/controller/topology_test.go b/pkg/controller/topology_test.go
index 7fd430294..f8abe1535 100644
--- a/pkg/controller/topology_test.go
+++ b/pkg/controller/topology_test.go
@@ -33,8 +33,7 @@ import (
 )
 
 const (
-	testDriverName     = "com.example.csi/test-driver"
-	preBetaNodeVersion = "1.13.0"
+	testDriverName = "com.example.csi/test-driver"
 )
 
 func TestGenerateVolumeNodeAffinity(t *testing.T) {
@@ -144,11 +143,12 @@ func TestGenerateVolumeNodeAffinity(t *testing.T) {
 	}
 
 	for name, tc := range testcases {
-		t.Logf("test: %s", name)
-		nodeAffinity := GenerateVolumeNodeAffinity(tc.accessibleTopology)
-		if !volumeNodeAffinitiesEqual(nodeAffinity, tc.expectedNodeAffinity) {
-			t.Errorf("expected node affinity %v; got: %v", tc.expectedNodeAffinity, nodeAffinity)
-		}
+		t.Run(name, func(t *testing.T) {
+			nodeAffinity := GenerateVolumeNodeAffinity(tc.accessibleTopology)
+			if !volumeNodeAffinitiesEqual(nodeAffinity, tc.expectedNodeAffinity) {
+				t.Errorf("expected node affinity %v; got: %v", tc.expectedNodeAffinity, nodeAffinity)
+			}
+		})
 	}
 }
 
@@ -388,7 +388,7 @@ func TestStatefulSetSpreading(t *testing.T) {
 		},
 	}
 
-	nodes := buildNodes(nodeLabels, k8sTopologyBetaVersion.String())
+	nodes := buildNodes(nodeLabels)
 	csiNodes := buildCSINodes(topologyKeys)
 
 	kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes)
@@ -401,35 +401,32 @@ func TestStatefulSetSpreading(t *testing.T) {
 			if strictTopology {
 				name += " with strict topology"
 			}
-			t.Logf("test: %s", name)
+			t.Run(name, func(t *testing.T) {
+				requirements, err := GenerateAccessibilityRequirements(
+					kubeClient,
+					testDriverName,
+					tc.pvcName,
+					tc.allowedTopologies,
+					nil,
+					strictTopology,
+					csiNodeLister,
+					nodeLister,
+				)
 
-			requirements, err := GenerateAccessibilityRequirements(
-				kubeClient,
-				testDriverName,
-				tc.pvcName,
-				tc.allowedTopologies,
-				nil,
-				strictTopology,
-				csiNodeLister,
-				nodeLister,
-			)
-
-			if err != nil {
-				t.Errorf("unexpected error found: %v", err)
-				continue
-			}
+				if err != nil {
+					t.Fatalf("unexpected error found: %v", err)
+				}
 
-			if requirements == nil {
-				t.Errorf("expected preferred to be %v but requirements is nil", tc.expectedPreferred)
-				continue
-			}
-			if requirements.Preferred == nil {
-				t.Errorf("expected preferred to be %v but requirements.Preferred is nil", tc.expectedPreferred)
-				continue
-			}
-			if !helper.Semantic.DeepEqual(requirements.Preferred, tc.expectedPreferred) {
-				t.Errorf("expected preferred requisite %v; got: %v", tc.expectedPreferred, requirements.Preferred)
-			}
+				if requirements == nil {
+					t.Fatalf("expected preferred to be %v but requirements is nil", tc.expectedPreferred)
+				}
+				if requirements.Preferred == nil {
+					t.Fatalf("expected preferred to be %v but requirements.Preferred is nil", tc.expectedPreferred)
+				}
+				if !helper.Semantic.DeepEqual(requirements.Preferred, tc.expectedPreferred) {
+					t.Errorf("expected preferred requisite %v; got: %v", tc.expectedPreferred, requirements.Preferred)
+				}
+			})
 		}
 	}
 }
@@ -797,29 +794,28 @@ func TestAllowedTopologies(t *testing.T) {
 			if strictTopology {
 				name += " with strict topology"
 			}
-			t.Logf("test: %s", name)
-			requirements, err := GenerateAccessibilityRequirements(
-				nil,           /* kubeClient */
-				"test-driver", /* driverName */
-				"testpvc",
-				tc.allowedTopologies,
-				nil, /* selectedNode */
-				strictTopology,
-				nil,
-				nil,
-			)
+			t.Run(name, func(t *testing.T) {
+				requirements, err := GenerateAccessibilityRequirements(
+					nil,           /* kubeClient */
+					"test-driver", /* driverName */
+					"testpvc",
+					tc.allowedTopologies,
+					nil, /* selectedNode */
+					strictTopology,
+					nil,
+					nil,
+				)
 
-			if err != nil {
-				t.Errorf("expected no error but got: %v", err)
-				continue
-			}
-			if requirements == nil {
-				t.Errorf("expected requirements not to be nil")
-				continue
-			}
-			if !requisiteEqual(requirements.Requisite, tc.expectedRequisite) {
-				t.Errorf("expected requisite %v; got: %v", tc.expectedRequisite, requirements.Requisite)
-			}
+				if err != nil {
+					t.Fatalf("expected no error but got: %v", err)
+				}
+				if requirements == nil {
+					t.Fatalf("expected requirements not to be nil")
+				}
+				if !requisiteEqual(requirements.Requisite, tc.expectedRequisite) {
+					t.Errorf("expected requisite %v; got: %v", tc.expectedRequisite, requirements.Requisite)
+				}
+			})
 		}
 	}
 }
@@ -831,7 +827,6 @@ func TestTopologyAggregation(t *testing.T) {
 		nodeLabels              []map[string]string
 		topologyKeys            []map[string][]string
 		hasSelectedNode         bool // if set, the first map in nodeLabels is for the selected node.
-		preBetaNode             bool // use a node before 1.14
 		expectedRequisite       []*csi.Topology
 		expectedStrictRequisite []*csi.Topology
 		expectError             bool
@@ -959,7 +954,7 @@ func TestTopologyAggregation(t *testing.T) {
 			expectedRequisite: nil,
 			expectError:       true,
 		},
-		// Node has not been upgraded yet
+		// Driver has not been registered on any nodes
 		"random node: no CSINodes": {
 			nodeLabels: []map[string]string{
 				{"com.example.csi/zone": "zone1"},
@@ -968,6 +963,7 @@ func TestTopologyAggregation(t *testing.T) {
 			},
 			topologyKeys:      nil,
 			expectedRequisite: nil,
+			expectError:       true,
 		},
 		// Driver on node has not been updated to report topology keys
 		"random node: missing keys": {
@@ -982,6 +978,7 @@ func TestTopologyAggregation(t *testing.T) {
 				{testDriverName: nil},
 			},
 			expectedRequisite: nil,
+			expectError:       true,
 		},
 		"random node: one node has been upgraded": {
 			nodeLabels: []map[string]string{
@@ -1039,17 +1036,6 @@ func TestTopologyAggregation(t *testing.T) {
 			expectedRequisite: nil,
 			expectError:       true,
 		},
-		"selected node: no CSINode info pre-beta": {
-			hasSelectedNode: true,
-			preBetaNode:     true,
-			nodeLabels: []map[string]string{
-				{"com.example.csi/zone": "zone1"},
-				{"com.example.csi/zone": "zone2"},
-				{"com.example.csi/zone": "zone2"},
-			},
-			topologyKeys:      nil,
-			expectedRequisite: nil,
-		},
 		"selected node is missing keys": {
 			hasSelectedNode: true,
 			nodeLabels: []map[string]string{
@@ -1072,62 +1058,53 @@ func TestTopologyAggregation(t *testing.T) {
 			if strictTopology {
 				name += " with strict topology"
 			}
-			t.Logf("test: %s", name)
+			t.Run(name, func(t *testing.T) {
+				nodes := buildNodes(tc.nodeLabels)
+				csiNodes := buildCSINodes(tc.topologyKeys)
 
-			nodeVersion := k8sTopologyBetaVersion.String()
-			if tc.preBetaNode {
-				nodeVersion = preBetaNodeVersion
-			}
-			nodes := buildNodes(tc.nodeLabels, nodeVersion)
-			csiNodes := buildCSINodes(tc.topologyKeys)
+				kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes)
 
-			kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes)
+				_, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient)
+				defer close(stopChan)
 
-			_, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient)
-			defer close(stopChan)
-
-			var selectedNode *v1.Node
-			if tc.hasSelectedNode {
-				selectedNode = &nodes.Items[0]
-			}
-			requirements, err := GenerateAccessibilityRequirements(
-				kubeClient,
-				testDriverName,
-				"testpvc",
-				nil, /* allowedTopologies */
-				selectedNode,
-				strictTopology,
-				csiNodeLister,
-				nodeLister,
-			)
+				var selectedNode *v1.Node
+				if tc.hasSelectedNode {
+					selectedNode = &nodes.Items[0]
+				}
+				requirements, err := GenerateAccessibilityRequirements(
+					kubeClient,
+					testDriverName,
+					"testpvc",
+					nil, /* allowedTopologies */
+					selectedNode,
+					strictTopology,
+					csiNodeLister,
+					nodeLister,
+				)
 
-			if tc.expectError {
-				if err == nil {
-					t.Error("expected error but got none")
+				if tc.expectError && err == nil {
+					t.Fatalf("expected error but got none")
 				}
-				continue
-			}
-			if err != nil {
-				t.Errorf("expected no error but got: %v", err)
-				continue
-			}
-			expectedRequisite := tc.expectedRequisite
-			if strictTopology && tc.expectedStrictRequisite != nil {
-				expectedRequisite = tc.expectedStrictRequisite
-			}
-			if requirements == nil {
-				if expectedRequisite != nil {
-					t.Errorf("expected requisite to be %v but requirements is nil", expectedRequisite)
+				if !tc.expectError && err != nil {
+					t.Fatalf("expected no error but got: %v", err)
 				}
-				continue
-			}
-			if expectedRequisite == nil {
-				t.Errorf("expected requirements to be nil but got requisite: %v", requirements.Requisite)
-				continue
-			}
-			if !requisiteEqual(requirements.Requisite, expectedRequisite) {
-				t.Errorf("expected requisite %v; got: %v", tc.expectedRequisite, requirements.Requisite)
-			}
+				expectedRequisite := tc.expectedRequisite
+				if strictTopology && tc.expectedStrictRequisite != nil {
+					expectedRequisite = tc.expectedStrictRequisite
+				}
+				if expectedRequisite == nil {
+					if requirements != nil {
+						t.Fatalf("expected requirements to be nil but got requisite: %v", requirements.Requisite)
+					}
+				} else {
+					if requirements == nil {
+						t.Fatalf("expected requisite to be %v but requirements is nil", expectedRequisite)
+					}
+					if !requisiteEqual(requirements.Requisite, expectedRequisite) {
+						t.Errorf("expected requisite %v; got: %v", tc.expectedRequisite, requirements.Requisite)
+					}
+				}
+			})
 		}
 	}
 }
@@ -1395,60 +1372,56 @@ func TestPreferredTopologies(t *testing.T) {
 			if strictTopology {
 				name += " with strict topology"
 			}
-			t.Logf("test: %s", name)
+			t.Run(name, func(t *testing.T) {
 
-			nodes := buildNodes(tc.nodeLabels, k8sTopologyBetaVersion.String())
-			csiNodes := buildCSINodes(tc.topologyKeys)
+				nodes := buildNodes(tc.nodeLabels)
+				csiNodes := buildCSINodes(tc.topologyKeys)
 
-			kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes)
-			selectedNode := &nodes.Items[0]
+				kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes)
+				selectedNode := &nodes.Items[0]
 
-			_, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient)
-			defer close(stopChan)
+				_, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient)
+				defer close(stopChan)
 
-			requirements, err := GenerateAccessibilityRequirements(
-				kubeClient,
-				testDriverName,
-				"testpvc",
-				tc.allowedTopologies,
-				selectedNode,
-				strictTopology,
-				csiNodeLister,
-				nodeLister,
-			)
+				requirements, err := GenerateAccessibilityRequirements(
+					kubeClient,
+					testDriverName,
+					"testpvc",
+					tc.allowedTopologies,
+					selectedNode,
+					strictTopology,
+					csiNodeLister,
+					nodeLister,
+				)
 
-			if tc.expectError {
-				if err == nil {
-					t.Error("expected error but got none")
+				if tc.expectError && err == nil {
+					t.Fatalf("expected error but got none")
 				}
-				continue
-			}
-			if err != nil {
-				t.Errorf("expected no error but got: %v", err)
-				continue
-			}
-			expectedPreferred := tc.expectedPreferred
-			if strictTopology && tc.expectedStrictPreferred != nil {
-				expectedPreferred = tc.expectedStrictPreferred
-			}
-			if requirements == nil {
-				if expectedPreferred != nil {
-					t.Errorf("expected preferred to be %v but requirements is nil", expectedPreferred)
+				if !tc.expectError && err != nil {
+					t.Fatalf("expected no error but got: %v", err)
 				}
-				continue
-			}
-			if expectedPreferred == nil {
-				t.Errorf("expected requirements to be nil but got preferred: %v", requirements.Preferred)
-				continue
-			}
-			if !helper.Semantic.DeepEqual(requirements.Preferred, expectedPreferred) {
-				t.Errorf("expected requisite %v; got: %v", tc.expectedPreferred, requirements.Preferred)
-			}
+				expectedPreferred := tc.expectedPreferred
+				if strictTopology && tc.expectedStrictPreferred != nil {
+					expectedPreferred = tc.expectedStrictPreferred
+				}
+				if expectedPreferred == nil {
+					if requirements != nil {
+						t.Fatalf("expected requirements to be nil but got preferred: %v", requirements.Preferred)
+					}
+				} else {
+					if requirements == nil {
+						t.Fatalf("expected preferred to be %v but requirements is nil", expectedPreferred)
+					}
+					if !helper.Semantic.DeepEqual(requirements.Preferred, expectedPreferred) {
+						t.Errorf("expected requisite %v; got: %v", tc.expectedPreferred, requirements.Preferred)
+					}
+				}
+			})
 		}
 	}
 }
 
-func buildNodes(nodeLabels []map[string]string, nodeVersion string) *v1.NodeList {
+func buildNodes(nodeLabels []map[string]string) *v1.NodeList {
 	list := &v1.NodeList{}
 	i := 0
 	for _, l := range nodeLabels {
@@ -1457,11 +1430,6 @@ func buildNodes(nodeLabels []map[string]string, nodeVersion string) *v1.NodeList
 				Name:   fmt.Sprintf("node-%d", i),
 				Labels: l,
 			},
-			Status: v1.NodeStatus{
-				NodeInfo: v1.NodeSystemInfo{
-					KubeletVersion: nodeVersion,
-				},
-			},
 		}
 		node.Labels["net.example.storage/rack"] = "rack1"
 		list.Items = append(list.Items, node)