Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/aws: spot_instance_request #2263

Merged
merged 1 commit into from
Jun 8, 2015
Merged

Conversation

phinze
Copy link
Contributor

@phinze phinze commented Jun 7, 2015

This is an iteration on the great work done by @dalehamel in PRs #2095
and #2109.

The core team went back and forth on how to best model Spot Instance
Requests, requesting and then rejecting a separate-resource
implementation in #2109.

After more internal discussion, we landed once again on a separate
resource to model Spot Instance Requests. Out of respect for
@dalehamel's already-significant donated time, with this I'm attempting
to pick up the work to take this across the finish line.

Important architectural decisions represented here:

  • Spot Instance Requests are always of type "persistent", to properly
    match Terraform's declarative model.
  • The spot_instance_request resource exports several attributes that
    are expected to be constantly changing as the spot market changes:
    spot_bid_status, spot_request_state, and instance_id. Creating
    additional resource dependencies based on these attributes is not
    recommended, as Terraform diffs will be continually generated to keep
    up with the live changes.
  • When a Spot Instance Request is deleted/canceled, an attempt is made
    to terminate the last-known attached spot instance. Race conditions
    dictate that this attempt cannot guarantee that the associated spot
    instance is terminated immediately.

Implementation notes:

  • This version of aws_spot_instance_request borrows a lot of common
    code from aws_instance.
  • In order to facilitate borrowing, we introduce awsInstanceOpts, an
    internal representation of instance details that's meant to be shared
    between resources. The goal here would be to refactor ASG Launch
    Configurations to use the same struct.
  • The new aws_spot_instance_request acc. test is passing.
  • All aws_instance acc. tests remain passing.

@phinze phinze mentioned this pull request Jun 7, 2015
3 tasks
This is an iteration on the great work done by @dalehamel in PRs #2095
and #2109.

The core team went back and forth on how to best model Spot Instance
Requests, requesting and then rejecting a separate-resource
implementation in #2109.

After more internal discussion, we landed once again on a separate
resource to model Spot Instance Requests. Out of respect for
@dalehamel's already-significant donated time, with this I'm attempting
to pick up the work to take this across the finish line.

Important architectural decisions represented here:

 * Spot Instance Requests are always of type "persistent", to properly
   match Terraform's declarative model.
 * The spot_instance_request resource exports several attributes that
   are expected to be constantly changing as the spot market changes:
   spot_bid_status, spot_request_state, and instance_id. Creating
   additional resource dependencies based on these attributes is not
   recommended, as Terraform diffs will be continually generated to keep
   up with the live changes.
 * When a Spot Instance Request is deleted/canceled, an attempt is made
   to terminate the last-known attached spot instance. Race conditions
   dictate that this attempt cannot guarantee that the associated spot
   instance is terminated immediately.

Implementation notes:

 * This version of aws_spot_instance_request borrows a lot of common
   code from aws_instance.
 * In order to facilitate borrowing, we introduce `awsInstanceOpts`, an
   internal representation of instance details that's meant to be shared
   between resources. The goal here would be to refactor ASG Launch
   Configurations to use the same struct.
 * The new aws_spot_instance_request acc. test is passing.
 * All aws_instance acc. tests remain passing.
@phinze phinze force-pushed the f-aws-spot-instance-request branch from c30d8f3 to 112724f Compare June 7, 2015 22:33
@mitchellh
Copy link
Contributor

Looks perfect.

I didn't vet line-for-line all the logic since there is a lot of shared lines but if the acc tests are passing I believe. 🙏

@catsby
Copy link
Contributor

catsby commented Jun 8, 2015

👍 excellent

@phinze
Copy link
Contributor Author

phinze commented Jun 8, 2015

@dalehamel - Going to merge this to avoid conflicts on the relatively large diff in aws_instance, but I'd still love to hear your thoughts on this. 👍

phinze added a commit that referenced this pull request Jun 8, 2015
@phinze phinze merged commit e305d7c into master Jun 8, 2015
@phinze phinze deleted the f-aws-spot-instance-request branch June 8, 2015 15:30
@dalehamel
Copy link

Sorry I was late to the party - glad to hear this got shipped! I'll provide
some more detailed feedback when I get to a terminal

On Monday, June 8, 2015, Paul Hinze [email protected] wrote:

@dalehamel https://github.com/dalehamel - Going to merge this to avoid
conflicts on the relatively large diff in aws_instance, but I'd still
love to hear your thoughts on this. [image: 👍]


Reply to this email directly or view it on GitHub
#2263 (comment).

@dalehamel
Copy link

Looked at the code, it's very similar to the approach I took in my second PR, only you finished it off by pulling out the option parsing as I suggested.

I'm really glad you also got some tests done. I see the doc markdown, but I guess that'll take another deploy of the doc site?

👍 thanks for taking this across the finish line!

phinze added a commit that referenced this pull request Jun 8, 2015
I snuck this in with #2263 because thought it was simply a stylistic
clarity thing, but it actually generates a resource-replacement-forcing
diff for existing resources that don't have this set in the config.
Definitely don't want that. :P

/cc @catsby
@radeksimko radeksimko mentioned this pull request Jan 7, 2016
7 tasks
@ghost
Copy link

ghost commented May 2, 2020

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.

@ghost ghost locked and limited conversation to collaborators May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants