From 870f5219cf847eb8a09cb411624317126ee2c2d3 Mon Sep 17 00:00:00 2001 From: William Rizzo Date: Sun, 9 Aug 2026 06:48:24 +0000 Subject: [PATCH] feat(updates): see template drift, and optionally roll cells to fix it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2. Two layers, because the first turned out to be missing entirely. **Drift was invisible.** Each cell was stamped with the hash of the template it was created from, and nothing ever read it back — so changing spec.cell left the pool silently unaware. The hash is now observed (OuterState.TemplateHash), surfaced per cell as status.cells[].templateHash, and compared in a new Updated condition. That alone is worth having: knowing you have drifted is useful even if you replace cells by hand. **spec.updatePolicy.type: RollingUpdate** then acts on it, opt-in. Replacement is deletion plus recreation — there is no in-place update of a VM's image — so every gate here is protecting running work: one cell at a time, only cells the capacity provider reports IDLE, not during a resize (a rollout must not race a scaling decision for the index it is about to free), not while the workload cluster is unreachable (then "is this cell busy?" has no answer), lowest index first so the order is predictable. The existing drain gate re-checks allocations again before the object goes. On a pool whose stale cells are all busy it makes no progress, indefinitely, and says so via UpdateBlocked. It will not evict anything to make room — this operator waits for a GPU to be released rather than taking it away, which is why it holds no pods/eviction right at all. A cell with no recorded hash counts as CURRENT, not stale: it predates the field, and treating unknown as out-of-date would replace an entire healthy pool the first time someone switched the policy on. **And a latent bug this exposed, which was never about rolling updates.** Cell names are reused — index 0 is always -0 — and status rows are keyed by name, so a replacement inherited its predecessor's Draining phase and was deleted on the pass that created it: create, destroy, create, destroy, with no timeout that could ever break the loop. The harness caught it as "the stale cell was never replaced"; the trace showed a new guest UID every pass. Rows describing a different guest UID no longer lend their phase. The failure counter still carries, because the replacement backoff is counted per index rather than per incarnation. Any replacement path could hit this — a failed cell, or a scale-down and scale-up on the same index — and it was masked only because nothing had previously refilled an index whose row still said Draining. Also: idleness was read from the capacity provider only when automatic scale-down was enabled, so a rolling update saw zero idle cells and silently never acted. Both destructive paths need it, so both ask for it now. Signed-off-by: William Rizzo --- CHANGELOG.md | 31 ++ api/v1alpha1/conditions.go | 13 + api/v1alpha1/gpucellpool_types.go | 39 +++ api/v1alpha1/zz_generated.deepcopy.go | 20 ++ .../crds/cells.kubeswift.io_gpucellpools.yaml | 19 ++ .../cells.kubeswift.io_gpucellpools.yaml | 19 ++ docs/README.md | 1 + docs/limitations.md | 21 +- docs/runbook.md | 19 ++ docs/updates.md | 95 +++++++ internal/controller/controller.go | 38 ++- internal/controller/helpers.go | 14 + internal/controller/rollout.go | 152 ++++++++++ internal/controller/rollout_test.go | 266 ++++++++++++++++++ internal/controller/status.go | 8 + internal/controller/suite_test.go | 30 ++ internal/provisioner/clusterapi.go | 4 + internal/provisioner/provisioner.go | 6 + internal/provisioner/swiftguest.go | 1 + 19 files changed, 777 insertions(+), 19 deletions(-) create mode 100644 docs/updates.md create mode 100644 internal/controller/rollout.go create mode 100644 internal/controller/rollout_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index eb5e0ea..edfc31d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,37 @@ All notable changes to this project are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +## [Unreleased] + +### Added + +- **Template drift is visible.** Every cell records the template it was created + from, surfaced as `status.cells[].templateHash`, and a new `Updated` condition + compares it with the pool's current template. The hash was being written to each + cell and never read back, so a `spec.cell` change was invisible to the pool. +- **`spec.updatePolicy.type: RollingUpdate`** replaces stale cells, one at a time, + through the same drain gate automatic scale-down uses: only cells the capacity + provider reports idle, never during a resize, never while another cell is coming + or going, never while the workload cluster is unreachable, lowest index first. + Opt-in, because a template edit is not consent to destroy running work — and it + stalls visibly (`UpdateBlocked`) rather than evicting anything. See + `docs/updates.md`. + +### Fixed + +- **A replacement cell inherited its predecessor's teardown.** Cell names are + reused (index 0 is always `-0`) and status rows are keyed by name, so a + freshly created cell adopted the previous incarnation's `Draining` phase and was + deleted on the pass that created it — create, destroy, create, destroy, with no + timeout that could break the loop. Rows for a different guest UID no longer lend + their phase; the failure counter still carries, because the replacement backoff + is counted per index. This affected any replacement path, not just the new + rolling update, and was only masked because nothing had previously refilled an + index whose row still said `Draining`. +- Cell idleness was read from the capacity provider only when *automatic + scale-down* was enabled, so a rolling update always saw zero idle cells and + silently never acted. + ## [v0.1.0] — 2026-08-08 First release. Every capability below has been run on real hardware (one diff --git a/api/v1alpha1/conditions.go b/api/v1alpha1/conditions.go index 504ca93..ea626b5 100644 --- a/api/v1alpha1/conditions.go +++ b/api/v1alpha1/conditions.go @@ -31,6 +31,16 @@ const ( // being drained or cordoned. Cells are never offline-migrated, so this is a // signal for the operator (and, later, for automated replacement). ConditionCellDrainRequested = "CellDrainRequested" + + // ConditionUpdated reports whether every cell was created from the CURRENT + // cell template. False means at least one cell is running an older shape — a + // previous image, guest class or interface set. + // + // It is only ever informational until spec.updatePolicy.type is RollingUpdate, + // because replacing a cell destroys whatever the old one was still running + // unless it is drained first. Knowing you have drifted is useful on its own; + // acting on it is a decision the operator opts into. + ConditionUpdated = "Updated" ) // Condition reasons. Every reason names a distinct operator action: "your token @@ -44,6 +54,9 @@ const ( ReasonCellCreating = "CellCreating" ReasonCellDraining = "CellDraining" ReasonTemplateChanged = "TemplateChanged" + ReasonAllCellsCurrent = "AllCellsCurrent" + ReasonRollingUpdate = "RollingUpdate" + ReasonUpdateBlocked = "UpdateBlocked" ReasonConnected = "Connected" ReasonUnreachable = "Unreachable" diff --git a/api/v1alpha1/gpucellpool_types.go b/api/v1alpha1/gpucellpool_types.go index 8da8023..88323e4 100644 --- a/api/v1alpha1/gpucellpool_types.go +++ b/api/v1alpha1/gpucellpool_types.go @@ -51,6 +51,38 @@ type GPUCellPoolSpec struct { // Deletion controls teardown behaviour for the pool. // +optional Deletion *DeletionSpec `json:"deletion,omitempty"` + + // UpdatePolicy decides what happens to existing cells when spec.cell changes. + // +optional + UpdatePolicy *UpdatePolicySpec `json:"updatePolicy,omitempty"` +} + +// Cell update modes. +const ( + // UpdateManual leaves existing cells alone when the template changes. The + // pool reports Updated=False/TemplateChanged and which cells are stale; you + // replace them when it suits you. + UpdateManual = "Manual" + // UpdateRolling replaces stale cells one at a time, using the same drain gate + // as automatic scale-down. + UpdateRolling = "RollingUpdate" +) + +// UpdatePolicySpec decides what happens to cells already running an older +// spec.cell than the pool now declares. +// +// Manual is the default because replacing a cell destroys whatever the old one was +// still running unless it is drained first, and a template edit is not consent to +// that. RollingUpdate is opt-in, and it is deliberately slow: one cell at a time, +// only cells the capacity provider reports as IDLE, and never while another cell is +// already being created or drained. On a pool whose cells are all busy it will +// therefore make no progress — and says so, rather than forcing its way through. +type UpdatePolicySpec struct { + // Type is Manual (default) or RollingUpdate. + // +kubebuilder:validation:Enum=Manual;RollingUpdate + // +kubebuilder:default=Manual + // +optional + Type string `json:"type,omitempty"` } // Scale-down modes. @@ -556,6 +588,13 @@ type CellStatus struct { // +optional ReadyOnce bool `json:"readyOnce,omitempty"` + // TemplateHash is the cell template this cell was CREATED from. When it differs + // from the pool's current template the cell is running an older shape, which is + // what the Updated condition reports — and, under + // updatePolicy.type: RollingUpdate, what gets it replaced. + // +optional + TemplateHash string `json:"templateHash,omitempty"` + // LastTransitionTime is when Phase last changed. // +optional LastTransitionTime *metav1.Time `json:"lastTransitionTime,omitempty"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 982f557..5e4437e 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -381,6 +381,11 @@ func (in *GPUCellPoolSpec) DeepCopyInto(out *GPUCellPoolSpec) { *out = new(DeletionSpec) (*in).DeepCopyInto(*out) } + if in.UpdatePolicy != nil { + in, out := &in.UpdatePolicy, &out.UpdatePolicy + *out = new(UpdatePolicySpec) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GPUCellPoolSpec. @@ -537,6 +542,21 @@ func (in *PhysicalCapacityStatus) DeepCopy() *PhysicalCapacityStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *UpdatePolicySpec) DeepCopyInto(out *UpdatePolicySpec) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UpdatePolicySpec. +func (in *UpdatePolicySpec) DeepCopy() *UpdatePolicySpec { + if in == nil { + return nil + } + out := new(UpdatePolicySpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *WorkloadCapacityStatus) DeepCopyInto(out *WorkloadCapacityStatus) { *out = *in diff --git a/charts/gpucellpool/crds/cells.kubeswift.io_gpucellpools.yaml b/charts/gpucellpool/crds/cells.kubeswift.io_gpucellpools.yaml index 73f5dbe..b84a12a 100644 --- a/charts/gpucellpool/crds/cells.kubeswift.io_gpucellpools.yaml +++ b/charts/gpucellpool/crds/cells.kubeswift.io_gpucellpools.yaml @@ -442,6 +442,18 @@ spec: format: int32 minimum: 0 type: integer + updatePolicy: + description: UpdatePolicy decides what happens to existing cells when + spec.cell changes. + properties: + type: + default: Manual + description: Type is Manual (default) or RollingUpdate. + enum: + - Manual + - RollingUpdate + type: string + type: object workloadCluster: description: WorkloadCluster is the cluster the cells join and where HAMi runs. @@ -640,6 +652,13 @@ spec: ReadyOnce records that this cell reached Ready at least once, so a later Ready after a regression is not mistaken for a startup. type: boolean + templateHash: + description: |- + TemplateHash is the cell template this cell was CREATED from. When it differs + from the pool's current template the cell is running an older shape, which is + what the Updated condition reports — and, under + updatePolicy.type: RollingUpdate, what gets it replaced. + type: string required: - index - name diff --git a/config/crd/bases/cells.kubeswift.io_gpucellpools.yaml b/config/crd/bases/cells.kubeswift.io_gpucellpools.yaml index 73f5dbe..b84a12a 100644 --- a/config/crd/bases/cells.kubeswift.io_gpucellpools.yaml +++ b/config/crd/bases/cells.kubeswift.io_gpucellpools.yaml @@ -442,6 +442,18 @@ spec: format: int32 minimum: 0 type: integer + updatePolicy: + description: UpdatePolicy decides what happens to existing cells when + spec.cell changes. + properties: + type: + default: Manual + description: Type is Manual (default) or RollingUpdate. + enum: + - Manual + - RollingUpdate + type: string + type: object workloadCluster: description: WorkloadCluster is the cluster the cells join and where HAMi runs. @@ -640,6 +652,13 @@ spec: ReadyOnce records that this cell reached Ready at least once, so a later Ready after a regression is not mistaken for a startup. type: boolean + templateHash: + description: |- + TemplateHash is the cell template this cell was CREATED from. When it differs + from the pool's current template the cell is running an older shape, which is + what the Updated condition reports — and, under + updatePolicy.type: RollingUpdate, what gets it replaced. + type: string required: - index - name diff --git a/docs/README.md b/docs/README.md index 4d1a9c0..268e883 100644 --- a/docs/README.md +++ b/docs/README.md @@ -10,6 +10,7 @@ Start here if you are installing or operating a pool. | [concepts](concepts.md) | the two-layer model — cells, the two identities, the two capacities, layered isolation | | [api-reference](api-reference.md) | the full `GPUCellPool` v1alpha1 spec/status, generated from the Go types and the validating webhook | | [autoscaling](autoscaling.md) | `spec.autoscaling` — both directions, the safety gates, the remembered cell shape | +| [updates](updates.md) | changing `spec.cell`: seeing drift, replacing cells by hand, `updatePolicy.type: RollingUpdate` | | [networking](networking.md) | the routable-interface requirement, `nodeIPFrom`, NADs, DNS, `port-forward` | | [security](security.md) | why creating a pool is node-root-equivalent authority, the webhook, the two RBAC scopes | | [cell-image](cell-image.md) | building and publishing a cell image with `hack/build-cell-image.sh` | diff --git a/docs/limitations.md b/docs/limitations.md index a773050..d6013b4 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -15,22 +15,6 @@ demand signal (`docs/autoscaling.md`), and readiness all fail loudly rather than silently reading zero. **`DevicePlugin` mode is the only implementation.** Use it (the default) even if your HAMi install also has DRA mode available. -## No rolling update on `guestTemplate` change - -Tracked as [#2](https://github.com/kubeswift-io/gpucellpool/issues/2). - -Changing `spec.cell.guestTemplate` (a new `imageRef`, a driver bump, a -different `guestClassRef`) bumps the per-cell template-hash annotation but -**does not roll existing cells**. `status.conditions` will not tell you a -template drifted either — there is no `Updated` condition in v1alpha1. - -This is the most likely real operational task you will hit: **updating the -NVIDIA driver (or anything else) in the cell image means manually recreating -every cell**, one at a time, and doing the *inner* drain yourself first — -cordon and drain the cell's workload Node before deleting the cell's -`SwiftGuest`/`Machine`, or you destroy running HAMi workloads. See -`docs/runbook.md` for the sequence. - ## No automated outer-drain sequencing Tracked as [#3](https://github.com/kubeswift-io/gpucellpool/issues/3). @@ -91,6 +75,11 @@ is 1 — use `resourceClaimTemplateName` for anything larger. ## What is *not* a limitation, stated for clarity +Replacing cells after a `spec.cell` change is **implemented**: the `Updated` +condition reports drift, and `updatePolicy.type: RollingUpdate` replaces stale +cells one at a time behind the drain gate. See `docs/updates.md`. It is opt-in +because a template edit is not consent to destroy running work. + - Scale-up and scale-down are both implemented (`docs/autoscaling.md`) — the earlier design draft that called scale-down "postponed" is stale; ignore any doc under `docs/design/` that still says so (they carry a banner). diff --git a/docs/runbook.md b/docs/runbook.md index 907a37e..ef12fbe 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -21,6 +21,7 @@ kubectl get cellpool -n -o jsonpath='{range .status.conditions[*]}{. | `PhysicalGPUsAvailable` | the infrastructure cluster's GPU inventory | | `CapacityAvailable` | the workload cluster's GPU is full, or unreadable | | `ScalingActive` | demand could not be read (only present when autoscaling is on) | +| `Updated` | cells are running an older `spec.cell` than the pool declares — see below | | `Ready` alone | individual cells — read `status.cells[]` | Per-cell detail, which names the layer and the reason: @@ -193,6 +194,24 @@ An empty providerID with the Machine in `Provisioning` means the provider is sti working. A `Provisioned` Machine with no providerID is a capi-kubeswift problem, not a pool problem. +### `Updated=False` — cells are running an older template + +Expected after any `spec.cell` edit. The reason says what will happen next: + +| Reason | Meaning | +|---|---| +| `TemplateChanged` | drift detected, and `updatePolicy.type` is `Manual` — nothing will be replaced. Replace cells yourself, or switch to `RollingUpdate`. See `docs/updates.md` | +| `RollingUpdate` | a cell is being replaced right now | +| `UpdateBlocked` | a rolling update is wanted but cannot proceed. The message names the cause: every stale cell still holds workloads, the pool is mid-resize, another cell is already being replaced, or the workload cluster is unreachable | + +`status.cells[].templateHash` tells you which cells are stale. An empty hash +counts as current — it predates the field, and treating unknown as out-of-date +would replace a healthy pool. + +A rolling update that is blocked on busy cells stays blocked indefinitely, by +design; it will not evict anything. `kubectl drain ` in the workload +cluster releases the GPU and the rollout proceeds on its next pass. + ### Capacity numbers look stale They are, and deliberately: a failed read retains the previous values rather than diff --git a/docs/updates.md b/docs/updates.md new file mode 100644 index 0000000..8e1dd9a --- /dev/null +++ b/docs/updates.md @@ -0,0 +1,95 @@ +# Changing the cell template + +Editing `spec.cell` — a new `imageRef` after a driver bump, a different +`guestClassRef`, another interface — changes the shape of a cell. Existing cells +are already running the old shape, and there is no in-place update of a VM's +image: adopting a new template means replacing cells. + +The pool always tells you the cells have drifted. Whether it replaces them is +yours to decide. + +## Seeing drift + +Every cell records the template it was created from, and the `Updated` condition +compares that with the pool's current one: + +```bash +kubectl get cellpool -n \ + -o jsonpath='{range .status.conditions[?(@.type=="Updated")]}{.status} {.reason} {.message}{"\n"}{end}' + +# which cells specifically +kubectl get cellpool -n \ + -o jsonpath='{range .status.cells[*]}{.name} {.templateHash}{"\n"}{end}' +``` + +| `Updated` | Reason | Meaning | +|---|---|---| +| True | `AllCellsCurrent` | every cell matches the current template | +| False | `TemplateChanged` | cells are out of date and `updatePolicy.type` is `Manual`, so nothing will happen to them | +| False | `RollingUpdate` | a cell is being replaced right now | +| False | `UpdateBlocked` | replacement is wanted but cannot proceed — the message says why | + +A cell with an empty `templateHash` is treated as current, not stale. It predates +the hash being read back, and treating unknown as out-of-date would replace a +whole healthy pool the first time you switched the policy on. + +## Manual (the default) + +Nothing is replaced. You do it when it suits you, one cell at a time: + +```bash +# in the WORKLOAD cluster: stop new work landing, move what is there +kubectl cordon +kubectl drain --ignore-daemonsets --delete-emptydir-data + +# in the INFRASTRUCTURE cluster: remove the cell; the pool refills the index +kubectl delete swiftguest -n # or: kubectl delete machine -n +``` + +Wait for the replacement to reach `Ready` before doing the next one. The pool +refills the freed index from the current template, so the cell comes back with +the same name and the new shape. + +## RollingUpdate + +```yaml +spec: + updatePolicy: + type: RollingUpdate +``` + +The pool replaces stale cells itself — deliberately slowly, and through the same +gate automatic scale-down uses: + +- **one cell at a time**, never while another cell is being created or drained; +- **only cells the capacity provider reports as idle.** A cell whose allocations + cannot be read is never replaced either — "empty" and "unknown" are different + answers; +- **not while the pool is resizing**, so a rollout cannot race a scaling decision + for the index it is about to free; +- **not while the workload cluster is unreachable**, because then "is this cell + busy?" has no answer; +- lowest index first, so the order is predictable. + +The drain gate re-checks allocations again immediately before the cell is +removed, so a workload that lands between the decision and the deletion is still +safe. + +### It can stall, and that is the point + +On a pool whose stale cells all hold workloads, a rolling update makes no +progress — indefinitely. `Updated` stays False with reason `UpdateBlocked` and a +message naming the cause. It will not evict anything to make room: this operator +waits for a GPU to be released rather than taking it away, which is why it holds +no `pods/eviction` right in the workload cluster at all. + +If you need the cell back sooner, drain it yourself with `kubectl drain` and the +rollout proceeds on its next pass. + +### Capacity during a rollout + +Replacing a cell costs its capacity for the duration of a full cell startup +(measured 4m45s). There is no surge: a cell holds a *physical* GPU, so bringing up +a replacement alongside the old one would need a spare device. On a single-cell +pool a rolling update therefore means a gap in service — which is a reason to run +`replicas: 2` if the workload cannot tolerate one. diff --git a/internal/controller/controller.go b/internal/controller/controller.go index 261dfd2..02be661 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -168,7 +168,7 @@ func (r *GPUCellPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) // Idleness is read from the capacity provider, not guessed: only a cell that // holds nothing may be removed automatically. var idle []string - if autoScaleDown(&pool) && reachable && provider != nil { + if needsIdleness(&pool) && reachable && provider != nil { idle = r.idleCells(ctx, provider, cells) } @@ -255,6 +255,22 @@ func (r *GPUCellPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) } } + // A template change replaces cells only when asked to, and only through the same + // drain gate scale-down uses. Deliberately after the membership plan: a rollout + // must not race a scale decision for the index it is about to free. + rollout := PlanRollout(RolloutInput{ + Policy: pool.Spec.UpdatePolicy, + DesiredHash: cellid.TemplateHash(pool.Spec.Cell.GuestTemplate.Raw), + Cells: cells, + IdleCells: idle, + WorkloadReachable: reachable, + AtDesiredSize: liveCells(cells) == scale.Desired && len(plan.Create) == 0, + }) + if rollout.Drain != "" { + r.markDraining(cells, rollout.Drain) + r.event(&pool, corev1.EventTypeNormal, rollout.Reason, rollout.Message) + } + // Draining cells: gate on allocations, then remove. for i := range cells { if cells[i].Phase != cellsv1alpha1.CellPhaseDraining { @@ -265,7 +281,7 @@ func (r *GPUCellPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) } } - r.writeStatus(ctx, &pool, cells, plan, scale, demand, demandKnown, + r.writeStatus(ctx, &pool, cells, plan, scale, rollout, demand, demandKnown, freeGPUs, health, cap0, capacityKnown, reachable, reachReason, reachMsg) if err := r.Status().Update(ctx, &pool); err != nil { return ctrl.Result{}, fmt.Errorf("updating status: %w", err) @@ -316,6 +332,20 @@ func (r *GPUCellPoolReconciler) discoverCells( return nil, fmt.Errorf("observing cell %s: %w", name, err) } + // Cell names are REUSED: index 0 is always -0, so a replacement lands on + // the name of the cell it replaced. A status row for a different incarnation + // must not lend its phase to the new one — carrying `Draining` across made a + // freshly created cell inherit its predecessor's teardown and be deleted on the + // very pass that created it, forever, with no timeout that could ever break it. + // The guest UID distinguishes them, exactly as it does for a stale workload + // Node. The failure counter is the one thing that must survive, because the + // replacement backoff is counted per index, not per incarnation. + if prev.GuestUID != "" && outer.UID != "" && prev.GuestUID != outer.UID { + prev = cellsv1alpha1.CellStatus{ + Name: prev.Name, Index: prev.Index, FailureCount: prev.FailureCount, + } + } + obs := Observation{ Current: phaseOr(prev.Phase, cellsv1alpha1.CellPhasePending), LastTransition: transitionOr(prev.LastTransitionTime, r.now()), @@ -409,6 +439,7 @@ func (r *GPUCellPoolReconciler) cellStatus( Phase: dec.Phase, Message: dec.Message, GuestUID: outer.UID, + TemplateHash: outer.TemplateHash, HostNode: outer.HostNode, Devices: outer.GPUDevices, NodeReady: obs.Node.Ready, @@ -648,7 +679,7 @@ func (r *GPUCellPoolReconciler) reconcileDeletion( func (r *GPUCellPoolReconciler) writeStatus( ctx context.Context, pool *cellsv1alpha1.GPUCellPool, cells []cellsv1alpha1.CellStatus, plan MembershipPlan, - scale ScaleDecision, demand capacity.Demand, demandKnown bool, + scale ScaleDecision, rollout RolloutDecision, demand capacity.Demand, demandKnown bool, freeGPUs *int, health capacity.Health, cap0 capacity.Capacity, capacityKnown bool, reachable bool, reachReason, reachMsg string, ) { @@ -719,6 +750,7 @@ func (r *GPUCellPoolReconciler) writeStatus( FreeGPUs: freeGPUs, CellsWaitingForGPU: waiting, Membership: plan, + Rollout: rollout, Progressing: creating > 0 || draining > 0 || len(plan.Create) > 0, Autoscaling: autoscalingEnabled(pool), Scale: scale, diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index 127da34..c3c9569 100644 --- a/internal/controller/helpers.go +++ b/internal/controller/helpers.go @@ -125,6 +125,20 @@ func autoScaleDown(pool *cellsv1alpha1.GPUCellPool) bool { return autoscalingEnabled(pool) && pool.Spec.Autoscaling.ScaleDown == cellsv1alpha1.ScaleDownAuto } +// rollingUpdate reports whether the pool replaces stale cells itself. +func rollingUpdate(pool *cellsv1alpha1.GPUCellPool) bool { + return pool.Spec.UpdatePolicy != nil && + pool.Spec.UpdatePolicy.Type == cellsv1alpha1.UpdateRolling +} + +// needsIdleness reports whether anything this pass may want to REMOVE a cell, and +// therefore needs to know which cells hold nothing. Both automatic scale-down and a +// rolling update destroy a cell, so both require it — reading it for only one of them +// left the other seeing no idle cells at all and silently never acting. +func needsIdleness(pool *cellsv1alpha1.GPUCellPool) bool { + return autoScaleDown(pool) || rollingUpdate(pool) +} + // liveCells counts cells that exist and are not on their way out — the baseline a // scaling decision grows from. func liveCells(cells []cellsv1alpha1.CellStatus) int32 { diff --git a/internal/controller/rollout.go b/internal/controller/rollout.go new file mode 100644 index 0000000..72446a4 --- /dev/null +++ b/internal/controller/rollout.go @@ -0,0 +1,152 @@ +package controller + +import ( + cellsv1alpha1 "github.com/kubeswift-io/gpucellpool/api/v1alpha1" +) + +// RolloutInput is what a rolling-update decision is made from. Everything is +// observed; nothing is remembered between reconciles. +type RolloutInput struct { + // Policy is spec.updatePolicy (nil means Manual). + Policy *cellsv1alpha1.UpdatePolicySpec + + // DesiredHash is the hash of the CURRENT cell template. + DesiredHash string + + // Cells is the pool's cells as observed this pass. + Cells []cellsv1alpha1.CellStatus + + // IdleCells are the cells the capacity provider reports as holding nothing, + // which is the same gate automatic scale-down uses. A cell whose allocations + // could not be READ is absent from this list, and therefore never replaced. + IdleCells []string + + // WorkloadReachable is false when the inner cluster could not be reached. No + // cell may be replaced then: we cannot tell whether it is holding work. + WorkloadReachable bool + + // AtDesiredSize is false while the pool is still growing or shrinking. A + // rollout must not race a scaling decision for the same index. + AtDesiredSize bool +} + +// RolloutDecision is the outcome. +type RolloutDecision struct { + // Drain names the cell to replace, or is empty. + Drain string + // UpToDate is true when every cell matches the current template. + UpToDate bool + // StaleCells counts cells running an older template, whether or not one is + // being replaced right now. + StaleCells int + Reason string + Message string +} + +// PlanRollout decides whether to replace one stale cell. +// +// Replacement is deletion plus recreation — there is no in-place update of a VM's +// image — so every refusal here is protecting running work. The gates, in the order +// they are checked: +// +// 1. the operator must have asked for it (Manual is the default); +// 2. the inner cluster must be readable, or "is this cell busy?" has no answer; +// 3. the pool must already be at its desired size, so a rollout cannot race a +// scale decision for the same index; +// 4. nothing else may be mid-flight — one cell at a time, always; +// 5. the cell must be reported IDLE. The drain gate re-checks allocations again +// before the object is deleted, so a workload that lands in between is still +// safe. +// +// A pool whose stale cells are all busy makes no progress, indefinitely. That is +// correct, and the reason says so instead of leaving an operator to wonder. +func PlanRollout(in RolloutInput) RolloutDecision { + stale := staleCells(in.Cells, in.DesiredHash) + out := RolloutDecision{StaleCells: len(stale), UpToDate: len(stale) == 0} + + if out.UpToDate { + out.Reason = cellsv1alpha1.ReasonAllCellsCurrent + return out + } + + out.Reason = cellsv1alpha1.ReasonTemplateChanged + out.Message = itoa(len(stale)) + " cell(s) were created from an older template" + + if in.Policy == nil || in.Policy.Type != cellsv1alpha1.UpdateRolling { + // Informational only: say what is stale, change nothing. + out.Message += "; updatePolicy.type is Manual, so they are left alone" + return out + } + + blocked := func(why string) RolloutDecision { + out.Reason = cellsv1alpha1.ReasonUpdateBlocked + out.Message = itoa(len(stale)) + " cell(s) out of date: " + why + return out + } + + if !in.WorkloadReachable { + return blocked("the workload cluster is unreachable, so it is unknown whether they hold work") + } + if !in.AtDesiredSize { + return blocked("the pool is still resizing; a rollout must not race a scaling decision") + } + if busy := midFlight(in.Cells); busy != "" { + return blocked("cell " + busy + " is already being replaced or created") + } + + for _, c := range stale { + if contains(in.IdleCells, c) { + return RolloutDecision{ + Drain: c, + StaleCells: len(stale), + Reason: cellsv1alpha1.ReasonRollingUpdate, + Message: "replacing " + c + " to adopt the current template (" + + itoa(len(stale)) + " out of date)", + } + } + } + return blocked("none of them is reported idle — a cell whose allocations cannot be read is never replaced either") +} + +// staleCells returns the names of cells created from a different template, in +// index order, so a rollout is deterministic and reads as a rolling one. +// +// A cell whose hash is EMPTY is not stale: it predates the annotation being read +// back, and treating unknown as out-of-date would replace a whole healthy pool the +// first time this policy was switched on. +func staleCells(cells []cellsv1alpha1.CellStatus, desired string) []string { + var out []string + for _, c := range cells { + switch c.Phase { + case cellsv1alpha1.CellPhaseDraining, cellsv1alpha1.CellPhaseDeleting: + continue + } + if c.TemplateHash != "" && c.TemplateHash != desired { + out = append(out, c.Name) + } + } + return out +} + +// midFlight names a cell that is already coming or going, if any. +func midFlight(cells []cellsv1alpha1.CellStatus) string { + for _, c := range cells { + switch c.Phase { + case cellsv1alpha1.CellPhaseDraining, cellsv1alpha1.CellPhaseDeleting, + cellsv1alpha1.CellPhasePending, cellsv1alpha1.CellPhaseAllocatingGPU, + cellsv1alpha1.CellPhaseGuestProvisioning, cellsv1alpha1.CellPhaseBooting, + cellsv1alpha1.CellPhaseJoining, cellsv1alpha1.CellPhaseAwaitingGPUCapacity: + return c.Name + } + } + return "" +} + +func contains(haystack []string, needle string) bool { + for _, h := range haystack { + if h == needle { + return true + } + } + return false +} diff --git a/internal/controller/rollout_test.go b/internal/controller/rollout_test.go new file mode 100644 index 0000000..f740168 --- /dev/null +++ b/internal/controller/rollout_test.go @@ -0,0 +1,266 @@ +package controller + +import ( + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + + cellsv1alpha1 "github.com/kubeswift-io/gpucellpool/api/v1alpha1" +) + +func rolling() *cellsv1alpha1.UpdatePolicySpec { + return &cellsv1alpha1.UpdatePolicySpec{Type: cellsv1alpha1.UpdateRolling} +} + +// twoStaleReadyCells is the happy path: both cells were built from an older +// template, both are Ready, both idle. +func twoStaleReadyCells() RolloutInput { + return RolloutInput{ + Policy: rolling(), + DesiredHash: "new", + Cells: []cellsv1alpha1.CellStatus{ + {Name: "pool-0", Index: 0, Phase: cellsv1alpha1.CellPhaseReady, TemplateHash: "old"}, + {Name: "pool-1", Index: 1, Phase: cellsv1alpha1.CellPhaseReady, TemplateHash: "old"}, + }, + IdleCells: []string{"pool-0", "pool-1"}, + WorkloadReachable: true, + AtDesiredSize: true, + } +} + +func TestRolloutReplacesOneCellAtATime(t *testing.T) { + got := PlanRollout(twoStaleReadyCells()) + if got.Drain != "pool-0" { + t.Errorf("Drain = %q, want pool-0 (lowest index first, so a rollout is deterministic)", got.Drain) + } + if got.UpToDate || got.StaleCells != 2 { + t.Errorf("got %+v, want 2 stale and not up to date", got) + } +} + +func TestRolloutIsOptIn(t *testing.T) { + // Manual is the default, and a template edit is not consent to destroy work. + in := twoStaleReadyCells() + in.Policy = nil + got := PlanRollout(in) + if got.Drain != "" { + t.Errorf("replaced a cell under the default policy: %+v", got) + } + if got.Reason != cellsv1alpha1.ReasonTemplateChanged { + t.Errorf("reason = %q, want the drift still REPORTED", got.Reason) + } + if !strings.Contains(got.Message, "left alone") { + t.Errorf("message should say nothing will happen: %q", got.Message) + } + + in.Policy = &cellsv1alpha1.UpdatePolicySpec{Type: cellsv1alpha1.UpdateManual} + if got := PlanRollout(in); got.Drain != "" { + t.Errorf("explicit Manual still replaced a cell: %+v", got) + } +} + +func TestRolloutNeverTouchesABusyCell(t *testing.T) { + in := twoStaleReadyCells() + in.IdleCells = nil + got := PlanRollout(in) + if got.Drain != "" { + t.Fatalf("replaced a cell holding workloads: %+v", got) + } + if got.Reason != cellsv1alpha1.ReasonUpdateBlocked || + !strings.Contains(got.Message, "reported idle") { + t.Errorf("got %+v, want a blocked reason that says why", got) + } + + // Only the busy one is skipped; the idle one still goes. + in.IdleCells = []string{"pool-1"} + if got := PlanRollout(in); got.Drain != "pool-1" { + t.Errorf("Drain = %q, want pool-1 — the idle one", got.Drain) + } +} + +func TestRolloutFreezesWhenTheWorkloadClusterIsUnreachable(t *testing.T) { + // "Is this cell busy?" has no answer, so the cell must not be replaced. + in := twoStaleReadyCells() + in.WorkloadReachable = false + got := PlanRollout(in) + if got.Drain != "" { + t.Fatalf("replaced a cell while the inner cluster was unreadable: %+v", got) + } + if !strings.Contains(got.Message, "unreachable") { + t.Errorf("message should name the cause: %q", got.Message) + } +} + +func TestRolloutWaitsForTheScalerAndForItself(t *testing.T) { + // Racing a scale decision for the index about to be freed. + in := twoStaleReadyCells() + in.AtDesiredSize = false + if got := PlanRollout(in); got.Drain != "" { + t.Errorf("rolled while the pool was still resizing: %+v", got) + } + + // One at a time: a cell already coming or going blocks the next. + for _, phase := range []cellsv1alpha1.CellPhase{ + cellsv1alpha1.CellPhaseDraining, cellsv1alpha1.CellPhaseBooting, + cellsv1alpha1.CellPhaseAllocatingGPU, cellsv1alpha1.CellPhaseJoining, + } { + in := twoStaleReadyCells() + in.Cells[1].Phase = phase + if got := PlanRollout(in); got.Drain != "" { + t.Errorf("rolled a second cell while one was %s: %+v", phase, got) + } + } +} + +func TestRolloutIgnoresCellsWithNoRecordedTemplate(t *testing.T) { + // A cell predating the hash being read back has an empty hash. Treating unknown + // as out-of-date would replace an entire healthy pool the first time this policy + // was switched on. + in := twoStaleReadyCells() + in.Cells[0].TemplateHash = "" + in.Cells[1].TemplateHash = "" + got := PlanRollout(in) + if got.Drain != "" || !got.UpToDate || got.StaleCells != 0 { + t.Errorf("got %+v, want an unknown hash treated as current", got) + } + if got.Reason != cellsv1alpha1.ReasonAllCellsCurrent { + t.Errorf("reason = %q, want AllCellsCurrent", got.Reason) + } +} + +func TestRolloutReportsUpToDate(t *testing.T) { + in := twoStaleReadyCells() + in.Cells[0].TemplateHash = "new" + in.Cells[1].TemplateHash = "new" + got := PlanRollout(in) + if !got.UpToDate || got.Drain != "" || got.Reason != cellsv1alpha1.ReasonAllCellsCurrent { + t.Errorf("got %+v, want up to date", got) + } +} + +// TestRollingUpdateReplacesACellEndToEnd drives the whole loop: a template change, +// the drain gate, deletion, and recreation from the NEW template at the same index. +// The pure function above cannot show that the recreated cell actually adopts the new +// shape — which is the only reason anyone turns this on. +func TestRollingUpdateReplacesACellEndToEnd(t *testing.T) { + f := newFixture(t, func(p *cellsv1alpha1.GPUCellPool) { + p.Spec.UpdatePolicy = &cellsv1alpha1.UpdatePolicySpec{Type: cellsv1alpha1.UpdateRolling} + }) + f.readyCell("cells-0") + + before := f.getPool().Status.Cells[0].TemplateHash + if before == "" { + t.Fatal("the cell recorded no template hash, so drift can never be detected") + } + if c := condition(t, f.getPool(), cellsv1alpha1.ConditionUpdated); c == nil || + c.Reason != cellsv1alpha1.ReasonAllCellsCurrent { + t.Fatalf("Updated = %+v, want AllCellsCurrent before any change", c) + } + + // Change the image the cells boot from — the driver-update case. + f.patchPool(func(p *cellsv1alpha1.GPUCellPool) { + p.Spec.Cell.GuestTemplate = runtime.RawExtension{Raw: []byte( + `{"guestClassRef":{"name":"gpu-worker"},"imageRef":{"name":"noble-580"}}`)} + }) + f.reconcile() + + pool := f.getPool() + if c := condition(t, pool, cellsv1alpha1.ConditionUpdated); c == nil || + c.Status != metav1.ConditionFalse { + t.Errorf("Updated = %+v, want False once a template drifted", c) + } + + // The cell is drained and removed, then recreated at the same index. + for i := 0; i < 8; i++ { + f.reconcile() + guests := f.guests() + if len(guests) == 1 { + if h := guests[0].GetAnnotations()[cellsv1alpha1.AnnotationTemplateHash]; h != before { + // Recreated from the new template. + spec := guests[0].Object["spec"].(map[string]any) + img, _ := spec["imageRef"].(map[string]any) + if img["name"] != "noble-580" { + t.Fatalf("the replacement did not adopt the new template: %v", spec["imageRef"]) + } + return + } + } + } + t.Fatalf("the stale cell was never replaced: %+v", f.getPool().Status.Cells) +} + +// A rolling update must obey the same gate scale-down does: a cell holding work is +// not replaced, however out of date it is. +func TestRollingUpdateWaitsForABusyCell(t *testing.T) { + f := newFixture(t, func(p *cellsv1alpha1.GPUCellPool) { + p.Spec.UpdatePolicy = &cellsv1alpha1.UpdatePolicySpec{Type: cellsv1alpha1.UpdateRolling} + }) + f.readyCell("cells-0") + f.gpuPod("holder", "cells-0") // a HAMi workload on the cell's GPU + + f.patchPool(func(p *cellsv1alpha1.GPUCellPool) { + p.Spec.Cell.GuestTemplate = runtime.RawExtension{Raw: []byte( + `{"guestClassRef":{"name":"gpu-worker"},"imageRef":{"name":"noble-580"}}`)} + }) + for i := 0; i < 4; i++ { + f.reconcile() + } + + if got := len(f.guests()); got != 1 { + t.Fatalf("got %d guests, want the busy cell left in place", got) + } + pool := f.getPool() + if pool.Status.Cells[0].Phase == cellsv1alpha1.CellPhaseDraining { + t.Error("started draining a cell that still holds a HAMi workload") + } + if c := condition(t, pool, cellsv1alpha1.ConditionUpdated); c == nil || + c.Reason != cellsv1alpha1.ReasonUpdateBlocked { + t.Errorf("Updated = %+v, want UpdateBlocked so the stall is visible", c) + } +} + +// TestReplacedCellDoesNotInheritTeardown pins a latent bug the rolling update +// exposed, which was never about rolling updates. +// +// Cell names are reused — index 0 is always -0 — so a replacement lands on the +// name of the cell it replaced, and status rows are keyed by name. The new cell +// therefore inherited its predecessor's `Draining` phase and was deleted on the pass +// that created it: create, destroy, create, destroy, with no timeout that could ever +// break the loop. Any replacement path reaches this (a failed cell, a scale-down +// followed by a scale-up on the same index), not just a template change. +func TestReplacedCellDoesNotInheritTeardown(t *testing.T) { + f := newFixture(t, nil) + f.reconcile() + first := f.guestUID("cells-0") + + // Put the row into teardown, as any removal path does, and delete the guest + // underneath it — the state the loop used to get stuck in. + f.patchPoolStatus(func(p *cellsv1alpha1.GPUCellPool) { + p.Status.Cells = []cellsv1alpha1.CellStatus{{ + Name: "cells-0", Index: 0, + Phase: cellsv1alpha1.CellPhaseDraining, + GuestUID: first, + }} + }) + f.deleteGuest("cells-0") + + // The pool recreates the index. The replacement must start clean. + f.reconcile() + f.reconcile() + + cells := f.getPool().Status.Cells + if len(cells) != 1 { + t.Fatalf("cells = %+v, want the index refilled", cells) + } + if cells[0].Phase == cellsv1alpha1.CellPhaseDraining { + t.Error("the replacement inherited its predecessor's teardown") + } + if cells[0].GuestUID == first { + t.Error("no new guest was created") + } + if len(f.guests()) != 1 { + t.Errorf("got %d guests, want exactly one surviving replacement", len(f.guests())) + } +} diff --git a/internal/controller/status.go b/internal/controller/status.go index d8449e7..aa787a8 100644 --- a/internal/controller/status.go +++ b/internal/controller/status.go @@ -100,6 +100,9 @@ type ConditionInput struct { Membership MembershipPlan Progressing bool + // Rollout is the template-drift decision, which drives the Updated condition. + Rollout RolloutDecision + // Autoscaling reports whether demand-driven scale-up is enabled, and Scale is // the last decision — the reason is where an operator looks to understand why // the pool did or did not grow. @@ -210,6 +213,11 @@ func ComputeConditions(in ConditionInput) []metav1.Condition { set(cellsv1alpha1.ConditionScalingActive, in.DemandKnown, in.Scale.Reason, in.Scale.Message) } + // Template drift. Present always, because "every cell matches what you asked + // for" is worth being able to assert, not only worth reporting when it breaks. + set(cellsv1alpha1.ConditionUpdated, in.Rollout.UpToDate, + in.Rollout.Reason, in.Rollout.Message) + // Aggregate. ready := in.Ready == in.Desired readyReason := cellsv1alpha1.ReasonAllCellsReady diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index db4b4da..1e2b010 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -407,3 +407,33 @@ func newUnreadySlice(node, driver, device string) *resourceapi.ResourceSlice { } func ptrTo[T any](v T) *T { return &v } + +// patchPoolStatus writes the pool's status directly, to set up a state the +// reconciler would otherwise take several passes to reach. +func (f *testFixture) patchPoolStatus(mutate func(*cellsv1alpha1.GPUCellPool)) { + f.t.Helper() + pool := f.getPool() + mutate(pool) + if err := outerClient.Status().Update(context.Background(), pool); err != nil { + f.t.Fatalf("update pool status: %v", err) + } +} + +// deleteGuest removes a cell's guest out from under the pool, clearing the drain +// finalizer first so the delete completes. +func (f *testFixture) deleteGuest(name string) { + f.t.Helper() + ctx := context.Background() + g := &unstructured.Unstructured{} + g.SetGroupVersionKind(provisioner.SwiftGuestGVK) + if err := outerClient.Get(ctx, types.NamespacedName{Namespace: f.ns, Name: name}, g); err != nil { + f.t.Fatalf("get guest %s: %v", name, err) + } + g.SetFinalizers(nil) + if err := outerClient.Update(ctx, g); err != nil { + f.t.Fatalf("clear finalizers on %s: %v", name, err) + } + if err := outerClient.Delete(ctx, g); err != nil { + f.t.Fatalf("delete guest %s: %v", name, err) + } +} diff --git a/internal/provisioner/clusterapi.go b/internal/provisioner/clusterapi.go index b54a6c4..e2ef13f 100644 --- a/internal/provisioner/clusterapi.go +++ b/internal/provisioner/clusterapi.go @@ -322,6 +322,7 @@ func (p *ClusterAPIProvisioner) observeMachine( created := ts st.CreatedAt = &created } + st.TemplateHash = machine.GetAnnotations()[cellsv1alpha1.AnnotationTemplateHash] st.Phase, _, _ = unstructured.NestedString(machine.Object, "status", "phase") switch st.Phase { @@ -355,6 +356,9 @@ func (p *ClusterAPIProvisioner) observeMachine( // The guest's own view supplies GPU, host and address; the Machine keeps // ownership of lifecycle and identity. guestState := observe(guest, req.NodeIPFrom) + // Deliberately NOT guestState.TemplateHash: under this provisioner the object + // this operator creates and versions is the Machine, and the backing guest is + // capi-kubeswift's to annotate however it likes. st.GPUDevices = guestState.GPUDevices st.HostNode = guestState.HostNode st.Address = guestState.Address diff --git a/internal/provisioner/provisioner.go b/internal/provisioner/provisioner.go index 617ba4b..49aadef 100644 --- a/internal/provisioner/provisioner.go +++ b/internal/provisioner/provisioner.go @@ -129,6 +129,12 @@ type OuterState struct { // UID is the outer object's UID — the cell's instance anchor, used to spot a // workload Node left behind by a previous incarnation. UID string + + // TemplateHash is the cell-template hash the object was CREATED from, read back + // from its annotation. Comparing it with the pool's current hash is the only way + // to know a cell is running an older shape: the hash was being written and never + // read, so template drift was invisible. + TemplateHash string } // DrainFinalizerClearer is implemented by provisioners that stamp the cell-drain diff --git a/internal/provisioner/swiftguest.go b/internal/provisioner/swiftguest.go index 4e04f8d..0d4bd0c 100644 --- a/internal/provisioner/swiftguest.go +++ b/internal/provisioner/swiftguest.go @@ -165,6 +165,7 @@ func observe(guest *unstructured.Unstructured, nodeIPFrom string) OuterState { st.CreatedAt = &created } + st.TemplateHash = guest.GetAnnotations()[cellsv1alpha1.AnnotationTemplateHash] st.Phase, _, _ = unstructured.NestedString(guest.Object, "status", "phase") st.HostNode, _, _ = unstructured.NestedString(guest.Object, "status", "gpu", "nodeName") if st.HostNode == "" {