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 MixedInstancesPolicy struct to better handle instance type #2248

Merged
merged 1 commit into from
Aug 26, 2019
Merged

Add MixedInstancesPolicy struct to better handle instance type #2248

merged 1 commit into from
Aug 26, 2019

Conversation

Jeffwan
Copy link
Contributor

@Jeffwan Jeffwan commented Aug 12, 2019 •
edited
Loading

Fix issue that user can not build instance type using MixedInstanceType.

#2246

@jaypipes

@k8s-ci-robot k8s-ci-robot added 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. labels Aug 12, 2019
@Jeffwan Jeffwan force-pushed the mixed_instance_policy branch 2 times, most recently from 702d4bc to ab24b62 Compare August 12, 2019 18:24
@Jeffwan Jeffwan mentioned this pull request Aug 12, 2019
Closed
@jsharpe
Copy link

jsharpe commented Aug 13, 2019

I've been looking at this issue; I've not tried this patch but based on the code in master I'm seeing the LaunchTemplate from the MixedInstances configuration returning an empty string for the template version. Setting this to "$Default" when the string is empty appears to fix the issue of it not being able to construct node information when scaling to 0.

jaypipes
jaypipes suggested changes Aug 13, 2019
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Fix issue that user can not build instance type using MixedInstanceType.

#2246

Jiaxin, I'd appreciate a little bit more descriptive commit message/PR comment than above. Something that describes exactly what was being fixed and why.

Perhaps something like this?

"Ensures that when MixedInstancePolicy is used in an AWS AutoScalingGroup, that
the buildInstanceType() AWS manager method returns an instance type after looking
at the MixedInstancePolicy.LaunchTemplateSpecification. The buildInstanceType()
method is called in numerous places including on cluster scale up actions.

Also adds documentation highlighting the minimum version of cluster autoscaler
supporting MixedInstancePolicy is 1.14."

cluster-autoscaler/cloudprovider/aws/README.md Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go Outdated Show resolved Hide resolved
@Jeffwan Jeffwan force-pushed the mixed_instance_policy branch from ab24b62 to e328966 Compare August 15, 2019 21:18
@Jeffwan
Copy link
Contributor Author

Jeffwan commented Aug 15, 2019

@jaypipes Thanks for the feedbacks. Resolve all the comments and please have another look

@Jeffwan
Add MixedInstancesPolicy struct to better handle instance type
8d567eb 
Ensures that when MixedInstancePolicy is used in an AWS AutoScalingGroup, that
the buildInstanceType() AWS manager method returns an instance type after looking
at the MixedInstancePolicy.LaunchTemplateSpecification. The buildInstanceType()
method is called in numerous places including on cluster scale up actions.

Also adds documentation highlighting the minimum version of cluster autoscaler
supporting MixedInstancePolicy is 1.14
@Jeffwan Jeffwan force-pushed the mixed_instance_policy branch from e328966 to 8d567eb Compare August 15, 2019 21:40
@Jeffwan
Copy link
Contributor Author

Jeffwan commented Aug 15, 2019

Fix issue that user can not build instance type using MixedInstanceType.
#2246

Jiaxin, I'd appreciate a little bit more descriptive commit message/PR comment than above. Something that describes exactly what was being fixed and why.

Perhaps something like this?

"Ensures that when MixedInstancePolicy is used in an AWS AutoScalingGroup, that
the buildInstanceType() AWS manager method returns an instance type after looking
at the MixedInstancePolicy.LaunchTemplateSpecification. The buildInstanceType()
method is called in numerous places including on cluster scale up actions.

Also adds documentation highlighting the minimum version of cluster autoscaler
supporting MixedInstancePolicy is 1.14."

Great! I add the comment to git commit.

jaypipes
jaypipes approved these changes Aug 15, 2019
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Awesome, thank you @Jeffwan! :)

@Jeffwan
Copy link
Contributor Author

Jeffwan commented Aug 19, 2019

/cc @MaciekPytel

@losipiuk
Copy link
Contributor

/lgtm
/approve
Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: losipiuk

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2019
@k8s-ci-robot k8s-ci-robot merged commit 8d9010e into kubernetes:master Aug 26, 2019
@Jeffwan Jeffwan deleted the mixed_instance_policy branch August 26, 2019 23:02
@jsharpe jsharpe mentioned this pull request Aug 27, 2019
Closed
@Jeffwan Jeffwan mentioned this pull request Aug 28, 2019
Closed
@coreypobrien
Copy link
Contributor

@losipiuk @Jeffwan Will this need to be cherry picked back to 1.15 and 1.14?

@Jeffwan
Copy link
Contributor Author

Jeffwan commented Sep 27, 2019

@coreypobrien This is a bug fix and should be a cherry-pick candidate to 1.14 and 1.15.
Can you help cherry-pick the change and I can approve it.

This was referenced Sep 30, 2019
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Sep 30, 2019
@k8s-ci-robot
Merge pull request #2398 from coreypobrien/cherry-2248-ca-1.15
c9af5e1 
CA 1.15 cherry-pick #2248
k8s-ci-robot added a commit that referenced this pull request Oct 3, 2019
@k8s-ci-robot
Merge pull request #2397 from coreypobrien/cherry-2248-ca-1.14
9bbe980 
CA 1.14 cherry-pick #2248
@Jeffwan Jeffwan mentioned this pull request Oct 11, 2019
Closed
@danielmellado danielmellado mentioned this pull request Oct 24, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaypipes jaypipes jaypipes approved these changes

@feiskyer feiskyer Awaiting requested review from feiskyer

@krzysztof-jastrzebski krzysztof-jastrzebski Awaiting requested review from krzysztof-jastrzebski

@MaciekPytel MaciekPytel Awaiting requested review from MaciekPytel

Assignees

@losipiuk losipiuk

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Jeffwan @jsharpe @losipiuk @k8s-ci-robot @coreypobrien @jaypipes