fix(kubernetes): force the kubevirt-csi temp NFS unmount after a timeout - #4584

[MAJOR] The umount -f retry is unreachable in the dead-server scenario this PR targets

This is not a regression: relative to main the change is strictly better, and the acute symptom of #4561 (NodePublishVolume hanging forever on the temp unmount) is genuinely fixed by the outer select. The problem is that the forced cleanup the PR advertises does not run in the case that motivates it.

unmountWithin calls UnmountWithForce(dir, budget/2). In mount-utils v0.36.0 that runs tryUnmount, which does exec.CommandContext(ctx, "umount", target).CombinedOutput() and then if ctx.Err() != nil { return context.DeadlineExceeded }. The forceUmount (umount -f) branch fires only when tryUnmount returns exactly DeadlineExceeded.

But CombinedOutput with the default WaitDelay == 0 reads the pipe to EOF, not to the ctx deadline. SIGKILL at the deadline kills the parent umount, but util-linux forks the external /sbin/umount.nfs helper, which, when stuck in umount2() on a vanished server, is orphaned and keeps the stdout/stderr pipe open, so CombinedOutput does not return until the umount actually finishes.

Reproduced deterministically (go 1.25): a CommandContext with a 1s ctx whose process forks a child that holds the pipe for 10s returns from CombinedOutput at 10.1s with ctx.Err() == DeadlineExceeded, i.e. only after the child closes the pipe. Confirmed on the image too (almalinux:9 + nfs-utils): /sbin/umount.nfs -> mount.nfs exists and umount calls the helper by default (the -i/--internal-only flag exists to disable exactly that).

Net effect when the server is gone: the graceful umount.nfs never unblocks, tryUnmount never returns DeadlineExceeded, umount -f never runs, and the outer 30s select is what returns, leaking the goroutine, the umount process and the mount until the container restarts. So the fix delivers the bounded return but not the forced cleanup that the title, the node.go:150-160 comments and Closes #4561 all promise. No test exercises a real *mount.Mounter against a stuck umount (TestUnmountTempMountGivesUpOnAStalledForceRetry drives the outer select with a fake), so the gap is uncovered.

Fix direction: run umount -f on a wall-clock deadline that does not depend on the first umount process finishing (a direct exec with WaitDelay set), rather than relying on UnmountWithForce, whose DeadlineExceeded path cannot be reached here. Alternatively, if the intent is only to bound the publish, adjust the title, the comments and the #4561 closure so they do not claim a forced cleanup that does not happen.

IvanHunters Reproduced: with WaitDelay at 0, CombinedOutput returned only after the orphaned child closed the pipe, so -f never ran. I dropped UnmountWithForce. The plugin now runs umount itself with WaitDelay set, then umount -f under its own timeout, and the outer 30s select stays for an umount that cannot be killed. TestUnmountWithinForcesWhenThePlainUmountHangsWithAnOrphan covers it and fails if WaitDelay is removed.

[MINOR] The operations before the unmount are not bounded by this budget

rootOpts has no soft/timeo, so the temp mount is a hard mount. Mount, MkdirAll, ReadDir, Stat and Rename on it all block with no deadline when the server is gone, so a server that disappears earlier in the publish still hangs on those, not on the unmount. You already track this as #4581. This comment ("Leaves room inside kubeletCSIOperationTimeout for the mounts and the migration") reads as if the 30s budget covers those operations; it only covers the unmount. Worth rewording so the guarantee is not overstated.

The comment overstated it. It now says the budget covers the unmount only, and that the hard mount still blocks the mount and the migration, which is #4581.

[MINOR] A late unmount result is swallowed

After the deadline fires, the worker's unmount err goes to the buffered done channel that nobody reads anymore, so a late failure or a late success is never logged; only the os.Remove failure is. The operator sees unmount of ... still running after 30s and cannot tell a mount that eventually cleaned up from one that leaked. Log the unmount result inside the worker for the case where it arrives after the caller has already returned.

Fixed. Once the caller has stopped waiting, the worker logs the late result itself, success or failure with the error. TestUnmountWithinLogsALateResult checks both.

[MINOR] nosharecache has a cache-coherence side effect worth a comment

The superblock isolation is correct and is the right safety measure: with nosharecache the temp mount gets its own nfs_server, so umount -f (nfs_umount_begin calling rpc_killall_tasks) does not touch the pods' mounts of the same export. But nosharecache is the legacy separate-cache mode: a Rename into /data done through the temp superblock may not be visible to pods already holding /data on a different superblock until acdirmax (~60s). Impact is low (the migration is a one-time backfill of legacy data), but it is a behaviour change from the shared-cache default and deserves a line in the comment.

Added a line to the comment: pods that already hold /data may not see what the migration moves in until their attribute cache expires.

[NIT] Force-path fake does not model the unmount, so the test cannot assert cleanup

forceFakeMounter.UnmountWithForce does not delegate to FakeMounter.Unmount, so on the force path the fake's mount table is never cleared and TestNodePublishVolumeNFSUnmountsTempMountWithForce prints a real Failed to remove temp dir ... directory not empty warning (the local /data subdir is still there). Production behaviour is correct; this is test fidelity only. Same as CodeRabbit's note. Separately, lines 54-56 assert forcedTimeout within (0, 2min) while the contract is budget/2 = 15s, so the assertion passes even at 119s.

The force-path fake is gone. Those tests now run the real mounter against a fake umount on PATH, check the call sequence and that the temp dir is removed, so the (0, 2min) assertion went away with it. The directory not empty warning in the publish test is expected: the fake mounts nothing, so /data ends up inside the temp dir itself.

[MINOR] the forced-unmount result at this line can be misclassified in both directions

runUmount classifies the outcome from err plus a substring match on out, and this reaches past the actual state of the mount in two opposite ways; both are bounded (a misleading log line and at worst a leaked empty temp dir, recovered on container restart) so this does not block, but the classification is the fragile part of an otherwise sound change.

False failure. This err != nil check runs only when ctx.Err() is nil. Per the os/exec contract (go doc os/exec Cmd.WaitDelay: "if pipes are closed due to WaitDelay ... and the command has otherwise exited with a successful status, Wait ... will return ErrWaitDelay instead of nil"), a umount -f that exits 0 while the forked umount.nfs helper lingers past umountPipeGrace (1s) holding the combined-output pipe makes CombinedOutput return ErrWaitDelay. out then lacks not mounted, so the branch returns an error for an unmount that actually succeeded, the worker skips os.Remove(dir), and the caller logs Failed to unmount falsely.

False success. The comment above the line asserts umount2 detaches the mount before it blocks, so the -f after a killed plain umount finds not mounted and the line treats that as done. If that kernel assumption does not hold on the real path this PR targets (a vanished NFS server), the -f can report not mounted while the forced teardown was a no-op, and the line reads the no-op as success. Neither pass could reproduce this against a real dead server (hermetic boundary), so it is called out, not graded higher.

Suggestion: classify from the typed error rather than the output string: treat errors.Is(err, exec.ErrWaitDelay) (empty or not mounted output) as success, and do not let a not mounted from the -f retry stand in for a confirmed teardown without the plain-umount DeadlineExceeded that is supposed to precede it.