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()