[cinder-csi-plugin] Rationalise all things topology (#2865)

* cinder-csi-plugin: Don't set segment with empty topology value

This is actually handled by csi-provisioner, but it's arguably
incorrect.

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

* tests: Don't use global mocks

Calling '.On' with the same expected arguments multiple times will only
result in a single mock: the first one. We should avoid doing this.

We also remove some useless comments that simply duplicate what the
calls already say.

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

* cinder-csi-plugin: Add tests for ignore-volume-az

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

* cinder-csi-plugin: Add tests for --with-topology=false

This highlights an issue (IMO) is how we handle parameter generation.

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

* cinder-csi-plugin: Group topology generation

Group everything together rather than having it spread out as it is
currently. A decoder of the logic, to ensure nothing has changed:

1. Determine what the OpenStack Cinder volume's AZ will be:

  a. If the `availability` parameter is set on the Storage Class, the
     cinder volume will use this as the AZ.
  b. Otherwise, the cinder volume will use: an AZ extracted from one of
     the preferred CSI Topologies; an AZ extracted from one of the
     requisite (available) Topologies; or no AZ.

2. Determine what the Kubernetes CSI Volume's topology will be:

  a. If the `ignore-volume-az` option is set, the CSI Volume will use
     the preferred CSI Volume Topologies, if any, else None. [*]
  b. Otherwise, the CSI Volume will generate a Volume Topology from the
     Cinder Volume's AZ.

[*] This practically means that `ignore-volume-az` is only useful when
    the availability parameter is set on the Storage Class. This in turn
    means the CSI Volume Topology is junk as it has no bearing on the
    "real" topology constraint on the volume.

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

---------

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

Author: stephenfin

pkg/csi/cinder/controllerserver.go

@@ -86,22 +86,22 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 	// Volume Type
 	volType := volParams["type"]
 
+	// Volume AZ
+
+	accessibleTopologyReq := req.GetAccessibilityRequirements()
+	ignoreVolumeAZ := cloud.GetBlockStorageOpts().IgnoreVolumeAZ
+
+	// 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 {
-		// First check if volAvailability is already specified, if not get preferred from Topology
-		// Required, incase vol AZ is different from node AZ
-		volAvailability = volParams["availability"]
-		if volAvailability == "" {
-			accessibleTopologyReq := req.GetAccessibilityRequirements()
-			// Check from Topology
-			if accessibleTopologyReq != nil {
-				volAvailability = sharedcsi.GetAZFromTopology(topologyKey, accessibleTopologyReq)
-			}
+		if volParams["availability"] != "" {
+			volAvailability = volParams["availability"]
+		} else if accessibleTopologyReq != nil {
+			volAvailability = sharedcsi.GetAZFromTopology(topologyKey, accessibleTopologyReq)
 		}
 	}
 
-	ignoreVolumeAZ := cloud.GetBlockStorageOpts().IgnoreVolumeAZ
-
 	// get the PVC annotation
 	pvcAnnotations := sharedcsi.GetPVCAnnotations(cs.Driver.pvcLister, volParams)
 	for k, v := range pvcAnnotations {
@@ -120,8 +120,11 @@ 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)
-		return getCreateVolumeResponse(&vols[0], nil, ignoreVolumeAZ, req.GetAccessibilityRequirements()), nil
-	} else if len(vols) > 1 {
+		accessibleTopology := getTopology(&vols[0], accessibleTopologyReq, ignoreVolumeAZ)
+		return getCreateVolumeResponse(&vols[0], nil, accessibleTopology), nil
+	}
+
+	if len(vols) > 1 {
 		klog.V(3).Infof("found multiple existing volumes with selected name (%s) during create", volName)
 		return nil, status.Error(codes.Internal, "Multiple volumes reported by Cinder with same name")
 	}
@@ -247,7 +250,9 @@ 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)
 
-	return getCreateVolumeResponse(vol, volCtx, ignoreVolumeAZ, req.GetAccessibilityRequirements()), nil
+	accessibleTopology := getTopology(vol, accessibleTopologyReq, ignoreVolumeAZ)
+
+	return getCreateVolumeResponse(vol, volCtx, accessibleTopology), nil
 }
 
 func (d *controllerServer) ControllerModifyVolume(ctx context.Context, req *csi.ControllerModifyVolumeRequest) (*csi.ControllerModifyVolumeResponse, error) {
@@ -1035,7 +1040,24 @@ func (cs *controllerServer) ControllerExpandVolume(ctx context.Context, req *csi
 	}, nil
 }
 
-func getCreateVolumeResponse(vol *volumes.Volume, volCtx map[string]string, ignoreVolumeAZ bool, accessibleTopologyReq *csi.TopologyRequirement) *csi.CreateVolumeResponse {
+func getTopology(vol *volumes.Volume, topologyReq *csi.TopologyRequirement, ignoreVolumeAZ bool) []*csi.Topology {
+	var accessibleTopology []*csi.Topology
+	if ignoreVolumeAZ {
+		if topologyReq != nil {
+			accessibleTopology = topologyReq.GetPreferred()
+		}
+	} else if vol.AvailabilityZone != "" {
+		// 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
+}
+
+func getCreateVolumeResponse(vol *volumes.Volume, volCtx map[string]string, accessibleTopology []*csi.Topology) *csi.CreateVolumeResponse {
 	var volsrc *csi.VolumeContentSource
 	volCnx := map[string]string{}
 
@@ -1075,21 +1097,6 @@ func getCreateVolumeResponse(vol *volumes.Volume, volCtx map[string]string, igno
 		}
 	}
 
-	var accessibleTopology []*csi.Topology
-	// If ignore-volume-az is true , dont set the accessible topology to volume az,
-	// use from preferred topologies instead.
-	if ignoreVolumeAZ {
-		if accessibleTopologyReq != nil {
-			accessibleTopology = accessibleTopologyReq.GetPreferred()
-		}
-	} else {
-		accessibleTopology = []*csi.Topology{
-			{
-				Segments: map[string]string{topologyKey: vol.AvailabilityZone},
-			},
-		}
-	}
-
 	resp := &csi.CreateVolumeResponse{
 		Volume: &csi.Volume{
 			VolumeId:           vol.ID,