Add comments to explain special cases #1496

kerthcet · 2023-12-19T16:17:21Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

~~Based on my understanding, even borrowlingLimit not nil, if cq.cohort == nil, we should still not consider the borrowingLimit in calculation. cc @alculquicondor is this a real bug or as designed?~~

Just for explaination.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

netlify · 2023-12-19T16:17:29Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`a6fec86`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/658cd6a09c10440008d412e7

alculquicondor · 2023-12-19T16:25:42Z

I agree that if the CQ doesn't belong to a cohort, the borrowingLimit should be considered 0.
Can you add a unit test case in TestPreemption that proves that there was a bug?

kerthcet · 2023-12-20T07:38:45Z

/retest pull-kueue-test-unit-main

k8s-ci-robot · 2023-12-20T07:38:47Z

@kerthcet: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-kueue-build-image-main
/test pull-kueue-test-e2e-main-1-25
/test pull-kueue-test-e2e-main-1-26
/test pull-kueue-test-e2e-main-1-27
/test pull-kueue-test-e2e-main-1-28
/test pull-kueue-test-integration-main
/test pull-kueue-test-unit-main
/test pull-kueue-verify-main

Use /test all to run all jobs.

In response to this:

/retest pull-kueue-test-unit-main

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/test-infra repository.

kerthcet · 2023-12-20T07:39:24Z

/test pull-kueue-test-unit-main

kerthcet · 2023-12-22T09:41:38Z

pkg/scheduler/preemption/preemption.go

@@ -334,12 +336,15 @@ func workloadFits(wlReq cache.FlavorResourceQuantities, cq *cache.ClusterQueue,
 			}
 			for rName, rReq := range flvReq {
 				limit := flvQuotas.Resources[rName].Nominal
-				if flvQuotas.Resources[rName].BorrowingLimit != nil && allowBorrowing {
+
+				if cq.Cohort != nil && allowBorrowing && flvQuotas.Resources[rName].BorrowingLimit != nil {


Based on the comment:

// If null, it means that there is no borrowing limit. // If not null, it must be non-negative. // borrowingLimit must be null if spec.cohort is empty. // +optional BorrowingLimit *resource.Quantity `json:"borrowingLimit,omitempty"`

If borrowingLimit is nil then cohort must be nil, no bug here. However, we didn't have any validation about this, that's say we can still have cq with cohort == nil && borrowingLimit != nil.

My suggestion here: #1385 (comment), tl;dr: can we ignore this rule? cc @alculquicondor

mimowo

It is a pity we don't have validation against cq.Cohort=nil and borrowingLimit!=nil.

Maybe it is still not too late to add, Kueue is still Beta API?

Anyway, if we go with the check for cq.Cohort != nil, should we also add it here?

mimowo · 2023-12-22T10:05:52Z

pkg/scheduler/preemption/preemption.go

@@ -64,7 +64,7 @@ func (p *Preemptor) OverrideApply(f func(context.Context, *kueue.Workload) error
 }

 func candidatesOnlyFromQueue(candidates []*workload.Info, clusterQueue string) []*workload.Info {
-	result := make([]*workload.Info, 0)
+	result := make([]*workload.Info, 0, len(candidates))


nit: I would suggest to revert unrelated changes

also, all the workloads might be from a different CQ.

So it could be better to do var result []*workload.Info

It's clear here that the length wouldn't greater than len(candidates).

pkg/scheduler/preemption/preemption.go

Signed-off-by: kerthcet <[email protected]>

kerthcet · 2023-12-23T05:39:08Z

As you reminded, we're beta now, we can not break the API semantics, so made it a cleanup. And will add the validations about the API. Maybe we can change that at v1beta2 if needed. PTAL @mimowo

kerthcet · 2023-12-23T05:41:36Z

/remove-kind bug
/kind cleanup

Signed-off-by: kerthcet <[email protected]>

kerthcet · 2023-12-28T02:00:28Z

Next, I'll add more validations to avoid this.

B1F030 · 2023-12-28T03:03:18Z

Next, I'll add more validations to avoid this.

I write a part of validation in webhook

alculquicondor

/approve
/lgtm

k8s-ci-robot · 2023-12-28T14:00:03Z

LGTM label has been added.

Git tree hash: 656842d66315b72d236829a12304bfdb4fa45b12

k8s-ci-robot · 2023-12-28T14:00:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, kerthcet

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

* code refactor Signed-off-by: kerthcet <[email protected]> * fix comment Signed-off-by: kerthcet <[email protected]> --------- Signed-off-by: kerthcet <[email protected]>

k8s-ci-robot requested review from denkensk and mimowo December 19, 2023 16:17

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 19, 2023

kerthcet force-pushed the fix/workloadsFits branch 2 times, most recently from aa8cfba to dbaea07 Compare December 22, 2023 09:30

kerthcet commented Dec 22, 2023

View reviewed changes

mimowo reviewed Dec 22, 2023

View reviewed changes

code refactor

ccd30b5

Signed-off-by: kerthcet <[email protected]>

kerthcet force-pushed the fix/workloadsFits branch from dbaea07 to ccd30b5 Compare December 23, 2023 05:36

kerthcet changed the title ~~[WIP] Fix fitting workloads in preemption~~ Add comments to explain special cases Dec 23, 2023

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 Dec 23, 2023

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/bug Categorizes issue or PR as related to a bug. labels Dec 23, 2023

fix comment

a6fec86

Signed-off-by: kerthcet <[email protected]>

B1F030 mentioned this pull request Dec 28, 2023

Add validation and test for borrowingLimit when cohort is nil #1525

Merged

alculquicondor reviewed Dec 28, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Dec 28, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 28, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 28, 2023

k8s-ci-robot merged commit 7aff061 into kubernetes-sigs:main Dec 28, 2023
14 checks passed

k8s-ci-robot added this to the v0.6 milestone Dec 28, 2023

kerthcet deleted the fix/workloadsFits branch December 29, 2023 02:03

tenzen-y mentioned this pull request Jan 10, 2024

Automated cherry pick of #1561: Fix the borrowing while preemption when no borrowingLimit #1564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add comments to explain special cases #1496

Add comments to explain special cases #1496

kerthcet commented Dec 19, 2023 •

edited

Loading

netlify bot commented Dec 19, 2023 •

edited

Loading

alculquicondor commented Dec 19, 2023

kerthcet commented Dec 20, 2023

k8s-ci-robot commented Dec 20, 2023

kerthcet commented Dec 20, 2023

kerthcet Dec 22, 2023

kerthcet Dec 22, 2023

mimowo left a comment

mimowo Dec 22, 2023

alculquicondor Dec 27, 2023

kerthcet Dec 28, 2023

kerthcet commented Dec 23, 2023

kerthcet commented Dec 23, 2023

kerthcet commented Dec 28, 2023

B1F030 commented Dec 28, 2023 •

edited

Loading

alculquicondor left a comment

k8s-ci-robot commented Dec 28, 2023

k8s-ci-robot commented Dec 28, 2023

Add comments to explain special cases #1496

Add comments to explain special cases #1496

Conversation

kerthcet commented Dec 19, 2023 • edited Loading

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?

netlify bot commented Dec 19, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

alculquicondor commented Dec 19, 2023

kerthcet commented Dec 20, 2023

k8s-ci-robot commented Dec 20, 2023

kerthcet commented Dec 20, 2023

kerthcet Dec 22, 2023

Choose a reason for hiding this comment

kerthcet Dec 22, 2023

Choose a reason for hiding this comment

mimowo left a comment

Choose a reason for hiding this comment

mimowo Dec 22, 2023

Choose a reason for hiding this comment

alculquicondor Dec 27, 2023

Choose a reason for hiding this comment

kerthcet Dec 28, 2023

Choose a reason for hiding this comment

kerthcet commented Dec 23, 2023

kerthcet commented Dec 23, 2023

kerthcet commented Dec 28, 2023

B1F030 commented Dec 28, 2023 • edited Loading

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 28, 2023

k8s-ci-robot commented Dec 28, 2023

kerthcet commented Dec 19, 2023 •

edited

Loading

netlify bot commented Dec 19, 2023 •

edited

Loading

B1F030 commented Dec 28, 2023 •

edited

Loading