Added Per-pod eviction backoff and retry logic #515
Conversation
| } | ||
|
|
||
| // Evict adds a pod to the EvictionQueue | ||
| func (e *EvictionQueue) Add(pods []*v1.Pod) { |
There was a problem hiding this comment.
A variadic might be a bit cleaner here, since you could imagine evicting a single pod. 😉
There was a problem hiding this comment.
Since the only code that is adding a pod is our code, I thought it would make more sense just to put an array here, since that's how we form the list of pods anyways. I have no strong feelings either way though.
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| const ( | ||
| evictionQueueMaxDelay = 10 * time.Second |
There was a problem hiding this comment.
Thoughts on pushing this to 60 seconds? I'm a bit wary of too much eviction QPS. 1,000 pods at 10 seconds is 100qps.
There was a problem hiding this comment.
I'm a little wary of pushing it to 60 seconds. Imagine I have 5 pods, with a PDB that allows me to evict one at a time. If we assume that our nodes take 50 seconds to be fully provisioned, we have enough time for the eviction delay to get to reach a minute for any one of these pods. At the worst, we could wait a whole minute after the PDB would allow an eviction to evict another pod. Let's say these are all on the same node, this node would then take much much longer to terminate and drain.
I realize this is a very specific case, but since our allocation logic tends to group pods that are created together (since scaling up deployments is the most basic happenstance of pending pods), our logic may end up running into something like this more than not.
I think we could fine tune these numbers when we get to performance and scale testing. What do you think?
| @@ -137,3 +145,37 @@ func ExpectReconcileSucceeded(reconciler reconcile.Reconciler, key client.Object | |||
| _, err := reconciler.Reconcile(context.Background(), reconcile.Request{NamespacedName: key}) | |||
| Expect(err).ToNot(HaveOccurred()) | |||
| } | |||
|
|
|||
| func ExpectPodsEvictingSucceeded(c client.Client, pods ...*v1.Pod) { | |||
There was a problem hiding this comment.
Thoughts on calling this
ExpectEvicted(c client.Client, pods ...*v1.Pod)
and then actually doing the deletion after we verify the deletion timestamp is set?
There was a problem hiding this comment.
I would rather have this check just check it, then separately delete the pod, just in case there is a time where we don't immediately want to delete it, and check some other things first.
| queue workqueue.RateLimitingInterface | ||
| coreV1Client corev1.CoreV1Interface | ||
|
|
||
| enqueued set.Set |
There was a problem hiding this comment.
Should we add this functionality to the newly added utils/parallel/workqueue.go? That implementation is slightly different, but there is a lot of shared functionality. This could definitely be done later though.
There was a problem hiding this comment.
Ellis and I talked about this, and we think there is a possibility to merge them here. We decided to punt on this merging effort later in favor of getting this feature in.
Issue, if available:
#452
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.