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

Accept InstanceDistribution with only one instance type. #1772

Merged
merged 1 commit into from
Feb 4, 2020

Conversation

ryotarai
Copy link
Contributor

@ryotarai ryotarai commented Jan 29, 2020

Description

Currently, InstanceDistribution must have more than or equal to 2 instance types but AWS auto scaling API accepts configuration with only one instance type too. This PR makes InstanceDistribution accept that case.

Q. Why we do not use just instanceType of nodegroup?

We want to launch spot instances with one instance type.

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, and examples directory)
  • Manually tested
  • Added labels for change area (e.g. area/nodegroup) and target version (e.g. version/0.12.0)
  • Added note in docs/release_notes/draft.md (or relevant release note)

ryotarai added a commit to ryotarai/eksctl that referenced this pull request Jan 29, 2020
ryotarai added a commit to ryotarai/eksctl that referenced this pull request Jan 29, 2020
ryotarai added a commit to ryotarai/eksctl that referenced this pull request Jan 29, 2020
@ryotarai ryotarai requested a review from martina-if January 30, 2020 01:03
@ryotarai
Copy link
Contributor Author

ryotarai commented Jan 30, 2020

@martina-if Hi, would you please review this PR?

@martina-if
Copy link
Contributor

Hi @ryotarai , last time I checked this wasn't supported with MixedInstances, do you have a link to some docs where it says now this is supported? (it was supported through another mechanism though https://github.com/weaveworks/eksctl/pull/781/files)

@ryotarai
Copy link
Contributor Author

ryotarai commented Jan 30, 2020

Thank you for reviewing.

last time I checked this wasn't supported with MixedInstances, do you have a link to some docs where it says now this is supported?

https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_LaunchTemplate.html
(MixedInstancesPolicy.LaunchTemplate of CreateAutoScalingGroup request)

This page says You can specify between 1 and 20 instance types. and the following config worked in my environment.

  instancesDistribution:
    instanceTypes: ["c5.large"]
    spotInstancePools: 1
    onDemandBaseCapacity: 0
    onDemandPercentageAboveBaseCapacity: 0

it was supported through another mechanism though https://github.com/weaveworks/eksctl/pull/781/files

I didn't notice that. Thank you.
However, to set onDemandBaseCapacity, we need instancesDistribution.

@martina-if
Copy link
Contributor

This is great! Apparently they changed that in October.

Ok, two tiny details to complete the PR:

Can you please fix the comment in those two files^? It says minimum 2 instance types. Then if you rebase I'll approve and merge it :)

Thanks a lot!

ryotarai added a commit to ryotarai/eksctl that referenced this pull request Feb 2, 2020
ryotarai added a commit to ryotarai/eksctl that referenced this pull request Feb 2, 2020
@ryotarai
Copy link
Contributor Author

ryotarai commented Feb 2, 2020

I've updated the example and doc and rebased this to master branch: 5d9436d
(Should I squash commits into one commit?)

@martina-if
Copy link
Contributor

Hi @ryotarai , yes it would be great if you could squash them :)

@ryotarai
Copy link
Contributor Author

ryotarai commented Feb 4, 2020

I've squashed commits into 2cc4936

@martina-if martina-if merged commit 8d77ed1 into eksctl-io:master Feb 4, 2020
@martina-if
Copy link
Contributor

Thank you @ryotarai !

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.

2 participants