-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Allow setting gp3 for terraform volumes without setting throughput #10572
Allow setting gp3 for terraform volumes without setting throughput #10572
Conversation
/cc @rifelpet @olemarkus |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman 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 |
// TODO: Remove when Terraform gets support for "throughput" with "aws_ebs_volume" | ||
// https://github.com/hashicorp/terraform-provider-aws/pull/16517 | ||
throughput := e.VolumeThroughput | ||
if fi.Int64Value(e.VolumeThroughput) <= 125 { |
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.
why are we only setting it to nil with this condition? if the throughput field isn't yet supported at all in terraform then I'd think we should always set it to nil?
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 we should have an integration test that generates this terraform output so that the verify-terraform job will catch 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.
It is set to nil if you don't use gp3 volumes. We have integration test in master #10569, but not sure how we can "catch" 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.
looking at the api docs
// The throughput to provision for a volume, with a maximum of 1,000 MiB/s.
//
// This parameter is valid only for gp3 volumes.
//
// Valid Range: Minimum value of 125. Maximum value of 1000.
Throughput *int64 `type:"integer"`
I feel like we should fail api validation if throughput is < 125 rather than ignoring the value. We could also fail validation if RootVolumeThroughput is set for RootVolumeTypes other than gp3 (and if cloudprovider != aws )
This is the only throughput validation we have currently:
kops/pkg/apis/kops/validation/instancegroup.go
Lines 72 to 74 in 0410058
if fi.Int32Value(g.Spec.RootVolumeThroughput) < 0 { | |
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "rootVolumeThroughput"), g.Spec.RootVolumeThroughput, "RootVolumeThroughput must be greater than 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.
We could, but it's not really this hack's purpose. I just want to not put the value in the TF template if the value is the minimum to not need a new kOps release when TF decides to merge the throughput arg. We set values lower than the minimum to minimum in model.
/lgtm |
Until hashicorp/terraform-provider-aws#16517 is merged, TF will not be able to use the
throughput
arg foraws_ebs_volume
.