skip eviction when replica count is below evictor `minReplicas` threshold setting #1257

a7i · 2023-10-07T03:33:21Z

alternate implementation to #1152

Closes #1058

/cc @jiangxiaobin96

a7i · 2023-11-12T16:41:05Z

@ingvagabund and @knelasevero what are your thoughts on this PR?

knelasevero · 2023-11-20T14:55:37Z

I think this makes sense. However I want to test somethings I just thought about.

knelasevero · 2023-11-20T14:56:24Z

Do we want to backport this and also make this available in v1alpha1 while we still maintain it? Since this is additive it shouldn't be too hard

a7i · 2023-11-20T15:39:30Z

@knelasevero I would prefer to encourage users to migrate to v1alpha2. Adding new functionality is one way to do it. But if you feel that we should support both, I'm happy to add it.

knelasevero · 2023-11-20T16:55:35Z

@knelasevero I would prefer to encourage users to migrate to v1alpha2. Adding new functionality is one way to do it. But if you feel that we should support both, I'm happy to add it.

I actually agree. Just asking so the decision is explicit in this PR

knelasevero · 2023-11-21T12:36:23Z

/lgtm

ingvagabund · 2023-11-21T13:05:10Z

pkg/framework/plugins/defaultevictor/defaultevictor.go

@@ -143,6 +143,21 @@ func New(args runtime.Object, handle frameworktypes.Handle) (frameworktypes.Plug
 		})
 	}

+	if defaultEvictorArgs.MinReplicas > 1 {
+		podLister := handle.SharedInformerFactory().Core().V1().Pods().Lister()


Any chance to list pods by owner UID? E.g. creating a new cache (when defaultEvictorArgs.MinReplicas > 1) that will get updated based on pod CRUD ops and allow to quickly get a list of pods owned by an owner UID? In huge clusters this might be a bottle neck.

The default evictor could own the cache.

that's a really good suggestion, let me take a stab at it and see how that works

I tried this and the indexer only supports a single key to index by, while ownerReferences is an array.

https://github.com/kubernetes-sigs/descheduler/blob/master/pkg/descheduler/pod/pods.go#L131 can return a list of keys. In case the implementation ends up comparing slice with a slice you can always get a list of all pods owned by first owner, second, third, etc. and make an intersection of all the lists.

What do you think is the performance gain here? Loading too much cached objects in memory?

Both approach are already using the informer cache so it's no extra lookup to the apiserver.

Iterating through too many pods in huge clusters. Though, we can merge the code as it is and improve it once there's a request for it I guess.

It's already namespaced. I could limit it further by adding the following labels:

pod-template-hash, if it exists

controller-revision-hash, if it exists

Thoughts? I'll take another look at indexer again

Right. I missed that namespaced part :) Fine then :)

aslafy-z · 2024-01-10T08:49:50Z

@a7i can you please rebase the PR? 🙏

a7i · 2024-01-10T13:55:27Z

@a7i can you please rebase the PR? 🙏

there hasn't been much traction from other maintainers, but happy to rebase just in case

aslafy-z · 2024-01-10T21:55:01Z

@knelasevero can you please give it a new look? Thank you

knelasevero · 2024-01-11T10:54:40Z

/retest

ingvagabund · 2024-01-12T21:10:22Z

/approve
/hold
feel free to unhold, leaving here in case someone else wants to review.

k8s-ci-robot · 2024-01-12T21:10:49Z

[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

ingvagabund · 2024-01-15T08:19:54Z

/hold cancel

ingvagabund · 2024-01-15T20:12:36Z

pkg/framework/plugins/defaultevictor/types.go

@@ -33,4 +33,5 @@ type DefaultEvictorArgs struct {
 	LabelSelector           *metav1.LabelSelector  `json:"labelSelector"`
 	PriorityThreshold       *api.PriorityThreshold `json:"priorityThreshold"`
 	NodeFit                 bool                   `json:"nodeFit"`
+	MinReplicas             uint                   `json:"minReplicas"`


So 0 and 1 means no effect. Will it ever make sense to interpret minReplicas=0 differently? Owner with 0 replicas means no pods. Owner with a single replica means the current behavior = skip the check. Would it make more sense to define minAdjointReplicas instead? To avoid the case where minReplicas=0. Just thinking aloud.

Please feel free to take over this PR.

At the moment, I have other higher priority items and have low confidence in any movement on this PR (been around since October of last year).

I will put it on my todo list where it will probably stay for some time before I find some free cycle for some more comprehensive though. Thank you for pushing this forward.

0 replicas mean a pod does not exist. Which is never a case. Fine to keep it as default = disabled.

I added a warning message in validator to print that it's a no-op when set to 1

k8s-ci-robot · 2024-03-13T09:10:53Z

PR needs rebase.

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.

pkg/framework/plugins/defaultevictor/defaultevictor.go

pkg/descheduler/pod/pods.go

…hold setting Signed-off-by: Amir Alavi <[email protected]>

ingvagabund · 2024-03-13T13:43:53Z

/lgtm

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 Oct 7, 2023

a7i requested review from knelasevero, ingvagabund and aartij17 and removed request for aartij17 October 7, 2023 18:37

a7i mentioned this pull request Oct 10, 2023

skip eviction when replica count is below evictor threshold setting #1152

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2023

a7i force-pushed the minreplicas-evictor-threshold branch from 903c51b to 18955ad Compare November 12, 2023 16:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2023

kubernetes-sigs deleted a comment from k8s-ci-robot Nov 12, 2023

k8s-ci-robot assigned knelasevero Nov 21, 2023

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

ingvagabund reviewed Nov 21, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023

a7i force-pushed the minreplicas-evictor-threshold branch from 18955ad to 6fdb783 Compare January 10, 2024 14:04

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

a7i force-pushed the minreplicas-evictor-threshold branch from 2aa29aa to 59c33cb Compare January 15, 2024 02:25

a7i requested a review from ingvagabund January 15, 2024 02:32

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

a7i force-pushed the minreplicas-evictor-threshold branch from 59c33cb to 35045a7 Compare January 15, 2024 14:27

ingvagabund reviewed Jan 15, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2024

ingvagabund reviewed Mar 13, 2024

View reviewed changes

pkg/framework/plugins/defaultevictor/defaultevictor.go Outdated Show resolved Hide resolved

a7i force-pushed the minreplicas-evictor-threshold branch from 35045a7 to 3284a8e Compare March 13, 2024 13:20

ingvagabund reviewed Mar 13, 2024

View reviewed changes

pkg/descheduler/pod/pods.go Outdated Show resolved Hide resolved

skip eviction when replica count is below evictor minReplicas thres…

37df42d

…hold setting Signed-off-by: Amir Alavi <[email protected]>

a7i force-pushed the minreplicas-evictor-threshold branch from 3284a8e to 37df42d Compare March 13, 2024 13:29

k8s-ci-robot assigned ingvagabund Mar 13, 2024

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

k8s-ci-robot merged commit 82af9c6 into kubernetes-sigs:master Mar 13, 2024
10 checks passed

a7i deleted the minreplicas-evictor-threshold branch March 13, 2024 14:51

a7i mentioned this pull request May 2, 2024

Something is wrong with Helm Chart v0.29.0... #1383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip eviction when replica count is below evictor `minReplicas` threshold setting #1257

skip eviction when replica count is below evictor `minReplicas` threshold setting #1257

a7i commented Oct 7, 2023 •

edited

Loading

a7i commented Nov 12, 2023

knelasevero commented Nov 20, 2023

knelasevero commented Nov 20, 2023

a7i commented Nov 20, 2023

knelasevero commented Nov 20, 2023

knelasevero commented Nov 21, 2023

ingvagabund Nov 21, 2023 •

edited

Loading

a7i Nov 21, 2023

a7i Nov 28, 2023

ingvagabund Jan 11, 2024 •

edited

Loading

a7i Jan 12, 2024

ingvagabund Jan 12, 2024

a7i Jan 12, 2024

ingvagabund Jan 12, 2024

aslafy-z commented Jan 10, 2024

a7i commented Jan 10, 2024

aslafy-z commented Jan 10, 2024

knelasevero commented Jan 11, 2024

ingvagabund commented Jan 12, 2024

k8s-ci-robot commented Jan 12, 2024

ingvagabund commented Jan 15, 2024

ingvagabund Jan 15, 2024

a7i Jan 22, 2024

ingvagabund Feb 1, 2024

ingvagabund Mar 13, 2024

a7i Mar 13, 2024

k8s-ci-robot commented Mar 13, 2024

ingvagabund commented Mar 13, 2024

skip eviction when replica count is below evictor minReplicas threshold setting #1257

skip eviction when replica count is below evictor minReplicas threshold setting #1257

Conversation

a7i commented Oct 7, 2023 • edited Loading

a7i commented Nov 12, 2023

knelasevero commented Nov 20, 2023

knelasevero commented Nov 20, 2023

a7i commented Nov 20, 2023

knelasevero commented Nov 20, 2023

knelasevero commented Nov 21, 2023

ingvagabund Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund Jan 11, 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

aslafy-z commented Jan 10, 2024

a7i commented Jan 10, 2024

aslafy-z commented Jan 10, 2024

knelasevero commented Jan 11, 2024

ingvagabund commented Jan 12, 2024

k8s-ci-robot commented Jan 12, 2024

ingvagabund commented Jan 15, 2024

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

k8s-ci-robot commented Mar 13, 2024

ingvagabund commented Mar 13, 2024

skip eviction when replica count is below evictor `minReplicas` threshold setting #1257

skip eviction when replica count is below evictor `minReplicas` threshold setting #1257

a7i commented Oct 7, 2023 •

edited

Loading

ingvagabund Nov 21, 2023 •

edited

Loading

ingvagabund Jan 11, 2024 •

edited

Loading