-
Notifications
You must be signed in to change notification settings - Fork 638
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
Use a fake client when evicting pods by individual strategies to accumulate the evictions #677
Use a fake client when evicting pods by individual strategies to accumulate the evictions #677
Conversation
With kubernetes-sigs/descheduler#677 it is possible to publish descheduler metrics even when running with --dry-run. Thus, also sharing the "what the descheduler would evict" with various configurations. Allowing a user to see effect of the current configuration without touching the cluster.
14a71e2
to
14764e1
Compare
/hold cancel |
With kubernetes-sigs/descheduler#677 it is possible to publish descheduler metrics even when running with --dry-run. Thus, also sharing the "what the descheduler would evict" with various configurations. Allowing a user to see effect of the current configuration without touching the cluster.
14764e1
to
33ecd57
Compare
33ecd57
to
6c17c8f
Compare
6c17c8f
to
1567ac7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention LGTM. I've few questions about implementation. Can you answer them?
@@ -74,6 +75,7 @@ func NewPodEvictor( | |||
evictSystemCriticalPods bool, | |||
ignorePvcPods bool, | |||
evictFailedBarePods bool, | |||
metricsEnabled bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid accumulating the metrics in case the metrics server is not enabled. To save some instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this related to current change?
namespaceInformer corev1informers.NamespaceInformer, | ||
priorityClassInformer schedulingv1informers.PriorityClassInformer, | ||
) (clientset.Interface, error) { | ||
fakeClient := fakeclientset.NewSimpleClientset() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need fakeClient, can't we get away with a local cache of pods just like we do in case of scheduler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the fake client so the strategies can stay unchanged. Replacing one client for another. Creating a local cache of pods would require more changes. Perhaps I don't see the full picture. Would you mind describing the approach in more detail? How a strategy is expected to consume the local cache?
@lixiang233 @seanmalloy PTAL |
…mulate the evictions Currently, when the descheduler is running with the --dry-run on, no strategy actually evicts a pod so every strategy always starts with a complete list of pods. E.g. when the PodLifeTime strategy evicts few pods, the RemoveDuplicatePods strategy still takes into account even the pods eliminated by the PodLifeTime strategy. Which does not correspond to the real case scenarios as the same pod can be evicted multiple times. Instead, use a fake client and evict/delete the pods from its cache so the strategies evict each pod at most once as it would be normally done in a real cluster.
1567ac7
to
901a16e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
Thank you for working on this @ingvagabund
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund, ravisantoshgudimetla 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:
Approvers can indicate their approval by writing |
/lgtm |
With kubernetes-sigs/descheduler#677 it is possible to publish descheduler metrics even when running with --dry-run. Thus, also sharing the "what the descheduler would evict" with various configurations. Allowing a user to see effect of the current configuration without touching the cluster.
With kubernetes-sigs/descheduler#677 it is possible to publish descheduler metrics even when running with --dry-run. Thus, also sharing the "what the descheduler would evict" with various configurations. Allowing a user to see effect of the current configuration without touching the cluster.
With kubernetes-sigs/descheduler#677 it is possible to publish descheduler metrics even when running with --dry-run. Thus, also sharing the "what the descheduler would evict" with various configurations. Allowing a user to see effect of the current configuration without touching the cluster.
…viction Use a fake client when evicting pods by individual strategies to accumulate the evictions
With kubernetes-sigs/descheduler#677 it is possible to publish descheduler metrics even when running with --dry-run. Thus, also sharing the "what the descheduler would evict" with various configurations. Allowing a user to see effect of the current configuration without touching the cluster.
Currently, when the descheduler is running with the --dry-run on, no strategy actually evicts a pod so every strategy always starts with a complete list of pods. E.g. when the PodLifeTime strategy evicts few pods, the RemoveDuplicatePods strategy still takes into account even the pods eliminated by the PodLifeTime strategy. Which does not correspond to the real case scenarios as the same pod can be evicted multiple times. Instead, use a fake client and evict/delete the pods from its cache so the strategies evict each pod at most once as it would be normally done in a real cluster.