add PodProtections for DefaultEvictorArgs #1665

googs1025 · 2025-04-12T13:21:03Z

k8s-ci-robot · 2025-04-12T13:21:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign a7i for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

ricardomaraschini · 2025-04-14T14:22:20Z

pkg/framework/plugins/defaultevictor/defaults.go

@@ -49,6 +49,9 @@ func SetDefaults_DefaultEvictorArgs(obj runtime.Object) {
 	if args.PriorityThreshold == nil {
 		args.PriorityThreshold = nil
 	}
+	if args.EvictionProtection == nil {
+		args.EvictionProtection = nil
+	}


What is the purpose of this function ?

oh... This is also where I am confused. I just added it in the same way as before...

Unfortunate product of duplicating the no-op constructs.

README.md

googs1025 · 2025-04-23T11:24:54Z

README.md

        nodeFit: true
        minReplicas: 2
+        # List of disabled default pod protections. This allows users to selectively disable certain protection rules.


We will recommend users to use the new config here

googs1025 · 2025-04-23T11:30:49Z

pkg/framework/plugins/defaultevictor/validation.go

 	}

 	if args.MinReplicas == 1 {
 		klog.V(4).Info("DefaultEvictor minReplicas must be greater than 1 to check for min pods during eviction. This check will be ignored during eviction.")
 	}

+	// check if any deprecated fields are set to true


validate check here: We do not want users to mix old and new config. If any of the old config is true and there is a value in the new config list (including: DisabledDefaultPodProtections and ExtraPodProtections), it means that they are mixed and the validation fails.

googs1025 · 2025-04-23T14:27:44Z

pkg/framework/plugins/defaultevictor/constraints.go

+	frameworktypes "sigs.k8s.io/descheduler/pkg/framework/types"
+	"sigs.k8s.io/descheduler/pkg/utils"
+)
+


I think add new file constraints.go to be more clear

googs1025 · 2025-05-13T05:20:19Z

I hope we can reach an agreement and merge it into the master before the next release. 😄
@ingvagabund @a7i @ricardomaraschini Can you help with this PR? I know this may take some time....

ingvagabund · 2025-05-15T16:25:08Z

pkg/framework/plugins/defaultevictor/types.go

+
+	// DisabledDefaultPodProtections is a list that specifies which default Pod protection mechanisms are disabled.
+	// Users can selectively disable certain default protection rules via this field.
+	DisabledDefaultPodProtections []DisabledDefaultPodProtection `json:"disabledDefaultPodProtections,omitempty"`


What about to put the list of disabled and extra enabled policies under a single field? E.g.

# Pod anti-eviction/protection policies. # The list of policies enabled by default: # - protect pods with local storage, extend disabled with 'podsWithLocalStorage' to disable the protection # - protect daemon set pods, extend disabled with `daemonSetPods` to disable the protection # - ... protectionPolicies: # list of extra enabled policies extraEnabled: # list of disabled pod policies disabled:

?

Here is a good suggestion
we can config like this:

- name: "DefaultEvictor" args: # Deprecated: Use `disabledDefaultPodProtections` with "systemCriticalPods" instead. # evictSystemCriticalPods: true # Deprecated: Use `disabledDefaultPodProtections` with "failedBarePods" instead. # evictFailedBarePods: true # Deprecated: Use `disabledDefaultPodProtections` with "withLocalStorage" instead. # evictLocalStoragePods: true nodeFit: true minReplicas: 2 protectionPolicies: extraEnabled: - withPVC - withoutPDB # list of disabled pod policies disabled: - withLocalStorage - systemCriticalPods - failedBarePods - daemonSetPods

ingvagabund · 2025-05-15T16:39:38Z

pkg/framework/plugins/defaultevictor/types.go

+
+// DisabledDefaultPodProtection defines the type for default Pod protection mechanisms.
+// Each value represents a specific protection rule that can be disabled.
+type DisabledDefaultPodProtection string


Both DisabledDefaultPodProtection and ExtraPodProtection can be consolidated into a single type. E.g.

type PodProtectionPolicy string const ( PodsWithLocalStorage PodProtectionPolicy = "podsWithLocalStorage" DaemonSetPods PodProtectionPolicy = "daemonSetPods" SystemCriticalPods PodProtectionPolicy = "systemCriticalPods" FailedBarePods PodProtectionPolicy = "failedBarePods" PodsWithPVC PodProtectionPolicy = "podsWithPVC" PodsWithoutPDB PodProtectionPolicy = "podsWithoutPDB" )

With some of the policies enabled by default. Other used for extra enabling or disabling. Also, would you please have defaults.go set the default list of enabled policies too? E.g.

args.defaultPodProtectionPolicies = [PodsWithLocalStorage, DaemonSetPods, ...]

while extending the new private defaultPodProtectionPolicies field with a comment saying the list of default policies is a subject to change?

#1664 (comment)
@ingvagabund
We have discussed why not to use the default value here

I thinks this(set default value) will make the behavior incompatible with previous. I think using two types will not have any impact for user. It will also avoid coding errors of different strategies in the delegate (because we distinguish enable and disable as different types).

defaultPodProtectionPolicies is going to be a private variable only, not exposed to customers. Only for internal use.

googs1025 · 2025-05-17T03:45:36Z

pkg/framework/plugins/defaultevictor/types.go

+}
+
+// PodProtectionPolicy defines the protection policy for a pod.
+type PodProtectionPolicy string


Change to one type and use ExtraEnabled Disabled to decide

googs1025 · 2025-05-17T03:46:51Z

pkg/framework/plugins/defaultevictor/defaultevictor.go

-			if uint(len(objs)) < defaultEvictorArgs.MinReplicas {
-				return fmt.Errorf("owner has %d replicas which is less than minReplicas of %d", len(objs), defaultEvictorArgs.MinReplicas)
-			}
+	if !useNewConfig {


We are here to maintain compatibility with the old config logic.

ingvagabund

To make the review and refactoring easier to follow would you please move the changes in constaints.go into a separate PR? Without introducing the pod protection policy. Just to refactor the code is it's easier to introduce the pod protection policies later.

pkg/framework/plugins/defaultevictor/defaultevictor.go

ingvagabund · 2025-05-18T13:19:53Z

pkg/framework/plugins/defaultevictor/defaultevictor.go

+// It supports both new-style (PodProtectionPolicies) and legacy-style flags.
+func getEffectiveProtectedPolicies(args *DefaultEvictorArgs) []PodProtectionPolicy {
+	if args.defaultPodProtectionPolicies == nil {
+		args.defaultPodProtectionPolicies = BuiltInPodProtectionPolicies


defaultPodProtectionPolicies is used only for internal purposes so the same list of default policies does not have to be duplicated. If you plan to define BuiltInPodProtectionPolicies as well the new variable can be used instead of the internal defaultPodProtectionPolicies field.

removed BuiltInPodProtectionPolicies
I think we just use defaultPodProtectionPolicies is enough 🤔

pkg/framework/plugins/defaultevictor/defaultevictor.go

pkg/framework/plugins/defaultevictor/constraints.go

googs1025 · 2025-05-19T15:04:47Z

README.md

-| `ignorePodsWithoutPDB`    |`bool`|`false`| set whether pods without PodDisruptionBudget should be evicted or ignored                                                   |
-
-### Example policy
+| Name                      | Type                     | Default Value | Description                                                                                                                                                                                                           |


see https://github.com/googs1025/descheduler/tree/refator/evict_arg?tab=readme-ov-file#evictor-plugin-configuration-default-evictor

This can see more clearer.

ingvagabund · 2025-05-23T19:33:23Z

Apologies for the delays. I will not be able to finish the review. I am leaving for a vacation until June 8. I will resume once I am back.

Signed-off-by: googs1025 <[email protected]>

k8s-ci-robot requested review from ingvagabund and seanmalloy April 12, 2025 13:21

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 Apr 12, 2025

googs1025 force-pushed the refator/evict_arg branch from a6656de to 837a434 Compare April 12, 2025 14:12

googs1025 marked this pull request as draft April 13, 2025 02:20

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 13, 2025

ricardomaraschini reviewed Apr 14, 2025

View reviewed changes

googs1025 force-pushed the refator/evict_arg branch 3 times, most recently from fa6f9f0 to 93b0f88 Compare April 23, 2025 02:00

googs1025 commented Apr 23, 2025

View reviewed changes

README.md Show resolved Hide resolved

googs1025 commented Apr 23, 2025

View reviewed changes

googs1025 force-pushed the refator/evict_arg branch from 93b0f88 to 6813080 Compare April 23, 2025 11:26

googs1025 commented Apr 23, 2025

View reviewed changes

googs1025 force-pushed the refator/evict_arg branch from 6813080 to cc5857d Compare April 23, 2025 11:58

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 23, 2025

googs1025 marked this pull request as ready for review April 23, 2025 12:05

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 23, 2025

k8s-ci-robot requested review from JaneLiuL and ricardomaraschini April 23, 2025 12:05

googs1025 force-pushed the refator/evict_arg branch from cc5857d to baf046e Compare April 23, 2025 12:14

googs1025 marked this pull request as draft April 23, 2025 12:41

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 23, 2025

googs1025 force-pushed the refator/evict_arg branch from baf046e to 11a8caf Compare April 23, 2025 13:01

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 23, 2025

googs1025 commented Apr 23, 2025

View reviewed changes

ingvagabund reviewed May 15, 2025

View reviewed changes

googs1025 force-pushed the refator/evict_arg branch 6 times, most recently from e4d12bc to d3ce730 Compare May 17, 2025 03:44

googs1025 commented May 17, 2025

View reviewed changes

googs1025 force-pushed the refator/evict_arg branch from d3ce730 to 478a67e Compare May 17, 2025 03:47

ingvagabund reviewed May 18, 2025

View reviewed changes

googs1025 force-pushed the refator/evict_arg branch from 478a67e to 0b33128 Compare May 19, 2025 00:44

googs1025 mentioned this pull request May 19, 2025

refactor: separate eviction constraints to constraints.go #1693

Merged

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

googs1025 force-pushed the refator/evict_arg branch from 0b33128 to cfd5c26 Compare May 19, 2025 12:48

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 19, 2025

googs1025 force-pushed the refator/evict_arg branch 4 times, most recently from 12f11a2 to bd0d074 Compare May 19, 2025 15:02

googs1025 commented May 19, 2025

View reviewed changes

googs1025 requested a review from ingvagabund May 19, 2025 15:10

googs1025 mentioned this pull request May 27, 2025

follow-up feature: refactor DefaultEvictorArgs for more clear config #1664

Open

add EvictionProtection filed for DefaultEvictorArgs

09c9970

Signed-off-by: googs1025 <[email protected]>

googs1025 force-pushed the refator/evict_arg branch from bd0d074 to 09c9970 Compare May 27, 2025 12:51

add PodProtections for DefaultEvictorArgs #1665

Are you sure you want to change the base?

add PodProtections for DefaultEvictorArgs #1665

Uh oh!

Conversation

googs1025 commented Apr 12, 2025

Uh oh!

k8s-ci-robot commented Apr 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googs1025 commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingvagabund left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingvagabund commented May 23, 2025

Uh oh!

Uh oh!