aws_instance accepts iops = 0 on non-io1 root_block_device

[mengesb]

There should be some exclusion in the aws_instance resource for root_block_device.

When using anything other than io1 for root_block_device.volume_type, if the user provides an iops it should be ignored. Currently it doesn't ignore it, so any value results in an error:

* aws_instance.chef-frontends.0: Error launching source instance: InvalidParameterCombination: The parameter iops is not supported for gp2 volumes.
    status code: 400, request id: f02269a9-7bd3-4a34-b804-ba2323571584

except when iops = 0:

$terraform plan
<SNIP>
+ aws_instance.chef-frontends.0
    ami:                                       "" => "ami-3bdd502c"
    associate_public_ip_address:               "" => "true"
    availability_zone:                         "" => "<computed>"
    ebs_block_device.#:                        "" => "<computed>"
    ebs_optimized:                             "" => "true"
    ephemeral_block_device.#:                  "" => "<computed>"
    instance_state:                            "" => "<computed>"
    instance_type:                             "" => "m4.large"
    key_name:                                  "" => "chef_provisioning"
    placement_group:                           "" => "<computed>"
    private_dns:                               "" => "<computed>"
    private_ip:                                "" => "<computed>"
    public_dns:                                "" => "<computed>"
    public_ip:                                 "" => "<computed>"
    root_block_device.#:                       "" => "1"
    root_block_device.0.delete_on_termination: "" => "true"
    root_block_device.0.iops:                  "" => "0"
    root_block_device.0.volume_size:           "" => "20"
    root_block_device.0.volume_type:           "" => "gp2"
    security_groups.#:                         "" => "<computed>"
    source_dest_check:                         "" => "true"
    subnet_id:                                 "" => "subnet-1186b749"
    tags.#:                                    "" => "2"
    tags.Description:                          "" => "Created using Terraform"
    tags.Name:                                 "" => "cheffrontend-001.<SNIP>"
    tenancy:                                   "" => "<computed>"
    vpc_security_group_ids.#:                  "" => "2"
    vpc_security_group_ids.2101024370:         "" => "sg-8ddbd6f6"
    vpc_security_group_ids.3483048042:         "" => "sg-8cdbd6f7"
<SNIP>

Trouble is when you need it (using io1 volume types, you should provide it, however since there's no logic to include or exclude a parameter, the plugin should handle this gracefully.

Enhancement: when usiong io1 disk types and iops is not provided, maybe assume the logical maximum (X GiB * 30 = IOPS) with a floor of 100 and a roof of 20000.

Terraform Version

Terraform v0.6.16

Affected Resource(s)

Please list the resources as a list, for example:

• aws_instance

Terraform Configuration Files

# Root disk variables
variable "root_delete_termination" {
  description = "Delete server root block device on termination"
  default     = true
}
variable "root_volume_size" {
  description = "Size in GB of root device"
  default     = {
    frontend  = 20
    backend   = 100
  }
}
variable "root_volume_type" {
  description = "Type of root volume"
  default     = {
    frontend  = "gp2"
    backend   = "gp2"
    fe_xiops  = 0
    be_xiops  = 0
  }
}
# END Variables
resource "aws_instance" "chef-frontends" {
  count         = "${var.count_frontends}"
  ami           = "${lookup(var.ami_map, "${var.ami_os}-${lookup(var.root_volume_type, "frontend")}-${lookup(var.aws_settings, "region")}")}"
  ebs_optimized = "${lookup(var.ebs_optimized, "frontend")}"
  instance_type = "${lookup(var.aws_flavor, "frontend")}"
  associate_public_ip_address = "${var.public_ip}"
  #subnet_id     = "${element(aws_subnet.chef-ha-subnet.*.id, count.index)}"
  subnet_id     = "${element(aws_subnet.chef-ha-subnet.*.id, count.index % length(keys(var.aws_subnets)))}"
  vpc_security_group_ids = ["${aws_security_group.chef-ha-frontend.id}","${aws_security_group.chef-ha-ssh.id}"]
  key_name      = "${var.aws_key_name}"
  tags          = {
    Name        = "${format("%s-%03d.%s", var.fe_hostname, count.index + 1, var.domain)}"
    Description = "${var.tag_description}"
  }
  root_block_device {
    delete_on_termination = "${var.root_delete_termination}"
    volume_size = "${lookup(var.root_volume_size, "frontend")}"
    volume_type = "${lookup(var.root_volume_type, "frontend")}"
    iops        = "${lookup(var.root_volume_size, "frontend") * lookup(var.root_volume_type, "fe_xiops")}"
  }
  connection {
    host        = "${self.public_ip}"
    user        = "${lookup(var.ami_usermap, var.ami_os)}"
    private_key = "${var.aws_private_key_file}"
  }
  # Setup
  provisioner "remote-exec" {
    script = "${path.module}/files/disable_firewall.sh"
  }
}

Debug Output

https://gist.github.com/anonymous/53ceef2dfdc20e2db0757dd734b0b46a

Panic Output

N/A

Expected Behavior

Any value provided to iops should result in the same error of being invalid when added to non-io1 volume types; OR (preferred) ignore and exclude the iops parameter on non-io1 volume types.

Actual Behavior

A value of 0 for iops results in successful processing. Any other value results in an error

Steps to Reproduce

1. terraform apply

Important Factoids

Running on AWS; no other specifics

References

N/A

[mengesb]

Further research seems to show that #4146 may apply here

[mengesb]

It looks like the change there somewhat applies; setting to 0 does produce the PRs described behavior; however I would have to do some math, or a lookup against disk types for static IOPS values. However, no matter the value, if the disk isn't an io1 disk, it should then implicitly set the IOPS to 0; forcing the compliance.

Doing this for all EBS types (additional and root_block_device) I think would be better operation.

[mengesb]

I made the following changes:

diff --git i/builtin/providers/aws/resource_aws_instance.go w/builtin/providers/aws/resource_aws_instance.go
index 7681d6e..bfa76bc 100644
--- i/builtin/providers/aws/resource_aws_instance.go
+++ w/builtin/providers/aws/resource_aws_instance.go
@@ -931,7 +931,12 @@ func readBlockDeviceMappingsFromConfig(
                                ebs.VolumeType = aws.String(v)
                        }

-                       if v, ok := bd["iops"].(int); ok && v > 0 {
+                       if v, ok := bd["iops"].(int); ok && v > 0 && *ebs.VolumeType == "io1" {
+                               // Only set the iops attribute if the volume type is io1. Setting otherwise
+                               // can trigger a refresh/plan loop based on the computed value that is given
+                               // from AWS, and prevent us from specifying 0 as a valid iops.
+                               //   See https://github.com/hashicorp/terraform/pull/4146
+                               //   See https://github.com/hashicorp/terraform/issues/7765
                                ebs.Iops = aws.Int64(int64(v))
                        }

While it does bypass the IOPS issue I was considering an warning message (not sure this is the right place given the lack of this sort of thing here). Ideally I would like the overridden value of 0 to show in the provisioning output (during the apply or plan phases, it still shows the IOPS value if supplied; which is ignored), but at minimum the log message might be all that's required.

[mengesb]

In builtin/providers/aws/resource_aws_ebs_volume.go

232   if volume.VolumeType != nil && *volume.VolumeType == "io1" {
233     // Only set the iops attribute if the volume type is io1. Setting otherwise
234     // can trigger a refresh/plan loop based on the computed value that is given
235     // from AWS, and prevent us from specifying 0 as a valid iops.
236     //   See https://github.com/hashicorp/terraform/pull/4146
237     if volume.Iops != nil {
238       d.Set("iops", *volume.Iops)
239     }
240   }

I'm guessing that this only applies to a resource aws_ebs_volume as opposed to the aws_instance type. Looks like some of this logic needs to be transported over to aws_instance

[mengesb]

The above change results in a complete ignoring of IOPS when the instance doesn't have an io1 disk type. Should warnings be emitted? is there a better pace to do this? Should I trim out the block comment?

[kwilczynski]

@mengesb a warning about an incompatible option-to-volume-type would be a nice touch for the user, so that they might spot something potentially wrong in their template. Might save them some troubleshooting time, etc.

[mengesb]

ah HA! got it!

2016/07/22 10:57:55 [DEBUG] plugin: terraform: aws-provider (internal) 2016/07/22 10:57:55 [WARN] iops attribute only valid with `io1` volume type.

[mengesb]

so that message can be customized of course, but I did get the error reporting; only question now would be how to show that in the plan... or if that's even worth it.

[mengesb]

If someone would comment about this suggested change that'd be great, but otherwise I might open a PR with this as-is.

diff --git i/builtin/providers/aws/resource_aws_instance.go w/builtin/providers/aws/resource_aws_instance.go
index 7681d6e..aa248bf 100644
--- i/builtin/providers/aws/resource_aws_instance.go
+++ w/builtin/providers/aws/resource_aws_instance.go
@@ -931,8 +931,16 @@ func readBlockDeviceMappingsFromConfig(
                                ebs.VolumeType = aws.String(v)
                        }

-                       if v, ok := bd["iops"].(int); ok && v > 0 {
+                       if v, ok := bd["iops"].(int); ok && v > 0 && *ebs.VolumeType == "io1" {
+                               // Only set the iops attribute if the volume type is io1. Setting otherwise
+                               // can trigger a refresh/plan loop based on the computed value that is given
+                               // from AWS, and prevent us from specifying 0 as a valid iops.
+                               //   See https://github.com/hashicorp/terraform/pull/4146
+                               //   See https://github.com/hashicorp/terraform/issues/7765
                                ebs.Iops = aws.Int64(int64(v))
+                       } else if v, ok := bd["iops"].(int); ok && v > 0 && *ebs.VolumeType != "io1" {
+                               // Message user about incompatibility
+                               log.Printf("[WARN] iops attribute only valid with 'io1' volume type.")
                        }

                        if dn, err := fetchRootDeviceName(d.Get("ami").(string), conn); err == nil {

[kwilczynski]

@mengesb if you open a PR, then it is going to be easier to comment on the change. If there are any tests (see: resource_aws_instance_test.go, make sure that they pass and amend accordingly so that you validate this scenario in a test. Otherwise, looks very good! 🍰

P.S. You can re-use warning message from here: resource_aws_ebs_volume.go#L104. Just for consistency.

[stack72]

Closed via #7783

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.