-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
WIP Spot bids as separate resource #2109
WIP Spot bids as separate resource #2109
Conversation
@catsby and I looked at this and we're back in the camp that we liked your previous PR, just remove the fallback for now. I think with that it'd be good to merge. Sorry for the back and forth. I think the Can you revert back to what you had before? Thanks! (Just open the PR agains ince its pretty diff) |
🆒 |
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.
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. |
An alternative solution to #1339 based on comments from #2095
This notably moves spot bids to be their own resource, and personally I think it's much cleaner.
The major drawback that would need to be covered in refactoring though is that they share lots of attributes, and those would need to be refactored to DRY things up, but as mentioned in comments on the other PR, I think this probably the most favorable solution.
One thing I'm not very happy with is that attributes provided by aws_spot_bid and required by aws_instance have to be specified twice.
A sample terraform file is provided below, which illustrates this problem:
This "works" but there are lots of FIXMEs and no tests, so this should be considered another rough-cut for review of the concept.
Between this and the other PR, the two major options of either having spot instances be special types of instances, verses having spot bids be their own resource are covered. I can't think of a third option, but perhaps there is one?
I'm closing the other PR in favor of this one, if we decide we don't like this approach we can reopen it.
summary of changes:
@cbednarski @mitchellh @catsby for review