Merge pull request #100100 from maxlaverse/fix_pv_provisioning

fix rounding of volume storage requests

pkg/volume/portworx/portworx_util.go

@@ -214,9 +214,9 @@ func (util *portworxVolumeUtil) ResizeVolume(spec *volume.Spec, newSize resource
 	}
 
 	vol := vols[0]
-	tBytes, ok := newSize.AsInt64()
+	tBytes, err := volumehelpers.RoundUpToB(newSize)
-	if !ok {
+	if err != nil {
-		return fmt.Errorf("quantity %v is too great, overflows int64", newSize)
+		return err
 	}
 	newSizeInBytes := uint64(tBytes)
 	if vol.Spec.Size >= newSizeInBytes {

staging/src/k8s.io/cloud-provider/volume/helpers/rounding.go

@@ -18,10 +18,22 @@ package helpers
 
 import (
 	"fmt"
+	"math"
+	"math/bits"
 
 	"k8s.io/apimachinery/pkg/api/resource"
 )
 
+/*
+The Cloud Provider's volume plugins provision disks for corresponding
+PersistentVolumeClaims. Cloud Providers use different allocation unit for their
+disk sizes. AWS allows you to specify the size as an integer amount of GiB,
+while Portworx expects bytes for example. On AWS, if you want a volume of
+1500MiB, the actual call to the AWS API should therefore be for a 2GiB disk.
+This file contains functions that help rounding a storage request based on a
+Cloud Provider's allocation unit.
+*/
+
 const (
 	// GB - GigaByte size
 	GB = 1000 * 1000 * 1000
@@ -41,139 +53,113 @@ const (
 
 // RoundUpToGiB rounds up given quantity upto chunks of GiB
 func RoundUpToGiB(size resource.Quantity) (int64, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt64(size, GiB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
-	}
-	return roundUpSize(requestBytes, GiB), nil
 }
 
 // RoundUpToMB rounds up given quantity to chunks of MB
 func RoundUpToMB(size resource.Quantity) (int64, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt64(size, MB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
-	}
-	return roundUpSize(requestBytes, MB), nil
 }
 
 // RoundUpToMiB rounds up given quantity upto chunks of MiB
 func RoundUpToMiB(size resource.Quantity) (int64, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt64(size, MiB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
-	}
-	return roundUpSize(requestBytes, MiB), nil
 }
 
 // RoundUpToKB rounds up given quantity to chunks of KB
 func RoundUpToKB(size resource.Quantity) (int64, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt64(size, KB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
-	}
-	return roundUpSize(requestBytes, KB), nil
 }
 
-// RoundUpToKiB rounds up given quantity upto chunks of KiB
+// RoundUpToKiB rounds up given quantity to chunks of KiB
 func RoundUpToKiB(size resource.Quantity) (int64, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt64(size, KiB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
 }
-	return roundUpSize(requestBytes, KiB), nil
+
+// RoundUpToB rounds up given quantity to chunks of bytes
+func RoundUpToB(size resource.Quantity) (int64, error) {
+	return roundUpSizeInt64(size, 1)
 }
 
 // RoundUpToGiBInt rounds up given quantity upto chunks of GiB. It returns an
 // int instead of an int64 and an error if there's overflow
 func RoundUpToGiBInt(size resource.Quantity) (int, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt(size, GiB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
-	}
-	return roundUpSizeInt(requestBytes, GiB)
 }
 
 // RoundUpToMBInt rounds up given quantity to chunks of MB. It returns an
 // int instead of an int64 and an error if there's overflow
 func RoundUpToMBInt(size resource.Quantity) (int, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt(size, MB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
-	}
-	return roundUpSizeInt(requestBytes, MB)
 }
 
 // RoundUpToMiBInt rounds up given quantity upto chunks of MiB. It returns an
 // int instead of an int64 and an error if there's overflow
 func RoundUpToMiBInt(size resource.Quantity) (int, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt(size, MiB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
-	}
-	return roundUpSizeInt(requestBytes, MiB)
 }
 
 // RoundUpToKBInt rounds up given quantity to chunks of KB. It returns an
 // int instead of an int64 and an error if there's overflow
 func RoundUpToKBInt(size resource.Quantity) (int, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt(size, KB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
-	}
-	return roundUpSizeInt(requestBytes, KB)
 }
 
 // RoundUpToKiBInt rounds up given quantity upto chunks of KiB. It returns an
 // int instead of an int64 and an error if there's overflow
 func RoundUpToKiBInt(size resource.Quantity) (int, error) {
-	requestBytes := size.Value()
+	return roundUpSizeInt(size, KiB)
-	return roundUpSizeInt(requestBytes, KiB)
 }
 
 // RoundUpToGiBInt32 rounds up given quantity up to chunks of GiB. It returns an
 // int32 instead of an int64 and an error if there's overflow
 func RoundUpToGiBInt32(size resource.Quantity) (int32, error) {
-	requestBytes, ok := size.AsInt64()
+	return roundUpSizeInt32(size, GiB)
-	if !ok {
-		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
-	}
-	return roundUpSizeInt32(requestBytes, GiB)
 }
 
 // roundUpSizeInt calculates how many allocation units are needed to accommodate
-// a volume of given size. It returns an int instead of an int64 and an error if
+// a volume of a given size. It returns an int and an error if there's overflow
-// there's overflow
+func roundUpSizeInt(size resource.Quantity, allocationUnitBytes int64) (int, error) {
-func roundUpSizeInt(volumeSizeBytes int64, allocationUnitBytes int64) (int, error) {
+	if bits.UintSize == 32 {
-	roundedUp := roundUpSize(volumeSizeBytes, allocationUnitBytes)
+		res, err := roundUpSizeInt32(size, allocationUnitBytes)
-	roundedUpInt := int(roundedUp)
+		return int(res), err
-	if int64(roundedUpInt) != roundedUp {
-		return 0, fmt.Errorf("capacity %v is too great, casting results in integer overflow", roundedUp)
 	}
-	return roundedUpInt, nil
+	res, err := roundUpSizeInt64(size, allocationUnitBytes)
+	return int(res), err
 }
 
 // roundUpSizeInt32 calculates how many allocation units are needed to accommodate
-// a volume of given size. It returns an int32 instead of an int64 and an error if
+// a volume of a given size. It returns an int32 and an error if there's overflow
-// there's overflow
+func roundUpSizeInt32(size resource.Quantity, allocationUnitBytes int64) (int32, error) {
-func roundUpSizeInt32(volumeSizeBytes int64, allocationUnitBytes int64) (int32, error) {
+	roundedUpInt32, err := roundUpSizeInt64(size, allocationUnitBytes)
-	roundedUp := roundUpSize(volumeSizeBytes, allocationUnitBytes)
+	if err != nil {
-	roundedUpInt32 := int32(roundedUp)
+		return 0, err
-	if int64(roundedUpInt32) != roundedUp {
-		return 0, fmt.Errorf("quantity %v is too great, overflows int32", roundedUp)
 	}
-	return roundedUpInt32, nil
+	if roundedUpInt32 > math.MaxInt32 {
+		return 0, fmt.Errorf("quantity %s is too great, overflows int32", size.String())
+	}
+	return int32(roundedUpInt32), nil
 }
 
-// roundUpSize calculates how many allocation units are needed to accommodate
+// roundUpSizeInt64 calculates how many allocation units are needed to accommodate
-// a volume of given size. E.g. when user wants 1500MiB volume, while AWS EBS
+// a volume of a given size. It returns an int64 and an error if there's overflow
-// allocates volumes in gibibyte-sized chunks,
+func roundUpSizeInt64(size resource.Quantity, allocationUnitBytes int64) (int64, error) {
-// RoundUpSize(1500 * 1024*1024, 1024*1024*1024) returns '2'
+	// Use CmpInt64() to find out if the value of "size" would overflow an
-// (2 GiB is the smallest allocatable volume that can hold 1500MiB)
+	// int64 and therefore have Value() return a wrong result. Then, retrieve
-func roundUpSize(volumeSizeBytes int64, allocationUnitBytes int64) int64 {
+	// the value as int64 and perform the rounding.
+	// It's not convenient to use AsScale() and related functions as they don't
+	// support BinarySI format, nor can we use AsInt64() directly since it's
+	// only implemented for int64 scaled numbers (int64Amount).
+
+	// CmpInt64() actually returns 0 when comparing an amount bigger than MaxInt64.
+	if size.CmpInt64(math.MaxInt64) >= 0 {
+		return 0, fmt.Errorf("quantity %s is too great, overflows int64", size.String())
+	}
+	volumeSizeBytes := size.Value()
+
 	roundedUp := volumeSizeBytes / allocationUnitBytes
 	if volumeSizeBytes%allocationUnitBytes > 0 {
 		roundedUp++
 	}
-	return roundedUp
+	return roundedUp, nil
 }

staging/src/k8s.io/cloud-provider/volume/helpers/rounding_test.go

@@ -60,8 +60,18 @@ func Test_RoundUpToGiB(t *testing.T) {
 			roundedVal: int64(1000),
 		},
 		{
-			name:        "round overflowed quantity to int64",
+			name:       "round decimal to GiB",
-			resource:    resource.MustParse("73786976299133170k"),
+			resource:   resource.MustParse("1.2Gi"),
+			roundedVal: int64(2),
+		},
+		{
+			name:       "round big value to GiB",
+			resource:   resource.MustParse("8191Pi"),
+			roundedVal: int64(8588886016),
+		},
+		{
+			name:        "round quantity to GiB that would lead to an int64 overflow",
+			resource:    resource.MustParse("8192Pi"),
 			roundedVal:  int64(0),
 			expectError: true,
 		},
@@ -125,8 +135,18 @@ func Test_RoundUpToMB(t *testing.T) {
 			roundedVal: int64(1073742),
 		},
 		{
-			name:        "round overflowed quantity to int64",
+			name:       "round decimal to MB",
-			resource:    resource.MustParse("73786976299133170k"),
+			resource:   resource.MustParse("1.2Gi"),
+			roundedVal: int64(1289),
+		},
+		{
+			name:       "round big value to MB",
+			resource:   resource.MustParse("8191Pi"),
+			roundedVal: int64(9222246136948),
+		},
+		{
+			name:        "round quantity to MB that would lead to an int64 overflow",
+			resource:    resource.MustParse("8192Pi"),
 			roundedVal:  int64(0),
 			expectError: true,
 		},
@@ -190,8 +210,18 @@ func Test_RoundUpToMiB(t *testing.T) {
 			roundedVal: int64(1024000),
 		},
 		{
-			name:        "round overflowed quantity to int64",
+			name:       "round decimal to MiB",
-			resource:    resource.MustParse("73786976299133170k"),
+			resource:   resource.MustParse("1.2Gi"),
+			roundedVal: int64(1229),
+		},
+		{
+			name:       "round big value to MiB",
+			resource:   resource.MustParse("8191Pi"),
+			roundedVal: int64(8795019280384),
+		},
+		{
+			name:        "round quantity to MiB that would lead to an int64 overflow",
+			resource:    resource.MustParse("8192Pi"),
 			roundedVal:  int64(0),
 			expectError: true,
 		},
@@ -255,8 +285,18 @@ func Test_RoundUpToKB(t *testing.T) {
 			roundedVal: int64(1073741824),
 		},
 		{
-			name:        "round overflowed quantity to int64",
+			name:       "round decimal to KB",
-			resource:    resource.MustParse("73786976299133170k"),
+			resource:   resource.MustParse("1.2Gi"),
+			roundedVal: int64(1288491),
+		},
+		{
+			name:       "round big value to KB",
+			resource:   resource.MustParse("8191Pi"),
+			roundedVal: int64(9222246136947934),
+		},
+		{
+			name:        "round quantity to KB that would lead to an int64 overflow",
+			resource:    resource.MustParse("8192Pi"),
 			roundedVal:  int64(0),
 			expectError: true,
 		},
@@ -320,8 +360,18 @@ func Test_RoundUpToKiB(t *testing.T) {
 			roundedVal: int64(1048576000),
 		},
 		{
-			name:        "round overflowed quantity to int64",
+			name:       "round decimal to KiB",
-			resource:    resource.MustParse("73786976299133170k"),
+			resource:   resource.MustParse("1.2Gi"),
+			roundedVal: int64(1258292),
+		},
+		{
+			name:       "round big value to KiB",
+			resource:   resource.MustParse("8191Pi"),
+			roundedVal: int64(9006099743113216),
+		},
+		{
+			name:        "round quantity to KiB that would lead to an int64 overflow",
+			resource:    resource.MustParse("8192Pi"),
 			roundedVal:  int64(0),
 			expectError: true,
 		},
@@ -385,8 +435,18 @@ func Test_RoundUpToGiBInt32(t *testing.T) {
 			roundedVal: int32(1000),
 		},
 		{
-			name:        "round overflowed quantity to int32",
+			name:       "round decimal to GiB",
-			resource:    resource.MustParse("73786976299133170k"),
+			resource:   resource.MustParse("1.2Gi"),
+			roundedVal: int32(2),
+		},
+		{
+			name:       "round big value to GiB",
+			resource:   resource.MustParse("2047Pi"),
+			roundedVal: int32(2146435072),
+		},
+		{
+			name:        "round quantity to GiB that would lead to an int32 overflow",
+			resource:    resource.MustParse("2048Pi"),
 			roundedVal:  int32(0),
 			expectError: true,
 		},
@@ -411,3 +471,53 @@ func Test_RoundUpToGiBInt32(t *testing.T) {
 		})
 	}
 }
+
+func Test_RoundUpToB(t *testing.T) {
+	testcases := []struct {
+		name        string
+		resource    resource.Quantity
+		roundedVal  int64
+		expectError bool
+	}{
+		{
+			name:       "round m to B",
+			resource:   resource.MustParse("987m"),
+			roundedVal: int64(1),
+		},
+		{
+			name:       "round decimal to B",
+			resource:   resource.MustParse("1.2Gi"),
+			roundedVal: int64(1288490189),
+		},
+		{
+			name:       "round big value to B",
+			resource:   resource.MustParse("8191Pi"),
+			roundedVal: int64(9222246136947933184),
+		},
+		{
+			name:        "round quantity to B that would lead to an int64 overflow",
+			resource:    resource.MustParse("8192Pi"),
+			roundedVal:  int64(0),
+			expectError: true,
+		},
+	}
+
+	for _, test := range testcases {
+		t.Run(test.name, func(t *testing.T) {
+			val, err := RoundUpToB(test.resource)
+			if !test.expectError && err != nil {
+				t.Errorf("expected no error got: %v", err)
+			}
+
+			if test.expectError && err == nil {
+				t.Errorf("expected error but got nothing")
+			}
+
+			if val != test.roundedVal {
+				t.Logf("actual rounded value: %d", val)
+				t.Logf("expected rounded value: %d", test.roundedVal)
+				t.Error("unexpected rounded value")
+			}
+		})
+	}
+}