Repository: kubernetes-sigs/gcp-filestore-csi-driver
Pull request #629 (2023-10-03 06:24:16)

[release-1.3] Silent volume provisioning surprise
This is an automated cherry-pick of #390

/assign tyuchn

```release-note
Previous Behavior: 
   * if a user requested a volume size smaller than the Standard minimum (1TB), we'd quietly round up to the standard Minimum.
   * we didn't account for different tiers having different minimums
   * we didn't check against the maximum volume size for a tier

New Behavior: 
  * Compare against min/max values of provided tier, instead of always standard. (If tier is unrecognized, default to standard)
  * If the requested min is less than possible min, and no upper bound is given, return an error instead of quietly rounding up
  * vice versa for max
```


Patch:
diff --git a/pkg/csi_driver/controller.go b/pkg/csi_driver/controller.go
index ecd4f00f..fa04e742 100644
--- a/pkg/csi_driver/controller.go
+++ b/pkg/csi_driver/controller.go
@@ -33,15 +33,26 @@ import (
 )
 
 const (
-	// premium tier min is 2.5 Tb, let GCFS error
-	minVolumeSize     int64 = 1 * util.Tb
-	modeInstance            = "modeInstance"
-	newInstanceVolume       = "vol1"
+	modeInstance      = "modeInstance"
+	newInstanceVolume = "vol1"
 
 	defaultTier    = "standard"
 	enterpriseTier = "enterprise"
+	premiumTier    = "premium"
+	basicHDDTier   = "basic_hdd"
+	basicSSDTier   = "basic_ssd"
+	highScaleTier  = "high_scale_ssd"
 	defaultNetwork = "default"
 
+	defaultTierMinSize    = 1 * util.Tb
+	defaultTierMaxSize    = 639 * util.Tb / 10
+	enterpriseTierMinSize = 1 * util.Tb
+	enterpriseTierMaxSize = 10 * util.Tb
+	highScaleTierMinSize  = 10 * util.Tb
+	highScaleTierMaxSize  = 100 * util.Tb
+	premiumTierMinSize    = 25 * util.Tb / 10
+	premiumTierMaxSize    = 639 * util.Tb / 10
+
 	directPeering        = "DIRECT_PEERING"
 	privateServiceAccess = "PRIVATE_SERVICE_ACCESS"
 
@@ -84,6 +95,11 @@ const (
 	tagKeyClusterLocation          = "storage_gke_io_cluster_location"
 )
 
+type capacityRangeForTier struct {
+	min int64
+	max int64
+}
+
 // controllerServer handles volume provisioning
 type controllerServer struct {
 	config *controllerServerConfig
@@ -137,7 +153,8 @@ func (s *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolu
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 
-	capBytes, err := getRequestCapacity(req.GetCapacityRange())
+	tier := getTierFromParams(req.GetParameters())
+	capBytes, err := getRequestCapacity(req.GetCapacityRange(), tier)
 	if err != nil {
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
@@ -413,36 +430,98 @@ func (s *controllerServer) ControllerGetCapabilities(ctx context.Context, req *c
 	}, nil
 }
 
-// getRequestCapacity returns the volume size that should be provisioned
-func getRequestCapacity(capRange *csi.CapacityRange) (int64, error) {
-	if capRange == nil {
-		return minVolumeSize, nil
+// getTierFromParams returns the provided tier or default
+func getTierFromParams(params map[string]string) string {
+	if val, ok := params[paramTier]; ok {
+		return val
 	}
 
-	rCap := capRange.GetRequiredBytes()
-	rSet := rCap > 0
-	lCap := capRange.GetLimitBytes()
-	lSet := lCap > 0
+	return defaultTier
+}
+
+// validator function to check for invalid capacity size requests
+func invalidCapacityRange(capRange *csi.CapacityRange, tier string) error {
+	validRange := provisionableCapacityForTier(tier)
 
-	if lSet && rSet && lCap < rCap {
-		return 0, fmt.Errorf("limit bytes %v is less than required bytes %v", lCap, rCap)
+	requiredCap := capRange.GetRequiredBytes()
+	requireSet := requiredCap > 0
+	limitCap := capRange.GetLimitBytes()
+	limitSet := limitCap > 0
+
+	if limitSet && requireSet && limitCap < requiredCap {
+		return fmt.Errorf("limit bytes %vTiB is less than required bytes %vTiB", float64(limitCap)/util.Tb, float64(requiredCap)/util.Tb)
 	}
 
-	if lSet && lCap < minVolumeSize {
-		return 0, fmt.Errorf("limit bytes %v is less than minimum instance size bytes %v", lCap, minVolumeSize)
+	if requireSet {
+		if requiredCap > validRange.max {
+			return fmt.Errorf("request bytes %vTiB is more than maximum instance size bytes %vTiB for tier %s", float64(requiredCap)/util.Tb, float64(validRange.max)/util.Tb, tier)
+		}
+
+		if !limitSet && requiredCap < validRange.min {
+			// Avoid surprising users by provisioning more than Requested
+			klog.Warningf("required bytes %vTiB is less than minimum instance size capacity %vTiB for tier %s, but no upper bound was specified. Rounding up capacity request to %vTiB for tier %s.", float64(requiredCap)/util.Tb, float64(validRange.min)/util.Tb, tier, float64(validRange.min)/util.Tb, tier)
+		}
 	}
+	if limitSet {
+		if limitCap < validRange.min {
+			return fmt.Errorf("limit bytes %vTiB is less than minimum instance size bytes %vTiB for tier %s", float64(limitCap)/util.Tb, float64(validRange.min)/util.Tb, tier)
 
-	if lSet {
-		if rCap == 0 {
-			// request not set
-			return lCap, nil
 		}
-		// request set, round up to min
-		return util.Max(rCap, minVolumeSize), nil
+		if !requireSet && limitCap > validRange.max {
+			// Avoid surprising users by provisioning less than Requested
+			klog.Warningf("required bytes %vTiB is greater than maximum instance size capacity %vTiB for tier %s, but no lower bound was specified. Rounding down capacity request to %vTiB for tier %s", float64(limitCap)/util.Tb, float64(validRange.max)/util.Tb, tier, float64(validRange.max)/util.Tb, tier)
+		}
+	}
+
+	return nil
+}
+
+// init function to get min and max volume sizes per tier
+func provisionableCapacityForTier(tier string) capacityRangeForTier {
+	defaultRange := capacityRangeForTier{min: defaultTierMinSize, max: defaultTierMaxSize}
+	enterpriseRange := capacityRangeForTier{min: enterpriseTierMinSize, max: enterpriseTierMaxSize}
+	highScaleRange := capacityRangeForTier{min: highScaleTierMinSize, max: highScaleTierMaxSize}
+	premiumRange := capacityRangeForTier{min: premiumTierMinSize, max: premiumTierMaxSize}
+	provisionableCapacityForTier := map[string]capacityRangeForTier{
+		defaultTier:    defaultRange,
+		enterpriseTier: enterpriseRange,
+		highScaleTier:  highScaleRange,
+		premiumTier:    premiumRange,
+		basicSSDTier:   premiumRange, //these two are aliases
+		basicHDDTier:   defaultRange, //these two are aliases
+	}
+
+	validRange, ok := provisionableCapacityForTier[tier]
+	if !ok {
+		validRange = provisionableCapacityForTier[defaultTier]
+	}
+	return validRange
+}
+
+// getRequestCapacity returns the volume size that should be provisioned
+func getRequestCapacity(capRange *csi.CapacityRange, tier string) (int64, error) {
+	validRange := provisionableCapacityForTier(tier)
+
+	if capRange == nil {
+		return validRange.min, nil
 	}
 
-	// limit not set
-	return util.Max(rCap, minVolumeSize), nil
+	if err := invalidCapacityRange(capRange, tier); err != nil {
+		return 0, err
+	}
+
+	requiredCap := capRange.GetRequiredBytes()
+	requireSet := requiredCap > 0
+	maxRequired := capRange.GetLimitBytes()
+	limitSet := maxRequired > 0
+
+	if requireSet {
+		return util.Max(requiredCap, validRange.min), nil
+	} else if limitSet {
+		return util.Min(maxRequired, validRange.max), nil
+	} else {
+		return validRange.min, nil
+	}
 }
 
 // generateNewFileInstance populates the GCFS Instance object using
@@ -555,11 +634,6 @@ func (s *controllerServer) ControllerExpandVolume(ctx context.Context, req *csi.
 		return response, err
 	}
 
-	reqBytes, err := getRequestCapacity(req.GetCapacityRange())
-	if err != nil {
-		return nil, status.Error(codes.InvalidArgument, err.Error())
-	}
-
 	if acquired := s.config.volumeLocks.TryAcquire(volumeID); !acquired {
 		return nil, status.Errorf(codes.Aborted, util.VolumeOperationAlreadyExistsFmt, volumeID)
 	}
@@ -570,6 +644,11 @@ func (s *controllerServer) ControllerExpandVolume(ctx context.Context, req *csi.
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 
+	reqBytes, err := getRequestCapacity(req.GetCapacityRange(), filer.Tier)
+	if err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}
+
 	filer.Project = s.config.cloud.Project
 	filer, err = s.config.fileService.GetInstance(ctx, filer)
 	if err != nil {


Test patch:
diff --git a/pkg/csi_driver/controller_test.go b/pkg/csi_driver/controller_test.go
index 4b28d021..b6942a4a 100644
--- a/pkg/csi_driver/controller_test.go
+++ b/pkg/csi_driver/controller_test.go
@@ -425,24 +425,29 @@ func TestGetRequestCapacity(t *testing.T) {
 		name          string
 		capRange      *csi.CapacityRange
 		bytes         int64
+		tier          string
 		errorExpected bool
 	}{
 		{
 			name:  "default",
 			bytes: 1 * util.Tb,
+			tier:  defaultTier,
 		},
 		{
-			name: "required below min",
+			name: "required below min, limit not provided",
 			capRange: &csi.CapacityRange{
 				RequiredBytes: 100 * util.Gb,
 			},
-			bytes: 1 * util.Tb,
+			tier:          defaultTier,
+			bytes:         1 * util.Tb,
+			errorExpected: false,
 		},
 		{
 			name: "required equals min",
 			capRange: &csi.CapacityRange{
 				RequiredBytes: 1 * util.Tb,
 			},
+			tier:  defaultTier,
 			bytes: 1 * util.Tb,
 		},
 		{
@@ -450,6 +455,7 @@ func TestGetRequestCapacity(t *testing.T) {
 			capRange: &csi.CapacityRange{
 				RequiredBytes: 1*util.Tb + 1*util.Gb,
 			},
+			tier:  defaultTier,
 			bytes: 1*util.Tb + 1*util.Gb,
 		},
 		{
@@ -457,6 +463,7 @@ func TestGetRequestCapacity(t *testing.T) {
 			capRange: &csi.CapacityRange{
 				LimitBytes: 1 * util.Tb,
 			},
+			tier:  defaultTier,
 			bytes: 1 * util.Tb,
 		},
 		{
@@ -464,6 +471,7 @@ func TestGetRequestCapacity(t *testing.T) {
 			capRange: &csi.CapacityRange{
 				LimitBytes: 1*util.Tb + 1*util.Gb,
 			},
+			tier:  defaultTier,
 			bytes: 1*util.Tb + 1*util.Gb,
 		},
 		{
@@ -472,6 +480,7 @@ func TestGetRequestCapacity(t *testing.T) {
 				RequiredBytes: 100 * util.Gb,
 				LimitBytes:    2 * util.Tb,
 			},
+			tier:  defaultTier,
 			bytes: 1 * util.Tb,
 		},
 		{
@@ -480,6 +489,7 @@ func TestGetRequestCapacity(t *testing.T) {
 				RequiredBytes: 100 * util.Gb,
 				LimitBytes:    500 * util.Gb,
 			},
+			tier:          defaultTier,
 			errorExpected: true,
 		},
 		{
@@ -488,20 +498,173 @@ func TestGetRequestCapacity(t *testing.T) {
 				RequiredBytes: 5 * util.Tb,
 				LimitBytes:    2 * util.Tb,
 			},
+			tier:          defaultTier,
+			errorExpected: true,
+		},
+		{
+			name: "limit below min default",
+			capRange: &csi.CapacityRange{
+				LimitBytes: 100 * util.Gb,
+			},
+			tier:          defaultTier,
+			errorExpected: true,
+		},
+		{
+			name: "required above max default",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 100 * util.Tb,
+			},
+			tier:          defaultTier,
+			errorExpected: true,
+		},
+		{
+			name: "limit above max and no min provided",
+			capRange: &csi.CapacityRange{
+				LimitBytes: 100 * util.Tb,
+			},
+			tier:          defaultTier,
+			bytes:         639 * util.Tb / 10,
+			errorExpected: false,
+		},
+		{
+			name: "limit above max but min in range",
+			capRange: &csi.CapacityRange{
+				LimitBytes:    100 * util.Tb,
+				RequiredBytes: 15 * util.Tb,
+			},
+			tier:  defaultTier,
+			bytes: 15 * util.Tb,
+		},
+		{
+			name: "limit below min enterprise",
+			capRange: &csi.CapacityRange{
+				LimitBytes: 100 * util.Gb,
+			},
+			tier:          enterpriseTier,
+			errorExpected: true,
+		},
+		{
+			name: "required above max enterprise",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 100 * util.Tb,
+			},
+			tier:          enterpriseTier,
+			errorExpected: true,
+		},
+		{
+			name: "required and limit both in range enterprise",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 2 * util.Tb,
+				LimitBytes:    3 * util.Tb,
+			},
+			tier:  enterpriseTier,
+			bytes: 2 * util.Tb,
+		},
+		{
+			name: "limit below min highScale",
+			capRange: &csi.CapacityRange{
+				LimitBytes: 5 * util.Tb,
+			},
+			tier:          highScaleTier,
+			errorExpected: true,
+		},
+		{
+			name: "required above max highScale",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 200 * util.Tb,
+			},
+			tier:          highScaleTier,
 			errorExpected: true,
 		},
 		{
-			name: "limit below min",
+			name: "required and limit both in range highScale",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 20 * util.Tb,
+				LimitBytes:    30 * util.Tb,
+			},
+			tier:  highScaleTier,
+			bytes: 20 * util.Tb,
+		},
+		{
+			name: "limit below min premium",
+			capRange: &csi.CapacityRange{
+				LimitBytes: 1 * util.Tb,
+			},
+			tier:          premiumTier,
+			errorExpected: true,
+		},
+		{
+			name: "required above max premium",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 70 * util.Tb,
+			},
+			tier:          premiumTier,
+			errorExpected: true,
+		},
+		{
+			name: "required and limit both in range premium",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 3 * util.Tb,
+				LimitBytes:    60 * util.Tb,
+			},
+			tier:  premiumTier,
+			bytes: 3 * util.Tb,
+		},
+		{
+			name: "limit below min basicSSD",
+			capRange: &csi.CapacityRange{
+				LimitBytes: 1 * util.Tb,
+			},
+			tier:          basicSSDTier,
+			errorExpected: true,
+		},
+		{
+			name: "required above max basicSSD",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 70 * util.Tb,
+			},
+			tier:          basicSSDTier,
+			errorExpected: true,
+		},
+		{
+			name: "required and limit both in range basicSSD",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 3 * util.Tb,
+				LimitBytes:    60 * util.Tb,
+			},
+			tier:  basicSSDTier,
+			bytes: 3 * util.Tb,
+		},
+		{
+			name: "limit below min basicHDD",
 			capRange: &csi.CapacityRange{
 				LimitBytes: 100 * util.Gb,
 			},
+			tier:          basicHDDTier,
 			errorExpected: true,
 		},
+		{
+			name: "required above max basicHDD",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 70 * util.Tb,
+			},
+			tier:          basicHDDTier,
+			errorExpected: true,
+		},
+		{
+			name: "required and limit both in range basicHDD",
+			capRange: &csi.CapacityRange{
+				RequiredBytes: 1 * util.Tb,
+				LimitBytes:    60 * util.Tb,
+			},
+			tier:  basicHDDTier,
+			bytes: 1 * util.Tb,
+		},
 	}
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			bytes, err := getRequestCapacity(tc.capRange)
+			bytes, err := getRequestCapacity(tc.capRange, tc.tier)
 			if err != nil && tc.errorExpected {
 				return
 			}