Add `instance_pools_to_use` parameter to `aws_spot_fleet_request` #5955

zytek · 2018-09-21T09:16:19Z

Fixes #5928

Changes proposed in this pull request:

add instance_pools_to_use parameter to aws_spot_fleet_request resource

Output from acceptance testing:

not run, please assist

Please comment and review, this is my attempt at working with terraform code.

Fixes hashicorp#5928

zytek · 2018-09-27T10:01:17Z

@bflad I've seen you used different name for this param in ec2_fleet. Should I update this PR with similar change? Any other comments?

bflad

Hi @zytek 👋 Thanks for submitting this!

Output from acceptance testing:
not run, please assist

How can we help? We generally prefer contributors to be able to run their own acceptance testing to not cause long change request and reply cycles. If you're unable to run the testing for some reason, another community member or a maintainer will likely need to finish up the pull request. Not a big deal, we will just want to know.

Should I update this PR with similar change?

We prefer to follow the API/SDK naming of attributes, but your current name is pretty close so I wouldn't necessarily consider that a blocking review item personally. It would be nice if it did include _count though.

Any other comments?

Any chance you could submit an acceptance test so we can ensure this works as expected and prevent future regressions? Its generally easiest to copy-paste an existing test and test configuration (making necessary adjustments).

bflad · 2018-09-28T15:57:39Z

aws/resource_aws_spot_fleet_request.go

@@ -285,6 +285,12 @@ func resourceAwsSpotFleetRequest() *schema.Resource {
 				Default:  "lowestPrice",
 				ForceNew: true,
 			},
+			"instance_pools_to_use": {


Nitpick: The API names this instance_pools_to_use_count so it might be nice to be consistent.

bflad · 2018-09-28T16:00:53Z

aws/resource_aws_spot_fleet_request.go

@@ -623,6 +629,13 @@ func resourceAwsSpotFleetRequestCreate(d *schema.ResourceData, meta interface{})
 		spotFleetConfig.AllocationStrategy = aws.String("lowestPrice")
 	}

+	if v, ok := d.GetOk("instance_pools_to_use"); ok {


I would recommend checking against the default value and removing our error check here (letting the API return the appropriate error if necessary), e.g.

if v, ok := d.GetOk("instance_pools_to_use_count"); ok && v.(int) != 1 { spotFleetConfig.InstancePoolsToUseCount = aws.Int64(int64(v.(int))) }

@bflad could you elaborate on why you prefer to return errors from API rather than catch them earlier? I am not opposing it, just curious about the rationale.

Working on updating this PR, I see one acceptance test failing due to spot prices jump, not sure if I should alter test spec or ignore it.

could you elaborate on why you prefer to return errors from API rather than catch them earlier? I am not opposing it, just curious about the rationale.

Unless we can generate plan time errors (multi-attribute validation in Terraform is a can of worms at the moment), we prefer the API because:

Upstream API logic may change

Upstream API may remove the restriction

Less code on our end

Which generally boils down to less maintenance burden in this project (someone updating the code then getting a release cut) and for operators (requiring everyone to update their provider). This is the same reasoning why we try to avoid things like EC2 instance type validation, it can be too fast of a moving target or provide little benefit for all the effort.

Working on updating this PR, I see one acceptance test failing due to spot prices jump, not sure if I should alter test spec or ignore it.

Is it this one? #6031 😄 You can ignore it or potentially look at the forward looking solution mentioned in the comment.

Thanks for clarification. Yeah, the PR you mentioned is the exact issue I got. ;-)

…h API

zytek · 2018-10-02T15:22:04Z

Added tests and addressed comments by @bflad

Two test failures but not related.

Output:

➜  terraform-provider-aws git:(spot_fleet_pools) ✗ time make testacc TESTARGS='-run=TestAccAWSSpotFleetRequest_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSSpotFleetRequest_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]


=== RUN   TestAccAWSSpotFleetRequest_associatePublicIpAddress
--- PASS: TestAccAWSSpotFleetRequest_associatePublicIpAddress (261.95s)
=== RUN   TestAccAWSSpotFleetRequest_instanceInterruptionBehavior
--- PASS: TestAccAWSSpotFleetRequest_instanceInterruptionBehavior (262.61s)
=== RUN   TestAccAWSSpotFleetRequest_fleetType
--- PASS: TestAccAWSSpotFleetRequest_fleetType (252.01s)
=== RUN   TestAccAWSSpotFleetRequest_iamInstanceProfileArn
--- PASS: TestAccAWSSpotFleetRequest_iamInstanceProfileArn (274.72s)
=== RUN   TestAccAWSSpotFleetRequest_changePriceForcesNewRequest
--- PASS: TestAccAWSSpotFleetRequest_changePriceForcesNewRequest (532.76s)
=== RUN   TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion (262.78s)
=== RUN   TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList (283.29s)
=== RUN   TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList
--- FAIL: TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList (78.48s)
    testing.go:527: Step 0 error: Error applying: 1 error occurred:
        	* aws_spot_fleet_request.foo: 1 error occurred:
        	* aws_spot_fleet_request.foo: Last events: [{
          EventInformation: {
            EventDescription: "m3.large, ami-d0f506b0, Linux/UNIX, us-west-2b, Spot bid price is less than Spot market price $0.0307",
            EventSubType: "launchSpecUnusable"
          },
          EventType: "information",
          Timestamp: 2018-10-02 13:36:49.663 +0000 UTC
        } {
          EventInformation: {
            EventDescription: "m3.large, ami-d0f506b0, Linux/UNIX, us-west-2a, Spot bid price is less than Spot market price $0.0307",
            EventSubType: "launchSpecUnusable"
          },
          EventType: "information",
          Timestamp: 2018-10-02 13:36:49.674 +0000 UTC
        } {
          EventInformation: {
            EventDescription: "All launch specifications are unusable. Please refer to individual information events for more detail.",
            EventSubType: "fleetProgressHalted"
          },
          EventType: "information",
          Timestamp: 2018-10-02 13:36:49.682 +0000 UTC
        }]




=== RUN   TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz (242.33s)
=== RUN   TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet (276.12s)
=== RUN   TestAccAWSSpotFleetRequest_overriddingSpotPrice
--- PASS: TestAccAWSSpotFleetRequest_overriddingSpotPrice (265.47s)
=== RUN   TestAccAWSSpotFleetRequest_withoutSpotPrice
--- PASS: TestAccAWSSpotFleetRequest_withoutSpotPrice (255.23s)
=== RUN   TestAccAWSSpotFleetRequest_diversifiedAllocation
--- PASS: TestAccAWSSpotFleetRequest_diversifiedAllocation (343.92s)
=== RUN   TestAccAWSSpotFleetRequest_multipleInstancePools
--- PASS: TestAccAWSSpotFleetRequest_multipleInstancePools (391.19s)
=== RUN   TestAccAWSSpotFleetRequest_withWeightedCapacity
--- PASS: TestAccAWSSpotFleetRequest_withWeightedCapacity (272.87s)
=== RUN   TestAccAWSSpotFleetRequest_withEBSDisk
--- PASS: TestAccAWSSpotFleetRequest_withEBSDisk (243.26s)
=== RUN   TestAccAWSSpotFleetRequest_withTags
--- FAIL: TestAccAWSSpotFleetRequest_withTags (412.63s)
    testing.go:588: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.

        Error: Error applying: 1 error occurred:
        	* aws_spot_fleet_request.foo (destroy): 1 error occurred:
        	* aws_spot_fleet_request.foo: error deleting spot request (sfr-b897cfaa-5808-42af-856c-cfcb3c46fe24): fleet still has (1) running instances
[...]
=== RUN   TestAccAWSSpotFleetRequest_placementTenancy
--- PASS: TestAccAWSSpotFleetRequest_placementTenancy (67.97s)
=== RUN   TestAccAWSSpotFleetRequest_WithELBs
--- PASS: TestAccAWSSpotFleetRequest_WithELBs (318.78s)
=== RUN   TestAccAWSSpotFleetRequest_WithTargetGroups
--- PASS: TestAccAWSSpotFleetRequest_WithTargetGroups (418.43s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	5716.830s
make: *** [testacc] Error 1
make testacc TESTARGS='-run=TestAccAWSSpotFleetRequest_'  30.54s user 6.88s system 0% cpu 1:35:26.04 total

bflad

LGTM, thanks @zytek! 🚀

--- PASS: TestAccAWSSpotFleetRequest_placementTenancy (54.44s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList (269.68s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz (270.39s)
--- PASS: TestAccAWSSpotFleetRequest_withWeightedCapacity (270.87s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet (279.35s)
--- PASS: TestAccAWSSpotFleetRequest_diversifiedAllocation (279.46s)
--- PASS: TestAccAWSSpotFleetRequest_overriddingSpotPrice (279.53s)
--- PASS: TestAccAWSSpotFleetRequest_multipleInstancePools (279.70s)
--- PASS: TestAccAWSSpotFleetRequest_withoutSpotPrice (279.77s)
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion (285.34s)
--- PASS: TestAccAWSSpotFleetRequest_withEBSDisk (289.03s)
--- PASS: TestAccAWSSpotFleetRequest_fleetType (289.22s)
--- PASS: TestAccAWSSpotFleetRequest_associatePublicIpAddress (289.27s)
--- PASS: TestAccAWSSpotFleetRequest_withTags (289.75s)
--- PASS: TestAccAWSSpotFleetRequest_instanceInterruptionBehavior (290.62s)
--- PASS: TestAccAWSSpotFleetRequest_iamInstanceProfileArn (292.59s)
--- PASS: TestAccAWSSpotFleetRequest_WithELBs (307.24s)
--- PASS: TestAccAWSSpotFleetRequest_WithTargetGroups (374.32s)
--- PASS: TestAccAWSSpotFleetRequest_changePriceForcesNewRequest (554.19s)

ghost · 2018-10-03T18:54:38Z

This has been released in version 1.39.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "aws" {
	version = "~> 1.39.0"
}
# ... other configuration ...

ghost · 2020-04-03T17:17:30Z

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!

Add instance_pools_to_use parameter to aws_spot_fleet_request

1f0afd1

Fixes hashicorp#5928

ghost added size/XS Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. labels Sep 21, 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 Sep 24, 2018

bflad reviewed Sep 28, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 28, 2018

zytek added 2 commits October 2, 2018 14:59

resource_aws_spot_fleet_request: rename parameter for consistence wit…

3852aa0

…h API

Add TestAccAWSSpotFleetRequest_multipleInstancePools

603c5b1

ghost added size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XS Managed by automation to categorize the size of a PR. labels Oct 2, 2018

zytek changed the title ~~[WIP] Add instance_pools_to_use parameter to aws_spot_fleet_request~~ Add instance_pools_to_use parameter to aws_spot_fleet_request Oct 2, 2018

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

bflad added this to the v1.39.0 milestone Oct 2, 2018

bflad approved these changes Oct 2, 2018

View reviewed changes

bflad merged commit 8717b61 into hashicorp:master Oct 2, 2018

bflad added a commit that referenced this pull request Oct 2, 2018

Update CHANGELOG for #5955

09027c5

ghost locked and limited conversation to collaborators Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `instance_pools_to_use` parameter to `aws_spot_fleet_request` #5955

Add `instance_pools_to_use` parameter to `aws_spot_fleet_request` #5955

zytek commented Sep 21, 2018

zytek commented Sep 27, 2018

bflad left a comment

bflad Sep 28, 2018

bflad Sep 28, 2018

zytek Oct 2, 2018 •

edited

Loading

bflad Oct 2, 2018

zytek Oct 2, 2018

zytek commented Oct 2, 2018

bflad left a comment

ghost commented Oct 3, 2018

ghost commented Apr 3, 2020

Add instance_pools_to_use parameter to aws_spot_fleet_request #5955

Add instance_pools_to_use parameter to aws_spot_fleet_request #5955

Conversation

zytek commented Sep 21, 2018

zytek commented Sep 27, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Sep 28, 2018

Choose a reason for hiding this comment

bflad Sep 28, 2018

Choose a reason for hiding this comment

zytek Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

bflad Oct 2, 2018

Choose a reason for hiding this comment

zytek Oct 2, 2018

Choose a reason for hiding this comment

zytek commented Oct 2, 2018

bflad left a comment

Choose a reason for hiding this comment

ghost commented Oct 3, 2018

ghost commented Apr 3, 2020

Add `instance_pools_to_use` parameter to `aws_spot_fleet_request` #5955

Add `instance_pools_to_use` parameter to `aws_spot_fleet_request` #5955

zytek Oct 2, 2018 •

edited

Loading