From fce9ac3eeabab78b4c15101bb57ca1791f82ff6b Mon Sep 17 00:00:00 2001 From: Michal Wozniak Date: Mon, 8 Jan 2024 17:46:16 +0100 Subject: [PATCH 1/2] Fix the borrowing while preemption when no borrowingLimit --- pkg/scheduler/preemption/preemption.go | 32 +++++++----- pkg/scheduler/preemption/preemption_test.go | 55 +++++++++++++++++++++ 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/pkg/scheduler/preemption/preemption.go b/pkg/scheduler/preemption/preemption.go index ed31b3f973..f9e45051f5 100644 --- a/pkg/scheduler/preemption/preemption.go +++ b/pkg/scheduler/preemption/preemption.go @@ -361,20 +361,26 @@ func workloadFits(wlReq cache.FlavorResourceQuantities, cq *cache.ClusterQueue, cohortResRequestable = cq.Cohort.RequestableResources[flvQuotas.Name] } for rName, rReq := range flvReq { - limit := flvQuotas.Resources[rName].Nominal - - // No need to check whether cohort is nil here because when borrowingLimit - // is non nil, cohort is always non nil. - if allowBorrowing && flvQuotas.Resources[rName].BorrowingLimit != nil { - limit += *flvQuotas.Resources[rName].BorrowingLimit - } - - if cqResUsage[rName]+rReq > limit { - return false + resource := flvQuotas.Resources[rName] + + if cq.Cohort != nil { + if allowBorrowing { + // When resource.BorrowingLimit == nil there is no borrowing + // limit, so we can skip the check. + if resource.BorrowingLimit != nil { + if cqResUsage[rName]+rReq > resource.Nominal+*resource.BorrowingLimit { + return false + } + } + } + if cohortResUsage[rName]+rReq > cohortResRequestable[rName] { + return false + } } - - if cq.Cohort != nil && cohortResUsage[rName]+rReq > cohortResRequestable[rName] { - return false + if cq.Cohort == nil || !allowBorrowing { + if cqResUsage[rName]+rReq > resource.Nominal { + return false + } } } } diff --git a/pkg/scheduler/preemption/preemption_test.go b/pkg/scheduler/preemption/preemption_test.go index 5120949199..cc7b4ab1ed 100644 --- a/pkg/scheduler/preemption/preemption_test.go +++ b/pkg/scheduler/preemption/preemption_test.go @@ -102,6 +102,30 @@ func TestPreemption(t *testing.T) { ReclaimWithinCohort: kueue.PreemptionPolicyAny, }). Obj(), + utiltesting.MakeClusterQueue("d1"). + Cohort("cohort-no-limits"). + ResourceGroup(*utiltesting.MakeFlavorQuotas("default"). + Resource(corev1.ResourceCPU, "6"). + Resource(corev1.ResourceMemory, "3Gi"). + Obj(), + ). + Preemption(kueue.ClusterQueuePreemption{ + WithinClusterQueue: kueue.PreemptionPolicyLowerPriority, + ReclaimWithinCohort: kueue.PreemptionPolicyLowerPriority, + }). + Obj(), + utiltesting.MakeClusterQueue("d2"). + Cohort("cohort-no-limits"). + ResourceGroup(*utiltesting.MakeFlavorQuotas("default"). + Resource(corev1.ResourceCPU, "6"). + Resource(corev1.ResourceMemory, "3Gi"). + Obj(), + ). + Preemption(kueue.ClusterQueuePreemption{ + WithinClusterQueue: kueue.PreemptionPolicyNever, + ReclaimWithinCohort: kueue.PreemptionPolicyAny, + }). + Obj(), utiltesting.MakeClusterQueue("l1"). Cohort("legion"). ResourceGroup(*utiltesting.MakeFlavorQuotas("default"). @@ -540,6 +564,37 @@ func TestPreemption(t *testing.T) { }), wantPreempted: sets.New("/c1-low"), }, + "preempting locally and borrowing same resource in cohort; no borrowing limit in the cohort": { + admitted: []kueue.Workload{ + *utiltesting.MakeWorkload("d1-med", ""). + Priority(0). + Request(corev1.ResourceCPU, "4"). + ReserveQuota(utiltesting.MakeAdmission("d1").Assignment(corev1.ResourceCPU, "default", "4").Obj()). + Obj(), + *utiltesting.MakeWorkload("d1-low", ""). + Priority(-1). + Request(corev1.ResourceCPU, "4"). + ReserveQuota(utiltesting.MakeAdmission("d1").Assignment(corev1.ResourceCPU, "default", "4").Obj()). + Obj(), + *utiltesting.MakeWorkload("d2-low-1", ""). + Priority(-1). + Request(corev1.ResourceCPU, "4"). + ReserveQuota(utiltesting.MakeAdmission("d2").Assignment(corev1.ResourceCPU, "default", "4").Obj()). + Obj(), + }, + incoming: utiltesting.MakeWorkload("in", ""). + Priority(1). + Request(corev1.ResourceCPU, "4"). + Obj(), + targetCQ: "d1", + assignment: singlePodSetAssignment(flavorassigner.ResourceAssignment{ + corev1.ResourceCPU: &flavorassigner.FlavorAssignment{ + Name: "default", + Mode: flavorassigner.Preempt, + }, + }), + wantPreempted: sets.New("/d1-low"), + }, "preempting locally and borrowing other resources in cohort, with cohort candidates": { admitted: []kueue.Workload{ *utiltesting.MakeWorkload("c1-med", ""). From a550f53f63796595dce5406ca194a79863976f48 Mon Sep 17 00:00:00 2001 From: Michal Wozniak Date: Tue, 9 Jan 2024 18:43:03 +0100 Subject: [PATCH 2/2] improve readability --- pkg/scheduler/preemption/preemption.go | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/pkg/scheduler/preemption/preemption.go b/pkg/scheduler/preemption/preemption.go index f9e45051f5..da75a7188d 100644 --- a/pkg/scheduler/preemption/preemption.go +++ b/pkg/scheduler/preemption/preemption.go @@ -363,24 +363,21 @@ func workloadFits(wlReq cache.FlavorResourceQuantities, cq *cache.ClusterQueue, for rName, rReq := range flvReq { resource := flvQuotas.Resources[rName] - if cq.Cohort != nil { - if allowBorrowing { - // When resource.BorrowingLimit == nil there is no borrowing - // limit, so we can skip the check. - if resource.BorrowingLimit != nil { - if cqResUsage[rName]+rReq > resource.Nominal+*resource.BorrowingLimit { - return false - } - } - } - if cohortResUsage[rName]+rReq > cohortResRequestable[rName] { - return false - } - } if cq.Cohort == nil || !allowBorrowing { if cqResUsage[rName]+rReq > resource.Nominal { return false } + } else { + // When resource.BorrowingLimit == nil there is no borrowing + // limit, so we can skip the check. + if resource.BorrowingLimit != nil { + if cqResUsage[rName]+rReq > resource.Nominal+*resource.BorrowingLimit { + return false + } + } + } + if cq.Cohort != nil && cohortResUsage[rName]+rReq > cohortResRequestable[rName] { + return false } } }