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

fixing #1012, cluster autoscaler requres a valid LT intance type #1013

Merged
merged 2 commits into from
Jul 17, 2019
Merged

fixing #1012, cluster autoscaler requres a valid LT intance type #1013

merged 2 commits into from
Jul 17, 2019

Conversation

alexei-led
Copy link
Contributor

Kubernetes cluster autoscaler (AWS) requres a valid LT intance type to scale up/down ASG. See #1012

Description

Adding first instance type from distribution instance types as a default LaunchTemplate instance type.

Checklist

  • Code compiles correctly (i.e make build)
  • Updated tests that cover your change
  • All unit tests passing (i.e. make test)
  • Manually tested

@errordeveloper
Copy link
Contributor

@alexei-led thanks for this PR! This looks good overall, but I'm just not sure if there are any implications on functionality, would you mind to provide a reference to AWS docs that suggests what does it mean when mixed instances are set along with traditional instance type?

@alexei-led
Copy link
Contributor Author

alexei-led commented Jul 11, 2019

@errordeveloper Ilya take a look at AWS LaunchTemplate API docs

The overrides are used to override the instance type specified by the launch template with multiple instance types that can be used to launch On-Demand Instances and Spot Instances.

i.e. when LT has Overrides section the instanceType in LT is ignored. I've also checked this.

@martina-if
Copy link
Contributor

👍 Thanks @alexei-led . I tested this creating 3 nodegroups: all spot instances, all on demand and mixed instances and it seems to work fine.

@errordeveloper
Copy link
Contributor

Let's merge it for 0.2.0. We have 0.1.40 underway, and I'd prefer that to fix exactly one bug (#1003).

@whereisaaron
Copy link

whereisaaron commented Jul 26, 2019

@alexei-led @errordeveloper is m5 a good choice for this? Remember that some AZ's don't have m5 instances like ap-southeast-2b. Will that affect being able to use this change in that zone, or will the override prevent any errors.

@alexei-led
Copy link
Contributor Author

@whereisaaron any non-empty string is a good choice - cluster autoscaler validates only that instanceType is not empty, no actual validation is done :)

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.

4 participants