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

[cluster-autoscaler] Additional expander options #4206

Closed
ryanmcnamara opened this issue Jul 19, 2021 · 8 comments
Closed

[cluster-autoscaler] Additional expander options #4206

ryanmcnamara opened this issue Jul 19, 2021 · 8 comments
Labels
area/cluster-autoscaler kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@ryanmcnamara
Copy link
Contributor

ryanmcnamara commented Jul 19, 2021

Users are able to specify the expander option to change which nodegroup (e.g. ASG) is selected when there are multiple that satisfy the scheduling requirements for a pod. The feature request is to allow selecting an additional expanders in the event that the chosen expander still arbitrarily ties multiple nodegroups. This is very similar to this closed ticket #2564.

For our specific use case, we are currently using the least-waste expander, but we want to have some nodegroups which are preferred. What we would then like is to use the priority expander but fall back to least-waste when multiple still match.

I see several ways this feature could be added, in my order of preference

  1. Use a list of expanders to use, with some validation of course very similar to what @adrienjt mentions here. Satisfying my above use case would look like setting expander=priority,least-waste.
  2. Add a priority-expander option, specifically only used in the case where expander=priority. Satisfying the above use case would look like setting expander=priority and priority-expander=least-waste

I think the first option is cleaner / easier to understand but is perhaps more complicated to implement. I'd be happy to submit a PR for this but wanted to get any feedback first, thanks!

cc @Jeffwan @bpineau @adrienjt

@ryanmcnamara ryanmcnamara added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 19, 2021
@x13n
Copy link
Member

x13n commented Jul 20, 2021

mostpods and waste expanders, similarly to priority, have a notion of fallback strategy. Option 1 is much cleaner from API point of view, and also fairly easy to implement - we'd just need to update expander factory to allow chaining (and return an error if there's an expander configured after a one that doesn't have a fallback).

@ryanmcnamara
Copy link
Contributor Author

ryanmcnamara commented Jul 22, 2021

Thanks @x13n
I'll probably take a shot at this over the next several days and push something up for PR review. Would you like to review or know who I should get for a review?

@x13n
Copy link
Member

x13n commented Jul 23, 2021

I'm happy to review the change, but you'll still need a repo owner to approve.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 14, 2021
@x13n
Copy link
Member

x13n commented Dec 15, 2021

This was done in #4233

/close

@k8s-ci-robot
Copy link
Contributor

@x13n: Closing this issue.

In response to this:

This was done in #4233

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@stevehipwell
Copy link
Contributor

@x13n which versions of CA have this code?

I think there is a bigger question here on backporting vs recommending newer CA builds for supported K8s versions, does this need a separate issue opening?

@x13n
Copy link
Member

x13n commented Dec 16, 2021

None yet, it will be a part of 1.23 release. I think backporting should mostly be limited to bug fixes, not new features like this one, but if you disagree then yes, a separate issue sounds like a good starting point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cluster-autoscaler kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

6 participants