CORS-4250: AWS: Add the ability to configure throughput on GP3 volumes #2480
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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: |
openshift-ci
bot
added
the
size/S
|
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. |
|
Is this feature available in CAPA upstream? |
Yes. |
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
4a4d3cf to
ee12153
Compare
|
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... |
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
2 times, most recently
from
ab99e54 to
b7238bc
Compare
|
+1 to @tthvo's #2480 (comment) I'm confused. IIUC this PR is adding configuration for MAPI; but MAPI-support is not part of the plan, this is a CAPI-only feature. I think we should just depend on the existing CAPI API and close this PR. @jhixson74 @JoelSpeed can you please clarify? |
We aren't clear yet when we will GA CAPI on OCP. We can avoid adding this to MAPI sure, but that currently restricts control plane usage (as control plane CAPI is further down the road), but it seems like a fairly minor effort to add it IMO and means that conversion between the two types of machine has fewer edge cases |
Agreed. Adding MAPI support seems like the right thing here. |
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
b7238bc to
86385ff
Compare
jhixson74
changed the title
|
@jhixson74: This pull request references CORS-4250 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 story 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. |
|
/jira refresh |
|
@jhixson74: This pull request references CORS-4250 which is a valid jira issue. 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. |
machine/v1beta1/types_awsprovider.go
Outdated
| // 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.
@jhixson74 Are you sure you want to add this to v1beta1 and not v1?
There was a problem hiding this comment.
There is no v1 for this API
machine/v1beta1/types_awsprovider.go
Outdated
| // throughput to provision in MiB/s supported for the volume type. Not applicable to all types. | ||
| // | ||
| // This parameter is valid only for gp3 volumes. | ||
| // Valid Range: Minimum value of 125. Maximum value of 1000. |
There was a problem hiding this comment.
Can it have any value between 125 and 1000?
| // | ||
| // 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. |
There was a problem hiding this comment.
Is there value in adding // +default=125? Can its value be modified in a running cluster?
Can we also add a link to the documentation that you have been using for reference?
There was a problem hiding this comment.
We shouldn't add defaults for this API as it's a configuration API. We may want to adjust this default over time so it's better to have nothing persisted and allow this to be defaulted by the controller when acting up on the value
openshift-ci
bot
added
lgtm
|
@jhixson74 One more lint fix for ci/prow/lint fail? 😅 |
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
ef60ce1 to
c4802d6
Compare
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
c4802d6 to
3331ded
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
openapi/openapi.json (1)
23388-23392: Add OpenAPI validation constraints for throughputMib.The throughputMib field specifies valid range (125–2000 MiB/s) in the description but does not enforce these bounds in the schema itself. Add minimum and maximum constraints to enable client-side validation.
"throughputMib": { "description": "throughputMib to provision in MiB/s supported for the volume type. Not applicable to all types.\n\nThis parameter is valid only for gp3 volumes. Valid Range: Minimum value of 125. Maximum value of 2000.\n\nWhen 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.", "type": "integer", - "format": "int32" + "format": "int32", + "minimum": 125, + "maximum": 2000 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.go(2 hunks)machine/v1beta1/zz_generated.deepcopy.go(1 hunks)machine/v1beta1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- openapi/generated_openapi/zz_generated.openapi.go
- machine/v1beta1/types_awsprovider.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machine/v1beta1/zz_generated.deepcopy.gomachine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/openapi.json
🔇 Additional comments (5)
machine/v1beta1/zz_generated.deepcopy.go (1)
584-588: LGTM! Deepcopy logic for ThroughputMib is correct.The deepcopy implementation correctly follows the established pattern for optional pointer fields (int32) and is consistent with other fields like
Iops,VolumeSize, andVolumeTypein the same function.machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
111-113: LGTM! Swagger documentation is comprehensive and accurate.The documentation correctly describes:
throughputMib: Valid for gp3 volumes, range 125-2000 MiB/s (reflecting AWS's latest limits), with clear default behaviorvolumeType: Updated to include gp3 alongside other volume typesBoth additions align with the PR objectives to support GP3 throughput configuration.
openapi/openapi.json (3)
14271-14340: Verify FormatMarkerExamples schema generation source.The FormatMarkerExamples schema appears to be a manually-added documentation reference within openapi.json. Since this file is typically auto-generated, confirm that this addition is intentional and not a build artifact or merge conflict remnant. If this is generated code, verify the source type/code generated it.
14430-14433: Godoc format is correct.The field description correctly starts with the serialized field name "formatMarkerExamples" and provides clear documentation of its purpose.
23399-23399: Fix volumeType field description to follow godoc conventions.The description should start with the field name "volumeType" to comply with Go documentation standards. According to the PR discussion, this was flagged as a linting issue that must be addressed.
- "description": "The volume type: gp3, gp2, io1, st1, sc1, or standard. Default: standard", + "description": "volumeType is the volume type: gp3, gp2, io1, st1, sc1, or standard. Default: standard",Likely an incorrect or invalid review comment.
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
3331ded to
04158de
Compare
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
04158de to
d1b602d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.go(1 hunks)machine/v1beta1/zz_generated.deepcopy.go(1 hunks)machine/v1beta1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- machine/v1beta1/zz_generated.deepcopy.go
- machine/v1beta1/zz_generated.swagger_doc_generated.go
- machine/v1beta1/types_awsprovider.go
- openapi/generated_openapi/zz_generated.openapi.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
openapi/openapi.json
🔇 Additional comments (1)
openapi/openapi.json (1)
23388-23399: Verify throughputMib field description aligns with generated code in other files.Ensure the throughputMib description in this OpenAPI schema matches the GoDoc string in
machine/v1beta1/types_awsprovider.goand the swagger documentation inzz_generated.swagger_doc_generated.gomentioned in the AI summary.
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
d1b602d to
e77874c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
openapi/openapi.json (3)
14430-14433: Remove formatMarkerExamples property—dependent on out-of-scope schema.This property reference depends on the
FormatMarkerExamplesschema (Hunk 1). Remove it when removing that schema to keep the PR focused.
14271-14340: FormatMarkerExamples schema remains out of scope—should be removed.This schema addition (and its reference in Hunk 2) is unrelated to the PR's stated objective of adding GP3 throughput configuration. Past reviews flagged this as scope creep from a separate commit. Remove this schema definition to keep the PR focused on the
ThroughputMibfeature.
23388-23392: Add minimum/maximum constraints to throughputMib schema.The
throughputMibfield description documents the valid range (125–2000), but the OpenAPI schema lacks explicitminimumandmaximumproperties. Add these constraints to self-document the allowed values and match the Go code's kubebuilder validation markers.Apply this diff:
"throughputMib": { "description": "throughputMib to provision in MiB/s supported for the volume type. Not applicable to all types.\n\nThis parameter is valid only for gp3 volumes. Valid Range: Minimum value of 125. Maximum value of 2000.\n\nWhen 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.", "type": "integer", - "format": "int32" + "format": "int32", + "minimum": 125, + "maximum": 2000 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.go(1 hunks)machine/v1beta1/zz_generated.deepcopy.go(1 hunks)machine/v1beta1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(1 hunks)openapi/openapi.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- openapi/generated_openapi/zz_generated.openapi.go
- machine/v1beta1/zz_generated.deepcopy.go
- machine/v1beta1/types_awsprovider.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/openapi.json
GP3 volumes have the ability to configure throughput from 125 MiB/s to 2000 MiB/s. This allows the ability to set this at install time in the install-config. https://issues.redhat.com/browse/CORS-4212
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
e77874c to
41ccbe5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
openapi/openapi.json (2)
14271-14340: FormatMarkerExamples schema remains unrelated to GP3 throughput—remove or defer to separate PR.This new schema and its reference (hunks 1–2) were previously flagged as scope creep. Git analysis in the prior review showed this content originated from a separate commit (80d1f5c) and is unrelated to the stated PR objective (CORS-4250: configuring GP3 EBS throughput). If format marker documentation is needed, it belongs in a dedicated PR, not bundled with the GP3 feature.
Apply this diff to remove both hunks:
- "com.github.openshift.api.example.v1.FormatMarkerExamples": { - "description": "FormatMarkerExamples demonstrates all Kubebuilder Format markers supported as of Kubernetes 1.33. This struct provides a comprehensive reference for format marker validation. Each field uses a different format marker to validate its value.", - "type": "object", - "properties": { - "base64Data": { - "description": "base64Data must be valid base64-encoded data. Valid examples include aGVsbG8= (encodes \"hello\") or SGVsbG8gV29ybGQh (encodes \"Hello World!\").", - "type": "string" - }, - ... (all other format marker fields) - } - },And in the StableConfigType properties:
- "formatMarkerExamples": { - "description": "formatMarkerExamples demonstrates all Kubebuilder Format markers supported as of Kubernetes 1.33. This field serves as a comprehensive reference for format marker validation.", - "$ref": "#/definitions/com.github.openshift.api.example.v1.FormatMarkerExamples" - },Also applies to: 14430-14433
23388-23392: Add OpenAPI validation constraints for throughputMib to match Kubebuilder markers.The
throughputMibfield description documents the valid range (125–2000), but the OpenAPI schema lacks explicitminimumandmaximumnumeric constraints. The Go type definition inmachine/v1beta1/types_awsprovider.goincludes:// +kubebuilder:validation:Minimum:=125 // +kubebuilder:validation:Maximum:=2000These constraints must be reflected in the OpenAPI schema for consistency and proper API documentation.
Apply this diff to add validation constraints:
"throughputMib": { "description": "throughputMib to provision in MiB/s supported for the volume type. Not applicable to all types.\n\nThis parameter is valid only for gp3 volumes. Valid Range: Minimum value of 125. Maximum value of 2000.\n\nWhen 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.", "type": "integer", - "format": "int32" + "format": "int32", + "minimum": 125, + "maximum": 2000 }If this file is auto-generated, ensure the OpenAPI generation process is re-run to regenerate the schema with these constraints included.
🧹 Nitpick comments (1)
openapi/generated_openapi/zz_generated.openapi.go (1)
40570-40576: LGTM! Generated OpenAPI schema correctly represents the throughputMib field.The schema accurately documents the gp3-only constraint and the valid range (125-2000 MiB/s), which aligns with the updated AWS limits.
Optional enhancement: The OpenAPI schema could include explicit
minimum: 125andmaximum: 2000constraints on thethroughputMibfield for schema-level validation and improved API documentation. However, since webhook validation handles runtime checks and this approach is consistent with other fields (e.g.,volumeSizedocuments constraints without schema enforcement), the current implementation is acceptable. If desired, this would need to be added in the source type definitions, not in this generated file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.go(2 hunks)machine/v1beta1/zz_generated.deepcopy.go(1 hunks)machine/v1beta1/zz_generated.swagger_doc_generated.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- machine/v1beta1/zz_generated.deepcopy.go
- machine/v1beta1/zz_generated.swagger_doc_generated.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machine/v1beta1/types_awsprovider.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.json
🔇 Additional comments (3)
machine/v1beta1/types_awsprovider.go (2)
216-228: LGTM!The
ThroughputMibfield implementation is well-structured and thoroughly documented. The field correctly uses*int32for optional semantics, includes appropriate validation markers (125-2000 MiB/s), clearly states it applies only to gp3 volumes, and documents the default behavior. The implementation follows established API conventions and addresses all past review feedback.
241-241: LGTM!Good documentation improvement. Explicitly listing gp3 alongside other supported volume types improves API clarity and aligns with the addition of the gp3-specific
ThroughputMibfield.openapi/generated_openapi/zz_generated.openapi.go (1)
40586-40586: LGTM! volumeType description correctly updated to include gp3.The explicit listing of gp3 as a valid volume type improves documentation clarity and aligns with the PR objectives.
|
/lgtm |
|
@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. |
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
Nothing changed since other than making lint happy :D
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, patrickdillon, sadasu, tthvo 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 |
|
/verified by @tthvo |
openshift-ci-robot
added
the
verified
|
@tthvo: This PR has been marked as verified by 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. |
|
Thanks for merging this PR! I'll wait for a nightly build and then do the test. |
9 participants
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