fix: recycle inference pods without invalid deadlines

[Tanguille]

What

Fixes #1225.

maxPodLifetimeSeconds currently puts activeDeadlineSeconds in a Deployment pod template. Kubernetes rejects that field for ReplicaSet-backed workloads, so the generated Deployment cannot be updated at all — the InferenceService is stuck failing every reconcile.

This changes the feature to recycle one expired, owned Pod at a time through the eviction subresource. The Deployment then creates the replacement normally.

• keeps activeDeadlineSeconds out of Deployment templates
• recycles one Pod at a time, oldest deadline first, ties broken by name
• waits for the Deployment to be stable and all active Pods to be ready before touching anything
• cross-checks the active Pod count against status.replicas, so a foreign Pod sharing the selector holds recycling rather than risking an eviction the operator does not own
• respects PodDisruptionBudgets and retries a rejected eviction after 30 seconds
• preserves default graceful termination
• filters terminal Pods out of the active replica count
• emits a PodRecycled event so a Pod vanishing on a timer is visible in kubectl describe isvc

Interaction with rolloutPolicy.waitForIdle

waitForIdle promises never to drop in-flight generations. Recycling now honours it: an expired Pod is not evicted while the backend is busy, and the idle probe fails closed exactly as reconcileRolloutPolicy does.

Waiting forever is wrong for a saturated service, though — that is precisely when leaked driver memory hurts most. The new optional maxPodLifetimeIdleTimeoutSeconds bounds the wait:

value	behaviour
omitted	wait indefinitely for idle (default)
N	recycle anyway once the Pod has been overdue for N seconds
0	recycle without consulting the idle probe at all

The budget runs from the Pod's own expiry, which is already derivable from its start time, so no "deferred since" timestamp has to be persisted the way the rollout path does with a status condition.

Live cluster validation

Verified on a 3-node Talos v1.13 / k8s v1.36.2 cluster running llmkube 0.9.10, behind a suspended HelmRelease, then reverted to a provably clean GitOps state (image, CR, ClusterRole and CRD/webhook inventory all confirmed back to baseline).

• Bug reproduced. Two live services with maxPodLifetimeSeconds: 86400 were failing every reconcile: 16 activeDeadlineSeconds: Forbidden errors in the hour before the swap, 0 after. Reverting to 0.9.10 brought it back at 12/minute.
• No disruption. Neither stuck Deployment rolled. The invalid update had always been rejected, so the live pod templates never carried the field — removing it from the desired template made desired match live.
• Recycling works end to end. A 10h-old Pod at a 120s lifetime was evicted, PodRecycled fired, the replacement was scheduled in 4s and went Ready, and the service never left Ready. Exactly one eviction.

Bug this test found

Upgrading the operator image without the chart that owns its ClusterRole leaves pods/eviction ungranted. The resulting Forbidden propagated out of evictPod as a reconcile error, failing the entire InferenceService reconcile — status, service and HPA included — and retrying on the error backoff indefinitely, since no amount of retrying grants a permission.

Fixed in 3b884bd: log it, raise a PodRecycleForbidden warning event, back off to the recycle retry interval, and let the rest of the reconcile complete with only recycling disabled. Every unit test, make check-helm-rbac, and the generated manifests were correct — the RBAC rule is present in this PR. The failure needed a real cluster where image and chart versions could disagree.

Not covered

• maxPodLifetimeIdleTimeoutSeconds and the waitForIdle gating — exercising them needs the new CRD, and a CRD schema change is a one-way door on that cluster (Helm skips crds/ on upgrade). Unit tests only.
• Multi-replica and GPU-contended rollouts. The recycled workload was a single-replica CPU/iGPU embedding service.

Validation

• make test
• make vet
• make lint
• make check-helm-rbac
• helm template llmkube charts/llmkube with YAML parsing
• git diff --check
• live cluster, as above

[codecov]

Codecov Report

❌ Patch coverage is 82.67717% with 22 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/controller/pod_lifetime.go	83.80%	11 Missing and 6 partials ⚠️
api/v1alpha1/zz_generated.deepcopy.go	0.00%	3 Missing and 1 partial ⚠️
internal/controller/inferenceservice_controller.go	88.88%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[Defilan]

Reviewed the code, not just the (excellent) writeup, and cross-checked the new logic against the existing helpers it leans on. This is a clean fix for #1225 and I'm approving. A few non-blocking notes, one of which is a design question worth a one-line answer.

Verified

• activeDeadlineSeconds is fully gone from the Deployment pod template, and the test now asserts it stays nil regardless of maxPodLifetimeSeconds. That is the actual #1225 fix, and it explains the "no disruption on swap" result: the invalid field never made it onto live pods, so removing it makes desired match live.
• The recycle path is correctly guarded. stableDeployment() gates on IsControlledBy + ObservedGeneration == Generation + all replica counts equal to Spec.Replicas, which transitively blocks recycling during an in-progress rollout (old and new ReplicaSet pods coexisting drive UpdatedReplicas != Replicas) and tracks HPA-driven scale. The oldest-deadline argmin (ties by name) is order-independent and evicts exactly one even when several pods are simultaneously expired.
• The idle-timeout semantics are right: nil means wait forever, *timeout == 0 means recycle without asking, and the nil-check precedes the dereference, so the 0 vs omitted distinction is not a zero-value bug. The budget is derived from the pod's own expiry, so nothing has to be persisted.
• boundedSeconds() clamps below math.MaxInt64/1e9, so the lifetime math can't overflow a time.Duration into a negative deadline.
• Eviction errors fail safe: Forbidden and PDB TooManyRequests return (30s, nil), so status/service/HPA reconciliation (already done earlier in Reconcile) is never blocked; only genuinely unexpected errors propagate. The PodRecycleForbidden degrade-gracefully path you found on the live cluster is exactly the right call.
• The pods/eviction RBAC is create-only and kept in sync across config/rbac/role.yaml and the chart ClusterRole; both CRD copies got the field identically.

Notes (non-blocking)

• Question, not a defect: recycling gates the idle wait on RolloutPolicyEnabled(), whereas the rollout-defer path uses ShouldDeferRollout(), which honors rolloutPolicy.force: true. So an operator using force: true as an emergency escape hatch to push changes through without waiting for idle will still find pod recycling waits for idle (indefinitely, if maxPodLifetimeIdleTimeoutSeconds is unset). I think this is deliberate since recycling has its own dedicated escape hatch via the timeout field, but confirm it's intentional rather than an oversight.
• Minor: ownership safety rests on the selector labels being namespace-unique (app + inference.llmkube.dev/service, both the isvc name) plus the count cross-check, not an ownerRef-chain walk. It fails closed (a foreign pod sharing the selector holds recycling, never triggers a wrongful eviction), so this is fine, just a soft guarantee via naming rather than a hard invariant.
• Informational: after a successful eviction the requeue is 0, relying on the Deployment watch to re-trigger. Normal case fires in seconds; a replacement pod stuck Pending would leave the next pod's deadline unchecked until another event or the cache resync. Not worth blocking on.

Thanks for the thorough live-cluster validation and for catching the ungranted pods/eviction RBAC failure mode. Approving; just confirm the force/idle interaction is intended.

[Tanguille]

Intentional, yes.

The reasoning is that force describes a rollout event, not a standing preference. You set it because you need a particular spec change out now, but it lives in the spec permanently, so if recycling honored it, one emergency config push would quietly convert recycling from "wait for an idle window" to "kill on a timer" for the life of the object, long after the thing you were forcing had shipped. Recycling isn't a rollout, so it shouldn't inherit a flag scoped to one.

The other half is that maxPodLifetimeIdleTimeoutSeconds is strictly more expressive than the boolean would be: 0 means don't wait at all (what force would give you), N means wait a bounded amount and then take the pod anyway, and omitted means wait indefinitely. Collapsing that onto force would lose the middle option, which is the one I'd actually want in most cases.

That said, you're pointing at something real and I don't want to wave it away. Omitted meaning "wait forever" is a bad default for the case the feature mostly exists to serve. We run maxPodLifetimeSeconds as leak mitigation, for a GPU memory leak that only a restart clears, and a pod leaking under sustained load is precisely the pod that never goes idle. So the default quietly defeats the use case, and nothing tells you. A held recycle is one Info log line. There's a PodRecycled event and a PodRecycleForbidden event, but nothing for "expired and still holding", so a pod can sit past its max lifetime indefinitely with no signal on the object.

Same shape as #1223, really. It goes quiet instead of telling you.

I'd rather not change the gate. But I'd like to make the held state visible: an event when a recycle is first held, and a condition or a metric so you can alert on "expired but not recycled" rather than reading logs. Happy to send that as a follow-up if you think it's worth having. Whether the default should stay "wait forever" is a separate question and your call. I'd lean toward keeping it and making it observable rather than changing behavior under people.