diff --git a/docs/parameters.md b/docs/parameters.md index be9ff0d2c0..466ffc65da 100644 --- a/docs/parameters.md +++ b/docs/parameters.md @@ -9,8 +9,9 @@ The AWS EBS CSI Driver supports [tagging](tagging.md) through `StorageClass.para |------------------------------|-------------------------------------------------|---------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | "csi.storage.k8s.io/fstype" | xfs, ext3, ext4 | ext4 | File system type that will be formatted during volume creation. This parameter is case sensitive! | | "type" | io1, io2, gp2, gp3, sc1, st1, standard | gp3* | EBS volume type. | -| "iopsPerGB" | | | I/O operations per second per GiB. Can be specified for IO1, IO2, and GP3 volumes. | +| "iopsPerGB" | | | I/O operations per second per GiB. Can be specified for IO1, IO2, and GP3. If `iopsPerGB * ` exceeds volume limits, the IOPS will be capped at the maximum allowed for that volume type and the call will succeed. | | "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`. | +| "allowAutoIOPSIncreaseOnModify" | true, false | false | When `"true"`, the CSI Driver adjusts IOPS for a volume during resizing if `iopsPerGB` is set. This ensures that the volume maintains the desired IOPS/GiB ratio. | "iops" | | | I/O operations per second. Can be specified for IO1, IO2, and GP3 volumes. | | "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" | true, false | false | 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 d1b2eca836..4b69c5c601 100644 --- a/pkg/cloud/cloud.go +++ b/pkg/cloud/cloud.go @@ -123,6 +123,10 @@ const ( KubernetesTagKeyPrefix = "kubernetes.io" // AwsEbsDriverTagKey is the tag to identify if a volume/snapshot is managed by ebs csi driver. AwsEbsDriverTagKey = util.DriverName + "/cluster" + // AllowAutoIOPSIncreaseOnModifyKey is the tag key for allowing IOPS increase on resizing if IOPSPerGB is set to ensure desired ratio is maintained. + AllowAutoIOPSIncreaseOnModifyKey = util.DriverName + "/AllowAutoIOPSIncreaseOnModify" + // IOPSPerGBKey represents the tag key for IOPS per GB. + IOPSPerGBKey = util.DriverName + "/IOPSPerGb" ) // Batcher. @@ -250,9 +254,11 @@ type DiskOptions struct { // ModifyDiskOptions represents parameters to modify an EBS volume. type ModifyDiskOptions struct { - VolumeType string - IOPS int32 - Throughput int32 + VolumeType string + IOPS int32 + Throughput int32 + IOPSPerGB int32 + AllowIopsIncreaseOnResize bool } // iopsLimits represents the IOPS limits set by EBS of a volume dependent on the volume type. @@ -905,27 +911,89 @@ func (c *cloud) ResizeOrModifyDisk(ctx context.Context, volumeID string, newSize if err != nil { return 0, err } - needsModification, volumeSize, err := c.validateModifyVolume(ctx, volumeID, newSizeGiB, options) + request := &ec2.DescribeVolumesInput{ + VolumeIds: []string{volumeID}, + } + volume, err := c.getVolume(ctx, request) + if err != nil { + return 0, err + } + needsModification, volumeSize, err := c.validateVolumeState(ctx, volumeID, newSizeGiB, *volume.Size, options) if err != nil || !needsModification { return volumeSize, err } + if options.IOPS > 0 && options.IOPSPerGB > 0 { + return 0, errors.New("invalid VAC parameter; specify either IOPS or IOPSPerGb, not both") + } + req := &ec2.ModifyVolumeInput{ VolumeId: aws.String(volumeID), } if newSizeBytes != 0 { req.Size = aws.Int32(newSizeGiB) } - if options.IOPS != 0 { - req.Iops = aws.Int32(options.IOPS) - } + volTypeToUse := volume.VolumeType if options.VolumeType != "" { req.VolumeType = types.VolumeType(options.VolumeType) + volTypeToUse = req.VolumeType } if options.Throughput != 0 { req.Throughput = aws.Int32(options.Throughput) } + + var sizeToUse int32 + if req.Size != nil { + sizeToUse = *req.Size + } else { + sizeToUse = *volume.Size + } + + allowAutoIncreaseIsSet, iopsPerGbVal, err := c.checkIfIopsIncreaseOnExpansion(volume.Tags) + if err != nil { + return 0, err + } + var iopsForModify int32 + switch { + case options.IOPS != 0: + iopsForModify = options.IOPS + case options.IOPSPerGB != 0 && (options.AllowIopsIncreaseOnResize || allowAutoIncreaseIsSet): + iopsForModify = sizeToUse * options.IOPSPerGB + case iopsPerGbVal > 0 && (options.AllowIopsIncreaseOnResize || allowAutoIncreaseIsSet): + iopsForModify = sizeToUse * iopsPerGbVal + } + if iopsForModify != 0 { + azParams := getVolumeLimitsParams{} + + if volume.AvailabilityZone != nil { + azParams.availabilityZone = *volume.AvailabilityZone + } + if volume.AvailabilityZoneId != nil { + azParams.availabilityZoneId = *volume.AvailabilityZoneId + } + // EBS doesn't handle empty outpost arn, so we have to include it only when it's non-empty + if volume.OutpostArn != nil { + azParams.outpostArn = *volume.OutpostArn + } + iopsLimit, err := c.getVolumeLimits(ctx, string(volTypeToUse), azParams) + if err != nil { + return 0, err + } + // Even if volume type does not support IOPS, still pass value and let EC2 handle error. + if iopsLimit.maxIops == 0 { + req.Iops = aws.Int32(iopsForModify) + } else { + req.Iops = aws.Int32(capIOPS(string(volTypeToUse), sizeToUse, iopsForModify, iopsLimit, true)) + } + options.IOPS = *req.Iops + } + + needsModification, volumeSize, err = c.validateModifyVolume(ctx, volumeID, newSizeGiB, options, *volume) + if err != nil || !needsModification { + return volumeSize, err + } + response, err := c.ec2.ModifyVolume(ctx, req, func(o *ec2.Options) { o.Retryer = c.rm.modifyVolumeRetryer }) @@ -2404,7 +2472,7 @@ func (c *cloud) AvailabilityZones(ctx context.Context) (map[string]struct{}, err return zones, nil } -func needsVolumeModification(volume types.Volume, newSizeGiB int32, options *ModifyDiskOptions) bool { +func needsVolumeModification(volume types.Volume, newSizeGiB int32, req *ModifyDiskOptions) bool { oldSizeGiB := *volume.Size //nolint:staticcheck // staticcheck suggests merging all of the below conditionals into one line, // but that would be extremely difficult to read @@ -2414,66 +2482,94 @@ func needsVolumeModification(volume types.Volume, newSizeGiB int32, options *Mod needsModification = true } - if options.IOPS != 0 && (volume.Iops == nil || *volume.Iops != options.IOPS) { + if req.IOPS != 0 && (volume.Iops == nil || *volume.Iops != req.IOPS) { needsModification = true } - if options.VolumeType != "" && !strings.EqualFold(string(volume.VolumeType), options.VolumeType) { + if req.VolumeType != "" && !strings.EqualFold(string(volume.VolumeType), req.VolumeType) { needsModification = true } - if options.Throughput != 0 && (volume.Throughput == nil || *volume.Throughput != options.Throughput) { + if req.Throughput != 0 && (volume.Throughput == nil || *volume.Throughput != req.Throughput) { needsModification = true } return needsModification } -func (c *cloud) validateModifyVolume(ctx context.Context, volumeID string, newSizeGiB int32, options *ModifyDiskOptions) (bool, int32, error) { - request := &ec2.DescribeVolumesInput{ - VolumeIds: []string{volumeID}, +func getVolumeAttachmentsList(volume types.Volume) []string { + var volumeAttachmentList []string + for _, attachment := range volume.Attachments { + if attachment.State == types.VolumeAttachmentStateAttached { + volumeAttachmentList = append(volumeAttachmentList, aws.ToString(attachment.InstanceId)) + } } - volume, err := c.getVolume(ctx, request) - if err != nil { - return true, 0, err - } + return volumeAttachmentList +} - if volume.Size == nil { - return true, 0, fmt.Errorf("volume %q has no size", volumeID) +// Checks if a volume's IOPS can be increased on expansion to adhere to IopsPerGB ratio. +func (c *cloud) checkIfIopsIncreaseOnExpansion(existingTags []types.Tag) (allowAutoIncreaseIsSet bool, iopsPerGbVal int32, err error) { + for _, tag := range existingTags { + switch *tag.Key { + case IOPSPerGBKey: + iopsPerGbVal64, err := strconv.ParseInt(*tag.Value, 10, 32) + if err != nil { + return false, 0, fmt.Errorf("%w: %w", ErrInvalidArgument, err) + } + iopsPerGbVal = int32(iopsPerGbVal64) + case AllowAutoIOPSIncreaseOnModifyKey: + allowAutoIncreaseIsSet, err = strconv.ParseBool(*tag.Value) + if err != nil { + return false, 0, fmt.Errorf("%w: %w", ErrInvalidArgument, err) + } + } } - oldSizeGiB := *volume.Size + return allowAutoIncreaseIsSet, iopsPerGbVal, nil +} - // This call must NOT be batched because a missing volume modification will return client error +func (c *cloud) getVolumeModificationState(ctx context.Context, volumeID string) (*types.VolumeModification, error) { latestMod, err := c.getLatestVolumeModification(ctx, volumeID, false) if err != nil && !errors.Is(err, ErrVolumeNotBeingModified) { - return true, oldSizeGiB, fmt.Errorf("error fetching volume modifications for %q: %w", volumeID, err) + return nil, fmt.Errorf("error fetching volume modifications for %q: %w", volumeID, err) + } + return latestMod, nil +} + +func (c *cloud) validateVolumeState(ctx context.Context, volumeID string, newSizeGiB int32, oldSizeGiB int32, options *ModifyDiskOptions) (bool, int32, error) { + latestMod, err := c.getVolumeModificationState(ctx, volumeID) + if err != nil { + return false, 0, err } - state := "" // latestMod can be nil if the volume has never been modified - if latestMod != nil { - state = string(latestMod.ModificationState) - if state == string(types.VolumeModificationStateModifying) { - // If volume is already modifying, detour to waiting for it to modify - klog.V(5).InfoS("[Debug] Watching ongoing modification", "volumeID", volumeID) - err = c.waitForVolumeModification(ctx, volumeID) - if err != nil { - return true, oldSizeGiB, err - } - returnGiB, returnErr := c.checkDesiredState(ctx, volumeID, newSizeGiB, options) - return false, returnGiB, returnErr + if latestMod != nil && string(latestMod.ModificationState) == string(types.VolumeModificationStateModifying) { + // If volume is already modifying, detour to waiting for it to modify + klog.V(5).InfoS("[Debug] Watching ongoing modification", "volumeID", volumeID) + err = c.waitForVolumeModification(ctx, volumeID) + if err != nil { + return false, oldSizeGiB, err } + returnGiB, returnErr := c.checkDesiredState(ctx, volumeID, newSizeGiB, options) + return false, returnGiB, returnErr } + return true, 0, nil +} + +func (c *cloud) validateModifyVolume(ctx context.Context, volumeID string, newSizeGiB int32, options *ModifyDiskOptions, volume types.Volume) (bool, int32, error) { + if volume.Size == nil { + return true, 0, fmt.Errorf("volume %q has no size", volumeID) + } + oldSizeGiB := *volume.Size // At this point, we know we are starting a new volume modification // If we're asked to modify a volume to its current state, ignore the request and immediately return a success // This is because as of March 2024, EC2 ModifyVolume calls that don't change any parameters still modify the volume - if !needsVolumeModification(*volume, newSizeGiB, options) { + if !needsVolumeModification(volume, newSizeGiB, options) { klog.V(5).InfoS("[Debug] Skipping modification for volume due to matching stats", "volumeID", volumeID) // Wait for any existing modifications to prevent race conditions where DescribeVolume(s) returns the new // state before the volume is actually finished modifying - err = c.waitForVolumeModification(ctx, volumeID) + err := c.waitForVolumeModification(ctx, volumeID) if err != nil { return true, oldSizeGiB, err } @@ -2481,7 +2577,12 @@ func (c *cloud) validateModifyVolume(ctx context.Context, volumeID string, newSi return false, returnGiB, returnErr } - if state == string(types.VolumeModificationStateOptimizing) { + latestMod, err := c.getVolumeModificationState(ctx, volumeID) + if err != nil { + return true, 0, err + } + + if latestMod != nil && string(latestMod.ModificationState) == string(types.VolumeModificationStateOptimizing) { return true, 0, fmt.Errorf("volume %q in OPTIMIZING state, cannot currently modify", volumeID) } @@ -2492,17 +2593,6 @@ func volumeModificationDone(state string) bool { return state == string(types.VolumeModificationStateCompleted) || state == string(types.VolumeModificationStateOptimizing) } -func getVolumeAttachmentsList(volume types.Volume) []string { - var volumeAttachmentList []string - for _, attachment := range volume.Attachments { - if attachment.State == types.VolumeAttachmentStateAttached { - volumeAttachmentList = append(volumeAttachmentList, aws.ToString(attachment.InstanceId)) - } - } - - return volumeAttachmentList -} - // Calculate actual IOPS for a volume and cap it at supported AWS limits. func capIOPS(volumeType string, requestedCapacityGiB int32, requestedIops int32, iopsLimits iopsLimits, allowIncrease bool) int32 { // If requestedIops is zero the user did not request a specific amount, and the default will be used instead diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index ea316528b1..0c5bd0446d 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -3338,6 +3338,7 @@ func TestResizeOrModifyDisk(t *testing.T) { VolumeId: aws.String("vol-test"), Size: aws.Int32(1), AvailabilityZone: aws.String(defaultZone), + VolumeType: types.VolumeTypeGp3, }, modifiedVolume: &ec2.ModifyVolumeOutput{ VolumeModification: &types.VolumeModification{ @@ -3358,6 +3359,7 @@ func TestResizeOrModifyDisk(t *testing.T) { VolumeId: aws.String("vol-test"), Size: aws.Int32(1), AvailabilityZone: aws.String(defaultZone), + VolumeType: types.VolumeTypeGp3, }, modifiedVolume: &ec2.ModifyVolumeOutput{ VolumeModification: &types.VolumeModification{ @@ -3387,6 +3389,7 @@ func TestResizeOrModifyDisk(t *testing.T) { VolumeId: aws.String("vol-test"), Size: aws.Int32(2), AvailabilityZone: aws.String(defaultZone), + VolumeType: types.VolumeTypeGp3, }, descModVolume: &ec2.DescribeVolumesModificationsOutput{ VolumesModifications: []types.VolumeModification{ @@ -3472,6 +3475,7 @@ func TestResizeOrModifyDisk(t *testing.T) { VolumeId: aws.String("vol-test"), Size: aws.Int32(1), AvailabilityZone: aws.String(defaultZone), + VolumeType: types.VolumeTypeGp3, }, descModVolume: &ec2.DescribeVolumesModificationsOutput{ VolumesModifications: []types.VolumeModification{ @@ -3496,6 +3500,7 @@ func TestResizeOrModifyDisk(t *testing.T) { VolumeId: aws.String("vol-test"), AvailabilityZone: aws.String(defaultZone), VolumeType: types.VolumeTypeGp2, + Size: aws.Int32(1), }, modifiedVolumeError: errors.New("GenericErr"), expErr: errors.New("GenericErr"), @@ -3572,6 +3577,7 @@ func TestResizeOrModifyDisk(t *testing.T) { AvailabilityZone: aws.String(defaultZone), Size: aws.Int32(13), Iops: aws.Int32(3000), + VolumeType: types.VolumeTypeGp3, }, reqSizeGiB: 13, modifyDiskOptions: &ModifyDiskOptions{ @@ -3620,6 +3626,12 @@ func TestResizeOrModifyDisk(t *testing.T) { }, tc.existingVolumeError) } } + + mockEC2.EXPECT().DescribeTags(gomock.Any(), gomock.Any()).Return(&ec2.DescribeTagsOutput{}, nil).AnyTimes() + + // Mock CreateVolume for getVolumeLimits dry-run calls + mockEC2.EXPECT().CreateVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, &smithy.GenericAPIError{Code: "DryRunOperation"}).AnyTimes() + if tc.modifiedVolume != nil || tc.modifiedVolumeError != nil { mockEC2.EXPECT().ModifyVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(tc.modifiedVolume, tc.modifiedVolumeError).AnyTimes() } @@ -5082,3 +5094,84 @@ func createDescribeVolumesOutput(volumeIDs []*string, nodeID, path, state string Volumes: volumes, } } + +func TestCheckIfIopsIncreaseOnExpansion(t *testing.T) { + testCases := []struct { + name string + tags []types.Tag + expectedAllowAutoIncrease bool + expectedIopsPerGb int32 + expectedError bool + }{ + { + name: "both tags present with valid values", + tags: []types.Tag{ + {Key: aws.String(IOPSPerGBKey), Value: aws.String("10")}, + {Key: aws.String(AllowAutoIOPSIncreaseOnModifyKey), Value: aws.String("true")}, + }, + expectedAllowAutoIncrease: true, + expectedIopsPerGb: 10, + expectedError: false, + }, + { + name: "only IOPSPerGB tag present", + tags: []types.Tag{ + {Key: aws.String(IOPSPerGBKey), Value: aws.String("5")}, + }, + expectedAllowAutoIncrease: false, + expectedIopsPerGb: 5, + expectedError: false, + }, + { + name: "only AllowAutoIOPSIncrease tag present", + tags: []types.Tag{ + {Key: aws.String(AllowAutoIOPSIncreaseOnModifyKey), Value: aws.String("false")}, + }, + expectedAllowAutoIncrease: false, + expectedIopsPerGb: 0, + expectedError: false, + }, + { + name: "no relevant tags", + tags: []types.Tag{}, + expectedAllowAutoIncrease: false, + expectedIopsPerGb: 0, + expectedError: false, + }, + { + name: "invalid IOPSPerGB value", + tags: []types.Tag{ + {Key: aws.String(IOPSPerGBKey), Value: aws.String("invalid")}, + }, + expectedAllowAutoIncrease: false, + expectedIopsPerGb: 0, + expectedError: true, + }, + { + name: "invalid AllowAutoIOPSIncrease value", + tags: []types.Tag{ + {Key: aws.String(AllowAutoIOPSIncreaseOnModifyKey), Value: aws.String("invalid")}, + }, + expectedAllowAutoIncrease: false, + expectedIopsPerGb: 0, + expectedError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + c := &cloud{} + + allowAutoIncrease, iopsPerGb, err := c.checkIfIopsIncreaseOnExpansion(tc.tags) + + if tc.expectedError { + require.Error(t, err) + require.ErrorIs(t, err, ErrInvalidArgument) + } else { + require.NoError(t, err) + assert.Equal(t, tc.expectedAllowAutoIncrease, allowAutoIncrease) + assert.Equal(t, tc.expectedIopsPerGb, iopsPerGb) + } + }) + } +} diff --git a/pkg/driver/constants.go b/pkg/driver/constants.go index d86ea9ef7c..8a9c61ba3c 100644 --- a/pkg/driver/constants.go +++ b/pkg/driver/constants.go @@ -47,6 +47,9 @@ const ( // AllowAutoIOPSPerGBIncreaseKey represents key for allowing automatic increase of IOPS. AllowAutoIOPSPerGBIncreaseKey = "allowautoiopspergbincrease" + // AllowAutoIOPSIncreaseOnModifyKey represents key for allowing IOPS increase on resizing if IopsPerGB is set to ensure desired ratio is maintained. + AllowAutoIOPSIncreaseOnModifyKey = "allowautoiopsincreaseonmodify" + // VolumeInitializationRateKey represents key for volume initialization rate when creating volumes from snapshots. VolumeInitializationRateKey = "volumeinitializationrate" diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 24a61ebd05..f0e8ddbc02 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -144,6 +144,9 @@ func (d *ControllerService) CreateVolume(ctx context.Context, req *csi.CreateVol return nil, status.Errorf(codes.InvalidArgument, "Could not parse invalid iopsPerGB: %v", parseIopsPerGBKeyErr) } iopsPerGB = int32(parseIopsPerGBKey) + volumeTags[cloud.IOPSPerGBKey] = strconv.Itoa(int(iopsPerGB)) + case AllowAutoIOPSIncreaseOnModifyKey: + volumeTags[cloud.AllowAutoIOPSIncreaseOnModifyKey] = strconv.FormatBool(isTrue(value)) case AllowAutoIOPSPerGBIncreaseKey: allowIOPSPerGBIncrease = isTrue(value) case IopsKey: diff --git a/pkg/driver/controller_modify_volume.go b/pkg/driver/controller_modify_volume.go index 5890e5b36c..d403765a95 100644 --- a/pkg/driver/controller_modify_volume.go +++ b/pkg/driver/controller_modify_volume.go @@ -146,7 +146,8 @@ func executeModifyVolumeRequest(c cloud.Cloud) func(string, modifyVolumeRequest) if err != nil { return 0, err } - if (req.modifyDiskOptions.IOPS != 0) || (req.modifyDiskOptions.Throughput != 0) || (req.modifyDiskOptions.VolumeType != "") || (req.newSize != 0) { + + if (req.modifyDiskOptions.IOPS != 0) || (req.modifyDiskOptions.Throughput != 0) || (req.modifyDiskOptions.VolumeType != "") || (req.newSize != 0) || (req.modifyDiskOptions.IOPSPerGB != 0) { actualSizeGiB, err := c.ResizeOrModifyDisk(ctx, volumeID, req.newSize, &req.modifyDiskOptions) if err != nil { switch { @@ -181,6 +182,7 @@ func parseModifyVolumeParameters(params map[string]string) (*modifyVolumeRequest }, } var rawTagsToAdd []string + var noValidationTags = make(map[string]string) tProps := new(template.PVProps) for key, value := range params { switch key { @@ -205,6 +207,16 @@ func parseModifyVolumeParameters(params map[string]string) (*modifyVolumeRequest options.modifyDiskOptions.VolumeType = value case ModificationKeyVolumeType: options.modifyDiskOptions.VolumeType = value + case IopsPerGBKey: + noValidationTags[cloud.IOPSPerGBKey] = value + iopsPerGb, err := strconv.ParseInt(value, 10, 32) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Could not parse IopsPerGb: %q", value) + } + options.modifyDiskOptions.IOPSPerGB = int32(iopsPerGb) + case AllowAutoIOPSIncreaseOnModifyKey: + noValidationTags[cloud.AllowAutoIOPSIncreaseOnModifyKey] = value + options.modifyDiskOptions.AllowIopsIncreaseOnResize = isTrue(value) case PVCNameKey: tProps.PVCName = value case PVCNamespaceKey: @@ -229,6 +241,9 @@ func parseModifyVolumeParameters(params map[string]string) (*modifyVolumeRequest if err := validateExtraTags(addTags, false); err != nil { return nil, status.Errorf(codes.InvalidArgument, "Invalid tag value: %v", err) } + for k, v := range noValidationTags { + addTags[k] = v + } options.modifyTagsOptions.TagsToAdd = addTags return &options, nil } diff --git a/pkg/driver/controller_modify_volume_test.go b/pkg/driver/controller_modify_volume_test.go index 6d2d930fc7..ccf1149493 100644 --- a/pkg/driver/controller_modify_volume_test.go +++ b/pkg/driver/controller_modify_volume_test.go @@ -151,9 +151,12 @@ func TestParseModifyVolumeParameters(t *testing.T) { { name: "basic params", params: map[string]string{ - ModificationKeyVolumeType: validType, - ModificationKeyIOPS: validIops, - ModificationKeyThroughput: validThroughput, + ModificationKeyVolumeType: validType, + ModificationKeyIOPS: validIops, + ModificationKeyThroughput: validThroughput, + AllowAutoIOPSIncreaseOnModifyKey: "true", + // IopsPerGB would not be actually allowed if IOPS is set but that is not checked in this function. Just testing that it properly parses it and adds the tag. + IopsPerGBKey: "1000", ModificationAddTag + "_1": validTagSpecificationInput, ModificationAddTag + "_2": "key2={{ .PVCName }}", ModificationAddTag + "_3": "key3={{ .PVCNamespace }}", @@ -165,9 +168,11 @@ func TestParseModifyVolumeParameters(t *testing.T) { }, expectedOptions: &modifyVolumeRequest{ modifyDiskOptions: cloud.ModifyDiskOptions{ - VolumeType: validType, - IOPS: validIopsInt, - Throughput: validThroughputInt, + VolumeType: validType, + IOPS: validIopsInt, + Throughput: validThroughputInt, + AllowIopsIncreaseOnResize: true, + IOPSPerGB: 1000, }, modifyTagsOptions: cloud.ModifyTagsOptions{ TagsToAdd: map[string]string{ @@ -175,6 +180,8 @@ func TestParseModifyVolumeParameters(t *testing.T) { "key2": "ebs-claim", "key3": "test-namespace", "key4": "testPV-Name", + "ebs.csi.aws.com/AllowAutoIOPSIncreaseOnModify": "true", + "ebs.csi.aws.com/IOPSPerGb": "1000", }, TagsToDelete: []string{ "key2", diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index 1dc4124d05..ef970eeba7 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -4911,6 +4911,7 @@ func TestControllerExpandVolume(t *testing.T) { } mockCloud := cloud.NewMockCloud(mockCtl) + mockCloud.EXPECT().GetDiskByID(gomock.Any(), gomock.Eq(tc.req.GetVolumeId())).AnyTimes() mockCloud.EXPECT().ResizeOrModifyDisk(gomock.Any(), gomock.Eq(tc.req.GetVolumeId()), gomock.Any(), gomock.Any()).Return(retSizeGiB, nil).AnyTimes() awsDriver := ControllerService{ diff --git a/pkg/driver/request_coalescing_test.go b/pkg/driver/request_coalescing_test.go index 330436a201..ad2147f105 100644 --- a/pkg/driver/request_coalescing_test.go +++ b/pkg/driver/request_coalescing_test.go @@ -105,6 +105,7 @@ func testBasicRequestCoalescingSuccess(t *testing.T, executor modifyVolumeExecut defer mockCtl.Finish() mockCloud := cloud.NewMockCloud(mockCtl) + mockCloud.EXPECT().GetDiskByID(gomock.Any(), gomock.Eq(volumeID)).AnyTimes() mockCloud.EXPECT().ResizeOrModifyDisk(gomock.Any(), gomock.Eq(volumeID), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, volumeID string, newSize int64, options *cloud.ModifyDiskOptions) (int64, error) { klog.InfoS("ResizeOrModifyDisk called", "volumeID", volumeID, "newSize", newSize, "options", options) if newSize != NewSize { @@ -167,6 +168,7 @@ func testRequestFail(t *testing.T, executor modifyVolumeExecutor) { defer mockCtl.Finish() mockCloud := cloud.NewMockCloud(mockCtl) + mockCloud.EXPECT().GetDiskByID(gomock.Any(), gomock.Eq(volumeID)).AnyTimes() mockCloud.EXPECT().ResizeOrModifyDisk(gomock.Any(), gomock.Eq(volumeID), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, volumeID string, newSize int64, options *cloud.ModifyDiskOptions) (int64, error) { klog.InfoS("ResizeOrModifyDisk called", "volumeID", volumeID, "newSize", newSize, "options", options) return 0, errors.New("ResizeOrModifyDisk failed") @@ -230,6 +232,7 @@ func testPartialFail(t *testing.T, executor modifyVolumeExecutor) { volumeTypeChosen := "" mockCloud := cloud.NewMockCloud(mockCtl) + mockCloud.EXPECT().GetDiskByID(gomock.Any(), gomock.Eq(volumeID)).AnyTimes() mockCloud.EXPECT().ResizeOrModifyDisk(gomock.Any(), gomock.Eq(volumeID), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, volumeID string, newSize int64, options *cloud.ModifyDiskOptions) (int64, error) { klog.InfoS("ResizeOrModifyDisk called", "volumeID", volumeID, "newSize", newSize, "options", options) if newSize != NewSize { @@ -322,6 +325,7 @@ func testSequentialRequests(t *testing.T, executor modifyVolumeExecutor) { defer mockCtl.Finish() mockCloud := cloud.NewMockCloud(mockCtl) + mockCloud.EXPECT().GetDiskByID(gomock.Any(), gomock.Eq(volumeID)).AnyTimes() mockCloud.EXPECT().ResizeOrModifyDisk(gomock.Any(), gomock.Eq(volumeID), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, volumeID string, newSize int64, options *cloud.ModifyDiskOptions) (int64, error) { klog.InfoS("ResizeOrModifyDisk", "volumeID", volumeID, "newSize", newSize, "options", options) return newSize, nil @@ -380,6 +384,7 @@ func testDuplicateRequest(t *testing.T, executor modifyVolumeExecutor) { defer mockCtl.Finish() mockCloud := cloud.NewMockCloud(mockCtl) + mockCloud.EXPECT().GetDiskByID(gomock.Any(), gomock.Eq(volumeID)).AnyTimes() mockCloud.EXPECT().ResizeOrModifyDisk(gomock.Any(), gomock.Eq(volumeID), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, volumeID string, newSize int64, options *cloud.ModifyDiskOptions) (int64, error) { klog.InfoS("ResizeOrModifyDisk called", "volumeID", volumeID, "newSize", newSize, "options", options) return newSize, nil @@ -438,6 +443,7 @@ func testResponseReturnTiming(t *testing.T, executor modifyVolumeExecutor) { defer mockCtl.Finish() mockCloud := cloud.NewMockCloud(mockCtl) + mockCloud.EXPECT().GetDiskByID(gomock.Any(), gomock.Eq(volumeID)).AnyTimes() mockCloud.EXPECT().ResizeOrModifyDisk(gomock.Any(), gomock.Eq(volumeID), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, volumeID string, newSize int64, options *cloud.ModifyDiskOptions) (int64, error) { klog.InfoS("ResizeOrModifyDisk called", "volumeID", volumeID, "newSize", newSize, "options", options) diff --git a/pkg/driver/validation.go b/pkg/driver/validation.go index 87a9646714..49b7fdd29a 100644 --- a/pkg/driver/validation.go +++ b/pkg/driver/validation.go @@ -25,6 +25,16 @@ import ( "k8s.io/klog/v2" ) +var ( + reservedTagKeys = map[string]string{ + cloud.VolumeNameTagKey: "", + cloud.AwsEbsDriverTagKey: "", + cloud.SnapshotNameTagKey: "", + IopsPerGBKey: "", + AllowAutoIOPSIncreaseOnModifyKey: "", + } +) + func ValidateDriverOptions(options *Options) error { if err := validateExtraTags(options.ExtraTags, false); err != nil { return fmt.Errorf("invalid extra tags: %w", err) @@ -43,14 +53,8 @@ func ValidateDriverOptions(options *Options) error { func validateExtraTags(tags map[string]string, warnOnly bool) error { validate := func(k, _ string) error { - if k == cloud.VolumeNameTagKey { - return fmt.Errorf("tag key '%s' is reserved", cloud.VolumeNameTagKey) - } - if k == cloud.AwsEbsDriverTagKey { - return fmt.Errorf("tag key '%s' is reserved", cloud.AwsEbsDriverTagKey) - } - if k == cloud.SnapshotNameTagKey { - return fmt.Errorf("tag key '%s' is reserved", cloud.SnapshotNameTagKey) + if _, ok := reservedTagKeys[k]; ok { + return fmt.Errorf("tag key '%s' is reserved", k) } if strings.HasPrefix(k, cloud.KubernetesTagKeyPrefix) { return fmt.Errorf("tag key prefix '%s' is reserved", cloud.KubernetesTagKeyPrefix)