Ignore Pods With Deletion Timestamp #639

JaneLiuL · 2021-10-01T08:45:21Z

Enhance ignore Pods With Deletion Timestamp for all strategies
Issue: #638

k8s-ci-robot · 2021-10-01T08:45:29Z

Hi @JaneLiuL. 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.

JaneLiuL · 2021-10-01T10:40:16Z

/ok-to-test

k8s-ci-robot · 2021-10-01T10:40:29Z

@JaneLiuL: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

damemi

/ok-to-test

pkg/descheduler/strategies/topologyspreadconstraint.go

pkg/descheduler/evictions/evictions.go

pkg/utils/pod.go

JaneLiuL · 2021-10-02T02:34:03Z

/retest

pkg/descheduler/evictions/evictions.go

a7i

Thoughts on updating README to indicate that going forward Pods that are being deleted are ignored. This is important because Pods that have a long termination grace period will no longer be considered for eviction so it's a change in behavior.

pkg/utils/pod.go

damemi · 2021-10-15T13:41:25Z

Thoughts on updating README to indicate that going forward Pods that are being deleted are ignored. This is important because Pods that have a long termination grace period will no longer be considered for eviction so it's a change in behavior.

This should definitely be updated in the readme, yeah. We might even want to make this a non-default behavior

JaneLiuL · 2021-10-17T11:46:23Z

Thoughts on updating README to indicate that going forward Pods that are being deleted are ignored. This is important because Pods that have a long termination grace period will no longer be considered for eviction so it's a change in behavior.

This should definitely be updated in the readme, yeah. We might even want to make this a non-default behavior

Update done, please check~

JaneLiuL · 2021-10-21T01:15:29Z

/assign @damemi

damemi

/hold cancel
/approve
one nit in the readme but I think it looks good. @seanmalloy do you want to take a look too?

README.md

k8s-ci-robot · 2021-10-26T13:57:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a7i, damemi, JaneLiuL

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

JaneLiuL · 2021-10-27T02:10:27Z

/retest

ingvagabund · 2021-11-01T13:30:30Z

pkg/descheduler/strategies/pod_lifetime_test.go

@@ -245,12 +253,34 @@ func TestPodLifeTime(t *testing.T) {
 			pods:                    []v1.Pod{*p12, *p13},
 			nodes:                   []*v1.Node{node1},
 			expectedEvictedPodCount: 1,
+			testFunc:                func(podList []v1.Pod) {},


There's no need for testFunc which sets the DeletionTimestamp only for pods p12 and p13. Can you create pods p14 and p15 instead as copies of p12 and p13 and set the DeletionTimestamp explicitly?

Hi @ingvagabund thanks for the reivew~~
I think testFunc is more easy to extend on the test case. This time we only enhance for DeletionTimestamp, maybe next time we need to do other actions on the Pod. So for me, testFunc is more like a BuildTestNode or BuildTestPod.

@JaneLiuL that's not bad thinking to think ahead, but we don't have any use cases or evidence that we'll need more extensibility at this time. If we do come across that, the test can be refactored then, but for now adding a testFunc is unnecessary

@ingvagabund @damemi Thanks for your comment~
Then I will try to remove the testFunc, and create the new testPod with deleteTimestamp, is it better?

Already remove testFunc, may I click Resolve conversation?

Yeah this is looking a lot cleaner now, thanks!

pkg/descheduler/strategies/pod_antiaffinity_test.go

ingvagabund · 2021-11-01T13:36:25Z

pkg/descheduler/strategies/pod_antiaffinity_test.go

+			expectedEvictedPodCount: 3,
+			testFunc: func(podList []v1.Pod) {
+				for _, pod := range podList {
+					pod.DeletionTimestamp = &metav1.Time{}


This actually modifies the pods permanently. Each test needs a freshly created pod. Instead, can you create new pods (e.g. p5 - p8) which have the DeletionTimestamp set right away?

Thanks @ingvagabund ~ I learned a lot~~ . I don't know that testcase need a freashly created pod before. So I would create a new pod for this test case, and because it already have (p5-p8) so I would create a (p9-p10) is it ok for you?

Fix done~ Please check :)

@JaneLiuL if you are creating p9 and p10 now specifically for this test, do you still need testFunc at all? you can just specify those pods with an empty timestamp

Already remove testFunc, may I click Resolve conversation?

pkg/descheduler/strategies/pod_antiaffinity_test.go

damemi · 2021-11-09T18:29:05Z

pkg/descheduler/strategies/pod_lifetime_test.go

@@ -245,12 +253,34 @@ func TestPodLifeTime(t *testing.T) {
 			pods:                    []v1.Pod{*p12, *p13},
 			nodes:                   []*v1.Node{node1},
 			expectedEvictedPodCount: 1,
+			testFunc:                func(podList []v1.Pod) {},


@JaneLiuL that's not bad thinking to think ahead, but we don't have any use cases or evidence that we'll need more extensibility at this time. If we do come across that, the test can be refactored then, but for now adding a testFunc is unnecessary

damemi · 2021-11-09T18:29:36Z

pkg/descheduler/strategies/pod_lifetime_test.go

 		},
 	}

 	for _, tc := range testCases {
 		fakeClient := &fake.Clientset{}
-
+		tc.testFunc([]v1.Pod{*p12, *p13})


this is again calling testFunc for just p12 and p13 on every test, even though the pods are only used in 1 test. this shouldn't be necessary

Already remove testFunc, may I click Resolve conversation?

ingvagabund · 2021-11-15T16:42:53Z

/lgtm

Ignore Pods With Deletion Timestamp

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 1, 2021

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 Oct 1, 2021

k8s-ci-robot requested review from k82cn and seanmalloy October 1, 2021 08:45

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 1, 2021

damemi reviewed Oct 1, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 1, 2021

a7i reviewed Oct 1, 2021

View reviewed changes

pkg/descheduler/strategies/topologyspreadconstraint.go Show resolved Hide resolved

a7i reviewed Oct 1, 2021

View reviewed changes

pkg/descheduler/evictions/evictions.go Outdated Show resolved Hide resolved

a7i reviewed Oct 1, 2021

View reviewed changes

pkg/utils/pod.go Show resolved Hide resolved

damemi reviewed Oct 4, 2021

View reviewed changes

pkg/descheduler/evictions/evictions.go Outdated Show resolved Hide resolved

a7i approved these changes Oct 14, 2021

View reviewed changes

a7i reviewed Oct 14, 2021

View reviewed changes

pkg/utils/pod.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2021

k8s-ci-robot assigned damemi Oct 21, 2021

damemi reviewed Oct 26, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2021

ingvagabund requested changes Nov 1, 2021

View reviewed changes

a7i mentioned this pull request Nov 5, 2021

Added request considerations to NodeFit Feature #636

Closed

damemi reviewed Nov 9, 2021

View reviewed changes

Ignore Pods With Deletion Timestamp

c752470

k8s-ci-robot assigned ingvagabund Nov 15, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2021

k8s-ci-robot merged commit d0f11a4 into kubernetes-sigs:master Nov 15, 2021

a7i mentioned this pull request Jan 29, 2022

Ignore Pods With Deletion Timestamp #638

Closed

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

Merge pull request kubernetes-sigs#639 from JaneLiuL/master

ea1f20d

Ignore Pods With Deletion Timestamp

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore Pods With Deletion Timestamp #639

Ignore Pods With Deletion Timestamp #639

JaneLiuL commented Oct 1, 2021 •

edited

k8s-ci-robot commented Oct 1, 2021

JaneLiuL commented Oct 1, 2021

k8s-ci-robot commented Oct 1, 2021

damemi left a comment

JaneLiuL commented Oct 2, 2021

a7i left a comment

damemi commented Oct 15, 2021

JaneLiuL commented Oct 17, 2021

JaneLiuL commented Oct 21, 2021

damemi left a comment

k8s-ci-robot commented Oct 26, 2021

JaneLiuL commented Oct 27, 2021

ingvagabund Nov 1, 2021

JaneLiuL Nov 2, 2021

damemi Nov 9, 2021

JaneLiuL Nov 10, 2021

JaneLiuL Nov 10, 2021

damemi Nov 15, 2021

ingvagabund Nov 1, 2021

JaneLiuL Nov 2, 2021 •

edited

JaneLiuL Nov 2, 2021

damemi Nov 9, 2021

JaneLiuL Nov 10, 2021

damemi Nov 9, 2021

damemi Nov 9, 2021

JaneLiuL Nov 10, 2021

ingvagabund commented Nov 15, 2021

Ignore Pods With Deletion Timestamp #639

Ignore Pods With Deletion Timestamp #639

Conversation

JaneLiuL commented Oct 1, 2021 • edited

k8s-ci-robot commented Oct 1, 2021

JaneLiuL commented Oct 1, 2021

k8s-ci-robot commented Oct 1, 2021

damemi left a comment

Choose a reason for hiding this comment

JaneLiuL commented Oct 2, 2021

a7i left a comment

Choose a reason for hiding this comment

damemi commented Oct 15, 2021

JaneLiuL commented Oct 17, 2021

JaneLiuL commented Oct 21, 2021

damemi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 26, 2021

JaneLiuL commented Oct 27, 2021

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

JaneLiuL Nov 2, 2021 • 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

ingvagabund commented Nov 15, 2021

JaneLiuL commented Oct 1, 2021 •

edited

JaneLiuL Nov 2, 2021 •

edited