Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

feat: support for AWS mixed instances #1531

Merged
merged 3 commits into from
Feb 12, 2019

Conversation

koen92
Copy link
Contributor

@koen92 koen92 commented Jan 14, 2019

AWS recently introduced the option of using autoscalinggroups and combine multiple instances.
https://aws.amazon.com/blogs/aws/new-ec2-auto-scaling-groups-with-multiple-instance-types-purchase-options/

This PR changes the CF LaunchConfiguration templates into CF LaunchTemplates, which will add the ability to combine multiple instance types within an AutoScalingGroup.

Please let me know what you think about this! (since it's quite a refactor in the CF node pool template).

Related issue: #1500

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 14, 2019
@codecov-io
Copy link

codecov-io commented Jan 14, 2019

Codecov Report

Merging #1531 into master will increase coverage by 0.45%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1531      +/-   ##
==========================================
+ Coverage   25.46%   25.92%   +0.45%     
==========================================
  Files          97       98       +1     
  Lines        5003     5034      +31     
==========================================
+ Hits         1274     1305      +31     
  Misses       3582     3582              
  Partials      147      147
Impacted Files Coverage Δ
pkg/api/worker_node_pool.go 11.86% <0%> (-0.21%) ⬇️
pkg/api/mixed_instances.go 100% <100%> (ø)
pkg/api/asg.go 100% <100%> (+100%) ⬆️
pkg/api/cluster.go 0% <0%> (ø) ⬆️
pkg/model/node_pool_config.go 34.88% <0%> (+3.17%) ⬆️

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 c50c2a0...8dbfa47. Read the comment docs.

@koen92
Copy link
Contributor Author

koen92 commented Jan 14, 2019

/assign @mumoshu

Autoscaler for mixedInstances is still work in progress, see:
kubernetes/autoscaler#1473
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mumoshu

If they are not already assigned, you can assign the PR to them by writing /assign @mumoshu in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@paalkr
Copy link
Contributor

paalkr commented Jan 21, 2019

Absolutely brilliant. Please make sure to add support for spot resources in the mix as well, as this is supported by this new AWS feature. I can totally see spot fleet requests being 100% replaced by AutoScaling using a launch template.
https://docs.aws.amazon.com/autoscaling/ec2/userguide/AutoScalingGroup.html#asg-purchase-options
https://docs.aws.amazon.com/autoscaling/ec2/userguide/create-asg-launch-template.html

EDIT
Looking at the PR code, the features I mentioned seems to be added. I'm looking forward to try out this new capability with the 0.13 release.

"SpotAllocationStrategy" : "{{.AutoScalingGroup.MixedInstances.SpotAllocationStrategy}}",
{{end}}
"SpotInstancePools" : {{.AutoScalingGroup.MixedInstances.SpotInstancePools}},
"SpotMaxPrice" : "{{.AutoScalingGroup.MixedInstances.SpotMaxPrice}}"
Copy link
Contributor

@paalkr paalkr Jan 31, 2019

Choose a reason for hiding this comment

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

SpotMaxPrice should be possible to leave out. If you are excluding SpotMaxPrice in the policy AWS will automatically set the on demand price as max price. This is especially important when having an override list of mixed instance types and sizes.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-autoscaling-autoscalinggroup-instancesdistribution.html#cfn-as-mixedinstancespolicy-spotmaxprice

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest that you add
{{if .AutoScalingGroup.MixedInstances.SpotMaxPrice}}
"SpotMaxPrice" : "{{.AutoScalingGroup.MixedInstances.SpotMaxPrice}}"
{{end}}

@paalkr
Copy link
Contributor

paalkr commented Jan 31, 2019

@davidmccormick
Copy link
Contributor

What will happen when this change is rolled into existing clusters?

@paalkr
Copy link
Contributor

paalkr commented Feb 8, 2019

For what I understand a Launch Configuration will be replaced by a Launch template, and if you don't specify AutoScalingGroup.MixedInstances.Enabled or AutoScalingGroup.MixedInstances at all, the AutoScaling group should just do a rolling update of all nodes.

The author @koen92 might have additional insight.

@sander-su
Copy link

What will happen when this change is rolled into existing clusters?

As the co-author of this pr;
The launchconfiguration for ALL worker autoscaling groups (only workers, not masters / etcd) will be replaced by a launchtemplate.
I thought it will do a rolling update of the nodes in the asg but I will retest this and post the result to be 100% sure. (could also be the replace of the asg)

@paalkr Good one, we will change this.

@koen92
Copy link
Contributor Author

koen92 commented Feb 11, 2019

@paalkr Fixed it!

@paalkr
Copy link
Contributor

paalkr commented Feb 11, 2019

Thx @koen92

According to the CloudFormation AutoScalingGroup documentation, changing either the LaunchTemplate or the LaunchConfiguration requires no interruption.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-as-group.html#cfn-as-group-launchtemplate

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-as-group.html#cfn-as-group-launchconfigurationname

@paalkr
Copy link
Contributor

paalkr commented Feb 11, 2019

Btw, I have tested using a Mixed Instance Policy with 100% spot resources, and an instance distribution and instance type mix (overrides), that simulates my currently running worker pool powered by a regular spotfleet (40+ nodes). When changing something in the LaunchTemplate that will issue a rolling update (like AMI ID or user data), the AutoScaling group nicely performs a rolling update of the spot resources - one by one. This actually solves a huge nightmare of how AWS spot fleet currently are handled. Where a change to a spot fleet request, replaces all spot instances in a one shot operation.

I can't see any situation where the old spot fleets are preferable over this new mixed instance policy with overrides.

@sander-su
Copy link

Just confirmed that cloudformation is doing a nice rolling update moving from launchconfiguration to launchtemplate. Seeing is believing, always nice if the docs are right ;).

@davidmccormick can you also approve this pr? have not seen a reaction @mumoshu recently.

@davidmccormick davidmccormick merged commit 8f015b3 into kubernetes-retired:master Feb 12, 2019
@davidmccormick davidmccormick added this to the v0.13.0 milestone Feb 12, 2019
@davidmccormick
Copy link
Contributor

Thank you for your contribution - good work! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants