fix(node): prevent unmount hangs and volumeLock deadlocks with cascading fallback by tyuchn · Pull Request #617 · GoogleCloudPlatform/lustre-csi-driver

tyuchn · 2026-08-12T18:41:25Z

Problem Statement

During pod teardown or volume unstage/unpublish operations (NodeUnstageVolume, NodeUnpublishVolume), if the Lustre filesystem is experiencing network partitions, dropped RPCs, or unresponsive backend targets (MDS/OSTs), the synchronous sys_umount syscall (ll_put_super) blocks in uninterruptible kernel sleep (TASK_UNINTERRUPTIBLE / D state) while attempting to flush dirty pages (cl_sync_io_wait), cancel locks (ldlm_cli_cancel), or send disconnect RPCs (obd_disconnect).

Because s.mounter.Unmount(target) internally executed exec.Command("umount", target) without a timeout or context deadline:

The gRPC handler goroutine blocked indefinitely inside the kernel syscall.
defer s.volumeLocks.Release(target) was never reached.
Kubelet timed out the gRPC call after 2 minutes and retried, but every subsequent retry immediately failed with codes.Aborted: An operation with the given volume key <target> already exists.
The volume and node remained permanently wedged until the CSI driver pod was manually restarted.

Design & Solution Reasoning

1. Three-Tier Cascading Unmount Fallback (`pkg/csi_driver/unmount.go`)

We introduce unmountPath(ctx, target) which applies a bounded, cascading fallback strategy:

Phase 1 (Standard Unmount): Runs exec.CommandContext(ctx, "umount", target) with a 15-second bounded timeout (DefaultUnmountTimeout).
Phase 2 (Force Unmount): If Phase 1 times out or fails with an error, escalates to exec.CommandContext(ctx, "umount", "-f", target) with a 10-second bounded timeout (DefaultForceUnmountTimeout).
Phase 3 (Lazy Unmount / MNT_DETACH): If Phase 2 also times out or fails, falls back to exec.CommandContext(ctx, "umount", "-l", target) with a 5-second bounded timeout (DefaultLazyUnmountTimeout).
Directory Cleanup: Once detached from the VFS namespace, the staging/target directory is removed.

Why umount -l (Lazy Unmount) is necessary:
When a Lustre MDS or OST is completely unreachable or deadlocked in kernel space, even umount -f can fail with transport errors or block. Lazy unmount (MNT_DETACH) detaches the mount point from the VFS namespace immediately. This enables Kubelet to clean up the mount directory and finalize volume teardown, while the Linux kernel finishes releasing Lustre resources asynchronously in the background once communication recovers.

2. Guaranteed Lock Release & Subprocess Cleanup

Using exec.CommandContext guarantees that if a timeout occurs, Go sends SIGKILL to the umount process rather than abandoning an orphaned goroutine.
The total worst-case unmount duration across all 3 phases is strictly bounded to $\le 30$ seconds (well below Kubelet's 120s RPC timeout).
Every code path is guaranteed to return, ensuring defer s.volumeLocks.Release(target) always executes and freeing the volume lock for future attempts.

3. gRPC Context Propagation (`pkg/csi_driver/node.go`)

Updated NodeStageVolume, NodeUnstageVolume, NodePublishVolume, and NodeUnpublishVolume to accept and respect ctx context.Context.
Propagated context to all cleanup error paths and IAM Workload Identity teardown routines (cleanUpIAMReference, cleanUpIAMReferenceForKey).

Upgrade Safety

No Impact on Active Workloads: Workload mounts exist in the host Linux kernel mount namespace via mountPropagation: Bidirectional. Upgrading the CSI driver DaemonSet does not unmount active volumes or interrupt in-flight I/O.
Kernel Modules Preserved: lustre-kmod-installer detects that kernel modules are already loaded and exits immediately without reloading.
IAM Reference State Preserved: IAM references and tokens are stored on hostPath storage (/var/lib/lustre/mounts/...) and survive pod restarts.

Verification

Added comprehensive unit tests in pkg/csi_driver/unmount_test.go:
- Phase 1 standard unmount success.
- Phase 2 force unmount fallback on timeout/failure.
- Phase 3 lazy unmount fallback on force unmount timeout/failure.
- Context cancellation and immediate volumeLocks release verification.
Verified test suite:
```
go test -race ./pkg/csi_driver/...
go test ./...
```
Results: All tests passing with 0 data races.

…ing fallback During pod teardown or volume unstage/unpublish operations (NodeUnstageVolume, NodeUnpublishVolume), if the Lustre filesystem experiences network partitions, dropped RPCs, or unresponsive backend targets (MDS/OSTs), the synchronous sys_umount syscall (ll_put_super) blocks in uninterruptible kernel sleep (D state) while attempting to flush dirty pages (cl_sync_io_wait), cancel locks (ldlm_cli_cancel), or send disconnect RPCs (obd_disconnect). Because s.mounter.Unmount(target) internally executed exec.Command("umount", target) without a timeout or context deadline, the gRPC handler goroutine blocked indefinitely, preventing defer s.volumeLocks.Release(target) from executing and causing all subsequent Kubelet retry attempts to fail with "VolumeOperationAlreadyExists". This change: 1. Implements a three-tier cascading unmount fallback in pkg/csi_driver/unmount.go (Phase 1 standard umount with 15s timeout -> Phase 2 force umount -f with 10s timeout -> Phase 3 lazy umount -l with 5s timeout). 2. Propagates ctx context.Context across NodeStageVolume, NodeUnstageVolume, NodePublishVolume, NodeUnpublishVolume, and IAM teardown paths. 3. Guarantees bounded execution (<= 30s) and deterministic volumeLock release. 4. Adds comprehensive unit tests for cascading fallback and lock release.

gemini-code-assist

Code Review

This pull request introduces a cascading unmount strategy for Lustre mounts in the CSI driver, falling back from standard to force and lazy unmounts, and adds corresponding unit tests. The code review feedback highlights several critical improvements: preventing indefinite hangs during uninterruptible sleep (D state) by implementing a non-blocking command execution, safely defaulting to unmounting when mount point checks return an error, and using context.Background() instead of the request context for cleanup operations to ensure they run even if the request context is cancelled or timed out.

…and cleanup contexts

tyuchn · 2026-08-12T18:50:47Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a cascading fallback unmount strategy (standard, force, and lazy unmounts) with bounded timeouts to prevent mount points from getting permanently wedged, along with comprehensive unit tests. The review feedback highlights a critical correctness issue where passing a cancellable context to the unmount phases can bypass the fallback mechanism entirely upon timeout or cancellation. It is recommended to use context.Background() for these timeout contexts and simplify the signatures of unmountPath and related helper functions by removing the unused context parameters. Additionally, a defensive check should be added to execUmount to prevent a potential out-of-bounds panic when arguments are empty.

…and simplify signatures

tyuchn · 2026-08-12T19:01:27Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a cascading unmount strategy in the CSI driver to handle standard, force, and lazy unmounts with bounded timeouts, preventing blocked operations. It adds a new unmount.go file with the unmountPath logic, along with corresponding unit tests, and refactors node.go to use this helper. The review feedback suggests using exec.CommandContext with cmd.Run() for idiomatic command execution, improving the robustness of the *mount.FakeMounter type assertion in tests, making the isNotMounted check case-insensitive, and passing error objects directly to log formatters instead of calling .Error() explicitly.

gemini-code-assist · 2026-08-12T19:04:37Z

+	cmd := exec.Command("umount", args...)
+	var buf bytes.Buffer
+	cmd.Stdout = &buf
+	cmd.Stderr = &buf
+
+	if err := cmd.Start(); err != nil {
+		return nil, err
+	}
+
+	done := make(chan error, 1)
+	go func() {
+		done <- cmd.Wait()
+	}()
+
+	select {
+	case err := <-done:
+		return buf.Bytes(), err
+	case <-ctx.Done():
+		if cmd.Process != nil {
+			_ = cmd.Process.Kill()
+		}
+		return nil, ctx.Err()
+	}


Using exec.CommandContext with cmd.Run() is the standard and idiomatic way to execute external commands with a context timeout in Go. It automatically handles process termination on context cancellation/timeout, eliminating the need for manual goroutine management, channel coordination, and process killing.

cmd := exec.CommandContext(ctx, "umount", args...) var buf bytes.Buffer cmd.Stdout = &buf cmd.Stderr = &buf if err := cmd.Run(); err != nil { if ctx.Err() != nil { return nil, ctx.Err() } return buf.Bytes(), err } return buf.Bytes(), nil

We intentionally use cmd.Start() with a background cmd.Wait() and select on ctx.Done() because cmd.Run() blocks in wait4 if the child umount process enters TASK_UNINTERRUPTIBLE (D) sleep in the kernel. Standard cmd.Run() cannot unblock or return until the kernel D-state wakes up, whereas selecting on ctx.Done() allows the CSI driver to return immediately on timeout.

gemini-code-assist · 2026-08-12T19:04:37Z

+	}
+
+	// In test environments using FakeMounter, delegate to FakeMounter.Unmount if no custom unmountExec is set.
+	if fm, ok := s.mounter.(*mount.FakeMounter); ok {


This type assertion only succeeds if s.mounter is exactly of type *mount.FakeMounter. If the test environment wraps or embeds *mount.FakeMounter (such as fakeMounter defined in node_test.go), this check will fail, causing the code to fall back to executing the real umount command on the host. To make this robust and support wrapped mounters in tests without referencing test-only types in production code, consider checking if the mounter implements an unwrapper interface or using reflection to extract the embedded *mount.FakeMounter.

Resolved in commit 1bb96db. Added extractFakeMounter which uses reflection to inspect structs embedding *mount.FakeMounter.

gemini-code-assist · 2026-08-12T19:04:37Z

+func isNotMounted(err error, output []byte) bool {
+	if err != nil && strings.Contains(err.Error(), errNotMounted) {
+		return true
+	}
+	if strings.Contains(string(output), errNotMounted) {
+		return true
+	}
+	return false
+}


To make the isNotMounted check more robust against potential variations in error messages across different OS or kernel versions, consider performing a case-insensitive comparison by converting the error and output strings to lowercase before checking for errNotMounted.

Suggested change

func isNotMounted(err error, output []byte) bool {

if err != nil && strings.Contains(err.Error(), errNotMounted) {

return true

}

if strings.Contains(string(output), errNotMounted) {

return true

}

return false

}

func isNotMounted(err error, output []byte) bool {

if err != nil && strings.Contains(strings.ToLower(err.Error()), errNotMounted) {

return true

}

if strings.Contains(strings.ToLower(string(output)), errNotMounted) {

return true

}

return false

}

Resolved in commit 1bb96db. Made isNotMounted check case-insensitive using strings.ToLower.

gemini-code-assist · 2026-08-12T19:04:37Z

 		klog.Errorf("Mount %q failed on node %s, cleaning up", target, nodeName)
-		if unmntErr := mount.CleanupMountPoint(target, s.mounter, false /* extensiveMountPointCheck */); unmntErr != nil {
+		if unmntErr := s.unmountPath(target); unmntErr != nil {
 			klog.Errorf("Unmount %q failed on node %s: %v", target, nodeName, unmntErr.Error())


Passing unmntErr directly to %v is more idiomatic in Go and avoids calling .Error() explicitly. This is also consistent with how errors are logged elsewhere in this file (e.g., line 410).

Suggested change

klog.Errorf("Unmount %q failed on node %s: %v", target, nodeName, unmntErr.Error())

klog.Errorf("Unmount %q failed on node %s: %v", target, nodeName, unmntErr)

Resolved in commit 1bb96db. Passed unmntErr directly to %v formatter.

gemini-code-assist · 2026-08-12T19:04:37Z

 		klog.V(5).Infof("setVolumeOwnershipTopLevel failed for volume %q, path %q, fsGroup %q, cleaning up mount point on node %s", volumeID, targetPath, fsGroup, nodeName)
-		if unmntErr := mount.CleanupMountPoint(targetPath, s.mounter, false /* extensiveMountPointCheck */); unmntErr != nil {
+		if unmntErr := s.unmountPath(targetPath); unmntErr != nil {
 			klog.Errorf("Unmount %q failed on node %s: %v", targetPath, nodeName, unmntErr.Error())


Passing unmntErr directly to %v is more idiomatic in Go and avoids calling .Error() explicitly. This is also consistent with how errors are logged elsewhere in this file (e.g., line 410).

Suggested change

klog.Errorf("Unmount %q failed on node %s: %v", targetPath, nodeName, unmntErr.Error())

klog.Errorf("Unmount %q failed on node %s: %v", targetPath, nodeName, unmntErr)

Resolved in commit 1bb96db. Passed unmntErr directly to %v formatter.

gemini-code-assist · 2026-08-12T19:04:37Z

 		klog.Errorf("Mount %q failed on node %s for principal %s, cleaning up", globalMountPath, nodeName, principal)
-		if unmntErr := mount.CleanupMountPoint(globalMountPath, s.mounter, false /* extensiveMountPointCheck */); unmntErr != nil {
+		if unmntErr := s.unmountPath(globalMountPath); unmntErr != nil {
 			klog.Errorf("Unmount %q failed on node %s for principal %s: %v", globalMountPath, nodeName, principal, unmntErr.Error())


Passing unmntErr directly to %v is more idiomatic in Go and avoids calling .Error() explicitly. This is also consistent with how errors are logged elsewhere in this file (e.g., line 410).

Suggested change

klog.Errorf("Unmount %q failed on node %s for principal %s: %v", globalMountPath, nodeName, principal, unmntErr.Error())

klog.Errorf("Unmount %q failed on node %s for principal %s: %v", globalMountPath, nodeName, principal, unmntErr)

Resolved in commit 1bb96db. Passed unmntErr directly to %v formatter.

… case insensitivity, and log formatting

tyuchn · 2026-08-12T19:27:09Z

Closing in favor of a clean, squashed PR for review.

gemini-code-assist Bot reviewed Aug 12, 2026

View reviewed changes

fix: address review comments on non-blocking exec, unmount fallback, …

d14f8ed

…and cleanup contexts

gemini-code-assist Bot reviewed Aug 12, 2026

View reviewed changes

tyuchn added 2 commits August 12, 2026 11:57

fix: make cascading unmount resilient to parent context cancellation …

e45a237

…and simplify signatures

docs: explain context.Background rationale in unmountPath

5437444

gemini-code-assist Bot reviewed Aug 12, 2026

View reviewed changes

fix: address review comments on fake mounter extraction, isNotMounted…

1bb96db

… case insensitivity, and log formatting

tyuchn closed this Aug 12, 2026

	klog.Errorf("Unmount %q failed on node %s: %v", target, nodeName, unmntErr.Error())
	klog.Errorf("Unmount %q failed on node %s: %v", target, nodeName, unmntErr)

Conversation

tyuchn commented Aug 12, 2026

Problem Statement

Design & Solution Reasoning

1. Three-Tier Cascading Unmount Fallback (pkg/csi_driver/unmount.go)

2. Guaranteed Lock Release & Subprocess Cleanup

3. gRPC Context Propagation (pkg/csi_driver/node.go)

Upgrade Safety

Verification

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tyuchn commented Aug 12, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tyuchn commented Aug 12, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

tyuchn Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

tyuchn Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

tyuchn Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

tyuchn Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

tyuchn Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

tyuchn Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

tyuchn commented Aug 12, 2026

1. Three-Tier Cascading Unmount Fallback (`pkg/csi_driver/unmount.go`)

3. gRPC Context Propagation (`pkg/csi_driver/node.go`)