Fix incorrect snapshot usage when lendingLimit enabled #1770

kerthcet · 2024-02-26T16:39:31Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

cc @B1F030 I reproduced the bug with a preemption test, PTAL.

Which issue(s) this PR fixes:

Fixes #1385 (comment)

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix incorrect quota management when lendingLimit enabled in preemption

netlify · 2024-02-26T16:39:51Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`f6369a1`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65ef5365bcf59c00087f7817

alculquicondor · 2024-02-27T14:29:13Z

/cc

pkg/scheduler/preemption/preemption_test.go

alculquicondor · 2024-02-27T18:05:45Z

pkg/scheduler/preemption/preemption_test.go

@@ -1043,6 +1042,27 @@ func TestPreemption(t *testing.T) {
 			wantPreempted:      sets.New("/lend1-low", "/lend2-low"),
 			enableLendingLimit: true,
 		},
+		"preempt from one ClusterQueue in cohort-lend": {


Suggested change

"preempt from one ClusterQueue in cohort-lend": {

"cannot preempt from other ClusterQueue if past lending limit": {

Or another message that describes what the input and output is like.

alculquicondor · 2024-02-27T19:32:03Z

pkg/cache/snapshot.go

@@ -43,18 +43,57 @@ func (s *Snapshot) RemoveWorkload(wl *workload.Info) {
 	delete(cq.Workloads, workload.Key(wl.Obj))
 	updateUsage(wl, cq.Usage, -1)
 	if cq.Cohort != nil {
-		updateUsage(wl, cq.Cohort.Usage, -1)
+		if features.Enabled(features.LendingLimit) {


We should have unit tests just for these functions. We should test for the transitions from "usage below guaranteed" and "usage above guaranteed" and the reverse, but also to the same states.
I'm not convinced the current implementation works.

However, maybe a simpler approach is:

remove the existing "clusterqueue usage from the cohort"

updateUsage(wl, cq.Usage, +-1)

Add the new "clusterqueue usage from cohort"

I have added some tests that can prove the current implementation works.

However, maybe a simpler approach is:

+1, more readable to me. And I think the code is reuseable for Addworkload.

alculquicondor · 2024-02-27T19:36:21Z

pkg/cache/snapshot.go

+					flv, flvExist := cq.Cohort.Usage[wlResFlv]
+					if flvExist && wlResExist {
+						if _, exists := flv[wlRes]; exists {
+							cohortUsage := cq.Usage[wlResFlv][wlRes] - cq.guaranteedQuota(wlResFlv, wlRes)


Suggested change

cohortUsage := cq.Usage[wlResFlv][wlRes] - cq.guaranteedQuota(wlResFlv, wlRes)

nonGuaranteedUsage := cq.Usage[wlResFlv][wlRes] - cq.guaranteedQuota(wlResFlv, wlRes)

kerthcet · 2024-02-28T06:10:58Z

pkg/cache/snapshot.go

+			for _, ps := range wl.TotalRequests {
+				for wlRes, wlResFlv := range ps.Flavors {
+					v, wlResExist := ps.Requests[wlRes]
+					// update cq.Cohort.Usage


Remove this comment. Meaningless

kerthcet · 2024-02-28T06:13:19Z

pkg/cache/snapshot.go

@@ -43,18 +43,57 @@ func (s *Snapshot) RemoveWorkload(wl *workload.Info) {
 	delete(cq.Workloads, workload.Key(wl.Obj))
 	updateUsage(wl, cq.Usage, -1)
 	if cq.Cohort != nil {
-		updateUsage(wl, cq.Cohort.Usage, -1)
+		if features.Enabled(features.LendingLimit) {


However, maybe a simpler approach is:

+1, more readable to me. And I think the code is reuseable for Addworkload.

kerthcet · 2024-02-28T06:21:54Z

pkg/cache/snapshot_test.go

+			}(),
+			enableLendingLimit: true,
+		},
+		"remove lend-a-2": {


Plz make the test name more readable, like the intention you want to test here.

kerthcet · 2024-02-28T06:33:54Z

pkg/scheduler/preemption/preemption_test.go

@@ -1043,6 +1042,27 @@ func TestPreemption(t *testing.T) {
 			wantPreempted:      sets.New("/lend1-low", "/lend2-low"),
 			enableLendingLimit: true,
 		},
+		"cannot preempt from other ClusterQueues if past lending limit": {


Suggested change

"cannot preempt from other ClusterQueues if past lending limit": {

"cannot preempt from other ClusterQueues if exceeds requestable qutota including lending limit": {

kerthcet

General LGTM

kerthcet · 2024-02-28T08:30:52Z

pkg/cache/snapshot.go

@@ -43,18 +43,26 @@ func (s *Snapshot) RemoveWorkload(wl *workload.Info) {
 	delete(cq.Workloads, workload.Key(wl.Obj))
 	updateUsage(wl, cq.Usage, -1)
 	if cq.Cohort != nil {
-		updateUsage(wl, cq.Cohort.Usage, -1)
+		if features.Enabled(features.LendingLimit) {
+			updateCohortUsage(wl, cq, cq.Cohort.Usage, -1)


can we pass cq as parameters only?

pkg/cache/snapshot_test.go

kerthcet · 2024-02-28T08:36:50Z

pkg/cache/snapshot_test.go

+		remove             []string
+		add                []string
+		want               Snapshot
+		enableLendingLimit bool


No need of this field.

kerthcet · 2024-02-28T08:53:23Z

/assign @alculquicondor

alculquicondor · 2024-02-28T18:16:25Z

pkg/cache/snapshot_test.go

+			add:    []string{"/lend-a-1", "/lend-a-2", "/lend-a-3", "/lend-b-1"},
+			want:   initialSnapshot,
+		},
+		"remove all with lending limit": {


Suggested change

"remove all with lending limit": {

"remove all": {

alculquicondor · 2024-02-28T18:17:47Z

pkg/cache/snapshot_test.go

+	workloads := []kueue.Workload{
+		*utiltesting.MakeWorkload("lend-a-1", "").
+			Request(corev1.ResourceCPU, "1").
+			ReserveQuota(utiltesting.MakeAdmission("lend-a").Assignment(corev1.ResourceCPU, "default", "1000m").Obj()).


Suggested change

ReserveQuota(utiltesting.MakeAdmission("lend-a").Assignment(corev1.ResourceCPU, "default", "1000m").Obj()).

ReserveQuota(utiltesting.MakeAdmission("lend-a").Assignment(corev1.ResourceCPU, "default", "1").Obj()).

For readability. Do the same for the rest.

alculquicondor · 2024-02-28T18:30:28Z

pkg/cache/snapshot_test.go

+	}
+	cmpOpts := append(snapCmpOpts,
+		cmpopts.IgnoreFields(ClusterQueue{}, "NamespaceSelector", "Preemption", "Status"),
+		cmpopts.IgnoreFields(Cohort{}),


Something looks off here. Not sure what this is ignoring. I think we should ignore ClusterQueues within Cohort to avoid recursion.

This is ignoring the whole Cohort struct. In this test function, all cqs belong to a same cohort, so there will be no difference. I think it's ok not to change this. WDYT?

ping @alculquicondor

But we actually want to check that the Usage field matches what is set in tc.want

Sounds resonable, modified.

alculquicondor · 2024-02-28T18:31:03Z

pkg/scheduler/preemption/preemption_test.go

@@ -1043,6 +1041,27 @@ func TestPreemption(t *testing.T) {
 			wantPreempted:      sets.New("/lend1-low", "/lend2-low"),
 			enableLendingLimit: true,
 		},
+		"cannot preempt from other ClusterQueues if exceeds requestable qutota including lending limit": {


Suggested change

"cannot preempt from other ClusterQueues if exceeds requestable qutota including lending limit": {

"cannot preempt from other ClusterQueues if exceeds requestable quota including lending limit": {

good catch, thx. Done.

alculquicondor · 2024-03-08T18:16:52Z

pkg/scheduler/preemption/preemption_test.go

+				*utiltesting.MakeWorkload("lend2-low", "").
+					Priority(-1).
+					Request(corev1.ResourceCPU, "10").
+					ReserveQuota(utiltesting.MakeAdmission("lend2").Assignment(corev1.ResourceCPU, "default", "10000m").Obj()).


Suggested change

ReserveQuota(utiltesting.MakeAdmission("lend2").Assignment(corev1.ResourceCPU, "default", "10000m").Obj()).

ReserveQuota(utiltesting.MakeAdmission("lend2").Assignment(corev1.ResourceCPU, "default", "10").Obj()).

Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <b1fzhang@gmail.com>

alculquicondor · 2024-03-12T20:05:38Z

/lgtm
/approve

k8s-ci-robot · 2024-03-12T20:05:43Z

LGTM label has been added.

Git tree hash: 83aaa484b027d52b380c0d645f2093f6ab5ea720

k8s-ci-robot · 2024-03-12T20:05:45Z

[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:

~~pkg/OWNERS~~ [alculquicondor]

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

alculquicondor · 2024-03-12T20:05:50Z

/cherry-pick release-0.6

k8s-infra-cherrypick-robot · 2024-03-12T20:05:53Z

@alculquicondor: once the present PR merges, I will cherry-pick it on top of release-0.6 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.6

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.

k8s-infra-cherrypick-robot · 2024-03-12T20:30:29Z

@alculquicondor: new pull request created: #1826

In response to this:

/cherry-pick release-0.6

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.

kubernetes-sigs#1770) Signed-off-by: B1F030 <b1fzhang@gmail.com> Co-authored-by: B1F030 <b1fzhang@gmail.com>

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/bug Categorizes issue or PR as related to a bug. labels Feb 26, 2024

k8s-ci-robot requested review from denkensk and trasc February 26, 2024 16:39

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 26, 2024

B1F030 force-pushed the fix/lending-limit branch from 52e0e46 to 770cba4 Compare February 27, 2024 11:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 27, 2024

B1F030 mentioned this pull request Feb 27, 2024

Feature LendingLimit(KEP-1224) #1385

Merged

k8s-ci-robot requested a review from alculquicondor February 27, 2024 14:29

alculquicondor reviewed Feb 27, 2024

View reviewed changes

B1F030 force-pushed the fix/lending-limit branch from c79936a to 56fcfb1 Compare February 28, 2024 02:46

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2024

kerthcet commented Feb 28, 2024

View reviewed changes

kerthcet changed the title ~~[WIP]Fix incorrect snapshot usage when lendingLimit enabled~~ Fix incorrect snapshot usage when lendingLimit enabled Feb 28, 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 Feb 28, 2024

B1F030 force-pushed the fix/lending-limit branch from 322dcb2 to 65468ac Compare February 28, 2024 08:45

k8s-ci-robot assigned alculquicondor Feb 28, 2024

alculquicondor reviewed Feb 28, 2024

View reviewed changes

B1F030 force-pushed the fix/lending-limit branch from 65468ac to 502e289 Compare February 29, 2024 02:35

alculquicondor reviewed Mar 8, 2024

View reviewed changes

fix incorrect quota management when lendingLimit enabled in preemption

f6369a1

Co-authored-by: kerthcet <kerthcet@gmail.com> Signed-off-by: B1F030 <b1fzhang@gmail.com>

B1F030 force-pushed the fix/lending-limit branch from bb53767 to f6369a1 Compare March 11, 2024 18:54

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

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

k8s-ci-robot merged commit 604dc07 into kubernetes-sigs:main Mar 12, 2024
14 checks passed

k8s-ci-robot added this to the v0.7 milestone Mar 12, 2024

k8s-infra-cherrypick-robot mentioned this pull request Mar 12, 2024

[release-0.6] Fix incorrect snapshot usage when lendingLimit enabled #1826

Merged

vsoch pushed a commit to researchapps/kueue that referenced this pull request Apr 18, 2024

fix incorrect quota management when lendingLimit enabled in preemption (

94b9538

kubernetes-sigs#1770) Signed-off-by: B1F030 <b1fzhang@gmail.com> Co-authored-by: B1F030 <b1fzhang@gmail.com>

	"preempt from one ClusterQueue in cohort-lend": {
	"cannot preempt from other ClusterQueue if past lending limit": {

	cohortUsage := cq.Usage[wlResFlv][wlRes] - cq.guaranteedQuota(wlResFlv, wlRes)
	nonGuaranteedUsage := cq.Usage[wlResFlv][wlRes] - cq.guaranteedQuota(wlResFlv, wlRes)

	"cannot preempt from other ClusterQueues if past lending limit": {
	"cannot preempt from other ClusterQueues if exceeds requestable qutota including lending limit": {

	ReserveQuota(utiltesting.MakeAdmission("lend-a").Assignment(corev1.ResourceCPU, "default", "1000m").Obj()).
	ReserveQuota(utiltesting.MakeAdmission("lend-a").Assignment(corev1.ResourceCPU, "default", "1").Obj()).

Fix incorrect snapshot usage when lendingLimit enabled #1770

Fix incorrect snapshot usage when lendingLimit enabled #1770

Conversation

kerthcet commented Feb 26, 2024

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 Feb 26, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

alculquicondor commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

B1F030 Feb 28, 2024 • edited

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

kerthcet 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

Choose a reason for hiding this comment

kerthcet commented Feb 28, 2024

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

alculquicondor commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

k8s-ci-robot commented Mar 12, 2024

alculquicondor commented Mar 12, 2024

k8s-infra-cherrypick-robot commented Mar 12, 2024

k8s-infra-cherrypick-robot commented Mar 12, 2024

netlify bot commented Feb 26, 2024 •

edited

B1F030 Feb 28, 2024 •

edited