Skip to content
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.

Sign up for GitHub

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

Jump to bottom

provider/aws/aws_instance: add instance_initiated_shutdown_behavior (supersedes 2779) #2887

Merged
merged 4 commits into from
Aug 18, 2015
Merged

provider/aws/aws_instance: add instance_initiated_shutdown_behavior (supersedes 2779) #2887

merged 4 commits into from
Aug 18, 2015

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Jul 29, 2015

Supersedes #2779 by merging master so the original contributor doesn't have to

tamsky and others added 2 commits July 18, 2015 09:45
@tamsky
provider/aws/aws_instance: add new argument `instance_initiated_shutd…
521451d 
…own_behavior`,

accepts string values of 'stop' or 'terminate'.

Signed-off-by: Marc Tamsky <tamsky@users.noreply.github.com>
@catsby
Merge branch 'master' into pr-2779
3057221 
* master: (86 commits)
  providers/google: Fix reading account_file path
  providers/google: Fix error appending
  providers/google: Return if we could parse JSON
  providers/google: Change account_file to JSON
  providers/google: Default account_file* to empty
  providers/google: Add account_file/account_file_contents ConflictsWith
  providers/google: Document account_file_contents
  providers/google: Use account_file_contents if provided
  providers/google: Add account_file_contents to provider
  Update CHANGELOG.md
  Update CHANGELOG.md
  use d.Id()
  Update CHANGELOG.md
  Update CHANGELOG.md
  scripts: change website_push to push from HEAD
  update analytics
  core: fix crash on provider warning
  provider/aws: Update source to comply with upstream breaking change
  Update CHANGELOG.
  provider/aws: Fix issue with IAM Server Certificates and Chains
  ...
@catsby catsby mentioned this pull request Jul 29, 2015
Closed
@catsby
Copy link
Contributor Author

catsby commented Jul 29, 2015

Hey @tamsky – I tried running the TestAccAWSInstance_basic basic test after this addition, and got a peculiar error:

* aws_instance.foo: Error launching source instance: InvalidParameterCombination: The attribute instanceInitiatedShutdownBehavior can only be used for elastic images.

Did you ever encounter that error by chance? I didn't see any mention of Elastic Images in the documentation, and this tested out fine with a different AMI than the one in the test.

The test uses AMI ami-4fccb37f, which I think is from Ubuntu..

I used AMI ami-dfc39aef with no issue (an Amazon Linux AMI)

@catsby catsby added enhancement waiting-response An issue/pull request is waiting for a response from the community provider/aws labels Jul 29, 2015
@tamsky
Copy link
Contributor

tamsky commented Aug 2, 2015

@catsby I have not seen that error. Others have, but without a clear origin or cause: puppetlabs-toy-chest/puppetlabs-aws#176

@catsby
Copy link
Contributor Author

catsby commented Aug 4, 2015

I opened a support case with AWS to inquire, and it turns out the AMI I was using is Instance-Store backed, thus not "elastic". Instance-store are always terminated at shutdown, so this behavior is impossible to set for them.

From what I can see we can't simply have a default, because that default isn't good for a class of instances. Even though that's what that API says is the default...

I've adjusted the PR to no longer have a default, and make this value be optional. This way, we don't try to send that default to an instance type that doesn't support it. Unfortunately now you can't simply remove the attribute from the config after you set it, or at least, doing so won't "restore the default".

E.g., an instance with terminate:

resource "aws_instance" "example" {
  ami = "ami-21f78e11"
  instance_initiated_shutdown_behavior = "terminate"
}

As it stands now with 8d5fe93, removing instance_initiated_shutdown_behavior will not restore the instance to stop. You have to manually supply stop, or that instance will continue with terminate.

@tamsky / @phinze let me know if you think that behavior is OK, or if you have other ideas on how to support instance_initiated_shutdown_behavior given the information above regarding instance-store instances.

@tamsky
Copy link
Contributor

tamsky commented Aug 4, 2015

I agree not having a default is ok.
Also agree it is correct that simply removing the attribute should not re-default the setting.

This behavior means the docs I wrote will need a bump.

Is it possible to also improve the user-visible error message?

When we see the error text The attribute instanceInitiatedShutdownBehavior can only be used for elastic images. it would be nice to have a tiny bit of explanatory error text, like: "Instance-store AMIs only allow terminate for this setting."

phinze
phinze reviewed Aug 12, 2015
View reviewed changes
website/source/docs/providers/aws/r/instance.html.markdown
@@ -36,6 +36,8 @@ The following arguments are supported:
EBS-optimized.
* `disable_api_termination` - (Optional) If true, enables [EC2 Instance
Termination Protection](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/terminating-instances.html#Using_ChangingDisableAPITermination)
* `instance_initiated_shutdown_behavior` - (Optional) [Shutdown Behavior](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/terminating-instances.html#Using_ChangingInstanceInitiatedShutdownBehavior) Can be `"stop"`
or `"terminate"`. (Default: `"stop"`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be updated to something like:

Defaults to stop for EBS-backed instances and terminate for instance-store instances. Cannot be set on instance-store instances.

@phinze
Copy link
Contributor

phinze commented Aug 12, 2015

With a wee docs tweak this looks good to go!

@catsby
Merge branch 'master' into f-aws-pr-2779
fde2a40 
* master: (84 commits)
  provider/aws: Update to aws-sdk 0.9.0 rc1
  use name instead of id  - launch configs use the name and not ID
  Fix typo on heroku_cert example
  provider/aws: add value into ELB name validation message
  tests: fix missed test update from last merge
  update prevent_destroy error message
  Update CHANGELOG.md
  Update CHANGELOG.md
  providers/aws: Update Launch Config. docs to detail naming and lifecycle recommendation
  release: cleanup after v0.6.3
  v0.6.3
  Update CHANGELOG.md
  core: fix deadlock when dependable node replaced with non-dependable one
  tests: extract deadlock checking test helper
  core: log every 5s while waiting for dependencies
  Fixed indentation in a code sample
  state/remote/s3: match with upstream changes
  provider/aws: match with upstream changes
  google: Add example of two-tier app
  Updating Launch Config Docs for Name attribute
  ...
catsby added a commit that referenced this pull request Aug 18, 2015
@catsby
Merge pull request #2887 from hashicorp/f-aws-pr-2779
79bd3a3 
provider/aws/aws_instance: add instance_initiated_shutdown_behavior (supersedes 2779)
@catsby catsby merged commit 79bd3a3 into master Aug 18, 2015
@phinze phinze deleted the f-aws-pr-2779 branch September 4, 2015 23:23
@ghost
Copy link

ghost commented May 1, 2020

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.

@ghost ghost locked and limited conversation to collaborators May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@catsby @tamsky @phinze