Skip to content

feat(limits): support NodePool node limits#2526

Merged
k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
maxcao13:tmp-max-nodes-nodepools
Mar 31, 2026
Merged

feat(limits): support NodePool node limits#2526
k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
maxcao13:tmp-max-nodes-nodepools

Conversation

@maxcao13
Copy link
Copy Markdown
Contributor

@maxcao13 maxcao13 commented Sep 20, 2025 •
edited by jmdeal
Loading

Fixes: #2657, #732

Description
This PR attempts to allow NodePool Node limits to work in the same behaviour as CPU and memory nodepool limits, and properly limit the resource from being created over the limit specifically during provisioning.

See the original issue for more details.

How was this change tested?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 20, 2025
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 20, 2025
@maxcao13 maxcao13 mentioned this pull request Sep 20, 2025
Closed
@coveralls
Copy link
Copy Markdown

coveralls commented Sep 20, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 22125100432

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 80.529%
Totals Coverage Status
Change from base Build 22117311323: 0.04%
Covered Lines: 11874
Relevant Lines: 14745
💛 - Coveralls

@maxcao13 maxcao13 force-pushed the tmp-max-nodes-nodepools branch from 0d11d60 to adcd3f0 Compare October 8, 2025 20:42
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2025
@maxcao13 maxcao13 force-pushed the tmp-max-nodes-nodepools branch 2 times, most recently from 9963517 to 1f521b4 Compare October 8, 2025 20:45
@maxcao13 maxcao13 force-pushed the tmp-max-nodes-nodepools branch from 1f521b4 to 02846f7 Compare November 26, 2025 08:36
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2025
@maxcao13 maxcao13 force-pushed the tmp-max-nodes-nodepools branch from 02846f7 to 7cc2f59 Compare November 26, 2025 08:40
@maxcao13 maxcao13 changed the title WIP: max node limits per NodePool feat(limits): eager node limits Nov 26, 2025
@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 26, 2025
@maxcao13 maxcao13 force-pushed the tmp-max-nodes-nodepools branch from 7cc2f59 to 28e49d7 Compare November 26, 2025 09:25
@maxcao13 maxcao13 mentioned this pull request Nov 26, 2025
Open
@maxcao13 maxcao13 force-pushed the tmp-max-nodes-nodepools branch from 28e49d7 to f6042e1 Compare November 26, 2025 09:27
ryan-mist
ryan-mist reviewed Dec 16, 2025
View reviewed changes
Comment thread pkg/controllers/state/statenode.go
@maxcao13 maxcao13 force-pushed the tmp-max-nodes-nodepools branch from f6042e1 to 03c3a72 Compare December 23, 2025 05:01
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 23, 2025
@maxcao13
Copy link
Copy Markdown
Contributor Author

maxcao13 commented Jan 19, 2026 •
edited
Loading

As talked about in the recent karpenter working group, I can get rid of the flag and just make this the default behaviour if we are bringing functionality parity for node limits in-line the other resource limits (cpu, memory), but not exactly sure if that's the case or not.

EDIT: based on #2657 (comment), I believe that node limits are not in line with the other resource limits. I will remove the feature flag in the next round of reviews if a maintainer agrees with me.

@maxcao13 maxcao13 changed the title feat(limits): eager node limits feat(limits): support NodePool node limits Jan 23, 2026
@DerekFrank
Copy link
Copy Markdown
Collaborator

DerekFrank commented Feb 16, 2026

I'm aligned, lets move forward with a flagless approach

@DerekFrank DerekFrank mentioned this pull request Feb 17, 2026
Open
@maxcao13
feat(limits): support node limits
0318d67 
Nodepool node limits are now enforced during provisioning, aligning it with other resource limits.
This commit adds the Node resource as a static field to the capacity of a StateNode which will always equal 1.

Signed-off-by: Max Cao <macao@redhat.com>
@maxcao13 maxcao13 force-pushed the tmp-max-nodes-nodepools branch from 03c3a72 to 0318d67 Compare February 18, 2026 03:18
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 18, 2026
@maxcao13
Copy link
Copy Markdown
Contributor Author

maxcao13 commented Feb 18, 2026

@DerekFrank
Thanks! Fixed in latest push, along with a unit test case that covers the use case. If there needs to be more validation here, please let me know.

@DerekFrank
Copy link
Copy Markdown
Collaborator

DerekFrank commented Mar 17, 2026

/assign

@DerekFrank
Copy link
Copy Markdown
Collaborator

DerekFrank commented Mar 17, 2026

Sorry I haven't had a chance to take a look at this, I'm hoping to take a look sometime this week

DerekFrank
DerekFrank approved these changes Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@DerekFrank DerekFrank left a comment

Choose a reason for hiding this comment

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

/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 31, 2026
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Mar 31, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DerekFrank, maxcao13

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Mar 31, 2026
@k8s-ci-robot k8s-ci-robot merged commit 9f749d9 into kubernetes-sigs:main Mar 31, 2026
17 checks passed
@maxcao13 maxcao13 deleted the tmp-max-nodes-nodepools branch March 31, 2026 22:21
@w1t w1t mentioned this pull request Apr 4, 2026
Open
@ryan-mist ryan-mist mentioned this pull request Apr 8, 2026
Merged
3 tasks
@jigisha620 jigisha620 mentioned this pull request Apr 13, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DerekFrank DerekFrank DerekFrank approved these changes

@engedaam engedaam Awaiting requested review from engedaam

@tallaxes tallaxes Awaiting requested review from tallaxes

+1 more reviewer

@ryan-mist ryan-mist ryan-mist left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@DerekFrank DerekFrank

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.

5 participants

@maxcao13 @coveralls @DerekFrank @k8s-ci-robot @ryan-mist
⚡