Fall back to the mount table when NodeUnpublishVolume cannot stat the target path

CHANGELOG.md

@@ -30,6 +30,7 @@
 - **Kubernetes:** Improved slow Qtree provisioning on large SVMs (Issue [#1142](https://github.com/NetApp/trident/issues/1142)).
 - **Kubernetes:** Fixed ControllerPublish to use volume config file system type to avoid using incorrect default file system type.
 - **Kubernetes:** Fixed an issue where orphan dm devices might cause repeated CSI NodeUnstageVolume failures.
+- **Kubernetes:** Fixed NodeUnpublishVolume failing indefinitely when the target path could not be inspected, such as an NFS mount whose export policy no longer admits the node (Issue [#1184](https://github.com/NetApp/trident/issues/1184)).
 
 **Enhancements:**
 

core/node/unmount.go

@@ -69,7 +69,13 @@ func (c *Core) unmountGeneric(ctx context.Context, volume, targetPath string) er
 				"target path (%s) not found; volume is not mounted.", targetPath)
 			return nil
 		}
-		return errors.InternalError("could not check if the target path (%s) is a directory; %v", targetPath, err)
+		// stat can fail on a path that is still mounted: EACCES when an NFS export rule no longer
+		// admits this node, ESTALE or EIO when the share has gone stale. None of these prevent
+		// umount(2). Consult the mount table, which does not touch the filesystem behind the
+		// target path, instead of failing the unpublish before it is attempted.
+		Logc(ctx).WithFields(LogFields{"targetPath": targetPath, "error": err}).Warning(
+			"Could not stat target path; checking the mount table instead.")
+		isDir = false
 	}
 
 	var notMountPoint bool

core/node/unmount_test.go

@@ -45,7 +45,7 @@ func TestUnmount_AcquiresVolumeLock_SerializesConcurrentCalls(t *testing.T) {
 
 	core.volumeLocks.Lock(volume)
 
-	mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, errors.New("stat failed"))
+	mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, os.ErrNotExist)
 
 	done := make(chan error, 1)
 	go func() { done <- core.Unmount(context.Background(), volume, UnmountRequest{TargetPath: "/target"}) }()
@@ -60,8 +60,7 @@ func TestUnmount_AcquiresVolumeLock_SerializesConcurrentCalls(t *testing.T) {
 
 	select {
 	case err := <-done:
-		require.Error(t, err)
-		assert.Contains(t, err.Error(), "stat failed")
+		require.NoError(t, err)
 	case <-time.After(2 * time.Second):
 		t.Fatal("Unmount did not proceed after volume lock was released")
 	}
@@ -88,15 +87,45 @@ func TestUnmountGeneric_TargetPathGone_ReturnsNilSuccess(t *testing.T) {
 	assert.NoError(t, err)
 }
 
-func TestUnmountGeneric_IsLikelyDirOtherError_Wrapped(t *testing.T) {
+func TestUnmountGeneric_StatError_Mounted_FallsBackToMountTableAndUnmounts(t *testing.T) {
 	core, mocks := newTestCore(t)
 
-	mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, errors.New("permission denied"))
+	mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, os.ErrPermission)
+	mocks.Mount.EXPECT().IsMounted(gomock.Any(), "", "/target", "").Return(true, nil)
+	// IsLikelyNotMountPoint must not be called: it would stat the path again.
+	mocks.Mount.EXPECT().Umount(gomock.Any(), "/target").Return(nil)
+	mocks.OsUtils.EXPECT().DeleteResourceAtPath(gomock.Any(), "/target").Return(nil)
+	mocks.NodeHelper.EXPECT().RemovePublishedPath(gomock.Any(), "vol1", "/target").Return(nil)
+
+	err := core.unmountGeneric(context.Background(), "vol1", "/target")
+	assert.NoError(t, err)
+}
+
+func TestUnmountGeneric_StatError_NotMounted_SkipsUmount(t *testing.T) {
+	core, mocks := newTestCore(t)
+
+	mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, os.ErrPermission)
+	mocks.Mount.EXPECT().IsMounted(gomock.Any(), "", "/target", "").Return(false, nil)
+	// IsLikelyNotMountPoint must not be called: it would stat the path again.
+	// No Umount call expected.
+	mocks.OsUtils.EXPECT().DeleteResourceAtPath(gomock.Any(), "/target").Return(nil)
+	mocks.NodeHelper.EXPECT().RemovePublishedPath(gomock.Any(), "vol1", "/target").Return(nil)
+
+	err := core.unmountGeneric(context.Background(), "vol1", "/target")
+	assert.NoError(t, err)
+}
+
+func TestUnmountGeneric_StatError_MountTableError_Wrapped(t *testing.T) {
+	core, mocks := newTestCore(t)
+
+	mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, os.ErrPermission)
+	// IsLikelyNotMountPoint must not be called: it would stat the path again.
+	mocks.Mount.EXPECT().IsMounted(gomock.Any(), "", "/target", "").Return(false, errors.New("boom"))
 
 	err := core.unmountGeneric(context.Background(), "vol1", "/target")
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "could not check if the target path")
-	assert.Contains(t, err.Error(), "permission denied")
+	assert.Contains(t, err.Error(), "unable to check if targetPath")
+	assert.Contains(t, err.Error(), "boom")
 }
 
 func TestUnmountGeneric_IsDir_UsesIsLikelyNotMountPoint(t *testing.T) {