From 6207d9035c874fb054d0e3215032b500f357db76 Mon Sep 17 00:00:00 2001 From: Tanguille Date: Thu, 23 Jul 2026 00:50:15 +0200 Subject: [PATCH 1/5] fix: recycle inference pods without invalid deadlines Signed-off-by: Tanguille --- api/v1alpha1/inferenceservice_types.go | 17 +- charts/llmkube/templates/clusterrole.yaml | 13 + .../templates/crds/inferenceservices.yaml | 17 +- ...ference.llmkube.dev_inferenceservices.yaml | 17 +- config/rbac/role.yaml | 13 + internal/controller/deployment_builder.go | 19 +- .../controller/inferenceservice_controller.go | 12 +- .../inferenceservice_deployment_test.go | 5 +- internal/controller/pod_lifetime.go | 215 ++++++++++++ internal/controller/pod_lifetime_test.go | 317 ++++++++++++++++++ 10 files changed, 603 insertions(+), 42 deletions(-) create mode 100644 internal/controller/pod_lifetime.go create mode 100644 internal/controller/pod_lifetime_test.go diff --git a/api/v1alpha1/inferenceservice_types.go b/api/v1alpha1/inferenceservice_types.go index d873668f..98f5d8ea 100644 --- a/api/v1alpha1/inferenceservice_types.go +++ b/api/v1alpha1/inferenceservice_types.go @@ -561,15 +561,14 @@ type InferenceServiceSpec struct { // +optional SLO *SLOSpec `json:"slo,omitempty"` - // MaxPodLifetimeSeconds sets the maximum lifetime (in seconds) for - // inference pods. When set, the operator copies this value to - // PodSpec.ActiveDeadlineSeconds on the generated Deployment's pod - // template, causing Kubernetes to terminate the pod after the - // specified duration even if it remains healthy. This is useful for - // workloads that need periodic process recycling to release driver - // memory (e.g. llama.cpp on AMD Vulkan with pinned GTT memory). - // When omitted, pods run indefinitely until manually restarted or - // the Deployment is updated. + // MaxPodLifetimeSeconds requests best-effort periodic recycling of + // deployment-backed inference pods. The controller serializes graceful + // replacement of eligible pods, using their status start time as the age + // reference; it is not a strict deadline and does not set + // PodSpec.ActiveDeadlineSeconds. This is useful for workloads that need + // periodic process recycling to release driver memory (e.g. llama.cpp on + // AMD Vulkan with pinned GTT memory). When omitted, pods run indefinitely + // until manually restarted or the Deployment is updated. // +kubebuilder:validation:Minimum=1 // +optional MaxPodLifetimeSeconds *int64 `json:"maxPodLifetimeSeconds,omitempty"` diff --git a/charts/llmkube/templates/clusterrole.yaml b/charts/llmkube/templates/clusterrole.yaml index 4ef96f7b..972ff37e 100644 --- a/charts/llmkube/templates/clusterrole.yaml +++ b/charts/llmkube/templates/clusterrole.yaml @@ -57,6 +57,12 @@ rules: - patch - update - watch +- apiGroups: + - "" + resources: + - pods/eviction + verbs: + - create - apiGroups: - apps resources: @@ -69,6 +75,13 @@ rules: - patch - update - watch +- apiGroups: + - apps + resources: + - replicasets + verbs: + - list + - watch - apiGroups: - autoscaling resources: diff --git a/charts/llmkube/templates/crds/inferenceservices.yaml b/charts/llmkube/templates/crds/inferenceservices.yaml index 46f6c8ea..114c0cac 100644 --- a/charts/llmkube/templates/crds/inferenceservices.yaml +++ b/charts/llmkube/templates/crds/inferenceservices.yaml @@ -3449,15 +3449,14 @@ spec: type: boolean maxPodLifetimeSeconds: description: |- - MaxPodLifetimeSeconds sets the maximum lifetime (in seconds) for - inference pods. When set, the operator copies this value to - PodSpec.ActiveDeadlineSeconds on the generated Deployment's pod - template, causing Kubernetes to terminate the pod after the - specified duration even if it remains healthy. This is useful for - workloads that need periodic process recycling to release driver - memory (e.g. llama.cpp on AMD Vulkan with pinned GTT memory). - When omitted, pods run indefinitely until manually restarted or - the Deployment is updated. + MaxPodLifetimeSeconds requests best-effort periodic recycling of + deployment-backed inference pods. The controller serializes graceful + replacement of eligible pods, using their status start time as the age + reference; it is not a strict deadline and does not set + PodSpec.ActiveDeadlineSeconds. This is useful for workloads that need + periodic process recycling to release driver memory (e.g. llama.cpp on + AMD Vulkan with pinned GTT memory). When omitted, pods run indefinitely + until manually restarted or the Deployment is updated. format: int64 minimum: 1 type: integer diff --git a/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml b/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml index 2cbf9e2b..7b920808 100644 --- a/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml +++ b/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml @@ -3445,15 +3445,14 @@ spec: type: boolean maxPodLifetimeSeconds: description: |- - MaxPodLifetimeSeconds sets the maximum lifetime (in seconds) for - inference pods. When set, the operator copies this value to - PodSpec.ActiveDeadlineSeconds on the generated Deployment's pod - template, causing Kubernetes to terminate the pod after the - specified duration even if it remains healthy. This is useful for - workloads that need periodic process recycling to release driver - memory (e.g. llama.cpp on AMD Vulkan with pinned GTT memory). - When omitted, pods run indefinitely until manually restarted or - the Deployment is updated. + MaxPodLifetimeSeconds requests best-effort periodic recycling of + deployment-backed inference pods. The controller serializes graceful + replacement of eligible pods, using their status start time as the age + reference; it is not a strict deadline and does not set + PodSpec.ActiveDeadlineSeconds. This is useful for workloads that need + periodic process recycling to release driver memory (e.g. llama.cpp on + AMD Vulkan with pinned GTT memory). When omitted, pods run indefinitely + until manually restarted or the Deployment is updated. format: int64 minimum: 1 type: integer diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index cb0af67b..757eee8a 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -39,6 +39,12 @@ rules: - patch - update - watch +- apiGroups: + - "" + resources: + - pods/eviction + verbs: + - create - apiGroups: - "" - events.k8s.io @@ -72,6 +78,13 @@ rules: - patch - update - watch +- apiGroups: + - apps + resources: + - replicasets + verbs: + - list + - watch - apiGroups: - autoscaling resources: diff --git a/internal/controller/deployment_builder.go b/internal/controller/deployment_builder.go index 70f7187b..ecf43e38 100644 --- a/internal/controller/deployment_builder.go +++ b/internal/controller/deployment_builder.go @@ -432,16 +432,15 @@ func (r *InferenceServiceReconciler) constructDeployment( Annotations: buildPodAnnotations(isvc), }, Spec: corev1.PodSpec{ - SecurityContext: inferPodSecurityContext(isvc, r.DefaultFSGroup), - InitContainers: storageConfig.initContainers, - Containers: []corev1.Container{container}, - Volumes: storageConfig.volumes, - PriorityClassName: r.resolvePriorityClassName(isvc), - RuntimeClassName: isvc.Spec.RuntimeClassName, - ImagePullSecrets: isvc.Spec.ImagePullSecrets, - EnableServiceLinks: resolveEnableServiceLinks(backend), - ResourceClaims: modelResourceClaims(model), - ActiveDeadlineSeconds: isvc.Spec.MaxPodLifetimeSeconds, + SecurityContext: inferPodSecurityContext(isvc, r.DefaultFSGroup), + InitContainers: storageConfig.initContainers, + Containers: []corev1.Container{container}, + Volumes: storageConfig.volumes, + PriorityClassName: r.resolvePriorityClassName(isvc), + RuntimeClassName: isvc.Spec.RuntimeClassName, + ImagePullSecrets: isvc.Spec.ImagePullSecrets, + EnableServiceLinks: resolveEnableServiceLinks(backend), + ResourceClaims: modelResourceClaims(model), }, }, }, diff --git a/internal/controller/inferenceservice_controller.go b/internal/controller/inferenceservice_controller.go index be240f0f..f49050ff 100644 --- a/internal/controller/inferenceservice_controller.go +++ b/internal/controller/inferenceservice_controller.go @@ -147,6 +147,8 @@ func initContainerSecurityContext(isvc *inferencev1alpha1.InferenceService) *cor // +kubebuilder:rbac:groups=core,resources=services,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=discovery.k8s.io,resources=endpointslices,verbs=get;list;watch // +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch +// +kubebuilder:rbac:groups=core,resources=pods/eviction,verbs=create +// +kubebuilder:rbac:groups=apps,resources=replicasets,verbs=list;watch // +kubebuilder:rbac:groups=core,resources=persistentvolumeclaims,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups=scheduling.k8s.io,resources=priorityclasses,verbs=get;list;watch // +kubebuilder:rbac:groups=autoscaling,resources=horizontalpodautoscalers,verbs=get;list;watch;create;update;patch;delete @@ -248,20 +250,26 @@ func (r *InferenceServiceReconciler) Reconcile(ctx context.Context, req ctrl.Req return finalResult, statusErr } + if requeueAfter, err := r.reconcilePodLifetime(ctx, inferenceService, isMetal); err != nil { + return finalResult, err + } else { + finalResult.RequeueAfter = earliestPositive(finalResult.RequeueAfter, requeueAfter) + } + // When a rollout is deferred pending idle, reconcileRolloutPolicy set // RolloutDeferred=True (persisted by the status update above). Drive a // recheck so the controller notices when the backend goes idle or the // idleTimeoutSeconds budget is spent. Metal-backed services never defer // (no Deployment), so this does not conflict with the metal requeue below. if cond := meta.FindStatusCondition(inferenceService.Status.Conditions, ConditionRolloutDeferred); cond != nil && cond.Status == metav1.ConditionTrue { - finalResult.RequeueAfter = inferencev1alpha1.DefaultIdleCheckInterval + finalResult.RequeueAfter = earliestPositive(finalResult.RequeueAfter, inferencev1alpha1.DefaultIdleCheckInterval) } // On the metal path a heartbeat going stale generates no watch event, so // force a periodic requeue when the Endpoints carry the annotation. if isMetal { if requeue := metalHeartbeatRequeueDuration(metalSnap); requeue > 0 { - finalResult.RequeueAfter = requeue + finalResult.RequeueAfter = earliestPositive(finalResult.RequeueAfter, requeue) } } diff --git a/internal/controller/inferenceservice_deployment_test.go b/internal/controller/inferenceservice_deployment_test.go index fff3518c..a72e784c 100644 --- a/internal/controller/inferenceservice_deployment_test.go +++ b/internal/controller/inferenceservice_deployment_test.go @@ -609,7 +609,7 @@ var _ = Describe("Multi-GPU Deployment Construction", func() { Expect(deployment.Spec.Template.Spec.ActiveDeadlineSeconds).To(BeNil()) }) - It("should set ActiveDeadlineSeconds on the pod template when maxPodLifetimeSeconds is set", func() { + It("should leave ActiveDeadlineSeconds nil when maxPodLifetimeSeconds is set", func() { lifetime := int64(3600) isvc := &inferencev1alpha1.InferenceService{ ObjectMeta: metav1.ObjectMeta{ @@ -623,8 +623,7 @@ var _ = Describe("Multi-GPU Deployment Construction", func() { }, } deployment := reconciler.constructDeployment(isvc, model, 1) - Expect(deployment.Spec.Template.Spec.ActiveDeadlineSeconds).NotTo(BeNil()) - Expect(*deployment.Spec.Template.Spec.ActiveDeadlineSeconds).To(Equal(int64(3600))) + Expect(deployment.Spec.Template.Spec.ActiveDeadlineSeconds).To(BeNil()) }) }) diff --git a/internal/controller/pod_lifetime.go b/internal/controller/pod_lifetime.go new file mode 100644 index 00000000..b5248abe --- /dev/null +++ b/internal/controller/pod_lifetime.go @@ -0,0 +1,215 @@ +package controller + +import ( + "context" + "sort" + "time" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + inferencev1alpha1 "github.com/defilantech/llmkube/api/v1alpha1" +) + +const ( + maxPodLifetimeSeconds int64 = 9223372036 + podLifetimeRetry = 30 * time.Second +) + +// earliestPositive merges controller timers without allowing a zero timer to +// create a polling loop. +func earliestPositive(values ...time.Duration) time.Duration { + var earliest time.Duration + for _, value := range values { + if value > 0 && (earliest == 0 || value < earliest) { + earliest = value + } + } + return earliest +} + +// reconcilePodLifetime recycles at most one healthy, expired pod. All +// workload checks are deliberately conservative: a watch will cause another +// attempt after the Deployment has settled or a replacement pod is ready. +func (r *InferenceServiceReconciler) reconcilePodLifetime(ctx context.Context, isvc *inferencev1alpha1.InferenceService, isMetal bool) (time.Duration, error) { + return r.reconcilePodLifetimeAt(ctx, isvc, isMetal, time.Now()) +} + +func (r *InferenceServiceReconciler) reconcilePodLifetimeAt(ctx context.Context, isvc *inferencev1alpha1.InferenceService, isMetal bool, now time.Time) (time.Duration, error) { + if isMetal || isvc.Spec.MaxPodLifetimeSeconds == nil { + return 0, nil + } + + deployment, err := r.getStableDeployment(ctx, isvc) + if err != nil || deployment == nil { + return 0, err + } + owned, err := r.ownedActivePods(ctx, deployment) + if err != nil { + return 0, err + } + if !activePodCountMatches(deployment, owned) || !allPodsReady(owned) { + return 0, nil + } + return r.recycleExpiredPod(ctx, owned, podLifetime(*isvc.Spec.MaxPodLifetimeSeconds), now) +} + +func (r *InferenceServiceReconciler) getStableDeployment(ctx context.Context, isvc *inferencev1alpha1.InferenceService) (*appsv1.Deployment, error) { + deployment := &appsv1.Deployment{} + err := r.Get(ctx, types.NamespacedName{Name: isvc.Name, Namespace: isvc.Namespace}, deployment) + if apierrors.IsNotFound(err) { + return nil, nil + } + if err != nil { + return nil, err + } + _, stable := stableDeployment(deployment, isvc.UID) + if !stable { + return nil, nil + } + return deployment, nil +} + +func activePodCountMatches(deployment *appsv1.Deployment, pods []*corev1.Pod) bool { + replicas := int32(1) + if deployment.Spec.Replicas != nil { + replicas = *deployment.Spec.Replicas + } + return int64(len(pods)) == int64(replicas) +} + +func (r *InferenceServiceReconciler) recycleExpiredPod(ctx context.Context, owned []*corev1.Pod, lifetime time.Duration, now time.Time) (time.Duration, error) { + type candidate struct { + pod *corev1.Pod + deadline time.Time + } + candidates := make([]candidate, 0, len(owned)) + var earliest time.Duration + for _, pod := range owned { + deadline := pod.Status.StartTime.Time.Add(lifetime) + if deadline.After(now) { + earliest = earliestPositive(earliest, deadline.Sub(now)) + } else { + candidates = append(candidates, candidate{pod: pod, deadline: deadline}) + } + } + if len(candidates) == 0 { + return earliest, nil + } + sort.Slice(candidates, func(i, j int) bool { + if candidates[i].deadline.Equal(candidates[j].deadline) { + return candidates[i].pod.Name < candidates[j].pod.Name + } + return candidates[i].deadline.Before(candidates[j].deadline) + }) + return evictPod(ctx, r.Client, candidates[0].pod) +} + +func evictPod(ctx context.Context, r client.Client, pod *corev1.Pod) (time.Duration, error) { + uid := pod.UID + eviction := &policyv1.Eviction{ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}, DeleteOptions: &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &uid}}} + err := r.SubResource("eviction").Create(ctx, pod, eviction) + switch { + case apierrors.IsNotFound(err): + return 0, nil + case apierrors.IsTooManyRequests(err): + return podLifetimeRetry, nil + default: + return 0, err + } +} + +func stableDeployment(deployment *appsv1.Deployment, isvcUID types.UID) (int32, bool) { + if !controlledBy(deployment, isvcUID) || deployment.Generation == 0 || deployment.Status.ObservedGeneration != deployment.Generation { + return 0, false + } + replicas := int32(1) + if deployment.Spec.Replicas != nil { + replicas = *deployment.Spec.Replicas + } + return replicas, deployment.Status.Replicas == replicas && deployment.Status.UpdatedReplicas == replicas && deployment.Status.ReadyReplicas == replicas && deployment.Status.AvailableReplicas == replicas +} + +func (r *InferenceServiceReconciler) ownedActivePods(ctx context.Context, deployment *appsv1.Deployment) ([]*corev1.Pod, error) { + listOpts := []client.ListOption{client.InNamespace(deployment.Namespace)} + if deployment.Spec.Selector != nil { + listOpts = append(listOpts, client.MatchingLabels(deployment.Spec.Selector.MatchLabels)) + } + replicaSets := &appsv1.ReplicaSetList{} + if err := r.List(ctx, replicaSets, listOpts...); err != nil { + return nil, err + } + ownedRS := make(map[types.UID]struct{}) + for i := range replicaSets.Items { + if controlledBy(&replicaSets.Items[i], deployment.UID) { + ownedRS[replicaSets.Items[i].UID] = struct{}{} + } + } + if len(ownedRS) == 0 { + return nil, nil + } + pods := &corev1.PodList{} + if err := r.List(ctx, pods, listOpts...); err != nil { + return nil, err + } + owned := make([]*corev1.Pod, 0, len(pods.Items)) + for i := range pods.Items { + pod := &pods.Items[i] + if _, ok := ownedRS[controllerUID(pod)]; !ok || pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed { + continue + } + owned = append(owned, pod) + } + return owned, nil +} + +func allPodsReady(pods []*corev1.Pod) bool { + for _, pod := range pods { + if pod.DeletionTimestamp != nil || pod.Status.StartTime == nil || !podReady(pod) { + return false + } + } + return true +} + +func podLifetime(seconds int64) time.Duration { + if seconds <= 0 { + return 0 + } + if seconds > maxPodLifetimeSeconds { + seconds = maxPodLifetimeSeconds + } + return time.Duration(seconds) * time.Second +} + +func controlledBy(obj metav1.Object, uid types.UID) bool { + for _, ref := range obj.GetOwnerReferences() { + if ref.Controller != nil && *ref.Controller && ref.UID == uid { + return true + } + } + return false +} + +func controllerUID(obj metav1.Object) types.UID { + for _, ref := range obj.GetOwnerReferences() { + if ref.Controller != nil && *ref.Controller { + return ref.UID + } + } + return "" +} + +func podReady(pod *corev1.Pod) bool { + for _, condition := range pod.Status.Conditions { + if condition.Type == corev1.PodReady { + return condition.Status == corev1.ConditionTrue + } + } + return false +} diff --git a/internal/controller/pod_lifetime_test.go b/internal/controller/pod_lifetime_test.go new file mode 100644 index 00000000..ea8ae8fb --- /dev/null +++ b/internal/controller/pod_lifetime_test.go @@ -0,0 +1,317 @@ +package controller + +import ( + "context" + "testing" + "time" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + inferencev1alpha1 "github.com/defilantech/llmkube/api/v1alpha1" +) + +func TestReconcilePodLifetimeRequeuesUnexpiredPod(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + start := now.Add(-5 * time.Second) + r, isvc, pod := lifetimeReconciler(t, start, 30*time.Second) + + requeue, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now) + if err != nil { + t.Fatal(err) + } + if want := 25 * time.Second; requeue != want { + t.Fatalf("requeue = %s, want %s", requeue, want) + } + if err := r.Get(context.Background(), client.ObjectKeyFromObject(pod), &corev1.Pod{}); err != nil { + t.Fatalf("unexpired pod was deleted: %v", err) + } +} + +func TestReconcilePodLifetimeDeletesOldestExpiredPod(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + r, isvc, first := lifetimeReconcilerReplicas(t, now.Add(-2*time.Minute), time.Minute, 2) + second := first.DeepCopy() + second.Name = "second" + second.UID = "pod-second" + second.ResourceVersion = "" + second.Status.StartTime = &metav1.Time{Time: now.Add(-90 * time.Second)} + if err := r.Create(context.Background(), second); err != nil { + t.Fatal(err) + } + recorder := &recordingClient{Client: r.Client} + r.Client = recorder + if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + t.Fatal(err) + } + if len(recorder.deleteOpts) != 1 { + t.Fatalf("eviction calls = %d", len(recorder.deleteOpts)) + } + if err := r.Get(context.Background(), client.ObjectKeyFromObject(second), &corev1.Pod{}); err != nil { + t.Fatalf("second pod was deleted too: %v", err) + } +} + +func TestReconcilePodLifetimeIgnoresTerminalPods(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + r, isvc, active := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) + terminal := active.DeepCopy() + terminal.Name = "succeeded" + terminal.UID = "pod-succeeded" + terminal.ResourceVersion = "" + terminal.Status.Phase = corev1.PodSucceeded + terminal.Status.StartTime = nil + if err := r.Create(context.Background(), terminal); err != nil { + t.Fatal(err) + } + recorder := &recordingClient{Client: r.Client} + r.Client = recorder + if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + t.Fatal(err) + } + if len(recorder.deleteOpts) != 1 { + t.Fatalf("eviction calls = %d, want 1", len(recorder.deleteOpts)) + } +} + +func TestReconcilePodLifetimeNoopWhenUnsetOrMetal(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) + isvc.Spec.MaxPodLifetimeSeconds = nil + if got, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil || got != 0 { + t.Fatalf("unset lifetime returned %s, %v", got, err) + } + seconds := int64(60) + isvc.Spec.MaxPodLifetimeSeconds = &seconds + if got, err := r.reconcilePodLifetimeAt(context.Background(), isvc, true, now); err != nil || got != 0 { + t.Fatalf("metal lifetime returned %s, %v", got, err) + } +} + +func TestReconcilePodLifetimeBlocksUnstablePodsAndDeployment(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + for name, mutate := range map[string]func(*appsv1.Deployment, *corev1.Pod){ + "terminating": func(_ *appsv1.Deployment, p *corev1.Pod) { p.DeletionTimestamp = &metav1.Time{Time: now} }, + "unready": func(_ *appsv1.Deployment, p *corev1.Pod) { p.Status.Conditions[0].Status = corev1.ConditionFalse }, + "no start": func(_ *appsv1.Deployment, p *corev1.Pod) { p.Status.StartTime = nil }, + "unobserved": func(d *appsv1.Deployment, _ *corev1.Pod) { d.Status.ObservedGeneration = d.Generation - 1 }, + "replicas": func(d *appsv1.Deployment, _ *corev1.Pod) { d.Status.Replicas = 0 }, + "updated": func(d *appsv1.Deployment, _ *corev1.Pod) { d.Status.UpdatedReplicas = 0 }, + "ready": func(d *appsv1.Deployment, _ *corev1.Pod) { d.Status.ReadyReplicas = 0 }, + "available": func(d *appsv1.Deployment, _ *corev1.Pod) { d.Status.AvailableReplicas = 0 }, + } { + t.Run(name, func(t *testing.T) { + var podMutator func(*corev1.Pod) + if name == "terminating" { + podMutator = func(p *corev1.Pod) { + p.DeletionTimestamp = &metav1.Time{Time: now} + p.Finalizers = []string{"test/finalizer"} + } + } + r, isvc, pod := lifetimeReconcilerWith(t, now.Add(-2*time.Minute), time.Minute, podMutator) + deployment := &appsv1.Deployment{} + if err := r.Get(context.Background(), types.NamespacedName{Name: "svc", Namespace: "ns"}, deployment); err != nil { + t.Fatal(err) + } + mutate(deployment, pod) + if err := r.Status().Update(context.Background(), deployment); err != nil { + t.Fatal(err) + } + if err := r.Status().Update(context.Background(), pod); err != nil { + t.Fatal(err) + } + if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + t.Fatal(err) + } + if err := r.Get(context.Background(), client.ObjectKeyFromObject(pod), &corev1.Pod{}); err != nil { + t.Fatalf("pod was deleted: %v", err) + } + }) + } +} + +func TestReconcilePodLifetimeIgnoresForeignOwnership(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + r, isvc, pod := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) + foreign := pod.DeepCopy() + foreign.Name = "foreign" + foreign.UID = "foreign-uid" + foreign.ResourceVersion = "" + foreign.OwnerReferences[0].UID = "other-rs" + foreignRS := &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{Name: "foreign-rs", Namespace: "ns", UID: "other-rs", Labels: map[string]string{"app": "svc"}, OwnerReferences: []metav1.OwnerReference{{UID: "other-deployment", Controller: boolPointer(true)}}}} + if err := r.Create(context.Background(), foreignRS); err != nil { + t.Fatal(err) + } + if err := r.Create(context.Background(), foreign); err != nil { + t.Fatal(err) + } + if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + t.Fatal(err) + } + if err := r.Get(context.Background(), client.ObjectKeyFromObject(pod), &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("owned pod was not deleted: %v", err) + } + if err := r.Get(context.Background(), client.ObjectKeyFromObject(foreign), &corev1.Pod{}); err != nil { + t.Fatalf("foreign pod was deleted: %v", err) + } +} + +func boolPointer(value bool) *bool { return &value } + +func TestEarliestPositive(t *testing.T) { + if got := earliestPositive(0, 5*time.Second, 2*time.Second, -time.Second); got != 2*time.Second { + t.Fatalf("got %s", got) + } + if got := earliestPositive(0, -time.Second); got != 0 { + t.Fatalf("got %s", got) + } +} + +type recordingClient struct { + client.Client + deleteOpts []client.DeleteOption + deleteErr error +} + +func (c *recordingClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { + c.deleteOpts = append(c.deleteOpts, opts...) + if c.deleteErr != nil { + return c.deleteErr + } + return c.Client.Delete(ctx, obj, opts...) +} + +func (c *recordingClient) SubResource(name string) client.SubResourceClient { + return &recordingSubResource{SubResourceClient: c.Client.SubResource(name), parent: c} +} + +type recordingSubResource struct { + client.SubResourceClient + parent *recordingClient +} + +func (s *recordingSubResource) Create(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error { + if options, ok := subResource.(*policyv1.Eviction); ok { + s.parent.deleteOpts = append(s.parent.deleteOpts, evictionDeleteOption{options.DeleteOptions}) + if s.parent.deleteErr != nil { + return s.parent.deleteErr + } + } + return s.SubResourceClient.Create(ctx, obj, subResource, opts...) +} + +type evictionDeleteOption struct{ options *metav1.DeleteOptions } + +func (o evictionDeleteOption) ApplyToDelete(target *client.DeleteOptions) { + if o.options != nil { + target.Preconditions = o.options.Preconditions + target.GracePeriodSeconds = o.options.GracePeriodSeconds + } +} + +func TestReconcilePodLifetimeDeleteOptions(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) + recorder := &recordingClient{Client: r.Client} + r.Client = recorder + if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + t.Fatal(err) + } + if len(recorder.deleteOpts) != 1 { + t.Fatalf("delete calls = %d", len(recorder.deleteOpts)) + } + got := &client.DeleteOptions{} + recorder.deleteOpts[0].ApplyToDelete(got) + if got.GracePeriodSeconds != nil { + t.Fatalf("grace period = %v", *got.GracePeriodSeconds) + } + if got.Preconditions == nil || got.Preconditions.UID == nil || *got.Preconditions.UID != "pod-first" { + t.Fatalf("missing UID precondition: %#v", got.Preconditions) + } +} + +func TestReconcilePodLifetimeNotFoundDeleteIsSuccess(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) + recorder := &recordingClient{Client: r.Client, deleteErr: apierrors.NewNotFound(corev1.Resource("pods"), "first")} + r.Client = recorder + if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + t.Fatal(err) + } +} + +func TestReconcilePodLifetimePDBRetry(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) + recorder := &recordingClient{Client: r.Client, deleteErr: apierrors.NewTooManyRequests("pdb", 1)} + r.Client = recorder + got, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now) + if err != nil { + t.Fatal(err) + } + if got != podLifetimeRetry { + t.Fatalf("requeue = %s, want %s", got, podLifetimeRetry) + } +} + +func TestPodLifetimeBoundsDuration(t *testing.T) { + if got := podLifetime(maxPodLifetimeSeconds); got != time.Duration(maxPodLifetimeSeconds)*time.Second { + t.Fatalf("duration = %s", got) + } + if got := podLifetime(maxPodLifetimeSeconds + 1); got != time.Duration(maxPodLifetimeSeconds)*time.Second { + t.Fatalf("overflow duration = %s", got) + } +} + +func lifetimeReconciler(t *testing.T, start time.Time, lifetime time.Duration) (*InferenceServiceReconciler, *inferencev1alpha1.InferenceService, *corev1.Pod) { + return lifetimeReconcilerReplicas(t, start, lifetime, 1) +} + +func lifetimeReconcilerWith(t *testing.T, start time.Time, lifetime time.Duration, podMutator func(*corev1.Pod)) (*InferenceServiceReconciler, *inferencev1alpha1.InferenceService, *corev1.Pod) { + return lifetimeReconcilerReplicasWith(t, start, lifetime, 1, podMutator) +} + +func lifetimeReconcilerReplicas(t *testing.T, start time.Time, lifetime time.Duration, replicaCount int32) (*InferenceServiceReconciler, *inferencev1alpha1.InferenceService, *corev1.Pod) { + return lifetimeReconcilerReplicasWith(t, start, lifetime, replicaCount, nil) +} + +func lifetimeReconcilerReplicasWith(t *testing.T, start time.Time, lifetime time.Duration, replicaCount int32, podMutator func(*corev1.Pod)) (*InferenceServiceReconciler, *inferencev1alpha1.InferenceService, *corev1.Pod) { + t.Helper() + const ( + isvcUID = types.UID("isvc-uid") + depUID = types.UID("deployment-uid") + rsUID = types.UID("rs-uid") + ) + controller := true + replicas := replicaCount + seconds := int64(lifetime / time.Second) + isvc := &inferencev1alpha1.InferenceService{ObjectMeta: metav1.ObjectMeta{Name: "svc", Namespace: "ns", UID: isvcUID}, Spec: inferencev1alpha1.InferenceServiceSpec{MaxPodLifetimeSeconds: &seconds}} + deployment := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "svc", Namespace: "ns", UID: depUID, Generation: 2, OwnerReferences: []metav1.OwnerReference{{UID: isvcUID, Controller: &controller}}}, Spec: appsv1.DeploymentSpec{Replicas: &replicas, Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "svc"}}}, Status: appsv1.DeploymentStatus{ObservedGeneration: 2, Replicas: replicaCount, UpdatedReplicas: replicaCount, ReadyReplicas: replicaCount, AvailableReplicas: replicaCount}} + rs := &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{Name: "svc-rs", Namespace: "ns", UID: rsUID, Labels: map[string]string{"app": "svc"}, OwnerReferences: []metav1.OwnerReference{{UID: depUID, Controller: &controller}}}} + ready := corev1.ConditionTrue + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "first", Namespace: "ns", UID: "pod-first", Labels: map[string]string{"app": "svc"}, OwnerReferences: []metav1.OwnerReference{{UID: rsUID, Controller: &controller}}}, Status: corev1.PodStatus{StartTime: &metav1.Time{Time: start}, Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: ready}}}} + if podMutator != nil { + podMutator(pod) + } + scheme := runtime.NewScheme() + if err := inferencev1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := appsv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := policyv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + return &InferenceServiceReconciler{Client: fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(deployment, pod).WithObjects(isvc, deployment, rs, pod).Build()}, isvc, pod +} From f63491c79db99fe0be5dabf9b9fbfe0570f4f9aa Mon Sep 17 00:00:00 2001 From: Tanguille <91473554+Tanguille@users.noreply.github.com> Date: Thu, 23 Jul 2026 01:56:19 +0200 Subject: [PATCH 2/5] refactor(controller): simplify pod lifetime recycling Quality cleanups over the recycling loop, no behaviour change: - use metav1.IsControlledBy / GetControllerOfNoCopy instead of hand-rolled owner-reference walks - drop stableDeployment's unread int32 return and activePodCountMatches; the stability gate already proves Status.Replicas is the desired count - replace the candidate struct + sort with a single-pass minimum - collapse the reconcilePodLifetime/reconcilePodLifetimeAt pair into one function taking now, and flatten the if/else at the call site - make evictPod a reconciler method and drop the UID temporary - record evictions directly in the test client instead of round-tripping them through a fake client.DeleteOption, and fold four fixture constructors into two Signed-off-by: Tanguille <91473554+Tanguille@users.noreply.github.com> --- .../controller/inferenceservice_controller.go | 6 +- .../inferenceservice_deployment_test.go | 17 +--- internal/controller/pod_lifetime.go | 94 +++++++----------- internal/controller/pod_lifetime_test.go | 95 +++++++------------ 4 files changed, 75 insertions(+), 137 deletions(-) diff --git a/internal/controller/inferenceservice_controller.go b/internal/controller/inferenceservice_controller.go index f49050ff..01c8c7fc 100644 --- a/internal/controller/inferenceservice_controller.go +++ b/internal/controller/inferenceservice_controller.go @@ -250,11 +250,11 @@ func (r *InferenceServiceReconciler) Reconcile(ctx context.Context, req ctrl.Req return finalResult, statusErr } - if requeueAfter, err := r.reconcilePodLifetime(ctx, inferenceService, isMetal); err != nil { + lifetimeRequeue, err := r.reconcilePodLifetime(ctx, inferenceService, isMetal, time.Now()) + if err != nil { return finalResult, err - } else { - finalResult.RequeueAfter = earliestPositive(finalResult.RequeueAfter, requeueAfter) } + finalResult.RequeueAfter = earliestPositive(finalResult.RequeueAfter, lifetimeRequeue) // When a rollout is deferred pending idle, reconcileRolloutPolicy set // RolloutDeferred=True (persisted by the status update above). Drive a diff --git a/internal/controller/inferenceservice_deployment_test.go b/internal/controller/inferenceservice_deployment_test.go index a72e784c..dd2298ab 100644 --- a/internal/controller/inferenceservice_deployment_test.go +++ b/internal/controller/inferenceservice_deployment_test.go @@ -594,21 +594,8 @@ var _ = Describe("Multi-GPU Deployment Construction", func() { } }) - It("should leave ActiveDeadlineSeconds nil when maxPodLifetimeSeconds is unset", func() { - isvc := &inferencev1alpha1.InferenceService{ - ObjectMeta: metav1.ObjectMeta{ - Name: "lifetime-service", - Namespace: "default", - }, - Spec: inferencev1alpha1.InferenceServiceSpec{ - ModelRef: "lifetime-model", - Image: "ghcr.io/ggml-org/llama.cpp:server", - }, - } - deployment := reconciler.constructDeployment(isvc, model, 1) - Expect(deployment.Spec.Template.Spec.ActiveDeadlineSeconds).To(BeNil()) - }) - + // Recycling is driven by the controller, not the pod template: the + // field must stay nil so the Deployment remains updatable. It("should leave ActiveDeadlineSeconds nil when maxPodLifetimeSeconds is set", func() { lifetime := int64(3600) isvc := &inferencev1alpha1.InferenceService{ diff --git a/internal/controller/pod_lifetime.go b/internal/controller/pod_lifetime.go index b5248abe..5535adf6 100644 --- a/internal/controller/pod_lifetime.go +++ b/internal/controller/pod_lifetime.go @@ -2,7 +2,6 @@ package controller import ( "context" - "sort" "time" appsv1 "k8s.io/api/apps/v1" @@ -36,11 +35,8 @@ func earliestPositive(values ...time.Duration) time.Duration { // reconcilePodLifetime recycles at most one healthy, expired pod. All // workload checks are deliberately conservative: a watch will cause another // attempt after the Deployment has settled or a replacement pod is ready. -func (r *InferenceServiceReconciler) reconcilePodLifetime(ctx context.Context, isvc *inferencev1alpha1.InferenceService, isMetal bool) (time.Duration, error) { - return r.reconcilePodLifetimeAt(ctx, isvc, isMetal, time.Now()) -} - -func (r *InferenceServiceReconciler) reconcilePodLifetimeAt(ctx context.Context, isvc *inferencev1alpha1.InferenceService, isMetal bool, now time.Time) (time.Duration, error) { +// now is injected so tests can drive the deadline arithmetic. +func (r *InferenceServiceReconciler) reconcilePodLifetime(ctx context.Context, isvc *inferencev1alpha1.InferenceService, isMetal bool, now time.Time) (time.Duration, error) { if isMetal || isvc.Spec.MaxPodLifetimeSeconds == nil { return 0, nil } @@ -53,7 +49,9 @@ func (r *InferenceServiceReconciler) reconcilePodLifetimeAt(ctx context.Context, if err != nil { return 0, err } - if !activePodCountMatches(deployment, owned) || !allPodsReady(owned) { + // stableDeployment already proved Status.Replicas is the desired count, so + // it doubles as the expected number of active pods. + if len(owned) != int(deployment.Status.Replicas) || !allPodsReady(owned) { return 0, nil } return r.recycleExpiredPod(ctx, owned, podLifetime(*isvc.Spec.MaxPodLifetimeSeconds), now) @@ -68,51 +66,38 @@ func (r *InferenceServiceReconciler) getStableDeployment(ctx context.Context, is if err != nil { return nil, err } - _, stable := stableDeployment(deployment, isvc.UID) - if !stable { + if !stableDeployment(deployment, isvc) { return nil, nil } return deployment, nil } -func activePodCountMatches(deployment *appsv1.Deployment, pods []*corev1.Pod) bool { - replicas := int32(1) - if deployment.Spec.Replicas != nil { - replicas = *deployment.Spec.Replicas - } - return int64(len(pods)) == int64(replicas) -} - +// recycleExpiredPod evicts the pod that expired first, or reports how long +// until the next one does. Ties break on name so repeated reconciles of the +// same state pick the same pod. func (r *InferenceServiceReconciler) recycleExpiredPod(ctx context.Context, owned []*corev1.Pod, lifetime time.Duration, now time.Time) (time.Duration, error) { - type candidate struct { - pod *corev1.Pod - deadline time.Time - } - candidates := make([]candidate, 0, len(owned)) + var expired *corev1.Pod + var expiredDeadline time.Time var earliest time.Duration for _, pod := range owned { deadline := pod.Status.StartTime.Time.Add(lifetime) if deadline.After(now) { earliest = earliestPositive(earliest, deadline.Sub(now)) - } else { - candidates = append(candidates, candidate{pod: pod, deadline: deadline}) + continue + } + if expired == nil || deadline.Before(expiredDeadline) || + (deadline.Equal(expiredDeadline) && pod.Name < expired.Name) { + expired, expiredDeadline = pod, deadline } } - if len(candidates) == 0 { + if expired == nil { return earliest, nil } - sort.Slice(candidates, func(i, j int) bool { - if candidates[i].deadline.Equal(candidates[j].deadline) { - return candidates[i].pod.Name < candidates[j].pod.Name - } - return candidates[i].deadline.Before(candidates[j].deadline) - }) - return evictPod(ctx, r.Client, candidates[0].pod) + return r.evictPod(ctx, expired) } -func evictPod(ctx context.Context, r client.Client, pod *corev1.Pod) (time.Duration, error) { - uid := pod.UID - eviction := &policyv1.Eviction{ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}, DeleteOptions: &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &uid}}} +func (r *InferenceServiceReconciler) evictPod(ctx context.Context, pod *corev1.Pod) (time.Duration, error) { + eviction := &policyv1.Eviction{ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}, DeleteOptions: &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &pod.UID}}} err := r.SubResource("eviction").Create(ctx, pod, eviction) switch { case apierrors.IsNotFound(err): @@ -124,15 +109,15 @@ func evictPod(ctx context.Context, r client.Client, pod *corev1.Pod) (time.Durat } } -func stableDeployment(deployment *appsv1.Deployment, isvcUID types.UID) (int32, bool) { - if !controlledBy(deployment, isvcUID) || deployment.Generation == 0 || deployment.Status.ObservedGeneration != deployment.Generation { - return 0, false +func stableDeployment(deployment *appsv1.Deployment, isvc *inferencev1alpha1.InferenceService) bool { + if !metav1.IsControlledBy(deployment, isvc) || deployment.Generation == 0 || deployment.Status.ObservedGeneration != deployment.Generation { + return false } replicas := int32(1) if deployment.Spec.Replicas != nil { replicas = *deployment.Spec.Replicas } - return replicas, deployment.Status.Replicas == replicas && deployment.Status.UpdatedReplicas == replicas && deployment.Status.ReadyReplicas == replicas && deployment.Status.AvailableReplicas == replicas + return deployment.Status.Replicas == replicas && deployment.Status.UpdatedReplicas == replicas && deployment.Status.ReadyReplicas == replicas && deployment.Status.AvailableReplicas == replicas } func (r *InferenceServiceReconciler) ownedActivePods(ctx context.Context, deployment *appsv1.Deployment) ([]*corev1.Pod, error) { @@ -146,7 +131,7 @@ func (r *InferenceServiceReconciler) ownedActivePods(ctx context.Context, deploy } ownedRS := make(map[types.UID]struct{}) for i := range replicaSets.Items { - if controlledBy(&replicaSets.Items[i], deployment.UID) { + if metav1.IsControlledBy(&replicaSets.Items[i], deployment) { ownedRS[replicaSets.Items[i].UID] = struct{}{} } } @@ -160,7 +145,14 @@ func (r *InferenceServiceReconciler) ownedActivePods(ctx context.Context, deploy owned := make([]*corev1.Pod, 0, len(pods.Items)) for i := range pods.Items { pod := &pods.Items[i] - if _, ok := ownedRS[controllerUID(pod)]; !ok || pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed { + if pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed { + continue + } + ref := metav1.GetControllerOfNoCopy(pod) + if ref == nil { + continue + } + if _, ok := ownedRS[ref.UID]; !ok { continue } owned = append(owned, pod) @@ -181,30 +173,14 @@ func podLifetime(seconds int64) time.Duration { if seconds <= 0 { return 0 } + // The CRD has no Maximum, so an unbounded value would overflow Duration + // into the past and turn recycling into an eviction loop. if seconds > maxPodLifetimeSeconds { seconds = maxPodLifetimeSeconds } return time.Duration(seconds) * time.Second } -func controlledBy(obj metav1.Object, uid types.UID) bool { - for _, ref := range obj.GetOwnerReferences() { - if ref.Controller != nil && *ref.Controller && ref.UID == uid { - return true - } - } - return false -} - -func controllerUID(obj metav1.Object) types.UID { - for _, ref := range obj.GetOwnerReferences() { - if ref.Controller != nil && *ref.Controller { - return ref.UID - } - } - return "" -} - func podReady(pod *corev1.Pod) bool { for _, condition := range pod.Status.Conditions { if condition.Type == corev1.PodReady { diff --git a/internal/controller/pod_lifetime_test.go b/internal/controller/pod_lifetime_test.go index ea8ae8fb..afb37d49 100644 --- a/internal/controller/pod_lifetime_test.go +++ b/internal/controller/pod_lifetime_test.go @@ -12,6 +12,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -23,7 +24,7 @@ func TestReconcilePodLifetimeRequeuesUnexpiredPod(t *testing.T) { start := now.Add(-5 * time.Second) r, isvc, pod := lifetimeReconciler(t, start, 30*time.Second) - requeue, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now) + requeue, err := r.reconcilePodLifetime(context.Background(), isvc, false, now) if err != nil { t.Fatal(err) } @@ -37,7 +38,7 @@ func TestReconcilePodLifetimeRequeuesUnexpiredPod(t *testing.T) { func TestReconcilePodLifetimeDeletesOldestExpiredPod(t *testing.T) { now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) - r, isvc, first := lifetimeReconcilerReplicas(t, now.Add(-2*time.Minute), time.Minute, 2) + r, isvc, first := lifetimeReconcilerWith(t, now.Add(-2*time.Minute), time.Minute, 2, nil) second := first.DeepCopy() second.Name = "second" second.UID = "pod-second" @@ -48,11 +49,11 @@ func TestReconcilePodLifetimeDeletesOldestExpiredPod(t *testing.T) { } recorder := &recordingClient{Client: r.Client} r.Client = recorder - if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + if _, err := r.reconcilePodLifetime(context.Background(), isvc, false, now); err != nil { t.Fatal(err) } - if len(recorder.deleteOpts) != 1 { - t.Fatalf("eviction calls = %d", len(recorder.deleteOpts)) + if len(recorder.evictions) != 1 { + t.Fatalf("eviction calls = %d", len(recorder.evictions)) } if err := r.Get(context.Background(), client.ObjectKeyFromObject(second), &corev1.Pod{}); err != nil { t.Fatalf("second pod was deleted too: %v", err) @@ -73,11 +74,11 @@ func TestReconcilePodLifetimeIgnoresTerminalPods(t *testing.T) { } recorder := &recordingClient{Client: r.Client} r.Client = recorder - if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + if _, err := r.reconcilePodLifetime(context.Background(), isvc, false, now); err != nil { t.Fatal(err) } - if len(recorder.deleteOpts) != 1 { - t.Fatalf("eviction calls = %d, want 1", len(recorder.deleteOpts)) + if len(recorder.evictions) != 1 { + t.Fatalf("eviction calls = %d, want 1", len(recorder.evictions)) } } @@ -85,12 +86,12 @@ func TestReconcilePodLifetimeNoopWhenUnsetOrMetal(t *testing.T) { now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) isvc.Spec.MaxPodLifetimeSeconds = nil - if got, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil || got != 0 { + if got, err := r.reconcilePodLifetime(context.Background(), isvc, false, now); err != nil || got != 0 { t.Fatalf("unset lifetime returned %s, %v", got, err) } seconds := int64(60) isvc.Spec.MaxPodLifetimeSeconds = &seconds - if got, err := r.reconcilePodLifetimeAt(context.Background(), isvc, true, now); err != nil || got != 0 { + if got, err := r.reconcilePodLifetime(context.Background(), isvc, true, now); err != nil || got != 0 { t.Fatalf("metal lifetime returned %s, %v", got, err) } } @@ -115,7 +116,7 @@ func TestReconcilePodLifetimeBlocksUnstablePodsAndDeployment(t *testing.T) { p.Finalizers = []string{"test/finalizer"} } } - r, isvc, pod := lifetimeReconcilerWith(t, now.Add(-2*time.Minute), time.Minute, podMutator) + r, isvc, pod := lifetimeReconcilerWith(t, now.Add(-2*time.Minute), time.Minute, 1, podMutator) deployment := &appsv1.Deployment{} if err := r.Get(context.Background(), types.NamespacedName{Name: "svc", Namespace: "ns"}, deployment); err != nil { t.Fatal(err) @@ -127,7 +128,7 @@ func TestReconcilePodLifetimeBlocksUnstablePodsAndDeployment(t *testing.T) { if err := r.Status().Update(context.Background(), pod); err != nil { t.Fatal(err) } - if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + if _, err := r.reconcilePodLifetime(context.Background(), isvc, false, now); err != nil { t.Fatal(err) } if err := r.Get(context.Background(), client.ObjectKeyFromObject(pod), &corev1.Pod{}); err != nil { @@ -145,14 +146,14 @@ func TestReconcilePodLifetimeIgnoresForeignOwnership(t *testing.T) { foreign.UID = "foreign-uid" foreign.ResourceVersion = "" foreign.OwnerReferences[0].UID = "other-rs" - foreignRS := &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{Name: "foreign-rs", Namespace: "ns", UID: "other-rs", Labels: map[string]string{"app": "svc"}, OwnerReferences: []metav1.OwnerReference{{UID: "other-deployment", Controller: boolPointer(true)}}}} + foreignRS := &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{Name: "foreign-rs", Namespace: "ns", UID: "other-rs", Labels: map[string]string{"app": "svc"}, OwnerReferences: []metav1.OwnerReference{{UID: "other-deployment", Controller: ptr.To(true)}}}} if err := r.Create(context.Background(), foreignRS); err != nil { t.Fatal(err) } if err := r.Create(context.Background(), foreign); err != nil { t.Fatal(err) } - if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + if _, err := r.reconcilePodLifetime(context.Background(), isvc, false, now); err != nil { t.Fatal(err) } if err := r.Get(context.Background(), client.ObjectKeyFromObject(pod), &corev1.Pod{}); !apierrors.IsNotFound(err) { @@ -163,8 +164,6 @@ func TestReconcilePodLifetimeIgnoresForeignOwnership(t *testing.T) { } } -func boolPointer(value bool) *bool { return &value } - func TestEarliestPositive(t *testing.T) { if got := earliestPositive(0, 5*time.Second, 2*time.Second, -time.Second); got != 2*time.Second { t.Fatalf("got %s", got) @@ -174,18 +173,12 @@ func TestEarliestPositive(t *testing.T) { } } +// recordingClient captures the evictions the controller submits and can fail +// them, so tests can assert on the request the apiserver would have seen. type recordingClient struct { client.Client - deleteOpts []client.DeleteOption - deleteErr error -} - -func (c *recordingClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { - c.deleteOpts = append(c.deleteOpts, opts...) - if c.deleteErr != nil { - return c.deleteErr - } - return c.Client.Delete(ctx, obj, opts...) + evictions []*policyv1.Eviction + evictErr error } func (c *recordingClient) SubResource(name string) client.SubResourceClient { @@ -198,39 +191,29 @@ type recordingSubResource struct { } func (s *recordingSubResource) Create(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error { - if options, ok := subResource.(*policyv1.Eviction); ok { - s.parent.deleteOpts = append(s.parent.deleteOpts, evictionDeleteOption{options.DeleteOptions}) - if s.parent.deleteErr != nil { - return s.parent.deleteErr + if eviction, ok := subResource.(*policyv1.Eviction); ok { + s.parent.evictions = append(s.parent.evictions, eviction) + if s.parent.evictErr != nil { + return s.parent.evictErr } } return s.SubResourceClient.Create(ctx, obj, subResource, opts...) } -type evictionDeleteOption struct{ options *metav1.DeleteOptions } - -func (o evictionDeleteOption) ApplyToDelete(target *client.DeleteOptions) { - if o.options != nil { - target.Preconditions = o.options.Preconditions - target.GracePeriodSeconds = o.options.GracePeriodSeconds - } -} - func TestReconcilePodLifetimeDeleteOptions(t *testing.T) { now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) recorder := &recordingClient{Client: r.Client} r.Client = recorder - if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + if _, err := r.reconcilePodLifetime(context.Background(), isvc, false, now); err != nil { t.Fatal(err) } - if len(recorder.deleteOpts) != 1 { - t.Fatalf("delete calls = %d", len(recorder.deleteOpts)) + if len(recorder.evictions) != 1 { + t.Fatalf("eviction calls = %d", len(recorder.evictions)) } - got := &client.DeleteOptions{} - recorder.deleteOpts[0].ApplyToDelete(got) - if got.GracePeriodSeconds != nil { - t.Fatalf("grace period = %v", *got.GracePeriodSeconds) + got := recorder.evictions[0].DeleteOptions + if got == nil || got.GracePeriodSeconds != nil { + t.Fatalf("delete options did not preserve graceful termination: %#v", got) } if got.Preconditions == nil || got.Preconditions.UID == nil || *got.Preconditions.UID != "pod-first" { t.Fatalf("missing UID precondition: %#v", got.Preconditions) @@ -240,9 +223,9 @@ func TestReconcilePodLifetimeDeleteOptions(t *testing.T) { func TestReconcilePodLifetimeNotFoundDeleteIsSuccess(t *testing.T) { now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) - recorder := &recordingClient{Client: r.Client, deleteErr: apierrors.NewNotFound(corev1.Resource("pods"), "first")} + recorder := &recordingClient{Client: r.Client, evictErr: apierrors.NewNotFound(corev1.Resource("pods"), "first")} r.Client = recorder - if _, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now); err != nil { + if _, err := r.reconcilePodLifetime(context.Background(), isvc, false, now); err != nil { t.Fatal(err) } } @@ -250,9 +233,9 @@ func TestReconcilePodLifetimeNotFoundDeleteIsSuccess(t *testing.T) { func TestReconcilePodLifetimePDBRetry(t *testing.T) { now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) - recorder := &recordingClient{Client: r.Client, deleteErr: apierrors.NewTooManyRequests("pdb", 1)} + recorder := &recordingClient{Client: r.Client, evictErr: apierrors.NewTooManyRequests("pdb", 1)} r.Client = recorder - got, err := r.reconcilePodLifetimeAt(context.Background(), isvc, false, now) + got, err := r.reconcilePodLifetime(context.Background(), isvc, false, now) if err != nil { t.Fatal(err) } @@ -271,18 +254,10 @@ func TestPodLifetimeBoundsDuration(t *testing.T) { } func lifetimeReconciler(t *testing.T, start time.Time, lifetime time.Duration) (*InferenceServiceReconciler, *inferencev1alpha1.InferenceService, *corev1.Pod) { - return lifetimeReconcilerReplicas(t, start, lifetime, 1) -} - -func lifetimeReconcilerWith(t *testing.T, start time.Time, lifetime time.Duration, podMutator func(*corev1.Pod)) (*InferenceServiceReconciler, *inferencev1alpha1.InferenceService, *corev1.Pod) { - return lifetimeReconcilerReplicasWith(t, start, lifetime, 1, podMutator) -} - -func lifetimeReconcilerReplicas(t *testing.T, start time.Time, lifetime time.Duration, replicaCount int32) (*InferenceServiceReconciler, *inferencev1alpha1.InferenceService, *corev1.Pod) { - return lifetimeReconcilerReplicasWith(t, start, lifetime, replicaCount, nil) + return lifetimeReconcilerWith(t, start, lifetime, 1, nil) } -func lifetimeReconcilerReplicasWith(t *testing.T, start time.Time, lifetime time.Duration, replicaCount int32, podMutator func(*corev1.Pod)) (*InferenceServiceReconciler, *inferencev1alpha1.InferenceService, *corev1.Pod) { +func lifetimeReconcilerWith(t *testing.T, start time.Time, lifetime time.Duration, replicaCount int32, podMutator func(*corev1.Pod)) (*InferenceServiceReconciler, *inferencev1alpha1.InferenceService, *corev1.Pod) { t.Helper() const ( isvcUID = types.UID("isvc-uid") From e555158c151c8fb2cec6a82c2a6d8ff946f35e03 Mon Sep 17 00:00:00 2001 From: Tanguille <91473554+Tanguille@users.noreply.github.com> Date: Thu, 23 Jul 2026 02:07:56 +0200 Subject: [PATCH 3/5] fix(controller): gate pod recycling on idle and drop the ReplicaSet walk Follow-up to the recycling loop, taking the behaviour changes the review surfaced: - honour rolloutPolicy.waitForIdle before evicting. waitForIdle promises never to drop in-flight generations; recycling was killing a busy pod every maxPodLifetimeSeconds regardless. Recycling is best-effort and has no deadline to race, so it waits for the next idle window instead of borrowing the rollout path's timeout budget, and fails closed when the idle probe cannot answer. - emit a PodRecycled event and log the PDB-blocked retry. A pod disappearing on a timer left no trace in `kubectl describe isvc`. - list pods by the Deployment selector instead of walking ReplicaSets to prove ownership. The selector labels are unique per InferenceService and the stability gate already cross-checks the count against Status.Replicas, so a foreign pod now holds recycling instead of being filtered out. Drops a cluster-wide ReplicaSet informer and the apps/replicasets RBAC rule. - move earliestPositive next to Reconcile, where its three other callers live. - document that single-replica recycling is a restart with a downtime window, not a rolling replacement. Signed-off-by: Tanguille <91473554+Tanguille@users.noreply.github.com> --- api/v1alpha1/inferenceservice_types.go | 17 ++-- charts/llmkube/templates/clusterrole.yaml | 7 -- .../templates/crds/inferenceservices.yaml | 17 ++-- ...ference.llmkube.dev_inferenceservices.yaml | 17 ++-- config/rbac/role.yaml | 7 -- .../controller/inferenceservice_controller.go | 13 ++- internal/controller/pod_lifetime.go | 89 +++++++++---------- internal/controller/pod_lifetime_test.go | 63 ++++++++++--- 8 files changed, 136 insertions(+), 94 deletions(-) diff --git a/api/v1alpha1/inferenceservice_types.go b/api/v1alpha1/inferenceservice_types.go index 98f5d8ea..2dd8a258 100644 --- a/api/v1alpha1/inferenceservice_types.go +++ b/api/v1alpha1/inferenceservice_types.go @@ -562,13 +562,16 @@ type InferenceServiceSpec struct { SLO *SLOSpec `json:"slo,omitempty"` // MaxPodLifetimeSeconds requests best-effort periodic recycling of - // deployment-backed inference pods. The controller serializes graceful - // replacement of eligible pods, using their status start time as the age - // reference; it is not a strict deadline and does not set - // PodSpec.ActiveDeadlineSeconds. This is useful for workloads that need - // periodic process recycling to release driver memory (e.g. llama.cpp on - // AMD Vulkan with pinned GTT memory). When omitted, pods run indefinitely - // until manually restarted or the Deployment is updated. + // deployment-backed inference pods. The controller evicts one expired pod + // at a time, using its status start time as the age reference; it is not a + // strict deadline and does not set PodSpec.ActiveDeadlineSeconds. This is + // useful for workloads that need periodic process recycling to release + // driver memory (e.g. llama.cpp on AMD Vulkan with pinned GTT memory). + // Eviction respects PodDisruptionBudgets, and when rolloutPolicy.waitForIdle + // is set it waits for the backend to go idle first. With a single replica + // recycling is a restart, not a rolling replacement: expect a downtime + // window while the model reloads. When omitted, pods run indefinitely until + // manually restarted or the Deployment is updated. // +kubebuilder:validation:Minimum=1 // +optional MaxPodLifetimeSeconds *int64 `json:"maxPodLifetimeSeconds,omitempty"` diff --git a/charts/llmkube/templates/clusterrole.yaml b/charts/llmkube/templates/clusterrole.yaml index 972ff37e..9bed69ac 100644 --- a/charts/llmkube/templates/clusterrole.yaml +++ b/charts/llmkube/templates/clusterrole.yaml @@ -75,13 +75,6 @@ rules: - patch - update - watch -- apiGroups: - - apps - resources: - - replicasets - verbs: - - list - - watch - apiGroups: - autoscaling resources: diff --git a/charts/llmkube/templates/crds/inferenceservices.yaml b/charts/llmkube/templates/crds/inferenceservices.yaml index 114c0cac..5f1eb736 100644 --- a/charts/llmkube/templates/crds/inferenceservices.yaml +++ b/charts/llmkube/templates/crds/inferenceservices.yaml @@ -3450,13 +3450,16 @@ spec: maxPodLifetimeSeconds: description: |- MaxPodLifetimeSeconds requests best-effort periodic recycling of - deployment-backed inference pods. The controller serializes graceful - replacement of eligible pods, using their status start time as the age - reference; it is not a strict deadline and does not set - PodSpec.ActiveDeadlineSeconds. This is useful for workloads that need - periodic process recycling to release driver memory (e.g. llama.cpp on - AMD Vulkan with pinned GTT memory). When omitted, pods run indefinitely - until manually restarted or the Deployment is updated. + deployment-backed inference pods. The controller evicts one expired pod + at a time, using its status start time as the age reference; it is not a + strict deadline and does not set PodSpec.ActiveDeadlineSeconds. This is + useful for workloads that need periodic process recycling to release + driver memory (e.g. llama.cpp on AMD Vulkan with pinned GTT memory). + Eviction respects PodDisruptionBudgets, and when rolloutPolicy.waitForIdle + is set it waits for the backend to go idle first. With a single replica + recycling is a restart, not a rolling replacement: expect a downtime + window while the model reloads. When omitted, pods run indefinitely until + manually restarted or the Deployment is updated. format: int64 minimum: 1 type: integer diff --git a/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml b/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml index 7b920808..0233dd2b 100644 --- a/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml +++ b/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml @@ -3446,13 +3446,16 @@ spec: maxPodLifetimeSeconds: description: |- MaxPodLifetimeSeconds requests best-effort periodic recycling of - deployment-backed inference pods. The controller serializes graceful - replacement of eligible pods, using their status start time as the age - reference; it is not a strict deadline and does not set - PodSpec.ActiveDeadlineSeconds. This is useful for workloads that need - periodic process recycling to release driver memory (e.g. llama.cpp on - AMD Vulkan with pinned GTT memory). When omitted, pods run indefinitely - until manually restarted or the Deployment is updated. + deployment-backed inference pods. The controller evicts one expired pod + at a time, using its status start time as the age reference; it is not a + strict deadline and does not set PodSpec.ActiveDeadlineSeconds. This is + useful for workloads that need periodic process recycling to release + driver memory (e.g. llama.cpp on AMD Vulkan with pinned GTT memory). + Eviction respects PodDisruptionBudgets, and when rolloutPolicy.waitForIdle + is set it waits for the backend to go idle first. With a single replica + recycling is a restart, not a rolling replacement: expect a downtime + window while the model reloads. When omitted, pods run indefinitely until + manually restarted or the Deployment is updated. format: int64 minimum: 1 type: integer diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 757eee8a..333c7c48 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -78,13 +78,6 @@ rules: - patch - update - watch -- apiGroups: - - apps - resources: - - replicasets - verbs: - - list - - watch - apiGroups: - autoscaling resources: diff --git a/internal/controller/inferenceservice_controller.go b/internal/controller/inferenceservice_controller.go index 01c8c7fc..3c7bbdfc 100644 --- a/internal/controller/inferenceservice_controller.go +++ b/internal/controller/inferenceservice_controller.go @@ -148,7 +148,6 @@ func initContainerSecurityContext(isvc *inferencev1alpha1.InferenceService) *cor // +kubebuilder:rbac:groups=discovery.k8s.io,resources=endpointslices,verbs=get;list;watch // +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch // +kubebuilder:rbac:groups=core,resources=pods/eviction,verbs=create -// +kubebuilder:rbac:groups=apps,resources=replicasets,verbs=list;watch // +kubebuilder:rbac:groups=core,resources=persistentvolumeclaims,verbs=get;list;watch;create;update;patch // +kubebuilder:rbac:groups=scheduling.k8s.io,resources=priorityclasses,verbs=get;list;watch // +kubebuilder:rbac:groups=autoscaling,resources=horizontalpodautoscalers,verbs=get;list;watch;create;update;patch;delete @@ -276,6 +275,18 @@ func (r *InferenceServiceReconciler) Reconcile(ctx context.Context, req ctrl.Req return finalResult, nil } +// earliestPositive merges the requeue timers Reconcile collects without +// allowing a zero timer (which means "no requeue") to win a plain minimum. +func earliestPositive(values ...time.Duration) time.Duration { + var earliest time.Duration + for _, value := range values { + if value > 0 && (earliest == 0 || value < earliest) { + earliest = value + } + } + return earliest +} + func (r *InferenceServiceReconciler) getModelForInferenceService(ctx context.Context, isvc *inferencev1alpha1.InferenceService) (*inferencev1alpha1.Model, bool, *ctrl.Result, error) { log := logf.FromContext(ctx) diff --git a/internal/controller/pod_lifetime.go b/internal/controller/pod_lifetime.go index 5535adf6..85440a0f 100644 --- a/internal/controller/pod_lifetime.go +++ b/internal/controller/pod_lifetime.go @@ -11,6 +11,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" inferencev1alpha1 "github.com/defilantech/llmkube/api/v1alpha1" ) @@ -20,18 +21,6 @@ const ( podLifetimeRetry = 30 * time.Second ) -// earliestPositive merges controller timers without allowing a zero timer to -// create a polling loop. -func earliestPositive(values ...time.Duration) time.Duration { - var earliest time.Duration - for _, value := range values { - if value > 0 && (earliest == 0 || value < earliest) { - earliest = value - } - } - return earliest -} - // reconcilePodLifetime recycles at most one healthy, expired pod. All // workload checks are deliberately conservative: a watch will cause another // attempt after the Deployment has settled or a replacement pod is ready. @@ -45,16 +34,18 @@ func (r *InferenceServiceReconciler) reconcilePodLifetime(ctx context.Context, i if err != nil || deployment == nil { return 0, err } - owned, err := r.ownedActivePods(ctx, deployment) + active, err := r.activePods(ctx, deployment) if err != nil { return 0, err } - // stableDeployment already proved Status.Replicas is the desired count, so - // it doubles as the expected number of active pods. - if len(owned) != int(deployment.Status.Replicas) || !allPodsReady(owned) { + // stableDeployment already proved Status.Replicas is the desired count, and + // it only counts pods the Deployment owns. A foreign pod sharing the + // selector therefore fails this check and holds recycling rather than + // risking an eviction the operator does not own. + if len(active) != int(deployment.Status.Replicas) || !allPodsReady(active) { return 0, nil } - return r.recycleExpiredPod(ctx, owned, podLifetime(*isvc.Spec.MaxPodLifetimeSeconds), now) + return r.recycleExpiredPod(ctx, isvc, active, podLifetime(*isvc.Spec.MaxPodLifetimeSeconds), now) } func (r *InferenceServiceReconciler) getStableDeployment(ctx context.Context, isvc *inferencev1alpha1.InferenceService) (*appsv1.Deployment, error) { @@ -75,11 +66,11 @@ func (r *InferenceServiceReconciler) getStableDeployment(ctx context.Context, is // recycleExpiredPod evicts the pod that expired first, or reports how long // until the next one does. Ties break on name so repeated reconciles of the // same state pick the same pod. -func (r *InferenceServiceReconciler) recycleExpiredPod(ctx context.Context, owned []*corev1.Pod, lifetime time.Duration, now time.Time) (time.Duration, error) { +func (r *InferenceServiceReconciler) recycleExpiredPod(ctx context.Context, isvc *inferencev1alpha1.InferenceService, active []*corev1.Pod, lifetime time.Duration, now time.Time) (time.Duration, error) { var expired *corev1.Pod var expiredDeadline time.Time var earliest time.Duration - for _, pod := range owned { + for _, pod := range active { deadline := pod.Status.StartTime.Time.Add(lifetime) if deadline.After(now) { earliest = earliestPositive(earliest, deadline.Sub(now)) @@ -93,20 +84,42 @@ func (r *InferenceServiceReconciler) recycleExpiredPod(ctx context.Context, owne if expired == nil { return earliest, nil } - return r.evictPod(ctx, expired) + // waitForIdle promises never to drop in-flight generations. Recycling is + // best-effort and has no deadline to race, so it simply waits for the next + // idle window instead of borrowing the rollout path's timeout budget. + if isvc.RolloutPolicyEnabled() { + idle, err := r.checkServiceIdle(ctx, isvc, &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: sanitizeDNSName(isvc.Name), Namespace: isvc.Namespace}}) + if err != nil || !idle { + // Fail closed, matching reconcileRolloutPolicy: an idle probe that + // cannot answer must not authorise killing a possibly-busy pod. + logf.FromContext(ctx).Info("Backend not idle, holding pod recycle", "pod", expired.Name, "error", err) + return inferencev1alpha1.DefaultIdleCheckInterval, nil + } + } + return r.evictPod(ctx, isvc, expired) } -func (r *InferenceServiceReconciler) evictPod(ctx context.Context, pod *corev1.Pod) (time.Duration, error) { +func (r *InferenceServiceReconciler) evictPod(ctx context.Context, isvc *inferencev1alpha1.InferenceService, pod *corev1.Pod) (time.Duration, error) { eviction := &policyv1.Eviction{ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}, DeleteOptions: &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &pod.UID}}} err := r.SubResource("eviction").Create(ctx, pod, eviction) switch { case apierrors.IsNotFound(err): return 0, nil case apierrors.IsTooManyRequests(err): + // A PodDisruptionBudget rejected the eviction; nothing watches that, so + // the retry has to be a timer. + logf.FromContext(ctx).Info("Eviction blocked by a PodDisruptionBudget, retrying later", "pod", pod.Name, "retryAfter", podLifetimeRetry) return podLifetimeRetry, nil - default: + case err != nil: return 0, err } + // A pod disappearing on a timer is invisible in `kubectl describe isvc` + // without this. + if r.Recorder != nil { + r.Recorder.Eventf(isvc, nil, corev1.EventTypeNormal, "PodRecycled", "Reconcile", + "Evicted pod %s after exceeding maxPodLifetimeSeconds; the Deployment will create a replacement", pod.Name) + } + return 0, nil } func stableDeployment(deployment *appsv1.Deployment, isvc *inferencev1alpha1.InferenceService) bool { @@ -120,44 +133,28 @@ func stableDeployment(deployment *appsv1.Deployment, isvc *inferencev1alpha1.Inf return deployment.Status.Replicas == replicas && deployment.Status.UpdatedReplicas == replicas && deployment.Status.ReadyReplicas == replicas && deployment.Status.AvailableReplicas == replicas } -func (r *InferenceServiceReconciler) ownedActivePods(ctx context.Context, deployment *appsv1.Deployment) ([]*corev1.Pod, error) { +// activePods lists the non-terminal pods matching the Deployment's selector. +// The selector labels are unique per InferenceService, and the caller +// cross-checks the count against Status.Replicas, so this deliberately does not +// walk ReplicaSets to prove ownership pod by pod. +func (r *InferenceServiceReconciler) activePods(ctx context.Context, deployment *appsv1.Deployment) ([]*corev1.Pod, error) { listOpts := []client.ListOption{client.InNamespace(deployment.Namespace)} if deployment.Spec.Selector != nil { listOpts = append(listOpts, client.MatchingLabels(deployment.Spec.Selector.MatchLabels)) } - replicaSets := &appsv1.ReplicaSetList{} - if err := r.List(ctx, replicaSets, listOpts...); err != nil { - return nil, err - } - ownedRS := make(map[types.UID]struct{}) - for i := range replicaSets.Items { - if metav1.IsControlledBy(&replicaSets.Items[i], deployment) { - ownedRS[replicaSets.Items[i].UID] = struct{}{} - } - } - if len(ownedRS) == 0 { - return nil, nil - } pods := &corev1.PodList{} if err := r.List(ctx, pods, listOpts...); err != nil { return nil, err } - owned := make([]*corev1.Pod, 0, len(pods.Items)) + active := make([]*corev1.Pod, 0, len(pods.Items)) for i := range pods.Items { pod := &pods.Items[i] if pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed { continue } - ref := metav1.GetControllerOfNoCopy(pod) - if ref == nil { - continue - } - if _, ok := ownedRS[ref.UID]; !ok { - continue - } - owned = append(owned, pod) + active = append(active, pod) } - return owned, nil + return active, nil } func allPodsReady(pods []*corev1.Pod) bool { diff --git a/internal/controller/pod_lifetime_test.go b/internal/controller/pod_lifetime_test.go index afb37d49..7e94045a 100644 --- a/internal/controller/pod_lifetime_test.go +++ b/internal/controller/pod_lifetime_test.go @@ -2,6 +2,9 @@ package controller import ( "context" + "fmt" + "net/http" + "net/http/httptest" "testing" "time" @@ -12,7 +15,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -138,7 +140,10 @@ func TestReconcilePodLifetimeBlocksUnstablePodsAndDeployment(t *testing.T) { } } -func TestReconcilePodLifetimeIgnoresForeignOwnership(t *testing.T) { +// A pod the Deployment does not own but which shares its selector makes the +// active count disagree with Status.Replicas. Recycling must hold rather than +// evict something the operator cannot account for. +func TestReconcilePodLifetimeHoldsOnForeignPod(t *testing.T) { now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) r, isvc, pod := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) foreign := pod.DeepCopy() @@ -146,21 +151,56 @@ func TestReconcilePodLifetimeIgnoresForeignOwnership(t *testing.T) { foreign.UID = "foreign-uid" foreign.ResourceVersion = "" foreign.OwnerReferences[0].UID = "other-rs" - foreignRS := &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{Name: "foreign-rs", Namespace: "ns", UID: "other-rs", Labels: map[string]string{"app": "svc"}, OwnerReferences: []metav1.OwnerReference{{UID: "other-deployment", Controller: ptr.To(true)}}}} - if err := r.Create(context.Background(), foreignRS); err != nil { - t.Fatal(err) - } if err := r.Create(context.Background(), foreign); err != nil { t.Fatal(err) } + recorder := &recordingClient{Client: r.Client} + r.Client = recorder if _, err := r.reconcilePodLifetime(context.Background(), isvc, false, now); err != nil { t.Fatal(err) } - if err := r.Get(context.Background(), client.ObjectKeyFromObject(pod), &corev1.Pod{}); !apierrors.IsNotFound(err) { - t.Fatalf("owned pod was not deleted: %v", err) + if len(recorder.evictions) != 0 { + t.Fatalf("eviction calls = %d, want 0", len(recorder.evictions)) } - if err := r.Get(context.Background(), client.ObjectKeyFromObject(foreign), &corev1.Pod{}); err != nil { - t.Fatalf("foreign pod was deleted: %v", err) +} + +func TestReconcilePodLifetimeWaitsForIdle(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + for name, busy := range map[string]bool{"busy": true, "idle": false} { + t.Run(name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.URL.Path != "/slots" { + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, `[{"id":0,"is_processing":%t}]`, busy) + })) + defer server.Close() + + r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) + isvc.Spec.RolloutPolicy = &inferencev1alpha1.RolloutPolicySpec{WaitForIdle: true} + r.RolloutIdleBaseURL = server.URL + recorder := &recordingClient{Client: r.Client} + r.Client = recorder + + requeue, err := r.reconcilePodLifetime(context.Background(), isvc, false, now) + if err != nil { + t.Fatal(err) + } + if busy { + if len(recorder.evictions) != 0 { + t.Fatalf("evicted a busy pod: %d calls", len(recorder.evictions)) + } + if requeue != inferencev1alpha1.DefaultIdleCheckInterval { + t.Fatalf("requeue = %s, want %s", requeue, inferencev1alpha1.DefaultIdleCheckInterval) + } + return + } + if len(recorder.evictions) != 1 { + t.Fatalf("idle pod was not recycled: %d calls", len(recorder.evictions)) + } + }) } } @@ -269,7 +309,6 @@ func lifetimeReconcilerWith(t *testing.T, start time.Time, lifetime time.Duratio seconds := int64(lifetime / time.Second) isvc := &inferencev1alpha1.InferenceService{ObjectMeta: metav1.ObjectMeta{Name: "svc", Namespace: "ns", UID: isvcUID}, Spec: inferencev1alpha1.InferenceServiceSpec{MaxPodLifetimeSeconds: &seconds}} deployment := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "svc", Namespace: "ns", UID: depUID, Generation: 2, OwnerReferences: []metav1.OwnerReference{{UID: isvcUID, Controller: &controller}}}, Spec: appsv1.DeploymentSpec{Replicas: &replicas, Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "svc"}}}, Status: appsv1.DeploymentStatus{ObservedGeneration: 2, Replicas: replicaCount, UpdatedReplicas: replicaCount, ReadyReplicas: replicaCount, AvailableReplicas: replicaCount}} - rs := &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{Name: "svc-rs", Namespace: "ns", UID: rsUID, Labels: map[string]string{"app": "svc"}, OwnerReferences: []metav1.OwnerReference{{UID: depUID, Controller: &controller}}}} ready := corev1.ConditionTrue pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "first", Namespace: "ns", UID: "pod-first", Labels: map[string]string{"app": "svc"}, OwnerReferences: []metav1.OwnerReference{{UID: rsUID, Controller: &controller}}}, Status: corev1.PodStatus{StartTime: &metav1.Time{Time: start}, Conditions: []corev1.PodCondition{{Type: corev1.PodReady, Status: ready}}}} if podMutator != nil { @@ -288,5 +327,5 @@ func lifetimeReconcilerWith(t *testing.T, start time.Time, lifetime time.Duratio if err := policyv1.AddToScheme(scheme); err != nil { t.Fatal(err) } - return &InferenceServiceReconciler{Client: fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(deployment, pod).WithObjects(isvc, deployment, rs, pod).Build()}, isvc, pod + return &InferenceServiceReconciler{Client: fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(deployment, pod).WithObjects(isvc, deployment, pod).Build()}, isvc, pod } From ec1723213335e05cc75e9335acbf3aca1e44417d Mon Sep 17 00:00:00 2001 From: Tanguille <91473554+Tanguille@users.noreply.github.com> Date: Thu, 23 Jul 2026 02:17:30 +0200 Subject: [PATCH 4/5] feat(api): add maxPodLifetimeIdleTimeoutSeconds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recycling waits indefinitely for an idle backend under rolloutPolicy.waitForIdle, which is safe for in-flight requests but means a saturated service never recycles — exactly when leaked driver memory hurts most. This makes the wait boundable: - omitted: wait indefinitely (unchanged 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 reconcileRolloutPolicy does it with a status condition. podLifetime becomes boundedSeconds now that two fields share its overflow clamp. Signed-off-by: Tanguille <91473554+Tanguille@users.noreply.github.com> --- api/v1alpha1/inferenceservice_types.go | 12 ++++++ api/v1alpha1/zz_generated.deepcopy.go | 5 +++ .../templates/crds/inferenceservices.yaml | 13 ++++++ ...ference.llmkube.dev_inferenceservices.yaml | 13 ++++++ internal/controller/pod_lifetime.go | 28 +++++++++---- internal/controller/pod_lifetime_test.go | 42 ++++++++++++------- 6 files changed, 90 insertions(+), 23 deletions(-) diff --git a/api/v1alpha1/inferenceservice_types.go b/api/v1alpha1/inferenceservice_types.go index 2dd8a258..626c5e28 100644 --- a/api/v1alpha1/inferenceservice_types.go +++ b/api/v1alpha1/inferenceservice_types.go @@ -575,6 +575,18 @@ type InferenceServiceSpec struct { // +kubebuilder:validation:Minimum=1 // +optional MaxPodLifetimeSeconds *int64 `json:"maxPodLifetimeSeconds,omitempty"` + + // MaxPodLifetimeIdleTimeoutSeconds bounds how long recycling will wait for + // an idle backend before evicting anyway, measured from the moment the pod + // exceeded maxPodLifetimeSeconds. It only applies when + // rolloutPolicy.waitForIdle is set, which otherwise makes recycling wait + // indefinitely — safe for in-flight requests, but a saturated service then + // never recycles, which is exactly when leaked driver memory hurts most. + // Set 0 to recycle without waiting for idle at all. When omitted, recycling + // waits indefinitely. + // +kubebuilder:validation:Minimum=0 + // +optional + MaxPodLifetimeIdleTimeoutSeconds *int64 `json:"maxPodLifetimeIdleTimeoutSeconds,omitempty"` } // RolloutPolicySpec defines how deployment updates should be gated on backend idleness. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index c947e4f4..08667a14 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -841,6 +841,11 @@ func (in *InferenceServiceSpec) DeepCopyInto(out *InferenceServiceSpec) { *out = new(int64) **out = **in } + if in.MaxPodLifetimeIdleTimeoutSeconds != nil { + in, out := &in.MaxPodLifetimeIdleTimeoutSeconds, &out.MaxPodLifetimeIdleTimeoutSeconds + *out = new(int64) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new InferenceServiceSpec. diff --git a/charts/llmkube/templates/crds/inferenceservices.yaml b/charts/llmkube/templates/crds/inferenceservices.yaml index 5f1eb736..bfd4814b 100644 --- a/charts/llmkube/templates/crds/inferenceservices.yaml +++ b/charts/llmkube/templates/crds/inferenceservices.yaml @@ -3447,6 +3447,19 @@ spec: Jinja enables Jinja2 chat template rendering for tool/function calling support. Required when using the OpenAI-compatible API with tools. Maps to llama.cpp --jinja flag. type: boolean + maxPodLifetimeIdleTimeoutSeconds: + description: |- + MaxPodLifetimeIdleTimeoutSeconds bounds how long recycling will wait for + an idle backend before evicting anyway, measured from the moment the pod + exceeded maxPodLifetimeSeconds. It only applies when + rolloutPolicy.waitForIdle is set, which otherwise makes recycling wait + indefinitely — safe for in-flight requests, but a saturated service then + never recycles, which is exactly when leaked driver memory hurts most. + Set 0 to recycle without waiting for idle at all. When omitted, recycling + waits indefinitely. + format: int64 + minimum: 0 + type: integer maxPodLifetimeSeconds: description: |- MaxPodLifetimeSeconds requests best-effort periodic recycling of diff --git a/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml b/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml index 0233dd2b..44686420 100644 --- a/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml +++ b/config/crd/bases/inference.llmkube.dev_inferenceservices.yaml @@ -3443,6 +3443,19 @@ spec: Jinja enables Jinja2 chat template rendering for tool/function calling support. Required when using the OpenAI-compatible API with tools. Maps to llama.cpp --jinja flag. type: boolean + maxPodLifetimeIdleTimeoutSeconds: + description: |- + MaxPodLifetimeIdleTimeoutSeconds bounds how long recycling will wait for + an idle backend before evicting anyway, measured from the moment the pod + exceeded maxPodLifetimeSeconds. It only applies when + rolloutPolicy.waitForIdle is set, which otherwise makes recycling wait + indefinitely — safe for in-flight requests, but a saturated service then + never recycles, which is exactly when leaked driver memory hurts most. + Set 0 to recycle without waiting for idle at all. When omitted, recycling + waits indefinitely. + format: int64 + minimum: 0 + type: integer maxPodLifetimeSeconds: description: |- MaxPodLifetimeSeconds requests best-effort periodic recycling of diff --git a/internal/controller/pod_lifetime.go b/internal/controller/pod_lifetime.go index 85440a0f..6062ffd6 100644 --- a/internal/controller/pod_lifetime.go +++ b/internal/controller/pod_lifetime.go @@ -45,7 +45,7 @@ func (r *InferenceServiceReconciler) reconcilePodLifetime(ctx context.Context, i if len(active) != int(deployment.Status.Replicas) || !allPodsReady(active) { return 0, nil } - return r.recycleExpiredPod(ctx, isvc, active, podLifetime(*isvc.Spec.MaxPodLifetimeSeconds), now) + return r.recycleExpiredPod(ctx, isvc, active, boundedSeconds(*isvc.Spec.MaxPodLifetimeSeconds), now) } func (r *InferenceServiceReconciler) getStableDeployment(ctx context.Context, isvc *inferencev1alpha1.InferenceService) (*appsv1.Deployment, error) { @@ -84,10 +84,9 @@ func (r *InferenceServiceReconciler) recycleExpiredPod(ctx context.Context, isvc if expired == nil { return earliest, nil } - // waitForIdle promises never to drop in-flight generations. Recycling is - // best-effort and has no deadline to race, so it simply waits for the next - // idle window instead of borrowing the rollout path's timeout budget. - if isvc.RolloutPolicyEnabled() { + // waitForIdle promises never to drop in-flight generations, so recycling + // waits for an idle window rather than killing a busy pod on a timer. + if isvc.RolloutPolicyEnabled() && !idleWaitExhausted(isvc, expiredDeadline, now) { idle, err := r.checkServiceIdle(ctx, isvc, &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: sanitizeDNSName(isvc.Name), Namespace: isvc.Namespace}}) if err != nil || !idle { // Fail closed, matching reconcileRolloutPolicy: an idle probe that @@ -99,6 +98,19 @@ func (r *InferenceServiceReconciler) recycleExpiredPod(ctx context.Context, isvc return r.evictPod(ctx, isvc, expired) } +// idleWaitExhausted reports whether maxPodLifetimeIdleTimeoutSeconds says to +// stop waiting for idle and recycle a still-busy pod. The budget runs from the +// pod's own expiry, so the deadline is derivable from the pod and nothing has +// to be persisted across reconciles. Omitted means wait indefinitely; 0 means +// never wait, since the caller only reaches here once deadline has passed. +func idleWaitExhausted(isvc *inferencev1alpha1.InferenceService, deadline time.Time, now time.Time) bool { + timeout := isvc.Spec.MaxPodLifetimeIdleTimeoutSeconds + if timeout == nil { + return false + } + return !now.Before(deadline.Add(boundedSeconds(*timeout))) +} + func (r *InferenceServiceReconciler) evictPod(ctx context.Context, isvc *inferencev1alpha1.InferenceService, pod *corev1.Pod) (time.Duration, error) { eviction := &policyv1.Eviction{ObjectMeta: metav1.ObjectMeta{Name: pod.Name, Namespace: pod.Namespace}, DeleteOptions: &metav1.DeleteOptions{Preconditions: &metav1.Preconditions{UID: &pod.UID}}} err := r.SubResource("eviction").Create(ctx, pod, eviction) @@ -166,12 +178,12 @@ func allPodsReady(pods []*corev1.Pod) bool { return true } -func podLifetime(seconds int64) time.Duration { +// boundedSeconds converts a spec field to a Duration, clamping it so an +// unbounded value cannot overflow into the past. +func boundedSeconds(seconds int64) time.Duration { if seconds <= 0 { return 0 } - // The CRD has no Maximum, so an unbounded value would overflow Duration - // into the past and turn recycling into an eviction loop. if seconds > maxPodLifetimeSeconds { seconds = maxPodLifetimeSeconds } diff --git a/internal/controller/pod_lifetime_test.go b/internal/controller/pod_lifetime_test.go index 7e94045a..a6c67db5 100644 --- a/internal/controller/pod_lifetime_test.go +++ b/internal/controller/pod_lifetime_test.go @@ -15,6 +15,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -164,9 +165,21 @@ func TestReconcilePodLifetimeHoldsOnForeignPod(t *testing.T) { } } +// The pod starts 2 minutes ago with a 1 minute lifetime, so by now it has been +// overdue for a minute: a 30s idle timeout is spent, a 300s one is not. func TestReconcilePodLifetimeWaitsForIdle(t *testing.T) { now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) - for name, busy := range map[string]bool{"busy": true, "idle": false} { + for name, tc := range map[string]struct { + busy bool + idleTimeout *int64 + wantEvicted bool + }{ + "idle backend recycles": {busy: false, wantEvicted: true}, + "busy backend waits indefinitely": {busy: true, wantEvicted: false}, + "busy backend waits within budget": {busy: true, idleTimeout: ptr.To(int64(300)), wantEvicted: false}, + "busy backend recycles once overdue": {busy: true, idleTimeout: ptr.To(int64(30)), wantEvicted: true}, + "zero timeout recycles without asking": {busy: true, idleTimeout: ptr.To(int64(0)), wantEvicted: true}, + } { t.Run(name, func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if req.URL.Path != "/slots" { @@ -174,12 +187,13 @@ func TestReconcilePodLifetimeWaitsForIdle(t *testing.T) { return } w.Header().Set("Content-Type", "application/json") - _, _ = fmt.Fprintf(w, `[{"id":0,"is_processing":%t}]`, busy) + _, _ = fmt.Fprintf(w, `[{"id":0,"is_processing":%t}]`, tc.busy) })) defer server.Close() r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) isvc.Spec.RolloutPolicy = &inferencev1alpha1.RolloutPolicySpec{WaitForIdle: true} + isvc.Spec.MaxPodLifetimeIdleTimeoutSeconds = tc.idleTimeout r.RolloutIdleBaseURL = server.URL recorder := &recordingClient{Client: r.Client} r.Client = recorder @@ -188,17 +202,15 @@ func TestReconcilePodLifetimeWaitsForIdle(t *testing.T) { if err != nil { t.Fatal(err) } - if busy { - if len(recorder.evictions) != 0 { - t.Fatalf("evicted a busy pod: %d calls", len(recorder.evictions)) - } - if requeue != inferencev1alpha1.DefaultIdleCheckInterval { - t.Fatalf("requeue = %s, want %s", requeue, inferencev1alpha1.DefaultIdleCheckInterval) - } - return + want := 0 + if tc.wantEvicted { + want = 1 + } + if len(recorder.evictions) != want { + t.Fatalf("eviction calls = %d, want %d", len(recorder.evictions), want) } - if len(recorder.evictions) != 1 { - t.Fatalf("idle pod was not recycled: %d calls", len(recorder.evictions)) + if !tc.wantEvicted && requeue != inferencev1alpha1.DefaultIdleCheckInterval { + t.Fatalf("requeue = %s, want %s", requeue, inferencev1alpha1.DefaultIdleCheckInterval) } }) } @@ -284,11 +296,11 @@ func TestReconcilePodLifetimePDBRetry(t *testing.T) { } } -func TestPodLifetimeBoundsDuration(t *testing.T) { - if got := podLifetime(maxPodLifetimeSeconds); got != time.Duration(maxPodLifetimeSeconds)*time.Second { +func TestBoundedSecondsClampsOverflow(t *testing.T) { + if got := boundedSeconds(maxPodLifetimeSeconds); got != time.Duration(maxPodLifetimeSeconds)*time.Second { t.Fatalf("duration = %s", got) } - if got := podLifetime(maxPodLifetimeSeconds + 1); got != time.Duration(maxPodLifetimeSeconds)*time.Second { + if got := boundedSeconds(maxPodLifetimeSeconds + 1); got != time.Duration(maxPodLifetimeSeconds)*time.Second { t.Fatalf("overflow duration = %s", got) } } From 12d6d8f9e1fbb23f1a3bd0837a5194c89480ac51 Mon Sep 17 00:00:00 2001 From: Tanguille <91473554+Tanguille@users.noreply.github.com> Date: Thu, 23 Jul 2026 03:04:47 +0200 Subject: [PATCH 5/5] fix(controller): back off instead of wedging when eviction is forbidden MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found testing this branch on a live cluster: swapping the operator image without the chart that owns its ClusterRole leaves pods/eviction ungranted, and the resulting Forbidden propagated out of evictPod as a reconcile error. That failed the entire InferenceService reconcile and retried on the error backoff — roughly two errors every ten seconds, indefinitely, since no amount of retrying grants a permission. Treat it like the PodDisruptionBudget case: log it, raise a PodRecycleForbidden warning event so it shows up in `kubectl describe isvc`, and back off to the recycle retry interval. The rest of the reconcile (status, service, HPA) now completes normally with only recycling disabled. Signed-off-by: Tanguille <91473554+Tanguille@users.noreply.github.com> --- internal/controller/pod_lifetime.go | 12 ++++++++++++ internal/controller/pod_lifetime_test.go | 19 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/internal/controller/pod_lifetime.go b/internal/controller/pod_lifetime.go index 6062ffd6..cc48b766 100644 --- a/internal/controller/pod_lifetime.go +++ b/internal/controller/pod_lifetime.go @@ -122,6 +122,18 @@ func (r *InferenceServiceReconciler) evictPod(ctx context.Context, isvc *inferen // the retry has to be a timer. logf.FromContext(ctx).Info("Eviction blocked by a PodDisruptionBudget, retrying later", "pod", pod.Name, "retryAfter", podLifetimeRetry) return podLifetimeRetry, nil + case apierrors.IsForbidden(err): + // Missing pods/eviction RBAC — seen for real when the operator image is + // upgraded without the chart that owns its ClusterRole. Returning an + // error here would fail the whole InferenceService reconcile and retry + // hot forever, since no amount of retrying grants a permission. Surface + // it where an operator will actually look, and back off. + logf.FromContext(ctx).Error(err, "Not permitted to evict pods; recycling is disabled until the operator is granted create on pods/eviction", "pod", pod.Name) + if r.Recorder != nil { + r.Recorder.Eventf(isvc, nil, corev1.EventTypeWarning, "PodRecycleForbidden", "Reconcile", + "Cannot recycle pod %s: %v; the operator needs create on pods/eviction", pod.Name, err) + } + return podLifetimeRetry, nil case err != nil: return 0, err } diff --git a/internal/controller/pod_lifetime_test.go b/internal/controller/pod_lifetime_test.go index a6c67db5..ba2f55f0 100644 --- a/internal/controller/pod_lifetime_test.go +++ b/internal/controller/pod_lifetime_test.go @@ -282,6 +282,25 @@ func TestReconcilePodLifetimeNotFoundDeleteIsSuccess(t *testing.T) { } } +// Found on a live cluster: swapping the operator image without the chart that +// owns its ClusterRole leaves pods/eviction ungranted. That must back off, not +// fail the reconcile and retry hot forever. +func TestReconcilePodLifetimeForbiddenBacksOff(t *testing.T) { + now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) + r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute) + forbidden := apierrors.NewForbidden(corev1.Resource("pods"), "first", + fmt.Errorf(`User "system:serviceaccount:ai:llmkube-controller-manager" cannot create resource "pods/eviction"`)) + r.Client = &recordingClient{Client: r.Client, evictErr: forbidden} + + got, err := r.reconcilePodLifetime(context.Background(), isvc, false, now) + if err != nil { + t.Fatalf("forbidden eviction failed the reconcile: %v", err) + } + if got != podLifetimeRetry { + t.Fatalf("requeue = %s, want %s", got, podLifetimeRetry) + } +} + func TestReconcilePodLifetimePDBRetry(t *testing.T) { now := time.Date(2026, 7, 22, 12, 0, 0, 0, time.UTC) r, isvc, _ := lifetimeReconciler(t, now.Add(-2*time.Minute), time.Minute)