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

Add support for mixed ASG with spot instances #794

Merged
merged 1 commit into from Jun 4, 2019

Conversation

@martina-if
Copy link
Member

commented May 10, 2019

closes #198
closes #320

It adds support in the API to specify a maximum price and uses that to create a nodegroup of spot instances.

Nodegroups can now have different kinds of instances: on-demand or spot, and different instance types.

  • Merge #802 first
  • Code compiles correctly (i.e make build)
  • Added tests that cover your change (if possible)
  • All unit tests passing (i.e. make test)
  • Has been manually tested
  • Added/modified documentation as required (such as the README.md, and examples directory)

@martina-if martina-if changed the title <!-- Please explain the changes you made here. --> WIP: add support for mixed ASG with spot instances May 10, 2019

@martina-if martina-if referenced this pull request May 10, 2019
0 of 4 tasks complete

@martina-if martina-if force-pushed the martina-if:mixed-instances branch from 037f073 to 51966ca May 13, 2019

pkg/cfn/builder/nodegroup.go Outdated Show resolved Hide resolved
pkg/apis/eksctl.io/v1alpha5/types.go Outdated Show resolved Hide resolved
pkg/apis/eksctl.io/v1alpha5/types.go Outdated Show resolved Hide resolved
docs/08-spot-instances.yaml Outdated Show resolved Hide resolved
pkg/apis/eksctl.io/v1alpha5/types.go Outdated Show resolved Hide resolved
pkg/apis/eksctl.io/v1alpha5/types.go Outdated Show resolved Hide resolved

@martina-if martina-if force-pushed the martina-if:mixed-instances branch from 3bb8b6d to 54dddf5 May 16, 2019

@martina-if martina-if changed the title WIP: add support for mixed ASG with spot instances Add support for mixed ASG with spot instances May 17, 2019

@martina-if martina-if force-pushed the martina-if:mixed-instances branch from 54dddf5 to f9418a2 May 17, 2019

@martina-if martina-if requested a review from mumoshu May 17, 2019

pkg/apis/eksctl.io/v1alpha5/types.go Outdated Show resolved Hide resolved
Type: "AWS::AutoScaling::AutoScalingGroup",
Properties: ngProps,
UpdatePolicy: map[string]map[string]string{
"AutoScalingRollingUpdate": {
"MinInstancesInService": "1",
"MinInstancesInService": "0",

This comment has been minimized.

Copy link
@Jeffwan

Jeffwan May 20, 2019

Contributor

Any reason 0 is used here?

This comment has been minimized.

Copy link
@martina-if

martina-if May 21, 2019

Author Member

This is what I've seen in some examples when people were using pure spot instance groups. I'm not sure what's best here. What do you think?

This comment has been minimized.

Copy link
@Jeffwan

Jeffwan May 21, 2019

Contributor

0-downtime should be the goal. Seems you are right, 0 should be used because it's impossible for AWS to guarantee a minimum number of instances to be kept in service during the rolling update.

This comment has been minimized.

Copy link
@voxxit

voxxit May 22, 2019

I think I was one of the people who suggested this. Yes, this is only to be used for 100% spot instance node groups using launch configurations (not templates) because CloudFormation does not support anything other than this config when updating LCs.

docs/08-spot-instances.yaml Outdated Show resolved Hide resolved
@Jeffwan

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

This feature would be really helpful! Thanks!

@martina-if

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

@Jeffwan thanks for the review!

@martina-if martina-if force-pushed the martina-if:mixed-instances branch from f9418a2 to ab69a27 May 21, 2019

@martina-if martina-if force-pushed the martina-if:mixed-instances branch 3 times, most recently from 1471ad2 to 16d7cf4 May 30, 2019

@errordeveloper
Copy link
Member

left a comment

Few comments, looks very good overall!

@martina-if martina-if force-pushed the martina-if:mixed-instances branch from 16d7cf4 to e540267 Jun 4, 2019

@martina-if martina-if merged commit fd7ee50 into weaveworks:master Jun 4, 2019

2 checks passed

WIP Ready for review
Details
ci/circleci: make-eksctl-image Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.