cinder-csi-plugin: Tweak behavior of --with-topology flag

This is currently backwards from what you'd expect, 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, and correct the latter so that we
never set topology requirements on the CSI Volume.

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

Author: stephenfin

pkg/csi/cinder/controllerserver.go

@@ -93,12 +93,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
@@ -119,7 +117,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
 	}
 
@@ -246,7 +244,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
 }
@@ -1038,18 +1036,20 @@ 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 ignoreVolumeAZ {
-		if topologyReq != nil {
-			accessibleTopology = topologyReq.GetPreferred()
-		}
-	} else {
-		// NOTE(stephenfin): We retrieve the AZ from the created volume rather than
-		// using the value we provided in our request since these can differ due to
-		// Cinder's '[DEFAULT] allow_availability_zone_fallback' option.
-		accessibleTopology = []*csi.Topology{
-			{Segments: map[string]string{topologyKey: vol.AvailabilityZone}},
+	if withTopology {
+		if ignoreVolumeAZ {
+			if topologyReq != nil {
+				accessibleTopology = topologyReq.GetPreferred()
+			}
+		} else {
+			// NOTE(stephenfin): We retrieve the AZ from the created volume rather than
+			// using the value we provided in our request since these can differ due to
+			// Cinder's '[DEFAULT] allow_availability_zone_fallback' option.
+			accessibleTopology = []*csi.Topology{
+				{Segments: map[string]string{topologyKey: vol.AvailabilityZone}},
+			}
 		}
 	}
 	return accessibleTopology

pkg/csi/cinder/controllerserver_test.go

@@ -213,27 +213,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 {
@@ -272,12 +266,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.NotNil(actualRes.Volume.AccessibleTopology)
-				assert.Equal(tt.expectedCSITopology, actualRes.Volume.AccessibleTopology[0].GetSegments()[topologyKey])
-			}
+			// This should always be nil if --with-topology=false
+			assert.Nil(actualRes.Volume.AccessibleTopology)
 		})
 	}
 }