provider/aws: tag the spot instance.

[fanxu]

Fix #3263

[phinze]

Thanks for this! Can we add an acceptance test (or enhance an existing test) to verify that tags are set on the instance and cover this behavior?

[caarlos0]

Nice

[dmuysson]

We could really use this capability - hoping it gets added soon!

[harlow]

+1

[johnnyshields]

Not to split hairs here but there is technically a difference between tagging the SpotInstanceRequest itself versus tagging the EC2 Instance which it generates. Maybe using a different attribute like instance_tags for the instance (while keeping tags for the SIR) would be more accurate?

(Or since tags on the EC2 instance is the far more commonly used behavior, you could make spot_instance_request_tags for the SIR)

[johnnyshields]

Another comment: what happens with persistent requests when the instance is terminated and a new instance is later auto-created, presumably the new instance will no longer have the tags?

[fctucker]

+1 for this pull request to be accepted

[johnnyshields]

@fctucker one thing to note here is that with persistent spot requests, if the bid price is hit and the instance stops and starts again, the tags will be not be added to the newly spawned instance.

While this PR can behave as a stop-gap, I think everyone who wants support for this should ask AWS to support PropagateAtLaunch for SpotInstanceRequest tags in the same way it works for AutoScalingGroup tags, as that's a much better solution.

[jedi4ever]

👍 to get this merged ; ideally it's added as an option = tag_instance true/false with default behaviour to false to be backwards compatible

[EntilZha]

Recently ran into this as well, would be great to get merged

[stack72]

FWIW, I think that @johnnyshields is correct here - I am going to open a feature request to AWS about this - feels like propagate_at_launch would be a good thing to have

[stack72]

just opened this aws/aws-sdk-go#804

[stack72]

FYI, the tests for this PR work as expected:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSSpotInstanceRequest_'                2 ↵
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/15 13:22:41 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSSpotInstanceRequest_ -timeout 120m
=== RUN   TestAccAWSSpotInstanceRequest_basic
--- PASS: TestAccAWSSpotInstanceRequest_basic (147.58s)
=== RUN   TestAccAWSSpotInstanceRequest_withBlockDuration
--- PASS: TestAccAWSSpotInstanceRequest_withBlockDuration (134.29s)
=== RUN   TestAccAWSSpotInstanceRequest_vpc
--- PASS: TestAccAWSSpotInstanceRequest_vpc (174.33s)
=== RUN   TestAccAWSSpotInstanceRequest_SubnetAndSG
--- PASS: TestAccAWSSpotInstanceRequest_SubnetAndSG (167.04s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    623.262s

I made one modification, I check to see if the instance has the tags as expected

func testAccCheckAWSSpotInstanceRequest_InstanceTags(
    sir *ec2.SpotInstanceRequest) resource.TestCheckFunc {
    return func(s *terraform.State) error {
        conn := testAccProvider.Meta().(*AWSClient).ec2conn
        resp, err := conn.DescribeInstances(&ec2.DescribeInstancesInput{
            InstanceIds: []*string{sir.InstanceId},
        })
        if err != nil {
            if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidInstanceID.NotFound" {
                return fmt.Errorf("Spot Instance not found")
            }
            return err
        }

        // If nothing was found, then return no state
        if len(resp.Reservations) == 0 {
            return fmt.Errorf("Spot Instance not found")
        }

        instance := resp.Reservations[0].Instances[0]

        var tagMatch bool
        for _, t := range instance.Tags {
            if *t.Value == "terraform-test" {
                tagMatch = true
            }
        }

        if !tagMatch {
            return fmt.Errorf("Error in matching Spot Instance Tags, expected 'terraform-test', got %s", instance.Tags[0].Value)
        }

        return nil
    }
}

[stack72]

We can wait and see what AWS has to say before merging this as a stop-gap

[radeksimko]

It would be super helpful to document that tags are only attached/detached if wait_for_fulfillment = true (default is false, so by default Terraform won't assign/change any tags). It may be obvious for us who know the implementation details behind it, but it may not be for a common user.

[elatt]

AWS now supports tagging natively for spot instances/fleets so this work-around should no longer be necessary if terraform updates its API params: https://aws.amazon.com/about-aws/whats-new/2017/07/tag-your-spot-fleet-ec2-instances/

[johnnyshields]

This PR should be closed and we should raise a new PR to support it natively.

[radeksimko]

Closing for the reasons mentioned. There's a new PR in the new repository also: hashicorp/terraform-provider-aws#1366

[c0d5x]

PR #1366 does NOT fix this, instead if fixes the spot fleet requests. This PR is created for spot instances, so it is still needed.

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.