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

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 accessibleTopologyReq != nil && cs.Driver.withTopology {
+		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)
 		})
 	}
 }