Repository: kubernetes-sigs/scheduler-plugins
Pull request #840 (2024-12-16 11:50:09)

fix(util): fix the problem of pod overhead not being calculated
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide/first-contribution.md#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/sig-release/release.md#issuepr-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/sig-testing/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

#### What type of PR is this?
/kind bug
/kind cleanup
<!--
Add one of the following kinds:
/kind bug
/kind cleanup
/kind documentation
/kind feature

Optionally add one or more of the following kinds if applicable:
/kind api-change
/kind deprecation
/kind failing-test
/kind flake
/kind regression
-->

#### What this PR does / why we need it:
- fix the problem of overhead not being calculated
#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes None

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->
```release-note
None
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/scheduler-plugins/pull/840#" title="Author self-approved">googs1025</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign [ffromani](https://github.com/ffromani) for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fscheduler-plugins).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/scheduler-plugins/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["ffromani"]} -->
netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *kubernetes-sigs-scheduler-plugins* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | e66235b5c9b5745617ea2558e14377fae9902c27 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/kubernetes-sigs-scheduler-plugins/deploys/676013f30d145c0008f0f894 |

Patch:
diff --git a/pkg/capacityscheduling/capacity_scheduling.go b/pkg/capacityscheduling/capacity_scheduling.go
index 75f235f5..1eb0ed8d 100644
--- a/pkg/capacityscheduling/capacity_scheduling.go
+++ b/pkg/capacityscheduling/capacity_scheduling.go
@@ -620,8 +620,6 @@ func (p *preemptor) SelectVictimsOnNode(
 
 	var victims []*v1.Pod
 	numViolatingVictim := 0
-	// Sort potentialVictims by pod priority from high to low, which ensures to
-	// reprieve higher priority pods first.
 	sort.Slice(potentialVictims, func(i, j int) bool {
 		return schedutil.MoreImportantPod(potentialVictims[i].Pod, potentialVictims[j].Pod)
 	})
@@ -668,11 +666,6 @@ func (p *preemptor) SelectVictimsOnNode(
 			return nil, 0, framework.AsStatus(err)
 		}
 	}
-
-	// Sort victims after reprieving pods to keep the pods in the victims sorted in order of priority from high to low.
-	if len(violatingVictims) != 0 && len(nonViolatingVictims) != 0 {
-		sort.Slice(victims, func(i, j int) bool { return schedutil.MoreImportantPod(victims[i], victims[j]) })
-	}
 	return victims, numViolatingVictim, framework.NewStatus(framework.Success)
 }
 
diff --git a/pkg/preemptiontoleration/preemption_toleration.go b/pkg/preemptiontoleration/preemption_toleration.go
index bf3ebd2e..7ab7767d 100644
--- a/pkg/preemptiontoleration/preemption_toleration.go
+++ b/pkg/preemptiontoleration/preemption_toleration.go
@@ -250,8 +250,6 @@ func (pl *PreemptionToleration) SelectVictimsOnNode(
 	}
 	var victims []*v1.Pod
 	numViolatingVictim := 0
-	// Sort potentialVictims by pod priority from high to low, which ensures to
-	// reprieve higher priority pods first.
 	sort.Slice(potentialVictims, func(i, j int) bool { return util.MoreImportantPod(potentialVictims[i].Pod, potentialVictims[j].Pod) })
 	// Try to reprieve as many pods as possible. We first try to reprieve the PDB
 	// violating victims and then other non-violating ones. In both cases, we start
@@ -286,11 +284,6 @@ func (pl *PreemptionToleration) SelectVictimsOnNode(
 			return nil, 0, framework.AsStatus(err)
 		}
 	}
-
-	// Sort victims after reprieving pods to keep the pods in the victims sorted in order of priority from high to low.
-	if len(violatingVictims) != 0 && len(nonViolatingVictims) != 0 {
-		sort.Slice(victims, func(i, j int) bool { return util.MoreImportantPod(victims[i], victims[j]) })
-	}
 	return victims, numViolatingVictim, framework.NewStatus(framework.Success)
 }
 
diff --git a/pkg/util/resource.go b/pkg/util/resource.go
index aee05f98..4d4d46fd 100644
--- a/pkg/util/resource.go
+++ b/pkg/util/resource.go
@@ -47,6 +47,7 @@ func ResourceList(r *framework.Resource) v1.ResourceList {
 // - the sum of all app containers(spec.Containers) request for a resource.
 // - the effective init containers(spec.InitContainers) request for a resource.
 // The effective init containers request is the highest request on all init containers.
+// Additionally, if the Pod specifies overhead, it will also be included in the final calculation.
 func GetPodEffectiveRequest(pod *v1.Pod) v1.ResourceList {
 	initResources := make(v1.ResourceList)
 	resources := make(v1.ResourceList)
@@ -73,5 +74,12 @@ func GetPodEffectiveRequest(pod *v1.Pod) v1.ResourceList {
 		}
 		resources[name] = quantity
 	}
+	// Add pod overhead if present
+	for name, quantity := range pod.Spec.Overhead {
+		if q, ok := resources[name]; ok {
+			quantity.Add(q)
+		}
+		resources[name] = quantity
+	}
 	return resources
 }


Test patch:
diff --git a/pkg/util/resource_test.go b/pkg/util/resource_test.go
index 2b1e7d3a..f8c17187 100644
--- a/pkg/util/resource_test.go
+++ b/pkg/util/resource_test.go
@@ -36,6 +36,7 @@ func TestGetPodEffectiveRequest(t *testing.T) {
 		name                 string
 		containerRequest     []v1.ResourceList
 		initContainerRequest []v1.ResourceList
+		podOverheadRequest   v1.ResourceList
 		want                 v1.ResourceList
 	}{
 		{
@@ -101,6 +102,27 @@ func TestGetPodEffectiveRequest(t *testing.T) {
 			},
 			want: makeResourceList(10, 4),
 		},
+		{
+			name: "1 container with pod overhead",
+			containerRequest: []v1.ResourceList{
+				makeResourceList(1, 1),
+			},
+			initContainerRequest: nil,
+			podOverheadRequest:   makeResourceList(1, 1),
+			want:                 makeResourceList(2, 2),
+		},
+		{
+			name: "2 containers and 1 init container with pod overhead",
+			containerRequest: []v1.ResourceList{
+				makeResourceList(1, 1),
+				makeResourceList(2, 3),
+			},
+			initContainerRequest: []v1.ResourceList{
+				makeResourceList(1, 1),
+			},
+			podOverheadRequest: makeResourceList(1, 1),
+			want:               makeResourceList(4, 5),
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -119,6 +141,7 @@ func TestGetPodEffectiveRequest(t *testing.T) {
 					},
 				})
 			}
+			pod.Spec.Overhead = tt.podOverheadRequest
 			if got := GetPodEffectiveRequest(pod); !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("GetPodEffectiveRequest() = %v, want %v", got, tt.want)
 			}