Skip to content

TAS: Add MostAllocated algorithm #4228

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
Merged
merged 23 commits into from
Feb 14, 2025
Merged

TAS: Add MostAllocated algorithm #4228

merged 23 commits into from
Feb 14, 2025

Conversation

@PBundyra
Copy link
Contributor

@PBundyra PBundyra commented Feb 11, 2025 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR introduces a new default algorithm to TAS (old one gated by the LargestFitTAS feature flag). This algorithm selects the domain that can accommodate all pods, and has the least amount of free resources, instead of the most amount of free resources, as it's done currently. This helps mitigate the resource fragmentation. The KEP has been updated to reflect these changes.

Which issue(s) this PR fixes:

Fixes #3756

Special notes for your reviewer:

Does this PR introduce a user-facing change?

TAS: Add a new default algorithm to minimize resource fragmentation. The old algorithm is gated behind the `TASLargestFit` feature gate.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 11, 2025
@PBundyra
Copy link
Contributor Author

/assign @mimowo

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 11, 2025
@netlify
Copy link

netlify bot commented Feb 11, 2025 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 007bd35
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67af1d8b9f3a440008ba4289

@mimowo
Copy link
Contributor

mimowo commented Feb 11, 2025

I skimmed through it and it looks great!
cc @gabesaba @tenzen-y would you like to give it a proper pass too / first?

@PBundyra
Copy link
Contributor Author

cc @mwysokin for visibility

@PBundyra
Copy link
Contributor Author

I'm adding more optimizations to updateCountsToMinimum()

@PBundyra PBundyra changed the title Add BestFit algorithm to TAS [WIP] Add BestFit algorithm to TAS Feb 11, 2025
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2025
gabesaba
gabesaba reviewed Feb 11, 2025
View reviewed changes
@PBundyra PBundyra changed the title [WIP] Add BestFit algorithm to TAS Add BestFit algorithm to TAS Feb 11, 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 Feb 11, 2025
@PBundyra
Copy link
Contributor Author

@gabesaba PTAL

@tenzen-y
Copy link
Member

I skimmed through it and it looks great! cc @gabesaba @tenzen-y would you like to give it a proper pass too / first?

let me see.

I'm adding more optimizations to updateCountsToMinimum()

Is this working in progress?

mimowo
mimowo reviewed Feb 12, 2025
View reviewed changes
mimowo
mimowo reviewed Feb 12, 2025
View reviewed changes
@PBundyra
Copy link
Contributor Author

Is this working in progress?

It's ready for review

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2025
gabesaba
gabesaba reviewed Feb 12, 2025
View reviewed changes
@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 12, 2025
@tenzen-y
Copy link
Member

Is this working in progress?

It's ready for review

ACK

tenzen-y
tenzen-y reviewed Feb 12, 2025
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Pretty cool feature

mimowo
mimowo reviewed Feb 14, 2025
View reviewed changes
mimowo
mimowo reviewed Feb 14, 2025
View reviewed changes
mimowo
mimowo reviewed Feb 14, 2025
View reviewed changes
mimowo
mimowo reviewed Feb 14, 2025
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

Overall LGTM, left some nits at this point

@mimowo
Copy link
Contributor

mimowo commented Feb 14, 2025

/hold cancel
I will add LGTM when the nits are addressed

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2025
mimowo
mimowo reviewed Feb 14, 2025
View reviewed changes
@mimowo
Copy link
Contributor

mimowo commented Feb 14, 2025

/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 Feb 14, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: ab6cd17d4dc295d83fb4220b1468aa291290f564

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, PBundyra, tenzen-y

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 merged commit c623953 into kubernetes-sigs:main Feb 14, 2025
19 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.11 milestone Feb 14, 2025
dgrove-oss pushed a commit to dgrove-oss/kueue that referenced this pull request Feb 14, 2025
@PBundyra @mimowo @dgrove-oss
Add BestFit algorithm to TAS (kubernetes-sigs#4228)
a0a0a3f 
* Add info about BestFit algorithm to KEP

* Implement BestFit algorithm

* Use built-in sort.Search function, add optimizations at every level

* Improve bestFit function, add test scenario

* Change BestFit into LargestFit, fix a test scenario

* Fix checker, correct featureGate name

* Adjust new feature gate

* Fix tests with changed feature gate

* Cleaned up logic

* Adjusted test results for the BestFit default algorithm

* Change naming LargestFit -> LeastAllocated, BestFit -> MostAllocated

* Pick the first MostAllocated domain that can accommodate all pods

* Fix linter, improve naming

* Simplify logic

* Simplify logic, adjust comments

* Mark some test scenarios to drop after removing the feature gate

* Add TODOs comments, mention dropping the feature gate in GA in KEP

* Improve logic, add test

* Improve logic

* Update keps/2724-topology-aware-scheduling/README.md

Co-authored-by: Michał Woźniak <[email protected]>

* Address nits

* Address nits

* Rename feature gate

---------

Co-authored-by: Michał Woźniak <[email protected]>
@gabesaba gabesaba mentioned this pull request Feb 17, 2025
Closed
@PBundyra PBundyra changed the title Add BestFit algorithm to TAS TAS: Add BestFit algorithm Feb 26, 2025
@PBundyra PBundyra changed the title TAS: Add BestFit algorithm TAS: Add MostAllocated algorithm Mar 4, 2025
@PBundyra PBundyra mentioned this pull request Mar 10, 2025
Merged
@mimowo mimowo mentioned this pull request Mar 12, 2025
Merged
kannon92 pushed a commit to openshift/kubernetes-sigs-kueue that referenced this pull request Mar 13, 2025
@PBundyra @mimowo @kannon92
Add BestFit algorithm to TAS (kubernetes-sigs#4228)
6d9d73d 
* Add info about BestFit algorithm to KEP

* Implement BestFit algorithm

* Use built-in sort.Search function, add optimizations at every level

* Improve bestFit function, add test scenario

* Change BestFit into LargestFit, fix a test scenario

* Fix checker, correct featureGate name

* Adjust new feature gate

* Fix tests with changed feature gate

* Cleaned up logic

* Adjusted test results for the BestFit default algorithm

* Change naming LargestFit -> LeastAllocated, BestFit -> MostAllocated

* Pick the first MostAllocated domain that can accommodate all pods

* Fix linter, improve naming

* Simplify logic

* Simplify logic, adjust comments

* Mark some test scenarios to drop after removing the feature gate

* Add TODOs comments, mention dropping the feature gate in GA in KEP

* Improve logic, add test

* Improve logic

* Update keps/2724-topology-aware-scheduling/README.md

Co-authored-by: Michał Woźniak <[email protected]>

* Address nits

* Address nits

* Rename feature gate

---------

Co-authored-by: Michał Woźniak <[email protected]>
kannon92 pushed a commit to openshift/kubernetes-sigs-kueue that referenced this pull request Mar 13, 2025
@PBundyra @mimowo @kannon92
Add BestFit algorithm to TAS (kubernetes-sigs#4228)
3ff838a 
* Add info about BestFit algorithm to KEP

* Implement BestFit algorithm

* Use built-in sort.Search function, add optimizations at every level

* Improve bestFit function, add test scenario

* Change BestFit into LargestFit, fix a test scenario

* Fix checker, correct featureGate name

* Adjust new feature gate

* Fix tests with changed feature gate

* Cleaned up logic

* Adjusted test results for the BestFit default algorithm

* Change naming LargestFit -> LeastAllocated, BestFit -> MostAllocated

* Pick the first MostAllocated domain that can accommodate all pods

* Fix linter, improve naming

* Simplify logic

* Simplify logic, adjust comments

* Mark some test scenarios to drop after removing the feature gate

* Add TODOs comments, mention dropping the feature gate in GA in KEP

* Improve logic, add test

* Improve logic

* Update keps/2724-topology-aware-scheduling/README.md

Co-authored-by: Michał Woźniak <[email protected]>

* Address nits

* Address nits

* Rename feature gate

---------

Co-authored-by: Michał Woźniak <[email protected]>
@PBundyra PBundyra deleted the bestFit-TAS branch August 12, 2025 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mimowo mimowo mimowo left review comments

@gabesaba gabesaba gabesaba left review comments

@tenzen-y tenzen-y Awaiting requested review from tenzen-y

Assignees

@mimowo mimowo

@tenzen-y tenzen-y

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. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v0.11

Development

Successfully merging this pull request may close these issues.

TAS: optimize the algorithm to minimize fragmentation

5 participants

@PBundyra @mimowo @tenzen-y @k8s-ci-robot @gabesaba