From 7a2c53fad3b1bef5a82ba38fe51ac27508c8bc51 Mon Sep 17 00:00:00 2001 From: Michelle Au Date: Thu, 14 Mar 2019 22:25:34 -0700 Subject: [PATCH] require CSINode if topology enabled --- pkg/controller/controller_test.go | 57 +++++++++++++++++++++++++++++++ pkg/controller/topology.go | 15 +++----- pkg/controller/topology_test.go | 24 +++++++++++++ 3 files changed, 86 insertions(+), 10 deletions(-) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 72d1aa2a21..b26e393fb6 100644 --- a/pkg/controller/controller_test.go +++ b/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"} diff --git a/pkg/controller/topology.go b/pkg/controller/topology.go index 09206e34f4..c04d52fde4 100644 --- a/pkg/controller/topology.go +++ b/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) diff --git a/pkg/controller/topology_test.go b/pkg/controller/topology_test.go index 8f0d1fff8e..aff46147a8 100644 --- a/pkg/controller/topology_test.go +++ b/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"},