add wait_for_fulfillment to spot_fleet_request

[svenwltr]

emerged from #1203

work-in-progress: There are still many tests to fix. Fixing them will take some time, since I am busy right now.

This adds the wait_for_fulfillment argument to aws_spot_fleet_request. Also many of the acceptance test actually don't work, therefore I am fixing everything which doesn't require #1203 and which I am able to fix. The most code is copied from aws_spot_instance_request.

Occured Issues:

• The AMI for m1.small instances don't work, since they don't support HVM images: Repeated errors have occured processing the launch specification "m1.small, ami-d06a90b0, Linux/UNIX, us-west-2a". It will not be retried for at least 13 minutes. Error message: com.amazonaws.services.ec2.model.AmazonEC2Exception: Non-Windows instances with a virtualization type of 'hvm' are currently not supported for this instance type. (Service: AmazonEC2; Status Code: 400; Error Code: InvalidParameterCombination)
• Some Tests had a race condition, because the aws_spot_fleet_request might get created before aws_iam_policy_attachment, which caused permission issues.
• Since the launch_specification changed in some tests the hash changed, which was confusing and took some time to figure out.
• Some bid prices were below market price, so I had to raise them.

Progress so far:

• TestAccAWSSpotFleetRequest_associatePublicIpAddress
• TestAccAWSSpotFleetRequest_changePriceForcesNewRequest
• TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion
• TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList wrong AMI
• TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList
• TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz
• TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet
• TestAccAWSSpotFleetRequest_overriddingSpotPrice
• TestAccAWSSpotFleetRequest_diversifiedAllocation – worked out of the box
• TestAccAWSSpotFleetRequest_withWeightedCapacity – worked out of the box
• TestAccAWSSpotFleetRequest_withEBSDisk – requires empty key_name
• TestAccAWSSpotFleetRequest_placementTenancy – This is hard to fix, since I get this error message regardless of which instance type I chose: Instance type m4.large is not supported for the tenancy value (dedicated)

[svenwltr]

@radeksimko This PR is ready now.

[radeksimko]

I left you one more and one less important note. Other than that the code looks very good. Thanks for this taking the time to submit this PR!

[radeksimko]

Just a nitpick but I feel the interface of this refresh function could be less greedy, i.e. it doesn't need all of the resource data nor all metadata. How about reducing it to something like this?

func resourceAwsSpotFleetRequestFulfillmentRefreshFunc(id string, conn *ec2.EC2)

[svenwltr]

Sounds good to me. I just copied the whole function from above. Also I already thought about merging these two somehow since they look almost the same.

[radeksimko]

The customizable timeout is currently ignored here - do you mind changing it to d.Timeout(schema.TimeoutCreate)? 😉

[svenwltr]

fixed

[svenwltr]

I fixed the remarks. The acceptance tests are still running on my machine.

[radeksimko]

The acceptance tests are still running on my machine.

My run just finished. I noticed 2 failures, but I believe they're either unrelated or can be addressed in a separate PR.

I'm specifically talking about

• aws_spot_fleet_request.foo: fleet still has (...) running instances

I believe these are intermittent.

[svenwltr]

Hmm ... I didn't had any failures in the run of my first commit.

$ make testacc TESTARGS="-run TestAccAWSSpotFleetRequest"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccAWSSpotFleetRequest -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSSpotFleetRequest_associatePublicIpAddress
--- PASS: TestAccAWSSpotFleetRequest_associatePublicIpAddress (409.18s)
=== RUN   TestAccAWSSpotFleetRequest_changePriceForcesNewRequest
--- PASS: TestAccAWSSpotFleetRequest_changePriceForcesNewRequest (585.67s)
=== RUN   TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzOrSubnetInRegion (280.77s)
=== RUN   TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceAzInGivenList (411.74s)
=== RUN   TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList
--- PASS: TestAccAWSSpotFleetRequest_lowestPriceSubnetInGivenList (325.72s)
=== RUN   TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameAz (315.49s)
=== RUN   TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet
--- PASS: TestAccAWSSpotFleetRequest_multipleInstanceTypesInSameSubnet (314.08s)
=== RUN   TestAccAWSSpotFleetRequest_overriddingSpotPrice
--- PASS: TestAccAWSSpotFleetRequest_overriddingSpotPrice (277.63s)
=== RUN   TestAccAWSSpotFleetRequest_diversifiedAllocation
--- PASS: TestAccAWSSpotFleetRequest_diversifiedAllocation (382.94s)
=== RUN   TestAccAWSSpotFleetRequest_withWeightedCapacity
--- PASS: TestAccAWSSpotFleetRequest_withWeightedCapacity (342.73s)
=== RUN   TestAccAWSSpotFleetRequest_withEBSDisk
--- PASS: TestAccAWSSpotFleetRequest_withEBSDisk (77.91s)
=== RUN   TestAccAWSSpotFleetRequest_placementTenancy
--- PASS: TestAccAWSSpotFleetRequest_placementTenancy (78.22s)
=== RUN   TestAccAWSSpotFleetRequest_CannotUseEmptyKeyName
--- PASS: TestAccAWSSpotFleetRequest_CannotUseEmptyKeyName (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3802.078s

[radeksimko]

Which confirms my theory of those failures being intermittent 😃

[Sprinkle]

With the 0.1.4 version of the provider, anyone having the problem where this "wait_for_fulliment" is not persisted back into state? Hence every run of "terraform plan" will show

wait_for_fulfillment: "" => "false"

Even after "terraform apply" or "terraform refresh" ?

[svenwltr]

I can confirm this issue. I tested it with the following template (in eu-west-1). First, I applied it without wait_for_fulfillment and after applying I did it again with wait_for_fulfillment.

resource "aws_iam_policy_attachment" "test-attach" {
  name       = "test-attachment-123"
  roles      = ["${aws_iam_role.test-role.name}"]
  policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonEC2SpotFleetRole"
}

resource "aws_iam_role" "test-role" {
  name = "test-role-123"

  assume_role_policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "",
      "Effect": "Allow",
      "Principal": {
        "Service": [
          "spotfleet.amazonaws.com",
          "ec2.amazonaws.com"
        ]
      },
      "Action": "sts:AssumeRole"
    }
  ]
}
EOF
}

resource "aws_spot_fleet_request" "foo" {
  iam_fleet_role                      = "${aws_iam_role.test-role.arn}"
  spot_price                          = "0.555"
  target_capacity                     = 2
  valid_until                         = "2019-11-04T20:44:20Z"
  terminate_instances_with_expiration = true

  # wait_for_fulfillment = true

  launch_specification {
    instance_type = "m4.large"
    ami           = "ami-417abe38"
  }

  depends_on = ["aws_iam_policy_attachment.test-attach"]
}

[radeksimko]

@Sprinkle @svenwltr do you mind raising a new issue for this? Otherwise it will very likely get lost and forgotten here.

Thanks.

[svenwltr]

@radeksimko Done: #1816

[ghost]

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!