Skip to content

Integrate resource quotas with scale up #8835

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
Open
norbertcyran wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Integrate resource quotas with scale up #8835

norbertcyran wants to merge 1 commit into from
+587 −164

Conversation

@norbertcyran
Copy link
Contributor

@norbertcyran norbertcyran commented Nov 20, 2025 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Integrating new resourcequotas package with scale up orchestrator. resourcequotas replaces the old resource management done by k8s.io/autoscaling/cluster-autoscaler/core/scaleup/resource package.

Which issue(s) this PR fixes:

Part of #8703

Special notes for your reviewer:

Please focus on possible regressions. I tested it manually on GKE and added some unit tests, but since the changes directly touch the most important CA functionality, let's ensure that we don't break anything here.

Does this PR introduce a user-facing change?

NONE

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/cluster-autoscaler labels Nov 20, 2025
@k8s-ci-robot k8s-ci-robot added do-not-merge/needs-area needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. and removed do-not-merge/needs-area labels Nov 20, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 20, 2025

Hi @norbertcyran. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 20, 2025
@norbertcyran norbertcyran force-pushed the resource-quotas-scale-up branch 2 times, most recently from 4433dbd to 5ff5949 Compare November 28, 2025 09:45
norbertcyran
norbertcyran commented Nov 28, 2025
View reviewed changes
cluster-autoscaler/core/scaleup/orchestrator/orchestrator.go
Comment on lines +806 to +819
func (o *ScaleUpOrchestrator) newQuotasTracker() (*resourcequotas.Tracker, error) {
var nodes []*apiv1.Node
nodeInfos, err := o.autoscalingCtx.ClusterSnapshot.ListNodeInfos()
if err != nil {
return nil, err
}
for _, nodeInfo := range nodeInfos {
node := nodeInfo.Node()
if utils.IsVirtualKubeletNode(node) {
continue
}
nodes = append(nodes, nodeInfo.Node())
}
return o.quotasTrackerFactory.NewQuotasTracker(o.autoscalingCtx, nodes)
}
Copy link
Contributor Author

@norbertcyran norbertcyran Nov 28, 2025

Choose a reason for hiding this comment

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

That probably could be done better:

  1. Checking for virtual kubelet nodes could be done via resourcequotas.NodeFilter
  2. Passing all the nodes, including upcoming nodes via nodes parameter in ScaleUp (this way we could also remove some logic in ScaleUp around upcoming nodes, which are used for checking total node limits)

That would make it also easier to integrate with the other implementations of the Orchestrator

Notes to interviewers: do you think it's worth the hassle?

@norbertcyran norbertcyran force-pushed the resource-quotas-scale-up branch from 5ff5949 to 41a74a5 Compare November 28, 2025 16:40
@norbertcyran norbertcyran marked this pull request as ready for review November 28, 2025 16:56
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2025
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 28, 2025
@norbertcyran
Copy link
Contributor Author

norbertcyran commented Nov 28, 2025 •
edited
Loading

/hold let's not merge it before #8834. The change is ready for review, though

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2025
@norbertcyran
Copy link
Contributor Author

norbertcyran commented Nov 28, 2025

/assign towca BigDarkClown

@norbertcyran norbertcyran force-pushed the resource-quotas-scale-up branch from 41a74a5 to 658a76c Compare December 2, 2025 14:23
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 2, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: norbertcyran
Once this PR has been reviewed and has the lgtm label, please ask for approval from bigdarkclown. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2025
@norbertcyran norbertcyran force-pushed the resource-quotas-scale-up branch from 658a76c to d5d93e4 Compare December 5, 2025 14:49
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2025
@norbertcyran norbertcyran force-pushed the resource-quotas-scale-up branch from d5d93e4 to ebcee0f Compare December 5, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@elmiko elmiko Awaiting requested review from elmiko

@BigDarkClown BigDarkClown Awaiting requested review from BigDarkClown

@x13n x13n Awaiting requested review from x13n

Assignees

@towca towca

@BigDarkClown BigDarkClown

Labels

area/cluster-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note-none Denotes a PR that doesn't merit a release note. 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.

4 participants

@norbertcyran @k8s-ci-robot @towca @BigDarkClown