Skip to content

Commit 395e708

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #2732 from ConnorJC3/fix-iops
Fix IOPS handling
2 parents 9226ebf + 1539e9f commit 395e708

File tree

3 files changed

+31
-66
lines changed

3 files changed

+31
-66
lines changed

‎pkg/cloud/cloud.go‎

Lines changed: 21 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -677,18 +677,15 @@ func (c *cloud) CreateDisk(ctx context.Context, volumeName string, diskOptions *
677677
outpostArn: diskOptions.OutpostArn,
678678
}
679679

680-
iopsLimit, err := c.getVolumeLimits(ctx, createType, azParams)
681-
if err != nil {
682-
return nil, fmt.Errorf("invalid AWS VolumeType %q", diskOptions.VolumeType)
683-
}
680+
iopsLimits := c.getVolumeLimits(ctx, createType, azParams)
684681

685682
if diskOptions.IOPS > 0 {
686683
iops = diskOptions.IOPS
687684
} else if diskOptions.IOPSPerGB > 0 {
688685
iops = diskOptions.IOPSPerGB * capacityGiB
689686
}
690-
if iopsLimit.maxIops > 0 && iops > 0 {
691-
iops = capIOPS(createType, capacityGiB, iops, iopsLimit, diskOptions.AllowIOPSPerGBIncrease)
687+
if iops > 0 {
688+
iops = capIOPS(createType, capacityGiB, iops, iopsLimits, diskOptions.AllowIOPSPerGBIncrease)
692689
}
693690

694691
if isClone {
@@ -724,7 +721,7 @@ func (c *cloud) CreateDisk(ctx context.Context, volumeName string, diskOptions *
724721
c.latestClientTokens.Set(volumeName, &nextTokenNumber)
725722
return nil, ErrIdempotentParameterMismatch
726723
case isAWSErrorInvalidParameterCombination(err):
727-
return nil, ErrInvalidArgument
724+
return nil, fmt.Errorf("%w: %w", ErrInvalidArgument, err)
728725
case isAWSErrorVolumeLimitExceeded(err):
729726
// EC2 API does NOT handle idempotency correctly when a theoretical volume
730727
// would put the caller over a limit for their account
@@ -1005,16 +1002,8 @@ func (c *cloud) ResizeOrModifyDisk(ctx context.Context, volumeID string, newSize
10051002
if volume.OutpostArn != nil {
10061003
azParams.outpostArn = *volume.OutpostArn
10071004
}
1008-
iopsLimit, err := c.getVolumeLimits(ctx, string(volTypeToUse), azParams)
1009-
if err != nil {
1010-
return 0, err
1011-
}
1012-
// Even if volume type does not support IOPS, still pass value and let EC2 handle error.
1013-
if iopsLimit.maxIops == 0 {
1014-
req.Iops = aws.Int32(iopsForModify)
1015-
} else {
1016-
req.Iops = aws.Int32(capIOPS(string(volTypeToUse), sizeToUse, iopsForModify, iopsLimit, true))
1017-
}
1005+
iopsLimits := c.getVolumeLimits(ctx, string(volTypeToUse), azParams)
1006+
req.Iops = aws.Int32(capIOPS(string(volTypeToUse), sizeToUse, iopsForModify, iopsLimits, true))
10181007
options.IOPS = *req.Iops
10191008
}
10201009

@@ -2622,7 +2611,7 @@ func volumeModificationDone(state string) bool {
26222611
return state == string(types.VolumeModificationStateCompleted) || state == string(types.VolumeModificationStateOptimizing)
26232612
}
26242613

2625-
// Calculate actual IOPS for a volume and cap it at supported AWS limits.
2614+
// Calculate actual IOPS for a volume and cap it at supported AWS limits. Any limit of 0 is considered "infinite" (i.e. is not applied).
26262615
func capIOPS(volumeType string, requestedCapacityGiB int32, requestedIops int32, iopsLimits iopsLimits, allowIncrease bool) int32 {
26272616
// If requestedIops is zero the user did not request a specific amount, and the default will be used instead
26282617
if requestedIops == 0 {
@@ -2631,29 +2620,27 @@ func capIOPS(volumeType string, requestedCapacityGiB int32, requestedIops int32,
26312620

26322621
iops := requestedIops
26332622

2634-
if iops < iopsLimits.minIops {
2635-
if allowIncrease {
2636-
iops = iopsLimits.minIops
2637-
klog.V(5).InfoS("[Debug] Increased IOPS to the min supported limit", "volumeType", volumeType, "requestedCapacityGiB", requestedCapacityGiB, "limit", iops)
2638-
}
2623+
if iopsLimits.minIops > 0 && iops < iopsLimits.minIops && allowIncrease {
2624+
iops = iopsLimits.minIops
2625+
klog.V(5).InfoS("[Debug] Increased IOPS to the min supported limit", "volumeType", volumeType, "requestedCapacityGiB", requestedCapacityGiB, "limit", iops)
26392626
}
2640-
if iops > iopsLimits.maxIops {
2627+
if iopsLimits.maxIops > 0 && iops > iopsLimits.maxIops {
26412628
iops = iopsLimits.maxIops
26422629
klog.V(5).InfoS("[Debug] Capped IOPS, volume at the max supported limit", "volumeType", volumeType, "requestedCapacityGiB", requestedCapacityGiB, "limit", iops)
26432630
}
26442631
maxIopsByCapacity := iopsLimits.maxIopsPerGb * requestedCapacityGiB
2645-
if iops > maxIopsByCapacity && maxIopsByCapacity >= iopsLimits.minIops {
2632+
if maxIopsByCapacity > 0 && iops > maxIopsByCapacity && maxIopsByCapacity >= iopsLimits.minIops {
26462633
iops = maxIopsByCapacity
26472634
klog.V(5).InfoS("[Debug] Capped IOPS for volume", "volumeType", volumeType, "requestedCapacityGiB", requestedCapacityGiB, "maxIOPSPerGB", iopsLimits.maxIopsPerGb, "limit", iops)
26482635
}
26492636
return iops
26502637
}
26512638

26522639
// Gets IOPS limits for a specific volume type in a specific Zone and caches it. If the limits are cached, simply return limits.
2653-
func (c *cloud) getVolumeLimits(ctx context.Context, volumeType string, azParams getVolumeLimitsParams) (iopsLimits iopsLimits, err error) {
2640+
func (c *cloud) getVolumeLimits(ctx context.Context, volumeType string, azParams getVolumeLimitsParams) (iopsLimits iopsLimits) {
26542641
cacheKey := fmt.Sprintf("%s|%s|%s|%s", volumeType, azParams.availabilityZone, azParams.availabilityZoneId, azParams.outpostArn)
26552642
if value, ok := c.latestIOPSLimits.Get(cacheKey); ok {
2656-
return *value, nil
2643+
return *value
26572644
}
26582645

26592646
dryRunRequestInput := &ec2.CreateVolumeInput{
@@ -2685,7 +2672,7 @@ func (c *cloud) getVolumeLimits(ctx context.Context, volumeType string, azParams
26852672
}
26862673

26872674
volType := strings.ToLower(string(dryRunRequestInput.VolumeType))
2688-
_, err = c.ec2.CreateVolume(ctx, dryRunRequestInput, func(o *ec2.Options) {
2675+
_, err := c.ec2.CreateVolume(ctx, dryRunRequestInput, func(o *ec2.Options) {
26892676
o.APIOptions = nil // Don't add our logging/metrics middleware because we expect errors.
26902677
})
26912678
useFallBackLimits := (err == nil) // If DryRun unexpectedly succeeds, we use fallback values.
@@ -2714,7 +2701,6 @@ func (c *cloud) getVolumeLimits(ctx context.Context, volumeType string, azParams
27142701

27152702
// Set minIops and maxIopsPerGb because we do not fetch these from DryRun Error, we can also catch invalid volume.
27162703
switch volType {
2717-
case VolumeTypeGP2, VolumeTypeSC1, VolumeTypeST1, VolumeTypeStandard:
27182704
case VolumeTypeIO1:
27192705
iopsLimits.minIops = io1MinTotalIOPS
27202706
iopsLimits.maxIopsPerGb = io1MaxIOPSPerGB
@@ -2725,18 +2711,22 @@ func (c *cloud) getVolumeLimits(ctx context.Context, volumeType string, azParams
27252711
iopsLimits.minIops = gp3MinTotalIOPS
27262712
iopsLimits.maxIopsPerGb = gp3MaxIOPSPerGB
27272713
default:
2728-
return iopsLimits, fmt.Errorf("invalid AWS VolumeType %q", volumeType)
2714+
klog.V(5).InfoS("[Debug] No known limits for volume type, not performing capping", "volumeType", volumeType)
27292715
}
27302716

27312717
if !useFallBackLimits {
27322718
c.latestIOPSLimits.Set(cacheKey, &iopsLimits)
27332719
}
27342720

2735-
return iopsLimits, nil
2721+
return iopsLimits
27362722
}
27372723

27382724
// Get what the maxIops is from DryRun error message.
27392725
func extractMaxIOPSFromError(errorMsg string, volumeType string) (int32, error) {
2726+
// Volume does not support IOPS, so return a limit of 0 (considered infinite in capIOPS).
2727+
if strings.Contains(errorMsg, "parameter iops is not supported") {
2728+
return 0, nil
2729+
}
27402730
// io1 and gp3 have the same error message but io2 has different one, using by default.
27412731
if volumeType == VolumeTypeIO2 {
27422732
if matches := io2ErrRegex.FindStringSubmatch(errorMsg); len(matches) > 1 {

‎pkg/cloud/cloud_test.go‎

Lines changed: 7 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1807,21 +1807,6 @@ func TestCreateDisk(t *testing.T) {
18071807
},
18081808
expErr: errors.New("CreateDisk: multi-attach is only supported for io2 volumes"),
18091809
},
1810-
{
1811-
name: "failure: invalid VolumeType",
1812-
volumeName: "vol-test-name",
1813-
diskOptions: &DiskOptions{
1814-
CapacityBytes: util.GiBToBytes(1),
1815-
Tags: map[string]string{VolumeNameTagKey: "vol-test", AwsEbsDriverTagKey: "true"},
1816-
VolumeType: "invalidVolumeType",
1817-
},
1818-
expDisk: &Disk{
1819-
VolumeID: "vol-test",
1820-
CapacityGiB: 1,
1821-
AvailabilityZone: defaultZone,
1822-
},
1823-
expErr: fmt.Errorf("invalid AWS VolumeType %q", "invalidVolumeType"),
1824-
},
18251810
{
18261811
name: "success: create volume returned volume limit exceeded error, but volume exists",
18271812
volumeName: "vol-test-name",
@@ -4778,7 +4763,6 @@ func TestGetVolumeLimits(t *testing.T) {
47784763
cachedLimits *iopsLimits
47794764
createVolumeErr error
47804765
expectedLimits iopsLimits
4781-
expectError bool
47824766
expectCaching bool
47834767
azParams getVolumeLimitsParams
47844768
}{
@@ -4897,9 +4881,11 @@ func TestGetVolumeLimits(t *testing.T) {
48974881
azParams: getVolumeLimitsParams{
48984882
availabilityZone: *aws.String("us-west-2a"),
48994883
},
4900-
createVolumeErr: errors.New("Generic error"),
4901-
expectError: true,
4902-
expectCaching: false,
4884+
createVolumeErr: errors.New("The parameter iops is not supported for gp2 volumes."),
4885+
expectedLimits: iopsLimits{
4886+
maxIops: 0,
4887+
},
4888+
expectCaching: true,
49034889
},
49044890
{
49054891
name: "cache miss: dry run unexpected success, use fallback limits",
@@ -4946,18 +4932,7 @@ func TestGetVolumeLimits(t *testing.T) {
49464932
}
49474933

49484934
ctx := context.Background()
4949-
limits, err := c.getVolumeLimits(ctx, tc.volumeType, tc.azParams)
4950-
4951-
if tc.expectError {
4952-
if err == nil {
4953-
t.Fatal("Expected error but got none")
4954-
}
4955-
return
4956-
}
4957-
4958-
if err != nil {
4959-
t.Fatalf("Unexpected error: %v", err)
4960-
}
4935+
limits := c.getVolumeLimits(ctx, tc.volumeType, tc.azParams)
49614936

49624937
if limits.maxIops != tc.expectedLimits.maxIops {
49634938
t.Errorf("Expected maxIops %d, got %d", tc.expectedLimits.maxIops, limits.maxIops)
@@ -4970,7 +4945,7 @@ func TestGetVolumeLimits(t *testing.T) {
49704945
}
49714946

49724947
// Verify cache is updated for non-cached scenarios
4973-
if tc.cachedLimits == nil && !tc.expectError {
4948+
if tc.cachedLimits == nil {
49744949
cacheKey := fmt.Sprintf("%s|%s|%s|%s", tc.volumeType, tc.azParams.availabilityZone, tc.azParams.availabilityZoneId, tc.azParams.outpostArn)
49754950

49764951
cachedValue, ok := c.latestIOPSLimits.Get(cacheKey)

‎tests/e2e/requires_aws_api.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -412,11 +412,11 @@ var _ = Describe("[ebs-csi-e2e] [single-az] [requires-aws-api] Dynamic Provision
412412
{
413413
CreateVolumeParameters: map[string]string{
414414
ebscsidriver.EncryptedKey: "true",
415-
ebscsidriver.VolumeTypeKey: awscloud.VolumeTypeGP2,
415+
ebscsidriver.VolumeTypeKey: awscloud.VolumeTypeGP3,
416416
ebscsidriver.IopsKey: sourceIops,
417417
ebscsidriver.FSTypeKey: ebscsidriver.FSTypeExt4,
418418
},
419-
ClaimSize: driver.MinimumSizeForVolumeType(awscloud.VolumeTypeGP3),
419+
ClaimSize: "10Gi", // Must be higher than minimum to be within max IOPS ratio.
420420
VolumeMount: testsuites.DefaultGeneratedVolumeMount,
421421
},
422422
},
@@ -428,7 +428,7 @@ var _ = Describe("[ebs-csi-e2e] [single-az] [requires-aws-api] Dynamic Provision
428428
CreateVolumeParameters: map[string]string{
429429
ebscsidriver.VolumeTypeKey: awscloud.VolumeTypeGP3,
430430
},
431-
ClaimSize: driver.MinimumSizeForVolumeType(awscloud.VolumeTypeGP3),
431+
ClaimSize: "10Gi",
432432
VolumeMount: testsuites.DefaultGeneratedVolumeMount,
433433
},
434434
},

0 commit comments

Comments
 (0)