resource/aws_spot_instance_request: Fix being able to stop requests #1986

tomelliff · 2017-10-20T16:49:45Z

As mentioned in #1735 (comment) this was failing locally for me.

Fixes an issue when setting the instance interruption behaviour to anything other than terminate (hibernate has since been added but not included in this PR) and adds a test to cover it.

tomelliff · 2017-10-23T09:40:57Z

I was considering adding a simple validate function on the instance interruption behaviours as well (because plan time failures are always nicer than apply time failures when you typo something) and then spotted that we have instance_interruption_behavior in the docs but it's instance_interruption_behaviour in the actual code.

The British part of me wants to change the docs to instance_interruption_behaviour but it doesn't align with the AWS API and most things want US spelling instead. Unfortunately, changing the key in the schema will then technically be a breaking change. However, it doesn't currently work so I'm not sure if people want to consider it a non breaking change and I can just change it all to behavior? @radeksimko any thoughts?

US Spelling to match the AWS API and the docs. While this is technically a breaking change this didn't work anyway. See discussion at hashicorp#1986 (comment)

tomelliff · 2017-10-27T16:48:31Z

I decided to go ahead with the "breaking" change considering it didn't work anyway this seems like the best solution instead of leaving it as is and updating docs or adding the US spelling and deprecating instance_interruption_behaviour.

tomelliff · 2017-11-29T10:36:43Z

It might be worth deprecating the aws_spot_instance_request resource in light of https://aws.amazon.com/blogs/aws/amazon-ec2-update-streamlined-access-to-spot-capacity-smooth-price-changes-instance-hibernation/ and as mentioned in #1243

Also if the validator is kept from here we need to add whatever the enum value is for hibernate (probably that) now that's been added: https://aws.amazon.com/about-aws/whats-new/2017/11/amazon-ec2-spot-lets-you-pause-and-resume-your-workloads/

tomelliff · 2018-03-12T11:05:38Z

I've migrated away from spot instance requests directly to only using spot instances in ASGs so have no real need for this anymore but it might still be useful for others if anyone is happy to review this pull request?

I still feel like the aws_spot_instance_request might want deprecating in favour of refactoring the aws_instance resource to use launch templates so that resource can more directly create spot instance requests for a much simpler API but that's going to be a fair bit more work and I'm not sure how many people are using this resource as is.

billyfoss · 2018-04-07T14:57:46Z

I would like to see this fixed. Between the spelling mismatch in documentation and being consistent with the AWS API, I agree with removing the 'u'.
I agree that long term, the focus should be on directly using the spot support in aws_instance, but if a little work will get this working, I'd prefer to deprecate the usage with working code.

I've merged this PR into my fork and have added a little code for the hibernate option. I tested the 'stop' support and it is working for my usage. I have not tested the 'hibernate' option.

bflad

Let's leave the attribute deprecation and renaming for another PR so we can get the actual fix in. 👍

bflad · 2018-04-09T12:04:48Z

aws/resource_aws_spot_instance_request.go

@@ -78,11 +78,12 @@ func resourceAwsSpotInstanceRequest() *schema.Resource {
 				Optional: true,
 				ForceNew: true,
 			}
-			s["instance_interruption_behaviour"] = &schema.Schema{


Can we please save this breaking change for another PR? It'll need to be handled in the 2.0.0 upgrade when that occurs. In the meantime, we should deprecate the existing attribute (but that change can be made separately).

bflad · 2018-04-09T12:05:31Z

aws/resource_aws_spot_instance_request.go

+			s["instance_interruption_behavior"] = &schema.Schema{
+				Type:         schema.TypeString,
+				Optional:     true,
+				Default:      "terminate",


Nitpick: SDK provided constant available: ec2.InstanceInterruptionBehaviorTerminate

bflad · 2018-04-09T12:06:53Z

aws/resource_aws_spot_instance_request.go

+				Optional:     true,
+				Default:      "terminate",
+				ForceNew:     true,
+				ValidateFunc: validateInstanceInterruptionBehavior,


Nitpick: Custom function can be removed for:

ValidateFunc: validation.StringInSlice([]string{ ec2.InstanceInterruptionBehaviorStop, ec2.InstanceInterruptionBehaviorTerminate, }, false),

There is also ec2.InstanceInterruptionBehaviorHibernate, available.

tomelliff · 2018-04-09T18:51:53Z

Still want the change separating when it's currently broken anyway? I'd definitely agree if it was working but it has never worked so I don't think we have to worry about breaking changes here and it's then simpler to fix both things in one go. Or separate them and merge the "breaking" change first without incrementing the major version and then merge the fix.

Agree with all the other changes requested, good spots so thanks for that. Happy to add the hibernate that was added since I raised this PR a while back too.

bflad · 2018-04-12T19:59:13Z

aws/resource_aws_spot_instance_request.go

@@ -132,6 +132,11 @@ func resourceAwsSpotInstanceRequestCreate(d *schema.ResourceData, meta interface
 		spotOpts.LaunchGroup = aws.String(v.(string))
 	}

+	// Placement GroupName can only be specified when instanceInterruptionBehavior is not set to 'stop'
+	if v, exists := d.GetOkExists("instance_interruption_behavior"); v == "terminate" || !exists {


I think this needs to be v.(string) == ec2.InstanceInterruptionBehaviorTerminate

bflad · 2018-04-12T20:01:47Z

Sorry if I'm being dense, but I don't understand where this is coming from:

but it has never worked

It looks like the attribute was passed in just fine before with the old (incorrectly documented) name. Plenty of people could have changed their configurations to use the different spelling and it should have worked just fine for them, unless I'm missing something.

bflad · 2018-04-12T20:04:31Z

For reference, people could have found this issue and adjusted their configurations: #2670

bflad · 2018-04-12T20:13:53Z

PR to fix the documentation: #4195

tomelliff · 2018-04-13T08:35:46Z

@bflad it's been a long time since I checked but as mentioned in the comment at https://github.com/terraform-providers/terraform-provider-aws/pull/1986/files#diff-f369c2449430a1386c70cf98c181c5a3R135 and this comment on the initial PR that added it #1735 (comment) it doesn't work at all with setting the stop behaviour because the AWS API returns the following:

Error requesting spot instances: InvalidParameterCombination: The parameter GroupName within placement information cannot be specified when instanceInterruptionBehavior is set to 'stop'

I can check again now though to see if the API has changed since but I'd be surprised if that is the case.

tomelliff · 2018-04-13T08:37:18Z

I guess technically someone might be using it to directly specify they want it to terminate instead of leaving it as the default which I hadn't considered until now. In that case then yeah I guess this would be a breaking change.

HIbernating instances requires c3/c4/c5/m4/m5/r3/r4 instance types so we need to bump the instance size to a c5.large which is the cheapest instance out of those families.

tomelliff · 2018-04-14T19:08:36Z

@bflad I've removed the breaking change, rebased on master and squashed my commits. Thanks for the review and comments earlier, and sorry it's taken a while to find time to address the feedback.

I've also added a test for hibernating instances and expanded the validation for that to work. If you'd rather I can split that commit out to a different PR to make the PR as small as possible.

Did you also want me to raise a PR with the breaking change to move things over to instance_interruption_behavior that can be merged when 2.0 is ready to be released?

piontec · 2018-04-17T09:53:51Z

Hi!
We're hitting this issue as well - we can't start spot instance with interruption_behavior = stop at all. Can you please review this PR @bflad ?

bflad

Thanks for the update @tomelliff! LGTM 🚀 We can add apply-time or plan-time validation of the termination case if necessary later.

9 tests passed (all tests)
=== RUN   TestAccAWSSpotInstanceRequest_basic
--- PASS: TestAccAWSSpotInstanceRequest_basic (98.17s)
=== RUN   TestAccAWSSpotInstanceRequest_withLaunchGroup
--- PASS: TestAccAWSSpotInstanceRequest_withLaunchGroup (110.07s)
=== RUN   TestAccAWSSpotInstanceRequest_withBlockDuration
--- PASS: TestAccAWSSpotInstanceRequest_withBlockDuration (117.99s)
=== RUN   TestAccAWSSpotInstanceRequest_SubnetAndSGAndPublicIpAddress
--- PASS: TestAccAWSSpotInstanceRequest_SubnetAndSGAndPublicIpAddress (199.24s)
=== RUN   TestAccAWSSpotInstanceRequest_NetworkInterfaceAttributes
--- PASS: TestAccAWSSpotInstanceRequest_NetworkInterfaceAttributes (212.57s)
=== RUN   TestAccAWSSpotInstanceRequest_vpc
--- PASS: TestAccAWSSpotInstanceRequest_vpc (213.23s)
=== RUN   TestAccAWSSpotInstanceRequestInterruptHibernate
--- PASS: TestAccAWSSpotInstanceRequestInterruptHibernate (275.24s)
=== RUN   TestAccAWSSpotInstanceRequestInterruptStop
--- PASS: TestAccAWSSpotInstanceRequestInterruptStop (277.36s)
=== RUN   TestAccAWSSpotInstanceRequest_getPasswordData
--- PASS: TestAccAWSSpotInstanceRequest_getPasswordData (305.40s)

tomelliff · 2018-04-17T16:53:49Z

@bflad thanks again for the review and merge. I'm not sure I follow about the validation as there is validation for all 3 behaviours right now (terminate, stop, hibernate) and refactored out of a stand alone validation function as per your feedback. Am I missing something there? I guess there's not an explicit terminate acceptance test if that's what you meant?

And did you want me to raise the breaking change PR to move things to use instance_interruption_behavior?

bflad · 2018-04-17T17:16:16Z

I guess there's not an explicit terminate acceptance test if that's what you meant?

We can provide an error (and acceptance test that error) if there is an invalid configuration (defining placement_group and a bad instance_interruption_behaviour) rather than silently skipping the invalid part of the configuration.

And did you want me to raise the breaking change PR to move things to use instance_interruption_behavior?

If you so desire. The first PR should support both the old and new attribute names (using ConflictsWith between them).

bflad · 2018-04-18T18:28:51Z

This has been released in version 1.15.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-06T17:40:49Z

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!

tomelliff force-pushed the stop-spot-instance-stop-spot-stop branch from 40d6708 to 11584aa Compare October 20, 2017 17:02

radeksimko added the bug Addresses a defect in current functionality. label Oct 21, 2017

tomelliff changed the title ~~WIP: Fix being able to stop spot instance requests~~ Fix being able to stop spot instance requests Oct 27, 2017

radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017

radeksimko added the upstream-terraform Addresses functionality related to the Terraform core binary. label Dec 5, 2017

radeksimko added the service/ec2 Issues and PRs that pertain to the ec2 service. label Jan 16, 2018

radeksimko changed the title ~~Fix being able to stop spot instance requests~~ resource/aws_spot_instance_request: Fix being able to stop requests Jan 16, 2018

billyfoss mentioned this pull request Apr 7, 2018

aws_spot_instance_request fails when instance_interruption_behaviour is set to stop #2023

Closed

bflad suggested changes Apr 9, 2018

View reviewed changes

bflad added waiting-response Maintainers are waiting on response from community or contributor. and removed upstream-terraform Addresses functionality related to the Terraform core binary. labels Apr 9, 2018

bflad reviewed Apr 12, 2018

View reviewed changes

ghost added size/L Managed by automation to categorize the size of a PR. size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Apr 14, 2018

tomelliff force-pushed the stop-spot-instance-stop-spot-stop branch from b7f61f2 to 47c2b6d Compare April 14, 2018 18:40

ghost added the size/M Managed by automation to categorize the size of a PR. label Apr 14, 2018

tomelliff force-pushed the stop-spot-instance-stop-spot-stop branch from 47c2b6d to 807f7fd Compare April 14, 2018 18:42

ghost added the size/M Managed by automation to categorize the size of a PR. label Apr 14, 2018

Fix spot instance stop interrupt behaviour

589c58d

tomelliff force-pushed the stop-spot-instance-stop-spot-stop branch from 807f7fd to 589c58d Compare April 14, 2018 18:50

ghost added the size/M Managed by automation to categorize the size of a PR. label Apr 14, 2018

Allow hibernation of spot instances when interrupted

b2f42c9

HIbernating instances requires c3/c4/c5/m4/m5/r3/r4 instance types so we need to bump the instance size to a c5.large which is the cheapest instance out of those families.

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Apr 17, 2018

bflad added this to the v1.15.0 milestone Apr 17, 2018

bflad approved these changes Apr 17, 2018

View reviewed changes

bflad merged commit 477ddd6 into hashicorp:master Apr 17, 2018

bflad added a commit that referenced this pull request Apr 17, 2018

Update CHANGELOG for #1986

fd0d5e3

haokanga mentioned this pull request Jun 13, 2018

instance_interruption_behavior misspelled in spot_fleet_request #4827

Closed

tomelliff mentioned this pull request Jan 1, 2019

Rename instance_interruption_behaviour attribute in v2.0.0 #7006

Closed

ghost locked and limited conversation to collaborators Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_spot_instance_request: Fix being able to stop requests #1986

resource/aws_spot_instance_request: Fix being able to stop requests #1986

tomelliff commented Oct 20, 2017 •

edited

Loading

tomelliff commented Oct 23, 2017

tomelliff commented Oct 27, 2017

tomelliff commented Nov 29, 2017

tomelliff commented Mar 12, 2018

billyfoss commented Apr 7, 2018 •

edited

Loading

bflad left a comment

bflad Apr 9, 2018

bflad Apr 9, 2018

bflad Apr 9, 2018

bflad Apr 9, 2018

tomelliff commented Apr 9, 2018 •

edited

Loading

bflad Apr 12, 2018

bflad commented Apr 12, 2018

bflad commented Apr 12, 2018

bflad commented Apr 12, 2018

tomelliff commented Apr 13, 2018

tomelliff commented Apr 13, 2018

tomelliff commented Apr 14, 2018

piontec commented Apr 17, 2018

bflad left a comment

tomelliff commented Apr 17, 2018 •

edited

Loading

bflad commented Apr 17, 2018

bflad commented Apr 18, 2018

ghost commented Apr 6, 2020

resource/aws_spot_instance_request: Fix being able to stop requests #1986

resource/aws_spot_instance_request: Fix being able to stop requests #1986

Conversation

tomelliff commented Oct 20, 2017 • edited Loading

tomelliff commented Oct 23, 2017

tomelliff commented Oct 27, 2017

tomelliff commented Nov 29, 2017

tomelliff commented Mar 12, 2018

billyfoss commented Apr 7, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

bflad Apr 9, 2018

Choose a reason for hiding this comment

bflad Apr 9, 2018

Choose a reason for hiding this comment

bflad Apr 9, 2018

Choose a reason for hiding this comment

bflad Apr 9, 2018

Choose a reason for hiding this comment

tomelliff commented Apr 9, 2018 • edited Loading

bflad Apr 12, 2018

Choose a reason for hiding this comment

bflad commented Apr 12, 2018

bflad commented Apr 12, 2018

bflad commented Apr 12, 2018

tomelliff commented Apr 13, 2018

tomelliff commented Apr 13, 2018

tomelliff commented Apr 14, 2018

piontec commented Apr 17, 2018

bflad left a comment

Choose a reason for hiding this comment

tomelliff commented Apr 17, 2018 • edited Loading

bflad commented Apr 17, 2018

bflad commented Apr 18, 2018

ghost commented Apr 6, 2020

tomelliff commented Oct 20, 2017 •

edited

Loading

billyfoss commented Apr 7, 2018 •

edited

Loading

tomelliff commented Apr 9, 2018 •

edited

Loading

tomelliff commented Apr 17, 2018 •

edited

Loading