require CSINode if topology enabled

pkg/controller/controller_test.go

@@ -704,6 +704,13 @@ func createFakePVCWithVolumeMode(requestBytes int64, volumeMode v1.PersistentVol
 	return claim
 }
 
+// createFakePVCWithSelectedNode returns PVC with a selectedNode
+func createFakePVCWithSelectedNode(requestBytes int64, selectedNode string) *v1.PersistentVolumeClaim {
+	claim := createFakePVC(requestBytes)
+	claim.Annotations = map[string]string{"volume.kubernetes.io/selected-node": selectedNode}
+	return claim
+}
+
 func TestGetSecretReference(t *testing.T) {
 	testcases := map[string]struct {
 		secretParams deprecatedSecretParamsMap
@@ -1880,6 +1887,56 @@ func TestProvisionWithTopology(t *testing.T) {
 	}
 }
 
+// TestProvisionWithTopologyDisabled checks that selected node is ignored and topology is not set on the PV
+func TestProvisionWithTopologyDisabled(t *testing.T) {
+	defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.Topology, false)()
+
+	accessibleTopology := []*csi.Topology{
+		{
+			Segments: map[string]string{
+				"com.example.csi/zone": "zone1",
+				"com.example.csi/rack": "rack2",
+			},
+		},
+	}
+
+	const requestBytes = 100
+
+	tmpdir := tempDir(t)
+	defer os.RemoveAll(tmpdir)
+	mockController, driver, identityServer, controllerServer, csiConn, err := createMockServer(t, tmpdir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer mockController.Finish()
+	defer driver.Stop()
+
+	clientSet := fakeclientset.NewSimpleClientset()
+	csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, nil, driverName)
+
+	out := &csi.CreateVolumeResponse{
+		Volume: &csi.Volume{
+			CapacityBytes:      requestBytes,
+			VolumeId:           "test-volume-id",
+			AccessibleTopology: accessibleTopology,
+		},
+	}
+
+	provisionWithTopologyMockServerSetupExpectations(identityServer, controllerServer)
+	controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1)
+
+	pv, err := csiProvisioner.Provision(controller.VolumeOptions{
+		PVC: createFakePVCWithSelectedNode(requestBytes, "test-node"), // dummy PVC
+	})
+	if err != nil {
+		t.Errorf("got error from Provision call: %v", err)
+	}
+
+	if pv.Spec.NodeAffinity != nil {
+		t.Errorf("expected nil PV node affinity; got: %v", pv.Spec.NodeAffinity)
+	}
+}
+
 // TestProvisionWithMountOptions is a test of provisioner integration with mount options.
 func TestProvisionWithMountOptions(t *testing.T) {
 	expectedOptions := []string{"foo=bar", "baz=qux"}

pkg/controller/topology.go

@@ -29,7 +29,6 @@ import (
 	storage "k8s.io/api/storage/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
-	"k8s.io/klog"
 )
 
 // topologyTerm represents a single term where its topology key value pairs are AND'd together.
@@ -108,7 +107,6 @@ func GenerateAccessibilityRequirements(
 	} else {
 		// selectedNode is set so use topology from that node to populate preferredTerms
 		// TODO (verult) reuse selected node info from aggregateTopologies
-		// TODO (verult) retry
 		nodeInfo, err := kubeClient.StorageV1beta1().CSINodes().Get(selectedNode.Name, metav1.GetOptions{})
 		if err != nil {
 			return nil, fmt.Errorf("error getting node info for selected node: %v", err)
@@ -141,12 +139,10 @@ func aggregateTopologies(
 
 	var topologyKeys []string
 	if selectedNode == nil {
-		// TODO (verult) retry
+		// TODO (#144): use informers
 		nodeInfos, err := kubeClient.StorageV1beta1().CSINodes().List(metav1.ListOptions{})
 		if err != nil {
-			// We must support provisioning if CSINode is missing, for backward compatibility.
-			klog.Warningf("error listing CSINodes: %v; proceeding to provision without topology information", err)
-			return nil, nil
+			return nil, fmt.Errorf("error listing CSINodes: %v", err)
 		}
 
 		rand.Shuffle(len(nodeInfos.Items), func(i, j int) {
@@ -161,12 +157,10 @@ func aggregateTopologies(
 			}
 		}
 	} else {
-		// TODO (verult) retry
+		// TODO (#144): use informers
 		selectedNodeInfo, err := kubeClient.StorageV1beta1().CSINodes().Get(selectedNode.Name, metav1.GetOptions{})
 		if err != nil {
-			// We must support provisioning if CSINode is missing, for backward compatibility.
-			klog.Warningf("error getting CSINode for selected node %q: %v; proceeding to provision without topology information", selectedNode.Name, err)
-			return nil, nil
+			return nil, fmt.Errorf("error getting CSINode for selected node %q: %v", selectedNode.Name, err)
 		}
 		topologyKeys = getTopologyKeys(selectedNodeInfo, driverName)
 	}
@@ -183,6 +177,7 @@ func aggregateTopologies(
 	if err != nil {
 		return nil, err
 	}
+	// TODO: use informers
 	nodes, err := kubeClient.CoreV1().Nodes().List(metav1.ListOptions{LabelSelector: selector})
 	if err != nil {
 		return nil, fmt.Errorf("error listing nodes: %v", err)

pkg/controller/topology_test.go

@@ -918,6 +918,7 @@ func TestTopologyAggregation(t *testing.T) {
 			nodeLabels:        []map[string]string{{}, {}, {}},
 			topologyKeys:      nil,
 			expectedRequisite: nil,
+			expectError:       true,
 		},
 		"random node: missing keys": {
 			nodeLabels: []map[string]string{{}, {}, {}},
@@ -1046,6 +1047,29 @@ func TestPreferredTopologies(t *testing.T) {
 				},
 			},
 		},
+		"allowedTopologies specified: no CSINode": {
+			allowedTopologies: []v1.TopologySelectorTerm{
+				{
+					MatchLabelExpressions: []v1.TopologySelectorLabelRequirement{
+						{
+							Key:    "com.example.csi/zone",
+							Values: []string{"zone1", "zone2"},
+						},
+						{
+							Key:    "com.example.csi/rack",
+							Values: []string{"rackA", "rackB"},
+						},
+					},
+				},
+			},
+			nodeLabels: []map[string]string{
+				{"com.example.csi/zone": "zone2", "com.example.csi/rack": "rackA"},
+				{"com.example.csi/zone": "zone1", "com.example.csi/rack": "rackA"},
+				{"com.example.csi/zone": "zone1", "com.example.csi/rack": "rackB"},
+			},
+			topologyKeys: nil,
+			expectError:  true,
+		},
 		"topology aggregation": {
 			nodeLabels: []map[string]string{
 				{"com.example.csi/zone": "zone2", "com.example.csi/rack": "rackA"},