Skip to content

WIP: add throughput field to EBSBlockDevice in machine-api #2220

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.

Sign up for GitHub

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

Closed
sjenning wants to merge 1 commit into from
Closed

WIP: add throughput field to EBSBlockDevice in machine-api #2220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions machine/v1beta1/types_awsprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,11 @@ type EBSBlockDeviceSpec struct {
// 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.
Copy link
Copy Markdown
Member

@wking wking Mar 5, 2025

Choose a reason for hiding this comment

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

CAPI docs:

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?

Copy link
Copy Markdown
Contributor Author

@sjenning sjenning Mar 5, 2025

Choose a reason for hiding this comment

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

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"

Copy link
Copy Markdown
Member

@wking wking Mar 5, 2025

Choose a reason for hiding this comment

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

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.

//
// Constraints: 125-1000 for General Purpose SSD (gp3)
Copy link
Copy Markdown
Member

@enxebre enxebre Mar 6, 2025

Choose a reason for hiding this comment

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

can we validate this constraints with min/max and that this is only set when VolumeType=gp3 with CEL

Copy link
Copy Markdown
Member

@enxebre enxebre Mar 6, 2025

Choose a reason for hiding this comment

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

can we also doc what's default if unset and since this is mutable what happens if changed day2?

// +optional
Throughput *int64 `json:"throughput,omitempty"`
Copy link
Copy Markdown
Contributor

@everettraven everettraven Mar 6, 2025

Choose a reason for hiding this comment

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

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.

// 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 int64 and could instead be an int32. I see that the upstream equivalent field is an int64 and int64 is used throughout this type so I won't hold you to this, but worth mentioning.
  • This field is optional, which means that even when volumeType is set to gp3 a 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 volumeType has no mention of gp3 as 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.

Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed Mar 6, 2025

Choose a reason for hiding this comment

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

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

// The size of the volume, in GiB.
//
// Constraints: 1-16384 for General Purpose SSD (gp2), 4-16384 for Provisioned
Expand Down
5 changes: 5 additions & 0 deletions machine/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions openapi/generated_openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions openapi/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21350,6 +21350,11 @@
"default": {},
"$ref": "#/definitions/com.github.openshift.api.machine.v1beta1.AWSResourceReference"
},
"throughput": {
"description": "Throughput to provision in MiB/s. This parameter is valid only for gp3 volumes.\n\nConstraints: 125-1000 for General Purpose SSD (gp3)",
"type": "integer",
"format": "int64"
},
"volumeSize": {
"description": "The size of the volume, in GiB.\n\nConstraints: 1-16384 for General Purpose SSD (gp2), 4-16384 for Provisioned IOPS SSD (io1), 500-16384 for Throughput Optimized HDD (st1), 500-16384 for Cold HDD (sc1), and 1-1024 for Magnetic (standard) volumes. If you specify a snapshot, the volume size must be equal to or larger than the snapshot size.\n\nDefault: If you're creating the volume from a snapshot and don't specify a volume size, the default is the snapshot size.",
"type": "integer",
Expand Down