Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sorting pods before evicting may be helpful #294

Closed
lixiang233 opened this issue May 26, 2020 · 9 comments
Closed

Sorting pods before evicting may be helpful #294

lixiang233 opened this issue May 26, 2020 · 9 comments
Labels
kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature.

Comments

@lixiang233
Copy link
Contributor

In README, the Pod Evictions chapter writes Best efforts pods are evicted before burstable and guaranteed pods. But I notice that only LowNodeUtilization implements it. I guess that's probably because sorting pods does not affect whether pods are evicted in other strategies. I think sorting pods before evicting may be helpful because we can evict high priority pods first to give them higher possibility of being perfertly scheduled.

Here I devide all these 7 strategies into two types: single-pod-related-strategies and multi-pods-related-strategies. In single-pod-related-strategies, pods will always be evicted if they violate the policies, so we should evict high priority pods first to give them higher possibility of being perfertly scheduled. But in multi-pods-related-strategies, pods may not need to be evicted if some other pods on the same node have been evicted, so here we should evict low priority pods first to try to avoid evicting high priority pods.

single-pod-related-strategies includes:

  • RemoveDuplicates
  • RemovePodsViolatingNodeAffinity
  • RemovePodsViolatingNodeTaints
  • RemovePodsHavingTooManyRestarts
  • PodLifeTime

multi-pods-related-strategies includes:

  • LowNodeUtilization
  • RemovePodsViolatingInterPodAntiAffinity.

Sorting pods in single-pod-related-strategies may be optional, but I think at least in multi-pods-related-strategies we should sort them by their priority before eviction starts.

@seanmalloy
Copy link
Member

/kind enhancement
/kind documentation

@k8s-ci-robot
Copy link
Contributor

@seanmalloy: The label(s) kind/enhancement cannot be applied, because the repository doesn't have them

In response to this:

/kind enhancement
/kind documentation

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.

@k8s-ci-robot k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label May 29, 2020
@seanmalloy
Copy link
Member

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 29, 2020
@ingvagabund
Copy link
Contributor

Strategies like PodLifeTime are iterating through every node sequentially. Once a node is cleared, other node is processed. Given one can set maximum number of eviction in total, you would need to first go through every node, collect all the pods that can be evicted, sort all the pods based on priority and then start eviction. Also, together with max evictions per node, you would need to check if the upper bound is not reached (trivial by checking pod.NodeName). So the transition looks to be quite trivial. Also, given LowNodeUtilization has the code for classifying the pods, it's more or less just about making the code exported and writing a bunch of unit tests to validate the new constraint.

On the other hand, you don't always want to iterate through all the nodes and then start evicting. E.g. going through 5000 nodes will take longer with the constraint enforced in case there's many pods to be bound to obedience before the maximum number of evictions get reached. Introducing new configuration option will do the trick. With that we might also introduce another configuration saying how many nodes the descheduler should iterate through. Scheduler has concept of nextStartNodeIndex which we might exercise.

@lixiang233
Copy link
Contributor Author

Thanks @ingvagabund ! My initial thought was sorting pods only in node dimension (get all evictable pods of a single node and do sorting and evicting) just like LowNodeUtilization, in this case, we needn't change a lot and will benefit from it. I'm not sure if it's worth to sort evictable pods in cluster dimension before evicting. We may need more disscussions and consider some specific examples.

@seanmalloy
Copy link
Member

@lixiang233 can we close this issue since #322 has been merged? Looking at the discussion I don't think we want to implement this for single-pod-related-strategies.

@lixiang233
Copy link
Contributor Author

@seanmalloy Yes, I'm not sure if we really need this in single-pod-related-strategies, it can be closed and once we are clear that it's needed, we can reopen this issue.

@seanmalloy
Copy link
Member

@lixiang233 thanks!

/close

@k8s-ci-robot
Copy link
Contributor

@seanmalloy: Closing this issue.

In response to this:

@lixiang233 thanks!

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants