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{