From 8234eae1770dc424f44bc89f949195f9f8c4d526 Mon Sep 17 00:00:00 2001 From: William Rizzo Date: Sun, 9 Aug 2026 12:11:25 +0000 Subject: [PATCH] fix: stop rebuilding cells against a fault that is not in the cell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cell whose Node joins and never advertises a GPU fails at spec.capacity.readyTimeout, and the pool replaced it: delete the guest, allocate a GPU, clone a 30 GiB root disk, boot, join, wait out the timeout again — five times per index, destroying the evidence each round. That is right when the cell is broken and useless when the workload cluster is. HAMi missing, its DaemonSet not tolerating the pool's taints, a broken inner CNI: a fresh VM reaches exactly the same place. Observed on hardware — HAMi could not register and the pool's answer was to rebuild the VM. The pool's capacity verdict already distinguishes the two, because HAMiProvider reports usable as soon as ANY cell node advertises a device. So a failed cell whose Node is Ready, advertising nothing, in a pool where nothing advertises anything, is no longer replaced: the pool stalls with Progressing=False/FaultNotInTheCell and a message naming CapacityProviderReady as the thing to fix, and leaves the VM up to be inspected. A single broken cell among healthy ones is still replaced, and a cell that never joined is still replaced — its failure really may be its own. No deadlock is possible: with no cells the provider is usable by definition, so a pool can never wedge itself out of ever creating one, and the veto is re-evaluated every pass. Validated on hardware in both directions — held Failed with zero rebuilds for four minutes, then refilled the index within two minutes of the provider becoming usable, carrying the failure count forward. Signed-off-by: William Rizzo --- api/v1alpha1/conditions.go | 1 + .../templates/monitoring/prometheusrule.yaml | 6 ++- docs/observability.md | 2 +- docs/runbook.md | 13 +++++ internal/controller/controller.go | 3 +- internal/controller/membership.go | 43 ++++++++++++++++- internal/controller/membership_test.go | 48 +++++++++++++++++-- 7 files changed, 108 insertions(+), 8 deletions(-) diff --git a/api/v1alpha1/conditions.go b/api/v1alpha1/conditions.go index ea626b5..eeaafe7 100644 --- a/api/v1alpha1/conditions.go +++ b/api/v1alpha1/conditions.go @@ -85,6 +85,7 @@ const ( ReasonNodeNameCollision = "NodeNameCollision" ReasonBootstrapCredentialSuspect = "BootstrapCredentialSuspect" ReasonCellReplacementExhausted = "CellReplacementExhausted" + ReasonFaultNotInTheCell = "FaultNotInTheCell" ReasonWaitingForAllocations = "WaitingForAllocations" ReasonDrainTimedOut = "DrainTimedOut" diff --git a/charts/gpucellpool/templates/monitoring/prometheusrule.yaml b/charts/gpucellpool/templates/monitoring/prometheusrule.yaml index 8ee0283..4d4fa10 100644 --- a/charts/gpucellpool/templates/monitoring/prometheusrule.yaml +++ b/charts/gpucellpool/templates/monitoring/prometheusrule.yaml @@ -60,7 +60,11 @@ spec: description: >- A failed cell is replaced behind a backoff, so this firing repeatedly means replacement is not fixing it — usually a bad image, an expired - join credential, or a GPU that never surfaces in the guest. + join credential, or a GPU that never surfaces in the guest. If it stays + Failed with no replacement attempts at all, check Progressing: on + FaultNotInTheCell the pool has deliberately stopped rebuilding because + nothing advertises a GPU pool-wide, and the fault is in the workload + cluster. runbook_url: https://github.com/kubeswift-io/gpucellpool/blob/main/docs/runbook.md - alert: GPUCellPoolCellFlapping diff --git a/docs/observability.md b/docs/observability.md index 44bf6fa..a0ffce0 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -85,7 +85,7 @@ conditions are states the operator reports deliberately rather than faults. |---|---|---| | `GPUCellPoolWorkloadClusterUnreachable` | unreachable for 10m — every destructive path is frozen meanwhile | transport | | `GPUCellPoolCellsNotReady` | short of Ready cells for 30m (a cell takes ~5 min, so this is stuck, not starting) | cells | -| `GPUCellPoolCellFailed` | a Failed cell persists for 15m — replacement is not fixing it | cells | +| `GPUCellPoolCellFailed` | a Failed cell persists for 15m — replacement is not fixing it, or the pool has stopped replacing (`Progressing=FaultNotInTheCell`) because the fault is pool-wide | cells | | `GPUCellPoolCellFlapping` | sustained phase churn | cells | | `GPUCellPoolNoFreePhysicalGPU` | the pool wants a cell **and** no device is free | outer | | `GPUCellPoolSharedGPUExhausted` | healthy cells whose GPU memory is entirely allocated | inner | diff --git a/docs/runbook.md b/docs/runbook.md index 69be18c..df33048 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -107,6 +107,19 @@ swiftctl ssh -n -- 'nvidia-smi -L; cat /run/gpu-cell-preflight' Do **not** expect the pool to call the cell Ready meanwhile. A running VM whose GPU never surfaced is exactly the failure this design refuses to paper over. +After `spec.capacity.readyTimeout` the cell goes `Failed`. What happens next depends +on whether the fault is the cell's: + +- **One cell advertises nothing, others advertise fine** → the cell is replaced, + with backoff, up to five attempts per index (`CellReplacementExhausted`). +- **No cell node advertises anything** → the pool **stops** and reports + `Progressing=False/FaultNotInTheCell`. Causes 2–4 above are workload-cluster + faults, so a rebuilt VM would fail identically — rebuilding would just cost a GPU + allocation, a root-disk clone, a boot and a join per attempt, and destroy the + evidence. The failed cell's VM is left up for you to inspect. Fix the provider + (`CapacityProviderReady` names the cause) and the cell is replaced on the next + pass. + ### `allocatable nvidia.com/gpu` says 10 and I have one GPU That is HAMi's `deviceSplitCount` inflation, not a bug and not a device count. The diff --git a/internal/controller/controller.go b/internal/controller/controller.go index 34edc70..688e63e 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -234,6 +234,7 @@ func (r *GPUCellPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) WorkloadReachable: reachable, EverReady: everReady(&pool, cells), DrainPreference: scale.DrainCandidates, + ProviderUsable: health.Ready, }) for _, idx := range plan.Create { @@ -260,7 +261,7 @@ func (r *GPUCellPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) // Replace failed cells whose backoff has expired: delete the guest and let // the next pass recreate the index. for i := range cells { - if ShouldReplace(cells[i], r.now()) { + if ShouldReplace(cells[i], r.now(), health.Ready) { if err := r.deleteCell(ctx, &pool, prov, cells[i], false); err != nil { return ctrl.Result{}, err } diff --git a/internal/controller/membership.go b/internal/controller/membership.go index f45cb7c..f225535 100644 --- a/internal/controller/membership.go +++ b/internal/controller/membership.go @@ -64,6 +64,12 @@ type MembershipInput struct { // burst of failures" from "this pool has never worked". EverReady bool + // ProviderUsable is the pool-wide capacity verdict: false means no cell node + // anywhere advertises a GPU, which makes a cell rebuild pointless. It is also + // false while the workload cluster is unreachable — where not acting is the + // rule, not an accident. + ProviderUsable bool + // DrainPreference is an ordered list of cell names to remove first when // shrinking. The autoscaler sets it to IDLE cells only; empty falls back to // highest-index-first, which is right for an operator-driven scale-down where @@ -181,6 +187,15 @@ func PlanMembership(in MembershipInput) MembershipPlan { // Replacement: a failed index is retired from the live set, so the create // path below refills it — but only once its backoff has expired. for _, c := range failed { + if RebuildWouldNotHelp(c, in.ProviderUsable) { + plan.Stalled = true + plan.Reason = cellsv1alpha1.ReasonFaultNotInTheCell + plan.Message = "not replacing cell " + c.Name + + ": its Node joined and advertises no GPU, and no cell node advertises one — " + + "the fault is in the workload cluster, so a rebuilt VM would fail the same way. " + + "Fix the capacity provider (see CapacityProviderReady) and the cell is replaced then" + return plan + } if c.FailureCount >= MaxFailuresPerIndex { plan.Stalled = true plan.Reason = cellsv1alpha1.ReasonCellReplacementExhausted @@ -255,19 +270,45 @@ func PlanMembership(in MembershipInput) MembershipPlan { // ShouldReplace reports whether a failed cell's backoff has expired, so the // reconciler can delete it and let the next pass recreate the index. -func ShouldReplace(c cellsv1alpha1.CellStatus, now time.Time) bool { +// +// providerUsable is the pool-wide capacity verdict: false means NO cell node +// anywhere advertises a GPU. See RebuildWouldNotHelp for why that vetoes a +// replacement. +func ShouldReplace(c cellsv1alpha1.CellStatus, now time.Time, providerUsable bool) bool { if c.Phase != cellsv1alpha1.CellPhaseFailed { return false } if c.FailureCount >= MaxFailuresPerIndex { return false } + if RebuildWouldNotHelp(c, providerUsable) { + return false + } if c.LastTransitionTime == nil { return true } return now.Sub(c.LastTransitionTime.Time) >= Backoff(c.FailureCount) } +// RebuildWouldNotHelp reports whether replacing a failed cell cannot possibly fix +// it, because the fault is not in the cell. +// +// The shape: the cell's Node joined and went Ready, it advertises no GPU, and no +// cell node ANYWHERE advertises one either. That is a workload-cluster fault — HAMi +// missing, its DaemonSet not tolerating the pool's taints, a broken inner CNI — and +// a fresh VM will reach exactly the same place. Rebuilding costs a GPU allocation, a +// full root-disk clone, a boot and a join per attempt, up to MaxFailuresPerIndex per +// index, and destroys the evidence each time. Observed on hardware: HAMi could not +// register, and the pool's answer was to rebuild the VM. +// +// A single broken cell in an otherwise healthy pool is NOT this: the provider +// reports usable as soon as any node advertises a device, so that cell is still +// replaced. And with no cells at all the provider is usable by definition, so a pool +// can never wedge itself out of ever creating one. +func RebuildWouldNotHelp(c cellsv1alpha1.CellStatus, providerUsable bool) bool { + return !providerUsable && c.NodeReady && c.CapacityDevices == 0 +} + func minDuration(a, b time.Duration) time.Duration { if a == 0 || (b > 0 && b < a) { return b diff --git a/internal/controller/membership_test.go b/internal/controller/membership_test.go index 1011c13..1af3817 100644 --- a/internal/controller/membership_test.go +++ b/internal/controller/membership_test.go @@ -185,22 +185,62 @@ func TestBackoffGrowsAndCaps(t *testing.T) { func TestShouldReplaceHonoursBackoff(t *testing.T) { failedAt := metav1.Time{Time: now.Add(-10 * time.Second)} c := cellsv1alpha1.CellStatus{Phase: cellsv1alpha1.CellPhaseFailed, FailureCount: 1, LastTransitionTime: &failedAt} - if ShouldReplace(c, now) { + if ShouldReplace(c, now, true) { t.Error("replaced before the 30s backoff expired") } c.LastTransitionTime = &metav1.Time{Time: now.Add(-31 * time.Second)} - if !ShouldReplace(c, now) { + if !ShouldReplace(c, now, true) { t.Error("did not replace after the backoff expired") } c.FailureCount = MaxFailuresPerIndex - if ShouldReplace(c, now) { + if ShouldReplace(c, now, true) { t.Error("replaced an exhausted index") } - if ShouldReplace(cellsv1alpha1.CellStatus{Phase: cellsv1alpha1.CellPhaseReady}, now) { + if ShouldReplace(cellsv1alpha1.CellStatus{Phase: cellsv1alpha1.CellPhaseReady}, now, true) { t.Error("replaced a healthy cell") } } +// TestShouldNotRebuildAgainstAPoolWideFault: a cell whose Node joined and +// advertises nothing, in a pool where nothing advertises anything, is not a broken +// cell — it is a broken workload cluster. Rebuilding it costs a GPU allocation, a +// full root-disk clone, a boot and a join per attempt and cannot succeed. Measured +// on hardware: HAMi could not register and the pool's answer was to rebuild the VM. +func TestShouldNotRebuildAgainstAPoolWideFault(t *testing.T) { + failedAt := metav1.Time{Time: now.Add(-10 * time.Minute)} + joined := cellsv1alpha1.CellStatus{ + Name: "cells-0", Phase: cellsv1alpha1.CellPhaseFailed, FailureCount: 1, + LastTransitionTime: &failedAt, NodeReady: true, CapacityDevices: 0, + } + + if ShouldReplace(joined, now, false) { + t.Error("rebuilt a cell against a pool-wide provider fault") + } + if !ShouldReplace(joined, now, true) { + t.Error("a single cell advertising nothing in an otherwise healthy pool must still be replaced") + } + + // A cell that never joined tells us nothing about the provider, so the veto + // must not catch it — that failure really may be the cell's. + neverJoined := joined + neverJoined.NodeReady = false + if !ShouldReplace(neverJoined, now, false) { + t.Error("a cell that never joined was not replaced; its fault is not the provider's") + } + + // And the stall is reported, not silent. + plan := PlanMembership(MembershipInput{ + Desired: 1, Cells: []cellsv1alpha1.CellStatus{joined}, Now: now, + WorkloadReachable: true, EverReady: true, ProviderUsable: false, + }) + if !plan.Stalled || plan.Reason != cellsv1alpha1.ReasonFaultNotInTheCell { + t.Errorf("plan = %+v, want a stall naming the pool-wide fault", plan) + } + if len(plan.Create) != 0 || len(plan.Drain) != 0 { + t.Errorf("plan acted on a fault it cannot fix: %+v", plan) + } +} + func TestPlanRequeuesWhenABackoffIsPending(t *testing.T) { failedAt := metav1.Time{Time: now.Add(-10 * time.Second)} in := input(1, cellsv1alpha1.CellStatus{