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 == "" {