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.

Sign up for GitHub

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

Jump to bottom

Add support for spot instances #199

Merged
merged 18 commits into from
Apr 23, 2020
Merged

Add support for spot instances #199

merged 18 commits into from
Apr 23, 2020

Conversation

MarcelMue
Copy link
Contributor

Please make sure in case you are changing the spec all our customers are informed beforehand.

You can throw a message in #sig-customer with the changes and the Solution Engineers will take care of
sharing the changes with the customers.

@MarcelMue MarcelMue self-assigned this Mar 25, 2020
@MarcelMue MarcelMue requested a review from a team March 25, 2020 15:25
@marians
Copy link
Member

marians commented Mar 25, 2020

I would prefer plural spot_instances_enabled over singular spot_instance_enabled, since a node pool is normally about a group of instances.

@tfussell tfussell mentioned this pull request Mar 25, 2020
Merged
marians
marians reviewed Mar 26, 2020
View reviewed changes
spec/definitions.yaml Outdated Show resolved Hide resolved
marians
marians reviewed Mar 26, 2020
View reviewed changes
spec/definitions.yaml Outdated Show resolved Hide resolved
marians
marians requested changes Mar 26, 2020
View reviewed changes
Copy link
Member

@marians marians left a comment

Choose a reason for hiding this comment

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

I think adding all the spot capacity vs. on-demand capacity settings in V5AddNodePoolRequestNodeSpec does not fit the purpose of the V5AddNodePoolRequestNodeSpec (which is about the specification of a worker node). Some of the attributes should be moved one level up into V5AddNodePoolRequest.

@marians
Copy link
Member

marians commented Mar 26, 2020

Actually, I don't feel as strongly after some more thinking. However, the comment

# Specification of a node in a node pool to be created
V5AddNodePoolRequestNodeSpec:

should be adapted to reflect the actual purpose better.

@marians
Copy link
Member

marians commented Mar 27, 2020

Related to https://github.com/giantswarm/giantswarm/issues/8347

@marians
Copy link
Member

marians commented Mar 27, 2020 •
edited
Loading

Consistency thing: property names should use camel case. E. g. instance_distribution should be instanceDistribution.

Wrong PR, sorry!

@marians marians changed the title Add spot instances to node pool responses Add support for spot instances Apr 20, 2020
@marians marians self-assigned this Apr 20, 2020
@marians
Copy link
Member

marians commented Apr 20, 2020

FYI: With the latest commits I also added default attributes to inform users about what we set as a default value if the attribute is not specified when creating a node pool.

@teemow teemow mentioned this pull request Apr 21, 2020
Merged
@marians marians merged commit a48edca into master Apr 23, 2020
@marians marians deleted the add-nodepool-spot-inst branch April 23, 2020 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marians marians marians approved these changes

@anvddriesch anvddriesch anvddriesch approved these changes

@paurosello paurosello Awaiting requested review from paurosello

@teemow teemow Awaiting requested review from teemow

@cornelius-keller cornelius-keller Awaiting requested review from cornelius-keller

Assignees

@marians marians

Labels
waiting-for-implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MarcelMue @marians @anvddriesch @teemow