descheduler
descheduler copied to clipboard
kubernetes-sigs
RemoveDuplicates can't exclude Deployment pods
What version of descheduler are you using?
descheduler version: v0.19.0
Please provide a copy of your descheduler policy config file apiVersion: "descheduler/v1alpha1" kind: "DeschedulerPolicy" strategies: "LowNodeUtilization": enabled: false "RemoveDuplicates": enabled: true params: removeDuplicates: excludeOwnerKinds: - "Deployment" "RemovePodsViolatingInterPodAntiAffinity": enabled: false "RemovePodsViolatingNodeAffinity": enabled: false "RemovePodsViolatingNodeTaints": enabled: false "RemovePodsHavingTooManyRestarts": enabled: false "PodLifeTime": enabled: false
What did you expect to see? pods created by deployment won't be evicted
What did you see instead? pods created by deployment got evicted
As pods created by deployment are acturally created and controlled by ReplicaSet, and we can only find ReplicaSet in these pods' ownerReferences, we can't only exclude Deployment pods in current implementation.
https://github.com/kubernetes-sigs/descheduler/blob/1682cc9462379bad46b079446c672828d6b5781e/pkg/descheduler/strategies/duplicates.go#L141-L152
/kind cleaup /remove-kind bug @seanmalloy @ingvagabund @damemi should we add support for Deployment here?
@lixiang233: The label(s) kind/cleaup cannot be applied, because the repository doesn't have them
In response to this:
/kind cleaup /remove-kind bug @seanmalloy @ingvagabund @damemi should we add support for Deployment 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.
Deployment owns a ReplicaSet IINM so setting kind to Deployment will not work.
Deployment owns a ReplicaSet IINM so setting kind to Deployment will not work.
yes, so I'm wondering if we should distinguish ReplicaSet pods(created by ReplicaSet) and Deployment pods(created by ReplicaSet whose owner is Deployment) by checking ReplicaSet's owner.
Does just setting it to exclude ReplicaSet cover the case here then? I guess for ease-of-use we could internally replace Deployment with ReplicaSet which would be a small change. The Deployment doesn't actually own any pods right?
Does just setting it to exclude
ReplicaSetcover the case here then? I guess for ease-of-use we could internally replaceDeploymentwithReplicaSetwhich would be a small change. The Deployment doesn't actually own any pods right?
Actually, these wouldn't work because we can't assume the user wants to exclude all ReplicaSets. This will take additional logic to determine if the pod is actually owned by a Deployment
Actually, these wouldn't work because we can't assume the user wants to exclude all ReplicaSets. This will take additional logic to determine if the pod is actually owned by a Deployment
If we want to keep current logic, maybe we should mention it in README and suggest users to use ReplicaSet instead of Deployment in ExcludeOwnerKinds?
Actually, these wouldn't work because we can't assume the user wants to exclude all ReplicaSets. This will take additional logic to determine if the pod is actually owned by a Deployment
If we want to keep current logic, maybe we should mention it in README and suggest users to use
ReplicaSetinstead ofDeploymentinExcludeOwnerKinds?
Yes, I think it would be good to add these additional details to the README here.
+1 to mentioning this in the readme, but I think this is still fixable. When a user excludes Deployments, we would just need to look at each Pod with a ReplicaSet ownerref, get that ReplicaSet, and check if the RS has a Deployment as its owner. Some additional recursive logic and overhead to check all of these
+1 to mentioning this in the readme, but I think this is still fixable. When a user excludes Deployments, we would just need to look at each Pod with a ReplicaSet ownerref, get that ReplicaSet, and check if the RS has a Deployment as its owner. Some additional recursive logic and overhead to check all of these
I think I can help to fix this. /assign
What do you think about adding this recursive check in the general case, to apply to more than just Deployments? This came up in OpenShift, where we have DeploymentConfigs which work similarly (but obviously it wouldn't make sense to hardcode that upstream here).
We could do the recursive check on every pod to see if any of its owners are the excluded kind. I think it's safe to assume that since most pods will have only 1-2 owners, this added computation would be small in real applications.
Though making the assumption that the user always wants to traverse the entire owner tree could cause problems as well. For example, if you have a pod owned by a configmap owned by a CRD owned by a deployment (pod->CM->CRD->Deployment) then is the pod evictable with excludeOwnerKinds: Deployment? In this case maybe an additional recursive bool parameter, or an entire list of recursiveExcludeOwnerKinds that indicate "If a pod is owned by this kind in any way, do not evict it". This could go alongside the current list, which says "only look one level up".
Maybe too complex, but I'm interested in what others think.
Can lead to infinite loop in case pod -> CM1 -> CM2 -> CM1 (if the loop can be created).
I'd like to avoid any complexity here and add per case refinement of the chain instead. With deployments we can always assume pod -> RS -> Deployment so we might just as well add support for:
excludeOwnerKinds:
- "ReplicaSet/Deployment"
I'll try to implement this like what @ingvagabund said. Besides, should we support CRD and how? Can we get a CR's owner without its client?
Can we get a CR's owner without its client?
You might use the dynamic discovery client assuming you will access only the common fields like name or kind. Though, I'd still limit the set of possible values to minimum. Once there's a request to support CRDs, we can re-iterate.
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale
I'm worring about the performance of RemoveDuplicates after we added this feature. hasExcludedOwnerRefKind will be called for every evictable pod, so we may need to send one or more requests to apiserver to get a pod's owner in each call, it'll cost very long time if descheduler runs in a huge cluster.
No need to use the dynamic client in the first iteration. Let's wait until there's a request for a specific CRD. We can discuss the details there.
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten