Skip to content

Follow-up to Flavor Fungibility Prefers Fit over NoBorrow#6181

Merged
k8s-ci-robot merged 3 commits intokubernetes-sigs:mainfrom
pajakd:fungiblity_followup
Jul 25, 2025
Merged

Follow-up to Flavor Fungibility Prefers Fit over NoBorrow#6181
k8s-ci-robot merged 3 commits intokubernetes-sigs:mainfrom
pajakd:fungiblity_followup

Conversation

@pajakd
Copy link
Copy Markdown
Contributor

@pajakd pajakd commented Jul 25, 2025

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Follow-up to #6132 addressing the comments #6132 (comment)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 25, 2025
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 25, 2025
pajakd
pajakd commented Jul 25, 2025
View reviewed changes
Comment thread pkg/scheduler/preemption/preemption_oracle.go
if len(candidates) == 0 {
return preemptioncommon.NoCandidates, borrowAfterPreemptions
borrow, _ := classical.FindHeightOfLowestSubtreeThatFits(cq, fr, quantity)
return preemptioncommon.NoCandidates, borrow
Copy link
Copy Markdown
Contributor Author

@pajakd pajakd Jul 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It actually matters what value of borrow we return with NoCandidates because it is later used to order entries in scheduling.

There are tests that were failing whenever I was trying to return some fixed value to this actually has to be correctly calculated. For that reason I can't think of any new testcase needed for this.

For example the new unit tests for capacity blocking (capacity blocked when lending clusterqueue not guaranteed to reclaim (ReclaimWithinCohort=Never)).

Copy link
Copy Markdown
Contributor

@mimowo mimowo Jul 25, 2025

Choose a reason for hiding this comment

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

Awesome, thank you for checking that we had unit tests already.

I like unit tests to be our first line of defense whenever possible (especially for scheduling), cause they are much easier to debug than integration or e2e.

gabesaba
gabesaba reviewed Jul 25, 2025
View reviewed changes
Comment thread pkg/scheduler/preemption/preemption_oracle.go Outdated
@pajakd @gabesaba
Update pkg/scheduler/preemption/preemption_oracle.go
c449ab5 
Co-authored-by: gabesaba <15304068+gabesaba@users.noreply.github.com>
@netlify
Copy link
Copy Markdown

netlify Bot commented Jul 25, 2025 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 1be6c96
🔍 Latest deploy log https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/68834898b1446d0008c84ed8

@gabesaba
Copy link
Copy Markdown
Contributor

gabesaba commented Jul 25, 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 Jul 25, 2025
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 25, 2025

LGTM label has been added.

DetailsGit tree hash: 5450ae3e52986518fd3688dd1bdaf05a58cedf35

@mimowo
Copy link
Copy Markdown
Contributor

mimowo commented Jul 25, 2025

/lgtm
/approve
Thank you!

@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 25, 2025
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jul 25, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabesaba, mimowo, pajakd

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

@mimowo
Copy link
Copy Markdown
Contributor

mimowo commented Jul 25, 2025

race condition :)

@k8s-ci-robot k8s-ci-robot merged commit a6e826d into kubernetes-sigs:main Jul 25, 2025
22 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.13 milestone Jul 25, 2025
@pajakd pajakd deleted the fungiblity_followup branch July 25, 2025 09:46
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Aug 11, 2025
@pajakd @gabesaba @kannon92
Follow-up to Flavor Fungibility Prefers Fit over NoBorrow (kubernetes…
0ae4185 
…-sigs#6181)

* Followup

* Update pkg/scheduler/preemption/preemption_oracle.go

Co-authored-by: gabesaba <15304068+gabesaba@users.noreply.github.com>

* Fix whitespaces

---------

Co-authored-by: gabesaba <15304068+gabesaba@users.noreply.github.com>
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

@PBundyra PBundyra Awaiting requested review from PBundyra

Assignees

@mimowo mimowo

@gabesaba gabesaba

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

v0.13

Development

Successfully merging this pull request may close these issues.

4 participants

@pajakd @gabesaba @k8s-ci-robot @mimowo