Fix pending workload from StrictFIFO CQ doesn't block borrowing from other CQ #1399

yaroslava-serdiuk · 2023-12-04T13:53:10Z

Which issue(s) this PR fixes:

related to #1283

Does this PR introduce a user-facing change?

Pending workload from StrictFIFO ClusterQueue doesn't block borrowing from other ClusterQueues

netlify · 2023-12-04T13:53:18Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`380ad7d`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/659ef7abd75eff00097db422

alculquicondor · 2023-12-05T13:30:20Z

/cc

test/integration/scheduler/scheduler_test.go

yaroslava-serdiuk · 2024-01-03T21:53:09Z

/assign @alculquicondor

alculquicondor · 2024-01-04T18:54:07Z

This definitely requires a release note.

alculquicondor

We should also have unit test coverage for the new logic

pkg/scheduler/flavorassigner/flavorassigner.go

pkg/scheduler/scheduler.go

test/integration/scheduler/scheduler_test.go

pkg/scheduler/scheduler_test.go

test/integration/scheduler/scheduler_test.go

pkg/scheduler/scheduler.go

test/integration/scheduler/scheduler_test.go

tenzen-y · 2024-01-09T20:26:45Z

pkg/scheduler/scheduler.go

+				if cqFlavor.Name == flavor {
+					cqQuota = cqFlavor.Resources
+				}


@alculquicondor In this implementation, the cqQuota will have only the last flavor's resources, right?
So, should we add all resources to the cqQuota?

if cqFlavor.Name == flavor { for name, quota := range cqFlavor.Resources { cqQuota[name] = quota } }

(This is a pseudo code.)

My bad, you're right. Should be fixed now.

yaroslava-serdiuk · 2024-01-10T10:03:19Z

/remove-kind feature
/kind bug

pkg/scheduler/scheduler.go

test/integration/scheduler/scheduler_test.go

yaroslava-serdiuk

Updated reservedResources if CQ is borrowing

pkg/scheduler/scheduler.go

yaroslava-serdiuk · 2024-01-10T16:30:25Z

pkg/scheduler/scheduler_test.go

@@ -1894,3 +1904,81 @@ func TestRequeueAndUpdate(t *testing.T) {
 		})
 	}
 }
+
+func TestResourcesToReserve(t *testing.T) {


TestSchedule test quite complex logic. I've added integration tests to test the full functionality

test/integration/scheduler/scheduler_test.go

alculquicondor · 2024-01-10T16:54:12Z

pkg/scheduler/scheduler.go

+				reservedUsage[flavor][resource] = min(usage, cqQuota.Nominal-cq.Usage[flavor][resource])
+			} else if cqQuota.BorrowingLimit != nil {
+				// if cq is borrowing, cq usage can't exceed nominal + borrowing limit
+				reservedUsage[flavor][resource] = min(usage, cqQuota.Nominal+*cqQuota.BorrowingLimit-cq.Usage[flavor][resource])


We don't want to block up to the borrowing limit. We only want to block up to nominal.

@mimowo what do you think should we do if borrowing with preemption? I guess we should block all of the usage in that case?

Updated, cq usage shouldn't go over cqQuota.Nominal+*cqQuota.BorrowingLimit if borrowing limit is set

@mimowo what do you think should we do if borrowing with preemption? I guess we should block all of the usage in that case?

TBH, I'm not sure, I'm wondering if we can simplify

if cqQuota.BorrowingLimit == nil { reservedUsage[flavor][resource] = usage } else { reservedUsage[flavor][resource] = min(usage, cqQuota.Nominal+*cqQuota.BorrowingLimit-cq.Usage[flavor][resource]) }

to just

reservedUsage[flavor][resource] = usage

@yaroslava-serdiuk do you know a scenario when this is needed?

This looks odd to me at the moment, because in the scenario of single shared CQ as here when cqQuota.Nominal=0, then if borrowingLimit=null we reserve usage, but if borrowingLimit=nominal for the shared (essentially the same a null), and cq.Usage[flavor][resource]=borrowigLimit, then we reserve 0.

we shouldn't go over cqQuota.Nominal+*cqQuota.BorrowingLimit if borrowing limit is set

Correct, but this is verified already in

kueue/pkg/scheduler/flavorassigner/flavorassigner.go

Lines 563 to 566 in 4bc332a

if (rQuota.BorrowingLimit == nil || val <= rQuota.Nominal+*rQuota.BorrowingLimit) && val <= cohortAvailable {

mode = Preempt

borrow = val > rQuota.Nominal

}

. I think it is ok to go beyond cqQuota.Nominal+*cqQuota.BorrowingLimit-cq.Usage[flavor][resource]

Had discussion with @mimowo offline. I explained the condition:

reservedUsage[flavor][resource] = usage } else { reservedUsage[flavor][resource] = min(usage, cqQuota.Nominal+*cqQuota.BorrowingLimit-cq.Usage[flavor][resource]) }

However this is unlikely will be used, since the borrowing limit is often nil.
I also constructed example when the condition unblock another workload from borrowing:
https://github.com/kubernetes-sigs/kueue/assets/43413443/3ee2a4f6-d8ee-4bb4-96d8-2e9662e8869c

Thank you for the sync and explanations indeed.

One thing that it let me understand is that the StrictFIFO isn't really the main point here (as the title suggests). If the queue has only one workload it does not matter what is its type. Maybe we could say in the release notes:
"Fix a bug that a preempting workload in one CQ could block another pending workload in another CQ even if the other workload could be scheduled by borrowing".

Regarding the extra condition I think it adds complexity (and the code is already complex). The scenario isn't tested currently by integration tests. Also, the importance of the scenario from the user perspective seems questionable, because it is only relevant if borrowingLimit<nominalQuota of shared CQ. I could open a PR to simplify. WDYT @alculquicondor ?

Also, the importance of the scenario from the user perspective seems questionable, because it is only relevant if borrowingLimit<nominalQuota of shared CQ.

@mimowo, Could you clarify this situation? Which does this mean, a: borrowingLimit<(nominalQuota of shared CQ) or b: (borrowingLimit<nominalQuota) of shared CQ?

If you mean a, I think a will often happen. I guess admins want to set borrowingLimit<(nominalQuota of shared CQ) in each ClusterQueue since admins want to prevent situations where one of the CQ monopolizes all shared capacity.

If my understanding is incorrect , please let me know.

@mimowo, Could you clarify this situation? Which does this mean, a: borrowingLimit<(nominalQuota of shared CQ) or b: (borrowingLimit<nominalQuota) of shared CQ?

I meant (a) borrowingLimit<(nominalQuota of shared CQ).

If you mean a, I think a will often happen. I guess admins want to set borrowingLimit<(nominalQuota of shared CQ) in each ClusterQueue since admins want to prevent situations where one of the CQ monopolizes all shared capacity.

Makes sense, I was thinking about the scenario as in here, but both are relevant.

So the remaining question: do we care about the scenario, that a workload that is borrowing (B) could get blocked by another workload (A) that is preempting and borrowing in the cohort. The trade-off does not seem obviousm because letting the other workload run may cause the waiting workload wait longer.

Feel free to open a PR

pkg/scheduler/scheduler_test.go

alculquicondor · 2024-01-10T17:06:40Z

pkg/scheduler/scheduler.go

+				reservedUsage[flavor][resource] = min(usage, cqQuota.Nominal-cq.Usage[flavor][resource])
+			} else if cqQuota.BorrowingLimit != nil {
+				// if cq is borrowing, cq usage can't exceed nominal + borrowing limit
+				reservedUsage[flavor][resource] = min(usage, cqQuota.Nominal+*cqQuota.BorrowingLimit-cq.Usage[flavor][resource])


@mimowo what do you think should we do if borrowing with preemption? I guess we should block all of the usage in that case?

alculquicondor · 2024-01-10T19:13:32Z

pkg/scheduler/scheduler.go

+		for resource, usage := range resourceUsage {
+			reservedUsage[flavor] = make(map[corev1.ResourceName]int64)


Suggested change

for resource, usage := range resourceUsage {

reservedUsage[flavor] = make(map[corev1.ResourceName]int64)

reservedUsage[flavor] = make(map[corev1.ResourceName]int64)

for resource, usage := range resourceUsage {

test/integration/scheduler/scheduler_test.go

…her CQ Apply suggestions from code review Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com>

alculquicondor · 2024-01-10T20:03:05Z

/lgtm
/approve

k8s-ci-robot · 2024-01-10T20:03:11Z

LGTM label has been added.

Git tree hash: 8f26d378f3d6b62823e674f008cb2380ff3706b4

k8s-ci-robot · 2024-01-10T20:03:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, yaroslava-serdiuk

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:

~~OWNERS~~ [alculquicondor]

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 added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Dec 4, 2023

k8s-ci-robot requested review from denkensk and trasc December 4, 2023 13:53

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 4, 2023

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 4, 2023

yaroslava-serdiuk changed the title ~~WIP: Add feature gate for priorities sorting in a cohort~~ WIP: Scheduler test Dec 5, 2023

yaroslava-serdiuk mentioned this pull request Dec 5, 2023

Do not consider priorities when sorting workloads from different ClusterQueues #1283

Closed

3 tasks

k8s-ci-robot requested a review from alculquicondor December 5, 2023 13:30

alculquicondor reviewed Dec 5, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2023

yaroslava-serdiuk force-pushed the scheduler-test branch from a903b03 to 04438b0 Compare January 3, 2024 21:40

yaroslava-serdiuk changed the title ~~WIP: Scheduler test~~ Make sure pending workload strictFIFO doesn't block borrowing from other CQ Jan 3, 2024

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 Jan 3, 2024

yaroslava-serdiuk force-pushed the scheduler-test branch from 04438b0 to 762046c Compare January 3, 2024 21:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 3, 2024

yaroslava-serdiuk force-pushed the scheduler-test branch 2 times, most recently from 4912c7e to 0658449 Compare January 3, 2024 21:51

yaroslava-serdiuk changed the title ~~Make sure pending workload strictFIFO doesn't block borrowing from other CQ~~ Make sure pending workload from StrictFIFO CQ doesn't block borrowing from other CQ Jan 3, 2024

k8s-ci-robot assigned alculquicondor Jan 3, 2024

yaroslava-serdiuk force-pushed the scheduler-test branch from 0658449 to a694fa8 Compare January 4, 2024 09:57

alculquicondor reviewed Jan 4, 2024

View reviewed changes

yaroslava-serdiuk force-pushed the scheduler-test branch from a694fa8 to 33cb8d2 Compare January 5, 2024 16:04

alculquicondor reviewed Jan 9, 2024

View reviewed changes

tenzen-y reviewed Jan 9, 2024

View reviewed changes

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Jan 10, 2024

yaroslava-serdiuk changed the title ~~Make sure pending workload from StrictFIFO CQ doesn't block borrowing from other CQ~~ Fix pending workload from StrictFIFO CQ doesn't block borrowing from other CQ Jan 10, 2024

yaroslava-serdiuk force-pushed the scheduler-test branch from 6ab1694 to 3e80a4f Compare January 10, 2024 11:57

alculquicondor reviewed Jan 10, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2024

yaroslava-serdiuk force-pushed the scheduler-test branch from ce056de to 1e546c7 Compare January 10, 2024 16:29

yaroslava-serdiuk commented Jan 10, 2024

View reviewed changes

yaroslava-serdiuk force-pushed the scheduler-test branch from 1e546c7 to 0c9b559 Compare January 10, 2024 16:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2024

alculquicondor reviewed Jan 10, 2024

View reviewed changes

yaroslava-serdiuk force-pushed the scheduler-test branch from eb46bd1 to c8bfa46 Compare January 10, 2024 19:36

Make sure pending workload strictfifo doesn't block borrowing from ot…

380ad7d

…her CQ Apply suggestions from code review Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com>

yaroslava-serdiuk force-pushed the scheduler-test branch from c8bfa46 to 380ad7d Compare January 10, 2024 20:01

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2024

k8s-ci-robot merged commit eb853f1 into kubernetes-sigs:main Jan 10, 2024
14 checks passed

k8s-ci-robot added this to the v0.6 milestone Jan 10, 2024

This was referenced Jan 11, 2024

Simplify quota reservation when preemption while borrowing #1571

Closed

Fix the flake preemption while borrowing unit test #1581

Merged

yaroslava-serdiuk mentioned this pull request Feb 27, 2024

Confusing QueueingStrategy API description #1379

Closed

alculquicondor mentioned this pull request Mar 18, 2024

Rework implementation for multiple admissions per cycle #1867

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pending workload from StrictFIFO CQ doesn't block borrowing from other CQ #1399

Fix pending workload from StrictFIFO CQ doesn't block borrowing from other CQ #1399

yaroslava-serdiuk commented Dec 4, 2023 •

edited

Loading

netlify bot commented Dec 4, 2023 •

edited

Loading

alculquicondor commented Dec 5, 2023

yaroslava-serdiuk commented Jan 3, 2024

alculquicondor commented Jan 4, 2024

alculquicondor left a comment

tenzen-y Jan 9, 2024

yaroslava-serdiuk Jan 10, 2024

yaroslava-serdiuk commented Jan 10, 2024

yaroslava-serdiuk left a comment

yaroslava-serdiuk Jan 10, 2024

alculquicondor Jan 10, 2024

alculquicondor Jan 10, 2024

yaroslava-serdiuk Jan 10, 2024 •

edited

Loading

mimowo Jan 11, 2024 •

edited

Loading

mimowo Jan 11, 2024

yaroslava-serdiuk Jan 11, 2024 •

edited

Loading

mimowo Jan 11, 2024

tenzen-y Jan 11, 2024

mimowo Jan 11, 2024

alculquicondor Jan 11, 2024

alculquicondor Jan 10, 2024

alculquicondor Jan 10, 2024

alculquicondor commented Jan 10, 2024

k8s-ci-robot commented Jan 10, 2024

k8s-ci-robot commented Jan 10, 2024

	if (rQuota.BorrowingLimit == nil \|\| val <= rQuota.Nominal+*rQuota.BorrowingLimit) && val <= cohortAvailable {
	mode = Preempt
	borrow = val > rQuota.Nominal
	}

		for resource, usage := range resourceUsage {
		reservedUsage[flavor] = make(map[corev1.ResourceName]int64)

Fix pending workload from StrictFIFO CQ doesn't block borrowing from other CQ #1399

Fix pending workload from StrictFIFO CQ doesn't block borrowing from other CQ #1399

Conversation

yaroslava-serdiuk commented Dec 4, 2023 • edited Loading

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

netlify bot commented Dec 4, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

alculquicondor commented Dec 5, 2023

yaroslava-serdiuk commented Jan 3, 2024

alculquicondor commented Jan 4, 2024

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslava-serdiuk commented Jan 10, 2024

yaroslava-serdiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslava-serdiuk Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

mimowo Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslava-serdiuk Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Jan 10, 2024

k8s-ci-robot commented Jan 10, 2024

k8s-ci-robot commented Jan 10, 2024

yaroslava-serdiuk commented Dec 4, 2023 •

edited

Loading

netlify bot commented Dec 4, 2023 •

edited

Loading

yaroslava-serdiuk Jan 10, 2024 •

edited

Loading

mimowo Jan 11, 2024 •

edited

Loading

yaroslava-serdiuk Jan 11, 2024 •

edited

Loading