Move sortPodsBasedOnPriority to pod util #322

lixiang233 · 2020-06-10T15:36:50Z

As #294 mentioned, we may need to sort pods in other strategies, so I moved this func to pod util.

k8s-ci-robot · 2020-06-10T15:36:58Z

Hi @lixiang233. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

lixiang233 · 2020-06-10T15:39:54Z

pkg/descheduler/pod/pods_test.go

+var (
+	lowPriority  = int32(0)
+	highPriority = int32(10000)
+)
+
+func setHighPriority(pod *v1.Pod) { pod.Spec.Priority = &highPriority }
+func setLowPriority(pod *v1.Pod)  { pod.Spec.Priority = &lowPriority }
+
+func makeBestEffortPod(pod *v1.Pod) {
+	pod.Spec.Containers[0].Resources.Requests = nil
+	pod.Spec.Containers[0].Resources.Requests = nil
+	pod.Spec.Containers[0].Resources.Limits = nil
+	pod.Spec.Containers[0].Resources.Limits = nil
+}
+
+func makeBurstablePod(pod *v1.Pod) {
+	pod.Spec.Containers[0].Resources.Limits = nil
+	pod.Spec.Containers[0].Resources.Limits = nil
+}
+
+func makeGuaranteedPod(pod *v1.Pod) {
+	pod.Spec.Containers[0].Resources.Limits[v1.ResourceCPU] = pod.Spec.Containers[0].Resources.Requests[v1.ResourceCPU]
+	pod.Spec.Containers[0].Resources.Limits[v1.ResourceMemory] = pod.Spec.Containers[0].Resources.Requests[v1.ResourceMemory]
+}
+


Should I move these to test util? @ingvagabund @seanmalloy

@lixiang233 yes, in my opinion I think it would be good to have makeGuaranteedPod, makeBurstablePod and makeBestEffortPod in test/test_utils.go

seanmalloy · 2020-06-11T04:24:07Z

/ok-to-test
/kind cleanup

@lixiang233 I think the code change look pretty good. I just have two questions.

Are you planning to update the RemovePodsViolatingInterPodAntiAffinity strategy to sort pods by priority in a follow up pull request?

You mentioned in #294 that the documentation in the Pod Evictions section of the README is not quite accurate because only the LowNodeUtilization strategy sorts pods. Can you update the README in this PR too?

lixiang233 · 2020-06-11T08:16:09Z

Changed test util and README.

I'm willing to work on sorting pods in RemovePodsViolatingInterPodAntiAffinity strategy. I'll probably update it and create a PR in the next week. @seanmalloy

ingvagabund · 2020-06-12T08:39:27Z

pkg/descheduler/pod/pods.go

+// SortPodsBasedOnPriorityLowToHigh sorts pods based on their priorities from low to high.
+// If pods have same priorities, they will be sorted by QoS in the following order:
+// BestEffort, Burstable, Guaranteed
+func SortPodsBasedOnPriorityLowToHigh(pods []*v1.Pod) {


What about to have:

func PodComparePriorityLowToHigh(i, j int) bool { if pods[i].Spec.Priority == nil && pods[j].Spec.Priority != nil { return true } if pods[j].Spec.Priority == nil && pods[i].Spec.Priority != nil { return false } if (pods[j].Spec.Priority == nil && pods[i].Spec.Priority == nil) || (*pods[i].Spec.Priority == *pods[j].Spec.Priority) { if IsBestEffortPod(pods[i]) { return true } if IsBurstablePod(pods[i]) && IsGuaranteedPod(pods[j]) { return true } return false } return *pods[i].Spec.Priority < *pods[j].Spec.Priority }

instead and replace all invocations of SortPodsBasedOnPriorityLowToHigh with sort.Slice(pods, PodComparePriorityLowToHigh? So the implementation is minimal and the function can be used by other sort.Slice* functions. E.g. SliceIsSorted or SliceStable.

This implementation looks like less func of QueueSortPlugin, as func in Slice(slice interface{}, less func(i, j int) only receives two int params, we may implement and use it like:

func PodComparePriorityLowToHigh(i, j pod) bool {...} sort.Slice(pods, func(i, j int) bool { return PodComparePriorityLowToHigh(pod[i], pod[j]) }

That looks a little complex, besides I think pod util should provide a sort func and not only a less func, maybe we can provide both and refactor sort func like:

func PodComparePriorityLowToHigh(i, j pod) bool {...} func SortPodsBasedOnPriorityLowToHigh(pods []*v1.Pod) { sort.Slice(pods, func(i, j int) bool { return PodComparePriorityLowToHigh(pod[i], pod[j]) } }

What do you think @ingvagabund ?

Sorry about that. Yeah, you are right. Too much lambda calculus in my head :) We can elaborate on that later once we have more callers of the same "less" implementation for sorting.

ingvagabund · 2020-06-13T11:43:19Z

pkg/descheduler/strategies/lownodeutilization_test.go

@@ -52,23 +50,6 @@ func setHighPriority(pod *v1.Pod)        { pod.Spec.Priority = &highPriority }
 func setLowPriority(pod *v1.Pod)         { pod.Spec.Priority = &lowPriority }
 func setNodeUnschedulable(node *v1.Node) { node.Spec.Unschedulable = true }


There's nothing specific to LowNodeUtilization strategy about these helper functions. You can move all of them under test/test_utils.go as well.

lixiang233 · 2020-06-15T03:02:20Z

Updated, I think low/high priority should be defined by user, so only provide SetPodPriority in testutil, I'll squash these 2 commits later if it's ok. @ingvagabund

ingvagabund · 2020-06-15T08:59:22Z

@lixiang233 can you put all the helpers function moved to the same commit? Two commits are more preferable here: 1 for moving the helper functions, 1 for moving the sort function.

lixiang233 · 2020-06-16T03:08:08Z

Completed. @ingvagabund

seanmalloy · 2020-06-16T04:45:49Z

/lgtm
/assign @ingvagabund @damemi

ingvagabund · 2020-06-16T09:55:08Z

@lixiang233 just a nit. Can you switch order of the commits so the helpers are not copied into pkg/descheduler/pod/pods_test.go and then removed in the next? Othwerwise, lgtm,

lixiang233 · 2020-06-16T11:27:29Z

switched. @ingvagabund

ingvagabund · 2020-06-16T11:47:48Z

@lixiang233 perfect, thanks!!!
/lgtm

damemi

There's nothing wrong with this change itself, but without applying the change to other strategies as mentioned in #294 this PR doesn't really do much.

I think moving sortPodsBasedOnPriority is just a part of a whole PR which would use that function in other strategies. So I would like to see the rest of #294 implemented here (for at least a couple strategies), so that we can see the whole outcome of this change (and what it is intended to provide). Moving the function alone doesn't necessitate its own PR

lixiang233 · 2020-06-16T16:08:36Z

I'm planning to work on RemovePodsViolatingInterPodAntiAffinity strategy and will commit recently. I think we can divide the whole process into two parts, one is for multi-pods-related-strategies the other is for single-pod-related-strategies because these two types of strategies use different sorting functions. So I prefer to complete the work of multi-pods-related-strategies in this PR and open another for future work of single-pod-related-strategies.

What do you think @damemi ?

damemi · 2020-06-16T16:35:08Z

@lixiang233 that sounds good to me, feel free to add the changes for multi-pods in a new commit. Thanks!

I definitely agree that we don't need to make too many changes at once, I just felt like we could have a little more here to show the ultimate goal.

lixiang233 · 2020-06-19T03:55:08Z

Changed RemovePodsViolatingInterPodAntiAffinity strategy and added a simple testcase for it. Please take a look @seanmalloy @ingvagabund @damemi

damemi

This looks good to me, I think the separate commits make sense here but if anyone prefers to squash I wouldn't mind that either.
/approve

k8s-ci-robot · 2020-06-22T18:54:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, lixiang233

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

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

seanmalloy · 2020-06-22T19:00:17Z

This looks good to me. Travis CI still seems to be flaky, so the e2e tests did not run. Someone should run the e2e tests on their laptop before this is merged.

seanmalloy · 2020-06-22T19:16:05Z

/lgtm

I was able to run the e2e tests on my laptop using k8s 1.18.2 ...

make test-e2e
ls: cannot access '_output/bin/golangci-lint': No such file or directory
./test/run-e2e-tests.sh
=== RUN   TestE2E
--- PASS: TestE2E (31.29s)
    e2e_test.go:261: Eviction of pods starting
    e2e_test.go:261: Eviction of pods starting
=== RUN   TestDeschedulingInterval
--- PASS: TestDeschedulingInterval (0.02s)
PASS
ok  	sigs.k8s.io/descheduler/test/e2e	31.509s

Move sortPodsBasedOnPriority to pod util

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 10, 2020

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. labels Jun 10, 2020

k8s-ci-robot requested review from k82cn and seanmalloy June 10, 2020 15:37

lixiang233 commented Jun 10, 2020

View reviewed changes

lixiang233 force-pushed the move_pod_sort branch from d4b1ee6 to 17b732f Compare June 11, 2020 08:02

ingvagabund reviewed Jun 12, 2020

View reviewed changes

ingvagabund reviewed Jun 13, 2020

View reviewed changes

lixiang233 force-pushed the move_pod_sort branch from 7084bd0 to 7b27c7f Compare June 16, 2020 03:05

k8s-ci-robot assigned damemi, ingvagabund and seanmalloy Jun 16, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2020

lixiang233 added 2 commits June 16, 2020 19:17

Move helper funcs to testutil

7457626

Move sortPodsBasedOnPriority to podutil

43525f6

lixiang233 force-pushed the move_pod_sort branch from 7b27c7f to 43525f6 Compare June 16, 2020 11:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2020

damemi requested changes Jun 16, 2020

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2020

Add sorting to RemovePodsViolatingInterPodAntiAffinity

65a03e7

lixiang233 force-pushed the move_pod_sort branch from ba93a6d to 65a03e7 Compare June 19, 2020 06:12

damemi approved these changes Jun 22, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2020

k8s-ci-robot merged commit c713537 into kubernetes-sigs:master Jun 22, 2020

lixiang233 deleted the move_pod_sort branch June 23, 2020 01:00

seanmalloy mentioned this pull request Jul 1, 2020

Sorting pods before evicting may be helpful #294

Closed

briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022

Merge pull request kubernetes-sigs#322 from lixiang233/move_pod_sort

be06f34

Move sortPodsBasedOnPriority to pod util

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move sortPodsBasedOnPriority to pod util #322

Move sortPodsBasedOnPriority to pod util #322

lixiang233 commented Jun 10, 2020

k8s-ci-robot commented Jun 10, 2020

lixiang233 Jun 10, 2020

seanmalloy Jun 11, 2020

seanmalloy commented Jun 11, 2020

lixiang233 commented Jun 11, 2020

ingvagabund Jun 12, 2020 •

edited

Loading

lixiang233 Jun 12, 2020

ingvagabund Jun 13, 2020

ingvagabund Jun 13, 2020

lixiang233 commented Jun 15, 2020

ingvagabund commented Jun 15, 2020

lixiang233 commented Jun 16, 2020

seanmalloy commented Jun 16, 2020

ingvagabund commented Jun 16, 2020

lixiang233 commented Jun 16, 2020

ingvagabund commented Jun 16, 2020

damemi left a comment

lixiang233 commented Jun 16, 2020

damemi commented Jun 16, 2020 •

edited

Loading

lixiang233 commented Jun 19, 2020

damemi left a comment

k8s-ci-robot commented Jun 22, 2020

seanmalloy commented Jun 22, 2020

seanmalloy commented Jun 22, 2020

		@@ -52,23 +50,6 @@ func setHighPriority(pod *v1.Pod) { pod.Spec.Priority = &highPriority }
		func setLowPriority(pod *v1.Pod) { pod.Spec.Priority = &lowPriority }
		func setNodeUnschedulable(node *v1.Node) { node.Spec.Unschedulable = true }

Move sortPodsBasedOnPriority to pod util #322

Move sortPodsBasedOnPriority to pod util #322

Conversation

lixiang233 commented Jun 10, 2020

k8s-ci-robot commented Jun 10, 2020

lixiang233 Jun 10, 2020

Choose a reason for hiding this comment

seanmalloy Jun 11, 2020

Choose a reason for hiding this comment

seanmalloy commented Jun 11, 2020

lixiang233 commented Jun 11, 2020

ingvagabund Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

lixiang233 Jun 12, 2020

Choose a reason for hiding this comment

ingvagabund Jun 13, 2020

Choose a reason for hiding this comment

ingvagabund Jun 13, 2020

Choose a reason for hiding this comment

lixiang233 commented Jun 15, 2020

ingvagabund commented Jun 15, 2020

lixiang233 commented Jun 16, 2020

seanmalloy commented Jun 16, 2020

ingvagabund commented Jun 16, 2020

lixiang233 commented Jun 16, 2020

ingvagabund commented Jun 16, 2020

damemi left a comment

Choose a reason for hiding this comment

lixiang233 commented Jun 16, 2020

damemi commented Jun 16, 2020 • edited Loading

lixiang233 commented Jun 19, 2020

damemi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 22, 2020

seanmalloy commented Jun 22, 2020

seanmalloy commented Jun 22, 2020

ingvagabund Jun 12, 2020 •

edited

Loading

damemi commented Jun 16, 2020 •

edited

Loading