Spotfleet Launch Template and On-Demand Capacity Support #4866

lossanarch · 2018-06-18T11:30:41Z

Updates the spotfleet_request resource to support the new launch_template resource and on-demand capacity.

Changes proposed in this pull request:

launch_template_configs

The new block looks like this, following the structure from the go sdk as closely as possible:

launch_template_configs {
    launch_template_specification {
      id = "${aws_launch_template.ecs_instance.id}"
      version = "${aws_launch_template.ecs_instance.latest_version}"
    }
    overrides {
      instance_type = "t2.micro"
      spot_price = "0.05"
    }
  }

on_demand_target_capacity

Allows specifying a minimum portion of capcity to be filled by on-demand instances.

Any other tips & suggestions are appreciated.

Output from acceptance testing:

=== RUN   TestAccAWSSpotFleetRequest_associatePublicIpAddress
--- PASS: TestAccAWSSpotFleetRequest_associatePublicIpAddress (298.31s)
=== RUN   TestAccAWSSpotFleetRequest_launchTemplate
--- PASS: TestAccAWSSpotFleetRequest_launchTemplate (385.74s)
=== RUN   TestAccAWSSpotFleetRequest_launchTemplateWithOnDemandCapacity
--- PASS: TestAccAWSSpotFleetRequest_launchTemplateWithOnDemandCapacity (118.89s)
=== RUN   TestAccAWSSpotFleetRequest_launchTemplateWithOverrides
--- PASS: TestAccAWSSpotFleetRequest_launchTemplateWithOverrides (258.26s)
=== RUN   TestAccAWSSpotFleetRequest_launchTemplateToLaunchSpec
--- PASS: TestAccAWSSpotFleetRequest_launchTemplateToLaunchSpec (637.84s)
=== RUN   TestAccAWSSpotFleetRequest_launchSpecToLaunchTemplate
--- PASS: TestAccAWSSpotFleetRequest_launchSpecToLaunchTemplate (521.70s)
=== RUN   TestAccAWSSpotFleetRequest_instanceInterruptionBehavior
--- PASS: TestAccAWSSpotFleetRequest_instanceInterruptionBehavior (325.34s)
=== RUN   TestAccAWSSpotFleetRequest_changePriceForcesNewRequest
--- PASS: TestAccAWSSpotFleetRequest_changePriceForcesNewRequest (635.60s)
=== RUN   TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion (325.68s)
=== RUN   TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList (352.56s)
=== RUN   TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList (262.74s)
=== RUN   TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz (318.36s)
=== RUN   TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet (333.31s)
=== RUN   TestAccAWSSpotFleetRequest_overriddingSpotPrice
--- PASS: TestAccAWSSpotFleetRequest_overriddingSpotPrice (391.70s)
=== RUN   TestAccAWSSpotFleetRequest_withoutSpotPrice
--- PASS: TestAccAWSSpotFleetRequest_withoutSpotPrice (385.29s)
=== RUN   TestAccAWSSpotFleetRequest_withWeightedCapacity
--- PASS: TestAccAWSSpotFleetRequest_withWeightedCapacity (326.93s)
=== RUN   TestAccAWSSpotFleetRequest_withEBSDisk
--- PASS: TestAccAWSSpotFleetRequest_withEBSDisk (311.59s)
=== RUN   TestAccAWSSpotFleetRequest_withTags
--- PASS: TestAccAWSSpotFleetRequest_withTags (325.39s)
=== RUN   TestAccAWSSpotFleetRequest_placementTenancy
--- PASS: TestAccAWSSpotFleetRequest_placementTenancy (107.13s)
=== RUN   TestAccAWSSpotFleetRequest_diversifiedAllocation
--- PASS: TestAccAWSSpotFleetRequest_diversifiedAllocation (363.19s)
=== RUN   TestAccAWSSpotFleetRequest_WithELBs
--- PASS: TestAccAWSSpotFleetRequest_WithELBs (350.88s)
=== RUN   TestAccAWSSpotFleetRequest_WithTargetGroups
--- PASS: TestAccAWSSpotFleetRequest_WithTargetGroups (443.07s)

The above was the full set of spot fleet resource tests.
Of the above tests, these are the new ones:

=== RUN   TestAccAWSSpotFleetRequest_launchTemplate
--- PASS: TestAccAWSSpotFleetRequest_launchTemplate (385.74s)
=== RUN   TestAccAWSSpotFleetRequest_launchTemplateWithOnDemandCapacity
--- PASS: TestAccAWSSpotFleetRequest_launchTemplateWithOnDemandCapacity (118.89s)
=== RUN   TestAccAWSSpotFleetRequest_launchTemplateWithOverrides
--- PASS: TestAccAWSSpotFleetRequest_launchTemplateWithOverrides (258.26s)
=== RUN   TestAccAWSSpotFleetRequest_launchTemplateToLaunchSpec
--- PASS: TestAccAWSSpotFleetRequest_launchTemplateToLaunchSpec (637.84s)
=== RUN   TestAccAWSSpotFleetRequest_launchSpecToLaunchTemplate
--- PASS: TestAccAWSSpotFleetRequest_launchSpecToLaunchTemplate (521.70s)

kkopachev · 2018-06-21T23:47:40Z

The numbers in the resource attr paths seem to be randomly generated and change every time I run the tests, so I can never get them to match my output assertions.

Looks like they calculated here and should not change if input config did not changed.
https://github.com/terraform-providers/terraform-provider-aws/blob/cca70b5b510958ea5f34412760143befa03573e0/resource_aws_spot_fleet_request.go#L996-L1009

lossanarch · 2018-06-23T02:56:39Z

EDIT: I went back and reevaluated and you're probably exactly right. I think I'm using the sets wrong. Let me make some changes and see if that works.
Thanks very much! Please ignore everything below.

Thanks @kkopachev, but I'm not sure that's quite right. That function is set in the schema for launch specification - Set: hashLaunchSpecification, - I think to make diff discovery easier, but there is no such function attached to my new template resource but it still generates the same numbers:

resource.TestCheckResourceAttr(
"aws_spot_fleet_request.foo", "launch_template_configs.3756671051.launch_template_specification.#", "1"),
resource.TestCheckResourceAttr(
"aws_spot_fleet_request.foo", "launch_template_configs.3756671051.overrides.#", "2"),
resource.TestCheckResourceAttr(
"aws_spot_fleet_request.foo", "launch_template_configs.3756671051.overrides.#", "2"),
resource.TestCheckResourceAttr(
"aws_spot_fleet_request.foo", "launch_template_configs.3756671051.overrides.201173659.instance_type", "t1.micro"),
resource.TestCheckResourceAttr(
"aws_spot_fleet_request.foo", "launch_template_configs.3756671051.overrides.201173659.weighted_capacity", "2"),
resource.TestCheckResourceAttr(
"aws_spot_fleet_request.foo", "launch_template_configs.3756671051.overrides.185159535.instance_type", "m3.medium"),
resource.TestCheckResourceAttr(
"aws_spot_fleet_request.foo", "launch_template_configs.3756671051.overrides.185159535.spot_price", "0.26"),

~~(apologies for the formatting)~~
~~I'm reasonably confident that the Terraform Core is responsible for these numbers as it appears to me to be part of the state file format and independent of anything in the AWS provider.~~

lossanarch · 2018-06-28T11:02:24Z

Ok, barring feedback and change requests this is now done with all spot fleet tests passing.

@kkopachev I added a hashing function for the launch template set but tbh I'm still a little fuzzy on whether it was necessary.
Your comment about the hash staying the same if the config didn't change was very helpful - I realised I was using the launch template id which would change each generation, so I switched to using name which made the hashes predictable.
Thanks again!

kkopachev · 2018-07-07T02:55:47Z

@lossanarch Could you rebase to resolve conflicts?

lossanarch · 2018-07-07T11:31:06Z

Sorry @kkopachev, for some reason I thought it had to be done by someone with write access. Please excuse my inexperience.
Looks clear now.

fredsig · 2018-07-25T16:35:36Z

Is this going to be included in the upcoming 1.29.0? Thanks.

lossanarch · 2018-08-07T00:26:22Z

Is there anything else I need to address with this one?

lossanarch · 2018-08-27T23:27:55Z

Any word on when this might be merged?

Zeerg · 2018-09-10T21:27:08Z

Can we get this in the next release please?

Ninir · 2018-10-11T17:21:23Z

@lossanarch Hey thanks for the work here! could you rebase this branch?

Will have a look at it as soon as it's resolved :)

Thanks!

siwyd · 2018-10-17T09:37:09Z

@lossanarch Definitely a big fan of this getting merged!

thaiphv · 2019-05-20T04:09:57Z

@Ninir do you have any plan to review and merge this PR?

alexejk · 2019-06-04T07:56:05Z

Really looking forward to this one being merged soon.

thaiphv · 2019-06-12T21:37:54Z

Why the silence? Should the PR be assigned to another reviewer?

Ninir · 2019-06-13T05:00:06Z

Sorry folks, I've been relocating from one country another and I need to get back in shape to contributing here.
I can ask @bflad to have a look at it, but I think he's busy on other things right now. I'll do my best in the coming days to make that happen, and again sorry for the latency here.

Ninir

Hey @lossanarch

I made a more in-depth review before going for the final one. Thank you so much for the contribution and very very sorry for the latency here!

It is definitely a good work that will benefit a lot of people :)

website/docs/r/spot_fleet_request.html.markdown

Ninir · 2019-07-02T06:30:18Z

website/docs/r/spot_fleet_request.html.markdown

+
+### Overrides
+
+* `instance_type` - (Optional) The type of instance to request.


Can you add an example about this one? If I may, would it be possible to make that example TF 0.12 compliant so that people understand how to do it?
This way, we would have a instance_type example with 0.12, and a subnet_id example with 0.11!

This sounds like a good idea but so far I've not been able to use 0.12 as not all providers I'm using are compatible. I'll need to get acquainted with it before I can do this.

Ninir · 2019-07-02T06:30:43Z

website/docs/r/spot_fleet_request.html.markdown

+* `weighted_capacity` - (Optional) The capacity added to the fleet by a fulfilled request.
+* `subnet_id` - (Optional) The subnet in which to launch the requested instance.
+
+  **Note:** Instead of statically defining these override blocks they can be passed in as a list of maps containing the above keys and their values. This allows dynamic, programmatic generation of overrides based on variable or environment data.


Thus would need to be reworded for TF 0.12 IMO, thoughts?

Again same 0.12 problem as above. But particularly for this one, I wrote all of this specifically for the feature of dynamic, programmatic generation of overrides based on variable or environment data which now feels defunct thanks to the new config syntax and particularly the foreach loops.

Ninir · 2019-07-02T06:50:32Z

aws/resource_aws_spot_fleet_request.go

+										Optional:      true,
+										Computed:      true,
+										ForceNew:      true,
+										ConflictsWith: []string{"launch_template_configs.0.launch_template_specification.0.id"},


This ConflictsWith value will not work since launch_template_configs is of TypeSet, and this syntax is allowed by TypeList only.
I'm thinking this should be handled in the CustomizeDiff instead, as it would allow to do it in a more efficient way.

Ninir · 2019-07-02T06:52:19Z

aws/resource_aws_spot_fleet_request.go

@@ -360,6 +454,14 @@ func resourceAwsSpotFleetRequest() *schema.Resource {
 				Set:      schema.HashString,
 			},
 		},
+		CustomizeDiff: customdiff.Sequence(
+			customdiff.ComputedIf("launch_template_configs.0.launch_template_specification.0.id", func(diff *schema.ResourceDiff, meta interface{}) bool {


This wouldn't work since launch_template_configs is of TypeSet and the syntax used here is for TypeList with MaxItems: 1.

In this case, I would advise to iterate over the elements and check for the value using Go instead of HCL-based access. Let me know if that doesn't make sense!

Ninir · 2019-07-02T06:54:20Z

aws/resource_aws_spot_fleet_request.go

@@ -360,6 +454,14 @@ func resourceAwsSpotFleetRequest() *schema.Resource {
 				Set:      schema.HashString,
 			},
 		},
+		CustomizeDiff: customdiff.Sequence(
+			customdiff.ComputedIf("launch_template_configs.0.launch_template_specification.0.id", func(diff *schema.ResourceDiff, meta interface{}) bool {


Also, since launch_template_specification is Computed and Optional, this seems extraneous, since the differences would never show if not configured

Ninir · 2019-07-02T06:57:38Z

aws/resource_aws_spot_fleet_request.go

-					return resource.RetryableError(
-						fmt.Errorf("Error creating Spot fleet request, retrying: %s", err))
+					switch {
+					case strings.Contains(awsErr.Message(), "LaunchTemplateSpecification"):


I feel we should be more specific about the message erroring, so that we can better handle different kind of errors.
We could rewrite part of this switch for something like:

if isAWSErr(err, "InvalidSpotFleetRequestConfig", "LaunchTemplateSpecification") { return resource.RetryableError(err) }

but instead of LaunchTemplateSpecification, having a part of the sentence that matches a given error. Does that make sense?

Ninir · 2019-07-02T07:04:59Z

aws/resource_aws_spot_fleet_request_test.go

+					resource.TestCheckResourceAttr(
+						"aws_spot_fleet_request.foo", "launch_template_configs.#", "1"),
+					resource.TestCheckResourceAttr(
+						"aws_spot_fleet_request.foo", "launch_template_configs.4018047529.launch_template_specification.#", "1"),


Note for me, I'm gonna check this one once running acceptance tests with TF_SCHEMA_PANIC_ON_ERROR=1 to be sure it's not a false positive

brianpham · 2019-07-30T20:24:51Z

@lossanarch Any updates on this? Was trying to follow along where the progress of this issue is, but not sure since it haven't been updated in awhile.

gammasts · 2019-08-15T01:07:51Z

@lossanarch are you able to continue this PR? I think it's super valuable since it brings parity to spot fleet requests especially because AWS seems to be going down the path of supporting launch templates for all the other resources (EC2 Fleet, Auto Scaling).

@bflad I noticed you had updated the milestones previously but somehow it got removed and GitHub made no mention of it. Anything you can do to assist shepherding this PR? I'm getting worried that this one has lost all of it's steam.

Co-Authored-By: Gauthier Wallet <[email protected]>

arwalls · 2019-11-22T15:45:51Z

What will it take to get this merged? We have the same need as many other commenters here.

brianpham · 2020-01-29T18:14:38Z

Is anyone still working on this feature? Is there anyone else that can take over? I would love to be able to manage our spot fleet launch templates through Terraform.

dbartokthomas · 2020-02-05T20:21:35Z

I'm surprised this isn't getting more comments. This feature would be fantastic.

bflad · 2020-04-24T01:11:38Z

Hi folks 👋 Please note that the commits in this pull request have been added into #12732 and that PR is now under review. Thanks to @lossanarch for working on this implementation. Please follow #12732 for further updates. 👍

ghost · 2020-05-24T17:11:24Z

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!

ghost added the size/XL Managed by automation to categorize the size of a PR. label Jun 18, 2018

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. labels Jun 18, 2018

gordonbondon mentioned this pull request Jun 19, 2018

Add support for launch templates to aws_spot_fleet_request #4267

Closed

ghost added the size/XL Managed by automation to categorize the size of a PR. label Jun 28, 2018

lossanarch changed the title ~~[WIP] Spotfleet Launch Template and On-Demand Capacity Support~~ Spotfleet Launch Template and On-Demand Capacity Support Jun 28, 2018

lossanarch force-pushed the spotfleet branch from c0e2eda to 91fbb28 Compare July 7, 2018 10:44

ghost added the size/XL Managed by automation to categorize the size of a PR. label Jul 7, 2018

lossanarch force-pushed the spotfleet branch from 91fbb28 to d49e8b0 Compare July 7, 2018 10:54

ghost added size/XXL Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. labels Jul 7, 2018

lossanarch force-pushed the spotfleet branch from d49e8b0 to c0e2eda Compare July 7, 2018 11:17

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Jul 7, 2018

lossanarch force-pushed the spotfleet branch from c0e2eda to 668346a Compare July 7, 2018 11:21

ghost added the size/XL Managed by automation to categorize the size of a PR. label Jul 7, 2018

Ninir added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 15, 2018

lossanarch force-pushed the spotfleet branch from e5e789c to f5bf4ba Compare October 20, 2018 08:48

ghost added the documentation Introduces or discusses updates to documentation. label Oct 20, 2018

Ninir added this to the v2.19.0 milestone Jul 2, 2019

Ninir suggested changes Jul 2, 2019

View reviewed changes

bflad modified the milestones: v2.19.0, v2.20.0 Jul 11, 2019

nywilken modified the milestones: v2.20.0, v2.21.0 Jul 19, 2019

bflad modified the milestones: v2.21.0, v2.22.0 Jul 25, 2019

bflad modified the milestones: v2.22.0, v2.23.0 Aug 1, 2019

Update website/docs/r/spot_fleet_request.html.markdown

0ba5456

Co-Authored-By: Gauthier Wallet <[email protected]>

lossanarch requested a review from a team August 25, 2019 12:16

lossanarch and others added 2 commits August 25, 2019 22:17

Update website/docs/r/spot_fleet_request.html.markdown

3df1662

Co-Authored-By: Gauthier Wallet <[email protected]>

Update website/docs/r/spot_fleet_request.html.markdown

cb99970

Co-Authored-By: Gauthier Wallet <[email protected]>

This was referenced Apr 8, 2020

r/aws_spot_fleet_request - add support for tagging + validations #12295

Merged

Spot Fleet Launch Template Support #12732

Merged

bflad closed this Apr 24, 2020

ghost locked and limited conversation to collaborators May 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spotfleet Launch Template and On-Demand Capacity Support #4866

Spotfleet Launch Template and On-Demand Capacity Support #4866

lossanarch commented Jun 18, 2018 •

edited

Loading

kkopachev commented Jun 21, 2018

lossanarch commented Jun 23, 2018 •

edited

Loading

lossanarch commented Jun 28, 2018

kkopachev commented Jul 7, 2018

lossanarch commented Jul 7, 2018

fredsig commented Jul 25, 2018

lossanarch commented Aug 7, 2018

lossanarch commented Aug 27, 2018

Zeerg commented Sep 10, 2018

Ninir commented Oct 11, 2018 •

edited

Loading

siwyd commented Oct 17, 2018

thaiphv commented May 20, 2019

alexejk commented Jun 4, 2019

thaiphv commented Jun 12, 2019

Ninir commented Jun 13, 2019

Ninir left a comment

Ninir Jul 2, 2019

lossanarch Aug 25, 2019

Ninir Jul 2, 2019

lossanarch Aug 25, 2019

Ninir Jul 2, 2019

Ninir Jul 2, 2019

Ninir Jul 2, 2019

Ninir Jul 2, 2019

Ninir Jul 2, 2019 •

edited

Loading

brianpham commented Jul 30, 2019

gammasts commented Aug 15, 2019

arwalls commented Nov 22, 2019

brianpham commented Jan 29, 2020

dbartokthomas commented Feb 5, 2020

bflad commented Apr 24, 2020 •

edited

Loading

ghost commented May 24, 2020


		### Overrides

		* `instance_type` - (Optional) The type of instance to request.

Spotfleet Launch Template and On-Demand Capacity Support #4866

Spotfleet Launch Template and On-Demand Capacity Support #4866

Conversation

lossanarch commented Jun 18, 2018 • edited Loading

kkopachev commented Jun 21, 2018

lossanarch commented Jun 23, 2018 • edited Loading

lossanarch commented Jun 28, 2018

kkopachev commented Jul 7, 2018

lossanarch commented Jul 7, 2018

fredsig commented Jul 25, 2018

lossanarch commented Aug 7, 2018

lossanarch commented Aug 27, 2018

Zeerg commented Sep 10, 2018

Ninir commented Oct 11, 2018 • edited Loading

siwyd commented Oct 17, 2018

thaiphv commented May 20, 2019

alexejk commented Jun 4, 2019

thaiphv commented Jun 12, 2019

Ninir commented Jun 13, 2019

Ninir left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ninir Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

brianpham commented Jul 30, 2019

gammasts commented Aug 15, 2019

arwalls commented Nov 22, 2019

brianpham commented Jan 29, 2020

dbartokthomas commented Feb 5, 2020

bflad commented Apr 24, 2020 • edited Loading

ghost commented May 24, 2020

lossanarch commented Jun 18, 2018 •

edited

Loading

lossanarch commented Jun 23, 2018 •

edited

Loading

Ninir commented Oct 11, 2018 •

edited

Loading

Ninir Jul 2, 2019 •

edited

Loading

bflad commented Apr 24, 2020 •

edited

Loading