fix empty key_name on aws_spot_fleet_request

[svenwltr]

This fixes one part of #492.

Basically Terraform sends an empty string for the key_name values to the AWS API, if the value isn't set in the template. This causes the instance creation to fail:

Instance fail to start with failed: Invalid value '' for keyPairNames. It should not be blank.

I am not sure, if it is worth doing an acceptance test here. It seems a bit complicated, because it would require to wait for the spot fleet instance creation. I am not sure how to handle this in a acceptance test (maybe with timeouts?). Please tell me, if I should add an test here.

[radeksimko]

Hi @svenwltr
thanks for this patch!

I am not sure, if it is worth doing an acceptance test here. It seems a bit complicated, because it would require to wait for the spot fleet instance creation. I am not sure how to handle this in a acceptance test (maybe with timeouts?). Please tell me, if I should add an test here.

It would be better to add a test covering this bugfix. We already wait for fulfilment, so there should be no need to add any extra logic in that sense. Just make sure the spot price & instance size is aligned with other requests from our testing suite. Feel free to take the "copy-paste-modify" approach if you like.
https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_spot_fleet_request_test.go

[svenwltr]

@radeksimko AFAIS active only means, that the spot fleet request is created and enabled. It might still fail, if there is an error in the launch_specification.

I created a test and removed my fix for testing. It succeeds, but it succeeded which shouldn't happen. This ist the History of the Spot Fleet Request:

Edit: Now I remember, that this behavior costed me some time on creating a template for the spot fleet, since Terraform ran successfully, but the creation of the instances actually failed. Is this the intended behavior?

[svenwltr]

Okay, this is what I found out after digging a bit deeper:

• A Spot Fleet Request has an ActivityStatus and a SpotFleetRequestState (source). The "State" doesn't respect the progress of the "child instances".
• The fulfillment check uses SpotFleetRequestState, which means that the Terraform run might succeed, despite the actual instances don't start.
• If I use the ActivityStatus in the fulfillment check other tests also fail (or at least TestAccAWSSpotFleetRequest_placementTenancy with: Non-Windows instances with a virtualization type of 'hvm' are currently not supported for this instance type.)

It looks easy to change the behavior of the fulfillment check, so it waits for the instances to actually be ready and fix the tests, but I cannot estimate the impact for such a change.

[radeksimko]

Ah, I see, apologies for misleading info then!

It looks like we should add an optional fulfilment check, like we have in https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_spot_instance_request.go#L54-L58

Are you willing to do this in a separate PR?

If I use the ActivityStatus in the fulfillment check other tests also fail (or at least TestAccAWSSpotFleetRequest_placementTenancy with: Non-Windows instances with a virtualization type of 'hvm' are currently not supported for this instance type.)

Yeah, that means those tests are actually broken. 😞

[svenwltr]

@radeksimko I guess it makes sense to add the fulfilment check to all existing test cases, right?

[radeksimko]

@svenwltr pretty much. I first thought we could default to wait, but it seems the aws_spot_instance doesn't wait by default, so better to stay consistent, which does mean we'll need to modify all existing test cases.

[svenwltr]

Here is the implementation so far: #1241

I still need some time to fix the tests.

[svenwltr]

@radeksimko I now rebased to the current master and fixed the TestAccAWSSpotFleetRequest_withEBSDisk test. Since this one already doesn't use a key_name, I didn't create an extra test. Is this sufficient?

[radeksimko]

Since this one already doesn't use a key_name, I didn't create an extra test. Is this sufficient?

Yeah, that's ok, good spot.

LGTM. 👍

Thanks again for going the extra mile with adding the fulfilment check.

[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!