From f2b3fd46f93d2640792c51c1013ab884b94a2cd3 Mon Sep 17 00:00:00 2001 From: Edward Sun Date: Sun, 28 Jun 2026 19:51:01 -0700 Subject: [PATCH] feat(controller): surface a sizing warning when the lmcache-server is OOMKilled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The standalone lmcache-server's memory is spec.resources (defaulted to a 4Gi request / 8Gi limit). For a large model the KV working set far exceeds 8Gi, so the server is silently OOMKilled under T2 write load — an under-provisioning break that previously looked identical to a healthy-but-idle backend. When a managed backend's workload is ReplicasUnavailable, the reconciler now checks whether a cache-server container was OOMKilled (current or last termination state, scoped to the owned Deployment's containers) and folds an actionable diagnostic into the Ready/Degraded condition message — "cache-server ... OOMKilled ... raise spec.resources.limits.memory". It rides the existing BackendDegraded Warning event (no new event/reason/limiter), so under- provisioning is loud and diagnosable instead of silent. - detectServerOOM helper (best-effort; no pod List on the healthy path) - message enrichment at the managedReadiness call site (reason unchanged, so the Degraded gate + Progressing derivation are untouched) - unit test (detection: current/last state, sidecar-scoping, healthy) + an end-to-end test (ReplicasUnavailable + OOM pod -> enriched Ready/Degraded msg) --- docs/design/cachebackend-api.md | 2 +- .../controller/cachebackend_controller.go | 12 ++++ .../cachebackend_controller_test.go | 50 +++++++++++++++++ .../controller/cachebackend_server_restart.go | 43 ++++++++++++++ .../cachebackend_server_restart_test.go | 56 +++++++++++++++++++ 5 files changed, 162 insertions(+), 1 deletion(-) diff --git a/docs/design/cachebackend-api.md b/docs/design/cachebackend-api.md index 969bc501..bcfd8832 100644 --- a/docs/design/cachebackend-api.md +++ b/docs/design/cachebackend-api.md @@ -180,7 +180,7 @@ Four condition types are published on managed backends (`Ready`, `Degraded`, `Pr | Type | Meaning | |---|---| | `Ready` | True once the backend Deployment has rolled out its current generation, has enough updated + available replicas to serve traffic, **and** — when the [KV-event readiness gate](#kv-event-readiness-gate) applies — at least one KV event has been observed for the backend (reason `KVEventsObserved`), **and** — when the [functional-probe gate](#functional-probe-gate) applies — the most recent probe call succeeded across every stage the backend runs. Workload Available but no event yet is `Ready=False`, reason `AwaitingFirstKVEvent`. Workload Available and KV-event observed but the probe reported a stage failure is `Ready=False` with reason `ProbeIngestFailed` / `ProbeRoutingFailed` / `ProbeT2Failed`. Deployment-level reasons: `BackendReady` (both gates disabled and Available), `RolloutInProgress`, `ScaledToZero`, `ReplicasUnavailable`. The `BackendDegraded` / `BackendRecovered` Events narrate the `ReplicasUnavailable` → `BackendReady` / `KVEventsObserved` transitions. | -| `Degraded` | True when the backend is in a terminal unhealthy state: rolled out but replicas unavailable (reason `ReplicasUnavailable`), or the managed workload is Available but no KV event observed within `firstEventTimeout` (reason `NoKVEventsObserved`). False (`NotDegraded`) otherwise. The functional-probe gate does NOT participate in `Degraded` — a probe failure is reflected only in `Ready` and `FunctionalProbeOK`, leaving `Degraded` reserved for managed-Deployment health (so an operator can tell "the probe says the cache plane is broken" apart from "the workload itself is in a terminal state"). | +| `Degraded` | True when the backend is in a terminal unhealthy state: rolled out but replicas unavailable (reason `ReplicasUnavailable` — and when a cache-server container was OOMKilled, the message names it and points at `spec.resources.limits.memory` so under-provisioning surfaces loudly instead of silently OOM-looping), or the managed workload is Available but no KV event observed within `firstEventTimeout` (reason `NoKVEventsObserved`). False (`NotDegraded`) otherwise. The functional-probe gate does NOT participate in `Degraded` — a probe failure is reflected only in `Ready` and `FunctionalProbeOK`, leaving `Degraded` reserved for managed-Deployment health (so an operator can tell "the probe says the cache plane is broken" apart from "the workload itself is in a terminal state"). | | `Progressing` | True while the controller is still driving the live state toward the desired state (rollout in flight, first apply, awaiting first KV event). False once converged (`Synced`), stuck (`Degraded`), or scaled to zero (`ScaledToZero`). The pair (`Ready=False`, `Progressing=True`) means "still converging"; (`Ready=False`, `Progressing=False`) means "stuck/degraded" (or scaled to zero). | | `FunctionalProbeOK` | The most recent functional-probe outcome. `True/ProbeOK` when every enabled stage (ingest, routing, and — for LMCache — tier-2 put/get) round-tripped; `True/ProbeBypassed` when the operator opted this CR out via the `inferencecache.io/skip-functional-probe: "true"` annotation; `False/ProbeIngestFailed`, `False/ProbeRoutingFailed`, or `False/ProbeT2Failed` when the named stage failed, with the server's diagnostic in `.message`; `Unknown/ProbeError` when the controller could not reach the server's `/probe` endpoint at all (transport error, 5xx) AND no prior stage failure existed. **Sticky-False**: an HTTP error while a `False/Probe*Failed` is already published preserves the prior failure and keeps `Ready` downgraded, so a transient server outage cannot fade a known per-stage failure back to `Unknown` and then to `Ready=True`. See [functional-probe gate](#functional-probe-gate). | diff --git a/internal/controller/cachebackend_controller.go b/internal/controller/cachebackend_controller.go index 8553169e..2d17a40e 100644 --- a/internal/controller/cachebackend_controller.go +++ b/internal/controller/cachebackend_controller.go @@ -1404,6 +1404,18 @@ func (r *CacheBackendReconciler) deleteIfOwned(ctx context.Context, key types.Na func (r *CacheBackendReconciler) updateManagedStatus(ctx context.Context, backend *cachev1alpha1.CacheBackend, endpoint, capacity string, dep *appsv1.Deployment, applyOK bool) (time.Duration, error) { now := time.Now() readyStatus, reason, message := managedReadiness(backend, dep) + // When the workload is replicas-unavailable, check whether the cache-server + // was OOMKilled and fold the actionable sizing diagnostic into the message. + // It flows to the Ready/Degraded condition message AND the BackendDegraded + // Warning event (degradedMessage surfaces the Ready message), turning a silent + // under-provisioning OOM into a loud "raise spec.resources.limits.memory" + // signal. The reason stays ReplicasUnavailable so the Degraded gate and + // Progressing derivation are unchanged — only the message is enriched. + if reason == conditionReasonReplicasUnavailable { + if oom := r.detectServerOOM(ctx, backend, dep); oom != "" { + message = message + " — " + oom + } + } // Resolve the stable timeout anchor: the latched FirstAvailableAt, or — the // first time the workload is Available — now. Using a latched value (not the // live Deployment Available condition, which resets on a flap) keeps the diff --git a/internal/controller/cachebackend_controller_test.go b/internal/controller/cachebackend_controller_test.go index 09f1ece8..70e3e53e 100644 --- a/internal/controller/cachebackend_controller_test.go +++ b/internal/controller/cachebackend_controller_test.go @@ -3,6 +3,7 @@ package controller import ( "context" "errors" + "strings" "sync/atomic" "testing" "time" @@ -1645,3 +1646,52 @@ func findCondition(conds []metav1.Condition, t string) *metav1.Condition { } return nil } + +// TestReplicasUnavailableMessageNamesServerOOM is the end-to-end check for the +// sizing warning: when a managed backend's workload is ReplicasUnavailable AND a +// cache-server container was OOMKilled, the Ready (and so Degraded) condition +// message names the OOM and the actionable fix, instead of a bare replica count. +func TestReplicasUnavailableMessageNamesServerOOM(t *testing.T) { + scheme := newScheme(t) + cb := lmcacheBackend("cache", "ns1") + cb.Spec.Replicas = ptrInt32(1) + r := newReconciler(scheme, cb) + reconcile(t, r, "cache", "ns1") + + // Drive the workload to ReplicasUnavailable: rolled out, 0/1 available. + dep := getDeployment(t, r, "cache", "ns1") + dep.Status.ObservedGeneration = dep.Generation + dep.Status.Replicas = 1 + dep.Status.UpdatedReplicas = 1 + dep.Status.AvailableReplicas = 0 + if err := r.Status().Update(context.Background(), dep); err != nil { + t.Fatalf("update deployment status: %v", err) + } + // A server pod whose cache-server container was OOMKilled (CrashLoopBackOff + // after the kubelet restarted it -> OOMKilled on lastState). + serverContainer := dep.Spec.Template.Spec.Containers[0].Name + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "cache-pod-1", Namespace: "ns1", Labels: selectorLabels("cache")}, + Status: corev1.PodStatus{ContainerStatuses: []corev1.ContainerStatus{{ + Name: serverContainer, + LastTerminationState: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "OOMKilled", ExitCode: 137}}, + }}}, + } + if err := r.Create(context.Background(), pod); err != nil { + t.Fatalf("create OOM pod: %v", err) + } + reconcile(t, r, "cache", "ns1") + + updated := getBackend(t, r, "cache", "ns1") + cond := findCondition(updated.Status.Conditions, conditionTypeReady) + if cond == nil || cond.Status != metav1.ConditionFalse { + t.Fatalf("Ready = %+v, want False", cond) + } + if !strings.Contains(cond.Message, "OOMKilled") || !strings.Contains(cond.Message, "spec.resources.limits.memory") { + t.Fatalf("Ready message should name the server OOM + sizing fix, got %q", cond.Message) + } + // The Degraded condition surfaces the same enriched message. + if d := findCondition(updated.Status.Conditions, conditionTypeDegraded); d == nil || !strings.Contains(d.Message, "OOMKilled") { + t.Fatalf("Degraded condition should also name the OOM, got %+v", d) + } +} diff --git a/internal/controller/cachebackend_server_restart.go b/internal/controller/cachebackend_server_restart.go index 38b9c9a1..e7281458 100644 --- a/internal/controller/cachebackend_server_restart.go +++ b/internal/controller/cachebackend_server_restart.go @@ -913,6 +913,49 @@ func containerRunSum(pod *corev1.Pod, cacheServerContainers map[string]struct{}) return sum } +// detectServerOOM returns a human-readable diagnostic when a managed cache-server +// container was OOMKilled — its memory limit is too small for the model's KV +// working set, the silent under-provisioning failure mode — else "". It is +// best-effort: a pod-list error returns "" rather than failing the reconcile, and +// the caller consults it only when the workload is already ReplicasUnavailable, so +// it adds no pod List on the healthy path. The scan is scoped to the owned +// Deployment's container names (so a crashed service-mesh sidecar is never misread +// as the cache server) and reads BOTH the current and last termination state — a +// freshly-OOMKilled container, and one the kubelet has already restarted into +// CrashLoopBackOff (where OOMKilled is on lastState). +func (r *CacheBackendReconciler) detectServerOOM(ctx context.Context, backend *cachev1alpha1.CacheBackend, dep *appsv1.Deployment) string { + reader := client.Reader(r.APIReader) + if reader == nil { + reader = r.Client + } + var pods corev1.PodList + if err := reader.List(ctx, &pods, + client.InNamespace(backend.Namespace), + client.MatchingLabelsSelector{Selector: labels.SelectorFromSet(selectorLabels(backend.Name))}, + ); err != nil { + return "" + } + serverContainers := make(map[string]struct{}, len(dep.Spec.Template.Spec.Containers)) + for _, c := range dep.Spec.Template.Spec.Containers { + serverContainers[c.Name] = struct{}{} + } + for i := range pods.Items { + p := &pods.Items[i] + for j := range p.Status.ContainerStatuses { + cs := &p.Status.ContainerStatuses[j] + if _, ok := serverContainers[cs.Name]; !ok { + continue + } + for _, term := range []*corev1.ContainerStateTerminated{cs.State.Terminated, cs.LastTerminationState.Terminated} { + if term != nil && term.Reason == "OOMKilled" { + return fmt.Sprintf("cache-server container %q in pod %q was OOMKilled — its memory limit is too small for the model's KV working set; raise spec.resources.limits.memory", cs.Name, p.Name) + } + } + } + } + return "" +} + // podOwnedByDeployment reports whether pod is transitively controller- // owned (pod → ReplicaSet → Deployment) by the given Deployment, // matched on both name AND UID at every link. The (owned, err) split diff --git a/internal/controller/cachebackend_server_restart_test.go b/internal/controller/cachebackend_server_restart_test.go index 29124dc7..0d25ef81 100644 --- a/internal/controller/cachebackend_server_restart_test.go +++ b/internal/controller/cachebackend_server_restart_test.go @@ -1867,3 +1867,59 @@ func (c *countingClient) Patch(ctx context.Context, obj client.Object, p client. } return c.Client.Patch(ctx, obj, p, opts...) } + +// TestDetectServerOOM pins the sizing-warning detection: a cache-server container +// OOMKilled (on current OR last termination state) is reported; a crashed +// service-mesh sidecar (not in the owned Deployment's container set) is NOT; a +// healthy server reports nothing. +func TestDetectServerOOM(t *testing.T) { + scheme := newScheme(t) + const ns, name = "team-a", "qwen-cache" + backend := &cachev1alpha1.CacheBackend{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns, UID: "be-uid"}} + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns, UID: "dep-uid"}, + Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "lmcache-server"}}, + }}}, + } + oomPod := func(container string, onLast bool) *corev1.Pod { + term := &corev1.ContainerStateTerminated{Reason: "OOMKilled", ExitCode: 137} + cs := corev1.ContainerStatus{Name: container} + if onLast { + cs.LastTerminationState.Terminated = term + } else { + cs.State.Terminated = term + } + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "cache-pod", Namespace: ns, Labels: selectorLabels(name)}, + Status: corev1.PodStatus{ContainerStatuses: []corev1.ContainerStatus{cs}}, + } + } + build := func(objs ...client.Object) *CacheBackendReconciler { + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + return &CacheBackendReconciler{Client: c, Scheme: scheme, Log: logr.Discard()} + } + cases := []struct { + name string + pod *corev1.Pod + want bool + }{ + {"oom on lastState (CrashLoopBackOff after restart)", oomPod("lmcache-server", true), true}, + {"oom on currentState (freshly killed)", oomPod("lmcache-server", false), true}, + {"sidecar OOM (not a cache-server container) is ignored", oomPod("istio-proxy", true), false}, + {"healthy running server", &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "cache-pod", Namespace: ns, Labels: selectorLabels(name)}, + Status: corev1.PodStatus{ContainerStatuses: []corev1.ContainerStatus{{ + Name: "lmcache-server", State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}, + }}}, + }, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := build(backend, dep, tc.pod).detectServerOOM(context.Background(), backend, dep) + if (got != "") != tc.want { + t.Fatalf("detectServerOOM = %q, want detected=%v", got, tc.want) + } + }) + } +}