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

Support Spot Instances #36

Merged

Conversation

eytan-avisror
Copy link
Collaborator

@eytan-avisror eytan-avisror commented Sep 3, 2019

Fixes #35

This PR adds support for spot-instances with the following implementation:

  • Recommendation reconciler was added to controller to watch all events, and trigger a reconcile for any event that is with reason SpotRecommendationGiven, the reconciler will reverse lookup the scaling group name from the event's involvedObject.Name, and find the instance group name and namespace in order to trigger it's reconcile.
  • aws client (asg) is now needed before context is created (for reverse lookup), we should refactor and move all of the AwsWorker object outside of context instead on a later PR.
  • spotPrice can be set manually in the CR, but will always be overridden by recommendations when it exists
  • when recommendations become unavailable instance group will switch back to on-demand
  • recommendations must follow the format:
# Event Message
{"apiVersion":"v1alpha1","spotPrice":"0.0067", "useSpot": true}
# Event involvedObject.Name = scaling group name
# Event reason = SpotRecommendationGiven
  • Also added two additional status fields - usingSpotRecommendations and lifecycle, a printer column was also added for lifecycle.

Testing

  • Added unit tests to test recommendations-based and manual setting of spotPrice
  • Manually tested create/update in various scenarios

@eytan-avisror eytan-avisror requested a review from a team as a code owner September 3, 2019 03:17
@CLAassistant
Copy link

CLAassistant commented Sep 3, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 3, 2019

Codecov Report

Merging #36 into master will decrease coverage by 0.29%.
The diff coverage is 70.89%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #36     +/-   ##
=========================================
- Coverage   70.82%   70.52%   -0.3%     
=========================================
  Files           5        6      +1     
  Lines         682      777     +95     
=========================================
+ Hits          483      548     +65     
- Misses        139      164     +25     
- Partials       60       65      +5
Impacted Files Coverage Δ
...ontrollers/provisioners/ekscloudformation/types.go 75.28% <100%> (+6.6%) ⬆️
...ollers/provisioners/ekscloudformation/bootstrap.go 83.33% <100%> (ø) ⬆️
...trollers/provisioners/ekscloudformation/helpers.go 60.52% <47.82%> (-5.52%) ⬇️
...rovisioners/ekscloudformation/ekscloudformation.go 79.25% <73.58%> (-2.83%) ⬇️
controllers/provisioners/ekscloudformation/spot.go 74.5% <74.5%> (ø)
...rollers/provisioners/ekscloudformation/strategy.go 52.38% <0%> (+1.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ade0320...3dc1dc2. Read the comment docs.

Copy link
Member

@tekenstam tekenstam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG. May need to add unit tests to new functions to make ccov pass.

docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
@shrinandj
Copy link
Collaborator

Fantastic! What the expected json payload if the recommendation is to not use spot-instances?

@eytan-avisror
Copy link
Collaborator Author

eytan-avisror commented Sep 4, 2019

@shrinandj

{"apiVersion":"v1alpha1","spotPrice":"", "useSpot": false}

Will be a recommendation to use on-demand (spotPrice is ignored)

@eytan-avisror eytan-avisror merged commit df1f81b into keikoproj:master Sep 6, 2019
@eytan-avisror eytan-avisror deleted the recommendations-reconciler branch September 6, 2019 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Recommendation Reconciler
4 participants