fix: wait for volumes to detach before deleting a server

[pkieszcz]

Deleting a Hetzner server force-detaches whatever is attached to it. If a detach is already in flight when that happens, the filesystem is torn away mid-write — and the volume can come back in a state where the mount hangs in the kernel, uninterruptibly, on every retry:

mount -> ext4_fill_super -> ext4_init_orphan_info -> ext4_bread -> __wait_on_buffer   [D state]

Not on-disk corruption — the same filesystem mounts in seconds on a different node, journal replay completes, no e2fsck needed. But the node that first tried to mount it accumulates one D-state mount per kubelet retry and can never mount that volume again.

Delete currently issues the server delete without ever looking at server.Volumes.

What this looks like in practice

From the CSI driver logs of an incident that cost 73 minutes of a replicated store being down to one replica:

08:02:54  node tainted for disruption
08:02:58  unpublishing volume            (unmount from the pod)
08:03:04  detaching volume from server   (five volumes at once from this node)
          ... no detach completion is ever logged for the affected volume ...
08:03:24  provider deletes the server    <- 20s after the detach began
08:04:20  attaching volume               (to the replacement server)
08:04:35  failed to attach volume / failed to get volume
08:04:37  publishing volume              <- the D-state mount begins here

Sibling volumes on the same node did log "volume not attached to a server" at 08:03:20 — clean detaches. The affected one never did. Three others failed outright with "failed to detach volume" err="context canceled", and their retries hit "cannot perform operation because server is locked" — the lock being the deletion already in progress.

So the volume was still attached, with a detach outstanding, when the server was destroyed underneath it.

Why Karpenter core does not already prevent this

It very nearly does. pkg/controllers/node/termination runs awaitDrain, then awaitVolumeDetachment, then awaitInstanceTermination, and cloudProvider.Delete is only reached from the third. So the provider is normally called on a node whose volumes are already gone.

The gap is what "detached" means. Core waits for the Kubernetes VolumeAttachment object to disappear, which is the attach-detach controller's view. That is not the same fact as the cloud provider having finished.

I measured the ordering on a synthetic consolidation — a throwaway PVC with a dirty ext4, pod deleted, polling both sides once a second:

t=13.90  hcloud volume DETACHED
t=17.08  VolumeAttachment GONE (k8s believes detached)

On the happy path Hetzner finishes ~3s before Kubernetes removes the attachment, so by the time Delete is called there is nothing attached and this change does nothing at all. It is only when a detach stalls or fails — as above — that the two diverge and the provider gets called on a server that still has volumes.

That measurement is worth stating plainly because it sets the cost: inert in normal operation, active only on the failure path.

The change

Delete reads the server first and, if server.Volumes is non-empty, returns without deleting.

Core's contract makes that sufficient by itself. In awaitInstanceTermination a nil error means "still terminating" and core requeues after 5s; only NodeClaimNotFoundError ends the loop. So the provider declines until it is safe and core does the polling — no core change, no new RBAC, no extra controller.

Three decisions worth a reviewer's attention

It is bounded. An unbounded wait turns a stuck detach into a node that can never terminate, holding its volume, its capacity and its bill — worse than the unclean detach it set out to prevent. Past the grace we delete anyway, which is exactly the behaviour that exists today, so the worst case is the status quo rather than a wedged node.

The grace comes from nodeClaim.DeletionTimestamp, not an in-process timer, so an operator restart mid-termination does not silently reset the clock and start the wait over.

It is not in the instance provider. Both delete paths funnel through instance.Provider.Delete, and the other caller is the orphaned-server sweep. An orphan whose volume is stuck must stay reclaimable — blocking it strands a billing server indefinitely, which is the precise failure that sweep exists to prevent. The wait therefore lives at the CloudProvider layer, which is also the only layer holding the NodeClaim the grace is measured against.

Failure behaviour

Every uncertain path proceeds with the delete rather than waiting: a nil deletion timestamp (no clock to bound against), a server that cannot be read, a server already gone. A check that cannot answer must not be able to block termination — and an unreadable server must still surface NodeClaimNotFoundError, or core never learns termination finished.

On the five minutes

Core has already waited for the Kubernetes attachment by the time Delete is called, so the remaining provider-side detach should be seconds; in the incident above the driver was still retrying a minute later. Five minutes is generous for the first and bounded well short of the second. Left as a constant rather than another configuration knob until there is evidence one is wanted — happy to promote it to an operator option.

Tests

Written RED-first, and each guard mutation-tested: reverting any one makes exactly its test fail. The nil-timestamp check is the interesting one — removing it is a nil dereference rather than a wrong answer, so it is load-bearing for a different reason than the others.

Covered: volume still attached (waits), no volumes (deletes immediately), grace elapsed (deletes anyway), no deletion timestamp (deletes), missing server (still reports NodeClaimNotFound). The two pre-existing delete tests pass unchanged.

What this does not fix

It does not stop a detach from stalling, and it is not a substitute for finding out why five simultaneous detaches stalled with attaches to the replacement nodes racing them, or what cancelled three of them — context canceled on a ControllerUnpublish means the caller gave up, and that is in the CSI layer, not here.

What it does is stop this provider from turning a slow or failed detach into a destroyed filesystem.

[stubbi]

On the five minutes: keep it a constant. Promoting it to an operator option before anyone has asked for a different value is a knob without a constituency; the constant plus the comment explaining its derivation is the right resting state. (govulncheck failure is unrelated — fixed on main by #57, rebase to clear.)