fix bug where volume cloning topology requirements are ignored when chosing the location of the volume

Author: amacaskill

pkg/gce-pd-csi-driver/controller.go

@@ -32,7 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/uuid"
 	"k8s.io/client-go/util/flowcontrol"
 	"k8s.io/klog/v2"
-
+	"k8s.io/utils/strings/slices"
 	"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
 	gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute"
 )
@@ -98,6 +98,12 @@ type workItem struct {
 	unpublishReq *csi.ControllerUnpublishVolumeRequest
 }
 
+// locationRequirements are additional location topology requirements that must be respected when creating a volume.
+type locationRequirements struct {
+	region string
+	zone   string
+}
+
 var _ csi.ControllerServer = &GCEControllerServer{}
 
 const (
@@ -142,6 +148,29 @@ func isDiskReady(disk *gce.CloudDisk) (bool, error) {
 	return false, nil
 }
 
+// cloningLocationRequirements returns additional location requirements to be applied to the given create volume requests topology.
+// If the CreateVolumeRequest will use volume cloning, location requirements in compliance with the volume cloning limitations
+// will be returned: https://cloud.google.com/kubernetes-engine/docs/how-to/persistent-volumes/volume-cloning#limitations
+func cloningLocationRequirements(req *csi.CreateVolumeRequest, replicationType string) (*locationRequirements, error) {
+	if !useVolumeCloning(req) {
+		return nil, nil
+	}
+	// If we are using volume cloning, this will be set.
+	volSrc := req.VolumeContentSource.GetVolume()
+	volSrcVolID := volSrc.GetVolumeId()
+
+	_, sourceVolKey, err := common.VolumeIDToKey(volSrcVolID)
+	if err != nil {
+		return nil, err
+	}
+	return &locationRequirements{zone: sourceVolKey.Zone, region: sourceVolKey.Region}, nil
+}
+
+// useVolumeCloning returns true if the create volume request should be created with volume cloning.
+func useVolumeCloning(req *csi.CreateVolumeRequest) bool {
+	return req.VolumeContentSource != nil && req.VolumeContentSource.GetVolume() != nil
+}
+
 func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) {
 	var err error
 	// Validate arguments
@@ -177,12 +206,21 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
 	if multiWriter {
 		gceAPIVersion = gce.GCEAPIVersionBeta
 	}
+
+	var locationTopReq *locationRequirements
+	if useVolumeCloning(req) {
+		locationTopReq, err = cloningLocationRequirements(req, params.ReplicationType)
+		if err != nil {
+			return nil, status.Errorf(codes.Internal, "failed to get location topology requirements for volume cloning: %v", err)
+		}
+	}
+
 	// Determine the zone or zones+region of the disk
 	var zones []string
 	var volKey *meta.Key
 	switch params.ReplicationType {
 	case replicationTypeNone:
-		zones, err = pickZones(ctx, gceCS, req.GetAccessibilityRequirements(), 1)
+		zones, err = pickZones(ctx, gceCS, req.GetAccessibilityRequirements(), 1, locationTopReq)
 		if err != nil {
 			return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to pick zones for disk: %v", err.Error())
 		}
@@ -192,7 +230,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
 		volKey = meta.ZonalKey(name, zones[0])
 
 	case replicationTypeRegionalPD:
-		zones, err = pickZones(ctx, gceCS, req.GetAccessibilityRequirements(), 2)
+		zones, err = pickZones(ctx, gceCS, req.GetAccessibilityRequirements(), 2, locationTopReq)
 		if err != nil {
 			return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to pick zones for disk: %v", err.Error())
 		}
@@ -1358,7 +1396,7 @@ func diskIsAttachedAndCompatible(deviceName string, instance *compute.Instance,
 	return false, nil
 }
 
-func pickZonesFromTopology(top *csi.TopologyRequirement, numZones int) ([]string, error) {
+func pickZonesFromTopology(top *csi.TopologyRequirement, numZones int, locationTopReq *locationRequirements) ([]string, error) {
 	reqZones, err := getZonesFromTopology(top.GetRequisite())
 	if err != nil {
 		return nil, fmt.Errorf("could not get zones from requisite topology: %w", err)
@@ -1368,6 +1406,11 @@ func pickZonesFromTopology(top *csi.TopologyRequirement, numZones int) ([]string
 		return nil, fmt.Errorf("could not get zones from preferred topology: %w", err)
 	}
 
+	// For volumeCloning, only 1 zone can be chosen. It doesn't matter if it comes from Requisite or Preferred.
+	if locationTopReq != nil && slices.Contains(prefZones, locationTopReq.zone) || slices.Contains(reqZones, locationTopReq.zone) {
+		return []string{locationTopReq.zone}, nil
+	}
+
 	if numZones <= len(prefZones) {
 		return prefZones[0:numZones], nil
 	} else {
@@ -1426,16 +1469,16 @@ func getZoneFromSegment(seg map[string]string) (string, error) {
 	return zone, nil
 }
 
-func pickZones(ctx context.Context, gceCS *GCEControllerServer, top *csi.TopologyRequirement, numZones int) ([]string, error) {
+func pickZones(ctx context.Context, gceCS *GCEControllerServer, top *csi.TopologyRequirement, numZones int, locationTopReq *locationRequirements) ([]string, error) {
 	var zones []string
 	var err error
 	if top != nil {
-		zones, err = pickZonesFromTopology(top, numZones)
+		zones, err = pickZonesFromTopology(top, numZones, locationTopReq)
 		if err != nil {
 			return nil, fmt.Errorf("failed to pick zones from topology: %w", err)
 		}
 	} else {
-		zones, err = getDefaultZonesInRegion(ctx, gceCS, []string{gceCS.CloudProvider.GetDefaultZone()}, numZones)
+		zones, err = getDefaultZonesInRegion(ctx, gceCS, []string{gceCS.CloudProvider.GetDefaultZone()}, numZones, locationTopReq)
 		if err != nil {
 			return nil, fmt.Errorf("failed to get default %v zones in region: %w", numZones, err)
 		}
@@ -1445,16 +1488,29 @@ func pickZones(ctx context.Context, gceCS *GCEControllerServer, top *csi.Topolog
 	return zones, nil
 }
 
-func getDefaultZonesInRegion(ctx context.Context, gceCS *GCEControllerServer, existingZones []string, numZones int) ([]string, error) {
+func getDefaultZonesInRegion(ctx context.Context, gceCS *GCEControllerServer, existingZones []string, numZones int, locationTopReq *locationRequirements) ([]string, error) {
 	region, err := common.GetRegionFromZones(existingZones)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get region from zones: %w", err)
 	}
-	needToGet := numZones - len(existingZones)
+
 	totZones, err := gceCS.CloudProvider.ListZones(ctx, region)
 	if err != nil {
 		return nil, fmt.Errorf("failed to list zones from cloud provider: %w", err)
 	}
+
+	// For volumeCloning, only 1 zone can be chosen. It doesn't matter if it comes from Requisite or Preferred.
+	if locationTopReq != nil {
+		if region != locationTopReq.region {
+			return nil, fmt.Errorf("default region is not suitable for volume cloning")
+		}
+		if !slices.Contains(totZones, locationTopReq.zone) {
+			return nil, fmt.Errorf("failed to get zone in region %s suitable for volume cloning. Zone must match", region)
+		}
+		return []string{locationTopReq.zone}, nil
+	}
+
+	needToGet := numZones - len(existingZones)
 	remainingZones := sets.NewString(totZones...).Difference(sets.NewString(existingZones...))
 	l := remainingZones.List()
 	if len(l) < needToGet {

pkg/gce-pd-csi-driver/controller_test.go

@@ -1924,7 +1924,7 @@ func TestPickZonesFromTopology(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		t.Logf("test case: %s", tc.name)
-		gotZones, err := pickZonesFromTopology(tc.top, tc.numZones)
+		gotZones, err := pickZonesFromTopology(tc.top, tc.numZones, nil)
 		if err != nil && !tc.expErr {
 			t.Errorf("Did not expect error but got: %v", err)
 		}