From f14af9a989633bdd3129539e96910daafea9ef69 Mon Sep 17 00:00:00 2001 From: William Rizzo Date: Sun, 9 Aug 2026 11:32:41 +0000 Subject: [PATCH] fix: stop reaping a Node the live replacement kubelet already adopted (#14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cell whose Node carried a previous incarnation's identity label was reaped as stale on the strength of that label alone. But a replacement's kubelet registers under the reused node name and ADOPTS the existing Node object, keeping labels it did not set — so a live cell looks exactly like a leftover. Deleting it is unrecoverable: a kubelet whose Node is removed under it never re-registers, it just logs "Error updating node status, will retry" while the cell waits in Joining forever. Observed on hardware; only restarting the guest's kubelet recovered it. Liveness is now the discriminator. NodeState carries KubeletLive, read from the kubelet's heartbeat Lease, falling back to the Ready condition's heartbeat and finally to "live" when neither can be read — an unknown must never authorise a delete. A Node with a foreign label and a live kubelet is adopted and re-labelled instead of reaped. That alone was not enough. A retired cell's row is dropped at the moment its kubelet has only just died, so its Node still looks live and is correctly left alone — and nothing remembers the cell afterwards, so a one-shot cleanup there reaps nothing. Measured: the Node sat NotReady for ten minutes with no further attempt, waiting for the next cell at that index to adopt it. Orphan cleanup is therefore an idempotent sweep keyed on the pool LABEL, which converges rather than getting one chance, and never touches a Node the pool does not own. The workload-cluster observer role gains coordination.k8s.io/leases get. Without it the operator degrades to the Ready-condition heartbeat (up to 5 minutes stale) rather than failing. Signed-off-by: William Rizzo --- config/rbac/workload-cluster-observer.yaml | 15 +++ docs/api-reference.md | 6 +- docs/design/gpucellpool-reconciliation.md | 11 ++ docs/runbook.md | 34 +++++- internal/controller/controller.go | 87 ++++++++++++--- internal/controller/fsm.go | 11 +- internal/controller/reconcile_test.go | 107 +++++++++++++++++-- internal/controller/suite_test.go | 23 ++++ internal/workload/node.go | 118 +++++++++++++++++++-- internal/workload/workload_test.go | 114 +++++++++++++++++++- 10 files changed, 486 insertions(+), 40 deletions(-) diff --git a/config/rbac/workload-cluster-observer.yaml b/config/rbac/workload-cluster-observer.yaml index be2ed83..5d7d9e2 100644 --- a/config/rbac/workload-cluster-observer.yaml +++ b/config/rbac/workload-cluster-observer.yaml @@ -29,6 +29,21 @@ rules: resources: [nodes] verbs: [get, list, watch, patch, update, delete] + # Kubelet heartbeat leases — what makes the node DELETE above safe. + # + # A Node carrying a previous incarnation's identity label is not necessarily a + # leftover: the replacement's own kubelet adopts the object under the reused name + # and keeps the label it finds. The Lease is the only thing that separates the two, + # and deleting the wrong one is unrecoverable — a kubelet whose Node is removed + # under it never re-registers. + # + # Without this grant the operator falls back to the Node's Ready heartbeat, which + # is up to 5 minutes stale by design, and when that is missing too it declines to + # delete at all. Nothing breaks; phantom Nodes just linger longer. + - apiGroups: [coordination.k8s.io] + resources: [leases] + verbs: [get] + # HAMi DevicePlugin-mode accounting: allocations live on pod annotations. - apiGroups: [""] resources: [pods] diff --git a/docs/api-reference.md b/docs/api-reference.md index 0fa5ca3..a3c5664 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -73,8 +73,10 @@ metadata.labels = cells.kubeswift.io/{pool,cell,cell-index} metadata.annotations = cells.kubeswift.io/template-hash= ``` -A template change bumps the hash but does **not** roll existing cells: v1alpha1 -has no rolling update for `guestTemplate` (see `docs/limitations.md`). +A template change bumps the hash and reports `Updated=False/TemplateChanged`. It +rolls existing cells only under `spec.updatePolicy.type: RollingUpdate`; the +default `Manual` leaves them alone, because a template edit is not consent to +destroy running work (see `docs/updates.md`). Under `provisioner: ClusterAPI` the allowed `guestTemplate` fields shrink to `imageRef`, `guestClassRef`, `interfaces` — everything else a `KubeSwiftMachine` diff --git a/docs/design/gpucellpool-reconciliation.md b/docs/design/gpucellpool-reconciliation.md index 32514b8..1261090 100644 --- a/docs/design/gpucellpool-reconciliation.md +++ b/docs/design/gpucellpool-reconciliation.md @@ -291,6 +291,17 @@ Every verb has a named consumer: # unconditionally need it: reaping a stale Node left by a replaced cell # (§3, §8), and removing a cell's Node when the cell itself is deleted. # Without it every teardown fails Forbidden and stale Nodes accumulate. +- apiGroups: [coordination.k8s.io] + resources: [leases] + verbs: [get] + # What makes the node DELETE above SAFE. A Node carrying a previous + # incarnation's identity label may simply be the same object after the + # replacement's kubelet adopted it — a kubelet keeps labels it did not set — + # and the Lease is the only signal that separates "leftover" from "live". + # Deleting a live one is unrecoverable: the kubelet does not re-register. + # Optional in the sense that its absence degrades rather than breaks: the + # operator falls back to the Ready condition's heartbeat (up to 5 min stale), + # and if that is missing too it declines to delete. - apiGroups: [""] resources: [pods] verbs: [get, list, watch] # HAMi DevicePlugin accounting diff --git a/docs/runbook.md b/docs/runbook.md index ef12fbe..69be18c 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -230,12 +230,38 @@ a cell's own Node when the cell is deleted, are both always-on paths `nodes: [get, list, watch, patch, update, delete]`) and mint a fresh token if the credential Secret was built from an older ClusterRole. +### A cell sits in `Joining` forever after being replaced + +`waiting for the workload Node to register`, indefinitely, while the VM is +healthy and reachable — and the workload cluster shows **no Node** for the cell +even though its CSRs were approved. The kubelet logs +`Error updating node status, will retry` and +`Failed to get node when trying to set owner ref to the node lease`. + +Its Node object was deleted from under a running kubelet, which then does not +re-register. Recover by restarting the kubelet inside the cell: + +```bash +# k0s +ssh sudo systemctl restart k0sworker +# kubeadm +ssh sudo systemctl restart kubelet +``` + +This was a bug (#14, fixed): a Node carrying a previous incarnation's identity +label was reaped as stale even when the replacement's own kubelet had already +adopted it. The reap now requires that nothing is heartbeating for the Node — +so grant `coordination.k8s.io/leases: [get]` in the workload cluster +(reapply `config/rbac/workload-cluster-observer.yaml`) or the operator has to +fall back to the Ready condition's heartbeat, which lags by up to 5 minutes. + ### Replacing a cell after a cell-image change -There is **no rolling update**. Changing `spec.cell.guestTemplate` (a new -`imageRef`, a driver bump) bumps the per-cell template-hash annotation but -does not touch existing cells (`docs/limitations.md`). To roll a driver -change out cell by cell, for each cell in turn: +Automatic rolling update exists — set `spec.updatePolicy.type: RollingUpdate` +(`docs/updates.md`). The default is `Manual`: changing `spec.cell.guestTemplate` +(a new `imageRef`, a driver bump) bumps the per-cell template-hash annotation, +reports `Updated=False/TemplateChanged` with the stale cells named, and touches +nothing. To roll a change out by hand, for each cell in turn: ```bash # 1. In the WORKLOAD cluster: drain the cell's inner workloads yourself. diff --git a/internal/controller/controller.go b/internal/controller/controller.go index 89c0fbd..34edc70 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -370,16 +370,24 @@ func (r *GPUCellPoolReconciler) discoverCells( } if reachable { - node, nErr := workload.GetNodeState(ctx, inner, name) + node, nErr := workload.GetNodeState(ctx, inner, name, obs.Now) if nErr != nil { return nil, fmt.Errorf("cell %s: %w", name, nErr) } obs.Node = node - obs.StaleNode = node.Exists && cellid.IsStaleNode(node.Labels, outer.UID) + // A foreign identity label alone does NOT make a Node stale. The + // replacement's own kubelet adopts the existing object and keeps the + // label it finds there, so a live kubelet means "adopt and re-label", + // and only a Node nobody is heartbeating for is a phantom to reap. + // Getting this wrong deletes the live cell's Node, and a kubelet whose + // Node is deleted under it never re-registers (#14). + obs.StaleNode = node.Exists && !node.KubeletLive && + cellid.IsStaleNode(node.Labels, outer.UID) if node.Exists && !obs.StaleNode { - // The kubelet may not have applied the identity labels itself; - // patching them here is the reliable path. + // The kubelet may not have applied the identity labels itself, and + // an adopted Node still carries the previous incarnation's; patching + // them here is the reliable path, and is what claims an adoption. if _, mErr := workload.EnsureNodeMetadata(ctx, inner, name, cellid.NodeLabels(pool.Name, idx, outer.UID, nodeLabels(pool)), nodeAnnotations(pool), nodeTaints(pool)); mErr != nil { @@ -423,17 +431,35 @@ func (r *GPUCellPoolReconciler) discoverCells( live[c.Name] = true } for name, prev := range previous { - if live[name] || prev.Phase != cellsv1alpha1.CellPhaseFailed { + if live[name] { continue } - if prev.Index >= pool.Spec.Replicas { - continue // the pool no longer wants this slot + // This row has no guest. Either it becomes a tombstone, or it is dropped — + // and dropping it is the last moment anything remembers the cell existed. + keepTombstone := prev.Phase == cellsv1alpha1.CellPhaseFailed && prev.Index < pool.Spec.Replicas + if keepTombstone { + prev.GuestUID = "" + prev.NodeName = "" + prev.Devices = nil + prev.CapacityDevices = 0 + out = append(out, prev) + continue + } + // Otherwise the row is dropped, and its workload Node is left behind. That is + // what sets up the collision in #14 — but it is NOT cleaned up here: at the + // moment a row is dropped the cell's kubelet has only just died, so its lease + // still looks fresh and the Node is (correctly) not reapable yet. A one-shot + // attempt here reaps nothing and then nothing remembers the cell. The + // idempotent sweep in reapOrphanNodes owns this. + } + if reachable { + keep := make(map[string]bool, len(out)) + for _, c := range out { + keep[c.Name] = true + } + if err := r.reapOrphanNodes(ctx, inner, pool, keep); err != nil { + return nil, err } - prev.GuestUID = "" - prev.NodeName = "" - prev.Devices = nil - prev.CapacityDevices = 0 - out = append(out, prev) } sort.Slice(out, func(i, j int) bool { return out[i].Index < out[j].Index }) return out, nil @@ -441,6 +467,43 @@ func (r *GPUCellPoolReconciler) discoverCells( // cellStatus folds a decision into the cell's status row, carrying the failure // count and transition time forward. +// reapOrphanNodes deletes workload Nodes that carry this pool's label but no +// longer correspond to any of its cells, once nothing is heartbeating for them. +// +// It exists because a retired cell is forgotten: its status row is dropped, and at +// that instant its kubelet has only just stopped, so the Node is not yet safe to +// delete. Anything that tried to clean up at drop time would find a Node that still +// looks live, skip it, and never look again — leaving the Node for the next cell at +// that index to adopt, which is the collision in #14. So the sweep is keyed on the +// pool LABEL and runs every reconcile: it converges instead of getting one chance. +// +// keep is the set of cell names the pool currently accounts for, including cells +// mid-teardown — their Nodes belong to the drain path, not here. +func (r *GPUCellPoolReconciler) reapOrphanNodes( + ctx context.Context, inner kubernetes.Interface, + pool *cellsv1alpha1.GPUCellPool, keep map[string]bool, +) error { + nodes, err := workload.ListPoolNodes(ctx, inner, pool.Name, r.now()) + if err != nil { + // A pool whose nodes cannot be listed is not a pool whose nodes should be + // deleted; the reachability condition already reports the transport. + logf.FromContext(ctx).V(1).Info("orphan node sweep skipped", "error", err.Error()) + return nil + } + for name, node := range nodes { + if keep[name] || node.KubeletLive { + continue + } + if err := workload.DeleteNode(ctx, inner, name); err != nil { + return fmt.Errorf("removing orphan node %s: %w", name, err) + } + r.event(pool, corev1.EventTypeNormal, cellsv1alpha1.ReasonNodeNameCollision, + fmt.Sprintf("removed workload Node %s: it carries this pool's label, "+ + "belongs to no current cell, and no kubelet is heartbeating for it", name)) + } + return nil +} + func (r *GPUCellPoolReconciler) cellStatus( prev cellsv1alpha1.CellStatus, pool, namespace, name string, idx int32, outer provisioner.OuterState, obs Observation, dec Decision, diff --git a/internal/controller/fsm.go b/internal/controller/fsm.go index 83b96e9..280488a 100644 --- a/internal/controller/fsm.go +++ b/internal/controller/fsm.go @@ -117,13 +117,16 @@ func AdvanceCell(obs Observation) Decision { return expireOr(obs, phase, obs.BootstrapTimeout, cellsv1alpha1.ReasonCellProvisionTimeout, msg) case obs.StaleNode: - // A Node with this cell's name but a previous incarnation's UID. Left in - // place it would be reported as this cell's Node — a phantom Ready cell, - // or capacity advertised for a GPU that no longer exists. + // A Node with this cell's name, a previous incarnation's UID, and NO kubelet + // heartbeating for it. Left in place it would be reported as this cell's + // Node — a phantom Ready cell, or capacity advertised for a GPU that no + // longer exists. A Node whose kubelet IS live is never stale, however old + // its identity label: that is the replacement adopting the object, and it + // gets re-labelled instead (see the observation site). return Decision{ Phase: cellsv1alpha1.CellPhaseJoining, Reason: cellsv1alpha1.ReasonNodeNameCollision, - Message: "a workload Node from a previous incarnation of this cell is being removed before the replacement joins", + Message: "a workload Node left by a previous incarnation of this cell (no kubelet heartbeat) is being removed before the replacement joins", ReapStaleNode: true, } diff --git a/internal/controller/reconcile_test.go b/internal/controller/reconcile_test.go index b6122fe..1d66667 100644 --- a/internal/controller/reconcile_test.go +++ b/internal/controller/reconcile_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -263,24 +264,118 @@ func TestReconcilePatchesIdentityLabelsOntoTheNode(t *testing.T) { } } -func TestReconcileReapsAStaleNodeBeforeTheReplacementJoins(t *testing.T) { +func TestReconcileReapsAPhantomNodeWhoseKubeletIsGone(t *testing.T) { f := newFixture(t, nil) f.reconcile() f.stampGuestRunning("cells-0", "10.77.0.5", "0000:01:00.0", "boba") f.reconcile() - // A Node with this cell's NAME but a previous incarnation's UID. Adopting it - // would report a phantom Ready cell, with HAMi advertising capacity for a GPU - // that no longer exists. + // A Node with this cell's NAME, a previous incarnation's UID, and no kubelet + // behind it. Adopting it would report a phantom Ready cell, with HAMi + // advertising capacity for a GPU that no longer exists. f.joinNode("cells-0", "uid-from-a-previous-life", true) + f.killKubelet("cells-0") f.reconcile() if _, err := innerClientset.CoreV1().Nodes().Get(context.Background(), "cells-0", metav1.GetOptions{}); !apierrors.IsNotFound(err) { - t.Fatalf("stale node survived: %v", err) + t.Fatalf("phantom node survived: %v", err) } pool := f.getPool() if pool.Status.Cells[0].Phase == cellsv1alpha1.CellPhaseReady { - t.Error("cell went Ready off a stale node") + t.Error("cell went Ready off a phantom node") + } +} + +// TestReconcileAdoptsANodeItsOwnKubeletTookOver is the other half of #14, and the +// case that cost a cell: a replacement's kubelet registers under the reused node +// name, adopts the EXISTING Node object, and keeps the identity label it finds +// there. By label alone that is indistinguishable from a phantom — but deleting it +// is unrecoverable, because a kubelet whose Node is removed under it does not +// re-register. It logs "Error updating node status, will retry" and the cell waits +// forever. +func TestReconcileAdoptsANodeItsOwnKubeletTookOver(t *testing.T) { + f := newFixture(t, nil) + f.reconcile() + f.stampGuestRunning("cells-0", "10.77.0.5", "0000:01:00.0", "boba") + f.reconcile() + + // Same foreign label as above — the difference is only that a kubelet is + // heartbeating for it (joinNode stamps a fresh heartbeat). + f.joinNode("cells-0", "uid-from-a-previous-life", true) + f.reconcile() + + node, err := innerClientset.CoreV1().Nodes().Get(context.Background(), "cells-0", metav1.GetOptions{}) + if err != nil { + t.Fatalf("the live cell's Node was deleted under its kubelet: %v", err) + } + if got := node.Labels[cellsv1alpha1.LabelInstance]; got != f.guestUID("cells-0") { + t.Errorf("instance label = %q, want the current guest UID %q — an adopted Node must be re-labelled, "+ + "or it looks stale again on the next pass", got, f.guestUID("cells-0")) + } + if p := f.getPool(); p.Status.Cells[0].Phase != cellsv1alpha1.CellPhaseReady { + t.Errorf("cell phase = %s, want Ready: an adopted Node is this cell's Node", p.Status.Cells[0].Phase) + } +} + +// TestReconcileReapsTheNodeOfARetiredCell covers what SET UP the collision: a +// failed cell's row is dropped when the pool no longer wants its index, and +// nothing else remembers the cell afterwards. Leaving its Node behind is what a +// later replacement adopts. +func TestReconcileReapsTheNodeOfARetiredCell(t *testing.T) { + f := newFixture(t, nil) + f.reconcile() + f.stampGuestRunning("cells-0", "10.77.0.5", "0000:01:00.0", "boba") + f.joinNode("cells-0", f.guestUID("cells-0"), true) + f.reconcile() + if p := f.getPool(); p.Status.Cells[0].Phase != cellsv1alpha1.CellPhaseReady { + t.Fatalf("setup: phase = %s", p.Status.Cells[0].Phase) + } + + // The guest goes away and the pool stops wanting the slot, which is the moment + // the status row is dropped. The kubelet has only just died, so the Node still + // LOOKS live — and that is the whole difficulty: a cleanup that only gets this + // one chance finds a live-looking Node, correctly declines to delete it, and + // never looks again. Measured on hardware before this was a sweep: the Node sat + // there NotReady for four minutes and nothing ever came back for it. + f.deleteGuest("cells-0") + f.patchPool(func(p *cellsv1alpha1.GPUCellPool) { p.Spec.Replicas = 0 }) + f.reconcile() + + if _, err := innerClientset.CoreV1().Nodes().Get(context.Background(), "cells-0", metav1.GetOptions{}); err != nil { + t.Fatalf("a Node whose kubelet had not yet gone cold was deleted: %v", err) + } + + // Now the heartbeat goes cold. A later pass must pick it up, with no status row + // left to remind it the cell ever existed. + f.killKubelet("cells-0") + f.reconcile() + + if _, err := innerClientset.CoreV1().Nodes().Get(context.Background(), "cells-0", metav1.GetOptions{}); !apierrors.IsNotFound(err) { + t.Fatalf("the retired cell's Node was left behind: %v", err) + } +} + +// TestReconcileLeavesForeignNodesAlone keeps the orphan sweep from becoming a +// licence to delete nodes: it is keyed on this pool's label, and a Node without it +// is none of the pool's business however dead its kubelet looks. +func TestReconcileLeavesForeignNodesAlone(t *testing.T) { + f := newFixture(t, nil) + f.reconcile() + f.stampGuestRunning("cells-0", "10.77.0.5", "0000:01:00.0", "boba") + f.joinNode("cells-0", f.guestUID("cells-0"), true) + + // A node belonging to somebody else, cold and unlabelled. + other := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "someone-elses-worker"}} + if _, err := innerClientset.CoreV1().Nodes().Create(context.Background(), other, metav1.CreateOptions{}); err != nil { + t.Fatalf("create foreign node: %v", err) + } + t.Cleanup(func() { + _ = innerClientset.CoreV1().Nodes().Delete(context.Background(), "someone-elses-worker", metav1.DeleteOptions{}) + }) + f.reconcile() + + if _, err := innerClientset.CoreV1().Nodes().Get(context.Background(), "someone-elses-worker", metav1.GetOptions{}); err != nil { + t.Fatalf("the pool deleted a Node it does not own: %v", err) } } diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 1e2b010..9dd5a81 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -317,6 +317,29 @@ func (f *testFixture) joinNode(name string, instance string, withHAMi bool) { } } +// killKubelet makes a Node look like one nobody is heartbeating for: the object +// stays, Ready stays True, only the heartbeat goes cold. That is precisely the +// shape of a Node left behind by a deleted VM, and the only thing that +// distinguishes it from the same object after a live replacement adopted it. +// +// envtest runs no kubelet, so nothing renews a Lease here and liveness resolves +// from the Ready condition's heartbeat. +func (f *testFixture) killKubelet(name string) { + f.t.Helper() + node, err := innerClientset.CoreV1().Nodes().Get(context.Background(), name, metav1.GetOptions{}) + if err != nil { + f.t.Fatalf("get node %s: %v", name, err) + } + for i := range node.Status.Conditions { + if node.Status.Conditions[i].Type == corev1.NodeReady { + node.Status.Conditions[i].LastHeartbeatTime = metav1.NewTime(time.Now().Add(-30 * time.Minute)) + } + } + if _, err := innerClientset.CoreV1().Nodes().UpdateStatus(context.Background(), node, metav1.UpdateOptions{}); err != nil { + f.t.Fatalf("stop node heartbeat: %v", err) + } +} + func (f *testFixture) node(name string) *corev1.Node { f.t.Helper() n, err := innerClientset.CoreV1().Nodes().Get(context.Background(), name, metav1.GetOptions{}) diff --git a/internal/workload/node.go b/internal/workload/node.go index dab3265..3ee6aad 100644 --- a/internal/workload/node.go +++ b/internal/workload/node.go @@ -4,12 +4,15 @@ import ( "context" "encoding/json" "fmt" + "time" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" + + cellsv1alpha1 "github.com/kubeswift-io/gpucellpool/api/v1alpha1" ) // NodeState is what the reconciler needs to know about a cell's workload Node. @@ -25,11 +28,46 @@ type NodeState struct { // in-guest preflight result. Labels map[string]string Annotations map[string]string + + // KubeletLive is true when a kubelet is heartbeating for this Node right now. + // + // It exists to separate two things a Node's labels cannot tell apart: a Node + // left behind by a dead incarnation of a cell, and the SAME Node object after + // the replacement's kubelet adopted it — a kubelet keeps labels it did not set, + // so both carry the old identity label. Deleting the second one is + // unrecoverable from this side: the kubelet does not re-register, it just logs + // "Error updating node status, will retry" forever and the cell never joins. + // + // Unknown reads as LIVE. Failing to delete a phantom Node costs a stale + // capacity reading that the next reconcile corrects; deleting a live one costs + // the cell. + KubeletLive bool + + // HeartbeatSource names where KubeletLive came from — "Lease", "NodeStatus", or + // "" when neither could be read. Recorded because the two have very different + // resolutions and an operator debugging a reap needs to know which was used. + HeartbeatSource string } +// Heartbeat grace periods, per source. +// +// The kubelet renews its Lease every ~10s (lease duration 40s), so 90s is already +// generous. Node STATUS is only pushed every nodeStatusReportFrequency — 5 minutes +// by default once leases are enabled — so judging liveness by it needs a much wider +// window, and is a fallback rather than the signal we want. +const ( + leaseGrace = 90 * time.Second + nodeStatusGrace = 6 * time.Minute +) + +// nodeLeaseNamespace is where kubelets renew their heartbeat leases. +const nodeLeaseNamespace = "kube-node-lease" + // GetNodeState reads a cell's Node. A missing Node is not an error: the cell is // simply still joining. -func GetNodeState(ctx context.Context, cs kubernetes.Interface, name string) (NodeState, error) { +// +// now is passed in rather than read from the clock so liveness is testable. +func GetNodeState(ctx context.Context, cs kubernetes.Interface, name string, now time.Time) (NodeState, error) { node, err := cs.CoreV1().Nodes().Get(ctx, name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { return NodeState{}, nil @@ -37,15 +75,76 @@ func GetNodeState(ctx context.Context, cs kubernetes.Interface, name string) (No if err != nil { return NodeState{}, fmt.Errorf("reading node %s: %w", name, err) } + live, src := kubeletLive(ctx, cs, node, now) return NodeState{ - Exists: true, - Ready: IsReady(node), - Unschedulable: node.Spec.Unschedulable, - Labels: node.Labels, - Annotations: node.Annotations, + Exists: true, + Ready: IsReady(node), + Unschedulable: node.Spec.Unschedulable, + Labels: node.Labels, + Annotations: node.Annotations, + KubeletLive: live, + HeartbeatSource: src, }, nil } +// ListPoolNodes returns the state of every Node in the workload cluster carrying +// this pool's label, keyed by node name. +// +// The pool label — not the operator's own status — is the authority on which Nodes +// belong to a pool. Status can be lost, and a row is dropped the moment a cell is +// retired, so anything that cleans up after cells has to be able to find them +// without it. +func ListPoolNodes(ctx context.Context, cs kubernetes.Interface, pool string, now time.Time) (map[string]NodeState, error) { + list, err := cs.CoreV1().Nodes().List(ctx, metav1.ListOptions{ + LabelSelector: cellsv1alpha1.LabelPool + "=" + pool, + }) + if err != nil { + return nil, fmt.Errorf("listing nodes of pool %s: %w", pool, err) + } + out := make(map[string]NodeState, len(list.Items)) + for i := range list.Items { + node := &list.Items[i] + live, src := kubeletLive(ctx, cs, node, now) + out[node.Name] = NodeState{ + Exists: true, + Ready: IsReady(node), + Unschedulable: node.Spec.Unschedulable, + Labels: node.Labels, + Annotations: node.Annotations, + KubeletLive: live, + HeartbeatSource: src, + } + } + return out, nil +} + +// kubeletLive reports whether a kubelet is currently heartbeating for a Node. +// +// The Lease is the real signal. When it cannot be read — RBAC, an old cluster, a +// transient error — this falls back to the Ready condition's heartbeat, and if +// that is missing too it reports live: see NodeState.KubeletLive for why unknown +// must not authorise a delete. +func kubeletLive(ctx context.Context, cs kubernetes.Interface, node *corev1.Node, now time.Time) (bool, string) { + lease, err := cs.CoordinationV1().Leases(nodeLeaseNamespace).Get(ctx, node.Name, metav1.GetOptions{}) + switch { + case err == nil && lease.Spec.RenewTime != nil: + return now.Sub(lease.Spec.RenewTime.Time) < leaseGrace, "Lease" + case err == nil: + // A Lease with no renewTime has never been held. + return false, "Lease" + case apierrors.IsNotFound(err): + // No Lease at all. On a lease-enabled cluster a live kubelet always has + // one, so this is evidence of absence — but only once the node status + // agrees, which the fallback below checks. + } + for _, c := range node.Status.Conditions { + if c.Type == corev1.NodeReady && !c.LastHeartbeatTime.IsZero() { + return now.Sub(c.LastHeartbeatTime.Time) < nodeStatusGrace, "NodeStatus" + } + } + return true, "" +} + // IsReady reports the Node's Ready condition. An absent condition is not ready — // never assume readiness from silence. func IsReady(node *corev1.Node) bool { @@ -136,8 +235,11 @@ func Cordon(ctx context.Context, cs kubernetes.Interface, name string) error { } // DeleteNode removes a cell's Node object. Called for a cell being torn down, and -// for a stale Node left by a previous incarnation — always BEFORE the replacement -// joins, never after. +// for a phantom Node left by a previous incarnation. +// +// "Before the replacement joins" cannot be assumed — the replacement's kubelet may +// already have adopted the object — so the caller must establish that no kubelet is +// heartbeating for it first (NodeState.KubeletLive). func DeleteNode(ctx context.Context, cs kubernetes.Interface, name string) error { err := cs.CoreV1().Nodes().Delete(ctx, name, metav1.DeleteOptions{}) if err != nil && !apierrors.IsNotFound(err) { diff --git a/internal/workload/workload_test.go b/internal/workload/workload_test.go index 03d055f..700d8ec 100644 --- a/internal/workload/workload_test.go +++ b/internal/workload/workload_test.go @@ -5,14 +5,18 @@ import ( "errors" "net" "testing" + "time" + coordinationv1 "k8s.io/api/coordination/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/rest" + k8stesting "k8s.io/client-go/testing" cellsv1alpha1 "github.com/kubeswift-io/gpucellpool/api/v1alpha1" ) @@ -115,6 +119,19 @@ func TestClassifyErrorDistinguishesTheThreeTickets(t *testing.T) { } } +// testNow is a fixed clock: liveness is a time comparison, and a test that read +// the wall clock would be the kind of test that passes at 3pm and fails at 3am. +var testNow = time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC) + +// nodeLease builds a kubelet heartbeat lease renewed at now-age. +func nodeLease(name string, age time.Duration) *coordinationv1.Lease { + renew := metav1.NewMicroTime(testNow.Add(-age)) + return &coordinationv1.Lease{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: nodeLeaseNamespace}, + Spec: coordinationv1.LeaseSpec{RenewTime: &renew}, + } +} + func readyNode(name string, labels map[string]string) *corev1.Node { return &corev1.Node{ ObjectMeta: metav1.ObjectMeta{Name: name, Labels: labels}, @@ -128,14 +145,14 @@ func TestGetNodeState(t *testing.T) { ctx := context.Background() t.Run("missing node is not an error", func(t *testing.T) { - st, err := GetNodeState(ctx, fake.NewSimpleClientset(), "cell-0") + st, err := GetNodeState(ctx, fake.NewSimpleClientset(), "cell-0", testNow) if err != nil || st.Exists { t.Errorf("got %+v/%v — a cell that has not joined yet is not a failure", st, err) } }) t.Run("ready", func(t *testing.T) { - st, err := GetNodeState(ctx, fake.NewSimpleClientset(readyNode("cell-0", map[string]string{"gpu": "on"})), "cell-0") + st, err := GetNodeState(ctx, fake.NewSimpleClientset(readyNode("cell-0", map[string]string{"gpu": "on"})), "cell-0", testNow) if err != nil || !st.Exists || !st.Ready { t.Errorf("got %+v/%v", st, err) } @@ -143,7 +160,7 @@ func TestGetNodeState(t *testing.T) { t.Run("no Ready condition is not ready", func(t *testing.T) { n := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "cell-0"}} - st, _ := GetNodeState(ctx, fake.NewSimpleClientset(n), "cell-0") + st, _ := GetNodeState(ctx, fake.NewSimpleClientset(n), "cell-0", testNow) if st.Ready { t.Error("readiness assumed from silence") } @@ -152,13 +169,102 @@ func TestGetNodeState(t *testing.T) { t.Run("cordoned", func(t *testing.T) { n := readyNode("cell-0", nil) n.Spec.Unschedulable = true - st, _ := GetNodeState(ctx, fake.NewSimpleClientset(n), "cell-0") + st, _ := GetNodeState(ctx, fake.NewSimpleClientset(n), "cell-0", testNow) if !st.Unschedulable { t.Error("cordon not reported") } }) } +// TestKubeletLive pins the signal that decides whether a Node may be deleted. +// Getting it wrong in the "live" direction deletes a working cell's Node and the +// kubelet never re-registers (#14), so every unknown case must report live. +func TestKubeletLive(t *testing.T) { + ctx := context.Background() + node := readyNode("cell-0", nil) + + cases := []struct { + name string + objects []runtime.Object + heartbeat time.Duration // Ready-condition heartbeat age; 0 means absent + wantLive bool + wantSource string + }{ + { + name: "fresh lease is live", + objects: []runtime.Object{nodeLease("cell-0", 5*time.Second)}, + wantLive: true, + wantSource: "Lease", + }, + { + name: "expired lease is not live", + objects: []runtime.Object{nodeLease("cell-0", 10*time.Minute)}, + wantLive: false, + wantSource: "Lease", + }, + { + name: "lease that was never renewed is not live", + objects: []runtime.Object{&coordinationv1.Lease{ObjectMeta: metav1.ObjectMeta{Name: "cell-0", Namespace: nodeLeaseNamespace}}}, + wantLive: false, + wantSource: "Lease", + }, + { + // No lease and no status heartbeat: nothing is known, and an unknown + // must never authorise a delete. + name: "nothing readable reports live", + wantLive: true, + wantSource: "", + }, + { + // A recent status heartbeat is the fallback signal. Its resolution is + // coarse (5 min by default), hence the wide grace. + name: "recent node status is live", + heartbeat: 2 * time.Minute, + wantLive: true, + wantSource: "NodeStatus", + }, + { + name: "stale node status is not live", + heartbeat: 30 * time.Minute, + wantLive: false, + wantSource: "NodeStatus", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + n := node.DeepCopy() + if c.heartbeat != 0 { + n.Status.Conditions[0].LastHeartbeatTime = metav1.NewTime(testNow.Add(-c.heartbeat)) + } + cs := fake.NewSimpleClientset(append([]runtime.Object{n}, c.objects...)...) + st, err := GetNodeState(ctx, cs, "cell-0", testNow) + if err != nil { + t.Fatalf("GetNodeState: %v", err) + } + if st.KubeletLive != c.wantLive || st.HeartbeatSource != c.wantSource { + t.Errorf("KubeletLive=%v source=%q, want %v/%q", + st.KubeletLive, st.HeartbeatSource, c.wantLive, c.wantSource) + } + }) + } + + t.Run("an unreadable lease falls back rather than reporting dead", func(t *testing.T) { + cs := fake.NewSimpleClientset(node.DeepCopy()) + cs.PrependReactor("get", "leases", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewForbidden( + schema.GroupResource{Group: "coordination.k8s.io", Resource: "leases"}, "cell-0", errors.New("no rbac")) + }) + st, err := GetNodeState(ctx, cs, "cell-0", testNow) + if err != nil { + t.Fatalf("a lease we may not read must not fail the observation: %v", err) + } + if !st.KubeletLive { + t.Error("a Node whose liveness cannot be established was reported dead — that authorises deleting it") + } + }) +} + func TestEnsureNodeMetadata(t *testing.T) { ctx := context.Background()