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

Support attribute-based instance selection for AWS #4588

Merged
merged 13 commits into from
Aug 10, 2022
Merged

Support attribute-based instance selection for AWS #4588

merged 13 commits into from
Aug 10, 2022

Conversation

AustinSiu
Copy link
Contributor

@AustinSiu AustinSiu commented Jan 10, 2022 •
edited
Loading

Which component this PR applies to?

What type of PR is this?

/kind feature

What this PR does / why we need it:

Support attribute-based instance selection using instance requirements.

This PR updates aws-sdk-go from v1.38.49 to v1.42.25 in order to pick up instance requirements feature changes.

Which issue(s) this PR fixes:

Fixes #4479

Special notes for your reviewer:

Testing done using an ASG w/ pre-existing instances:

  1. Created an ASG w/ Spot and CapacityOptimized allocation strategy
  2. Set InstanceRequirements Overrides w/ vcpu min: 10 max: 100
  3. Update Cluster Autoscaler Min size to 1, Max 10
  4. Waited for an instance to be added with Min 10 vcpu
  5. Update InstanceRequirements Overrides to vcpu min: 2, max: 100
  6. Cordon the 1st Node that came up in the ASG so that we can differentiate between nodes from step 2 vs step 5
  7. Create a deployment with pods with resource request of 1 vcpu each
  8. Scale deployment to 5 replicas
  9. Expectation and observed: CA raises Desired Capacity to 6 under newer requirements, vs 1-2 expected nodes if under step 2 requirements

Does this PR introduce a user-facing change?

Added support for attribute-based instance type selection for AWS using available instance requirements.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 10, 2022
@AustinSiu
Copy link
Contributor Author

@aleksandra-malinowska and @feiskyer may I ask if you are available to help review this PR?

@gjtempleton
Copy link
Member

/area provider/aws

@k8s-ci-robot k8s-ci-robot added the area/provider/aws Issues or PRs related to aws provider label Jan 25, 2022
@AustinSiu
Copy link
Contributor Author

@gjtempleton I haven't been able to get a response from the reviewers unfortunately. I've posted on the #pr-reviews Slack channel, but may I ask if there's anyone in particular I should reach out to for help reviewing?

@feiskyer
Copy link
Member

+AWS owners @Jeffwan @jaypipes @gjtempleton could you have a look?

bwagner5
bwagner5 suggested changes Jan 28, 2022
View reviewed changes
Copy link

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

Great work! Very clean addition to this rather complicated API add-on :D

A few comments on handling the case where a user specified the InstanceRequirements within the LT rather than within an ASG MixedInstancesPolicy Override. In that case, the InstanceRequirement will not show up in the MixedInstancePolicy, so the LT will need to be inspected to retrieve the Requirements.

cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper_test.go Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_manager.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2022
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 22, 2022
jaypipes
jaypipes suggested changes Feb 23, 2022
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.

Great start on this @AustinSiu! Thank you!

I've left a number of comments inline, please take a look.

cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_manager.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_manager.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_manager.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_manager.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
bwagner5
bwagner5 approved these changes Feb 23, 2022
View reviewed changes
Copy link

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

nice!

@AustinSiu AustinSiu requested a review from jaypipes February 24, 2022 23:04
jaypipes
jaypipes suggested changes Feb 25, 2022
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.

@AustinSiu this is very close! just a couple remaining nil guards to address inline, otherwise this is looking good! :)

cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/auto_scaling_groups.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_manager.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
cluster-autoscaler/cloudprovider/aws/aws_wrapper.go Outdated Show resolved Hide resolved
jaypipes
jaypipes approved these changes Mar 2, 2022
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.

Thanks for the revisions and your patience on this, @AustinSiu :)

@jaypipes
Copy link
Contributor

jaypipes commented Mar 2, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2022
@AustinSiu
Copy link
Contributor Author

/assign @feiskyer

@AustinSiu AustinSiu force-pushed the feature-aws-abs branch 2 times, most recently from 138013d to f581524 Compare July 19, 2022 21:26
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2022
@AustinSiu
Copy link
Contributor Author

@mwielgus @gjtempleton I've updated this change to handle the dependencies differently, does this address your concerns?

AustinSiu and others added 13 commits August 8, 2022 14:45
@AustinSiu
Support attribute-based instance selection for AWS
fed54b5
@AustinSiu
Add support for LT Instance Requirements with no overrides in MIP
7ff7b5c
@AustinSiu
Update and fix tests
bfed0d6
@AustinSiu
Update README with IAM for new APIs
87bbc43
@AustinSiu
Check MIP instance req overrides before LT overrides
11f5c5d
@AustinSiu
Add nil check when building tempalte with requirements, improve reada…
8ac2acf 
…bility
@AustinSiu
Improve nil checking, some naming, replace json logic for requirement…
b79be36 
… translation with field-setting
@AustinSiu
Add instance type nil check
e0bd8f9
@AustinSiu
Consolidate DescribeLaunchTemplateVersions logic into one function
923fe20
@AustinSiu
Add testing for describing launch template data
06ca5c8
@AustinSiu
Fix guard on instance type overrides when requirements present
67deeee
@AustinSiu
Restore previous sdk version
ebf2f2a
@AustinSiu
Update all imports across cloudprovider
833c6fd
gjtempleton
gjtempleton approved these changes Aug 10, 2022
View reviewed changes
Copy link
Member

@gjtempleton gjtempleton left a comment

Choose a reason for hiding this comment

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

Thanks for your persistence with this, great job.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2022
@k8s-ci-robot k8s-ci-robot merged commit 62c3b26 into kubernetes:master Aug 10, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AustinSiu, bwagner5, gjtempleton, jaypipes

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

This was referenced Aug 30, 2022
Closed
Closed
Merged
k8s-ci-robot added a commit that referenced this pull request Sep 5, 2022
@k8s-ci-robot
Merge pull request #5150 from AustinSiu/automated-cherry-pick-of-#4950-…
fadc0cf 
…#4424-#4588-upstream-cluster-autoscaler-release-1.24

Automated cherry pick of #4950: Initial vendoring
#4424: feat(aws): reduce auto-discovery API calls
#4588: Support attribute-based instance selection for AWS
navinjoy pushed a commit to navinjoy/autoscaler that referenced this pull request Oct 26, 2022
@k8s-ci-robot
Merge pull request kubernetes#4588 from AustinSiu/feature-aws-abs
2ee2eb5 
Support attribute-based instance selection for AWS
@bwagner5 bwagner5 mentioned this pull request Mar 10, 2023
Closed
@SayakMukhopadhyay SayakMukhopadhyay mentioned this pull request May 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sauryadas sauryadas sauryadas left review comments

@gjtempleton gjtempleton gjtempleton approved these changes

@bwagner5 bwagner5 bwagner5 approved these changes

@jaypipes jaypipes jaypipes approved these changes

@aleksandra-malinowska aleksandra-malinowska Awaiting requested review from aleksandra-malinowska

@feiskyer feiskyer Awaiting requested review from feiskyer

@mwielgus mwielgus Awaiting requested review from mwielgus

Assignees

@jaypipes jaypipes

@feiskyer feiskyer

@gjtempleton gjtempleton

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support AWS "Instance type requirements"
9 participants
@AustinSiu @gjtempleton @feiskyer @jaypipes @k8s-ci-robot @bwagner5 @mwielgus @sauryadas @jbartosik