From 0ecc0e97a49a3128edd35e295ba459736e251c3b Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Thu, 25 Mar 2021 15:19:28 +0100 Subject: [PATCH] Add increaseTooLowIOPS parameter --- docs/README.md | 3 +- pkg/cloud/cloud.go | 42 +++++++++----- pkg/cloud/cloud_test.go | 121 +++++++++++++++++++++++++++------------ pkg/driver/constants.go | 3 + pkg/driver/controller.go | 40 +++++++------ 5 files changed, 137 insertions(+), 72 deletions(-) diff --git a/docs/README.md b/docs/README.md index 724e9c593e..ccd2e4bc93 100644 --- a/docs/README.md +++ b/docs/README.md @@ -35,7 +35,8 @@ There are several optional parameters that could be passed into `CreateVolumeReq |-----------------------------|----------------------------------------|----------|---------------------| | "csi.storage.k8s.io/fsType" | xfs, ext2, ext3, ext4 | ext4 | File system type that will be formatted during volume creation | | "type" | io1, io2, gp2, gp3, sc1, st1,standard | gp3* | EBS volume type | -| "iopsPerGB" | | | I/O operations per second per GiB. Required when io1 or io2 volume type is specified. If this value multiplied by the size of a requested volume produces a value below the minimum or above the maximum IOPs allowed for the volume type, as documented [here](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volume-types.html), the driver will increase or reduce the volume IOPS to fit into the AWS IOPS limits. | +| "iopsPerGB" | | | I/O operations per second per GiB. Required when io1 or io2 volume type is specified. If this value multiplied by the size of a requested volume produces a value above the maximum IOPs allowed for the volume type, as documented [here](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volume-types.html), AWS will cap the IOPS to maximum supported value. If the value is lower than minimal supported IOPS value per volume, either error is returned (the default behavior) or the value is increased to fit into the supported range when `allowautoiopspergbincrease` is `"true"`.| +| "allowAutoIOPSPerGBIncrease"| true, false | false | When `"true"`, the CSI driver increases IOPS for a volume when `iopsPerGB * ` is too low to fit into IOPS range supported by AWS. This allows dynamic provisioning to always succeed, even when user specifies too small PVC capacity or `iopsPerGB` value. On the other hand, it may introduce additional costs, as such volumes have higher IOPS than requested in `iopsPerGB`.| | "iops" | | 3000 | I/O operations per second. Only effetive when gp3 volume type is specified. If empty, it will set to 3000 as documented [here](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volume-types.html). | | "throughput" | | 125 | Throughput in MiB/s. Only effective when gp3 volume type is specified. If empty, it will set to 125MiB/s as documented [here](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-volume-types.html). | | "encrypted" | | | Whether the volume should be encrypted or not. Valid values are "true" or "false" | diff --git a/pkg/cloud/cloud.go b/pkg/cloud/cloud.go index fdae19a21f..8835b9121a 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -153,15 +153,16 @@ type Disk struct { // DiskOptions represents parameters to create an EBS volume type DiskOptions struct { - CapacityBytes int64 - Tags map[string]string - VolumeType string - IOPSPerGB int - IOPS int - Throughput int - AvailabilityZone string - OutpostArn string - Encrypted bool + CapacityBytes int64 + Tags map[string]string + VolumeType string + IOPSPerGB int + AllowIOPSPerGBIncrease bool + IOPS int + Throughput int + AvailabilityZone string + OutpostArn string + Encrypted bool // KmsKeyID represents a fully qualified resource name to the key to use for encryption. // example: arn:aws:kms:us-east-1:012345678910:key/abcd1234-a123-456a-a12b-a123b4cd56ef KmsKeyID string @@ -267,6 +268,7 @@ func (c *cloud) CreateDisk(ctx context.Context, volumeName string, diskOptions * createType string iops int64 throughput int64 + err error ) capacityGiB := util.BytesToGiB(diskOptions.CapacityBytes) @@ -275,10 +277,16 @@ func (c *cloud) CreateDisk(ctx context.Context, volumeName string, diskOptions * createType = diskOptions.VolumeType case VolumeTypeIO1: createType = diskOptions.VolumeType - iops = capIOPS(diskOptions.VolumeType, capacityGiB, int64(diskOptions.IOPSPerGB), io1MinTotalIOPS, io1MaxTotalIOPS, io1MaxIOPSPerGB) + iops, err = capIOPS(diskOptions.VolumeType, capacityGiB, int64(diskOptions.IOPSPerGB), io1MinTotalIOPS, io1MaxTotalIOPS, io1MaxIOPSPerGB, diskOptions.AllowIOPSPerGBIncrease) + if err != nil { + return nil, err + } case VolumeTypeIO2: createType = diskOptions.VolumeType - iops = capIOPS(diskOptions.VolumeType, capacityGiB, int64(diskOptions.IOPSPerGB), io2MinTotalIOPS, io2MaxTotalIOPS, io2MaxIOPSPerGB) + iops, err = capIOPS(diskOptions.VolumeType, capacityGiB, int64(diskOptions.IOPSPerGB), io2MinTotalIOPS, io2MaxTotalIOPS, io2MaxIOPSPerGB, diskOptions.AllowIOPSPerGBIncrease) + if err != nil { + return nil, err + } case VolumeTypeGP3: createType = diskOptions.VolumeType iops = int64(diskOptions.IOPS) @@ -1092,12 +1100,16 @@ func getVolumeAttachmentsList(volume *ec2.Volume) []string { // Using requstedIOPSPerGB allows users to create a "fast" storage class // (requstedIOPSPerGB = 50 for io1), which can provide the maximum iops // that AWS supports for any requestedCapacityGiB. -func capIOPS(volumeType string, requestedCapacityGiB int64, requstedIOPSPerGB, minTotalIOPS, maxTotalIOPS, maxIOPSPerGB int64) int64 { +func capIOPS(volumeType string, requestedCapacityGiB int64, requstedIOPSPerGB, minTotalIOPS, maxTotalIOPS, maxIOPSPerGB int64, allowIncrease bool) (int64, error) { iops := requestedCapacityGiB * requstedIOPSPerGB if iops < minTotalIOPS { - iops = minTotalIOPS - klog.V(5).Infof("Increased IOPS for %s %d GB volume to the min supported limit: %d", volumeType, requestedCapacityGiB, iops) + if allowIncrease { + iops = minTotalIOPS + klog.V(5).Infof("Increased IOPS for %s %d GB volume to the min supported limit: %d", volumeType, requestedCapacityGiB, iops) + } else { + return 0, fmt.Errorf("invalid combination of volume size %d GB and iopsPerGB %d: the resulting IOPS %d is too low for AWS, it must be at least %d", requestedCapacityGiB, requstedIOPSPerGB, iops, minTotalIOPS) + } } if iops > maxTotalIOPS { iops = maxTotalIOPS @@ -1107,5 +1119,5 @@ func capIOPS(volumeType string, requestedCapacityGiB int64, requstedIOPSPerGB, m iops = maxIOPSPerGB * requestedCapacityGiB klog.V(5).Infof("Capped IOPS for %s %d GB volume at %d IOPS/GB: %d", volumeType, requestedCapacityGiB, maxIOPSPerGB, iops) } - return iops + return iops, nil } diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index e9935cde16..28550d1d60 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -59,6 +59,7 @@ func TestCreateDisk(t *testing.T) { CapacityBytes: util.GiBToBytes(1), Tags: map[string]string{VolumeNameTagKey: "vol-test"}, }, + expCreateVolumeInput: &ec2.CreateVolumeInput{}, expDisk: &Disk{ VolumeID: "vol-test", CapacityGiB: 1, @@ -118,7 +119,8 @@ func TestCreateDisk(t *testing.T) { CapacityGiB: 1, AvailabilityZone: expZone, }, - expErr: nil, + expCreateVolumeInput: &ec2.CreateVolumeInput{}, + expErr: nil, }, { name: "success: normal with encrypted volume", @@ -135,7 +137,8 @@ func TestCreateDisk(t *testing.T) { CapacityGiB: 1, AvailabilityZone: expZone, }, - expErr: nil, + expCreateVolumeInput: &ec2.CreateVolumeInput{}, + expErr: nil, }, { name: "success: outpost volume", @@ -152,7 +155,8 @@ func TestCreateDisk(t *testing.T) { AvailabilityZone: expZone, OutpostArn: "arn:aws:outposts:us-west-2:111111111111:outpost/op-0aaa000a0aaaa00a0", }, - expErr: nil, + expCreateVolumeInput: &ec2.CreateVolumeInput{}, + expErr: nil, }, { name: "success: empty outpost arn", @@ -168,7 +172,8 @@ func TestCreateDisk(t *testing.T) { AvailabilityZone: expZone, OutpostArn: "", }, - expErr: nil, + expCreateVolumeInput: &ec2.CreateVolumeInput{}, + expErr: nil, }, { name: "fail: ec2.CreateVolume returned CreateVolume error", @@ -178,8 +183,9 @@ func TestCreateDisk(t *testing.T) { Tags: map[string]string{VolumeNameTagKey: "vol-test"}, AvailabilityZone: expZone, }, - expErr: fmt.Errorf("could not create volume in EC2: CreateVolume generic error"), - expCreateVolumeErr: fmt.Errorf("CreateVolume generic error"), + expCreateVolumeInput: &ec2.CreateVolumeInput{}, + expErr: fmt.Errorf("could not create volume in EC2: CreateVolume generic error"), + expCreateVolumeErr: fmt.Errorf("CreateVolume generic error"), }, { name: "fail: ec2.DescribeVolumes error after volume created", @@ -190,9 +196,10 @@ func TestCreateDisk(t *testing.T) { Tags: map[string]string{VolumeNameTagKey: "vol-test"}, AvailabilityZone: "", }, - expErr: fmt.Errorf("failed to get an available volume in EC2: DescribeVolumes generic error"), - expDescVolumeErr: fmt.Errorf("DescribeVolumes generic error"), - cleanUpFailedVolume: true, + expCreateVolumeInput: &ec2.CreateVolumeInput{}, + expErr: fmt.Errorf("failed to get an available volume in EC2: DescribeVolumes generic error"), + expDescVolumeErr: fmt.Errorf("DescribeVolumes generic error"), + cleanUpFailedVolume: true, }, { name: "fail: Volume is not ready to use, volume stuck in creating status and controller timed out waiting for the condition", @@ -203,8 +210,9 @@ func TestCreateDisk(t *testing.T) { Tags: map[string]string{VolumeNameTagKey: "vol-test"}, AvailabilityZone: "", }, - cleanUpFailedVolume: true, - expErr: fmt.Errorf("failed to get an available volume in EC2: timed out waiting for the condition"), + cleanUpFailedVolume: true, + expCreateVolumeInput: &ec2.CreateVolumeInput{}, + expErr: fmt.Errorf("failed to get an available volume in EC2: timed out waiting for the condition"), }, { name: "success: normal from snapshot", @@ -220,10 +228,31 @@ func TestCreateDisk(t *testing.T) { CapacityGiB: 1, AvailabilityZone: expZone, }, + expCreateVolumeInput: &ec2.CreateVolumeInput{}, + expErr: nil, + }, + { + name: "success: io1 with too low iopsPerGB and AllowIOPSPerGBIncrease", + volumeName: "vol-test-name", + diskOptions: &DiskOptions{ + CapacityBytes: util.GiBToBytes(4), + Tags: map[string]string{VolumeNameTagKey: "vol-test"}, + VolumeType: VolumeTypeIO1, + IOPSPerGB: 1, + AllowIOPSPerGBIncrease: true, + }, + expDisk: &Disk{ + VolumeID: "vol-test", + CapacityGiB: 4, + AvailabilityZone: defaultZone, + }, + expCreateVolumeInput: &ec2.CreateVolumeInput{ + Iops: aws.Int64(100), + }, expErr: nil, }, { - name: "success: io1 with too low iopsPerGB", + name: "fail: io1 with too low iopsPerGB", volumeName: "vol-test-name", diskOptions: &DiskOptions{ CapacityBytes: util.GiBToBytes(4), @@ -236,10 +265,8 @@ func TestCreateDisk(t *testing.T) { CapacityGiB: 4, AvailabilityZone: defaultZone, }, - expCreateVolumeInput: &ec2.CreateVolumeInput{ - Iops: aws.Int64(100), - }, - expErr: nil, + expCreateVolumeInput: nil, + expErr: fmt.Errorf("invalid combination of volume size 4 GB and iopsPerGB 1: the resulting IOPS 4 is too low for AWS, it must be at least 100"), }, { name: "success: small io1 with too high iopsPerGB", @@ -280,13 +307,14 @@ func TestCreateDisk(t *testing.T) { expErr: nil, }, { - name: "success: io2 with too low iopsPerGB", + name: "success: io2 with too low iopsPerGB and AllowIOPSPerGBIncrease", volumeName: "vol-test-name", diskOptions: &DiskOptions{ - CapacityBytes: util.GiBToBytes(4), - Tags: map[string]string{VolumeNameTagKey: "vol-test"}, - VolumeType: VolumeTypeIO2, - IOPSPerGB: 1, + CapacityBytes: util.GiBToBytes(4), + Tags: map[string]string{VolumeNameTagKey: "vol-test"}, + VolumeType: VolumeTypeIO2, + IOPSPerGB: 1, + AllowIOPSPerGBIncrease: true, }, expDisk: &Disk{ VolumeID: "vol-test", @@ -298,6 +326,23 @@ func TestCreateDisk(t *testing.T) { }, expErr: nil, }, + { + name: "fail: io2 with too low iopsPerGB", + volumeName: "vol-test-name", + diskOptions: &DiskOptions{ + CapacityBytes: util.GiBToBytes(4), + Tags: map[string]string{VolumeNameTagKey: "vol-test"}, + VolumeType: VolumeTypeIO2, + IOPSPerGB: 1, + }, + expDisk: &Disk{ + VolumeID: "vol-test", + CapacityGiB: 4, + AvailabilityZone: defaultZone, + }, + expCreateVolumeInput: nil, + expErr: fmt.Errorf("invalid combination of volume size 4 GB and iopsPerGB 1: the resulting IOPS 4 is too low for AWS, it must be at least 100"), + }, { name: "success: small io2 with too high iopsPerGB", volumeName: "vol-test-name", @@ -362,24 +407,24 @@ func TestCreateDisk(t *testing.T) { State: aws.String("completed"), } ctx := context.Background() - matcher := gomock.Any() + if tc.expCreateVolumeInput != nil { - matcher = eqCreateVolume(tc.expCreateVolumeInput) - } - mockEC2.EXPECT().CreateVolumeWithContext(gomock.Eq(ctx), matcher).Return(vol, tc.expCreateVolumeErr) - mockEC2.EXPECT().DescribeVolumesWithContext(gomock.Eq(ctx), gomock.Any()).Return(&ec2.DescribeVolumesOutput{Volumes: []*ec2.Volume{vol}}, tc.expDescVolumeErr).AnyTimes() - if len(tc.diskOptions.SnapshotID) > 0 { - mockEC2.EXPECT().DescribeSnapshotsWithContext(gomock.Eq(ctx), gomock.Any()).Return(&ec2.DescribeSnapshotsOutput{Snapshots: []*ec2.Snapshot{snapshot}}, nil).AnyTimes() - } - if tc.cleanUpFailedVolume == true { - mockEC2.EXPECT().DeleteVolumeWithContext(gomock.Eq(ctx), gomock.Any()).Return(&ec2.DeleteVolumeOutput{}, nil) - } - if len(tc.diskOptions.AvailabilityZone) == 0 { - mockEC2.EXPECT().DescribeAvailabilityZonesWithContext(gomock.Eq(ctx), gomock.Any()).Return(&ec2.DescribeAvailabilityZonesOutput{ - AvailabilityZones: []*ec2.AvailabilityZone{ - {ZoneName: aws.String(defaultZone)}, - }, - }, nil) + matcher := eqCreateVolume(tc.expCreateVolumeInput) + mockEC2.EXPECT().CreateVolumeWithContext(gomock.Eq(ctx), matcher).Return(vol, tc.expCreateVolumeErr) + mockEC2.EXPECT().DescribeVolumesWithContext(gomock.Eq(ctx), gomock.Any()).Return(&ec2.DescribeVolumesOutput{Volumes: []*ec2.Volume{vol}}, tc.expDescVolumeErr).AnyTimes() + if len(tc.diskOptions.SnapshotID) > 0 { + mockEC2.EXPECT().DescribeSnapshotsWithContext(gomock.Eq(ctx), gomock.Any()).Return(&ec2.DescribeSnapshotsOutput{Snapshots: []*ec2.Snapshot{snapshot}}, nil).AnyTimes() + } + if tc.cleanUpFailedVolume == true { + mockEC2.EXPECT().DeleteVolumeWithContext(gomock.Eq(ctx), gomock.Any()).Return(&ec2.DeleteVolumeOutput{}, nil) + } + if len(tc.diskOptions.AvailabilityZone) == 0 { + mockEC2.EXPECT().DescribeAvailabilityZonesWithContext(gomock.Eq(ctx), gomock.Any()).Return(&ec2.DescribeAvailabilityZonesOutput{ + AvailabilityZones: []*ec2.AvailabilityZone{ + {ZoneName: aws.String(defaultZone)}, + }, + }, nil) + } } disk, err := c.CreateDisk(ctx, tc.volumeName, tc.diskOptions) diff --git a/pkg/driver/constants.go b/pkg/driver/constants.go index 67a65f44eb..29e43d1db2 100644 --- a/pkg/driver/constants.go +++ b/pkg/driver/constants.go @@ -31,6 +31,9 @@ const ( // IopsPerGBKey represents key for IOPS per GB IopsPerGBKey = "iopspergb" + // AllowAutoIOPSPerGBIncreaseKey represents key for allowing automatic increase of IOPS + AllowAutoIOPSPerGBIncreaseKey = "allowautoiopspergbincrease" + // Iops represents key for IOPS for volume IopsKey = "iops" diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 45d3b5c7de..b5653576f6 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -132,13 +132,14 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol } var ( - volumeType string - iopsPerGB int - iops int - throughput int - isEncrypted bool - kmsKeyID string - volumeTags = map[string]string{ + volumeType string + iopsPerGB int + allowIOPSPerGBIncrease bool + iops int + throughput int + isEncrypted bool + kmsKeyID string + volumeTags = map[string]string{ cloud.VolumeNameTagKey: volName, } ) @@ -154,6 +155,8 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol if err != nil { return nil, status.Errorf(codes.InvalidArgument, "Could not parse invalid iopsPerGB: %v", err) } + case AllowAutoIOPSPerGBIncreaseKey: + allowIOPSPerGBIncrease = value == "true" case IopsKey: iops, err = strconv.Atoi(value) if err != nil { @@ -232,17 +235,18 @@ func (d *controllerService) CreateVolume(ctx context.Context, req *csi.CreateVol } opts := &cloud.DiskOptions{ - CapacityBytes: volSizeBytes, - Tags: volumeTags, - VolumeType: volumeType, - IOPSPerGB: iopsPerGB, - IOPS: iops, - Throughput: throughput, - AvailabilityZone: zone, - OutpostArn: outpostArn, - Encrypted: isEncrypted, - KmsKeyID: kmsKeyID, - SnapshotID: snapshotID, + CapacityBytes: volSizeBytes, + Tags: volumeTags, + VolumeType: volumeType, + IOPSPerGB: iopsPerGB, + AllowIOPSPerGBIncrease: allowIOPSPerGBIncrease, + IOPS: iops, + Throughput: throughput, + AvailabilityZone: zone, + OutpostArn: outpostArn, + Encrypted: isEncrypted, + KmsKeyID: kmsKeyID, + SnapshotID: snapshotID, } disk, err = d.cloud.CreateDisk(ctx, volName, opts)