-
Notifications
You must be signed in to change notification settings - Fork 1.4k
CORS-4212: AWS: Add the ability to configure throughput on GP3 volumes #9945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
GP3 volumes have the ability to configure throughput from 125 MiB/s to 1000 MiB/s. This allows the ability to set this at install time in the install-config. https://issues.redhat.com/browse/CORS-4212
@jhixson74: This pull request references CORS-4212 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@jhixson74: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/label do-not-merge |
@jhixson74: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/label do-not-merge/hold |
@jhixson74: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/hold |
/cc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good to me
/approve
There are a few small things that should be cleaned up, commented below. And there is no rush because this depends on openshift/api#4212
|
||
// Throughput to provision in MiB/s supported for the volume type. Not applicable to all types. | ||
// | ||
// +kubebuilder:validation:Minimum=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the minimum is listing as 0, but we validate the minimum as 125, so it seems like the minimum here should be set to 125 and also the maximum should be set as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A non-zero value will break things. This setting is only valid for gp3 volumes. While I don't know the behavior from Amazon that this would cause, I do know that cluter-api-provider-aws will blow up:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We handle IOPS in a similar fashion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// +optional | ||
IOPS int `json:"iops"` | ||
|
||
// Throughput to provision in MiB/s supported for the volume type. Not applicable to all types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Throughput to provision in MiB/s supported for the volume type. Not applicable to all types. | |
// Throughput to provision in MiB/s supported for the volume type. Only supported for gp3. |
EdgeZonesGatewayIndex map[string]int `json:"aws_edge_parent_zones_index,omitempty"` | ||
EdgeZonesType map[string]string `json:"aws_edge_zones_type,omitempty"` | ||
IOPS int64 `json:"aws_master_root_volume_iops"` | ||
Throughput int64 `json:"aws_master_root_volume_throughput"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this change. AFAIK AWS is not using the Terraform configs (a few providers get information from the terraform configs in the capi workflows, but I don't think AWS is one of those. correct me if I'm wrong). We will delete them to avoid future confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove. Why does this code still exist if we aren't using it?
if rootVolume.EBS.Throughput != nil { | ||
cfg.Throughput = *rootVolume.EBS.Throughput | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, I don't think we need this terraform update.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
throughput := p.EC2RootVolume.Throughput | ||
|
||
switch volumeType { | ||
case "gp3": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickdillon @jhixson74 should we allow io2
too as it is commonly used to environments which requires highly throughput/performance on disks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtulio according to documentation in the code, this is only supported by gp3. If you can show otherwise, I'm happy to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.aws.amazon.com/cli/latest/reference/ec2/create-volume.html
Scroll down to --throughput
Only gp3 is supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I messed up with iops
GP3 volumes have the ability to configure throughput from 125 MiB/s to 1000 MiB/s. This allows the ability to set this at install time in the install-config.
https://issues.redhat.com/browse/CORS-4212
https://issues.redhat.com/browse/CORS-4213
https://issues.redhat.com/browse/CORS-4214
This depends on openshift/api#2480