Read iops only when volume type is io1 #1573

fraenkel · 2017-09-01T19:37:38Z

Iops can only be specified when the volume type is io1. The current code
handles that.
When the iops are read, it should only record the value when the volume
type is io1 otherwise it can conflict with what is specified but never
applied.

fixes #1521

Ninir

Hey @fraenkel

Thanks for the suggested change here.
Asked for a small change, and would like to get your point on my second comment.

Thanks!

Ninir · 2017-09-06T17:12:23Z

aws/resource_aws_instance.go

-		}
-		if vol.Iops != nil {
-			bd["iops"] = *vol.Iops
+			if "io1" == strings.ToLower(*vol.VolumeType) && vol.Iops != nil {


Can you use the constant VolumeTypeIo1 defined in https://raw.githubusercontent.com/terraform-providers/terraform-provider-aws/master/vendor/github.com/aws/aws-sdk-go/service/ec2/api.go instead of io1?

Also, I would prefer to write the state in both io1 & gp2 cases: when gp2 is used, the iops are 3 times the volume size with a minimum of 100gb. So if you have a 34GB volume, it would result in 104 iops.
I think it's worth keeping that information for people wanting to know it.

Thoughts?

@Ninir Regarding your second comment, a problem I see with keeping state for gp2 volume iops is that anybody managing EBS volumes with Terraform would have to keep track of the iops multiplier and update 2 values when adding/changing gp2 volumes.
IMHO, there's little discernible benefit to having to manage this number unless it is io1.

It might also create confusion for users who think they could change iops on a gp2 volume when they cannot.

Updated to use the type and perform the calculation for gp2.

Thanks for that @fraenkel!

@mrwacky42 Could you clarify the part about anybody managing EBS volumes with Terraform would have to keep track of the iops multiplier and update 2 values when adding/changing gp2 volumes please?
Your point seems interesting, however not sure to get it right! 😅

Thanks!

@Ninir -
The current expectation is that I do not have to specify iops for gp2 volumes because it is meaningless. I worry that mapping values for gp2 volume iops would force me to change my Terraform config from:

resource "aws_instance" "foo" { ... root_block_device { volume_type = "gp2" volume_size = "500" } }

to include iops in order to avoid Terraform thinking the resource is changed on every run:

resource "aws_instance" "foo" { ... root_block_device { volume_type = "gp2" volume_size = "500" iops = "1500" } }

However, I could be misunderstanding the how the code change affects everything, so feel free to disregard!

See also the PR and issue mentioned in the comments:
https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_instance.go#L1388-L1394

fraenkel · 2017-09-06T23:32:03Z

Sorry, I need to redo this. I realized I did this all backwards now.

mrwacky42 · 2017-09-07T17:10:06Z

See also: hashicorp/terraform#7765

fraenkel · 2017-09-07T18:47:47Z

This doesn't actually fix anything.... I am trying to see if it is at all possible.

fraenkel · 2017-09-07T19:51:04Z

Ok, I found a way to make this work. I wish there was a good way to test this.
My test is to do an apply followed by a plan.

Ninir · 2017-09-18T21:26:58Z

aws/resource_aws_instance.go

+								vt := k[:i+1] + "volume_type"
+								v := d.Get(vt).(string)
+								return strings.ToLower(v) != ec2.VolumeTypeIo1
+							},


Can you make this a function so that both DiffSuppressFunc blocks can use it?

Thanks!

@Ninir Done

Iops can only be specified with io1. Only perform the diff when the volume_type is io1.

Ninir

Hey @fraenkel

LGTM !

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSInstance_'           
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSInstance_ -timeout 120m
=== RUN   TestAccAWSInstance_importBasic
--- PASS: TestAccAWSInstance_importBasic (146.58s)
=== RUN   TestAccAWSInstance_importInDefaultVpc
--- PASS: TestAccAWSInstance_importInDefaultVpc (145.48s)
=== RUN   TestAccAWSInstance_importInEc2Classic
--- SKIP: TestAccAWSInstance_importInEc2Classic (2.25s)
	provider_test.go:69: This test can only run in EC2 Classic, platforms available in us-west-2: ["VPC"]
=== RUN   TestAccAWSInstance_basic
--- PASS: TestAccAWSInstance_basic (204.75s)
=== RUN   TestAccAWSInstance_userDataBase64
--- PASS: TestAccAWSInstance_userDataBase64 (139.13s)
=== RUN   TestAccAWSInstance_GP2IopsDevice
--- PASS: TestAccAWSInstance_GP2IopsDevice (94.64s)
=== RUN   TestAccAWSInstance_blockDevices
--- PASS: TestAccAWSInstance_blockDevices (100.26s)
=== RUN   TestAccAWSInstance_rootInstanceStore
--- PASS: TestAccAWSInstance_rootInstanceStore (109.34s)
=== RUN   TestAccAWSInstance_noAMIEphemeralDevices
--- PASS: TestAccAWSInstance_noAMIEphemeralDevices (69.03s)
=== RUN   TestAccAWSInstance_sourceDestCheck
--- PASS: TestAccAWSInstance_sourceDestCheck (236.43s)
=== RUN   TestAccAWSInstance_disableApiTermination
--- PASS: TestAccAWSInstance_disableApiTermination (201.57s)
=== RUN   TestAccAWSInstance_vpc
--- PASS: TestAccAWSInstance_vpc (125.59s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCount
--- PASS: TestAccAWSInstance_ipv6_supportAddressCount (123.10s)
=== RUN   TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError
--- PASS: TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError (32.58s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCountWithIpv4
--- PASS: TestAccAWSInstance_ipv6_supportAddressCountWithIpv4 (150.79s)
=== RUN   TestAccAWSInstance_multipleRegions
--- PASS: TestAccAWSInstance_multipleRegions (142.35s)
=== RUN   TestAccAWSInstance_NetworkInstanceSecurityGroups
--- PASS: TestAccAWSInstance_NetworkInstanceSecurityGroups (150.36s)
=== RUN   TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (143.97s)
=== RUN   TestAccAWSInstance_tags
--- PASS: TestAccAWSInstance_tags (157.77s)
=== RUN   TestAccAWSInstance_volumeTags
--- PASS: TestAccAWSInstance_volumeTags (180.97s)
=== RUN   TestAccAWSInstance_volumeTagsComputed
--- PASS: TestAccAWSInstance_volumeTagsComputed (141.31s)
=== RUN   TestAccAWSInstance_instanceProfileChange
--- PASS: TestAccAWSInstance_instanceProfileChange (161.38s)
=== RUN   TestAccAWSInstance_withIamInstanceProfile
--- PASS: TestAccAWSInstance_withIamInstanceProfile (135.49s)
=== RUN   TestAccAWSInstance_privateIP
--- PASS: TestAccAWSInstance_privateIP (154.36s)
=== RUN   TestAccAWSInstance_associatePublicIPAndPrivateIP
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (140.39s)
=== RUN   TestAccAWSInstance_keyPairCheck
--- PASS: TestAccAWSInstance_keyPairCheck (84.21s)
=== RUN   TestAccAWSInstance_rootBlockDeviceMismatch
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (130.97s)
=== RUN   TestAccAWSInstance_forceNewAndTagsDrift
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (216.95s)
=== RUN   TestAccAWSInstance_changeInstanceType
--- PASS: TestAccAWSInstance_changeInstanceType (175.24s)
=== RUN   TestAccAWSInstance_primaryNetworkInterface
--- PASS: TestAccAWSInstance_primaryNetworkInterface (155.32s)
=== RUN   TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck
--- PASS: TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck (151.76s)
=== RUN   TestAccAWSInstance_addSecondaryInterface
--- PASS: TestAccAWSInstance_addSecondaryInterface (248.56s)
=== RUN   TestAccAWSInstance_addSecurityGroupNetworkInterface
--- PASS: TestAccAWSInstance_addSecurityGroupNetworkInterface (350.60s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPrivate
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (149.95s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPublic
--- PASS: TestAccAWSInstance_associatePublic_defaultPublic (129.93s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPublic
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (130.13s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPrivate
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (139.67s)
=== RUN   TestAccAWSInstance_associatePublic_overridePublic
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (132.82s)
=== RUN   TestAccAWSInstance_associatePublic_overridePrivate
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (173.61s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	5759.656s

Read iops only when volume type is io1

ghost · 2020-04-11T17:17:04Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Sep 6, 2017

mrwacky42 mentioned this pull request Sep 6, 2017

Fix terraform resource churn coreos/tectonic-installer#1826

Merged

Ninir suggested changes Sep 6, 2017

View reviewed changes

fraenkel force-pushed the gp2iops branch from 33ae81c to 9e56388 Compare September 6, 2017 22:58

fraenkel force-pushed the gp2iops branch from 9e56388 to 35a62b6 Compare September 6, 2017 23:47

fraenkel force-pushed the gp2iops branch 2 times, most recently from cbf8e8c to 45f0935 Compare September 7, 2017 22:10

Ninir reviewed Sep 18, 2017

View reviewed changes

Ninir added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 18, 2017

Allow diff of iops when volume_type is io1

3b9f715

Iops can only be specified with io1. Only perform the diff when the volume_type is io1.

fraenkel force-pushed the gp2iops branch from 45f0935 to 3b9f715 Compare September 19, 2017 02:38

Ninir approved these changes Sep 21, 2017

View reviewed changes

Ninir merged commit 5857910 into hashicorp:master Sep 21, 2017

Ninir removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 21, 2017

nbaztec pushed a commit to nbaztec/terraform-provider-aws that referenced this pull request Sep 26, 2017

Merge pull request hashicorp#1573 from fraenkel/gp2iops

6c80c15

Read iops only when volume type is io1

trung mentioned this pull request Sep 27, 2017

resource/aws_instance: Added acceptance test when using encrypted volume & IOPS #1728

Merged

ghost locked and limited conversation to collaborators Apr 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read iops only when volume type is io1 #1573

Read iops only when volume type is io1 #1573

fraenkel commented Sep 1, 2017

Ninir left a comment •

edited

Loading

Ninir Sep 6, 2017

Ninir Sep 6, 2017

mrwacky42 Sep 6, 2017

fraenkel Sep 6, 2017

Ninir Sep 7, 2017

mrwacky42 Sep 7, 2017

fraenkel commented Sep 6, 2017

mrwacky42 commented Sep 7, 2017

fraenkel commented Sep 7, 2017

fraenkel commented Sep 7, 2017

Ninir Sep 18, 2017

fraenkel Sep 19, 2017

Ninir left a comment

ghost commented Apr 11, 2020

Read iops only when volume type is io1 #1573

Read iops only when volume type is io1 #1573

Conversation

fraenkel commented Sep 1, 2017

Ninir left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fraenkel commented Sep 6, 2017

mrwacky42 commented Sep 7, 2017

fraenkel commented Sep 7, 2017

fraenkel commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ninir left a comment

Choose a reason for hiding this comment

ghost commented Apr 11, 2020

Ninir left a comment •

edited

Loading