[FEA]: Throttle eviction retries per node instead of slowing the whole reconcile pass

[ayuskauskas]

Is this a new feature, an improvement, or a change to existing functionality?

Improvement

Component

Operator (controller-manager)

Problem description

#625 (for #541) reclassifies a PodDisruptionBudget eviction rejection from a reconcile error to a wait state. That is the right call, but it removes an accidental throttle: the error return was what put the CR into controller-runtime's exponential backoff and kept the operator from re-attempting a refused eviction on every pass. #541 anticipated this and proposed a 30s RequeueAfter as the replacement.

The replacement does not fit the controller's shape.

Reconcile ignores req entirely — it lists every NodeWright and every Node, builds one cluster state, and loops over all of them under MaxConcurrentReconciles: 1. There is exactly one ctrl.Result per pass for the whole cluster. So if len(drainBlocks) > 0 { requeueAfter = 30 * time.Second } inside RunSkyhookPackages is not a per-NodeWright backoff. processSkyhooksPerNode keeps the last non-nil result (if res != nil { result = res }), so which value survives depends on map iteration order: a blocked NodeWright processed last slows every healthy one to 30s, and one processed first is overwritten back to 2s. The behavior the docs describe is a coin flip.

One dial is also doing three unrelated jobs:

1. how often to retry an eviction the apiserver just refused — the only thing [FEA]: Surface drain blockers (PDB, unmanaged pods, emptyDir) as a DrainBlocked condition #541 asked for;
2. how quickly the operator notices the blocker cleared;
3. the precision of spec.drainConfig.timeout, since drain.TimedOut is only evaluated at the top of DrainNode on a subsequent pass.

(2) matters more than it appears. The heavy pass watches only NodeWright and Node — pod events are deliberately excluded ("PodReconciler and JobReconciler own them on their own watches"), and PodReconciler filters to ownedPod(), i.e. package pods. The workload pod holding a PDB is watched by nothing. When it finally terminates and the PDB regains headroom, no event reaches this reconciler. The requeue timer is the only wakeup, so lengthening it lengthens drain-resume latency directly.

Feature description

As a cluster administrator, I want the operator to stop re-attempting an eviction the apiserver just refused, without slowing unrelated NodeWrights and without delaying the resumption of a drain once its blocker clears.

Describe your ideal solution

Throttle the attempt, not the pass.

• Leave the reconcile cadence at 2s.
• In DrainNode, skip the eviction call for a node whose last attempt was less than drainBlockedEvictionInterval ago (30s as a starting point). Name the constant next to configSyncRetryInterval (skyhook_controller.go:1990) — same value, same shape, and it already carries the why-comment-plus-issue-reference pattern this should follow.
• Track last-attempt time per node in memory on the reconciler.

Explicit exception to the "no reconciler-owned state" rule.

AGENTS.md says not to cache between reconciles and to persist anything that must survive. This is a deliberate, scoped exception, and the justification is the failure mode itself: the only thing lost when the operator pod restarts or leadership moves is the memory of when we last tried. The next pass then attempts the eviction immediately — which is precisely today's behavior, and precisely the behavior this issue exists to narrow. The worst case of losing this state is the current cadence. No correctness, convergence, or user-visible state depends on it; the map is a rate-limit hint, not progress.

Persisting a last-attempt timestamp as a Node annotation buys nothing against that worst case and costs a write to every blocked Node on every interval — on the path that is already under pressure precisely because the cluster is blocked. Persistence is the right default for state whose loss changes an outcome. Losing this changes a retry delay.

Requirements that come with taking the exception:

• a why-comment at the declaration stating this reasoning, so the next reader does not "fix" it into an annotation;
• entries pruned for nodes that leave the NodeWright, so a long-lived operator does not leak the map;
• mutex-guarded — MaxConcurrentReconciles is 1 for this controller today, but the pod and job reconcilers share the struct.

Also in scope, independent of the mechanism:

• Clamp to the drain deadline. Any requeue longer than the base cadence returned while a drain clock is running should be clamped to the time remaining before drainStart_ + spec.drainConfig.timeout. DrainStartedAt() is already read at the top of DrainNode. Without this, a timeout shorter than the throttle overshoots by up to the throttle — a timeout: 10s fires at ~30s.
• Fix the result aggregation regardless. processSkyhooksPerNode should take the minimum RequeueAfter across NodeWrights rather than last-wins, so no NodeWright's interval can ever leak onto another.

Alternatives you have considered

• Minimum-aggregation alone, keeping the per-pass 30s. Fixes the leak honestly and is two lines, but the throttle then only applies when no other NodeWright has work — rare on a busy cluster — and when it does apply, it applies to everyone. Delivers the correctness fix without delivering the throttle.
• Read the PDB instead of probing by evicting. status.disruptionsAllowed from the informer cache says whether an eviction can succeed; reading it is free and needs no throttle at all, and the condition detail could be built from the PDB object rather than from apiserver prose that is explicitly not a stable contract. Best end state, but it needs a pod→PDB selector mapping and must keep the 429 path anyway for the stale-cache race. Worth its own issue if attempt-gating proves insufficient.
• Persist last-attempt as a Node annotation. Rejected above: all of the cost, none of the benefit, for state whose loss is harmless.
• Do nothing, keep 2s. No throttle at all. [FEA]: Surface drain blockers (PDB, unmanaged pods, emptyDir) as a DrainBlocked condition #541's concern stands: spec.drainConfig.timeout is nil by default, so the common case re-attempts a refused eviction every 2s for as long as the PDB holds.

Additional context

Split out of #541 / #625, as that issue anticipated: "a per-wait-state backoff mechanism would be a good fit here ... That is a larger change and probably belongs in its own issue."

The review recommendation on #625 is to land the DrainBlocked condition on its own and drop the RequeueAfter change; this issue covers the replacement throttle.

Related: #542 / #582 surface the podNonInterruptLabels barrier, which is the same wait-state shape but returns before DrainNode is ever called, so it never reaches the eviction path and is unaffected by the mechanism chosen here.