Evict promptly within DisruptionBudget #111
Conversation
This reverts commit 3d6caca.
|
Ack. Thanks for the contribution @JHK, it definitely make sense. |
There was a problem hiding this comment.
Sorry for the late review @JHK and happy new year! I should be more responsive from now on.
| pdbList, err := c.clientset.PolicyV1().PodDisruptionBudgets(pod.Namespace).List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| klog.Errorf("unable to list PodDisruptionBudgets: %s", err) | ||
| return false |
There was a problem hiding this comment.
I'm afraid we hide permission issues here. Should we return an error if we can list PodDisruptionBudgets ? Shouldn't happen right ?
There was a problem hiding this comment.
Indeed we are hiding permissions issues. Without the permission the eviction would happen like before - with the pause time. It would still respect the PDB though when deleting pods. Also it would spam the logs with errors on each execution.
The gain is a simpler method interface without a err in the return values. I think this is worth it given that the permission issue is not even harmful.
| } | ||
| } | ||
|
|
||
| return c.clientset.PolicyV1().Evictions(pod.Namespace).Evict(ctx, &evictionPolicy) |
There was a problem hiding this comment.
Now that we're evicting Pods asynchronously, there is a higher chance we try to evict a Pod that is already gone right ? Do you know what kind of error is returned in such a case ? Could we silence it ? Can also be in another PR if you want to observe that live.
There was a problem hiding this comment.
Indeed this can happen. Before this PR the loop was blocking the next execution for long enough such that the next iteration had to wait until all pods had been sent an eviction (even sigterm).
I don't think this is too big of a deal. If it gets too verbose I'll issue another PR.
|
|
||
| if c.hasDisruptionBudget(ctx, pod) { | ||
| if len(c.pdbChan) == cap(c.pdbChan) { | ||
| klog.Warningf("PDB channel is full, skipping eviction for Pod '%s/%s'", pod.Namespace, pod.Name) |
There was a problem hiding this comment.
We're not monitoring this controller with metrics. What about a safety net, which would consist in having a "skipped eviction counter", reset when we enqueue, incremented when we skip, and crash the controller if we reach let's say 10 skipped evictions ?
There was a problem hiding this comment.
In general I like the idea. If the queue is full, it will likely be soon again. The forced restart will empty the queue and maybe even trigger a notification based on frequent restarts.
However, when evictions are being skipped it is because the queue is already full. We won't miss an eviction though, because there will be another run trying to schedule pods to be evicted before the queue has been emptied (at least if you run the controller close to the default values).
I think this could be a dedicated change.
| }() | ||
|
|
||
| for pod := range c.pdbChan { | ||
| err := c.evictPod(ctx, pod) |
There was a problem hiding this comment.
Since we don't observe any eviction pause for Pods with a PodDisruptionBudget, we could theoretically evict 10 at a time if the eviction was instantaneous
Given that, why not always start a goroutine to evict such Pods and be as fast as possible ? There is the risk we blow up the controller with long running evictions, but in theory, the PodDisruptionBudget should protect us from evicting all Pods of a same Deployment at once and disrupting a service.
There was a problem hiding this comment.
It goes with all at a time until it hits one, that it cannot evict due to the disruption budget being exhausted. This one is being retried and then blocks the queue from further execution.
I think it makes sense to remove the whole backoff logic and solely rely on the check interval. Will do that.
I was trying to evict all pods beyond the threshold as soon as possible. Maybe we will find a way to do this at a later point, but until now this is already an improvement over the status quo.
|
Are we good to merge @JHK ? I would try it out on a cluster then, manually add the missing RBAC, before release a new version + updating the chart. |
|
Yes, I think this is good to be merged. I tried this version already in one of our upcoming clusters. It didn't need to do much there yet. wrt the release I still have JHK#1 up my sleeves. Will create the PR once this is merged. |
For Pods having a PodDisruptionBudget attached we would like to evict them without the need to wait for further runs. To not interrupt workloads without a PDB this PR separates eviction into two background processes - one with the previous behavior and one for pods with PDB.
The background eviction uses buffered channels. The buffer size has been added as additional parameter. This also fixes the controller blocking when being started and immediately evicting Pods.
I also updated dependencies and migrated from
policyv1beta1topolicyv1as this got removed in K8s v1.25.