cinder-csi-plugin: Tweak behavior of --with-topology flag (#2998)

This is currently backwards from what you'd expect when the value is
false, with the topology information always being ignored for the Cinder
requests but never ignored for the k8s CSI requests. Modify the former
so that we respect the 'availability' parameter when set - regardless of
the value of --with-topology - and ensure that we never set topology
requirements on the CSI Volume when --with-topology=false.

Signed-off-by: Stephen Finucane <[email protected]>

Author: stephenfin

pkg/csi/cinder/controllerserver.go

@@ -94,12 +94,10 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 	// First check if volAvailability is already specified, if not get preferred from Topology
 	// Required, in case vol AZ is different from node AZ
 	var volAvailability string
-	if cs.Driver.withTopology {
-		if volParams["availability"] != "" {
-			volAvailability = volParams["availability"]
-		} else if accessibleTopologyReq != nil {
-			volAvailability = sharedcsi.GetAZFromTopology(topologyKey, accessibleTopologyReq)
-		}
+	if volParams["availability"] != "" {
+		volAvailability = volParams["availability"]
+	} else if cs.Driver.withTopology && accessibleTopologyReq != nil {
+		volAvailability = sharedcsi.GetAZFromTopology(topologyKey, accessibleTopologyReq)
 	}
 
 	// get the PVC annotation
@@ -120,7 +118,7 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 			return nil, status.Error(codes.AlreadyExists, "Volume Already exists with same name and different capacity")
 		}
 		klog.V(4).Infof("Volume %s already exists in Availability Zone: %s of size %d GiB", vols[0].ID, vols[0].AvailabilityZone, vols[0].Size)
-		accessibleTopology := getTopology(&vols[0], accessibleTopologyReq, ignoreVolumeAZ)
+		accessibleTopology := getTopology(&vols[0], accessibleTopologyReq, cs.Driver.withTopology, ignoreVolumeAZ)
 		return getCreateVolumeResponse(&vols[0], nil, accessibleTopology), nil
 	}
 
@@ -250,7 +248,7 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 
 	klog.V(4).Infof("CreateVolume: Successfully created volume %s in Availability Zone: %s of size %d GiB", vol.ID, vol.AvailabilityZone, vol.Size)
 
-	accessibleTopology := getTopology(vol, accessibleTopologyReq, ignoreVolumeAZ)
+	accessibleTopology := getTopology(vol, accessibleTopologyReq, cs.Driver.withTopology, ignoreVolumeAZ)
 
 	return getCreateVolumeResponse(vol, volCtx, accessibleTopology), nil
 }
@@ -1040,8 +1038,12 @@ func (cs *controllerServer) ControllerExpandVolume(ctx context.Context, req *csi
 	}, nil
 }
 
-func getTopology(vol *volumes.Volume, topologyReq *csi.TopologyRequirement, ignoreVolumeAZ bool) []*csi.Topology {
+func getTopology(vol *volumes.Volume, topologyReq *csi.TopologyRequirement, withTopology bool, ignoreVolumeAZ bool) []*csi.Topology {
 	var accessibleTopology []*csi.Topology
+	if !withTopology {
+		return accessibleTopology
+	}
+
 	if ignoreVolumeAZ {
 		if topologyReq != nil {
 			accessibleTopology = topologyReq.GetPreferred()

pkg/csi/cinder/controllerserver_test.go

@@ -265,27 +265,21 @@ func TestCreateVolumeWithTopologyDisabled(t *testing.T) {
 	assert := assert.New(t)
 
 	tests := []struct {
-		name                string
-		volumeParams        map[string]string
-		expectedVolumeAZ    string
-		expectedCSITopology string
+		name             string
+		volumeParams     map[string]string
+		expectedVolumeAZ string
 	}{
 		{
 			name:             "empty paramters",
 			volumeParams:     nil,
 			expectedVolumeAZ: "",
-			// FIXME(stephenfin): This should be unset
-			expectedCSITopology: "nova",
 		},
 		{
 			name: "availability parameter",
 			volumeParams: map[string]string{
 				"availability": "cinder",
 			},
-			// FIXME(stephenfin): This should be "cinder" per the parameter
-			expectedVolumeAZ: "",
-			// ...and this should be unset
-			expectedCSITopology: "nova",
+			expectedVolumeAZ: "cinder",
 		},
 	}
 	for _, tt := range tests {
@@ -324,12 +318,8 @@ func TestCreateVolumeWithTopologyDisabled(t *testing.T) {
 			assert.NotNil(actualRes.Volume)
 			assert.NotNil(actualRes.Volume.CapacityBytes)
 			assert.NotEqual(0, len(actualRes.Volume.VolumeId), "Volume Id is nil")
-			if tt.expectedCSITopology == "" {
-				assert.Nil(actualRes.Volume.AccessibleTopology)
-			} else {
-				assert.GreaterOrEqual(len(actualRes.Volume.AccessibleTopology), 1)
-				assert.Equal(tt.expectedCSITopology, actualRes.Volume.AccessibleTopology[0].GetSegments()[topologyKey])
-			}
+			// This should always be nil if --with-topology=false
+			assert.Nil(actualRes.Volume.AccessibleTopology)
 		})
 	}
 }