Unblock scheduler after failed preemption(s) #7665

olekzabl · 2025-11-14T13:07:22Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #5590

Special notes for your reviewer:

This PR is currently split into 2 commits:

The first commit is small, and fixes the issue.
(In short, we're roughly aligned with @mimowo that it does.
For details, see the discussion in Kueue Scheduler getting stuck if preemption request fails #5590, starting from this comment)
The second commit is my attempt to give this fix any reasonable test coverage.
I chose integration tests (to verify the whole path of "preemption error -> retry -> desired outcome").
However, for that, I needed selectively injecting fake errors into K8s client, in integration tests, which AFAICS has been never done in Kueue.
Hence, I needed some custom tweaking of the test setup to achieve that.
I'm curious about reviewers' opinions whether this is nice enough.

The test coverage may be not yet as full as I'd wish (for example, I haven't cared for testing the "preemption failed" vs. "sticky workloads" interaction); yet, as I'm hearing this issue gained some urgency, I'm un-drafting this PR now, hoping that it may be good to go as it is. Then, further tests may come in a follow-up PR. (Tracked in issue #7806).

Does this PR introduce a user-facing change?

Fix a bug that an error during workload preemption could leave the scheduler stuck without retrying.

netlify · 2025-11-14T13:07:30Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`caf3b8b`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/69204ad5c33d9d0008be211f

k8s-ci-robot · 2025-11-14T13:07:32Z

Hi @olekzabl. 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.

Details

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.

mimowo · 2025-11-14T15:59:45Z

/ok-to-test

olekzabl · 2025-11-18T23:21:46Z

@gabesaba @mbobrovskyi May I ask for a review at this point?

While this is still technically a draft (due to non-perfect test coverage), I'd like to verify my testing approach with you before I continue work on that. And the code written so far I've already tried to "polish" reasonably.

Please see the PR description for more context.

olekzabl · 2025-11-19T07:08:29Z

/test

k8s-ci-robot · 2025-11-19T07:08:32Z

@olekzabl: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-kueue-build-image-main

/test pull-kueue-test-e2e-certmanager-main

/test pull-kueue-test-e2e-customconfigs-main

/test pull-kueue-test-e2e-kueueviz-main

/test pull-kueue-test-e2e-main-1-32

/test pull-kueue-test-e2e-main-1-33

/test pull-kueue-test-e2e-main-1-34

/test pull-kueue-test-e2e-multikueue-main

/test pull-kueue-test-e2e-tas-main

/test pull-kueue-test-integration-baseline-main

/test pull-kueue-test-integration-extended-main

/test pull-kueue-test-integration-multikueue-main

/test pull-kueue-test-scheduling-perf-main

/test pull-kueue-test-unit-main

/test pull-kueue-verify-main

Use /test all to run all jobs.

Details

In response to this:

/test

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.

olekzabl · 2025-11-19T07:08:43Z

/test all

olekzabl · 2025-11-19T07:51:46Z

/retest

olekzabl · 2025-11-19T08:02:49Z

/retest

pajakd · 2025-11-21T09:17:43Z

pkg/scheduler/preemption/preemption_test.go

+			if failed != 0 {
+				t.Errorf("Reported %d failed preemptions, want 0", failed)
+			}


Have you considered adding unit test(s) for this change (here or in pkg/scheduler/scheduler_test.go)?

I considered - but (as mentioned in description of this PR) I started with smaller coverage due to reported urgency of this PR.

Since you ask about it, I opened a follow-up issue: #7806

pajakd · 2025-11-21T09:23:57Z

test/integration/singlecluster/scheduler/preemption_test.go

+					return fallThrough, nil
+				}
+				// Ignore patches triggered by util.FinishEvictionForWorkloads()
+				if cond := apimeta.FindStatusCondition(wl.Status.Conditions, kueue.WorkloadQuotaReserved); cond != nil && cond.Status == metav1.ConditionFalse && cond.Message == "By test" {


nit: if someone changes that "By test" message, we might start getting failures here? Perhaps add a constant to test/util/constants.go with this message.

I considered it - but then I noticed lots of "by test" literals spread across our tests.
So I concluded adding such constant would go a bit against our current conventions.

pajakd · 2025-11-21T09:26:36Z

Left a few non-blocking comments but overall lgtm

Singularity23x0 · 2025-11-21T10:52:03Z

test/integration/framework/framework.go

 }

-func (f *Framework) StartManager(ctx context.Context, cfg *rest.Config, managerSetup ManagerSetup) {
+func (f *Framework) StartManager(ctx context.Context, cfg *rest.Config, managerSetup ManagerSetup, opts ...ManagerSetupOption) {


Ok, i see the issue.

Well then as I stated - I think updating mgrOptions directly instead of using managerSetupOptions removes unnecessary busywork and the cost of direct access to options seems negligible given we are talking about tests.

But leaving as is is also okay - leaving it up to you to decide.

PBundyra · 2025-11-21T11:57:08Z

Nice, thanks!
/lgtm

k8s-ci-robot · 2025-11-21T11:57:15Z

LGTM label has been added.

Details

Git tree hash: d3284f65ff732b0c8d8308bb31a8f4bf98bf36ad

mimowo

Awesome, looks quite good 👍
/approve
/cherrypick release-0.14
/cherrypick release-0.13

k8s-ci-robot · 2025-11-21T15:44:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, olekzabl, Singularity23x0

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:

~~OWNERS~~ [mimowo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mimowo · 2025-11-21T15:44:43Z

@olekzabl please prepare CPs manually in case the automated cherrypick fails

olekzabl · 2025-11-21T20:38:40Z

@olekzabl please prepare CPs manually in case the automated cherrypick fails

Hmm, I don't see any activity from k8s-infra-cherrypick-robot following your earlier cherrypick slash-commands. No idea why.

I'll retry these commands below, though not sure if I have permissions for this.

olekzabl · 2025-11-21T20:38:57Z

/cherrypick release-0.14

k8s-infra-cherrypick-robot · 2025-11-21T20:39:34Z

@olekzabl: #7665 failed to apply on top of branch "release-0.14":

Applying: Add `RequeueReasonPreemptionFailed`
Using index info to reconstruct a base tree...
M	pkg/cache/queue/cluster_queue.go
M	pkg/scheduler/preemption/preemption.go
M	pkg/scheduler/preemption/preemption_hierarchical_test.go
M	pkg/scheduler/preemption/preemption_test.go
M	pkg/scheduler/scheduler.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/scheduler/scheduler.go
CONFLICT (content): Merge conflict in pkg/scheduler/scheduler.go
Auto-merging pkg/scheduler/preemption/preemption_test.go
CONFLICT (content): Merge conflict in pkg/scheduler/preemption/preemption_test.go
Auto-merging pkg/scheduler/preemption/preemption_hierarchical_test.go
CONFLICT (content): Merge conflict in pkg/scheduler/preemption/preemption_hierarchical_test.go
Auto-merging pkg/scheduler/preemption/preemption.go
CONFLICT (content): Merge conflict in pkg/scheduler/preemption/preemption.go
Auto-merging pkg/cache/queue/cluster_queue.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Add `RequeueReasonPreemptionFailed`

Details

In response to this:

/cherrypick release-0.14

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.

olekzabl · 2025-11-21T20:39:59Z

/cherrypick release-0.13

k8s-infra-cherrypick-robot · 2025-11-21T20:40:35Z

@olekzabl: #7665 failed to apply on top of branch "release-0.13":

Applying: Add `RequeueReasonPreemptionFailed`
Using index info to reconstruct a base tree...
A	pkg/cache/queue/cluster_queue.go
M	pkg/scheduler/preemption/preemption.go
M	pkg/scheduler/preemption/preemption_hierarchical_test.go
M	pkg/scheduler/preemption/preemption_test.go
M	pkg/scheduler/scheduler.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/scheduler/scheduler.go
CONFLICT (content): Merge conflict in pkg/scheduler/scheduler.go
Auto-merging pkg/scheduler/preemption/preemption_test.go
CONFLICT (content): Merge conflict in pkg/scheduler/preemption/preemption_test.go
Auto-merging pkg/scheduler/preemption/preemption_hierarchical_test.go
CONFLICT (content): Merge conflict in pkg/scheduler/preemption/preemption_hierarchical_test.go
Auto-merging pkg/scheduler/preemption/preemption.go
CONFLICT (content): Merge conflict in pkg/scheduler/preemption/preemption.go
Auto-merging pkg/queue/cluster_queue.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Add `RequeueReasonPreemptionFailed`

Details

In response to this:

/cherrypick release-0.13

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.

…tion(s) (#7817) * Add `RequeueReasonPreemptionFailed` * Add integration test + a way to inject failures * Address a comment * Adjust function signature * Only set "failed" reason when sth certainly failed * Remove one layer of options * Fix import mismatch

…tion(s) (#7818) * Add `RequeueReasonPreemptionFailed` * Add integration test + a way to inject failures * Address a comment * Adjust function signature * Only set "failed" reason when sth certainly failed * Remove one layer of options * Fix import mismatch

mimowo · 2025-11-24T10:50:48Z

test/integration/singlecluster/scheduler/suite_test.go

 	gomega.Expect(err).NotTo(gomega.HaveOccurred())
 }
+
+func setupInterceptedClient() (context.Context, client.Client) {


btw, I think it is nice to see this PR paves the way for simulating API server errors. We have some older code which is not tested wrt error handling because we never added the mechanism. For example #7364, but many more in the past.

Now I can refer to this PR as an example for covering error cases in integration tests. Thanks !

cc @PBundyra @pajakd .

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 14, 2025

k8s-ci-robot requested review from gabesaba and mbobrovskyi November 14, 2025 13:07

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 14, 2025

olekzabl mentioned this pull request Nov 14, 2025

Kueue Scheduler getting stuck if preemption request fails #5590

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 14, 2025

Add RequeueReasonPreemptionFailed

c357064

olekzabl force-pushed the br20 branch from c93d99a to ed9e2c3 Compare November 18, 2025 23:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 18, 2025

olekzabl force-pushed the br20 branch from ed9e2c3 to 8f8ca1b Compare November 19, 2025 07:51

Add integration test + a way to inject failures

58b809e

olekzabl force-pushed the br20 branch from 8f8ca1b to 58b809e Compare November 19, 2025 08:02

olekzabl marked this pull request as ready for review November 20, 2025 09:10

k8s-ci-robot requested review from kannon92 and mimowo November 20, 2025 09:10

Only set "failed" reason when sth certainly failed

1813b8f

olekzabl requested a review from pajakd November 21, 2025 09:11

pajakd reviewed Nov 21, 2025

View reviewed changes

olekzabl mentioned this pull request Nov 21, 2025

Better test coverage for scheduler handling of failed preemptions #7806

Open

olekzabl requested a review from pajakd November 21, 2025 10:34

Singularity23x0 approved these changes Nov 21, 2025

View reviewed changes

Remove one layer of options

caf3b8b

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2025

mimowo reviewed Nov 21, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2025

k8s-ci-robot merged commit ea52fbe into kubernetes-sigs:main Nov 21, 2025
22 checks passed

k8s-ci-robot added this to the v0.15 milestone Nov 21, 2025

This was referenced Nov 21, 2025

Automated cherry pick of #7665: Unblock scheduler after failed preemption(s) #7817

Merged

Automated cherry pick of #7665: Unblock scheduler after failed preemption(s) #7818

Merged

mimowo reviewed Nov 24, 2025

View reviewed changes

mimowo mentioned this pull request Nov 24, 2025

Fix race condition that prevents Workload from reflecting Inadmissible status. #7845

Merged

mimowo mentioned this pull request Dec 17, 2025

Fail to preempt an older low-priority job when borrowing within cohort #6814

Closed

Unblock scheduler after failed preemption(s) #7665

Unblock scheduler after failed preemption(s) #7665

Uh oh!

Conversation

olekzabl commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

netlify bot commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Uh oh!

k8s-ci-robot commented Nov 14, 2025

Uh oh!

mimowo commented Nov 14, 2025

Uh oh!

olekzabl commented Nov 18, 2025

Uh oh!

olekzabl commented Nov 19, 2025

Uh oh!

k8s-ci-robot commented Nov 19, 2025

Uh oh!

olekzabl commented Nov 19, 2025

Uh oh!

olekzabl commented Nov 19, 2025

Uh oh!

olekzabl commented Nov 19, 2025

Uh oh!

pajakd Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

olekzabl Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

pajakd Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

olekzabl Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

pajakd commented Nov 21, 2025

Uh oh!

Singularity23x0 Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

PBundyra commented Nov 21, 2025

Uh oh!

k8s-ci-robot commented Nov 21, 2025

Uh oh!

mimowo left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 21, 2025

Uh oh!

mimowo commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

olekzabl commented Nov 21, 2025

Uh oh!

olekzabl commented Nov 21, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Nov 21, 2025

Uh oh!

olekzabl commented Nov 21, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Nov 21, 2025

Uh oh!

mimowo Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

olekzabl commented Nov 14, 2025 •

edited

Loading

netlify bot commented Nov 14, 2025 •

edited

Loading

mimowo commented Nov 21, 2025 •

edited

Loading