-
-
Notifications
You must be signed in to change notification settings - Fork 322
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
support IOPS in AWS (required for io1) #314
Conversation
@@ -30,6 +30,7 @@ resource "aws_instance" "controllers" { | |||
root_block_device { | |||
volume_type = "${var.disk_type}" | |||
volume_size = "${var.disk_size}" | |||
iops = "${var.disk_iops}" |
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.
Begs the question of whether iops
is a valid parameter for standard and gp2 disks and if so, what the value should be.
@@ -52,6 +52,12 @@ variable "disk_type" { | |||
description = "Type of the EBS volume (e.g. standard, gp2, io1)" | |||
} | |||
|
|||
variable "disk_iops" { | |||
type = "string" | |||
default = "120" |
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 pulled from the console I take 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.
There need to be instructions on calculating this since it changes with type and size (just compared with my own) or maybe just drop the ability to customize the disk_type.
I will look and see if there is a better way to handle iops and default values. Ideally we would conditionally omit the iops setting, but Terraform does not appear that it will support this directly until 0.12 (https://www.hashicorp.com/blog/terraform-0-12-conditional-operator-improvements). |
If I pass |
I updated the default to 0 since AWS appears to do the right thing with that value set. Please let me know if you have any further concerns. |
Sounds alright. I'll give this another look soon. Already switched over all clusters to v1.12.1 about 2 days back so it'll miss the v1.12.1 train, but can make it into the next release. |
Can you squash to one commit and rebase please?
…On Wed, Oct 17, 2018, 6:53 AM Robert Fairburn ***@***.***> wrote:
I updated the default to 0 since AWS appears to do the right thing with
that value set. Please let me know if you have any further concerns.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#314 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACJidBYZH3wmRyO4va5Ur_FpY4VOfeH7ks5ulzZBgaJpZM4XclJs>
.
|
2564f8b
to
0058773
Compare
Done |
Rebased on master and added a changelog entry. Merged as 0be4673. Thanks! |
This change allows for specifying a disk_type of io1 in AWS. Without having IOPS as a configurable variable,
terraform apply
will error out with a missing required setting.Testing
I was able to successfully provision both workers and controllers as io1 with a custom number of iops after this change.