WIP: add throughput field to EBSBlockDevice in machine-api#2220
WIP: add throughput field to EBSBlockDevice in machine-api#2220sjenning wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Hello @sjenning! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sjenning The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
| // 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. This parameter is valid only for gp3 volumes. |
There was a problem hiding this comment.
Throughput to provision in MiB/s supported for the volume type. Not applicable to all types.
Do we want to echo that punt to AWS? Or do we want the inlined AWS context you have here?
There was a problem hiding this comment.
I was following this, which I felt was more informative.
https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_EbsBlockDevice.html
"This only applies to the enumerated volume types" is more helpful than "This doesn't apply to some unenumerated volumes types"
There was a problem hiding this comment.
it is more helpful while it's accurate, but it means you're on the hook to keep it accurate. The rate of AWS-side changes is likely not high, so it could go either way. I'm just pointing out that CAPI has gone the punt-to-AWS way.
|
@sjenning: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
| Iops *int64 `json:"iops,omitempty"` | ||
| // Throughput to provision in MiB/s. This parameter is valid only for gp3 volumes. | ||
| // | ||
| // Constraints: 125-1000 for General Purpose SSD (gp3) |
There was a problem hiding this comment.
can we validate this constraints with min/max and that this is only set when VolumeType=gp3 with CEL
There was a problem hiding this comment.
can we also doc what's default if unset and since this is mutable what happens if changed day2?
| // | ||
| // Constraints: 125-1000 for General Purpose SSD (gp3) | ||
| // +optional | ||
| Throughput *int64 `json:"throughput,omitempty"` |
There was a problem hiding this comment.
I'm still learning the ropes for what guidelines we typically follow for the openshift APIs when reviewing the machine APIs as they don't result in CRDs. I'm going to review this change from a standard openshift API change perspective and may make some slight tweaks based on what I do know about how the machine types are used.
- Generally for the GoDoc on fields we recommend following the guidelines here: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc . Even though it won't match the GoDoc in the rest of this type, it might be worth aligning this with how we are trying to write GoDoc throughout the rest of the OpenShift APIs for consistency. I'd suggest something along the lines of:
// throughput is an optional field used to specify the throughput, in MiB/s, that the volume should be provisioned with.
// throughput may be specified when volumeType is set to gp3 and is forbidden otherwise.
// throughput must be greater than or equal to 125 and less than or equal to 1000.- With a constraint of 125 - 1000 it seems like this doesn't really need to be an
int64and could instead be anint32. I see that the upstream equivalent field is anint64andint64is used throughout this type so I won't hold you to this, but worth mentioning. - This field is optional, which means that even when
volumeTypeis set togp3a user doesn't have to specify this. Is there a default value that is set if this field is omitted? - Doesn't need to be included in the change here, but a link to where the validations for this field are implemented would be handy to better understand the constraints that should be specified for this field.
- Note: the GoDoc for the
volumeTypehas no mention ofgp3as an allowed value. I think this aligns with @wking's previous comment of needing to be on top of maintaining this as things change if we want to be explicit in the GoDoc of what is and is not supported.
There was a problem hiding this comment.
Also worth noting that this doesn't need to be a pointer if it's omitempty, a Go marshalled zero value won't show up so it won't trigger the validation on size limits
|
Decided I didn't want to take this on. Future is CAPI and might just wait for it to arrive. Thanks for the reviews though! |
|
Hey, #2480 added |
5 participants
cc @JoelSpeed
Still need to create the Jira stuff
https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_EbsBlockDevice.html