-
Notifications
You must be signed in to change notification settings - Fork 565
CORS-4212: AWS: Add the ability to configure throughput on GP3 volumes #2480
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: master
Are you sure you want to change the base?
Conversation
@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. |
Hello @jhixson74! Some important instructions when contributing to openshift/api: |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
I see that the linter is failing. For the life of me, I don't know why since every other field in that struct also does not start with the name. Any pointers? ;-) |
Pre-existing fields do not meet conventions, please follow the linter so that new content does meet conventions. |
machine/v1beta1/types_awsprovider.go
Outdated
// | ||
// Valid Range: Minimum value of 125. Maximum value of 1000. | ||
// +optional | ||
Throughput *int64 `json:"throughput,omitempty"` |
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.
Do we really need an int64 if the maximum value is 1000?
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 would say no, we don't, and agree with you. However, this is what the AWS sdk uses so I went with that:
(can't display the file due to how large it is but you want to view the CreateVolumeInput struct)
https://raw.githubusercontent.com/aws/aws-sdk-go/refs/heads/main/service/ec2/api.go
Thoughts? I'm happy to change it.
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.
Prefer to align to downstream API conventions rather then upstream APIs, so would switch to int32 here
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.
@JoelSpeed CAPA uses int64 as well. Do you still want to make it an int32?
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.
Sigh. An int32 is the right thing to use, but I'm ok if we keep as int64 given CAPA has made that decision. Given this is not a structured API, it makes little difference anyway (where in a structured API it does change the schema)
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.
Just wanted to add that CAPA, even though using int64
for the ebs throughput field, it actually converts the value to int32
. This is the result of migrating to AWS SDK v2, which uses the int32
now.
So, I agree int32
is best :D
Is this feature available in CAPA upstream? |
Yes. |
4a4d3cf
to
ee12153
Compare
// it is not used in requests to create gp2, st1, sc1, or standard volumes. | ||
// +optional | ||
Iops *int64 `json:"iops,omitempty"` | ||
// 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. Not applicable to all types. |
Not sure if lint happy with this 🤔?
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.
Locally, make lint is expecting the lowercase version. Uppercase fails. It passed the lint test, so I'm inclined to believe the lowercase version is the expectation here.
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.
Ahh thanks, that's a bit odd tho :D
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.
Oh, I missed Joel's comment: #2480 (comment). That makes sense now. Sorry :D
machine/v1beta1/types_awsprovider.go
Outdated
// | ||
// Valid Range: Minimum value of 125. Maximum value of 1000. | ||
// +optional | ||
Throughput *int32 `json:"throughput,omitempty"` |
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.
Just wondering if we need to mention the default here? That should be 125
MiB/s right?
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.
This parameter, just like Iops above it, will set the default when not configured.
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.
Also, it is only valid for gp3 volumes. I don't know what behavior would occur if we set it to 125 on a non-gp3 volume. 0 is a good default.
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.
Right, I was thinking of mentioning in the description about what the default value will be set if not configured.
But i guess if Iops is not saying anything, we can skip it :D
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.
@jhixson74 Though, one more question: I see the we define the valid range
Valid Range: Minimum value of 125. Maximum value of 1000.
Do we expect this change in the future (i.e. AWS might announce new limits)? If so, should we follow the field Iops
and say this instead:
// Minimal and maximal throughput for gp3 are constrained. Please, check
// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSVolumeTypes.html
// for precise boundaries for individual volumes.
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 tend to include this kind of information, in a sentence approximately
When omitted, this means no opinion, and the platform is left to choose a reasonable default, which is subject to change over time.
The current default is 125.
IIUC, the throughput setting is not yet supported in MAPI (thus this PR is created) right? From OCPSTRAT-2410, it looks like we are not looking to add support MAPI, but rather wait for CAPI migration in OCPSTRAT-1992 (the throughput is already supported there). So, is this change still needed? I might be misunderstanding something... |
machine/v1beta1/types_awsprovider.go
Outdated
// | ||
// Valid Range: Minimum value of 125. Maximum value of 1000. | ||
// +optional | ||
Throughput *int32 `json:"throughput,omitempty"` |
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 tend to include this kind of information, in a sentence approximately
When omitted, this means no opinion, and the platform is left to choose a reasonable default, which is subject to change over time.
The current default is 125.
machine/v1beta1/types_awsprovider.go
Outdated
// | ||
// Valid Range: Minimum value of 125. Maximum value of 1000. | ||
// +optional | ||
Throughput *int32 `json:"throughput,omitempty"` |
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 should probably call this field ThroughputMiB
, we prefer putting units on integer field names when there is a unit implicitly
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.
@JoelSpeed I followed the conventions of what the current struct looks like. None of the fields contain units of measurement in the name. IMO this would look a little strange with the rest of the fields not containing the same.
So, for clarity and to get this PR further along:
- Update comment
- Update to be int32
- Add unit of measurement to filed name?
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 made a choice that new fields would follow newer conventions, and that we wouldn't repeat mistakes of the past, so I think in this case, we should still change the name yes, ThroughputMiB
, and update the docs as with the other suggestion.
Validation on this API is implemented in a webhook, not openapi, so I think this has to stay as a pointer so that we can ensure in the webhook that it is not accidentally set to 0
by an unstructured client
ee12153
to
ab99e54
Compare
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
ab99e54
to
b7238bc
Compare
@jhixson74: all tests passed! 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. |
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