fix: stop reaping a Node the live replacement kubelet already adopted (#14)

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]

docs/api-reference.md

@@ -73,8 +73,10 @@ metadata.labels           = cells.kubeswift.io/{pool,cell,cell-index}
 metadata.annotations       = cells.kubeswift.io/template-hash=<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`

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

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 <cell> sudo systemctl restart k0sworker
+# kubeadm
+ssh <cell> 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.

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,24 +431,79 @@ 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
 }
 
 // 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,

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,
 		}
 

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

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{})