Refactor Preemption to interact with Quota/Usage through ClusterQueueSnapshot interface #2595

gabesaba · 2024-07-12T15:36:27Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Follow up to #2592. In preparation for #79.

Copied from #2592: we will change the Scheduler, FlavorAssigner, and Preemption logic to only interact with the ClusterQueueSnapshot's capacity through high level queries. E.g. "am I borrowing" or "how much capacity do I have left".

Additionally, we fix the test preempting locally and borrowing other resources in cohort, without cohort candidates. Since we were only looping over CQ resource groups, we didn't
find out that we had no capacity in CQ for this flavor code. We fix this by adding the alpha flavor to that CQ, and adding a CQ which lends resources to those FlavorResources

Does this PR introduce a user-facing change?

NONE

netlify · 2024-07-12T15:36:44Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`3541f14`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6697b4f27d88d500084c6f6f

gabesaba · 2024-07-12T16:28:08Z

pkg/scheduler/preemption/preemption.go

-			}
-			cqResUsage := cq.Usage[fName]
-			for rName := range flvReq {
-				if cqResUsage[rName] >= cq.QuotaFor(resources.FlavorResource{Flavor: fName, Resource: rName}).Nominal {


note to reviewers: this >= turns into a >. I think this is correct (as we want to make sure we're within nominal quota), but please scrutinize it @alculquicondor

Actually, a more accurate check would be whether the usage plus the resources for the incoming workload would be borrowing.

If it's not borrowing, then this means that this CQ is preempting to reclaim quota, then it is allowed to preempt other workloads in the cohort. Otherwise, it should only be allowed to preempt workloads within its CQ or those below the threshold.

@mimowo wdyt? This was added in #1979 and reused in #2110

If there's a bug in the accounting, let's fix it in a follow-up PR, as this PR is intended to cleanup existing code without any change in behavior

note to reviewers: this >= turns into a >. I think this is correct (as we want to make sure we're within nominal quota), but please scrutinize it @alculquicondor

I think !queueUnderNominalInResourcesNeedingPreemption <> cqIsBorrowing. For example if you have one resource, and cqResUsage[rName] == Nominal then the answer is "false", so !queueUnderNominalInResourcesNeedingPreemption is true, while cqIsBorrowing is false.

@mimowo wdyt? This was added in #1979 and reused in #2110

ack, I remember this was quite complex, so I would prefer to keep the logic in this PR, and do a dedicated one for fix if needed. I will yet look a bit into this.

Folded the two functions into one #2595 (comment)

I've met with @gabesaba and agreed that he will try to adjust this refactoring PR not to change any logic.We will have a dedicated follow up to adjust the logic if needed. We will then consider changing <, with <=, or the idea from #2595 (comment).

Done. Unfolded the functions, and added back in the cohort == nil check.

gabesaba · 2024-07-12T16:34:26Z

pkg/scheduler/preemption/preemption.go

 	if cq.Cohort == nil {
 		return false
 	}


can we get rid of this check? if so, can fold queueUnderNominalInResourcesNeedingPreemption into this function. unit tests still pass after its removal

Yeah, I think the check can be removed, because this logic is only called for CQs that belong to the same cohort as the preempting one.

Replaced usages of queueUnderNominalInResourcesNeedingPreemption with !cqIsBorrowing

alculquicondor · 2024-07-12T19:02:34Z

pkg/cache/clusterqueue_snapshot.go

+	return 0
+}
+
+func (c *ClusterQueueSnapshot) borrowing(fr resources.FlavorResource) *int64 {


Suggested change

func (c *ClusterQueueSnapshot) borrowing(fr resources.FlavorResource) *int64 {

func (c *ClusterQueueSnapshot) borrowingLimit(fr resources.FlavorResource) *int64 {

alculquicondor · 2024-07-12T19:03:26Z

pkg/cache/clusterqueue_snapshot.go

+
+	// if the borrowing limit exists, we cap our available capacity by the borrowing limit.
+	if borrowingLimit := c.borrowing(fr); borrowingLimit != nil {
+		borrowingRemaining := c.nominal(fr) + *borrowingLimit - c.usageFor(fr)


Suggested change

borrowingRemaining := c.nominal(fr) + *borrowingLimit - c.usageFor(fr)

withBorrowingRemaining := c.nominal(fr) + *borrowingLimit - c.usageFor(fr)

alculquicondor · 2024-07-12T19:11:31Z

pkg/scheduler/preemption/preemption.go

 	if cq.Cohort == nil {
 		return false
 	}


Yeah, I think the check can be removed, because this logic is only called for CQs that belong to the same cohort as the preempting one.

alculquicondor · 2024-07-12T19:43:33Z

pkg/scheduler/preemption/preemption.go

-			}
-			cqResUsage := cq.Usage[fName]
-			for rName := range flvReq {
-				if cqResUsage[rName] >= cq.QuotaFor(resources.FlavorResource{Flavor: fName, Resource: rName}).Nominal {


Actually, a more accurate check would be whether the usage plus the resources for the incoming workload would be borrowing.

If it's not borrowing, then this means that this CQ is preempting to reclaim quota, then it is allowed to preempt other workloads in the cohort. Otherwise, it should only be allowed to preempt workloads within its CQ or those below the threshold.

@mimowo wdyt? This was added in #1979 and reused in #2110

alculquicondor · 2024-07-12T19:51:22Z

pkg/scheduler/preemption/preemption_test.go

@@ -91,6 +91,23 @@ func TestPreemption(t *testing.T) {
 				Resource(corev1.ResourceCPU, "6", "6").
 				Resource(corev1.ResourceMemory, "3Gi", "3Gi").
 				Obj(),
+				*utiltesting.MakeFlavorQuotas("alpha").


Instead, change the test to use default for both resources. They are part of the same resource group, so they shouldn't get different flavors.

mimowo · 2024-07-17T12:36:25Z

/lgtm
/approve
I believe all comments are addressed.

k8s-ci-robot · 2024-07-17T12:36:31Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [mimowo]

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 · 2024-07-17T12:36:31Z

LGTM label has been added.

Git tree hash: 5d12594ef18c940ed7cbc15cda6969d616343427

…Snapshot interface (kubernetes-sigs#2595) * Fix preemption test * Refactor preemption.go

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 12, 2024

k8s-ci-robot requested review from alculquicondor and trasc July 12, 2024 15:36

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 12, 2024

gabesaba commented Jul 12, 2024

View reviewed changes

alculquicondor reviewed Jul 12, 2024

View reviewed changes

Fix preemption test

58990de

gabesaba force-pushed the refactor_preemption branch from d4ed6f5 to a89149b Compare July 15, 2024 08:06

Refactor preemption.go

3541f14

gabesaba force-pushed the refactor_preemption branch from a89149b to 3541f14 Compare July 17, 2024 12:11

k8s-ci-robot assigned mimowo Jul 17, 2024

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

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

k8s-ci-robot merged commit 1c559b6 into kubernetes-sigs:main Jul 17, 2024
16 checks passed

k8s-ci-robot added this to the v0.8 milestone Jul 17, 2024

gabesaba deleted the refactor_preemption branch August 9, 2024 13:04

kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024

Refactor Preemption to interact with Quota/Usage through ClusterQueue…

5558904

…Snapshot interface (kubernetes-sigs#2595) * Fix preemption test * Refactor preemption.go

gabesaba mentioned this pull request Jan 30, 2025

Account for new workload usage in queueUnderNominalInResourcesNeedingPreemption #4104

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Preemption to interact with Quota/Usage through ClusterQueueSnapshot interface #2595

Refactor Preemption to interact with Quota/Usage through ClusterQueueSnapshot interface #2595

gabesaba commented Jul 12, 2024 •

edited

Loading

netlify bot commented Jul 12, 2024 •

edited

Loading

gabesaba Jul 12, 2024

alculquicondor Jul 12, 2024

gabesaba Jul 15, 2024

mimowo Jul 15, 2024

gabesaba Jul 17, 2024

mimowo Jul 17, 2024

gabesaba Jul 17, 2024

gabesaba Jul 12, 2024

alculquicondor Jul 12, 2024

gabesaba Jul 17, 2024

alculquicondor Jul 12, 2024

alculquicondor Jul 12, 2024

alculquicondor Jul 12, 2024

alculquicondor Jul 12, 2024

alculquicondor Jul 12, 2024

mimowo commented Jul 17, 2024

k8s-ci-robot commented Jul 17, 2024

k8s-ci-robot commented Jul 17, 2024

	func (c ClusterQueueSnapshot) borrowing(fr resources.FlavorResource) int64 {
	func (c ClusterQueueSnapshot) borrowingLimit(fr resources.FlavorResource) int64 {

	borrowingRemaining := c.nominal(fr) + *borrowingLimit - c.usageFor(fr)
	withBorrowingRemaining := c.nominal(fr) + *borrowingLimit - c.usageFor(fr)

Refactor Preemption to interact with Quota/Usage through ClusterQueueSnapshot interface #2595

Refactor Preemption to interact with Quota/Usage through ClusterQueueSnapshot interface #2595

Conversation

gabesaba commented Jul 12, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

netlify bot commented Jul 12, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

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

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

Choose a reason for hiding this comment

mimowo commented Jul 17, 2024

k8s-ci-robot commented Jul 17, 2024

k8s-ci-robot commented Jul 17, 2024

gabesaba commented Jul 12, 2024 •

edited

Loading

netlify bot commented Jul 12, 2024 •

edited

Loading