skip eviction when pod creation time is below minPodAge threshold setting #1475

victorgs · 2024-07-23T16:30:37Z

In the default initialization phase of the descheduler, add a new constraint to not evict pods that creation time is below minPodAge threshold. I have used minutes for the threshold, but if you prefer seconds, I don't mind changing it.

Added value:

Avoid crazy pod movement when the autoscaler scales up and down.
Avoid evicting pods when they are warming up.
Decreases the overall cost of eviction as no pod will be evicted before doing significant amount of work.
Guard against scheduling. Descheduling loops in situations where the descheduler has a different node fit logic from scheduler, like not considering topology spread constraints.

I was testing the change in a local cluster:
I0716 13:51:27.441401 1 defaultevictor.go:249] "Pod fails the following checks" pod="kube-system/nginx-deployment-8-68c4499688-vjvp8" checks="pod age is not older than MinPodAge: 10000 minutes"

We think adding this feature upstream is more beneficial than maintaining our own fork, and it may be useful to others.

…ting In the default initialization phase of the descheduler, add a new constraint to not evict pods that creation time is below minPodAge threshold. Added value: - Avoid crazy pod movement when the autoscaler scales up and down. - Avoid evicting pods when they are warming up. - Decreases the overall cost of eviction as no pod will be evicted before doing significant amount of work. - Guard against scheduling. Descheduling loops in situations where the descheduler has a different node fit logic from scheduler, like not considering topology spread constraints.

k8s-ci-robot · 2024-07-23T16:30:47Z

Hi @victorgs. 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-sigs/prow repository.

ingvagabund · 2024-07-24T10:55:42Z

/ok-to-test

ingvagabund

Thank you for this improvement. This is a useful addition to the project. Much appreciated.

Users will need to keep in mind to avoid combining PodLifeTime's maxPodLifeTimeSeconds with minPodAge incorrectly. We currently do not validate accross plugin configurations to detect invalid combinations.

ingvagabund · 2024-07-24T11:01:18Z

pkg/api/v1alpha1/types.go

@@ -55,6 +55,9 @@ type DeschedulerPolicy struct {

 	// MaxNoOfPodsToTotal restricts maximum of pods to be evicted total.
 	MaxNoOfPodsToEvictTotal *uint `json:"maxNoOfPodsToEvictTotal,omitempty"`
+
+	// MinPodAge prevents recently created pods (within the specified minutes) from being evicted.
+	MinPodAge *uint `json:"minPodAge,omitempty"`


v1alpha1 is deprecated, we will be removing this type soon. Better to avoid extending it. Also, a conversion needs to be wired to take the new field into account.

Thank you for your comments Jan :)

I did not get this one tbh, I removed MinPodAge from v1alpha1 and move it to DefaultEvictorArgs

descheduler/pkg/framework/plugins/defaultevictor/types.go

Lines 25 to 39 in ec7ad04

type DefaultEvictorArgs struct {

metav1.TypeMeta `json:",inline"`

NodeSelector string `json:"nodeSelector"`

EvictLocalStoragePods bool `json:"evictLocalStoragePods"`

EvictDaemonSetPods bool `json:"evictDaemonSetPods"`

EvictSystemCriticalPods bool `json:"evictSystemCriticalPods"`

IgnorePvcPods bool `json:"ignorePvcPods"`

EvictFailedBarePods bool `json:"evictFailedBarePods"`

LabelSelector *metav1.LabelSelector `json:"labelSelector"`

PriorityThreshold *api.PriorityThreshold `json:"priorityThreshold"`

NodeFit bool `json:"nodeFit"`

MinReplicas uint `json:"minReplicas"`

MinPodAge *metav1.Duration `json:"minPodAge"`

}

So if we want to avoid extending v1alpha1, I do not know which conversion we need to do.

This is perfect :) I meant any new field under v1alpha1 type needs a conversion from v1alpha1 -> v1alpha2 included as well. Nothing else to be done here. Thank you.

pkg/framework/plugins/defaultevictor/types.go

ingvagabund · 2024-07-26T10:12:01Z

README.md

@@ -145,7 +145,7 @@ The Default Evictor Plugin is used by default for filtering pods before processi
 |`priorityThreshold`|`priorityThreshold`||(see [priority filtering](#priority-filtering))|
 |`nodeFit`|`bool`|`false`|(see [node fit filtering](#node-fit-filtering))|
 |`minReplicas`|`uint`|`0`| ignore eviction of pods where owner (e.g. `ReplicaSet`) replicas is below this threshold |
-|`minPodAge`|`uint`|`0`| ignore eviction of pods with a creation time within this threshold (in minutes) |
+|`minPodAge`|`metav1.Duration`|`0`| ignore eviction of pods with a creation time within this threshold (in minutes) |


"in minutes" can be dropped

Done in a8502e4 ty!

ingvagabund · 2024-07-26T10:14:01Z

pkg/framework/plugins/defaultevictor/defaultevictor.go

-			if pod.Status.StartTime == nil || time.Since(pod.Status.StartTime.Time) < time.Duration(defaultEvictorArgs.MinPodAge)*time.Minute {
-				return fmt.Errorf("pod age is not older than MinPodAge: %d minutes", defaultEvictorArgs.MinPodAge)
+			if pod.Status.StartTime == nil || time.Since(pod.Status.StartTime.Time) < defaultEvictorArgs.MinPodAge.Duration {
+				return fmt.Errorf("pod age is not older than MinPodAge: %d seconds", uint(defaultEvictorArgs.MinPodAge.Duration.Seconds()))


Duration.String() will help to keep the value short and redeable when user chooses high duration. https://pkg.go.dev/time#Duration.String

Done in 9adea9f

ingvagabund · 2024-07-26T10:16:07Z

pkg/api/v1alpha1/conversion.go

@@ -19,7 +19,6 @@ package v1alpha1
 import (
 	"context"
 	"fmt"
-


Can you please undone the new line removal? To keep this file unchanged.

Done in 61b7b99

ingvagabund · 2024-07-26T10:20:42Z

pkg/framework/plugins/defaultevictor/defaultevictor_test.go

@@ -326,6 +329,8 @@ func TestDefaultEvictorFilter(t *testing.T) {
 	lowPriority := int32(800)
 	highPriority := int32(900)

+	minPodAge := metav1.Duration{50 * time.Minute}


pkg/framework/plugins/defaultevictor/defaultevictor_test.go:332:15: k8s.io/apimachinery/pkg/apis/meta/v1.Duration struct literal uses unkeyed fields

metav1.Duration{Duration: 50 * time.Minute}

Done in c7de3c4

victorgs · 2024-07-26T12:28:56Z

pkg/framework/plugins/defaultevictor/defaultevictor.go

@@ -64,6 +65,7 @@ func HaveEvictAnnotation(pod *v1.Pod) bool {
 }

 // New builds plugin from its arguments while passing a handle
+// nolint: gocyclo


@ingvagabund do not want to forget about this. I need to add it to make the linter pass. I saw the codebase has a bunch of these, but if you want to proceed with something else let me know.

This is fine. We will need to examine the code at some point and reduce the complexities.

ingvagabund · 2024-07-26T12:55:58Z

/label tide/merge-method-squash

ingvagabund · 2024-07-26T12:56:03Z

/approve
/lgtm

k8s-ci-robot · 2024-07-26T12:56:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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

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

victorgs · 2024-07-26T13:02:43Z

@ingvagabund thanks for your help and review 🙇

ingvagabund · 2024-07-26T13:16:49Z

Happy to help. Thank you for your time and contribution.

k8s-ci-robot requested review from ingvagabund and knelasevero July 23, 2024 16:30

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 23, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 23, 2024

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 Jul 24, 2024

ingvagabund self-assigned this Jul 24, 2024

ingvagabund requested changes Jul 24, 2024

View reviewed changes

Use *time.Duration instead of uint for MinPodAge type

ec7ad04

ingvagabund requested changes Jul 26, 2024

View reviewed changes

ingvagabund reviewed Jul 26, 2024

View reviewed changes

victorgs added 4 commits July 26, 2024 14:18

Remove '(in minutes)' from default evictor configuration table

a8502e4

make fmt

61b7b99

Add explicit name for Duration field

c7de3c4

Use Duration.String()

9adea9f

victorgs commented Jul 26, 2024

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 26, 2024

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

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

k8s-ci-robot merged commit 55a0812 into kubernetes-sigs:master Jul 26, 2024
8 checks passed

victorgs deleted the defaultevictor-minpodage branch July 26, 2024 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip eviction when pod creation time is below minPodAge threshold setting #1475

skip eviction when pod creation time is below minPodAge threshold setting #1475

victorgs commented Jul 23, 2024

k8s-ci-robot commented Jul 23, 2024

ingvagabund commented Jul 24, 2024

ingvagabund left a comment

ingvagabund Jul 24, 2024

victorgs Jul 26, 2024 •

edited

Loading

ingvagabund Jul 26, 2024

ingvagabund Jul 26, 2024

victorgs Jul 26, 2024

ingvagabund Jul 26, 2024

victorgs Jul 26, 2024

ingvagabund Jul 26, 2024

victorgs Jul 26, 2024

ingvagabund Jul 26, 2024

victorgs Jul 26, 2024

victorgs Jul 26, 2024 •

edited

Loading

ingvagabund Jul 26, 2024

ingvagabund commented Jul 26, 2024

ingvagabund commented Jul 26, 2024

k8s-ci-robot commented Jul 26, 2024

victorgs commented Jul 26, 2024 •

edited

Loading

ingvagabund commented Jul 26, 2024

	type DefaultEvictorArgs struct {
	metav1.TypeMeta `json:",inline"`

	NodeSelector string `json:"nodeSelector"`
	EvictLocalStoragePods bool `json:"evictLocalStoragePods"`
	EvictDaemonSetPods bool `json:"evictDaemonSetPods"`
	EvictSystemCriticalPods bool `json:"evictSystemCriticalPods"`
	IgnorePvcPods bool `json:"ignorePvcPods"`
	EvictFailedBarePods bool `json:"evictFailedBarePods"`
	LabelSelector *metav1.LabelSelector `json:"labelSelector"`
	PriorityThreshold *api.PriorityThreshold `json:"priorityThreshold"`
	NodeFit bool `json:"nodeFit"`
	MinReplicas uint `json:"minReplicas"`
	MinPodAge *metav1.Duration `json:"minPodAge"`
	}

skip eviction when pod creation time is below minPodAge threshold setting #1475

skip eviction when pod creation time is below minPodAge threshold setting #1475

Conversation

victorgs commented Jul 23, 2024

k8s-ci-robot commented Jul 23, 2024

ingvagabund commented Jul 24, 2024

ingvagabund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorgs Jul 26, 2024 • edited Loading

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

victorgs Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund commented Jul 26, 2024

ingvagabund commented Jul 26, 2024

k8s-ci-robot commented Jul 26, 2024

victorgs commented Jul 26, 2024 • edited Loading

ingvagabund commented Jul 26, 2024

victorgs Jul 26, 2024 •

edited

Loading

victorgs Jul 26, 2024 •

edited

Loading

victorgs commented Jul 26, 2024 •

edited

Loading