Skip to content
This repository has been archived by the owner on Feb 5, 2020. It is now read-only.

Fix terraform resource churn #1826

Merged
merged 2 commits into from
Sep 8, 2017

Conversation

mrwacky42
Copy link
Contributor

@mrwacky42 mrwacky42 commented Sep 1, 2017

Possibly related to this issue with the Terraform AWS provider:
hashicorp/terraform-provider-aws#1521

Terraform or the AWS API report 0 IOPS for gp2 volumes after creation.
If I create a gp2 volume with these modules, every subsequent run of
Terraform will attempt to destroy and recreate the LC in order to make
IOPs match. This change sets IOPS to 0 for non-io1 type volumes.

Since the number of IOPS for gp2 volume is based on the size, Terraform
arguably should only accept the iops parameter for io1 volumes.

Interestingly, for the etcd nodes, this problem does not occur because
the Terraform state shows 100 IOPS even for gp2 volumes.

@coreosbot
Copy link

Can one of the admins verify this patch?

2 similar comments
@coreosbot
Copy link

Can one of the admins verify this patch?

@coreosbot
Copy link

Can one of the admins verify this patch?

@squat
Copy link
Contributor

squat commented Sep 6, 2017

@mrwacky42 this generally looks pretty good but seems fragile. Why not update the etcd node launch config as well? And what happens when that node's IOPs in Terraform state is no longer 100?

@mrwacky42
Copy link
Contributor Author

@squat - I based this change on the current behavior of Terraform and AWS.

The etcd nodes are not using autoscaling, so there is no launch configuration, just aws_instance resources.

For aws_instance resources, Terraform state currently reports 100 iops (for non-io1 EBS volumes). For aws_launch_configuration resources, Terraform state currently reports 0 iops.

There is a PR attached to the original issue I linked above to fix the aws_instance to behave consistent with aws_launch_configuration: hashicorp/terraform-provider-aws#1573

@squat
Copy link
Contributor

squat commented Sep 6, 2017

thank I see now. From the issue you linked, it looks like even settings iops to 100 for gp2 volumes will not always work. @fraenkel writes that for large volume sizes the iops can be reported to be 150, so we may still have resource churn.

@squat
Copy link
Contributor

squat commented Sep 7, 2017

As @mrwacky42 correctly, notes, iops defaults to 0 for gp2 volumes in launch configurations, but defaults to size*3 for instance resources. This explains why @simnalamburt was seeing the issue in #1397 (comment) for only the volumes used in launch configurations but not the volumes used etcd nodes. This PR partially undoes #1397. We should merge #953 next to ensure that iops is set correctly for volumes in instance resources.

cc @s-urbaniak @alexsomesan

squat
squat previously approved these changes Sep 7, 2017
@squat
Copy link
Contributor

squat commented Sep 7, 2017

closing and reopening to trigger tests

@squat squat closed this Sep 7, 2017
@squat squat reopened this Sep 7, 2017
@coreos coreos deleted a comment from coreosbot Sep 7, 2017
@coreos coreos deleted a comment from coreosbot Sep 7, 2017
@coreos coreos deleted a comment from coreosbot Sep 7, 2017
@squat
Copy link
Contributor

squat commented Sep 7, 2017

@mrwacky42 tests are failing with:

misformatted files (run 'terraform fmt .' to fix): platforms/aws/variables.tf

can you please fix the formatting?

@mrwacky42
Copy link
Contributor Author

@squat - Maybe I shouldn't have rebased. Did I confuse Jenkins?

@squat
Copy link
Contributor

squat commented Sep 7, 2017

@mrwacky42 no, rebasing was good 👍. Linting is still failing. Can you please make docs && make examples and commit the result?

Possibly related to this issue with the Terraform AWS provider:
hashicorp/terraform-provider-aws#1521

Terraform or the AWS API report `0` IOPS for `gp2` volumes after creation.
If I create a `gp2` volume with these modules, every subsequent run of
Terraform will attempt to destroy and recreate the LC in order to make
IOPs match. This change sets IOPS to 0 for non-`io1` type volumes.

Since the number of IOPS for `gp2` volume is based on the size, Terraform
arguably should only accept the `iops` parameter for `io1` volumes.

Interestingly, for the etcd nodes, this problem does not occur because
the Terraform state shows 100 IOPS even for `gp2` volumes.
Also apply `terraform fmt`.
@mrwacky42
Copy link
Contributor Author

I cheated and ran make docs examples, then rebased again.

😀

@squat
Copy link
Contributor

squat commented Sep 8, 2017

@mrwacky42 had an unrelated flake in one of the azure tests. Merging!

@squat squat merged commit 9f3d0e0 into coreos:master Sep 8, 2017
@mrwacky42 mrwacky42 deleted the fix-terraform-resource-churn branch September 21, 2017 00:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants