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

googs1025 · 2025-04-11T12:32:59Z

Is your feature request related to a problem? Please describe.

base on: #1603 (comment)

so we create an issue to track. And I will try to discuss the changed config struct in this issue.

Describe the solution you'd like

Describe alternatives you've considered

What version of descheduler are you using?

descheduler version:

Additional context

googs1025 · 2025-04-11T12:33:21Z

/assign

googs1025 · 2025-04-11T12:53:32Z

go struct

P.S.: Maybe we need to keep the old configuration for a while? And mark it as deprecated.

type EvictionProtectionType string

const (
    withPvc           EvictionProtectionType = "withPvc"
    withoutPdb        EvictionProtectionType = "withoutPdb"
    withLocalStorage   EvictionProtectionType = "withLocalStorage"
    withSystemCritical EvictionProtectionType = "withSystemCritical"
    withFailedBarePods EvictionProtectionType = "withFailedBarePods"
)

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

	NodeSelector                 string                 `json:"nodeSelector,omitempty"`
	EvictLocalStoragePods        bool                   `json:"evictLocalStoragePods,omitempty"`
	EvictDaemonSetPods           bool                   `json:"evictDaemonSetPods,omitempty"`
	EvictSystemCriticalPods      bool                   `json:"evictSystemCriticalPods,omitempty"`
	IgnorePvcPods                bool                   `json:"ignorePvcPods,omitempty"`
	EvictFailedBarePods          bool                   `json:"evictFailedBarePods,omitempty"`
	LabelSelector                *metav1.LabelSelector  `json:"labelSelector,omitempty"`
	PriorityThreshold            *api.PriorityThreshold `json:"priorityThreshold,omitempty"`
	NodeFit                      bool                   `json:"nodeFit,omitempty"`
	MinReplicas                  uint                   `json:"minReplicas,omitempty"`
	MinPodAge                    *metav1.Duration       `json:"minPodAge,omitempty"`
	IgnorePodsWithoutPDB         bool                   `json:"ignorePodsWithoutPDB,omitempty"`
	
        EvictionProtection []EvictionProtectionType `json:"evictionProtection,omitempty"`
}

yaml:

defaultEvictor:
  evictionProtection:
    - withPvc           # Protect Pods with PVC
    - withoutPdb        # Protect Pods without PDB
    - withLocalStorage   # Default: Protect Pods using local storage
    - withSystemCritical # Default: Protect system-critical Pods
    - withFailedBarePods # Default: Protect failed bare Pods

googs1025 · 2025-04-13T02:35:20Z

Back here again, if we use this method, we may need to change the default behavior. For example, the default EvictLocalStoragePods was false before, which means that pods with LocalStorage are not evicted by default. If we use evictionProtection: withLocalStorage without setting it, it means that pods with LocalStorage will be evicted by default. If we put withLocalStorage in evictionProtection by default, then the current design means that users cannot cancel the pod with LocalStorage that has been evicted.

googs1025 · 2025-04-13T02:38:11Z

@ingvagabund Any good suggestions? :)

ingvagabund · 2025-04-16T12:12:25Z

You are right. Then what about:

withoutLocalStorage
withoutSystemCritical
withoutFailedBarePods

while keeping evictionProtection empty by default? Mentioning the default eviction policy in README.md.

Maybe we need to keep the old configuration for a while? And mark it as deprecated.

Yes. Also, making sure the new configuration does not collide with the current one. If either of the current fields get used setting evictionProtection will be rejected.

googs1025 · 2025-04-17T07:03:02Z

I thought about it and decided that this might be a better approach. We still distinguish between two types: EvictionActions and EvictionProtections. EvictionActions means that the user should not be evicted, but can be set to be evicted. EvictionProtections means that the user should be evicted, but the user can set the user to be protected (not evicted).
@ingvagabund @ricardomaraschini wdyt? 😄

like this:

// DefaultEvictorArgs holds arguments used to configure DefaultEvictor plugin.
type DefaultEvictorArgs struct {
	metav1.TypeMeta `json:",inline"`

	NodeSelector string `json:"nodeSelector,omitempty"`
	// Deprecated: Use EvictionActions with "withLocalStorage" instead.
	EvictLocalStoragePods bool `json:"evictLocalStoragePods,omitempty"`
	// Deprecated: Use EvictionActions with "withDaemonSet" instead.
	EvictDaemonSetPods bool `json:"evictDaemonSetPods,omitempty"`
	// Deprecated: Use EvictionActions with "withSystemCritical" instead.
	EvictSystemCriticalPods bool `json:"evictSystemCriticalPods,omitempty"`
	// Deprecated: Use EvictionProtections with "withPVC" instead.
	IgnorePvcPods bool `json:"ignorePvcPods,omitempty"`
	// Deprecated: Use EvictionActions with "withFailedBarePods" instead.
	EvictFailedBarePods bool                   `json:"evictFailedBarePods,omitempty"`
	LabelSelector       *metav1.LabelSelector  `json:"labelSelector,omitempty"`
	PriorityThreshold   *api.PriorityThreshold `json:"priorityThreshold,omitempty"`
	NodeFit             bool                   `json:"nodeFit,omitempty"`
	MinReplicas         uint                   `json:"minReplicas,omitempty"`
	MinPodAge           *metav1.Duration       `json:"minPodAge,omitempty"`
	// Deprecated: Use EvictionProtection with "withoutPDB" instead.
	IgnorePodsWithoutPDB bool `json:"ignorePodsWithoutPDB,omitempty"`

	// EvictionActions specifies actions to take during eviction. 
	// These represent types of Pods that should not be evicted by default,
	// but users can explicitly specify them to be evicted.	
	// Supported values:
	// - "withLocalStorage": Evict pods with local storage.
	// - "daemonSetPods": Evict pods managed by DaemonSets.
	// - "systemCriticalPods": Evict system-critical pods.
	// - "failedBarePods": Evict failed bare pods (uncontrolled pods).
	EvictionActions []EvictionActionType `json:"evictionAction,omitempty"`
	// EvictionProtections specifies protections to apply during eviction. 
	// These represent types of Pods that should be evicted by default,
	// but users can explicitly specify them to be protected from eviction.
	// Supported values:
	// - "withPVC": Protect pods with PersistentVolumeClaims (PVCs).
	// - "withoutPDB": Protect pods not covered by a PodDisruptionBudget (PDB).
	EvictionProtections []EvictionProtectionType `json:"evictionProtection,omitempty"`
}

type EvictionActionType string

const (

	// WithLocalStorage indicates that pods with local storage should be evicted.
	WithLocalStorage EvictionActionType = "withLocalStorage"

	// DaemonSetPods indicates that pods managed by DaemonSets should be evicted.
	DaemonSetPods EvictionActionType = "daemonSetPods"

	// SystemCriticalPods indicates that system-critical pods should be evicted.
	SystemCriticalPods EvictionActionType = "systemCriticalPods"

	// FailedBarePods indicates that failed bare pods (uncontrolled pods) should be evicted.
	FailedBarePods EvictionActionType = "failedBarePods"
)

type EvictionProtectionType string

const (
	// WithPVC indicates that pods with PersistentVolumeClaims (PVCs) should be protected from eviction.
	WithPVC EvictionProtectionType = "withPVC"
	// WithoutPDB indicates that pods not covered by a PodDisruptionBudget (PDB) should be protected from eviction.
	WithoutPDB EvictionProtectionType = "withoutPDB"
)

ricardomaraschini · 2025-04-18T10:42:48Z

Please let me know if I missed/misunderstood any aspect here that being said:

These are the configurations relevant to this issue:

Name	Description
evictLocalStoragePods	Allows eviction of pods with local storage
evictSystemCriticalPods	Allows eviction of pods with any priority, including system pods like kube-dns
ignorePvcPods	Sets whether PVC pods should be allowed to be evicted
evictFailedBarePods	Allows eviction of pods without owner references and in failed phase
ignorePodsWithoutPDB	Sets whether pods without PodDisruptionBudget should be evicted or ignored
evictDaemonSetPods	Sets if pods part of DaemonSets can be evicted

In essence they all seem to be designed to "disable" default protections, the default value for all of them is false.
Reading the above in a different way we can understand them as.

Name	Description	Default Action
Local Storage Pods	Pods with local storage	Not evicted
System CriticalPods	Pods classified as System Critical	Not evicted
Pods With PVC	Pods with PVC mounts	Not evicted
Failed Bare Pods	Pods without owner references and in failed phase	Not evicted
Pods With PDB	Pods with PodDisruptionBudget	Not evicted
DaemonSet	Pods owned by a DaemonSet	Not evicted

We can "disable the protections" by setting any of them to true. Can't this all be converted into a single configuration like so ?

disabledProtections:
  - LocalStorage
  - SystemCritical
  - PersistentVolumeClaim
  - FailedBare
  - PodDisruptionBudget
  - DaemonSet

Something like this:

// DefaultEvictorArgs holds arguments used to configure DefaultEvictor plugin.
type DefaultEvictorArgs struct {
        EvictLocalStoragePods   bool // replaced by disabledProtections["LocalStorage"]
        EvictDaemonSetPods      bool // replaced by disabledProtections["DaemonSet"]
        EvictSystemCriticalPods bool // replaced by disabledProtections["SystemCritical"]
        IgnorePvcPods           bool // replaced by disabledProtections["PersistentVolumeClaim"]
        EvictFailedBarePods     bool // replaced by disabledProtections["FailedBare"]
        IgnorePodsWithoutPDB    bool // replaced by disabledProtections["PodDisruptionBudget"]

        // Here protections can be disabled.
        DisabledProtections []Protections `json:"disabledProtections,omitempty"`
}

// Protection is a default pod eviction protection.
type Protection string

const (
        LocalStorage          Protection = "LocalStorage"
        SystemCritical        Protection = "SystemCritical"
        PersistentVolumeClaim Protection = "PersistentVolumeClaim"
        FailedBare            Protection = "FailedBare"
        PodDisruptionBudget   Protection = "PodDisruptionBudget"
        DaemonSet             Protection = "DaemonSet"
)

Again, I may be over simplifying this, please let me know what I am not seeing.

Edited to add the go sample code and add the missing DaemonSet protection

ricardomaraschini · 2025-04-18T10:47:28Z

I think that what I am trying to say is that the with/without dichotomy is hard to understand.

googs1025 · 2025-04-18T15:58:18Z

I think that what I am trying to say is that the with/without dichotomy is hard to understand.

thanks @ricardomaraschini 😄
Yes, this is probably related to the original design (I think?). For example, the original config "EvictXXX" and "IgnoreXXX" made it difficult for me to refactor the config structure because we want to be compatible with the previous. base on: #1603 (comment)

	// Deprecated: Use EvictionActions with "withLocalStorage" instead.
	EvictLocalStoragePods bool `json:"evictLocalStoragePods,omitempty"`
	// Deprecated: Use EvictionActions with "withDaemonSet" instead.
	EvictDaemonSetPods bool `json:"evictDaemonSetPods,omitempty"`
	// Deprecated: Use EvictionActions with "withSystemCritical" instead.
	EvictSystemCriticalPods bool `json:"evictSystemCriticalPods,omitempty"`
	// Deprecated: Use EvictionProtections with "withPVC" instead.
	IgnorePvcPods bool `json:"ignorePvcPods,omitempty"`
	// Deprecated: Use EvictionActions with "withFailedBarePods" instead.
	EvictFailedBarePods bool                   `json:"evictFailedBarePods,omitempty"`
	// Deprecated: Use EvictionProtection with "withoutPDB" instead.
	IgnorePodsWithoutPDB bool `json:"ignorePodsWithoutPDB,omitempty"`

Because of this, I think a better way is to distinguish between EvictionActionType and EvictionProtectionType.

ricardomaraschini · 2025-04-21T08:26:24Z

Did you have the chance to scrutinize my proposed solution here ? What do you think ?

googs1025 · 2025-04-21T09:00:01Z

thanks @ricardomaraschini 😄
Let me imagine a scenario:
If evictLocalStoragePods is set to false, the behavior is that disabledProtections does not include LocalStorage, right?

If ignorePvcPods is false by default, it means that pods with PVCs will not be ignored (because false means that pods with PVCs will be evicted). This behavior where disabledProtections includes PersistentVolumeClaim. so to maintain compatibility with previous logic, we need to inject PersistentVolumeClaim into the disabledProtections list by default. However, if PersistentVolumeClaim is injected into the disabledProtections list by default, how can users remove it from the configuration file?

ricardomaraschini · 2025-04-21T10:16:17Z

If ignorePvcPods is false by default, it means that pods with PVCs will not be ignored (because false means that pods with PVCs will be evicted).

You are absolutely right. My initial understanding was that pods with PVC were protected by default. This defeats my simplified view and now I see where you were going with your separation of concerns. So what we want is a way of:

Disable one or more of the default protections.
- Pods with local storage.
- System critical pods.
- Failed bare pods.
- Pods owned by DaemonSets.
Add one or more extra protections.
- Pods with PVC.
- Pods without PodDisruptionBudget

This is basicaly what you wrote with a proposed slightly different naming:

// PodProtection is a name of a policy that protects pods against eviction.
type PodProtection string

// This is a list of all supported protections, some are enabled by default
// some may be enabled by the user through the configuration.
const (
        PodsWithLocalStorage PodProtection = "PodsWithLocalStorage"
        SystemCriticalPods   PodProtection = "SystemCriticalPods"
        FailedBarePods       PodProtection = "FailedBarePods"
        DaemonSetPods        PodProtection = "DaemonSetPods"
        PodsWithoutPDB       PodProtection = "PodsWithoutPDB"
        PodsWithPVC          PodProtection = "PodsWithPVC"
)

// DefaultPodProtections holds the list of the protections that are enabled by
// default. User can use disabledDefaultPodProtections evictor arguments to
// disable these.
var DefaultPodProtections = []PodProtection{
        PodsWithLocalStorage,
        SystemCriticalPods,
        FailedBarePods,
        DaemonSetPods,
}

// ExtraPodProtections holds a list of the protections the user can enable
// through the configuration. See extraPodProtections below. These are
// not enabled by default.
var ExtraPodProtections = []PodProtection{
        PodsWithPVC,
        PodsWithoutPDB,
}

// DefaultEvictorArgs holds arguments used to configure DefaultEvictor plugin.
type DefaultEvictorArgs struct {
        // redacted.

        // DisabledDefaultPodProtections is a list of default pod protections
        // the user wants to disable.
        DisabledDefaultPodProtections []PodProtection `json:"disabledDefaultPodProtections,omitempty"`

        // ExtraPodProtections is a list of extra pod protection the user can
        // enable.
        ExtraPodProtections []PodProtection `json:"extraPodProtections,omitempty"`
}

Am I still missing something here ? What do you think about this naming ?

googs1025 · 2025-04-22T01:22:22Z

You are absolutely right. My initial understanding was that pods with PVC were protected by default. This defeats my simplified view and now I see where you were going with your separation of concerns.

I originally wanted to simplify the config, but there were too many parameters before, which made it confusing and complicated to be compatible. 🤔

I think this name is more appropriate, thank you. @ricardomaraschini 😄

For example:
Let's imagine a few cases to confirm whether we have reached a consensus.

For example: The config is as follows.

Empty config: By default, PodsWithLocalStorage SystemCriticalPods FailedBarePods DaemonSetPods will not be evicted, and PodsWithPVC PodsWithoutPDB will be evicted. This is consistent with the original logic.

defaultEvictor:
  disabledDefaultPodProtections:
  extraPodProtections:

config 1: If PodsWithLocalStorage FailedBarePods is set in disabledDefaultPodProtections, PodsWithLocalStorage and FailedBarePods will be evicted. If PodsWithPVC PodsWithoutPDB is set in extraPodProtections, PodsWithPVC PodsWithoutPDB will be protected from being evicted.

defaultEvictor:
  disabledDefaultPodProtections:
    - PodsWithLocalStorage
    - FailedBarePods
  extraPodProtections:
    - PodsWithPVC
    - PodsWithoutPDB

config 2: Setting PodsWithLocalStorage, FailedBarePods, SystemCriticalPods, and DaemonSetPods in disabledDefaultPodProtections means that we will evict these pods that are protected by default. Setting PodsWithPVC, PodsWithoutPDB in extraPodProtections will protect these two pods by default.

defaultEvictor:
  disabledDefaultPodProtections:
    - PodsWithLocalStorage
    - FailedBarePods
    - SystemCriticalPods
    - DaemonSetPods
  extraPodProtections:
    - PodsWithPVC
    - PodsWithoutPDB

Note: If any list in disabledDefaultPodProtections or extraPodProtections has a value, for example (PodsWithLocalStorage FailedBarePods is set in disabledDefaultPodProtection or PodsWithoutPDB is set in extraPodProtections) , and any of the parameters originally planned to be deprecated are true, an error will be returned.

We hope that users will either use obsolete parameters or new parameters, and not mix them. This will be relatively safe for our subsequent parameter obsolete configuration. cc @ingvagabund

googs1025 · 2025-04-22T13:28:56Z

I think it is better to distinguish these two as two types, which is more clear in meaning.
like this:

type EvictionActionType string

const (
	// WithLocalStorage indicates that pods with local storage should be evicted.
	WithLocalStorage EvictionActionType = "withLocalStorage"
	// DaemonSetPods indicates that pods managed by DaemonSets should be evicted.
	DaemonSetPods EvictionActionType = "daemonSetPods"
	// SystemCriticalPods indicates that system-critical pods should be evicted.
	SystemCriticalPods EvictionActionType = "systemCriticalPods"
	// FailedBarePods indicates that failed bare pods (uncontrolled pods) should be evicted.
	FailedBarePods EvictionActionType = "failedBarePods"
)

type EvictionProtectionType string

const (
	// WithPVC indicates that pods with PersistentVolumeClaims (PVCs) should be protected from eviction.
	WithPVC EvictionProtectionType = "withPVC"
	// WithoutPDB indicates that pods not covered by a PodDisruptionBudget (PDB) should be protected from eviction.
	WithoutPDB EvictionProtectionType = "withoutPDB"
)

googs1025 · 2025-04-22T13:38:30Z

like this:

// DefaultEvictorArgs holds arguments used to configure DefaultEvictor plugin.
type DefaultEvictorArgs struct {
	metav1.TypeMeta `json:",inline"`

	NodeSelector string `json:"nodeSelector,omitempty"`
	// Deprecated: Use EvictionActions with "withLocalStorage" instead.
	EvictLocalStoragePods bool `json:"evictLocalStoragePods,omitempty"`
	// Deprecated: Use EvictionActions with "withDaemonSet" instead.
	EvictDaemonSetPods bool `json:"evictDaemonSetPods,omitempty"`
	// Deprecated: Use EvictionActions with "withSystemCritical" instead.
	EvictSystemCriticalPods bool `json:"evictSystemCriticalPods,omitempty"`
	// Deprecated: Use EvictionProtections with "withPVC" instead.
	IgnorePvcPods bool `json:"ignorePvcPods,omitempty"`
	// Deprecated: Use EvictionActions with "withFailedBarePods" instead.
	EvictFailedBarePods bool                   `json:"evictFailedBarePods,omitempty"`
	LabelSelector       *metav1.LabelSelector  `json:"labelSelector,omitempty"`
	PriorityThreshold   *api.PriorityThreshold `json:"priorityThreshold,omitempty"`
	NodeFit             bool                   `json:"nodeFit,omitempty"`
	MinReplicas         uint                   `json:"minReplicas,omitempty"`
	MinPodAge           *metav1.Duration       `json:"minPodAge,omitempty"`
	// Deprecated: Use EvictionProtection with "withoutPDB" instead.
	IgnorePodsWithoutPDB bool `json:"ignorePodsWithoutPDB,omitempty"`

	// 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"`

	// ExtraPodProtections is a list that specifies additional Pod protection mechanisms that users can enable.
	// These protection rules are optional and allow users to extend the protection scope based on their needs.
	// Users can selectively enable certain extra protection rules via this field.
	ExtraPodProtections []ExtraPodProtection `json:"extraPodProtections,omitempty"`
}

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

const (
	// WithLocalStorage indicates disabling protection for Pods using local storage.
	// Once disabled, Pods using local storage may be deleted or interfered with.
	WithLocalStorage DisabledDefaultPodProtection = "withLocalStorage"

	// DaemonSetPods indicates disabling protection for Pods managed by DaemonSet.
	// Once disabled, Pods created by DaemonSet may be deleted or interfered with.
	DaemonSetPods DisabledDefaultPodProtection = "daemonSetPods"

	// SystemCriticalPods indicates disabling protection for system-critical Pods.
	// Once disabled, system-critical Pods may be deleted or interfered with.
	SystemCriticalPods DisabledDefaultPodProtection = "systemCriticalPods"

	// FailedBarePods indicates disabling protection for failed bare Pods (Pods not bound to ReplicaSet or Deployment).
	// Once disabled, failed bare Pods may be deleted or interfered with.
	FailedBarePods DisabledDefaultPodProtection = "failedBarePods"
)

// ExtraPodProtection defines the type for additional Pod protection mechanisms that users can enable.
// Each value represents an extra protection rule that can be enabled.
type ExtraPodProtection string

const (
	// WithPVC indicates enabling protection for Pods using Persistent Volume Claims (PVC).
	// Once enabled, Pods using PVCs will be protected from deletion or interference.
	WithPVC ExtraPodProtection = "withPVC"

	// WithoutPDB indicates enabling protection for Pods without a Pod Disruption Budget (PDB).
	// Once enabled, Pods without a configured PDB will be protected from deletion or interference.
	WithoutPDB ExtraPodProtection = "withoutPDB"
)

tiraboschi · 2025-04-24T22:11:57Z

See also: #1673

I think we should also consider the case where descheduler.alpha.kubernetes.io/evict=false can be used to produce the opposite effect of descheduler.alpha.kubernetes.io/evict=true:
descheduler.alpha.kubernetes.io/evict=true annotation is currently marking/forcing a pod as a valid candidate for eviction bypassing all the other checks.
I think that, for symmetry, descheduler.alpha.kubernetes.io/evict=false should force a pod to not be considered as a candidate for eviction even if all the other checks are satisfied.

ricardomaraschini · 2025-05-01T07:46:26Z

@googs1025 I failed to see how have the same thing divided into two types bring us any clarity. Isn't it easier to reason about these as Protections ? Some are enabled by default while others may be enabled on demand.

ricardomaraschini · 2025-05-01T08:00:46Z

See also: #1673

I think we should also consider the case where descheduler.alpha.kubernetes.io/evict=false can be used to produce the opposite effect of descheduler.alpha.kubernetes.io/evict=true: descheduler.alpha.kubernetes.io/evict=true annotation is currently marking/forcing a pod as a valid candidate for eviction bypassing all the other checks. I think that, for symmetry, descheduler.alpha.kubernetes.io/evict=false should force a pod to not be considered as a candidate for eviction even if all the other checks are satisfied.

I understand. How do you see the annotation relationship with the protections discussed here ? The former seems to be a "hardcoded" thing that overwrites everything on the latter, no ?

googs1025 · 2025-05-01T13:24:39Z

I failed to see how have the same thing divided into two types bring us any clarity. Isn't it easier to reason about these as Protections ? Some are enabled by default while others may be enabled on demand.

@ricardomaraschini
In this design, the two terms are more distinct in terms of meaning. In terms of user usage, there is actually no distinction.

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

// ExtraPodProtection defines the type for additional Pod protection mechanisms that users can enable.
// Each value represents an extra protection rule that can be enabled.
type ExtraPodProtection string

Users only need to use them as follows.

For example: The config is as follows.

Empty config: By default, PodsWithLocalStorage SystemCriticalPods FailedBarePods DaemonSetPods will not be evicted, and PodsWithPVC PodsWithoutPDB will be evicted. This is consistent with the original logic.

defaultEvictor:
disabledDefaultPodProtections:
extraPodProtections:

config 1: If PodsWithLocalStorage FailedBarePods is set in disabledDefaultPodProtections, PodsWithLocalStorage and FailedBarePods will be evicted. If PodsWithPVC PodsWithoutPDB is set in extraPodProtections, PodsWithPVC PodsWithoutPDB will be protected from being evicted.

defaultEvictor:
disabledDefaultPodProtections:
- PodsWithLocalStorage
- FailedBarePods
extraPodProtections:
- PodsWithPVC
- PodsWithoutPDB

config 2: Setting PodsWithLocalStorage, FailedBarePods, SystemCriticalPods, and DaemonSetPods in disabledDefaultPodProtections means that we will evict these pods that are protected by default. Setting PodsWithPVC, PodsWithoutPDB in extraPodProtections will protect these two pods by default.

defaultEvictor:
disabledDefaultPodProtections:
- PodsWithLocalStorage
- FailedBarePods
- SystemCriticalPods
- DaemonSetPods
extraPodProtections:
- PodsWithPVC
- PodsWithoutPDB

googs1025 · 2025-05-01T13:35:25Z

Because the options of PodsWithLocalStorage FailedBarePods SystemCriticalPods DaemonSetPods are: "Originally cannot be evicted, but open to users to choose". PodsWithPVC PodsWithoutPDB are "Originally can be evicted, but users are given the choice of protection". The two are semantically different. We should distinguish them in the code. This will also make subsequent maintenance more convenient. It will not cause other developers to add the logic that is not originally evicted (such as: FailedBarePods) to the logic that is originally capable of being evicted.

ricardomaraschini · 2025-05-27T12:32:23Z

@googs1025 I still think that what we have here is a list of protections that are classified into two groups: defaults that can be disabled and extras that can be enabled.

If you look at my example here you will see that the classification of the protections are made through two variables: "DefaultPodProtections" and "ExtraPodProtections".

I don't see the need to have a different type on our code. That being said I don't want to be the one holding this back. Are you planning to implement this ? Let me know what your plans are and I can either implement or review your implementation.

googs1025 · 2025-05-27T12:44:29Z

@ricardomaraschini
Thank you for your response! Your suggestion is great and I already use it.
Just not updated in this issue

// DefaultEvictorArgs holds arguments used to configure DefaultEvictor plugin.
type DefaultEvictorArgs struct {
	metav1.TypeMeta `json:",inline"`

	NodeSelector string `json:"nodeSelector,omitempty"`
	// Deprecated: Use DisabledDefaultPodProtection with "withLocalStorage" instead.
	EvictLocalStoragePods bool `json:"evictLocalStoragePods,omitempty"`
	// Deprecated: Use DisabledDefaultPodProtection with "daemonSetPods" instead.
	EvictDaemonSetPods bool `json:"evictDaemonSetPods,omitempty"`
	// Deprecated: Use DisabledDefaultPodProtection with "systemCriticalPods" instead.
	EvictSystemCriticalPods bool `json:"evictSystemCriticalPods,omitempty"`
	// Deprecated: Use ExtraPodProtection with "withPVC" instead.
	IgnorePvcPods bool `json:"ignorePvcPods,omitempty"`
	// Deprecated: Use DisabledDefaultPodProtection with "failedBarePods" instead.
	EvictFailedBarePods bool                   `json:"evictFailedBarePods,omitempty"`
	LabelSelector       *metav1.LabelSelector  `json:"labelSelector,omitempty"`
	PriorityThreshold   *api.PriorityThreshold `json:"priorityThreshold,omitempty"`
	NodeFit             bool                   `json:"nodeFit,omitempty"`
	MinReplicas         uint                   `json:"minReplicas,omitempty"`
	MinPodAge           *metav1.Duration       `json:"minPodAge,omitempty"`
	// Deprecated: Use ExtraPodProtection with "withoutPDB" instead.
	IgnorePodsWithoutPDB bool `json:"ignorePodsWithoutPDB,omitempty"`

	// PodProtectionPolicies holds the list of enabled and disabled protection policies.
	// Users can selectively disable certain default protection rules or enable extra ones.
	PodProtectionPolicies PodProtections `json:"protectionPolicies,omitempty"`

	// defaultPodProtectionPolicies is a private field that contains the list of PodProtectionPolicy values
	// that are enabled by default. These policies represent the built-in, default protections applied to
	// certain types of Pods, such as DaemonSet pods or SystemCritical pods. Users can selectively disable
	// any of these using the "disabled" list in PodProtectionPolicies.
	// The following four policies are included by default:
	//   - PodsWithLocalStorage: Protects pods with local storage.
	//   - DaemonSetPods: Protects DaemonSet managed pods.
	//   - SystemCriticalPods: Protects system-critical pods.
	//   - FailedBarePods: Protects failed bare pods (not part of any controller).
	defaultPodProtectionPolicies []PodProtectionPolicy
}

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

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

// PodProtections holds the list of enabled and disabled protection policies.
// NOTE: The list of default enabled pod protection policies is subject to change in future versions.
// +k8s:deepcopy-gen=true
type PodProtections struct {
	// ExtraEnabled specifies additional protection policies that should be enabled.
	// Supports: podsWithPVC, podsWithoutPDB
	ExtraEnabled []PodProtectionPolicy `json:"extraEnabled,omitempty"`

	// Disabled specifies which default protection policies should be disabled.
	// Supports: podsWithLocalStorage, daemonSetPods, systemCriticalPods, failedBarePods
	Disabled []PodProtectionPolicy `json:"disabled,omitempty"`
}

googs1025 · 2025-05-27T12:49:02Z

if you have time please also help this: #1665 😄

googs1025 added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 11, 2025

k8s-ci-robot assigned googs1025 Apr 11, 2025

googs1025 mentioned this issue Apr 11, 2025

feature: add IgnorePodsWithResourceClaims parameter for configuration #1603

Open

googs1025 linked a pull request Apr 12, 2025 that will close this issue

add PodProtections for DefaultEvictorArgs #1665

Open

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

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

Comments

googs1025 commented Apr 11, 2025

googs1025 commented Apr 11, 2025

Uh oh!

googs1025 commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googs1025 commented Apr 13, 2025

Uh oh!

googs1025 commented Apr 13, 2025

Uh oh!

ingvagabund commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googs1025 commented Apr 17, 2025

Uh oh!

ricardomaraschini commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardomaraschini commented Apr 18, 2025

Uh oh!

googs1025 commented Apr 18, 2025

Uh oh!

ricardomaraschini commented Apr 21, 2025

Uh oh!

googs1025 commented Apr 21, 2025

Uh oh!

ricardomaraschini commented Apr 21, 2025

Uh oh!

googs1025 commented Apr 22, 2025

Uh oh!

googs1025 commented Apr 22, 2025

Uh oh!

googs1025 commented Apr 22, 2025

Uh oh!

tiraboschi commented Apr 24, 2025

Uh oh!

ricardomaraschini commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricardomaraschini commented May 1, 2025

Uh oh!

googs1025 commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googs1025 commented May 1, 2025

Uh oh!

ricardomaraschini commented May 27, 2025

Uh oh!

googs1025 commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googs1025 commented May 27, 2025

Uh oh!

googs1025 commented Apr 11, 2025 •

edited

Loading

ingvagabund commented Apr 16, 2025 •

edited

Loading

ricardomaraschini commented Apr 18, 2025 •

edited

Loading

ricardomaraschini commented May 1, 2025 •

edited

Loading

googs1025 commented May 1, 2025 •

edited

Loading

googs1025 commented May 27, 2025 •

edited

Loading