Skip to content

Conversation

mdzraf
Copy link
Member

@mdzraf mdzraf commented Aug 20, 2025

What type of PR is this?

/kind feature

What is this PR about? / Why do we need it?

This PR adds a way for allowing IOPS to be increased if a volume is resized and IopsPerGB is set. This way the ratio will remain as expected after a resize, in order to use this feature allowautoiopsincreaseonresize must be set to true as well.

This PR also adds iopspergb as a VAC parameter so that it can be changed.

How was this change tested?

As of this first revision, no unit tests have been added or existing ones have been modified, manual testing listed below. Unit tests will be a part of revision 2 after initial rev is reviewed

Manual Tests Performed:

  • Resizing and ensuring IOPS are adjusted to match set ratio ✅
  • Resizing + increasing iopspergb ratio and ensuring IOPS are adjusted correctly to new size and ratio ✅
  • Resizing with iopspergb set but allowautoiopsincreaseonresize not set/set to false, ensure that resize happens but no IOPS are adjusted ✅
  • iopspergb is increased and IOPS are adjusted accordingly depending on volume size ✅
  • allowautoiopsincreaseonresize is set to true after initial volume creation, and then resizes. IOPS are adjusted accordingly ✅

Does this PR introduce a user-facing change?

Added AllowAutoIopsIncreaseOnResize SC and VAC parameter To Allow For Maintaining desired IOPS:GB Ratio When A Volume is Resized and Has IopsPerGB Set. Added ability to modify IopsPerGB parameter through VAC. 

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Aug 20, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 20, 2025
@mdzraf
Copy link
Member Author

mdzraf commented Aug 20, 2025

/hold

Unit tests need to be added after initial rev is reviewed.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2025
@mdzraf mdzraf force-pushed the allowIopsIncreaseOnResize branch from e825560 to f18c9a7 Compare August 21, 2025 14:55
@mdzraf mdzraf changed the title Adding IOPS Increase On Resize If iopspergb & allowautoiopsincreaseonresize are set [WIP] Adding IOPS Increase On Resize If iopspergb & allowautoiopsincreaseonresize are set Aug 21, 2025
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2025
@mdzraf mdzraf force-pushed the allowIopsIncreaseOnResize branch 4 times, most recently from d46dd5a to 5bfc434 Compare August 21, 2025 19:42
Copy link

github-actions bot commented Aug 21, 2025

Code Coverage Diff

File Old Coverage New Coverage Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/cloud.go 87.1% 85.8% -1.2
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/controller.go 83.0% 82.9% -0.1
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/controller_modify_volume.go 76.1% 70.6% -5.5
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/validation.go 96.3% 95.7% -0.6

@mdzraf mdzraf force-pushed the allowIopsIncreaseOnResize branch from 5bfc434 to 93ead5c Compare August 21, 2025 19:59
@mdzraf mdzraf changed the title [WIP] Adding IOPS Increase On Resize If iopspergb & allowautoiopsincreaseonresize are set Adding IOPS Increase On Resize If iopspergb & allowautoiopsincreaseonresize are set Aug 26, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2025
@mdzraf mdzraf force-pushed the allowIopsIncreaseOnResize branch from 93ead5c to 28ee283 Compare September 11, 2025 16:33
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from connorjc3. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mdzraf mdzraf requested a review from ConnorJC3 September 11, 2025 18:22
Copy link
Contributor

@ConnorJC3 ConnorJC3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Significantly improved versus the previous version, did some manual testing but want to do a more comprehensive test for the final revision. Left some new comments but most should be very easy to address.

} else {
req.Iops = aws.Int32(capIOPS(string(volTypeToUse), sizeToUse, iopsValue, IopLimits, false))
}
options.IOPS = *req.Iops
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make the code saner if we passed req into validateModifyVolume instead of options?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants